FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Add versioning support to DLPack APIs by rgommers · Pull Request #602 · data-apis/array-api · GitHub

Add versioning support to DLPack APIs - #602

Merged
leofang merged 8 commits into
data-apis:mainfrom
rgommers:dlpack-api-update
Feb 9, 2024
Merged

Add versioning support to DLPack APIs#602
leofang merged 8 commits into
data-apis:mainfrom
rgommers:dlpack-api-update

Conversation

Copy link
Copy Markdown
Member

rgommers added API extension Adds new functions or objects to the API. topic: DLPack DLPack. labels Feb 21, 2023
rgommers added this to the v2023 milestone Feb 21, 2023
leofang assigned leofang and unassigned leofang Mar 23, 2023
leofang self-requested a review March 23, 2023 18:37

kgryte commented Apr 17, 2023

Copy link
Copy Markdown
Contributor

@rgommers Currently, this PR adds a max_version kwarg, which assumes that a consumer can handle any version prior to the specified max version. Given that major versions indicate ABI breaks, would it be preferable to use something like semver ranges in order to be able to accommodate both min and max versions? This would allow a single kwarg for handling version support in a general way.

I suppose, in theory, could also add a min_version kwarg, either now or in the future, which could be used in tandem with max_version to achieve the same thing.

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 max_version is greater than the producer's max version, the consumer understands the producer. Is that guaranteed to always be true? That puts the onus on the consumer to effectively support all versions if different producers vary in their supported max versions.

Copy link
Copy Markdown
Member Author

Currently, this PR adds a max_version kwarg, which assumes that a consumer can handle any version prior to the specified max version

It doesn't really. It indicates nothing beyond what the max version is, with the intended effect being "if the producer supports a version >max_version as well as one <=max_version, please return the latter.

Given that major versions indicate ABI breaks, would it be preferable to use something like semver ranges in order to be able to accommodate both min and max versions? This would allow a single kwarg for handling version support in a general way.

What does knowing the minimum version change? I think nothing, either way it's either okay or the user gets an error.

That puts the onus on the consumer to effectively support all versions if different producers vary in their supported max versions.

It doesn't, the consumer is free to raise an exception if it has dropped support for some old version.

kgryte commented Apr 18, 2023

Copy link
Copy Markdown
Contributor

@leofang I see that you self-requested a review. You have any further thoughts on this PR?

leofang 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Sorry for late response, Ralf! Left a few questions.

leofang commented Jan 15, 2024

Copy link
Copy Markdown
Contributor

@rgommers do you have time to address the review, or should I take over this PR?

Copy link
Copy Markdown
Member Author

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 1.0 tag, and am pretty swamped with the last mile for NumPy 2.0

leofang self-assigned this Jan 23, 2024

kgryte commented Feb 7, 2024

Copy link
Copy Markdown
Contributor

@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?

leofang commented Feb 7, 2024

Copy link
Copy Markdown
Contributor

If you have time, yes please, but otherwise no worries I'll get to it tonight or tomorrow 😓

leofang marked this pull request as draft February 7, 2024 16:57
leofang marked this pull request as ready for review February 7, 2024 17:51

leofang commented Feb 7, 2024

Copy link
Copy Markdown
Contributor

@tqchen @seberg @rgommers @oleksandr-pavlyk Could you review the change here? We're finalizing the v2023 standard. Thanks! 🙂

seberg 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks Ralf! LGTM, one small suggestion.

kgryte and others added 2 commits February 8, 2024 05:04
Co-authored-by: Sebastian Berg <sebastian@sipsolutions.net>

rgommers commented Feb 8, 2024

Copy link
Copy Markdown
Member Author

Thanks for the updates! This looks ready to merge to me.

leofang commented Feb 9, 2024

Copy link
Copy Markdown
Contributor

Thanks all, let's merge and refine if needed, then.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

API extension Adds new functions or objects to the API. topic: DLPack DLPack.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants


Back | FazBrowse Home | New Git URL