BUG: array interface PyCapsule reference #20674
Conversation
|
there is a small self contained example I've used for testing this change here |
numpy/core/src/multiarray/ctors.c
Outdated
| /* a tuple to hold references */ | ||
| PyObject *refs = PyTuple_New(2); | ||
| if (!refs) { | ||
| PyErr_SetString(PyExc_MemoryError, "Failed to allocate a Tuple of size 2"); |
There was a problem hiding this comment.
The error should already be set, and since it is an allocation error the less done after that the better.
| PyErr_SetString(PyExc_MemoryError, "Failed to allocate a Tuple of size 2"); |
There was a problem hiding this comment.
Ah, OK. Thanks for pointing this out. I made the suggested change.
|
What is printed for the |
|
The sample seems small enough it could be added as a test, using testing.extbuild to build a c-extension. This should probably be marked with |
With this change numpy holds a reference to the PyCapsule by which the PyArrayInterface is returned in addition to the object which supplied it. This lets numpy signal (via the PyCapsule destructor) when it is finished with the data, which may not be managed by the object which provided the PyCapsule instance. The old behavior of holding a reference only to the object which provided the PyCapsule is kept to ensure backward compatibility. closes numpygh-20673
5ed9183 to
a2d854b
Compare
before: after: The above output was obtained from the example I linked to above.
Thanks for the pointers. I added a simplified test based on the example. The test is in a second commit to make it easy to experiment before/after the bug fix patch. |
The test exercises data shared through the c-struct method of the array interface protocol.
a2d854b to
5fa7168
Compare
|
Cool. The extension building code is rather new, so suggestions to make the workflow easier would be appreciated. |
| PyObject *ret = PyArray_NewFromDescrAndBase( | ||
| &PyArray_Type, thetype, | ||
| inter->nd, inter->shape, inter->strides, inter->data, | ||
| inter->flags, NULL, input); | ||
| Py_DECREF(attr); | ||
| inter->flags, NULL, refs); |
There was a problem hiding this comment.
(Not much of an opinion on whether this should be backported). But what do you think about my proposal to change this to:
ctx = PyCapsule_GetContext(attr)
if (ctx == NULL && PyErr_Occurred()) {
/* error cleanup */
return NULL;
}
/* ctx should not be NULL, but do not rely on it for now: */
ref = ctx != NULL ? ctx : input;I feel like that may be a nice way to achieve the same thing, and makes me more comfortable with the change.
The copy=NEVER champions could look into adding something a ctx == input check if desired.
There was a problem hiding this comment.
what do you think about my proposal to change this to
Two things come to mind.
Note that the type of the PyCapsule's context field is void *, and that the base argument to PyArray_NewFromDescrAndBase is type PyObject*.
numpy/numpy/core/src/multiarray/ctors.c
Lines 948 to 951 in 1a0d1fb
The proposed change would make memory management of resources other than PyObject's more complicated because it would require the capsule's context field be a PyObject but in data shared with numpy from C/C++ the most common use case would be management of temporary buffers allocated with new or malloc. See the self contained example I've been using for testing linked to above for an example. With the proposed change, the capsule's context field could no longer be used to track such resources directly, and the capsule's destructor could no longer be used to directly release them. Such resources would instead need to be managed by a secondary object. This could be accomplished, by setting the context field to a second PyCapsule, but it is more complicated to do it that way.
There's also a potential type safety issue introduced here because thePyCapsule's context is type void * . It would be easy to misuse this design by inadvertently passing something other than a PyObject as the context. Such accidental misuse would lead to an invalid memory access, and would potentially crash with a segv. There's not a safe way in C to check if the memory pointed to by the void* context is in fact a PyObject* before numpy uses it. One could argue that it's the responsibility of the sharer of the data, but it's worth noting that this issue isn't present in the original proposal.
I don't know that these two things are necessarily show stoppers for your suggestion, but they might be worth considering.
There was a problem hiding this comment.
Note that the type of the PyCapsule's context field is void , and that the base argument to PyArray_NewFromDescrAndBase is type PyObject.
Hmm, missed that. I guess the docs specify it must be an object, but this was never really enforced, so it is unclear that we should do it.
I can also see the point that it would limit your flexibility compared to this solution, have to think a bit more about this, but if others want to move forward, please do not worry too much... I suppose in the end we have "flags" we can use to tag on new information to add new information (not ideal but OK).
A small problem would be if there are consumers other than NumPy, but that would only affect your type actually using this.
|
In the latest developer meeting we discussed:
@seberg correct me if I misrepresented the discussion. The bottom line is to polish any rough edges in this PR and accept it. |
|
Yes, I still don't love it, but that is fine :). I am unsure if we should backport this, as I think it largly extends the protocol rather than fixing a bug per-se and it changes For "rough edges" beyond my "why :)?": It would be nice to just use |
@seberg Could you expand on this? |
|
@charris: before this PR, |
|
Previously, if I had an object that had an array struct (say JAX implemented it, though they do not): Then, before: while now we will have: |
Ah, so |
|
Yes, it would export a capsule containing the same information that This makes the protocol safer, since before passing around an I agree with this, and I am starting to agree that from my "long term" thinking, I just shouldn't care about this :). Long term |
|
Thanks @burlen |
|
Next thing to do is to deprecate |
|
I marked for backport, but the changed return type worries me. Is there a good reason to backport this, or should it wait on 1.23? |
|
I tend to not backporting, I view this more as an enhancement rather than a bug-fix to begin with. |
|
I agree with @seberg |
|
Thanks. I unmarked it. |
With this change numpy holds a reference to the PyCapsule by which the
PyArrayInterface is returned in addition to the object which supplied
it. This lets numpy signal (via the PyCapsule destructor) when it is
finished with the data, which may not be managed by the object which
provided the PyCapsule instance. The old behavior of holding a reference
only to the object which provided the PyCapsule is kept to ensure backward
compatibility.
closes gh-20673