Skip to content

GH-39295: [C++][Python] ConsumingDLPack on Arrays and Tensor - #51122

Open
AntoinePrv wants to merge 6 commits into
apache:mainfrom
AntoinePrv:dl-consume-2
Open

GH-39295: [C++][Python] ConsumingDLPack on Arrays and Tensor#51122
AntoinePrv wants to merge 6 commits into
apache:mainfrom
AntoinePrv:dl-consume-2

Conversation

@AntoinePrv

@AntoinePrv AntoinePrv commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Rationale for this change

There are already utilities to import tensor data from NumPy.
With DLPack standard, it will work across many tensor providers.

What changes are included in this PR?

  • Bind FixedShapedTensorArray::FromTensor in Python
  • Add ImportArrayVersionedFromDLPack and ImportTensorVersionedFromDLPack in C++
  • Add in Python Array.from_dlpack and Tensor.from_dlpack
    Similar to the export, multidimensional tensor import require an explicit step through Tensor.
  • No Array::FromTensor or even FixedSizeListArray::FromTensor. IMHO it does not feel as necessary in this direction but open to anyone's take one it.

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes, new APIs.

@github-actions github-actions Bot added the awaiting review Awaiting review label Sep 1, 2026
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

⚠️ GitHub issue #39295 has been automatically assigned in GitHub to PR creator.

@AntoinePrv
AntoinePrv marked this pull request as ready for review September 3, 2026 17:25
Copilot AI lite review requested due to automatic review settings September 3, 2026 17:25
@AntoinePrv

Copy link
Copy Markdown
Collaborator Author

@AlenkaF @rok @pitrou this is looking good

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

There is a likely C++ compile-breaking scoping issue in cpp/src/arrow/tensor.cc around the stride helper functions, plus several doc/API consistency issues to resolve.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR adds DLPack consumer support to Arrow Arrays/Tensors in C++ and exposes new Python factory APIs (Array.from_dlpack, Tensor.from_dlpack), enabling zero-copy (or requested-copy) imports from DLPack-producing libraries (e.g., NumPy, PyTorch).

Changes:

  • Add C++ DLPack consumers: arrow::dlpack::ImportArrayVersioned and arrow::dlpack::ImportTensorVersioned, plus supporting utilities and tests.
  • Expose Python APIs for importing via versioned DLPack capsules, and bind FixedShapeTensorArray::FromTensor.
  • Add Python test coverage for tensor/array import, copy vs zero-copy behavior, and unsupported cases.
File summaries
File Description
python/pyarrow/tests/test_dlpack.py Adds Python tests for Tensor.from_dlpack / Array.from_dlpack, including copy semantics and unsupported multidim array import.
python/pyarrow/tensor.pxi Implements Tensor.from_dlpack and wires it to C++ ImportTensorVersioned.
python/pyarrow/includes/libarrow.pxd Declares DLPack version struct/constant and new C++ import APIs for Cython bindings; adds FixedShapeTensorArray::FromTensor.
python/pyarrow/array.pxi Implements Array.from_dlpack and adds FixedShapeTensorArray.from_tensor binding.
cpp/src/arrow/tensor.h Declares ComputeTensorSize helper for stride/shape-based size computation.
cpp/src/arrow/tensor.cc Defines ComputeTensorSize and adjusts internal stride helper placement/calls.
cpp/src/arrow/c/dlpack.h Exposes compiled DLPack version constant and declares new import APIs.
cpp/src/arrow/c/dlpack.cc Implements DLPack version constant and consumer-side import logic for arrays/tensors.
cpp/src/arrow/c/dlpack_test.cc Adds comprehensive C++ tests for importing (ownership, copy vs share, dtype coverage, errors).
Review details

Suppressed comments (1)

cpp/src/arrow/c/dlpack.h:132

  • The parameter doc for ImportTensorVersioned says raw is an "Arrow array", but the function takes a DLManagedTensorVersioned* (a DLPack tensor).
/// \param[in] raw Arrow array
  • Files reviewed: 9/9 changed files
  • Comments generated: 5
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread cpp/src/arrow/tensor.cc
Comment thread cpp/src/arrow/c/dlpack.h Outdated
Comment thread python/pyarrow/array.pxi
Comment thread python/pyarrow/tensor.pxi
Comment thread python/pyarrow/tensor.pxi Outdated
Copilot AI review requested due to automatic review settings September 4, 2026 07:53

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

There are correctness and robustness issues in the new DLPack consumer path (notably error type mismatch for unsupported array layouts, plus missing input validation that can crash on malformed tensors).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 9/9 changed files
  • Comments generated: 4
  • Review effort level: Lite

Comment thread cpp/src/arrow/c/dlpack.cc
Comment thread cpp/src/arrow/c/dlpack.cc
Comment thread cpp/src/arrow/c/dlpack.cc
Comment thread python/pyarrow/array.pxi
Copilot AI review requested due to automatic review settings September 4, 2026 08:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

There is a confirmed C++ compilation issue in tensor.cc, and the Python copy= contract isn’t enforced by the current from_dlpack implementations.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

cpp/src/arrow/tensor.cc:563

  • Tensor::is_row_major() / is_column_major() call internal::IsTensorStridesRowMajor/ColumnMajor, but those helpers are defined inside an anonymous namespace nested under arrow::internal (in this translation unit), so arrow::internal::IsTensorStridesRowMajor doesn't exist. This will fail to compile. Consider computing the expected row/column-major strides directly in these methods (or moving the helpers out of the anonymous namespace).
bool Tensor::is_row_major() const {
  return internal::IsTensorStridesRowMajor(type_, shape_, strides_);
}
  • Files reviewed: 9/9 changed files
  • Comments generated: 3
  • Review effort level: Lite

Comment thread cpp/src/arrow/c/dlpack.cc Outdated
Comment thread python/pyarrow/array.pxi
Comment thread python/pyarrow/tensor.pxi
Copilot AI review requested due to automatic review settings September 4, 2026 09:12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

There are compile- and correctness-blocking issues in the C++ tensor stride helper linkage and in the Cython from_dlpack implementations (nogil assignment and unchecked capsule consumption marking).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

cpp/src/arrow/tensor.cc:227

  • Tensor::is_row_major() / is_column_major() call internal::IsTensorStridesRowMajor/ColumnMajor, but those functions are currently only defined in an anonymous namespace (not arrow::internal), so this won’t compile. Define IsTensorStridesRowMajor and IsTensorStridesColumnMajor in namespace internal (next to IsTensorStridesContiguous) or revert the qualified calls.
}  // namespace
}  // namespace internal

namespace internal {

  • Files reviewed: 9/9 changed files
  • Comments generated: 3
  • Review effort level: Lite

Comment thread python/pyarrow/array.pxi
Comment thread python/pyarrow/tensor.pxi
Comment thread python/pyarrow/tests/test_dlpack.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants