MAINT: Only warn for transferred ownership if env variable is set#20200
Merged
seberg merged 4 commits intonumpy:mainfrom Oct 27, 2021
Merged
MAINT: Only warn for transferred ownership if env variable is set#20200seberg merged 4 commits intonumpy:mainfrom
seberg merged 4 commits intonumpy:mainfrom
Conversation
mattip
force-pushed
the
unbreak-pythran
branch
2 times, most recently
from
41b8ab4 to
10cb75e
Compare
Member
Author
|
My confidence in using a |
mattip
force-pushed
the
unbreak-pythran
branch
from
10cb75e to
d02ded3
Compare
mattip
force-pushed
the
unbreak-pythran
branch
from
d02ded3 to
27106b0
Compare
mattip
force-pushed
the
unbreak-pythran
branch
from
6d2a533 to
865d168
Compare
Member
Author
|
Added a test, which led me to correct the code example. |
Member
Author
|
The new test is passing and so is the pythran code. |
seberg
approved these changes
Oct 27, 2021
Member
seberg
left a comment
seberg
left a comment
There was a problem hiding this comment.
Thanks looks good, basically just super nits and maybe a clarification. If we want to cover all ground, we could add this to: https://numpy.org/doc/stable/reference/global_state.html
We could also mention the trick in the documentation for NPY_ARRAY_OWNDATA and for the heck of it, just stumbled on these stackoverflow posts telling people to enable the flag...
| "is not set. If you take ownership of the data, you must " | ||
| "also set a memory policy."; | ||
| WARN_IN_DEALLOC(PyExc_RuntimeWarning, msg); | ||
| char *env = getenv("NUMPY_WARN_IF_NO_MEM_POLICY"); |
Member
There was a problem hiding this comment.
I guess it is fine to check this in free itself, considering that the whole branch should be very rare?
(If it was, there is also a helper here to toggle the "state" of HUGEPAGES).
Member
Author
There was a problem hiding this comment.
worst case the warning will be swallowed.
Member
Author
|
I cleaned up and added more documentation around the use of OWNDATA and NUMPY_WARN_IF_NO_MEM_POLICY |
seberg
changed the title
seberg
approved these changes
Oct 27, 2021
Member
seberg
left a comment
seberg
left a comment
There was a problem hiding this comment.
Thanks a lot for those extra docs! I will put in once tests pass, I think we can always improve docs now if questions arise.
|
|
||
| Some users might pass ownership of the data pointer to the ``ndarray`` by | ||
| setting the ``OWNDATA`` flag. If they do this without setting (manually) a | ||
| memory allocation policy, the default will be to call ``free``. If |
Member
There was a problem hiding this comment.
Still not sure how users would set the allocation policy manually, if we allow that it would be another route they can take?
Member
Author
There was a problem hiding this comment.
fa->mem_handler = something_cool;
Member
There was a problem hiding this comment.
OK, that requires using the deprecated API, but it is possible. I was not sure we wanted people to allow to actually write to that slot.
(But if we would not, we should add least add an underscore, I guess)
Co-authored-by: Sebastian Berg <[email protected]>
Member
|
Thanks Matti! |
seberg
reviewed
Oct 29, 2021
| "also set a memory policy."; | ||
| WARN_IN_DEALLOC(PyExc_RuntimeWarning, msg); | ||
| char *env = getenv("NUMPY_WARN_IF_NO_MEM_POLICY"); | ||
| if ((env == NULL) || (strncmp(env, "1", 1) == 0)) { |
Member
There was a problem hiding this comment.
Ooopst, should have been env != NULL && :/
saimn
added a commit
to saimn/astropy
that referenced
this pull request
Nov 9, 2022
Fix astropy#12324 : Fix the warnings emitted by Numpy when transferring ownership by setting ``NPY_ARRAY_OWNDATA``: RuntimeWarning: Trying to dealloc data, but a memory policy is not set. If you take ownership of the data, you must set a base owning the data (e.g. a PyCapsule). The warning was added in [1], and is now visible only when setting the NUMPY_WARN_IF_NO_MEM_POLICY environment variable [2]. For more details and a fix see [3]. [1] numpy/numpy#17582 [2] numpy/numpy#20200 [3] https://github.com/numpy/numpy/blob/main/doc/source/reference/c-api/data_memory.rst#what-happens-when-deallocating-if-there-is-no-policy-set
saimn
added a commit
to saimn/astropy
that referenced
this pull request
Nov 15, 2022
Fix astropy#12324 : Fix the warnings emitted by Numpy when transferring ownership by setting ``NPY_ARRAY_OWNDATA``: RuntimeWarning: Trying to dealloc data, but a memory policy is not set. If you take ownership of the data, you must set a base owning the data (e.g. a PyCapsule). The warning was added in [1], and is now visible only when setting the NUMPY_WARN_IF_NO_MEM_POLICY environment variable [2]. For more details and a fix see [3]. [1] numpy/numpy#17582 [2] numpy/numpy#20200 [3] https://github.com/numpy/numpy/blob/main/doc/source/reference/c-api/data_memory.rst#what-happens-when-deallocating-if-there-is-no-policy-set
saimn
added a commit
to saimn/astropy
that referenced
this pull request
Nov 18, 2022
Fix astropy#12324 : Fix the warnings emitted by Numpy when transferring ownership by setting ``NPY_ARRAY_OWNDATA``: RuntimeWarning: Trying to dealloc data, but a memory policy is not set. If you take ownership of the data, you must set a base owning the data (e.g. a PyCapsule). The warning was added in [1], and is now visible only when setting the NUMPY_WARN_IF_NO_MEM_POLICY environment variable [2]. For more details and a fix see [3]. [1] numpy/numpy#17582 [2] numpy/numpy#20200 [3] https://github.com/numpy/numpy/blob/main/doc/source/reference/c-api/data_memory.rst#what-happens-when-deallocating-if-there-is-no-policy-set
saimn
added a commit
to saimn/astropy
that referenced
this pull request
Dec 11, 2022
Fix astropy#12324 : Fix the warnings emitted by Numpy when transferring ownership by setting ``NPY_ARRAY_OWNDATA``: RuntimeWarning: Trying to dealloc data, but a memory policy is not set. If you take ownership of the data, you must set a base owning the data (e.g. a PyCapsule). The warning was added in [1], and is now visible only when setting the NUMPY_WARN_IF_NO_MEM_POLICY environment variable [2]. For more details and a fix see [3]. [1] numpy/numpy#17582 [2] numpy/numpy#20200 [3] https://github.com/numpy/numpy/blob/main/doc/source/reference/c-api/data_memory.rst#what-happens-when-deallocating-if-there-is-no-policy-set
saimn
added a commit
to saimn/astropy
that referenced
this pull request
Dec 11, 2022
Fix astropy#12324 : Fix the warnings emitted by Numpy when transferring ownership by setting ``NPY_ARRAY_OWNDATA``: RuntimeWarning: Trying to dealloc data, but a memory policy is not set. If you take ownership of the data, you must set a base owning the data (e.g. a PyCapsule). The warning was added in [1], and is now visible only when setting the NUMPY_WARN_IF_NO_MEM_POLICY environment variable [2]. For more details and a fix see [3]. [1] numpy/numpy#17582 [2] numpy/numpy#20200 [3] https://github.com/numpy/numpy/blob/main/doc/source/reference/c-api/data_memory.rst#what-happens-when-deallocating-if-there-is-no-policy-set
saimn
added a commit
to saimn/astropy
that referenced
this pull request
Dec 13, 2022
Fix astropy#12324 : Fix the warnings emitted by Numpy when transferring ownership by setting ``NPY_ARRAY_OWNDATA``: RuntimeWarning: Trying to dealloc data, but a memory policy is not set. If you take ownership of the data, you must set a base owning the data (e.g. a PyCapsule). The warning was added in [1], and is now visible only when setting the NUMPY_WARN_IF_NO_MEM_POLICY environment variable [2]. For more details and a fix see [3]. [1] numpy/numpy#17582 [2] numpy/numpy#20200 [3] https://github.com/numpy/numpy/blob/main/doc/source/reference/c-api/data_memory.rst#what-happens-when-deallocating-if-there-is-no-policy-set
dougbrn
pushed a commit
to dougbrn/astropy
that referenced
this pull request
Mar 13, 2023
Fix astropy#12324 : Fix the warnings emitted by Numpy when transferring ownership by setting ``NPY_ARRAY_OWNDATA``: RuntimeWarning: Trying to dealloc data, but a memory policy is not set. If you take ownership of the data, you must set a base owning the data (e.g. a PyCapsule). The warning was added in [1], and is now visible only when setting the NUMPY_WARN_IF_NO_MEM_POLICY environment variable [2]. For more details and a fix see [3]. [1] numpy/numpy#17582 [2] numpy/numpy#20200 [3] https://github.com/numpy/numpy/blob/main/doc/source/reference/c-api/data_memory.rst#what-happens-when-deallocating-if-there-is-no-policy-set
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Builds on #20194. Fixes breakage of SciPy in scipy/scipy#14917
At some point we could flip the default to "warn" instead of "no warning"