Skip to content

ENH: Add NumPy declarations to be used by Cython 3.0+ #16986

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 10 commits into from
Aug 5, 2020

Conversation

scoder
Copy link
Contributor

@scoder scoder commented Aug 1, 2020

As discussed in cython/cython#3573, the latest alpha versions of Cython 3.0 need updated NumPy declarations to allow users to set -D NPY_NO_DEPRECATED_API=NPY_1_7_API_VERSION. Currently, builds with that setting fail with NumPy 1.19.[01] because the numpy/__init__.pxd in NumPy overrides the more current one in Cython 3.0.

This PR adds an updated copy of numpy/__init__.pxd as numpy/__init__.cython-30.pxd to make Cython 3.0+ use it.

I first created a copy of the original __init__.pxd in order to make the changes visible in the diff. Two additional changes:

  • I'm not sure if we want the check_size ignore declarations in there. Cython has never had them (except for the one on ndarray), and that's what people were probably using, but … not my decision, I guess.
  • numpy.broadcast used to have struct fields defined in the NumPy version but not in the Cython version previously, so keeping them out is probably ok, but would now break backwards compatibility with NumPy 1.19.[01], where Cython 0.29.x already picks up the (old) NumPy version. Can't say how important that is for you. If at all, then they should rather be added as C-properties (as with the ndarray fields).

Whatever you decide, I'll follow in the Cython-3.0-side version of the file.

@jbrockmendel @mattip

@charris charris changed the title [ENH] Add NumPy declarations to be used by Cython 3.0+ ENH: Add NumPy declarations to be used by Cython 3.0+ Aug 1, 2020
@charris
Copy link
Member

charris commented Aug 1, 2020

Needs a release note. If this might affect backward compatibility that should be mentioned.

How big is the diff? If it is small might be worth posting so we can see what is different.

@scoder
Copy link
Contributor Author

scoder commented Aug 1, 2020

How big is the diff? If it is small might be worth posting so we can see what is different.

Do you mean the second commit? Or what diff are you referring to?

@mattip mattip added the 09 - Backport-Candidate PRs tagged should be backported label Aug 2, 2020
@mattip
Copy link
Member

mattip commented Aug 2, 2020

Missing a line in setup.py to add this to the wheel, so we don't get a repeat of that PR. Also should be added to MANIFEST.in next to __init__.pxd so it shows up in the sdist. Otherwise LGTM, thanks.

We need the check_size ignore, see the long discussion cython/cython#2221.

For those who want some background what all this is, see the details.

I think this should be backported to 1.19.2 since people who use -D NPY_NO_DEPRECATED_API=NPY_1_7_API_VERSION will not be able to use Cython 3.0 without it.

I tried building pandas HEAD with this PR and it seems to work, unless I did something wrong.

This will allow us to ship two versions of the `__init__.pxd`: one for Cython 3.0 that respects the "new" post 1.7 numpy API interfaces, and one for Cython 0.29 that uses the "old" pre-1.7 interfaces.

The difference is these decorated property getters, that turn cython code like s = array.shape into a function call np_intp *s = PyArray_DIMS(array);rather than an C-level structure lookupnp_intp *s = array.shape;`. The version used will be automatically chosen via the file name.

@scoder
Copy link
Contributor Author

scoder commented Aug 2, 2020

Also should be added to MANIFEST.in next to __init__.pxd so it shows up in the sdist.

I changed include numpy/__init__.pxd to include numpy/*.pxd. Likewise for setup.py.

We need the check_size ignore

I was merely asking whether we need it for all types or just ndarray. Cython only had it set on ndarray previously. I can certainly add it to all extension types, if that's what you want.

Since this only regards Cython 3.0, I don't really see an issue with backwards compatibility.

I'll leave the decision to you whether to expose the numpy.broadcast attributes/properties or not. If you want to add the properties on your side, I'd be happy to receive a PR for Cython that copies the change back.

….pxd" that previously had the option set in "__init__.pxd".
@eric-wieser
Copy link
Member

If you want to add the properties on your side, I'd be happy to receive a PR for Cython that copies the change back.

I'm a bit confused - why are both numpy and cython distributing this file, and how do users specify which they intend to use?

@scoder
Copy link
Contributor Author

scoder commented Aug 2, 2020

why are both numpy and cython distributing this file

The file was originally distributed by Cython, until we realised that a representation of NumPy's C-API is better maintained by NumPy in a version specific way. So we created the infrastructure for that, which now allows NumPy to distribute it. However, older NumPy versions do not have it, which is why Cython still ships a copy.

how do users specify which they intend to use?

Cython only uses its own version if it doesn't find one on the Python import path. If NumPy is installed, its version will be preferred. Note that this PR only adds an improved version of the (existing) declarations that requires features from Cython 3.0+. Thus the file suffix that prevents older Cython versions from picking it up. They will continue to use the existing numpy/__init__.pxd file or their own shipped version.

@scoder
Copy link
Contributor Author

scoder commented Aug 3, 2020

I also found an unused private utility function in the files that was only used by the legacy __getbuffer__ implementation, which was already removed previously. I removed the function in both files in separate commits, in case you want to keep it in the old one (for whatever reason).

@mattip
Copy link
Member

mattip commented Aug 4, 2020

LGTM, I will give other reviewers some time to take a look.

@charris
Copy link
Member

charris commented Aug 4, 2020

Please squash the PR when merging to facilitate the backport.

@charris
Copy link
Member

charris commented Aug 4, 2020

only used by the legacy getbuffer implementation

@scoder Was the legacy implementation in NumPy or Cython? I ask because this will be backported to NumPy 1.19.x.

@scoder
Copy link
Contributor Author

scoder commented Aug 4, 2020

Was the legacy implementation in NumPy or Cython?

The code was only in Cython, never in NumPy. The latter only accidentally inherited the unused utility function. It should have been removed before adding the file to NumPy (1.19).

@mattip mattip merged commit e1211b8 into numpy:master Aug 5, 2020
@mattip
Copy link
Member

mattip commented Aug 5, 2020

Thanks @scoder

charris pushed a commit to charris/numpy that referenced this pull request Aug 5, 2020
* Create copy of numpy.pxd for Cython 3.0 changes and improve it.
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Aug 5, 2020
@charris charris mentioned this pull request Oct 10, 2020
20 tasks
@zoj613
Copy link
Contributor

zoj613 commented Aug 28, 2023

For some reason, I can no longer use np.broadcast in Cython 3.0+ in GIL releasing code or functions that expect C types. I get errors like Cannot convert Python object to 'npy_intp *' and Truth-testing Python object not allowed without gil when using the struct properties. Is there a way to fix this on my side?

@seberg
Copy link
Member

seberg commented Sep 5, 2023

You will need to be more precise. cnp.broadcast requires the GIL, its methods probably don't (genrally!). Truth-testing is used where exactly/how?
I could imagine Cython 3 is just better at figuring out that your code was never really allowed to release the GIL.

@zoj613
Copy link
Contributor

zoj613 commented Sep 5, 2023

You will need to be more precise. cnp.broadcast requires the GIL, its methods probably don't (genrally!). Truth-testing is used where exactly/how? I could imagine Cython 3 is just better at figuring out that your code was never really allowed to release the GIL.

Basically, I cannot access the struct properties outlined here: https://numpy.org/doc/stable/reference/c-api/types-and-structures.html#c.PyArrayMultiIterObject . This is only if I upgrade cython to 3.x. This is not an issue in 0.29.X. I noticed that this may be due to the different definition of numpy.broadcast in https://github.com/scoder/numpy/blob/df769ef591f8a21f8e39ee30c80e09d1b7e0a705/numpy/__init__.cython-30.pxd#L230-L232 compared to https://github.com/scoder/numpy/blob/df769ef591f8a21f8e39ee30c80e09d1b7e0a705/numpy/__init__.pxd#L227-L232

Here is an example piece of code where these are meant to be used: https://github.com/zoj613/polyagamma/blob/6696587c8a1ca69090d6fff374a649526f114089/polyagamma/_polyagamma.pyx#L309-L332

@seberg
Copy link
Member

seberg commented Sep 5, 2023

Can you open a new issue with the more concrete issues that you need access to the struct because there are no macros for most fields?
(We could consider adding macros, in Cython 3 it is possible to add properties so that attribute access is translated to a macro call.)

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.

6 participants