-
Notifications
You must be signed in to change notification settings - Fork 50
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
Add versioning support to DLPack APIs #602
Conversation
@rgommers Currently, this PR adds a I suppose, in theory, could also add a In the sample code (ref: https://github.com/data-apis/array-api/pull/602/files#diff-546629919d7440647da1b89638a960e831bfa6e855e662983976d48643ec3022R368), you indicate that it's on the consumer to accommodate the producer, which seems reasonable if the goal is to reduce the burden for producers by limiting the number of versions to support. But then you also state in https://github.com/data-apis/array-api/pull/602/files#diff-546629919d7440647da1b89638a960e831bfa6e855e662983976d48643ec3022R362 that, when the |
It doesn't really. It indicates nothing beyond what the max version is, with the intended effect being "if the producer supports a version
What does knowing the minimum version change? I think nothing, either way it's either okay or the user gets an error.
It doesn't, the consumer is free to raise an exception if it has dropped support for some old version. |
@leofang I see that you self-requested a review. You have any further thoughts on this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for late response, Ralf! Left a few questions.
@rgommers do you have time to address the review, or should I take over this PR? |
If you'd be able to take over @leofang, that would help I think. I'm not fully up to speed on what happened with the removed |
@leofang Would you like me to go ahead and resolve the merge conflicts on this PR, so that you can just focus on making the updates? |
If you have time, yes please, but otherwise no worries I'll get to it tonight or tomorrow 😓 |
@tqchen @seberg @rgommers @oleksandr-pavlyk Could you review the change here? We're finalizing the v2023 standard. Thanks! 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Ralf! LGTM, one small suggestion.
Co-authored-by: Sebastian Berg <[email protected]>
Thanks for the updates! This looks ready to merge to me. |
Thanks all, let's merge and refine if needed, then. |
xref dmlc/dlpack#116