fix(core): honour DLTensor.byte_offset in StridedMemoryView.from_dlpack - #2576
Open
LeSingh1 wants to merge 1 commit into
Open
fix(core): honour DLTensor.byte_offset in StridedMemoryView.from_dlpack#2576LeSingh1 wants to merge 1 commit into
LeSingh1 wants to merge 1 commit into
Conversation
`view_as_dlpack` -- the path behind `StridedMemoryView.from_dlpack()`,
`StridedMemoryView.from_any_interface()` on a DLPack producer, and the
deprecated `StridedMemoryView(obj, stream_ptr)` constructor -- built the view
pointer from `data` alone:
buf.ptr = <intptr_t>(dl_tensor.data)
`byte_offset` is a mandatory DLTensor field, and a producer may legally leave
the allocation base in `data` and encode a slice in `byte_offset`. The
sibling capsule-consuming helper in the same file already does the right thing
(`_smv_from_dlpack_capsule`, _memoryview.pyx:866):
view.ptr = <intptr_t>(dl_tensor.data) + <intptr_t>(dl_tensor.byte_offset)
Nothing downstream recovers the offset -- `layout_from_dlpack` reads only
ndim/shape/strides/dtype -- so `view.ptr` was short by exactly `byte_offset`
bytes, and every consumer that reads `ptr` alone (`as_tensor_map()`, the
`__dlpack__` re-export, which writes `byte_offset = 0` and uses `ptr` as
`data`, and the 16-byte alignment check in _tensor_map.pyx) silently operated
on the wrong memory. No exception was raised.
Test builds a real capsule, rewrites it to describe src[1:] as base +
byte_offset, and asserts `ptr`, for both the versioned and unversioned
capsule flavours. It uses the CPU device path, so it needs no GPU.
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
view_as_dlpackbuilds the view pointer fromdataalone (_memoryview.pyx:1105):byte_offsetis a mandatoryDLTensorfield (_dlpack.pxd:55), and a producer may legally leave the allocation base indataand encode a slice inbyte_offset. The sibling capsule-consuming helper in the same file already handles it (_smv_from_dlpack_capsule,_memoryview.pyx:866):view_as_dlpackis the public consumer path —StridedMemoryView.from_dlpack()(:228),StridedMemoryView.from_any_interface()(via:283), and the deprecatedStridedMemoryView(obj, stream_ptr)constructor (:194) all funnel through it. Nothing downstream recovers the offset:layout_from_dlpack()(:1305) reads onlyndim/shape/strides/dtype.So for any exporter with
byte_offset != 0,view.ptris short by exactlybyte_offsetbytes, and every consumer that readsptralone operates on the wrong memory with no exception raised:as_tensor_map()(_tensor_map.pyx:599)__dlpack__re-export (_smv_setup_dl_tensor_common:740-741, which writesbyte_offset = 0and usesptrasdata— so the offset is lost permanently on a round-trip)_tensor_map.pyx:391Fix
One line, matching the sibling helper.
Tests
test_from_dlpack_honours_byte_offsetincuda_core/tests/test_utils_dlpack.py, built on the ctypesDLTensor/DLManagedTensorstructures already defined in that file and followingtest_from_dlpack_null_deleter_dealloc's pattern: export a real capsule, rewrite it in place to describesrc[1:]asdata = base, byte_offset = itemsize, shape[0] = n - 1, then import it and assertptr == src.ctypes.data + itemsize. Parametrised over the versioned and unversioned capsule flavours. It uses the CPU device path (stream_ptr=-1), like the neighbouring tests, so it needs no GPU.What I ran
Environment: macOS, no CUDA driver and no CUDA toolkit, so
cuda.corecannot be built or imported here.cuda_core/tests/— they need a builtcuda.core.python -m py_compile,ruff check,ruff format --checkoncuda_core/tests/test_utils_dlpack.py— clean, no new findings against amainbaseline.byte_offsetappears at exactly four places in the tree (_dlpack.pxd:55declaration;_dlpack.pyx:88and_memoryview.pyx:741both on the export side, where the offset is folded intodataand the field set to 0;_memoryview.pyx:866on the capsule import side)._memoryview.pyx:1105was the only import site missing it, and the export side'sbyte_offset = 0convention is exactly why folding it intoptron import is the correct fix rather than propagating the field._memoryview.pyx; Test using pytest-run-parallel and related fixups in the tests #2194 touchestest_utils_dlpack.pybut not this region.