Copilot commented on code in PR #50827:
URL: https://github.com/apache/arrow/pull/50827#discussion_r3729234236
##########
cpp/src/arrow/c/dlpack_abi.h:
##########
@@ -224,11 +258,23 @@ typedef struct {
int32_t ndim;
/*! \brief The data type of the pointer*/
DLDataType dtype;
- /*! \brief The shape of the tensor */
+ /*!
+ * \brief The shape of the tensor
+ *
+ * When ndim == 0, shape can be set to NULL.
+ */
int64_t* shape;
/*!
- * \brief strides of the tensor (in number of elements, not bytes)
- * can be NULL, indicating tensor is compact and row-majored.
+ * \brief strides of the tensor (in number of elements, not bytes),
+ * can not be NULL if ndim != 0, must points to
+ * an array of ndim elements that specifies the strides,
+ * so consumer can always rely on strides[dim] being valid for 0 <= dim <
ndim.
+ *
Review Comment:
DLPack v1.2+ requires `strides` to be non-NULL when `ndim != 0`, but Arrow’s
current DLPack Array exporter still sets `dl_tensor.strides = NULL` (see
`cpp/src/arrow/c/dlpack.cc:97`). After bumping this header to v1.3, consumers
may assume `strides[dim]` is always valid and crash or mis-handle Arrow
exports. Please update the exporter to always provide a strides array for
arrays (e.g., for 1D contiguous data set stride[0] = 1 and store it in the
manager ctx so the pointer stays valid).
##########
cpp/src/arrow/c/dlpack_abi.h:
##########
@@ -309,11 +371,279 @@ struct DLManagedTensorVersioned {
* stable, to ensure that deleter can be correctly called.
*
* \sa DLPACK_FLAG_BITMASK_READ_ONLY
+ * \sa DLPACK_FLAG_BITMASK_IS_COPIED
*/
uint64_t flags;
/*! \brief DLTensor which is being memory managed */
DLTensor dl_tensor;
-};
+} DLManagedTensorVersioned;
+
+//----------------------------------------------------------------------
+// DLPack `__dlpack_c_exchange_api__` fast exchange protocol definitions
+//----------------------------------------------------------------------
+/*!
+ * \brief Request a producer library to create a new tensor.
+ *
+ * Create a new `DLManagedTensorVersioned` within the context of the producer
+ * library. The allocation is defined via the prototype DLTensor.
+ *
+ * This function is exposed by the framework through the DLPackExchangeAPI.
+ *
+ * \param prototype The prototype DLTensor. Only the dtype, ndim, shape,
+ * and device fields are used.
+ * \param out The output DLManagedTensorVersioned.
+ * \param error_ctx Context for `SetError`.
+ * \param SetError The function to set the error.
+ * \return The owning DLManagedTensorVersioned* or NULL on failure.
+ * SetError is called exactly when NULL is returned (the implementer
+ * must ensure this).
+ * \note - As a C function, must not thrown C++ exceptions.
+ * - Error propagation via SetError to avoid any direct need
+ * of Python API. Due to this `SetError` may have to ensure the GIL is
+ * held since it will presumably set a Python error.
+ *
+ * \sa DLPackExchangeAPI
+ */
+typedef int (*DLPackManagedTensorAllocator)(
//
+ DLTensor* prototype, DLManagedTensorVersioned** out, void* error_ctx,
//
+ void (*SetError)(void* error_ctx, const char* kind, const char* message)
//
+);
+
+/*!
+ * \brief Exports a PyObject* Tensor/NDArray to a DLManagedTensorVersioned.
+ *
+ * This function does not perform any stream synchronization. The consumer
should query
+ * DLPackCurrentWorkStream to get the current work stream and launch kernels
on it.
+ *
+ * This function is exposed by the framework through the DLPackExchangeAPI.
+ *
+ * \param py_object The Python object to convert. Must have the same type
+ * as the one the `DLPackExchangeAPI` was discovered from.
+ * \param out The output DLManagedTensorVersioned.
+ * \return The owning DLManagedTensorVersioned* or NULL on failure with a
+ * Python exception set. If the data cannot be described using DLPack
+ * this should be a BufferError if possible.
+ * \note - As a C function, must not thrown C++ exceptions.
+ *
+ * \sa DLPackExchangeAPI, DLPackCurrentWorkStream
+ */
+typedef int (*DLPackManagedTensorFromPyObjectNoSync)( //
+ void* py_object, //
+ DLManagedTensorVersioned** out //
+);
+
+/*!
+ * \brief Exports a PyObject* Tensor/NDArray to a provided DLTensor.
+ *
+ * This function provides a faster interface for temporary, non-owning,
exchange.
+ * The producer (implementer) still owns the memory of data, strides, shape.
+ * The liveness of the DLTensor and the data it views is only guaranteed until
+ * control is returned.
+ *
+ * This function currently assumes that the producer (implementer) can fill
+ * in the DLTensor shape and strides without the need for temporary
allocations.
+ *
+ * This function does not perform any stream synchronization. The consumer
should query
+ * DLPackCurrentWorkStream to get the current work stream and launch kernels
on it.
+ *
+ * This function is exposed by the framework through the DLPackExchangeAPI.
+ *
+ * \param py_object The Python object to convert. Must have the same type
+ * as the one the `DLPackExchangeAPI` was discovered from.
+ * \param out The output DLTensor, whose space is pre-allocated on stack.
+ * \return 0 on success, -1 on failure with a Python exception set.
+ * \note - As a C function, must not thrown C++ exceptions.
+ *
+ * \sa DLPackExchangeAPI, DLPackCurrentWorkStream
+ */
+typedef int (*DLPackDLTensorFromPyObjectNoSync)( //
+ void* py_object, //
+ DLTensor* out //
+);
+
+/*!
+ * \brief Obtain the current work stream of a device.
+ *
+ * Obtain the current work stream of a device from the producer framework.
+ * For example, it should map to torch.cuda.current_stream in PyTorch.
+ *
+ * When device_type is kDLCPU, the consumer do not have to query the stream
+ * and the producer can simply return NULL when queried.
+ * The consumer do not have to do anything on stream sync or setting.
+ * So CPU only framework can just provide a dummy implementation that
+ * always set out_current_stream[0] to NULL.
+ *
+ * \param device_type The device type.
+ * \param device_id The device id.
+ * \param out_current_stream The output current work stream.
+ *
+ * \return 0 on success, -1 on failure with a Python exception set.
+ * \note - As a C function, must not thrown C++ exceptions.
+ *
+ * \sa DLPackExchangeAPI
+ */
+typedef int (*DLPackCurrentWorkStream)( //
+ DLDeviceType device_type, //
+ int32_t device_id, //
+ void** out_current_stream //
+);
+
+/*!
+ * \brief Imports a DLManagedTensorVersioned to a PyObject* Tensor/NDArray.
+ *
+ * Convert an owning DLManagedTensorVersioned* to the Python tensor of the
+ * producer (implementer) library with the correct type.
+ *
+ * This function does not perform any stream synchronization.
+ *
+ * This function is exposed by the framework through the DLPackExchangeAPI.
+ *
+ * \param tensor The DLManagedTensorVersioned to convert the ownership of the
+ * tensor is stolen.
+ * \param out_py_object The output Python object.
+ * \return 0 on success, -1 on failure with a Python exception set.
+ *
+ * \sa DLPackExchangeAPI
+ */
+typedef int (*DLPackManagedTensorToPyObjectNoSync)( //
+ DLManagedTensorVersioned* tensor, //
+ void** out_py_object //
+);
+
+/*!
+ * \brief DLPackExchangeAPI stable header.
+ * \sa DLPackExchangeAPI
+ */
+typedef struct DLPackExchangeAPIHeader {
+ /*!
+ * \brief The provided DLPack version the consumer must check major version
+ * compatibility before using this struct.
+ */
+ DLPackVersion version;
+ /*!
+ * \brief Optional pointer to an older DLPackExchangeAPI in the chain.
+ *
+ * It must be NULL if the framework does not support older versions.
+ * If the current major version is larger than the one supported by the
+ * consumer, the consumer may walk this to find an earlier supported version.
+ *
+ * \sa DLPackExchangeAPI
+ */
+ struct DLPackExchangeAPIHeader* prev_api;
+} DLPackExchangeAPIHeader;
+
+/*!
+ * \brief Framework-specific function pointers table for DLPack exchange.
+ *
+ * Additionally to `__dlpack__()` we define a C function table sharable by
+ *
+ * Python implementations via `__dlpack_c_exchange_api__`.
+ * This attribute must be set on the type as a Python PyCapsule
+ * with name "dlpack_exchange_api".
+ *
+ * A consumer library may use a pattern such as:
+ *
+ * \code
+ *
+ * PyObject *api_capsule = PyObject_GetAttrString(
+ * (PyObject *)Py_TYPE(tensor_obj), "__dlpack_c_exchange_api__")
+ * );
+ * if (api_capsule == NULL) { goto handle_error; }
+ * MyDLPackExchangeAPI *api = (MyDLPackExchangeAPI *)PyCapsule_GetPointer(
+ * api_capsule, "dlpack_exchange_api"
+ * );
+ * Py_DECREF(api_capsule);
+ * if (api == NULL) { goto handle_error; }
+ *
+ * \endcode
+ *
+ * Note that this must be defined on the type. The consumer should look up the
+ * attribute on the type and may cache the result for each unique type.
+ *
+ * The precise API table is given by:
+ * \code
+ * struct MyDLPackExchangeAPI : public DLPackExchangeAPI {
+ * MyDLPackExchangeAPI() {
+ * header.version.major = DLPACK_MAJOR_VERSION;
+ * header.version.minor = DLPACK_MINOR_VERSION;
+ * header.prev_version_api = nullptr;
Review Comment:
The example initialization uses `header.prev_version_api`, but the
`DLPackExchangeAPIHeader` field is named `prev_api`. As written, the example
won’t compile.
##########
cpp/src/arrow/c/dlpack_abi.h:
##########
@@ -309,11 +371,279 @@ struct DLManagedTensorVersioned {
* stable, to ensure that deleter can be correctly called.
*
* \sa DLPACK_FLAG_BITMASK_READ_ONLY
+ * \sa DLPACK_FLAG_BITMASK_IS_COPIED
*/
uint64_t flags;
/*! \brief DLTensor which is being memory managed */
DLTensor dl_tensor;
-};
+} DLManagedTensorVersioned;
+
+//----------------------------------------------------------------------
+// DLPack `__dlpack_c_exchange_api__` fast exchange protocol definitions
+//----------------------------------------------------------------------
+/*!
+ * \brief Request a producer library to create a new tensor.
+ *
+ * Create a new `DLManagedTensorVersioned` within the context of the producer
+ * library. The allocation is defined via the prototype DLTensor.
+ *
+ * This function is exposed by the framework through the DLPackExchangeAPI.
+ *
+ * \param prototype The prototype DLTensor. Only the dtype, ndim, shape,
+ * and device fields are used.
+ * \param out The output DLManagedTensorVersioned.
+ * \param error_ctx Context for `SetError`.
+ * \param SetError The function to set the error.
+ * \return The owning DLManagedTensorVersioned* or NULL on failure.
+ * SetError is called exactly when NULL is returned (the implementer
+ * must ensure this).
+ * \note - As a C function, must not thrown C++ exceptions.
Review Comment:
The documentation for `DLPackManagedTensorAllocator` says it returns a
`DLManagedTensorVersioned*`/NULL, but the typedef returns `int` and delivers
the tensor via the `out` parameter. This mismatch is likely to confuse
implementers of the exchange API.
This issue also appears on line 423 of the same file.
--
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]
