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

Address a few device related issues #259

Merged
merged 10 commits into from
Nov 1, 2021
Merged

Address a few device related issues #259

merged 10 commits into from
Nov 1, 2021

Conversation

leofang
Copy link
Contributor

@leofang leofang commented Sep 13, 2021

Close #256. Close #156.

This PR addresses a few things:

  • Add stream to .to_device() and allow any library-specific stream object to be used
  • Clarify that .to_device() is to copy (not move) data across devices
  • Clarify that the standard is not concerned with (a)synchronous transfer
  • Clarify that any library-specific device management is permitted

@leofang
Copy link
Contributor Author

leofang commented Sep 13, 2021

Let's merge #171 first.

@leofang leofang marked this pull request as ready for review September 14, 2021 05:22

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@leofang leofang marked this pull request as draft September 14, 2021 05:26
@rgommers rgommers added the Narrative Content Narrative documentation content. label Sep 14, 2021
@kgryte kgryte added this to the v2021 milestone Oct 4, 2021
@kgryte
Copy link
Contributor

kgryte commented Oct 4, 2021

@leofang Following up here: are there things related to this PR that we should discuss and resolve during the next consortium meeting?

@leofang leofang marked this pull request as ready for review October 8, 2021 21:33
@leofang
Copy link
Contributor Author

leofang commented Oct 8, 2021

PTAL!

@leofang leofang changed the title [WIP] Address a few device related issues Address a few device related issues Oct 8, 2021
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Leo. I made some suggestions to try and avoid the average reader to keep wondering what the message here. We are building in an escape hatch here for libraries by allowing arbitrary device and stream objects; the downside of that is that the code then becomes non-portable, and it should be clear that there's no obligation on implementers to provide or support such objects.

Copy link
Contributor

@kgryte kgryte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small textual changes, some of which should probably be reviewed to ensure that the original intent is preserved. Some of the textual changes are intended to mirror language used elsewhere in this specification.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-authored-by: Athan <kgryte@gmail.com>
Copy link
Contributor Author

@leofang leofang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @kgryte! The suggested changes look good and I've applied them. I left a quick question below.

My only whine at this point is without using a library-specific device object, there is no way to initialize an array on an arbitrary device, nor can I move it to any other device when it's created on the default device. Can someone remind me why we made this decision choice?

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Copy link
Contributor

@kgryte kgryte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Any additional changes can be made in follow-up PRs. Thanks, @leofang!

@kgryte kgryte merged commit 5ecbce4 into data-apis:main Nov 1, 2021
@leofang leofang deleted the device branch November 1, 2021 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Narrative Content Narrative documentation content.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The spec of to_device() method is missing Device object and device kwarg for creation
4 participants