Skip to content

consistently call import_array() after cimport of numpy #17054

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 28, 2020
Merged

consistently call import_array() after cimport of numpy #17054

merged 1 commit into from
Apr 28, 2020

Conversation

grlee77
Copy link
Contributor

@grlee77 grlee77 commented Apr 26, 2020

Reference Issues/PRs

This might help with #16956, although I have not verified on ppc64le. Even if it does not fully resolve the issue, it seems that these calls to import_array are recommended by Cython.

We had the same issue as #16956 in the PyWavelets feedstock where ppc64le+pypy would crash, and adding calls to import_array fixed the issue. We also saw a similar issue on x86 with cython 3.0a1 in scikit-image where adding these resolved the segfaults.

What does this implement/fix? Explain your changes.

After calls to cimport numpy as np in Cython there should also be a call to np.import_array().

This is done many places in scikit-learn already. This PR aims to add it to the remaining files where it was missing (with a few exceptions in .pxd files when the corresponding .pyx file already had such a call)

Any other comments?

rth
rth approved these changes Apr 27, 2020
View reviewed changes
Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

Thanks @grlee77 ! Yes, there seem to be a few related reports on the Cython side lately e.g. cython/cython#3520. In Cython 3 cython/cython#3524 should fix it, but meanwhile this LGTM.

@jeremiedbb
Copy link
Member

I think that we also need to make sure that we include the headers in the setups for each cython extension using the cython numpy api: include_dirs=[numpy.get_include()].

There are files where you added the import_array even though we don't use the ndarray api at all, in _gradient_boosting.pyx for instance. I think we should not add import_array in such files, and probably also remove the cimport numpy.

@rth
Copy link
Member

rth commented Apr 27, 2020

think that we also need to make sure that we include the headers in the setups for each cython extension using the cython numpy ap

Shouldn't it fail at compilation time if headers are missing, though?

import_array even though we don't use the ndarray api at all, in _gradient_boosting.pyx for instance.

Are you sure memoryviews don't interact with numpy at all?

@jeremiedbb
Copy link
Member

Shouldn't it fail at compilation time if headers are missing, though?

right

Are you sure memoryviews don't interact with numpy at all?

I'm almost sure but couldn't find confirmation in the docs.

@jnothman
Copy link
Member

@jeremiedbb does this have your approval?

jeremiedbb
jeremiedbb approved these changes Apr 27, 2020
View reviewed changes
Copy link
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

lgtm. We can put this for the release and check when it's really necessary to cimport numpy afterwards

@jeremiedbb
Copy link
Member

thanks @grlee77 !

@jeremiedbb jeremiedbb merged commit 3deacb9 into scikit-learn:master Apr 28, 2020
@thomasjpfan
Copy link
Member

#17010

adrinjalali pushed a commit that referenced this pull request Apr 30, 2020
@grlee77 @adrinjalali
MNT consistently call import_array() after cimport of numpy (#17054)
166b3ad
gio8tisu pushed a commit to gio8tisu/scikit-learn that referenced this pull request May 15, 2020
@grlee77 @gio8tisu
MNT consistently call import_array() after cimport of numpy (scikit-l…
79a7ea9 
…earn#17054)
viclafargue pushed a commit to viclafargue/scikit-learn that referenced this pull request Jun 26, 2020
@grlee77 @viclafargue
MNT consistently call import_array() after cimport of numpy (scikit-l…
bf87f87 
…earn#17054)
@thomasjpfan thomasjpfan mentioned this pull request Mar 8, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rth rth rth approved these changes

@jeremiedbb jeremiedbb jeremiedbb approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@grlee77 @jeremiedbb @rth @jnothman @thomasjpfan