GH-41670: [C++][Python] Move to DLPack 1.3 - #50827
Conversation
AntoinePrv
changed the title
|
|
There was a problem hiding this comment.
🟡 Changes recommended
Arrow’s current DLPack Array exporter still emits strides = NULL (non-compliant with DLPack v1.2+ requirements) and the new header text contains documentation mismatches that should be corrected.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Updates Arrow’s vendored DLPack ABI header to DLPack v1.3, expanding the ABI surface (new device/type enums, flags, and the __dlpack_c_exchange_api__ protocol) while bumping the reported minor version.
Changes:
- Bump DLPack minor version to 1.3 and sync header contents to a newer upstream commit.
- Add new
DLDeviceType/DLDataTypeCodevalues and additionalDLManagedTensorVersionedflag bitmasks. - Introduce DLPack fast exchange protocol (
DLPackExchangeAPI*) type definitions and documentation.
File summaries
| File | Description |
|---|---|
| cpp/src/arrow/c/dlpack_abi.h | Vendor update to DLPack 1.3 ABI definitions, enums, flags, and fast exchange protocol types. |
Review details
Suppressed comments (1)
cpp/src/arrow/c/dlpack_abi.h:426
- The documentation for
DLPackManagedTensorFromPyObjectNoSyncclaims the function returns an owning pointer/NULL, but the typedef returnsintand delivers the tensor viaout. The return contract should match the signature.
* \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.
- Files reviewed: 1/1 changed files
- Comments generated: 3
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
github-actions
Bot
added
awaiting committer review
There was a problem hiding this comment.
🟡 Changes recommended
The new public versioned export APIs are not covered by tests, and there is a small but concrete maintainability issue (std::move on a const shared_ptr reference) to address.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (3)
cpp/src/arrow/c/dlpack.cc:212
tis aconst std::shared_ptr<Tensor>&, sostd::move(t)won’t actually move (it will fall back to a copy viaoperator=(const shared_ptr&)). Using a plain copy here is clearer and avoids the misleadingstd::move.
ctx->t = std::move(t);
cpp/src/arrow/c/dlpack_test.cc:49
- New public APIs
ExportArrayVersioned/ExportTensorVersioned(and theversion/flagsfields they populate) aren’t covered by tests here; currently the tests only exercise the legacyExportArray/ExportTensorpaths. Adding a small set of assertions for version/flags on the versioned exports would prevent regressions.
ASSERT_EQ(1, *dltensor.strides); // Must be non-null with ndim>0 since 1.2
cpp/src/arrow/c/dlpack.h:48
ExportTensoralso returns the legacyDLManagedTensor(deprecated in DLPack in favor ofDLManagedTensorVersioned), but onlyExportArrayis annotated as deprecated in this header. To keep the API docs consistent and steer callers to the versioned path, add the same deprecation note forExportTensor.
ARROW_EXPORT
Result<DLManagedTensor*> ExportTensor(const std::shared_ptr<Tensor>& t);
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Changes recommended
cpp/src/arrow/c/dlpack.cc has missing semicolons after ARROW_ASSIGN_OR_RAISE statements that will break compilation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
cpp/src/arrow/c/dlpack.cc:175
- Missing semicolon after ARROW_ASSIGN_OR_RAISE; as written this won’t compile because the macro expansion isn’t terminated as a statement.
ARROW_ASSIGN_OR_RAISE(auto device, ExportDevice(t))
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
@AlenkaF would you mind providing early feedback before I move on to the Python side. |
There was a problem hiding this comment.
🟡 Changes recommended
The legacy ExportTensor path now exposes immutable tensor memory without a read-only signal, which can violate Arrow’s immutability contract unless behavior/tests are adjusted accordingly.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
cpp/src/arrow/c/dlpack.h:48
ExportTensoris also a legacy (pre-versioned) DLPack API, but onlyExportArrayis documented as deprecated. This makes the header inconsistent and can mislead users into using the legacy tensor export instead of the versioned API.
ARROW_EXPORT
Result<DLManagedTensor*> ExportTensor(const std::shared_ptr<Tensor>& t);
cpp/src/arrow/c/dlpack_test.cc:265
- This test currently validates exporting an immutable tensor for both legacy and versioned producers. If the legacy API is made to reject immutable tensors (since it cannot express read-only), the test should branch: expect a TypeError for
LegacyProducerand continue to validate successful export + read-only flag forVersionedProducer.
CheckDLTensor<TypeParam>(read_only_tensor, float32(), DLDataTypeCode::kDLFloat, shape,
dlpack_strides);
}
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| if (t->size() == 0) { | ||
| ctx->tensor.dl_tensor.data = NULL; | ||
| ctx->tensor.dl_tensor.data = nullptr; | ||
| } else { | ||
| ctx->tensor.dl_tensor.data = t->raw_mutable_data(); | ||
| ctx->tensor.dl_tensor.data = const_cast<uint8_t*>(t->raw_data()); | ||
| } |
Rationale for this change
Pure version bump to enable implementing more features.
What changes are included in this PR?
raw_dataforTensorsimilar toArray(instead ofmutable_raw_datathat is not available on immutable tensors).NULLExportArrayVersionedandExportTensorVersionedC APIsAre these changes tested?
Yes, with existing tests.
Are there any user-facing changes?