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

Make cimport numpy without import_array safer #3524

Merged
merged 9 commits into from
Apr 18, 2020

Conversation

da-woods
Copy link
Contributor

@da-woods da-woods commented Apr 16, 2020

Changes to Numpy module to use their C API rather than the
internal data structure mean that more things crash if import_array
isn't called.

For the discussion in #3520
it sounds like it's worth doing something to alert people to this.
This PR adds a call to _import_array in the module initialization.

I've added it to GilCheck (just because that's at the end of the
pipeline) however it could go elsewhere if there's a transform
suitable.

I wonder if it might be worthwhile to add a no_import_array
empty call to indicate "I'm deliberately not calling import_array.
I know what I'm doing. Don't warn." I haven't done this here but
it could be easily added.

There are cases when import_array isn't necessary (e.g. just using np.uint64_t or similar). I haven't attempted to detect these, and I don't think it's worthwhile trying to.

Changes to Numpy module to use their C API rather than the
internal data structure mean that more things crash if `import_array`
isn't called.

For the discussion in cython#3520
it sounds like it's worth doing _something_ to alert people to this.
It also sounds like implicitly adding `import_array` isn't the
best idea. Therefore this PR adds a warning. It can always be
reverted if a better option is found.

I've added it to GilCheck (just because that's at the end of the
pipeline) however it could go elsewhere if there's a transform
suitable.

I wonder if it might be worthwhile to add a `no_import_array`
empty call to indicate "I'm deliberately not calling import_array.
I know what I'm doing. Don't warn." I haven't done this here but
it could be easily added.
@da-woods da-woods changed the title Added warnings for cimport numpy without import_array Make cimport numpy without import_array safer Apr 17, 2020
(builtins aren't initialized in time to use the Cython import_array
version)
Cython/Compiler/ParseTreeTransforms.py Outdated Show resolved Hide resolved
Cython/Compiler/ParseTreeTransforms.py Outdated Show resolved Hide resolved
Cython/Compiler/ParseTreeTransforms.py Outdated Show resolved Hide resolved
tests/run/numpy_cimport_1.pyx Outdated Show resolved Hide resolved
tests/run/numpy_cimport_2.pyx Outdated Show resolved Hide resolved
tests/run/numpy_cimport_4.pyx Outdated Show resolved Hide resolved
Cython/Compiler/ParseTreeTransforms.py Outdated Show resolved Hide resolved
Co-Authored-By: Stefan Behnel <stefan_ml@behnel.de>
@da-woods
Copy link
Contributor Author

Moved as suggested. I notice that there is also import_umath and import_ufunc. My belief is those are much less generally useful (so not worth doing anything with) but I'm noting it here for completeness.

Cython/Utility/NumpyImportArray.c Outdated Show resolved Hide resolved
Cython/Includes/numpy/__init__.pxd Outdated Show resolved Hide resolved
Cython/Utility/NumpyImportArray.c Outdated Show resolved Hide resolved
Cython/Utility/NumpyImportArray.c Outdated Show resolved Hide resolved
Cython/Compiler/Nodes.py Outdated Show resolved Hide resolved
da-woods and others added 2 commits April 17, 2020 19:46
Co-Authored-By: Stefan Behnel <stefan_ml@behnel.de>
@scoder
Copy link
Contributor

scoder commented Apr 17, 2020 via email

Cython/Utility/NumpyImportArray.c Outdated Show resolved Hide resolved
Cython/Utility/NumpyImportArray.c Outdated Show resolved Hide resolved
Cython/Compiler/Nodes.py Outdated Show resolved Hide resolved
Cython/Compiler/Nodes.py Outdated Show resolved Hide resolved
Cython/Compiler/Nodes.py Outdated Show resolved Hide resolved
Cython/Compiler/Nodes.py Outdated Show resolved Hide resolved
@@ -8200,7 +8230,7 @@ def analyse_expressions(self, env):
return self

def generate_execution_code(self, code):
pass
cimport_numpy_check(self, code)


class FromCImportStatNode(StatNode):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we're missing a test for from numpy cimport ….

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes - those tests were actually failing so good catch. The FromCImportStatNode nodes were being dropped in MarkParallelAssignments - (I think unintentionally)

da-woods and others added 2 commits April 18, 2020 11:19
Co-Authored-By: Stefan Behnel <stefan_ml@behnel.de>
Also added a bit more detail to the exception.
@@ -8200,7 +8230,7 @@ def analyse_expressions(self, env):
return self

def generate_execution_code(self, code):
pass
cimport_numpy_check(self, code)


class FromCImportStatNode(StatNode):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes - those tests were actually failing so good catch. The FromCImportStatNode nodes were being dropped in MarkParallelAssignments - (I think unintentionally)

Cython/Utility/NumpyImportArray.c Show resolved Hide resolved
@scoder scoder merged commit f18e3c9 into cython:master Apr 18, 2020
@da-woods da-woods deleted the warn_numpy branch April 18, 2020 20:47
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.

None yet

2 participants