Copilot commented on code in PR #51122:
URL: https://github.com/apache/arrow/pull/51122#discussion_r3932596902


##########
python/pyarrow/array.pxi:
##########
@@ -2269,6 +2274,51 @@ cdef class Array(_PandasConvertible):
 
         return pyarrow_wrap_array(array)
 
+    @staticmethod
+    def from_dlpack(x, /, *, device=None, copy=None):
+        """
+        Construct an Array from an object implementing the DLPack protocol.
+
+        Parameters
+        ----------
+        x : object
+            The input object containing array data, following the DLPack
+            protocol (has a ``__dlpack__`` method).
+        device : tuple[enum.Enum, int], optional
+            Designates where the resulting Array should reside, in the
+            format returned by :meth:`Array.__dlpack_device__`. When None,
+            the output Array occupies the same device as the source.
+            Default: None.
+        copy : bool, optional
+            Controls duplication behavior. True mandates copying; False
+            prohibits copying and raises ``BufferError`` if unavoidable;
+            None duplicates only when necessary. Default: None.
+
+        Returns
+        -------
+        Array
+            An Array housing the data from the input object, potentially
+            as a copy or view.
+        """
+        version = (DLPACK_VERSION.major, DLPACK_VERSION.minor)
+        pycapsule = x.__dlpack__(max_version=version, dl_device=device, 
copy=copy)
+        if not PyCapsule_CheckExact(pycapsule):
+            raise TypeError("DLPack producer did not return a PyCapsule")
+        cdef DLManagedTensorVersioned* ptr = 
<DLManagedTensorVersioned*>PyCapsule_GetPointer(
+            pycapsule, "dltensor_versioned")
+        if ptr == NULL:
+            raise ValueError(
+                'DLPack producer did not produce a "dltensor_versioned" 
PyCapsule')
+        # Mark the capsule as consumed so its destructor does not also invoke 
the deleter.
+        # ImportArrayVersionedFromDLPack will take ownership even if it errors 
(calling
+        # the deleter in that case).
+        PyCapsule_SetName(pycapsule, "used_dltensor_versioned")
+        with nogil:
+            # Copy handled on producer side
+            result = ImportArrayVersionedFromDLPack(ptr, False)
+        carray = GetResultValue(result)

Review Comment:
   In `Array.from_dlpack()`, `result` is assigned inside a `with nogil:` block 
but isn’t declared as a Cython cdef variable, which will fail to compile (and 
can’t use a Python variable without the GIL). Also check `PyCapsule_SetName` 
for failure; if it fails and you proceed, the capsule destructor may still call 
the DLPack deleter, risking double-free.



##########
python/pyarrow/tensor.pxi:
##########
@@ -300,7 +306,52 @@ strides: {self.strides}"""
         buffer.strides = <Py_ssize_t *> 
cp.PyBytes_AsString(self._ssize_t_strides)
         buffer.suboffsets = NULL
 
-    def __dlpack__(self, stream=None, max_version=None, dl_device=None, 
copy=None):
+    @staticmethod
+    def from_dlpack(x, /, *, device=None, copy=None):
+        """
+        Construct a Tensor from an object implementing the DLPack protocol.
+
+        Parameters
+        ----------
+        x : object
+            The input object containing array data, following the DLPack
+            protocol (has a ``__dlpack__`` method).
+        device : tuple[enum.Enum, int], optional
+            Designates where the resulting Tensor should reside, in the
+            format returned by :meth:`Tensor.__dlpack_device__`. When None,
+            the output Tensor occupies the same device as the source.
+            Default: None.
+        copy : bool, optional
+            Controls duplication behavior. True mandates copying; False
+            prohibits copying and raises ``BufferError`` if unavoidable;
+            None duplicates only when necessary. Default: None.
+
+        Returns
+        -------
+        Tensor
+            A Tensor housing the data from the input object, potentially
+            as a copy or view.
+        """
+        version = (DLPACK_VERSION.major, DLPACK_VERSION.minor)
+        pycapsule = x.__dlpack__(max_version=version, dl_device=device, 
copy=copy)
+        if not PyCapsule_CheckExact(pycapsule):
+            raise TypeError("DLPack producer did not return a PyCapsule")
+        cdef DLManagedTensorVersioned* ptr = 
<DLManagedTensorVersioned*>PyCapsule_GetPointer(
+            pycapsule, "dltensor_versioned")
+        if ptr == NULL:
+            raise ValueError(
+                'DLPack producer did not produce a "dltensor_versioned" 
PyCapsule')
+        # Mark the capsule as consumed so its destructor does not also invoke 
the deleter.
+        # ImportTensorVersionedFromDLPack will take ownership even if it 
errors (calling
+        # the deleter in that case).
+        PyCapsule_SetName(pycapsule, "used_dltensor_versioned")
+        with nogil:
+            # Copy handled on producer side
+            result = ImportTensorVersionedFromDLPack(ptr, False)
+        ctensor = GetResultValue(result)

Review Comment:
   In `Tensor.from_dlpack()`, `result` is assigned inside a `with nogil:` block 
but isn’t declared as a Cython cdef variable, which will fail to compile (and 
can’t use a Python variable without the GIL). Also check `PyCapsule_SetName` 
for failure; if it fails and you proceed, the capsule destructor may still call 
the DLPack deleter, risking double-free.



##########
python/pyarrow/tests/test_dlpack.py:
##########
@@ -339,3 +339,81 @@ def test_dlpack_cuda_not_supported():
     with pytest.raises(NotImplementedError, match="DLPack support is 
implemented "
                        "only for buffers on CPU device."):
         carr.__dlpack_device__()
+
+
+@check_bytes_allocated
[email protected]('np_type',
+                         [np.uint8, np.uint16, np.uint32, np.uint64,
+                          np.int8, np.int16, np.int32, np.int64,
+                          np.float16, np.float32, np.float64])
+def test_tensor_from_dlpack(np_type):
+    if Version(np.__version__) < Version("2.1.0"):
+        pytest.skip("Versioned DLPack capsules require numpy 2.1.0 or later")
+
+    # Non-contiguous, strided slice: DLPack carries explicit strides, so this
+    # should not need a copy on export.

Review Comment:
   Comment says “copy on export” but this test is exercising *import* via 
`from_dlpack`.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to