Skip to content

MAINT: Only warn for transferred ownership if env variable is set #20200

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 4 commits into from
Oct 27, 2021
Merged

MAINT: Only warn for transferred ownership if env variable is set #20200

merged 4 commits into from
Oct 27, 2021

Conversation

mattip
Copy link
Member

@mattip mattip commented Oct 26, 2021

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"

@mattip mattip force-pushed the unbreak-pythran branch 2 times, most recently from 41b8ab4 to 10cb75e Compare October 26, 2021 21:51
@mattip
Copy link
Member Author

mattip commented Oct 26, 2021

My confidence in using a PyCapsule is shaken by my attempt to use this concept in pythran. The PR there is failing. Maybe we should add another test to prove it works

@mattip
Copy link
Member Author

mattip commented Oct 27, 2021

Added a test, which led me to correct the code example.

@mattip
Copy link
Member Author

mattip commented Oct 27, 2021

The new test is passing and so is the pythran code.

seberg
seberg approved these changes Oct 27, 2021
View reviewed changes
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.

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...

numpy/core/src/multiarray/arrayobject.c
"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");
Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

Choose a reason for hiding this comment

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

worst case the warning will be swallowed.

@mattip
Copy link
Member Author

mattip commented Oct 27, 2021

I cleaned up and added more documentation around the use of OWNDATA and NUMPY_WARN_IF_NO_MEM_POLICY

@seberg seberg changed the title make warning conditional on NUMPY_WARN_IF_NO_MEM_POLICY MAINT: Only warn for transferred ownership if env variable is set Oct 27, 2021
seberg
seberg approved these changes Oct 27, 2021
View reviewed changes
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.

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.

doc/source/reference/global_state.rst

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
Copy link
Member

Choose a reason for hiding this comment

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

Still not sure how users would set the allocation policy manually, if we allow that it would be another route they can take?

Copy link
Member Author

Choose a reason for hiding this comment

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

fa->mem_handler = something_cool;

Copy link
Member

@seberg seberg Oct 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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)

@mattip @seberg
typo
3744930 
Co-authored-by: Sebastian Berg <sebastian@sipsolutions.net>
@seberg seberg merged commit aebf386 into numpy:main Oct 27, 2021
@seberg
Copy link
Member

seberg commented Oct 27, 2021

Thanks Matti!

@rgommers rgommers mentioned this pull request Oct 29, 2021
Closed
seberg
seberg reviewed Oct 29, 2021
View reviewed changes
numpy/core/src/multiarray/arrayobject.c
"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)) {
Copy link
Member

Choose a reason for hiding this comment

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

Ooopst, should have been env != NULL && :/

@seberg seberg mentioned this pull request Oct 29, 2021
Merged
saimn added a commit to saimn/astropy that referenced this pull request Nov 9, 2022
@saimn
Fix memory policy warnings
cdff855 
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
@saimn
Fix memory policy warnings
3340b48 
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
@saimn
Fix memory policy warnings
506681c 
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
@saimn
Fix memory policy warnings
f3298a7 
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
@saimn
Fix memory policy warnings
5fb0787 
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 saimn mentioned this pull request Dec 11, 2022
Merged
saimn added a commit to saimn/astropy that referenced this pull request Dec 13, 2022
@saimn
Fix memory policy warnings
32a80a1 
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
@mattip mattip deleted the unbreak-pythran branch December 27, 2022 16:49
dougbrn pushed a commit to dougbrn/astropy that referenced this pull request Mar 13, 2023
@saimn @dougbrn
Fix memory policy warnings
fb6c5a0 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seberg seberg seberg approved these changes

Assignees
No one assigned
Labels
03 - Maintenance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mattip @seberg