-
Notifications
You must be signed in to change notification settings - Fork 223
feat: add from_* style constructor classmethods to StridedMemoryView and make constructor amenable to future from_*-style constructors #1224
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
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@cpcloud This is a breaking change. Can we please restore the old behavior and add a deprecation warning? We need a release note, too.
If we want to push for alternative constructors, we should eventually raise in
__init__following what we do in other objects.Uh oh!
There was an error while loading. Please reload this page.
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.
Can you point me to downstream breakages so I can understand the impact? Across all of GitHub the only use of
StridedMemoryViewis in a comment:https://github.com/search?q=StridedMemoryView+NOT+is%3Afork+NOT+repo%3ANVIDIA%2Fcuda-python+NOT+owner%3APopcornFX&type=code
I'd strongly prefer not to restore the behavior without evidence that this change causes problems.
Happy to add the release note.
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.
We have real users that cannot be found this way. Examples include issues filed by people outside of the team:
StridedMemoryViewattribues to typed efficient C/C++/Cython accessible values #180StridedMemoryViewfails with Jax arrays #285 (comment)We have way too many invisible users at this point (check out the download counts from e.g. pypistats). We have also been slammed by our PM for breaking too often and too much (despite I disagreed in that case; in any case I was the one arguing and taking time in sorting things out, so plz back me up).
We need to make sure major breaking changes like this have a deprecation period, even before GA. In particular, in this case it is absolutely fine that we add extra constructors without breaking anything. I am not sure why we must leap so aggressively.
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.
+1 to @leofang guidance. Breaking existing public facing API should go through our teams standard deprecation flow where we notify users at least a release before to when we remove the code.
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.
Apologies for approving while misunderstanding the backward-compat guarantees of the
experimentalnamespace.I think this is all a reasonable policy to have, but challenging to follow as long as we are releasing from main. If we had a maintenance branch, we could put the "backward-compatible-with-deprecation-warnings" version of this on the maintenance branch, and the aspirational "where-we-want-to-be" on main. As it stands, we need to do the first now, put a reminder in a calendar to do the second and hope that reviews get it merged in the expected window.
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, I've meant to follow up here and offline with Phillip, but I got distracted (as always) by other things including the pixi stuff that Phillip started playing around with 😛
I did not mean that this PR is bad. I apologize if my earlier comments gave this impression. The alternative constructors are totally fine. They give users finer control if they already have prior knowledge on what protocols a foreign object supports. I meant that we just need to preserve the default constructor's behavior for a little longer, at least for another minor release. We could just revert the change on
__init__()and leave everything else as-is, though now that we have an urgent (TBD) patch release in preparation, it is good that it's reverted for now.We're dealing with an interesting situation. On the one hand we're still beta/experimental software, and we do allow breaking changes in each minor release. On the other hand we are perhaps too successful in telling our CUDA Python story that we have unexpectedly high number of users, and this does increasingly keep me up at night. If we break too often, downstream projects end up pinning too tightly (example), resulting in NVIDIA software unable to be installed in the same environment. This hurts our storytelling.
I suggest that we clearly highlight all PRs that contain breaking changes with the "breaking" label and a rel-note entry under the Breaking Change section. With such marks we can make informed decisions and evaluate all breaking changes on a case-by-case basis. There is also no need to seek short turnaround in merging breaking PRs. We still want to design
cuda.coreproperly in a future-looking fashion to avoid as much breaks as possible, it's been one of the core principles since Day 1, and for that I am fine taking a bit more time thinking through the design.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.
I didn't interpret your comments as this PR being bad, just haven't had a chance to re-PR the changes while preserving the current constructor behavior. I will PR that today.