Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor c10::DataPtr by subclassing from c10::detail::UniqueVoidPtr #128669

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cyyever
Copy link
Collaborator

@cyyever cyyever commented Jun 14, 2024

That seems more reasonable and saves lots of delegated code.

Copy link

pytorch-bot bot commented Jun 14, 2024

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/128669

Note: Links to docs will display an error until the docs builds have been completed.

✅ You can merge normally! (1 Unrelated Failure)

As of commit b39b695 with merge base bf60963 (image):

BROKEN TRUNK - The following job failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@cyyever cyyever changed the title Make c10::detail::UniqueVoidPtr parent of c10::DataPtr Refactor c10::DataPtr by subclassing from c10::detail::UniqueVoidPtr Jun 14, 2024
@mikaylagawarecki mikaylagawarecki added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Jun 17, 2024
@cyyever cyyever requested a review from malfet June 21, 2024 07:07
@cyyever cyyever requested a review from ezyang July 4, 2024 06:59
@cyyever
Copy link
Collaborator Author

cyyever commented Jul 4, 2024

@pytorchbot rebase -b main

@pytorchmergebot
Copy link
Collaborator

@pytorchbot started a rebase job onto refs/remotes/origin/main. Check the current status here

@pytorchmergebot
Copy link
Collaborator

Successfully rebased DataPtr onto refs/remotes/origin/main, please pull locally before adding more changes (for example, via git checkout DataPtr && git pull --rebase)

@ezyang
Copy link
Contributor

ezyang commented Jul 4, 2024

Meh. If you are going to do this at minimum it should be private inheritance so you don't open up the possibility of slicing

@cyyever
Copy link
Collaborator Author

cyyever commented Jul 4, 2024

Using private inheritance means it still requires lots of boilerplate code to forward method calls, the same as before.

@ezyang
Copy link
Contributor

ezyang commented Jul 5, 2024

But public inheritance means that UniqueVoidPtr is convertible from DataPtr, which is undesirable.

@cyyever
Copy link
Collaborator Author

cyyever commented Jul 6, 2024

Convert UniqueVoidPtr to DataPtr is not allowed, but the DataPtr to UniqueVoidPtr is allowed. In essence, DataPtr is just UniqueVoidPtr + device and all methods except constructors are defined in UniqueVoidPtr.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants