Improve the functionality of untyped storage for privateuse1. #100868

wbigat · 2023-05-08T12:02:30Z

Complete the implementation of the interface is_pinned() of untyped storage class for privateuse1.
And refactor the implementation in typed storage by untyped_storage.is_pinned().

Hi, @ezyang
This is another improvement of untyped storage for privateuse1, can you take a moment to review it? Thanks.

pytorch-bot · 2023-05-08T12:02:33Z

🔗 Helpful Links

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

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit the bot commands wiki or our office hours

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

✅ No Failures

As of commit 88a7612:
💚 Looks good so far! There are no failures yet. 💚

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

ezyang · 2023-05-08T22:32:36Z

@kurtamohler do you think you can take this one?

wbigat · 2023-05-09T01:38:24Z

Hi, since the TypedStorage API will be deprecated in the future, I'm trying to implement is_pined() in untyped storage to support other backends. The implementation of is_pined() in typed storage is only to invoke the interface in untyped storage.

The default value of the ‘device’ parameter is ‘cuda’. Therefore, users of the CUDA backend do not need to modify the usage of this interface. This parameter only needs to be set when other backends support the construction of pin_memory tensors.

I sincerely look forward to your discussion and comments, @kurtamohler @ezyang

torch/storage.py

torch/csrc/StorageMethods.cpp

kurtamohler · 2023-05-09T19:40:04Z

I think we should probably add a property function like Tensor/UntypedStorage.pinned_device. That can be in a different PR though

wbigat · 2023-05-10T13:39:52Z

I think we should probably add a property function like Tensor/UntypedStorage.pinned_device. That can be in a different PR though

Hi, @kurtamohler
I am trying to understand this idea. Do you mean that we need to add an attribute to record the pinned device and a new function to obtain this attribute, like Tensor/UntypedStorage.pinned_device() ?

kurtamohler · 2023-05-10T17:33:34Z

Do you mean that we need to add an attribute to record the pinned device and a new function to obtain this attribute, like Tensor/UntypedStorage.pinned_device() ?

Yeah that's what I'm suggesting. Although we could make it a readonly property to avoid needing the parentheses, so that it's semantically similar to `Tensor.device'

torch/storage.py

wbigat · 2023-05-11T15:56:45Z

Yeah, in this latest PR, both string and torch.device types are supported as input of is_pinned and pin_memory. And in the implementation of pin_memory, we always create the tensor without any conditions as you suggested before.
@kurtamohler, please review this change.

wbigat · 2023-05-13T00:25:33Z

Hi, @kurtamohler @ezyang
Dose this change be finished as expected? Would you please take a moment to review this pr, thanks a lot.

wbigat · 2023-05-18T00:55:29Z

@pytorchbot merge

github-actions · 2023-05-18T00:55:57Z

This PR needs a label

If your changes are user facing and intended to be a part of release notes, please use a label starting with release notes:.

If not, please add the topic: not user facing label.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "topic: not user facing"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

pytorchmergebot · 2023-05-18T00:57:27Z

Merge failed

Reason: Approval needed from one of the following:
EscapeZero, 842974287, miguelmartin75, kauterry, priyaramani, ...

Details for Dev Infra team

Raised by workflow job

Failing merge rule: Core Maintainers

wbigat · 2023-05-18T01:10:17Z

@pytorchbot label "release notes: python_frontend"

wbigat · 2023-05-18T12:57:36Z

Hi, @kurtamohler @ezyang
I'm so sorry to bother you. I found that kurtamohler has approved these changes, but I'm not sure if that means this pr can be appoveed? If so, what else do I need to do to merge in this PR? Thanks a lot.

kurtamohler · 2023-05-18T21:35:16Z

I'm not on the list of people whose approval allows a PR to be merged. When Edward has time, his approval will make it possible to merge

ykddd · 2023-05-19T01:55:28Z

@pytorchbot merge -r

pytorchmergebot · 2023-05-19T01:57:18Z

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

…edStorage.is_pinned

pytorchmergebot · 2023-05-19T01:57:24Z

Successfully rebased Master-storage-privateuse1 onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via git checkout Master-storage-privateuse1 && git pull --rebase)

github-actions · 2023-05-19T01:58:03Z

This PR needs a `release notes:` label

If your changes are user facing and intended to be a part of release notes, please use a label starting with release notes:.

If not, please add the topic: not user facing label.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "topic: not user facing"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

pytorchmergebot · 2023-05-19T01:58:30Z

Merge failed

Reason: Approval needed from one of the following:
laurencer, miguelmartin75, serhaty, jubinchheda, qxy11, ...

Details for Dev Infra team

Raised by workflow job

Failing merge rule: Core Maintainers

github-actions · 2023-05-19T01:59:36Z

This PR needs a `release notes:` label

If your changes are user facing and intended to be a part of release notes, please use a label starting with release notes:.

If not, please add the topic: not user facing label.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "topic: not user facing"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

wbigat · 2023-05-19T02:04:27Z

@pytorchbot label "release notes: python_frontend"

ezyang · 2023-05-19T03:21:13Z

@pytorchbot merge

pytorchmergebot · 2023-05-19T03:23:14Z

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

wbigat changed the title ~~Master storage privateuse1~~ Improve the functionality of untyped storage for privateuse1. May 8, 2023

pytorchbot added the open source label May 8, 2023

wbigat force-pushed the Master-storage-privateuse1 branch from f3ee099 to fc9f805 Compare May 8, 2023 13:08

ezyang requested a review from kurtamohler May 8, 2023 22:32

kurtamohler reviewed May 9, 2023

View reviewed changes

torch/storage.py Outdated Show resolved Hide resolved

kurtamohler reviewed May 9, 2023

View reviewed changes

torch/csrc/StorageMethods.cpp Outdated Show resolved Hide resolved

ezyang added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label May 10, 2023

wbigat force-pushed the Master-storage-privateuse1 branch 2 times, most recently from 7881ab1 to aa04df4 Compare May 10, 2023 08:46

kurtamohler reviewed May 10, 2023

View reviewed changes

torch/storage.py Outdated Show resolved Hide resolved

kurtamohler reviewed May 10, 2023

View reviewed changes

torch/storage.py Outdated Show resolved Hide resolved

kurtamohler reviewed May 10, 2023

View reviewed changes

torch/storage.py Outdated Show resolved Hide resolved

wbigat force-pushed the Master-storage-privateuse1 branch from a5bf3fe to ea5b67f Compare May 11, 2023 15:18

wbigat force-pushed the Master-storage-privateuse1 branch from 22d4d3d to d61f3d5 Compare May 17, 2023 06:48

kurtamohler approved these changes May 17, 2023

View reviewed changes

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label May 18, 2023

pytorchmergebot added the merging label May 18, 2023

pytorchmergebot removed the merging label May 18, 2023

wbigat requested a review from kurtamohler May 18, 2023 01:15

wbigat added 4 commits May 19, 2023 01:57

[feature] Improve the functionality of untyped storage for privateuse1.

a54c09b

move the implement of is_pinned from C++ to python

4559968

is_pinned support both str and torch.device as input arg

33bc17b

Throw error for invalid device, rather than returning False for Untyp…

88a7612

…edStorage.is_pinned

pytorchmergebot force-pushed the Master-storage-privateuse1 branch from d992418 to 88a7612 Compare May 19, 2023 01:57

pytorchmergebot added the merging label May 19, 2023

pytorchmergebot removed the merging label May 19, 2023

pytorch-bot bot added the release notes: python_frontend python frontend release notes category label May 19, 2023

ezyang approved these changes May 19, 2023

View reviewed changes

pytorchmergebot added the merging label May 19, 2023

pytorchmergebot added Merged and removed merging labels May 19, 2023

pytorchmergebot closed this in 4486a1d May 19, 2023

Improve the functionality of untyped storage for privateuse1. #100868

Improve the functionality of untyped storage for privateuse1. #100868

Uh oh!

Conversation

wbigat commented May 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented May 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

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

✅ No Failures

Uh oh!

ezyang commented May 8, 2023

Uh oh!

wbigat commented May 9, 2023

Uh oh!

Uh oh!

Uh oh!

kurtamohler commented May 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wbigat commented May 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kurtamohler commented May 10, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wbigat commented May 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wbigat commented May 13, 2023

Uh oh!

wbigat commented May 18, 2023

Uh oh!

github-actions bot commented May 18, 2023

This PR needs a label

Uh oh!

pytorchmergebot commented May 18, 2023

Merge failed

Uh oh!

wbigat commented May 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wbigat commented May 18, 2023

Uh oh!

kurtamohler commented May 18, 2023

Uh oh!

ykddd commented May 19, 2023

Uh oh!

pytorchmergebot commented May 19, 2023

Uh oh!

pytorchmergebot commented May 19, 2023

Uh oh!

github-actions bot commented May 19, 2023

This PR needs a release notes: label

Uh oh!

pytorchmergebot commented May 19, 2023

Merge failed

Uh oh!

github-actions bot commented May 19, 2023

This PR needs a release notes: label

Uh oh!

wbigat commented May 19, 2023

Uh oh!

ezyang commented May 19, 2023

Uh oh!

pytorchmergebot commented May 19, 2023

Merge started

Uh oh!

Uh oh!

wbigat commented May 8, 2023 •

edited

Loading

pytorch-bot bot commented May 8, 2023 •

edited

Loading

kurtamohler commented May 9, 2023 •

edited

Loading

wbigat commented May 10, 2023 •

edited

Loading

wbigat commented May 11, 2023 •

edited

Loading

wbigat commented May 18, 2023 •

edited

Loading

This PR needs a `release notes:` label

This PR needs a `release notes:` label