-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
fix: include numpy._core
imports for NumPy 2.0
#4857
Conversation
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:
I see it's your own PR: numpy/numpy#24634 What will be the official (non-private) way to obtain the If we continue to need access to those pointers, could you provide an official API for them? |
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: pybind11/tests/test_exceptions.cpp Line 249 in 7e5edbc
|
That would definitely be better. Pointing out for completeness:
|
Hi! I'm mentoring @mtsokol's Numpy 2.0 API migration project.
Can you open issues on the NumPy issue tracker describing your needs for these two symbols? For For 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 |
Hi @ngoldbaum, some background and opinions:
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:
Probably a silly question: how do you |
I think leaving behind
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:
and then later in that same file do:
(in the extension module I'm working on right now this happens inside of the Other compilation units need something like:
The You probably also want |
@rwgk For now I think yes, in terms of numpy/numpy#24634 changes - I don't plan any other changes to the
Sounds good in a long term - I'm curious about changes that would be required to |
Ok, I added additional import checks for each step. |
13fb695
to
340afab
Compare
41100e6
to
45cc2a0
Compare
@ngoldbaum A couple thoughts:
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?
So this sounds like nothing is needed 1 time, Did you consider something like this?
That way you'd have something special only 1 time, nothing N times. |
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.
I'm not sure if there is a problem with adding a new macro, I'd open an upstream NumPy issue. |
include/pybind11/numpy.h
Outdated
@@ -120,6 +120,25 @@ inline numpy_internals &get_numpy_internals() { | |||
return *ptr; | |||
} | |||
|
|||
PYBIND11_NOINLINE module_ import_numpy_core_submodule(const char *submodule_name) { | |||
try { | |||
return module_::import((std::string("numpy._core.") + submodule_name).c_str()); |
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.
Could you please add a (terse) comment here explaining when this is expected to work, and below similarly?
Rough idea:
// numpy._core was established with numpy PR ...
// numpy.core was the import needed up until numpy release ...
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.
Sure! I added a comment.
+ " from numpy."); | ||
} | ||
} | ||
} |
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.
What you have here looks acceptable, but I wonder if we could make this more robust pretty easily, along the lines of:
import numpy
- Inspect
numpy.__version__
or similar to decide ifcore
or_core
is needed, and try only that one import. (I'm hoping this might be more feasible than usual because you own the numpy API.)
This is less likely to be surprising if there is some low-level issue in pybind11 or Python itself (refactoring, new major dev version), or numpy is changed further in the future. The error message then could be the original import error, re-raised using raise_from
(very easy):
pybind11/include/pybind11/pytypes.h
Line 817 in 7e5edbc
inline void raise_from(error_already_set &err, PyObject *type, const char *message) { |
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.
So I did similar changes for JAX and originally I also decided to go with numpy version checks in an if
statement and then a single import.
But I was advised that try ... catch
approach is more robust. Here's an explanation from Jake: jax-ml/jax#16972 (review)
Version checks are fine for tests, but I'd prefer to avoid them in package import paths – e.g. imagine a user who has some custom numpy build with a version string that we don't anticipate, and then import jax might fail on trying to cast the version to a tuple of ints.
But I'm happy to switch to a version check if you prefer it!
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're dealing with 1 try vs 2 tries here, that's similar but not the same.
version check
I'm thinking of it more as a "capability check".
(Version numbers a convenient way to answer "what exactly do I have", that's a different purpose.)
I'm still hoping that we can rise to a level of organization that allows us to only have 1 import and report the original error: does or can numpy provide a simple API that allows us to only try once?
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.
Sure! I switched to an if
statement that selects package path based on numpy version - now (_)core.multiarray
or (_)core._internal
is imported only once.
(it might look lengthy, but this is the recommended way to get major version integer: https://numpy.org/doc/stable/release/1.9.0-notes.html#numpyversion-class-added)
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 think I somehow lost a comment here that I thought I typed in already. Sorry if something appears twice.)
std::string numpy_core_path = major_version >= 2 ? "numpy._core" : "numpy.core";
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.
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 can’t really check (at a conference), but what are we using the core module for? If it’s private that means NumPy is free to break us any time. Would be very nice to start moving to whatever is supported instead longer term.
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.
Is it only our tests? Ahh, no, multiarray. Is that available directly? Doesn’t seem to be. Is it supposed to stick around?
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.
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.
(Numpy has a public C API as well, but that's difficult for us to use because that would complicate our build process quite a bit).
@@ -263,7 +282,7 @@ struct npy_api { | |||
}; | |||
|
|||
static npy_api lookup() { | |||
module_ m = module_::import("numpy.core.multiarray"); | |||
module_ m = detail::import_numpy_core_submodule("multiarray"); | |||
auto c = m.attr("_ARRAY_API"); | |||
void **api_ptr = (void **) PyCapsule_GetPointer(c.ptr(), nullptr); |
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.
It's an existing problem, but could you please add this fix or similar while you're at it (untested)?
if (api_ptr == nullptr) {
raise_from(PyExc_SystemError, "FAILURE obtaining numpy _ARRAY_API pointer.");
}
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.
Sure, I added this if
statement - looks good to me.
Oh, sorry, I just didn't know :-)
If |
I just added @EthanSteinberg as a reviewer. I believe he knows much more about numpy than I do. |
Thanks for helping fix this @ngoldbaum. I agree with @rwgk's recommendations but have two comments
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? |
This makes me very sad TBH, but mainly because there is no way I could reasonably disagree.
That sounds like a great plan!
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? |
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. |
d125e07
to
4c39988
Compare
I see that one of the pre-commit checks disallows |
+ " from numpy."); | ||
} | ||
} | ||
} |
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 think I somehow lost a comment here that I thought I typed in already. Sorry if something appears twice.)
std::string numpy_core_path = major_version >= 2 ? "numpy._core" : "numpy.core";
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 Otherwise it's ready from my side! |
@@ -287,6 +282,7 @@ struct npy_api { | |||
void **api_ptr = (void **) PyCapsule_GetPointer(c.ptr(), nullptr); | |||
if (api_ptr == nullptr) { | |||
raise_from(PyExc_SystemError, "FAILURE obtaining numpy _ARRAY_API pointer."); | |||
throw error_already_set(); |
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 think you're right! This needs to be here. How did you notice?
Looks like a made a mistake in #4570.
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.
Here in documentation: https://pybind11.readthedocs.io/en/stable/advanced/exceptions.html#chaining-exceptions-raise-from
To do a similar thing in pybind11, you can use the py::raise_from function. It sets the current python error indicator, so to continue propagating the exception you should throw py::error_already_set().
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.
Good catch (it completely slipped my mind), thanks a lot!
I'm happy, too. Waiting for @EthanSteinberg or @henryiii for a 2nd set of eyes. |
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.
Looks good to me! Thanks again @mtsokol for the PR!
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.
Left a few comments, but not blockers.
Co-authored-by: Ralf W. Grosse-Kunstleve <[email protected]>
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. |
@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. |
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. |
.cast<object>() | ||
.release() | ||
.ptr(); | ||
module_ m = detail::import_numpy_core_submodule("_internal"); |
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.
@rwgk I would bet this change right here is causing the deadlocking. Look at how the old code did it all in one static call so the import was only once (even if this function got called a bunch).
The new code is importing _internal every time this function is called.
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.
That was my first suspicion, too, but that change by itself did not help.
Then I decided to roll back and debug after production is in a healthy state again.
Sorry I should have mentioned this before. (I'm currently battling multiple fires.)
Almost certainly we should change the code here back, but I want to get to the bottom of the timeouts first. (I only have an internal reproducer at the moment. It involves running a test 100 times, ~1/3 of the tests time out.)
This applies the following patches to pybind11: - pybind/pybind11#4857 - pybind/pybind11#4877 to avoid deadlock when using pybind11 without importing numpy in multi-threaded environment.
This applies the following patches to pybind11: - pybind/pybind11#4857 - pybind/pybind11#4877 to avoid deadlock when using pybind11 without importing numpy in multi-threaded environment.
…mmand (#618) ### Update Catch2 to v3.4.0 Without upgrading Catch2, The following error occurs when building on Ubuntu 22.04 due to glibc: ``` cucim/build-debug/_deps/deps-catch2-src/single_include/catch2/catch.hpp:10830:58: error: call to non-‘constexpr’ function ‘long int sysconf(int)’ 10830 | static constexpr std::size_t sigStackSize = 32768 >= MINSIGSTKSZ ? 32768 : MINSIGSTKSZ; ``` ### Update pybind11 to v2.11.1 Even with the latest version of pybind11, we still have an issue with `pybind11::array_t` when cuCIM is used in multithread without importing numpy in the main thread. See pybind/pybind11#4877 Will need to wait for the next release of pybind11. ### Use runtime option instead of using nvidia-docker command nvidia-docker binary is not available if user doesn't install nvidia-docker2 package. This change uses runtime option instead of using nvidia-docker command. ### Apply pybind11 patch to avoid deadlock (until new release is available) This applies the following patches to pybind11: - pybind/pybind11#4857 - pybind/pybind11#4877 to avoid deadlock when using pybind11 without importing numpy in multi-threaded environment. Authors: - Gigon Bae (https://github.com/gigony) - Gregory Lee (https://github.com/grlee77) Approvers: - Gregory Lee (https://github.com/grlee77) - https://github.com/jakirkham URL: #618
numpy._core
importsnumpy._core
imports for NumPy 2.0
* fix: Use lowercase builtin collection names (pybind#4833) * Update render for buffer sequence and handle (pybind#4831) * fix: Add capitalize render name of `py::buffer` and `py::sequence` * fix: Render `py::handle` same way as `py::object` * tests: Fix tests `handle` -> `object` * tests: Test capitaliation of `py::sequence` and `py::buffer` * style: pre-commit fixes * fix: Render `py::object` as `Any` * Revert "fix: Render `py::object` as `Any`" This reverts commit 7861dcf. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Ralf W. Grosse-Kunstleve <[email protected]> * fix: Missing typed variants of `iterator` and `iterable` (pybind#4832) * Fix small bug introduced with PR pybind#4735 (pybind#4845) * Bug fix: `result[0]` called if `result.empty()` * Add unit test that fails without the fix. * fix(cmake): correctly detect FindPython policy and better warning (pybind#4806) * fix(cmake): support DEBUG_POSTFIX correctly (pybind#4761) * cmake: split extension Into suffix and debug postfix. Pybind11 is currently treating both as suffix, which is problematic when something else defines the DEBUG_POSTFIX because they will be concatenated. pybind11_extension sets SUFFIX to _d.something and if DEBUG_POSTFIX is set to _d. _d + _d.something = _d_d.something The issue has been reported at: pybind#4699 * style: pre-commit fixes * fix(cmake): support postfix for old FindPythonInterp mode too Signed-off-by: Henry Schreiner <[email protected]> --------- Signed-off-by: Henry Schreiner <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Henry Schreiner <[email protected]> * Avoid copy in iteration by using const auto & (pybind#4861) This change is fixing a Coverity AUTO_CAUSES_COPY issues. * Add 2 missing `throw error_already_set();` (pybind#4863) Fixes oversights in PR pybind#4570. * MAINT: Include `numpy._core` imports (pybind#4857) * MAINT: Include numpy._core imports * style: pre-commit fixes * Apply review comments * style: pre-commit fixes * Add no-inline attribute * Select submodule name based on numpy version * style: pre-commit fixes * Update pre-commit check * Add error_already_set and simplify if statement * Update .pre-commit-config.yaml Co-authored-by: Ralf W. Grosse-Kunstleve <[email protected]> --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Ralf W. Grosse-Kunstleve <[email protected]> * MAINT: Remove np.int_ (pybind#4867) * chore(deps): update pre-commit hooks (pybind#4868) * chore(deps): update pre-commit hooks updates: - [github.com/psf/black-pre-commit-mirror: 23.7.0 → 23.9.1](psf/black-pre-commit-mirror@23.7.0...23.9.1) - [github.com/astral-sh/ruff-pre-commit: v0.0.287 → v0.0.292](astral-sh/ruff-pre-commit@v0.0.287...v0.0.292) - [github.com/codespell-project/codespell: v2.2.5 → v2.2.6](codespell-project/codespell@v2.2.5...v2.2.6) - [github.com/shellcheck-py/shellcheck-py: v0.9.0.5 → v0.9.0.6](shellcheck-py/shellcheck-py@v0.9.0.5...v0.9.0.6) - [github.com/PyCQA/pylint: v3.0.0a7 → v3.0.0](pylint-dev/pylint@v3.0.0a7...v3.0.0) * Update .pre-commit-config.yaml * style: pre-commit fixes * Update .pre-commit-config.yaml --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Henry Schreiner <[email protected]> --------- Signed-off-by: Henry Schreiner <[email protected]> Co-authored-by: Sergei Izmailov <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Henry Schreiner <[email protected]> Co-authored-by: László Papp <[email protected]> Co-authored-by: Oleksandr Pavlyk <[email protected]> Co-authored-by: Mateusz Sokół <[email protected]>
Description
numpy.core
is becoming officially a private module and is being renamed tonumpy._core
in numpy/numpy#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 nightlynumpy
2.0 installed, and the other way around). A warning will be emitted when accessing an attribute or submodule vianumpy.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
importsnumpy
and retrieves its version string. If numpy >= 2.x is installednumpy._core.*
path is used for importing internal attributes. Otherwisenumpy.core.*
is used.With this change downstream libraries depending on
numpy
nightly builds andpybind11
won't experience the deprecation warning.core
to_core
rename: API: Renamenumpy/core
tonumpy/_core
[NEP 52] numpy/numpy#24634Suggested changelog entry: