Skip to content

Add make_tensor method which takes ownership of DLManagedTensor object#1157

Open
simonbyrne wants to merge 19 commits intomainfrom
sbyrne/dlpack
Open

Add make_tensor method which takes ownership of DLManagedTensor object#1157
simonbyrne wants to merge 19 commits intomainfrom
sbyrne/dlpack

Conversation

@simonbyrne
Copy link
Copy Markdown
Collaborator

No description provided.

@copy-pr-bot
Copy link
Copy Markdown

copy-pr-bot Bot commented Apr 16, 2026

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@simonbyrne simonbyrne marked this pull request as ready for review April 17, 2026 20:09
@simonbyrne
Copy link
Copy Markdown
Collaborator Author

I think this is ready. I also tried to clean up the python interop example, as I thought it was a bit confusing.

Comment thread docs_input/api/creation/tensors/make.rst
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds DLPack ownership-transfer support to MatX tensor import/export so that imported storage lifetimes are safely managed by MatX until the last view is released, with corresponding tests, docs, and Python integration sample updates.

Changes:

  • Add pointer-owning make_tensor(tensor, DLManagedTensor*) and make_tensor(tensor, DLManagedTensorVersioned*) overloads; deprecate the legacy non-owning make_tensor(tensor, DLManagedTensor) import.
  • Refactor MatX DLPack export to support both legacy (ToDlPack) and versioned (ToDlPackVersioned) managed tensor types via a shared implementation.
  • Update vendored dlpack.h, add lifetime tests for owning import, and refresh docs/examples/devcontainer to reflect the new ownership model.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
test/00_tensor/BasicTensorTests.cu Adds lifetime tests verifying DLPack deleter is called exactly once after last MatX reference is released.
include/matx/core/tensor.h Refactors DLPack export; adds ToDlPackVersioned() and shared deleter logic.
include/matx/core/make_tensor.h Introduces ownership-taking DLPack import overloads and factors dtype validation/shape-stride extraction into helpers.
include/matx/core/dlpack.h Updates vendored DLPack header to newer definitions (versioned tensors + exchange API).
examples/python_integration_sample/mypythonlib.py Simplifies Python-side printing of received DLPack tensor.
examples/python_integration_sample/matxutil.cu Updates pybind helper to consume capsules and use pointer-owning MatX import.
examples/python_integration_sample/example_matxutil.py Updates sample script to demonstrate single-consumption semantics and error cases.
docs_input/external.rst Adds/expands DLPack interoperability documentation.
docs_input/api/creation/tensors/make.rst Documents new DLPack-related overloads and APIs.
docs_input/Doxyfile.in / docs_input/CMakeLists.txt Excludes examples from Doxygen generation.
.devcontainer/dev.Dockerfile Adds Python/Sphinx deps and CuPy installation for the sample/docs workflow.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread include/matx/core/make_tensor.h Outdated
Comment thread include/matx/core/make_tensor.h
Comment thread examples/python_integration_sample/matxutil.cu
Comment thread include/matx/core/dlpack.h
Comment thread docs_input/external.rst Outdated
Comment thread include/matx/core/make_tensor.h
Comment thread include/matx/core/make_tensor.h
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 17, 2026

Greptile Summary

This PR adds ownership-taking make_tensor overloads for both DLManagedTensor* and DLManagedTensorVersioned*, so MatX can import external DLPack tensors and automatically invoke the producer's deleter when the last MatX reference is dropped. It also updates dlpack.h to v1.3, refactors ToDlPack into a shared ToDlPackImpl template, adds ToDlPackVersioned(), properly handles byte_offset, null strides (legacy producers), and the DLPACK_FLAG_BITMASK_READ_ONLY flag via const T tensor types.

Confidence Score: 4/5

Safe to merge after addressing the DLPackLaneInfo primitive-type mismatch for char/short vector specializations.

All previously flagged P0/P1 issues (resource leak on validation throw, byte_offset ignored, null strides crash, kDLFloat-16 accepting matxBf16, read-only flag unhandled) are now addressed. One remaining P2 issue: DLPackLaneInfo specializations use C primitive types (char, short, int, long) whose scalar_type won't satisfy the is_same_v checks against fixed-width types (int8_t, int16_t, int32_t, int64_t) in validate_dlpack_tensor_type, breaking import roundtrips for char/short-based vector types. Low practical impact but a real correctness gap in the new lane infrastructure.

include/matx/core/type_utils_both.h — DLPackLaneInfo specializations should use int8_t/uint8_t/int16_t/uint16_t instead of char/unsigned char/short/unsigned short.

Important Files Changed

Filename Overview
include/matx/core/make_tensor.h Adds two new owning make_tensor overloads (DLManagedTensor* and DLManagedTensorVersioned*) that immediately wrap the pointer in a shared_ptr before validation, fix byte_offset handling, add null-stride fallback, support read-only const tensor imports, and deprecate the old by-value overload. Extracts validate_dlpack_tensor_type and dlpack_shape_and_strides into detail helpers.
include/matx/core/type_utils_both.h Adds DLPackLaneInfo trait to map vector types (float4, int2, etc.) to scalar type and lane count for DLPack. Specializations using char/unsigned char/short/int/long won't match the fixed-width types (int8_t, uint16_t, etc.) checked in validate_dlpack_tensor_type.
include/matx/core/tensor.h Refactors ToDlPack() into a shared ToDlPackImpl() template, adds ToDlPackVersioned(), sets DLPACK_FLAG_BITMASK_READ_ONLY for const tensor exports, and fixes const_cast for void* data pointer in the DLPack struct.
examples/python_integration_sample/matxutil.cu Rewrites the Python integration example to use the new owning make_tensor overloads via make_tensor_from_capsule. Properly marks capsules as consumed before calling make_tensor (which now handles cleanup on exception via shared_ptr).
test/00_tensor/DLPackTests.cu New comprehensive test suite for DLPack: export (legacy and versioned), owning import lifetime tracking, byte_offset, null strides, read-only const tensor enforcement, and vector lane matching/mismatch.
include/matx/core/dlpack.h Updates dlpack.h to version 1.3 (minor bump from 1.1), adds kDLTrn device, updates strides-can-be-null documentation, typedef-wraps DLManagedTensorVersioned for C compatibility, and adds the new DLPackExchangeAPI fast-path definitions.
include/matx/core/storage.h Fixes const_cast in the shared_ptr deleter so that const T* storage can be freed via matxFree without UB.

Sequence Diagram

sequenceDiagram
    participant Producer as Producer (CuPy/PyTorch)
    participant Helper as make_tensor_from_capsule
    participant MakeTensor as matx::make_tensor
    participant Owner as shared_ptr owner
    participant Tensor as tensor_t

    Producer->>Helper: py::capsule ("dltensor")
    Helper->>Helper: PyCapsule_SetName("used_dltensor")
    Helper->>MakeTensor: make_tensor(tensor, managed*)
    MakeTensor->>Owner: wrap managed* in shared_ptr immediately
    MakeTensor->>MakeTensor: validate type, rank, strides, byte_offset
    alt validation throws
        Owner-->>Producer: shared_ptr dtor calls managed->deleter()
    else success
        MakeTensor->>Tensor: alias shared_ptr ties DLMgd lifetime to tensor
        Tensor-->>Helper: tensor populated
    end
    Note over Owner,Tensor: DLMgd deleter called when last tensor copy is destroyed
Loading

Reviews (9): Last reviewed commit: "revert dockerfile changes" | Re-trigger Greptile

Comment thread include/matx/core/make_tensor.h
Comment thread examples/python_integration_sample/matxutil.cu
Comment thread include/matx/core/make_tensor.h Outdated
Comment thread include/matx/core/make_tensor.h
Comment thread .devcontainer/dev.Dockerfile Outdated
Comment thread include/matx/core/make_tensor.h
Copy link
Copy Markdown
Collaborator

@cliffburdick cliffburdick left a comment

Choose a reason for hiding this comment

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

Lots of comments from the bot

@simonbyrne
Copy link
Copy Markdown
Collaborator Author

I think i addressed all the reviewbot comments.

Comment thread include/matx/core/make_tensor.h
@simonbyrne simonbyrne requested a review from cliffburdick April 20, 2026 16:18
@simonbyrne
Copy link
Copy Markdown
Collaborator Author

/build

1 similar comment
@cliffburdick
Copy link
Copy Markdown
Collaborator

/build

@simonbyrne
Copy link
Copy Markdown
Collaborator Author

I merged in main.

@cliffburdick
Copy link
Copy Markdown
Collaborator

/build

@simonbyrne
Copy link
Copy Markdown
Collaborator Author

/build

@coveralls
Copy link
Copy Markdown

coveralls commented Apr 23, 2026

Coverage Status

Coverage is 91.426%sbyrne/dlpack into main. No base build found for main.

@cliffburdick
Copy link
Copy Markdown
Collaborator

/build

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants