Skip to content

Remove usage of APIs outside the limited API #692

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

Merged
merged 3 commits into from
Apr 28, 2025
Merged

Remove usage of APIs outside the limited API #692

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e0b7c07
Limited API only
vyasr Apr 10, 2025
d212c47
Merge branch 'branch-25.06' into fix/limited_api_usage
vyasr Apr 10, 2025
b942226
Merge branch 'branch-25.06' into fix/limited_api_usage
vyasr Apr 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 15 additions & 11 deletions python/kvikio/kvikio/_lib/arr.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,18 @@
# Copyright (c) 2020-2024, NVIDIA CORPORATION. All rights reserved.
# Copyright (c) 2020-2025, NVIDIA CORPORATION. All rights reserved.
# See file LICENSE for terms.

# cython: language_level=3


from cpython.buffer cimport PyBuffer_IsContiguous
from cpython.buffer cimport (
PyBUF_FULL_RO,
PyBuffer_IsContiguous,
PyBuffer_Release,
PyObject_GetBuffer,
)
from cpython.mem cimport PyMem_Free, PyMem_Malloc
from cpython.memoryview cimport PyMemoryView_FromObject, PyMemoryView_GET_BUFFER
from cpython.ref cimport Py_INCREF
from cpython.tuple cimport PyTuple_New, PyTuple_SET_ITEM
from cpython.tuple cimport PyTuple_New, PyTuple_SetItem
from cython cimport auto_pickle, boundscheck, initializedcheck, nonecheck, wraparound
from cython.view cimport array
from libc.stdint cimport uintptr_t
Expand Down Expand Up @@ -75,7 +79,7 @@ cdef class Array:
def __cinit__(self, obj):
cdef dict iface = getattr(obj, "__cuda_array_interface__", None)
self.cuda = (iface is not None)
cdef const Py_buffer* pybuf
cdef Py_buffer pybuf
cdef str typestr
cdef tuple data, shape, strides
cdef Py_ssize_t i
Expand Down Expand Up @@ -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)
Copy link
Member

@jakirkham jakirkham Apr 11, 2025

Choose a reason for hiding this comment

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

If we are going to do this, it needs to be part of a try...finally... block

Copy link
Contributor Author

@vyasr vyasr Apr 15, 2025

Choose a reason for hiding this comment

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

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

Copy link
Member

@jakirkham jakirkham May 22, 2025

Choose a reason for hiding this comment

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

Sorry for the slow follow up here

Have filed a fix for this in PR: #728

Hopefully that makes it clearer what I meant


if pybuf.suboffsets != NULL:
raise NotImplementedError("Suboffsets are not supported")
Expand All @@ -144,7 +147,7 @@ cdef class Array:
pybuf.shape,
self.ndim * sizeof(Py_ssize_t)
)
if not PyBuffer_IsContiguous(pybuf, b"C"):
if not PyBuffer_IsContiguous(&pybuf, b"C"):
self.strides_mv = new_Py_ssize_t_array(self.ndim)
memcpy(
&self.strides_mv[0],
Expand All @@ -156,6 +159,7 @@ cdef class Array:
else:
self.shape_mv = None
self.strides_mv = None
PyBuffer_Release(&pybuf)

cpdef bint _c_contiguous(self):
return _c_contiguous(
Expand Down Expand Up @@ -203,7 +207,7 @@ cdef class Array:
for i in range(self.ndim):
o = self.shape_mv[i]
Py_INCREF(o)
PyTuple_SET_ITEM(shape, i, o)
PyTuple_SetItem(shape, i, o)
Comment on lines 209 to +210
Copy link
Member

@jakirkham jakirkham Apr 11, 2025

Choose a reason for hiding this comment

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

Is the incref still needed here?

Same question occurs with similar lines below

Copy link
Contributor Author

@vyasr vyasr Apr 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@jakirkham jakirkham May 22, 2025

Choose a reason for hiding this comment

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

Ok great. Thank you for checking! 🙏

Remember there being subtle behavior around stealing with these functions

Agree this seems reasonable then

return shape

@property
Expand All @@ -219,13 +223,13 @@ cdef class Array:
for i from self.ndim > i >= 0 by 1:
o = self.strides_mv[i]
Py_INCREF(o)
PyTuple_SET_ITEM(strides, i, o)
PyTuple_SetItem(strides, i, o)
else:
s = self.itemsize
for i from self.ndim > i >= 0 by 1:
o = s
Py_INCREF(o)
PyTuple_SET_ITEM(strides, i, o)
PyTuple_SetItem(strides, i, o)
s *= self.shape_mv[i]
return strides

Expand Down