Remove usage of APIs outside the limited API #692
Conversation
vyasr
added
improvement
vyasr
force-pushed
the
fix/limited_api_usage
branch
2 times, most recently
from
40b3df0 to
52c153d
Compare
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
vyasr
force-pushed
the
fix/limited_api_usage
branch
from
52c153d to
e0b7c07
Compare
There was a problem hiding this comment.
Thank Vyas! 🙏
So my understanding is these are all macros. As a result by definition they are not part of the limited API. However they are eliminated as part of the build
So the real question is what do they do under-the-hood. Usually the answer is attribute access or assignment
Given the struct types referenced here are part of stable and limited API, I suspect whatever issue we are seeing is a tooling bug and not actually something we need to change. Would be interested in hearing more about how you are checking this and what results the tool is presenting
| @@ -125,8 +129,7 @@ cdef class Array: | |||
| self.shape_mv = None | |||
| self.strides_mv = None | |||
| else: | |||
| mv = PyMemoryView_FromObject(obj) | |||
| pybuf = PyMemoryView_GET_BUFFER(mv) | |||
| PyObject_GetBuffer(obj, &pybuf, PyBUF_FULL_RO) | |||
There was a problem hiding this comment.
If we are going to do this, it needs to be part of a try...finally... block
There was a problem hiding this comment.
What would that do? This is a C API function that does not push exceptions onto the stack. If it fails, it will return -1, so we could check the error value, but I chose not to do that in this PR since the same is true of the old functions (they could return NULL references, in which case we would have to pop the last exception).
There was a problem hiding this comment.
Sorry for the slow follow up here
Have filed a fix for this in PR: #728
Hopefully that makes it clearer what I meant
| Py_INCREF(o) | ||
| PyTuple_SET_ITEM(shape, i, o) | ||
| PyTuple_SetItem(shape, i, o) |
There was a problem hiding this comment.
Is the incref still needed here?
Same question occurs with similar lines below
There was a problem hiding this comment.
It looks like both PyTuple_SET_ITEM and PyTuple_SetItem both steal the reference. That means that 1) nothing has changed relative to the old behavior in this PR, and 2) this incref is indeed necessary since we are retaining local references to o here and without the incref we will wind up undercounting the number of references alive.
There was a problem hiding this comment.
Ok great. Thank you for checking! 🙏
Remember there being subtle behavior around stealing with these functions
Agree this seems reasonable then
|
I'm not sure I follow your logic. Just because something is a macro it doesn't mean that it will always be available in a limited api build. Many macros in cpython are conditionally defined based on whether we are compiling under the limited api or not. In this case, my test doesn't involve any tooling, I am simply instructing python to be compiled with the limited api. You can see the test in https://github.com/rapidsai/devcontainers/actions/runs/14388179703/job/40348643586. The result is a compilation failure indicating that these functions/macros are not defined when the limited api define is set. |
vyasr
dismissed
jakirkham’s stale review
Comments were addressed or responded to.
|
/merge |
Follow up on this discussion: #692 (comment) Ensure that the acquired buffer is properly released even if an exception is raised. Given we may raise an exception, this is important to check. cc @vyasr Authors: - https://github.com/jakirkham Approvers: - Vyas Ramasubramani (https://github.com/vyasr) URL: #728
I hadn't revisited kvikio's usage of CPython's functions outside the limited API since #504, and while reviving rapidsai/devcontainers#278 I found that these usages persist and are easy enough to remove.