Skip to content

Fail fast on unsupported StridedMemoryView strides#1730

Merged
cpcloud merged 2 commits intoNVIDIA:mainfrom
cpcloud:issue-1429
Mar 6, 2026
Merged

Fail fast on unsupported StridedMemoryView strides#1730
cpcloud merged 2 commits intoNVIDIA:mainfrom
cpcloud:issue-1429

Conversation

@cpcloud
Copy link
Contributor

@cpcloud cpcloud commented Mar 6, 2026

Summary

  • Validate CAI and array-interface layout during StridedMemoryView construction so unsupported strides fail immediately instead of on first metadata access.
  • Update the existing array-interface regression to assert constructor-time failure.
  • Add a CUDA array interface regression for unsupported non-itemsize-divisible strides.

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_strides
  • pixi run --manifest-path cuda_core -e cu13 pytest /home/cloud/src/agent-work/issue-1429/cuda_core/tests/test_utils.py -k from_array_interface

Made with Cursor

@copy-pr-bot
Copy link
Contributor

copy-pr-bot bot commented Mar 6, 2026

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.

@cpcloud
Copy link
Contributor Author

cpcloud commented Mar 6, 2026

/ok to test

@cpcloud cpcloud requested a review from mdboom March 6, 2026 14:25
@github-actions

This comment has been minimized.

@cpcloud cpcloud requested a review from rwgk March 6, 2026 16:32
Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

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

  1. Early validation placement: Calling buf.get_layout() before setting ptr ensures failures happen at construction time rather than on first metadata access. ✅
  2. Exception safety: If validation fails, the object isn't returned, so partial initialization is fine. ✅
  3. Test coverage: Tests cover the main case (non-divisible strides) for both array interface and CAI. ✅
  4. None/NULL strides handling: The code correctly handles None strides (C-contiguous case) - if strides is None, base.strides is set to NULL and _divide_strides isn't called. ✅
  5. 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
@cpcloud
Copy link
Contributor Author

cpcloud commented Mar 6, 2026

/ok to test

@cpcloud cpcloud enabled auto-merge (squash) March 6, 2026 21:36
@cpcloud cpcloud merged commit 3695783 into NVIDIA:main Mar 6, 2026
86 checks passed
@github-actions
Copy link

github-actions bot commented Mar 6, 2026

Doc Preview CI
Preview removed because the pull request was closed or merged.

@cpcloud cpcloud deleted the issue-1429 branch March 6, 2026 22:42
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.

Fail on construction of StridedMemoryView when unsupported strides are used

2 participants