Skip to content
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

MAINT: Check numpy version in C-API #1

Merged
merged 4 commits into from
Sep 11, 2023

Conversation

mtsokol
Copy link
Owner

@mtsokol mtsokol commented Sep 8, 2023

Hi @rgommers @seberg,

This PR is an additional change to the core rename PR that involves modification of the C API. I decided to make it separate for an easier review.
As pointed out in numpy#24634 (comment) we should also make it possible to run a downstream library compiled against numpy 2.x that uses/imports numpy 1.x in execution.

As a result, all PyImport_ImportModule statements that import numpy._core and might end up in the downstream library extension modules need to check installed version of numpy.

As far as I understand, such code covers .h files from include and code generators that create entry point headers (included in this PR).

There are a few more PyImport_ImportModule("numpy._core...") in the C codebase, but these files are parts of multiarray .so file, so their compiled contents are imported in the runtime and aren't incorporated into downstream library extension module directly. Is it correct?

These are the files:

 numpe/_core/src/multiarray/arrayfunction_override.c  
 numpy/_core/src/multiarray/buffer.c            
 numpy/_core/src/multiarray/convert_datatype.c     
 numpy/_core/src/multiarray/ctors.c           
 numpy/_core/src/multiarray/descriptor.c        
 numpy/_core/src/multiarray/dtypemeta.c          
 numpy/_core/src/multiarray/getset.c              
 numpy/_core/src/multiarray/methods.c       
 numpy/_core/src/multiarray/multiarraymodule.c     
 numpy/_core/src/multiarray/scalarapi.c          
 numpy/_core/src/multiarray/scalartypes.c.src      
 numpy/_core/src/multiarray/strfuncs.c            
 numpy/_core/src/umath/funcs.inc.src                
 numpy/_core/src/umath/override.c               
 numpy/_core/src/umath/ufunc_object.c               
 numpy/_core/src/umath/ufunc_type_resolution.c  

Py_DECREF(version_obj);

return major_version;
}
Copy link

Choose a reason for hiding this comment

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

Using this to get the major version is a bit overloaded IMO, since we also have it available as a C function in the API table.
So, I think this helper should just import _multiarray_umath, that will also simplify all other code anyway and separate the concern of what the major version is.
Maybe PyObject_HasAttr(numpy, "_core") is enough for a check?

That said, we can probably probably add such a helper, although I think it should also be called _npy_... to more clearly signal that it is off-limits and a NumPy implementation detail.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ah, using PyObject_HasAttr(numpy, "_core") looks way simpler and it does the same job.

Copy link

Choose a reason for hiding this comment

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

Since I am not sure it was clear: I would suggest if we have a helper, it should be _npy_import_numpy_multiarray_umath().

Copy link
Owner Author

Choose a reason for hiding this comment

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

@seberg How about _npy_import_from_numpy_core(submodule_name)?

In autogenerated headers _multiarray_umath is imported, but there's also _internal being imported from _core in src/common/npy_ctypes.h. This header is defined as a dependency for libnpymath.a so it's sourcecode ends up in downstream library (if I understand it correctly).

(A downstream lib could use libnpymath.a when compiling and then run with NumPy 1.x - this could cause import error on numpy._core._internal).

Copy link

Choose a reason for hiding this comment

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

This header is defined as a dependency for libnpymath.a so it's

Sorry, I am not seeing/understanding this. Those should be private headers, and even if the function exposed there isn't used in libmpymath.a? Where do you see them being a dependency?

I would be surprised to find any use of this in public API accidentally or not. But, it wouldn't be the first surprise like this :).

Copy link
Owner Author

@mtsokol mtsokol Sep 8, 2023

Choose a reason for hiding this comment

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

I thought common/npy_ctypes.h is included here https://github.com/numpy/numpy/blob/d676a1fe2d495f9d8a86103644bed141c2e69787/numpy/core/meson.build#L544, but it only defines where to search for header files, right?
Then sorry for confusion!

Copy link
Owner Author

@mtsokol mtsokol Sep 8, 2023

Choose a reason for hiding this comment

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

Ok, I finished refactoring - I ended up with a simpler version.
For some reason I couldn't import numpy in this initialization function (PyImport_ImportModule("numpy") returns null, maybe due to some a circular import?)

return major_version;
PyObject *multiarray = PyImport_ImportModule("numpy._core._multiarray_umath");
if (multiarray == NULL) {
multiarray = PyImport_ImportModule("numpy.core._multiarray_umath");
Copy link

Choose a reason for hiding this comment

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

That is a try/except and somewhat legal, but not as is, you need to add a PyErr_Clear(), could check which exception type PyErr_ExceptionMatches (to be the same)

Copy link
Owner Author

Choose a reason for hiding this comment

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

Sure! I updated it.

@mtsokol
Copy link
Owner Author

mtsokol commented Sep 11, 2023

Hi @seberg,
Would you have any more comments to this PR or can I merge it to my main working branch?

if (numpy == NULL) {
PyErr_SetString(PyExc_ImportError,
"_multiarray_umath failed to import");
Copy link

Choose a reason for hiding this comment

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

Do you need this, it should already say something similar? But OK.

Generally seems fine, I am not immediately sure if utils.h is a good place for the helper.

The one worry I would have is that we might get a ModuleNotFoundError also because _multiarray_umath is missing (broken install). However, not sure I want to worry too much about it, since I suspect we will have enough info available and numpy is probably more often imported in Python first anyway.

@mtsokol mtsokol merged commit cdebcf3 into rename-numpy-core Sep 11, 2023
1 check failed
@mtsokol
Copy link
Owner Author

mtsokol commented Sep 11, 2023

Hmm... I see that jobs are failing with:

../numpy/_core/include/numpy/utils.h:39:18: error: ‘PyObject* _npy_import_numpy_multiarray_umath()’ defined but not used [-Werror=unused-function]
   39 | static PyObject *_npy_import_numpy_multiarray_umath()
      |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

as utils.h is also used for building other files. I can either create new .h file that will be included only where it's actually used, or move these few lines implementation directly instead of calling a function. Do you have a preference how to solve it?

[EDIT] I went with moving implementation directly to files where function was called - it's just 3 lines of a simple if statement more.

@mtsokol mtsokol deleted the rename-numpy-core-c-api branch September 12, 2023 06:41
mtsokol pushed a commit that referenced this pull request Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants