Fail fast on unsupported StridedMemoryView strides#1730
Conversation
|
Auto-sync is disabled for ready for review pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test |
This comment has been minimized.
This comment has been minimized.
rwgk
left a comment
There was a problem hiding this comment.
I asked Cursor specifically about testing edge cases. See below for the response, but this PR looks good to me as-is.
✅ Overall Assessment
The changes look solid! The early validation approach is correct, exception handling is proper, and the tests cover the main use case. The PR successfully achieves its goal of failing fast on unsupported strides.
What Looks Good
- Early validation placement: Calling
buf.get_layout()before settingptrensures failures happen at construction time rather than on first metadata access. ✅ - Exception safety: If validation fails, the object isn't returned, so partial initialization is fine. ✅
- Test coverage: Tests cover the main case (non-divisible strides) for both array interface and CAI. ✅
- None/NULL strides handling: The code correctly handles
Nonestrides (C-contiguous case) - ifstridesisNone,base.stridesis set toNULLand_divide_stridesisn't called. ✅ - DLPack path: Correctly doesn't need validation since DLPack strides are already in element counts, not bytes. ✅
Potential Edge Cases (Optional Improvements)
The following edge cases are likely already handled correctly by the existing validation logic, but could be worth explicitly testing:
1. Zero Strides
Zero strides are valid for singleton dimensions (e.g., shape (1, 1) with strides (0, 0)). The validation check stride * itemsize == base.strides[i] should handle this correctly (0 * itemsize == 0), but consider adding an explicit test:
def test_from_cuda_array_interface_zero_strides(init_cuda):
"""Test that zero strides (valid for singleton dims) are handled correctly."""
cai_obj = type("ZeroStridesCAI", (), {
"__cuda_array_interface__": {
"shape": (1, 1),
"strides": (0, 0), # Valid for singleton dimensions
"typestr": "<f8",
"data": (0, False),
"version": 3,
}
})()
# Should not raise
smv = StridedMemoryView.from_cuda_array_interface(cai_obj, stream_ptr=-1)
assert smv.shape == (1, 1)
assert smv.strides == (0, 0)2. Negative Strides
If negative strides are supported (for reversed arrays), the validation should work correctly (-8 // 8 == -1, and -1 * 8 == -8). Consider adding a test if this is a supported use case.
3. Empty Arrays
Empty arrays (shape with zeros, e.g., (0, 3)) should work correctly since the validation only checks divisibility, not array validity. Consider adding an explicit test to ensure empty arrays are handled:
def test_from_cuda_array_interface_empty_array(init_cuda):
"""Test that empty arrays are handled correctly."""
cai_obj = type("EmptyArrayCAI", (), {
"__cuda_array_interface__": {
"shape": (0, 3),
"strides": (24, 8), # Valid strides for empty array
"typestr": "<f8",
"data": (0, False),
"version": 3,
}
})()
# Should not raise
smv = StridedMemoryView.from_cuda_array_interface(cai_obj, stream_ptr=-1)
assert smv.shape == (0, 3)4. Integer Overflow (Low Priority)
Very large strides might overflow when multiplied (stride * itemsize), but this is likely a rare edge case and may be acceptable.
Conclusion
The PR is ready to merge as-is. The suggestions above are optional improvements for additional test coverage, but the core functionality is solid and the main use case is well-tested. Great work on the fail-fast validation! 🎉
Validate CAI and array-interface layout during StridedMemoryView construction so non-itemsize-divisible strides fail immediately. Add regression coverage to assert constructor-time failures for both interfaces. Made-with: Cursor
Add explicit CAI tests for zero, empty-array, and negative-stride layouts so stride divisibility validation remains covered for uncommon but valid cases. Require `shape` and `strides` as keyword-only args in the synthetic CAI helper to keep test call sites unambiguous. Made-with: Cursor
|
/ok to test |
|
Summary
StridedMemoryViewconstruction so unsupported strides fail immediately instead of on first metadata access.Closes #1429
Test plan
pixi run --manifest-path cuda_core -e cu13 pytest /home/cloud/src/agent-work/issue-1429/cuda_core/tests/test_utils.py -k unsupported_stridespixi run --manifest-path cuda_core -e cu13 pytest /home/cloud/src/agent-work/issue-1429/cuda_core/tests/test_utils.py -k from_array_interfaceMade with Cursor