-
Notifications
You must be signed in to change notification settings - Fork 53
The spec of to_device()
method is missing
#256
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
Comments
I think the signature should be as follows:
where |
Good catch, that is indeed missing.
So you don't want it to be positional-only? That's possible, because it's a new method name so every library can choose the same name. However |
Hi @rgommers
Ah yes, I meant it to be position-only, but I typed the signature wrong. It should read to_device(self, device, /, *, stream=None) |
Thanks, Athan, I just did! |
Actually, let me ask a question about |
That's a good point, Ralf. Thanks for brining it up. How about we make it a union, i.e. we accept both the DLPack-like integer and library-specific stream objects? |
That makes sense to me. The "integer to stream" logic must be implemented anyway to support DLPack, so is not much of an extra burden. The actual union then becomes It would also be good to write up here in a few sentences how often stream support in |
Technically a Examplesfrom __future__ import annotation
from typing import Union, Protocol, Any, TypeVar
_Stream = Any
_T_contra = TypeVar("_T_contra", bound=Union[None, int, _Stream], contravariant=True)
# Concrete implementations must support all union-members
class SupportsToDevice1(Protocol):
to_device(self, /, device, *, stream: int | None | _Stream = None): ...
# Concrete implementations must support one or more of the typevar bounds
class SupportsToDevice2(Protocol[_T_contra]):
to_device(self, /, device, *, stream: None | _T_contra = None): ... |
Yes I think so too, so PyTorch's So what I have in mind is taking one step back. We can say "if |
This makes perfect sense. It doesn't belong in the API standard docs though I think, that'd be pretty confusing to the reader. Maybe the right approach is to avoid using For a library implementing this, the protocol approach makes sense. Right now we don't have a good place to put these type annotations though.
This sounds good. |
Here we mentioned there's a
to_device()
method to enable data copy/movement across devices:array-api/spec/design_topics/device_support.md
Lines 48 to 49 in 0941067
But we haven't defined the signature and semantics yet.
The text was updated successfully, but these errors were encountered: