Skip to content

API: allow building in cython with Py_LIMITED_API #25531

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 9 commits into from
Jan 16, 2024

Conversation

mattip
Copy link
Member

@mattip mattip commented Jan 3, 2024

xref cython/cython#5697

The change in numpy/_core/include/numpy/arrayscalars.h is the start of what will be needed, but I don't know if it is enough since the test I added crashed. Py_LIMITED_API is a work-in-progress on Cython and it seems more is needed.

@mattip mattip marked this pull request as draft January 3, 2024 15:18
@mattip mattip added 62 - Python API Changes or additions to the Python API. Mailing list should usually be notified. Numpy 2.0 API Changes labels Jan 3, 2024
@mattip mattip added this to the 2.0.0 release milestone Jan 3, 2024
except subprocess.CalledProcessError as p:
print(f"{p.stdout=}")
print(f"{p.stderr=}")
raise
Copy link
Member Author

Choose a reason for hiding this comment

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

Try to print out the error instead of swallowing it

@mattip
Copy link
Member Author

mattip commented Jan 4, 2024

The test generates a C file that includes the arrayscalars.h header. Without the change to exclude PyUnicodeObject, compilation fails when Py_LIMITED_API is defined. This change may not be the only one needed for the limited API, but it is definitely needed to start, so it is worth putting in to 2.0.

Note that the PyUnicodeScalarObject NumPy type is not used explicitly in the public API. The only public use of *ScalarObject I could find is in the macro

#define PyArrayScalar_VAL(obj, cls)             \
        ((Py##cls##ScalarObject *)obj)->obval

That macro is not used in scipy, astropy, scikit-learn, nor cython. The only one of the scalar types used in the public API is PyBoolScalarObject which is exposed as

#define _PyArrayScalar_BoolValues ((PyBoolScalarObject *)PyArray_API[9])

and is used in the public macro for PyArrayScalar_False and PyArrayScalar_True.

I wonder if we could move much of this into private headers. @seberg thoughts?

@seberg
Copy link
Member

seberg commented Jan 7, 2024

I wonder if we could move much of this into private headers. @seberg thoughts?

Not sure how much PyArrayScalar_VAL is used. In principle it can be replaced with PyArray_ScalarAsCtype() which should be pretty much the same thing (with the same limitations).

In either case, hiding the obval in the limited API seems OK (even generally, TBH). There is a clear way to get the value available, and in normal code the overhead should be meaningless anyway.

I also think that defining the exported bool singletons as PyObject * would be reasonable also, I bet they are as likely to be used as PyObject * then scalar typed probably... But not sure that is actually helpful.

@mattip
Copy link
Member Author

mattip commented Jan 10, 2024

Conclusion from the triage discussion: there are projects that use PyArrayScalar_VAL and so it should get a limited API guard.

@mattip mattip marked this pull request as ready for review January 11, 2024 10:14
@seberg
Copy link
Member

seberg commented Jan 12, 2024

Thanks for updating, the test failures look real. Also might be good to add the note in the PyArrayScalar_VAL docs about what to use as replacement (and unavailability).
The change seems rather miniscule to some degree, how about still adding a realease note as an improvement that this is no supported but with the tiny caveat.

@mattip
Copy link
Member Author

mattip commented Jan 16, 2024

I added a release note

add the note in the PyArrayScalar_VAL docs

Hmm. Which docs 😄?

Copy link
Member

@seberg seberg left a comment

Choose a reason for hiding this comment

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

LGTM, I am not sure if we just should add the Py_LIMITED_API definition to the cython file, or whether Cython will limit itself to the limited API, but compile with the full one if we don't.

But beyond adding that, please feel free to merge (or if you know that it doesn't matter).

@mattip
Copy link
Member Author

mattip commented Jan 16, 2024

@charris: would this be worth back-porting? We haven't gotten complaints about the limited API, but it might prevent those complaints from appearing on older versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
30 - API 62 - Python API Changes or additions to the Python API. Mailing list should usually be notified. Numpy 2.0 API Changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants