-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat: drop PYBIND11_NUMPY_1_ONLY #5595
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
feat: drop PYBIND11_NUMPY_1_ONLY #5595
Conversation
@seberg Do you think removing the |
Yes, thanks! I think it existed an abundance of caution in case someone runs into problems. If someone used it, they would also hopefully done so only as a hot-fix anyway. |
Signed-off-by: Henry Schreiner <[email protected]>
abf7493
to
0c10429
Compare
Thanks @seberg, and thanks @itamaro for also looking around! I added 0c10429, very simple, JIC it helps someone not getting surprised. I think this is a great cleanup, what made me hesitate is what I shared on a slack group chat before. Copying here with slight modifications: With the pybind11 v3.0.0 release in mind: I think it's important to strategize carefully. I feel strongly that this isn't the right time for a systematic cleanup: Maximizing the v3.0.0 adoption rate seems far more important to me. From working on Google's Python team for 4ish years, and being on the hook for fixing up every single glitch around a gigantic code base, arising from even small-ish changes in pybind11, I learned the hard way that convoluting multiple changes can very quickly create hard-to-diagnose showstoppers. In many environments people may not have the time to troubleshoot right now and then, and upgrade attempts go stale. My preferred timeline is:
Bottom line: I think we should only pick out cleanup that clearly helps us moving forward in that 2-3-month time window. To me, this PR is in the gray area. We could comfortably get through the next 2-3 months without adding this non-essential change. But it does help us a little bit (cmake and .github changes), and based on the feedback we got from @seberg and @itamaro, I believe it'll not make the pybind11 v3.0.0 adoption any slower. |
And my response, also edited: Don’t we explicitly say we follow SemVer? Dropping a deprecated feature in a minor release is in violation of SemVer. (I don’t actually like SemVer, and am quite fine with Python’s version of it (actually, this is nearly identical to pytest’s version where they drop in x.1 releases), but our changelog makes a big deal about “now we follow SemVer starting in 1.8"). These features have been waiting for 3.0 to be dropped, and now we are saying we'll drop them randomly in a minor release? Then why do we say we follow SemVer and why didn't we drop them years ago? If that's the case, this is the only time we can do clean up like this. Every one of these deprecated features is untested (our test suite must build without warnings). And they all are producing a warning, some of them have been producing a warning for up to 8 years! When you have a major version, users are more likely to look at the changelog, and be willing to do some minor updates. We aren't asking for changes that are backward incompatible, just using non-deprecated features that have had better, supported alternatives for up to 8 years. We could plan for a 4.0 in the future (1 year-ish?) instead. But we need to pick which is more important for versioning (SemVer or ABI) and drop the mention of the other one, otherwise we’ll always be stuck in the same predicament, not wanting people to be stuck on an older version. By the way, a lot of users are pinning to old versions already, and a lot more use git checkouts. I've just done the simplest ones that I could easily search with cs.github.com and be pretty sure no one is using. Ones that used to be popular (like |
Description
I can find no usages at all of this define on GitHub. Most packages support
NumPy 2, and the few that don't yet still don't need this define, which was
added in an abundance of caution, I believe. Let's remove it and simplify our
testing.
Suggested changelog entry:
* Legacy-mode option `PYBIND11_NUMPY_1_ONLY` has been removed.