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

Improve documentation for stream argument to __dlpack__ #185

Merged
merged 2 commits into from
May 26, 2021

Conversation

rgommers
Copy link
Member

This is getting pretty long for reference documentation of a method, we may need to split it out to a separate page soon. Especially given that there may be more to add soon:

For now this is the minimal update for things that are being discussed.

Cc @oleksandr-pavlyk, @tqchen, @leofang

Closes data-apisgh-183

The need to document the ownership of `stream` came up in
pytorch/pytorch#57781.
@rgommers rgommers added the Narrative Content Narrative documentation content. label May 24, 2021
@@ -434,7 +434,13 @@ Exports the array for consumption by {ref}`function-from_dlpack` as a DLPack cap

- **stream**: _Optional\[ int ]_
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- **stream**: _Optional\[ int ]_
- **stream**: _Any_

You could also go for Optional[Union[int, Any]] or Union[int, None, Any], but in the end all these statements are equivalent.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks Bas, I missed updating that. From a typing point of view they're indeed all the same. From a "this serves as documentation as well" point of view, I think I prefer Optional[Union[int, Any]].

@leofang
Copy link
Contributor

leofang commented May 25, 2021

This is getting pretty long for reference documentation of a method, we may need to split it out to a separate page soon.

I agree. By moving the new additions to a separate page (say the current "Data interchange mechanisms"), we can just add a new :::{admonition} block for SYCL to keep the docstring reasonably long.

@rgommers
Copy link
Member Author

By moving the new additions to a separate page (say the current "Data interchange mechanisms")

Makes sense. Let me do that in a separate PR, and address some of the other points in the PR description as well once we've figured out how to communicate version information.

@rgommers
Copy link
Member Author

I propose to merge this as is; the bigger update and content moving will take a little while.

@rgommers
Copy link
Member Author

Thanks for the reviews everyone!

@rgommers rgommers merged commit e3927fd into data-apis:main May 26, 2021
@rgommers rgommers deleted the dlpack-doc-update branch May 26, 2021 17:53
@BvB93 BvB93 mentioned this pull request Jul 28, 2021
13 tasks
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.

stream in __dlpack__ should not be required to be integers
4 participants