Skip to content

Address a few device related issues#259

Merged
kgryte merged 10 commits into
data-apis:mainfrom
leofang:device
Nov 1, 2021
Merged

Address a few device related issues#259
kgryte merged 10 commits into
data-apis:mainfrom
leofang:device

Conversation

@leofang

@leofang leofang commented Sep 13, 2021

Copy link
Copy Markdown
Contributor

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

leofang commented Sep 13, 2021

Copy link
Copy Markdown
Contributor Author

Let's merge #171 first.

@leofang leofang marked this pull request as ready for review September 14, 2021 05:22
@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

kgryte commented Oct 4, 2021

Copy link
Copy Markdown
Contributor

@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

leofang commented Oct 8, 2021

Copy link
Copy Markdown
Contributor Author

PTAL!

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

@rgommers rgommers left a comment

Copy link
Copy Markdown
Member

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.

Comment thread spec/API_specification/array_object.md Outdated
Comment thread spec/API_specification/array_object.md Outdated
Comment thread spec/design_topics/device_support.md Outdated
Comment thread spec/API_specification/array_object.md

@kgryte kgryte left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread spec/API_specification/array_object.md Outdated
Comment thread spec/API_specification/array_object.md Outdated
Comment thread spec/API_specification/array_object.md Outdated
Comment thread spec/API_specification/array_object.md Outdated
Comment thread spec/design_topics/device_support.md Outdated
Comment thread spec/design_topics/device_support.md Outdated
Comment thread spec/design_topics/device_support.md Outdated
Comment thread spec/design_topics/device_support.md Outdated
Co-authored-by: Athan <kgryte@gmail.com>

@leofang leofang left a comment

Copy link
Copy Markdown
Contributor Author

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?

Comment thread spec/design_topics/device_support.md
Comment thread spec/API_specification/array_object.md Outdated
Comment thread spec/API_specification/array_object.md Outdated
Comment thread spec/API_specification/array_object.md Outdated
Comment thread spec/API_specification/array_object.md

@kgryte kgryte left a comment

Copy link
Copy Markdown
Contributor

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