| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
@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. |
Sorry, something went wrong.
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.
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. |
Sorry, something went wrong.
|
@leofang I see that you self-requested a review. You have any further thoughts on this PR? |
Sorry, something went wrong.
There was a problem hiding this comment.
Sorry for late response, Ralf! Left a few questions.
Sorry, something went wrong.
|
@rgommers do you have time to address the review, or should I take over this PR? |
Sorry, something went wrong.
|
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 |
Sorry, something went wrong.
|
@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? |
Sorry, something went wrong.
|
If you have time, yes please, but otherwise no worries I'll get to it tonight or tomorrow 😓 |
Sorry, something went wrong.
|
@tqchen @seberg @rgommers @oleksandr-pavlyk Could you review the change here? We're finalizing the v2023 standard. Thanks! 🙂 |
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks Ralf! LGTM, one small suggestion.
Sorry, something went wrong.
Co-authored-by: Sebastian Berg <sebastian@sipsolutions.net>
|
Thanks for the updates! This looks ready to merge to me. |
Sorry, something went wrong.
|
Thanks all, let's merge and refine if needed, then. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
xref dmlc/dlpack#116