fix: include `numpy._core` imports for NumPy 2.0

[mtsokol]

Description

numpy.core is becoming officially a private module and is being renamed to numpy._core in https://github.com/numpy/numpy/pull/24634 PR.

It is a backward and forward compatible change (in terms of this change only libraries compiled against numpy 1.x will work with nightly numpy 2.0 installed, and the other way around). A warning will be emitted when accessing an attribute or submodule via numpy.core.* path.

To avoid warnings for downstream libraries that use pybind11, such as scipy (these depercation warnings are turned into errors in the CI), I propose a change: pybind11/numpy.h imports numpy and retrieves its version string. If numpy >= 2.x is installed numpy._core.* path is used for importing internal attributes. Otherwise numpy.core.* is used.

With this change downstream libraries depending on numpy nightly builds and pybind11 won't experience the deprecation warning.

• NEP 52: https://numpy.org/neps/nep-0052-python-api-cleanup.html
• core to _core rename: https://github.com/numpy/numpy/pull/24634

Suggested changelog entry:

``pybind11/numpy.h`` now imports NumPy's ``multiarray`` and ``_internal`` submodules with paths depending on the version of installed NumPy (handling change in NumPy 2.0).

[rwgk]

I got tripped up a few times by the try-this-import-then-that pattern, while working on changes to pybind11. I think making this safe requires a little more attention, with an eventual error message clearly indicating that both imports failed.

However, I have a more fundamental concern:

• If _core is made private, don't we undermine that effort by using the private access?

I see it's your own PR: https://github.com/numpy/numpy/pull/24634

What will be the official (non-private) way to obtain the _ARRAY_API and _dtype_from_pep3118 pointers in the future?

If we continue to need access to those pointers, could you provide an official API for them?

[Skylion007]

Can we also ensure we just catch the ImportError? Do we have bindings for that exception type?

Ah, we do this properly one other place in the codebase: https://github.com/pybind/pybind11/blob/7e5edbc947168fa73e25c5b6e58d656f5e2e9863/tests/test_exceptions.cpp#L249

[rwgk]

Can we also ensure we just catch the ImportError?

That would definitely be better.

Pointing out for completeness:

void **api_ptr = (void **) PyCapsule_GetPointer(c.ptr(), nullptr);

if (PyErr_Occurred()) is missing after this. I.e. this will segfault a couple lines down if there is an error.

[ngoldbaum]

Hi! I'm mentoring @mtsokol's Numpy 2.0 API migration project.

What will be the official (non-private) way to obtain the _ARRAY_API and _dtype_from_pep3118 pointers in the future? If we continue to need access to those pointers, could you provide an official API for them?

Can you open issues on the NumPy issue tracker describing your needs for these two symbols?

For _dtype_from_pep3118, @hoodmane was recently looking at writing a grammar for pep3118 format strings (see https://github.com/numpy/numpy/issues/24428) maybe it makes sense to have a small package on pypi that parses this format? Just spitballing, I don't have any context about why NumPy's PEP 3118 format string parser is in internals and don't know if there is some issue with making NumPy's implementation public.

For _ARRAY_API, this is used internally by NumPy to set up its own public C API. It looks like pybind11's usage is doing something very similar to numpy's import_array() function that downstream code is supposed to call to set up the C API. If there isn't a public way for a downstream project like pybind11 to get access to the NumPy C API symbols without going through the _ARRAY_API capsule, we probably need to add such a public mechanism that won't break if NumPy decides to change the way it sets up the public C API in the future.

We want to work with you to make sure you have public APIs for everything you need so you can move away from the things you're using in _core. One of the goals of making this change was to shake out downstream usages of numpy internals that we aren't aware of.

[rwgk]

Hi @ngoldbaum, some background and opinions:

• I wasn't involved in the implementation of pybind11/numpy.h. I just happen to be the most active pybind11 maintainer at the moment.

• IIUC pybind11/numpy.h was designed to work even if the numpy headers are not installed on the system on which the pybind11 bindings needing pybind11/numpy.h are compiled. Which is extremely convenient. But implies that it is assumed that the numpy API is set in stone and will never change. — Full disclosure: I was horrified when I first saw that around 3 years ago. Then amazed that nothing obviously broke or at least showed signs of bit rot in the meantime.

But maybe with the Numpy 2.0 API it's finally happening?

What do we do now? Taking away candy usually isn't going over well. Which brings me to a maybe radical question:

• Could we just leave pybind11/numpy.h as is?

• Add pybind11/numpy2.h and start over with the implementation, this time requiring the numpy headers? — We could work together to figure out what APIs we need.

Probably a silly question: how do you #include the numpy 2.0 API?

[ngoldbaum]

I think leaving behind numpy.h for older numpy releases and then adding a numpy2.h that does something nicer makes a lot of sense.

Probably a silly question: how do you #include the numpy 2.0 API?

Not silly, the numpy docs are really bad about explaining this.

You need to do something like this in one compilation unit per C extension:

#define NPY_TARGET_VERSION NPY_2_0_API_VERSION
#include "numpy/arrayobject.h"

and then later in that same file do:

import_array();

(in the extension module I'm working on right now this happens inside of the PyMODINIT_FUNC module initialization function)

Other compilation units need something like:

#define NPY_TARGET_VERSION NPY_2_0_API_VERSION
#define NO_IMPORT_ARRAY
#include "numpy/arrayobject.h"

The NO_IMPORT_ARRAY bit is to ensure import_array only gets called once per extension.

You probably also want #define NPY_NO_DEPRECATED_API NPY_2_0_API_VERSION to get warnings about uses of deprecated API items, but that's not required.

[mtsokol]

Could we just leave pybind11/numpy.h as is?

@rwgk For now I think yes, in terms of https://github.com/numpy/numpy/pull/24634 changes - I don't plan any other changes to the core module. I will push additional checks to this PR that you mentioned in the comments (I think it can be merged to avoid warnings in CI, if you build against numpy nightly builds).

Add pybind11/numpy2.h and start over with the implementation, this time requiring the numpy headers? - We could work together to figure out what APIs we need.

Sounds good in a long term - I'm curious about changes that would be required to include/numpy.h file.

[mtsokol]

Ok, I added additional import checks for each step.

[rwgk]

@ngoldbaum A couple thoughts:

#define NPY_TARGET_VERSION NPY_2_0_API_VERSION
#define NO_IMPORT_ARRAY

This looks like you've chosen a "maximally incremental" path. I'm not sure if my numpy2.h idea makes sense in that case, but we can see.

Is the API above (the two defines) already set in stone?

The NO_IMPORT_ARRAY bit is to ensure import_array only gets called once per extension.

So this sounds like nothing is needed 1 time, NO_IMPORT_ARRAY is needed N times.

Did you consider something like this?

#define NPY_TARGET_VERSION NPY_2_0_API_VERSION
#define NPY_2_0_API_IMPORT_ARRAY_IN_THIS_TRANSLATION_UNIT

That way you'd have something special only 1 time, nothing N times.

[ngoldbaum]

Is the API above (the two defines) already set in stone?

This is how it's always worked in NumPy, you'd just be targeting a different API version to target an older version of the NumPy API.

Did you consider something like this?

I'm not sure if there is a problem with adding a new macro, I'd open an upstream NumPy issue.

[rwgk]

This is how it's always worked in NumPy

Oh, sorry, I just didn't know :-)

I'm not sure if there is a problem with adding a new macro

If NO_IMPORT_ARRAY is long established I have a different mind about it and wouldn't add a new macro (I believe it's more likely to make things worse rather than a little better as hoped).

[rwgk]

I just added @EthanSteinberg as a reviewer. I believe he knows much more about numpy than I do.

[EthanSteinberg]

Thanks for helping fix this @ngoldbaum. I agree with @rwgk's recommendations but have two comments

1. We absolutely cannot use numpy's C code or headers because that significantly complicates the build system (which is almost always a mess for C++ extensions). So we cannot use import_array(); or #include "numpy/arrayobject.h" or similar.

2. What we can do is transition away from numpy's C api and instead work with its python api. There will probably be some slight overhead here, but I expect it will be minimal, especially because we can use tools like https://docs.python.org/3/c-api/buffer.html#c.PyObject_GetBuffer to avoid a lot of overhead.

This will require a significant revamp, but we can do it gradually, removing one numpy C api call at a time and replacing each of those calls with their python api alternatives.

What are your thoughts @rwgk, should we go through that effort? Is it worth the cost of maybe introducing bugs?

[rwgk]

We absolutely cannot use numpy's C code or headers because that significantly complicates the build system

This makes me very sad TBH, but mainly because there is no way I could reasonably disagree.

What we can do is transition away from numpy's C api and instead work with its python api.

That sounds like a great plan!

This will require a significant revamp ... should we go through that effort?

It would be fantastic if someone stepped up to do it. (I can only offer to do timely reviews at the moment.)

But it sounds like we could also incrementally work towards that goal? Like what we're currently doing here?

[EthanSteinberg]

I can help port over at least some of the calls, but not until at least November. I'll add a reminder to my calender.

[mtsokol]

I see that one of the pre-commit checks disallows Numpy capitalization - but it's needed to access an attribute. Can I remove that keyword from pre-commit config?

[rwgk]

Disallow improper capitalization.........................................Failed
- hook id: disallow-caps
- exit code: 1

include/pybind11/numpy.h:128:    object numpy_version = numpy_lib.attr("NumpyVersion")(version_string);

I've never seen an error like that before. It seems super weird that it would complain about caps in a string literal, how can that possibly be useful? If you have more background knowledge and know how to deal with this, please try a commit, maybe I'll understand better when I see it.

[mtsokol]

I've never seen an error like that before. It seems super weird that it would complain about caps in a string literal, how can that possibly be useful? If you have more background knowledge and know how to deal with this, please try a commit, maybe I'll understand better when I see it.

It's pygrep check in the pre-commit configuration that forbids "Numpy": https://github.com/pybind/pybind11/blob/7e5edbc947168fa73e25c5b6e58d656f5e2e9863/.pre-commit-config.yaml#L145

I guess it's made this way to only allow "numpy" or "NumPy" capitalization in the source code or docs. Well, here I need to access np.lib.NumpyVersion that was introduced at least 9 years ago so I would say "Numpy" is also a legal spelling. I pushed a change that drops this constraint.

[mtsokol]

What you have looks fine though, if you prefer, the only thing I'd do is remove the explicit std::string() inside the two if branches, i.e. I think simply numpy_core_path = "numpy._core"; will work and is what people usually do.

Right, I changed it to a one-line variant.

I also added throw error_already_set(); to void **api_ptr = ... because I found it explained this way in exceptions.rst. Let me know if I should reverse it.

Otherwise it's ready from my side!

[rwgk]

Otherwise it's ready from my side!

I'm happy, too.

Waiting for @EthanSteinberg or @henryiii for a 2nd set of eyes.

[rwgk]

For completeness:

I ran this PR through Google's global testing (internal test ID OCL:568738744:BASE:568878849:1695833342001:c72445f1).

I didn't expect issues and in fact found none.

[rwgk]

@mtsokol Could you please update the PR description and add a Suggested changelog entry?

I already restored the usual markup template, pasting most of your original description into the template, but it's no accurate anymore (doesn't match the merged implementation). If you could tweak it that would be great. Short and terse is best (IMO).

The changelog entry is usually just one sentence, just enough to give the idea for someone glancing through the list of changes for a release.

[mtsokol]

@mtsokol Could you please update the PR description and add a Suggested changelog entry?

@rwgk Sure! I updated the PR description.

[rwgk]

I deployed this PR Google-internally yesterday. Unfortunately I had to roll it back today. We saw a number of tests timing out, certainly related to this PR. My best guess, but totally without proof at the moment, is deadlocking.

It'll try to get to the bottom of it asap.