Skip to content

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

Merged
merged 11 commits into from
Apr 28, 2023

Conversation

seberg
Copy link
Member

@seberg seberg commented Apr 4, 2023

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:

#define NPY_TARGET_VERSION NPY_1_22_API_VERSION

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:

  • feedback?
  • release notes
  • Brief user documentation (in the document linked also)
  • Brief point in the contributor guide since adding guards and 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.

seberg added 2 commits April 4, 2023 17:17
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
Copy link
Member Author

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?

Copy link
Member

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.

@mattip
Copy link
Member

mattip commented Apr 5, 2023

I have compiled SciPy

How did you pass the define to meson?

@seberg
Copy link
Member Author

seberg commented Apr 5, 2023

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>
Copy link
Member Author

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.

@mattip
Copy link
Member

mattip commented Apr 5, 2023

Ahh, the default in this PR to use NPY_1_17_API_VERSION. So going forward anyone who wants to use 1.25 locally will need to use a define, specifically #define NPY_TARGET_VERSION NPY_1_25_API_VERSION. This is a change. Until now it was sufficient to install NumPy 1.25 (or list it as a dependency) to get 1.25 semantics. Going forward, everyone will need to define the desired API version in addition to installing the version.

@seberg
Copy link
Member Author

seberg commented Apr 5, 2023

Yes, if you want to use the newest API you will have to define NPY_TARGET_VERSION NPY_API_VERSION. I don't think this matters in practice (at this point), because there is only one relevant change in the API (the memory handler) and that is very niche.

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.

seberg added 2 commits April 6, 2023 17:43
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.
@seberg seberg force-pushed the allow-backcomp-builds branch from 2058ec4 to df7b235 Compare April 11, 2023 15:08
@seberg seberg removed the 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes label Apr 12, 2023
@seberg seberg changed the title ENH: Allow backwards compatible builds ENH: Allow, and default to, downstream building with old API Apr 13, 2023
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...
@rgommers rgommers added this to the 1.25.0 release milestone Apr 19, 2023
Copy link
Member

@rgommers rgommers left a 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?

``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).
Copy link
Member

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.

@rgommers
Copy link
Member

I haven't tested yet, but will make some time to play with this.

@seberg
Copy link
Member Author

seberg commented Apr 20, 2023

It may be useful to say something about the new DType related C API, and why it does or doesn't matter here.

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

I think at least this becomes more of a problem with the use of libnpymath/libnpyrandom

Hmmm, I figured as static libs that shouldn't use numpy proper we don't have to worry too much.
Is downstream ever using old compilers intentionally for compat with old NumPy versions? It seems like possibly important, but I can't think of where it matters. (Although, I would probably worry more if it was C++ API.)

@rgommers
Copy link
Member

The new DType API isn't public yet

Ah I missed that - never mind then:)

Is downstream ever using old compilers intentionally for compat with old NumPy versions?

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 libnpymath.so that isn't.

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.
@seberg seberg force-pushed the allow-backcomp-builds branch from 9609edf to c62881f Compare April 25, 2023 09:18
@seberg seberg requested a review from mattip April 26, 2023 19:33
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not 1.17?

Suggested change
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

@mattip
Copy link
Member

mattip commented Apr 27, 2023

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>
@mattip mattip merged commit 4605582 into numpy:main Apr 28, 2023
@mattip
Copy link
Member

mattip commented Apr 28, 2023

Thanks @seberg

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants