Fix DLPack byte_offset handling on import#1573
Conversation
Signed-off-by: Minh Vu <vuhoangminh97@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesDLPack byte_offset fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR fixes
Confidence Score: 5/5Safe to merge — the change is a minimal, spec-correct one-liner in the import path with a targeted regression test that validates pointer address, read values, and write-through behavior. The fix is two lines, directly implements what the DLPack spec requires (element 0 is at data + byte_offset), and is guarded for the null-data case. The new test exercises the full round-trip — pointer identity, NumPy read-back, and Warp-to-backing write — on a manually constructed capsule. No pre-existing behavior is altered for the common case where byte_offset is zero. No files require special attention. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Producer as DLPack Producer (e.g. NumPy)
participant Capsule as PyCapsule
participant Warp as wp.from_dlpack()
participant Array as Warp Array
Producer->>Capsule: "PyCapsule_New(DLManagedTensor) data=alloc_base, byte_offset=N"
Capsule->>Warp: pass capsule
Warp->>Warp: PyCapsule_GetPointer → managed_ptr
Warp->>Warp: "dlt = managed_tensor.dl_tensor"
Note over Warp: Before fix: ptr = dlt.data, After fix: ptr = dlt.data + dlt.byte_offset
Warp->>Array: "array(ptr=ptr, shape=shape, ...)"
Warp->>Capsule: PyCapsule_SetName(used_dltensor)
Array-->>Warp: return arr (ptr points to logical element 0)
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Producer as DLPack Producer (e.g. NumPy)
participant Capsule as PyCapsule
participant Warp as wp.from_dlpack()
participant Array as Warp Array
Producer->>Capsule: "PyCapsule_New(DLManagedTensor) data=alloc_base, byte_offset=N"
Capsule->>Warp: pass capsule
Warp->>Warp: PyCapsule_GetPointer → managed_ptr
Warp->>Warp: "dlt = managed_tensor.dl_tensor"
Note over Warp: Before fix: ptr = dlt.data, After fix: ptr = dlt.data + dlt.byte_offset
Warp->>Array: "array(ptr=ptr, shape=shape, ...)"
Warp->>Capsule: PyCapsule_SetName(used_dltensor)
Array-->>Warp: return arr (ptr points to logical element 0)
Reviews (2): Last reviewed commit: "Fix DLPack test alignment padding" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| shape = (3,) | ||
|
|
||
| managed_tensor_size = ctypes.sizeof(DLManagedTensor) | ||
| padding = managed_tensor_size & 7 |
There was a problem hiding this comment.
managed_tensor_size & 7 is semantically inverted. It yields the number of bytes already consumed within the current 8-byte block, not the bytes needed to reach the next boundary. For a size of 57 it gives 1 instead of 7. The result happens to be correct here because DLManagedTensor is 64 bytes (a multiple of 8), but the formula would silently misalign the shape array if the struct size ever changed. The correct expression is (-managed_tensor_size) & 7.
| padding = managed_tensor_size & 7 | |
| padding = (-managed_tensor_size) & 7 |
Signed-off-by: Minh Vu <vuhoangminh97@gmail.com>
1 participant
Description
DLTensor.byte_offsetis part of the logical address of element zero in the DLPack tensor: consumers should read fromdata + byte_offset. Warp imported DLPack arrays usingdlt.datadirectly, so a valid producer that keepsdataat the allocation base and describes a view withbyte_offsetwould be imported at the wrong address.I found this while checking Warp's DLPack import path against the DLPack tensor layout. I have not seen this from a production failure. The new regression test builds a small valid CPU DLPack capsule with a nonzero
byte_offsetand shows that the imported Warp array should read and write the logical view, not the allocation base.Changes
DLTensor.byte_offsetwhen constructing the imported Warp array pointer.wp.from_dlpack()fix.Checklist
Unreleasedsection.Validation summary
I verified the change locally on macOS arm64 with a CPU-only Warp build:
uv run build_lib.py --quickuv run warp/tests/interop/test_dlpack.py -k test_dlpack_from_dlpack_byte_offsetuv run warp/tests/interop/test_dlpack.pyuvx pre-commit run --files CHANGELOG.md warp/_src/dlpack.py warp/tests/interop/test_dlpack.pygit diff --checkThe local run does not have Torch, JAX, or Paddle installed, so those optional DLPack interop tests were skipped. The new regression test does not depend on those packages.
Bug fix
The added test is the minimal repro: it creates a DLPack tensor whose
datapoints to the start of anint32NumPy allocation whilebyte_offsetpoints to element 3.wp.from_dlpack()should import the logical three-element view[3, 4, 5]and writes through the Warp array should update only that slice.New feature / enhancement
Not applicable.
Summary by CodeRabbit
DLTensor.byte_offsetwhen converting external (zero-copy) DLPack tensors into Warp arrays.