[Accelerator] Fix Python typing in accelerator #152394
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/152394
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 daac118 with merge base 7243c69 ( FLAKY - The following job failed but was likely due to flakiness present on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
@pytorchbot label "topic: not user facing" |
|
@pytorchbot label "topic: not user facing" |
|
@pytorchmergebot merge |
Merge failedReason: Approvers from one of the following sets are needed:
|
torch/types.py
Outdated
| # None means use the default device (typically CPU) | ||
| Device: TypeAlias = Union[_device, str, int, None] | ||
| OptionalDevice: TypeAlias = Union[ExplicitDevice, None] | ||
| Device: TypeAlias = OptionalDevice |
There was a problem hiding this comment.
Device is not optional usually !
There was a problem hiding this comment.
I can't think of a better type name.. Any advice?
There was a problem hiding this comment.
I think we can deprecate Device and add a new DeviceLike type.
There was a problem hiding this comment.
Isn't the current "Device" type used heavily today? I would not expect we can change it !
There was a problem hiding this comment.
DeviceLike makes less sense because str and int are treated as device in device APIs for many years. Is there any API where Device is allowed but str and int aren't?
torch/types.py
Outdated
| @@ -70,8 +70,10 @@ | |||
|
|
|||
| # Meta-type for "device-like" things. Not to be confused with 'device' (a | |||
| # literal device object). This nomenclature is consistent with PythonArgParser. | |||
| ExplicitDevice: TypeAlias = Union[_device, str, int] | |||
There was a problem hiding this comment.
What is the benefit of having this "ExplicitDevice" and "Device" if they're the same thing?
There was a problem hiding this comment.
Device can be None, which is cpu. But for accelerator APIs, their devices are accelerator cards (CUDA,XPU,...).
ExplicitDevice means that users can't pass None (CPU) to these APIs, that is meaningless..
There was a problem hiding this comment.
Device can be None, which is cpu.
I don't understand what you mean by that? Where did you read that?
Generally, when passing an optional device around, empty means the current default device not None.
In current_accelerator() API, we use None when no accelerator is available. But that's very unique to only that one function.
There was a problem hiding this comment.
Sorry, misunderstanding..
There was a problem hiding this comment.
ExplicitDevice is used in only one place set_device_index, here the device shouldn't be None. Otherwise, it becomes "set device to the current device".
|
@albanD OptionalDevice removed |
Co-authored-by: Xuehai Pan <XuehaiPan@outlook.com>
Co-authored-by: Xuehai Pan <XuehaiPan@outlook.com>
Co-authored-by: Yu, Guangye <106960996+guangyey@users.noreply.github.com>
|
@pytorchbot merge |
|
This PR has pending changes requested. Please address the comments and update the PR before merging. |
The new device type has been reverted because there is no suitable name for it.
|
@pytorchbot merge |
Merge failedReason: Approvers from one of the following sets are needed:
|
There are some changes:
__exit__ofdevice_indexis changed to return None.