-
-
Notifications
You must be signed in to change notification settings - Fork 11.1k
ENH: Allow, and default to, downstream building with old API #23528
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
Conversation
This is a way for downstream users to specify which NumPy version they wish to be compaible with. Note that we provide a conservative default here (because almost nobody actually uses new API as they would lose backwards compatibility). Initially I had thought we should just redefine it so that the target version uses the same scheme as the Python hex version (and limited API), but since we have `NPY_1_15_API_VERSION` defines, use those... This commit does not include any actual use of this!
The default compiles compatibly with 1.17.x, we allow going back to 1.15 (mainly because it is easy). There were few additions in this time, a few structs grew and very few API functions were added. Added a way to mark API functions as requiring a specific target version. If a user wishes to use the *new* API, they have to add the definition: #define NPY_TARGET_VERSION NPY_1_22_API_VERSION Before importing NumPy. (Our version numbering is a bit funny I first thought to use a hex version of the main NumPy version, but since we already have the `NPY_1_22_API_VERSION` defines...)
0x00000010 = 04a7bf1e65350926a0e528798da263c0 | ||
|
||
# Version 17 (NumPy 1.25) No actual change. | ||
0x00000011 = ca1aebdad799358149567d9d93cbca09 |
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.
The min-version macros do goes into the hash right now. So removing them will also change it, but that seems fine?
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.
Only for >= 1.25, right? Should be fine I'd think.
How did you pass the define to meson? |
I didn't pass anything, so it would default to 1.17, which is what SciPy would want to use. |
@@ -89,6 +89,7 @@ def get_module(tmp_path): | |||
"""), | |||
] | |||
prologue = ''' | |||
#define NPY_TARGET_VERSION NPY_1_22_API_VERSION | |||
#define NPY_NO_DEPRECATED_API NPY_1_7_API_VERSION | |||
#include <numpy/arrayobject.h> |
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.
@mattip this is also a nice confirmation. Without this define build fails for the memory policy tests, since it requires API newer than 1.17.
Ahh, the default in this PR to use |
Yes, if you want to use the newest API you will have to define Overall, I honestly think this is a big improvement because those who use cython and distribute wheels will have not have to worry about it. |
This still needs to be expanded and maybe removes a bit more than it should. OTOH, the actual changes will mainly be necessary once NumPy 2.0 comes, right now the old scheme remains valid.
2058ec4
to
df7b235
Compare
This seemed like the clearest place to add a more in-depth note on it. The actual table, etc. also would make sense, but there you probably see the `MinVersion` anyway...
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.
This looks quite good to me, thanks @seberg!
The main questions/comments I have are:
- It may be useful to say something about the new
DType
related C API, and why it does or doesn't matter here. - Compilers and compiler versions: I'm wondering if we are making stronger assumptions about whether the older and latest numpy versions are built with the same (or a compatible) compiler versions
- I think at least this becomes more of a problem with the use of
libnpymath
/libnpyrandom
, and we'll probably have to get rid of those before starting to use this in SciPy?
- I think at least this becomes more of a problem with the use of
``conda`` will always use ``-no-build-isolation``; dependencies for conda | ||
builds are given in the conda recipe (``meta.yaml``), the ones in | ||
``pyproject.toml`` have no effect. | ||
before including any NumPy headers (or the equivalent ``-D`` compiler flag). |
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 briefly wondered whether it matters whether this define is used consistently globally or per extension module. I believe it's the latter; would be useful to clarify that here.
I haven't tested yet, but will make some time to play with this. |
The new DType API isn't public yet and much more restrictive (no compat guarantee), so I don't see that it needs mentioning? Once release, it would be hidden behind the versioning (mostly).
Hmmm, I figured as static libs that shouldn't use numpy proper we don't have to worry too much. |
Ah I missed that - never mind then:)
Yes, it happened at least once, on Windows when the vc141 and vc142 toolchains weren't compatible. It was a corner case though. In general, I'd say using the same compilers for building your whole stack is important either way though. So I can imagine cases where you build against numpy 1.25.0, which then gives you a C API that's compatible with >=1.19.0 but a static Let's not worry about that too much here though, we should just go ahead with deprecating those static libraries and stop using them. |
We have three choices: 1. Be compatible with limited API (oldest supported Python version) 2. Be compatible with everything on the same Python version (this) 3. Be strict and default to NEP 29 This just rephrases things to be 2. Because our API version was not bumped over the relevant time frame, it is actually also compatible with the first.
9609edf
to
c62881f
Compare
Starting with NumPy 1.25 when including NumPy headers, NumPy now | ||
defaults to exposing a backwards compatible API. | ||
This means that by default binaries such as wheels build against | ||
NumPy 1.25 will also work with NumPy 1.16 because it has the same API version |
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.
Not 1.17?
NumPy 1.25 will also work with NumPy 1.16 because it has the same API version | |
NumPy 1.25 will also work with NumPy 1.17 because it has the same API version |
Some comment and docstring suggestions, I left it to you to decide if they are improvements or not. Other than that this looks ready to go. |
Co-authored-by: Matti Picus <matti.picus@gmail.com>
Thanks @seberg |
This implements gh-23525. I made the slightly more conservative choice of not messing with our C version numbering after realizing that we have the
NPY_1_23_API_VERSION
macros anyway.So this adds the ability to do:
that defaults to
NPY_1_17_API_VERSION
(since that is reasonable) and I allow to go back up to 1.15.I did not bother to hide all structs (if they are unusable because all accessor structs are hidden). The most relevant change that I found looking through commit history and release notes that isn't captured is this one:
https://github.com/numpy/numpy/pull/14393/files
Which is arguably always better, but of course compatibility is in theory not perfect (in both directions really!).
TODOs:
MinVersion
is very important now.I have compiled SciPy and run on an older version, although not old enough to be relevant yet (M1 just doesn't support much back). OTOH, it clearly tests that the target mechanism works.