Use Python properties for Cython descriptors by kkraus14 · Pull Request #2056 · NVIDIA/cuda-python

kkraus14 · 2026-05-08T16:18:49Z

Summary

Add a shared python_property decorator that creates real Python property descriptors inside Cython cdef class bodies.
Convert the known public Cython descriptors that may raise or perform CUDA/NVML work during completion to use @python_property instead of generated getset_descriptors.
Add regression coverage that checks those public descriptors are property instances and not getset_descriptor objects.

Testing

pixi run ruff check cuda_core/cuda/core/_utils/properties.py cuda_core/tests/test_cython_property_descriptors.py
pixi run --manifest-path cuda_core -e cu13 pytest cuda_core/tests/test_cython_property_descriptors.py (blocked locally because cuda_core/pixi.toml on this branch still has the known python_version < "3.11" package target parse error; the pixi fix is intentionally left to the separate PR)
commit hooks: ruff, SPDX, toml, cython-lint, and other pre-commit checks passed

github-actions · 2026-05-08T16:47:45Z

Doc Preview CI
🚀 View preview at https://nvidia.github.io/cuda-python/pr-preview/pr-2056/
https://nvidia.github.io/cuda-python/pr-preview/pr-2056/cuda-core/
https://nvidia.github.io/cuda-python/pr-preview/pr-2056/cuda-bindings/
https://nvidia.github.io/cuda-python/pr-preview/pr-2056/cuda-pathfinder/
Preview will be ready when the GitHub Pages deployment is complete.

mdboom

One suggestion is just a micro-op for startup time that ultimately doesn't matter.

It would be nice to simplify the testing (if possible) though. My concern is corner case syntactic constructions that might not get caught.

mdboom · 2026-05-08T17:29:39Z

+    descriptor = inspect.getattr_static(cls, cython_property.name)
+
+    assert isinstance(descriptor, property)
+    assert not isinstance(descriptor, types.GetSetDescriptorType)


It seems kind of brittle to do a source-level analysis here.

Could we just iterate over all classes and assert that they don't have any GetSetDescriptorType's? It's possible I'm missing something (maybe there are implicit descriptors always that we can't remove), but that seems less complicated.

Good call. I replaced the source-level parser with runtime introspection over the cuda-core modules/classes.

A literal “no GetSetDescriptorType anywhere” check still needs filtering because Cython also emits getset descriptors for non-property things like __dict__, __weakref__, and typed cdef fields such as option dataclass fields. The updated test now allows those known field descriptors, but fails on any unexpected getset descriptor exposed by cuda-core classes.

Yeah, thanks for humoring me. The __dict__ and __weakref__ thing doesn't bother me (that's a pretty static set), but I forgot that cdef readonly/cdef public things would be indistinguishable from Cython properties from the outside. Probably best to revert to the first revision that parsed the code. /Maybe/ the agent could do better than regexes by using the Cython parser (I've had reasonable success with that elsewhere), but we could also cross that bridge if we find the regex approach breaks on some future code.

leofang · 2026-05-08T19:46:58Z

All Linux-64 CI segfaulted. (Ignored linux-arm64 CIs, there seems to be a widespread outage.)

kkraus14 · 2026-05-08T19:52:46Z

+_OPTIONAL_IMPORT_FAILURES = {"cuda.core._tensor_bridge"}
+
+_GETSET_FIELD_ALLOWLIST = {
+    ("cuda.core._kernel_arg_handler", "ParamHolder", "ptr"),
+    ("cuda.core._layout", "_StridedLayout", "itemsize"),
+    ("cuda.core._layout", "_StridedLayout", "slice_offset"),
+    ("cuda.core._memoryview", "StridedMemoryView", "device_id"),
+    ("cuda.core._memoryview", "StridedMemoryView", "exporting_obj"),
+    ("cuda.core._memoryview", "StridedMemoryView", "is_device_accessible"),
+    ("cuda.core._memoryview", "StridedMemoryView", "ptr"),
+    ("cuda.core._memoryview", "StridedMemoryView", "readonly"),
+    ("cuda.core._memoryview", "_StridedMemoryViewProxy", "has_dlpack"),
+    ("cuda.core._memoryview", "_StridedMemoryViewProxy", "obj"),
+}


This still feels very brittle where I'm not too happy about this and don't want to have to maintain a list like this.

kkraus14 · 2026-05-08T19:53:10Z

+def _is_allowed_getset_descriptor(cls, name, descriptor) -> bool:
+    if name in {"__dict__", "__weakref__"}:
+        return True
+    # Typed cdef fields generate getset descriptors too, but their docs follow
+    # this compact "field: type" form rather than a property docstring.
+    doc = descriptor.__doc__
+    if doc is not None and doc.startswith(f"{name}:"):
+        return True
+    return (cls.__module__, cls.__qualname__, name) in _GETSET_FIELD_ALLOWLIST


This also feels quite brittle

leofang · 2026-05-08T21:50:12Z

(Ignored linux-arm64 CIs, there seems to be a widespread outage.)

FYI, this should be restored now.

leofang · 2026-05-08T22:22:45Z

+import cuda.core.graph
+import cuda.core.system
+
+pytestmark = pytest.mark.no_cuda


Is this really needed...?

leofang

The tensor bridge module is not designed to be importable casually, so it causes a test error.

leofang

Review

Approach

python_property = property is elegant — zero runtime overhead, works with mypy/pyright (as confirmed in this thread), and makes Cython cdef class properties produce real property descriptors that rlcompleter's isinstance(..., property) guard recognizes. This is the right fix.

The `_layout` segfault (resolved)

The CI segfaults were caused by StridedMemoryView._layout: there's both a cdef _layout field and a @property _layout on the class. With Cython's native getset_descriptor, these coexist (the cdef field is C-only, the property is Python-only). But when @python_property is used, the Python property descriptor masks the cdef field even from Cython code, causing infinite recursion (_layout getter → get_layout() → self._layout → _layout getter → …). The rename to _get_layout() method is the correct fix.

Issues

CI failure: _tensor_bridge exclusion mismatch. The guardrail test fails in CI because the installed package uses a versioned subpackage (cuda.core.cu12._tensor_bridge), but the exclusion set only matched the unversioned name (cuda.core._tensor_bridge). I pushed a fix (6e182ef) that matches by suffix instead of exact name, so cuda.core.cu12._tensor_bridge, cuda.core.cu13._tensor_bridge, etc. are all excluded.
No functional rlcompleter test. The guardrail test checks descriptor types structurally, which is good for preventing regressions. But a test that actually calls rlcompleter.Completer.complete() on affected objects and asserts no exception would be stronger end-to-end proof. For example:
```
import rlcompleter
completer = rlcompleter.Completer({"mr": mr})
assert completer.complete("mr.", 0) is not None  # should not raise
```
Minor: setter pattern could use a comment. The pattern used for read-write properties:
```
def peer_accessible_by(self):
    ...
def _set_peer_accessible_by(self, devices):
    ...
peer_accessible_by = python_property(peer_accessible_by, _set_peer_accessible_by)
```
…works correctly, but it's non-obvious why the more Pythonic @peer_accessible_by.setter pattern isn't used. A brief comment noting that Cython cdef class bodies don't support .setter on a python_property would help future maintainers.

Not an issue

Over-broad scope (converting all ~439 properties, not just the ~13 affected ones) — defensively correct, prevents future surprises. Worth the churn.
Error message changes ("attribute 'X' ... is not writable" → "can't set attribute 'X'") — correct, Python property uses different wording than getset_descriptor.
graph/_graph_builder.pyx is not modified — this is fine. GraphBuilder is a regular Python class (not cdef class), so its @property decorators already produce Python property descriptors that rlcompleter handles correctly.

-- Leo's bot

leofang · 2026-05-09T01:40:37Z

(fixing it locally)

leofang

(Corrections merged into the review above.)

In CI, _tensor_bridge lives under a versioned subpackage (cuda.core.cu12._tensor_bridge), but the exclusion set only matched the unversioned name. Switch to suffix matching so that all versioned variants are excluded.

copy-pr-bot · 2026-05-09T01:52:53Z

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.

In CI, _tensor_bridge lives under a versioned subpackage (cuda.core.cu12._tensor_bridge), but the exclusion set only matched the unversioned name. Switch to suffix matching so that all versioned variants are excluded.

leofang · 2026-05-09T01:56:43Z

/ok to test 6e182ef

Python 3.10 uses "property '...' of '...' object has no setter" while 3.11+ uses "can't set attribute '...'". Relax the regex to accept both.

leofang · 2026-05-09T02:25:38Z

/ok to test 7c52713

Use Python properties for Cython descriptors

2801e98

kkraus14 added this to the cuda.core next milestone May 8, 2026

kkraus14 added the cuda.core Everything related to the cuda.core module label May 8, 2026

Keep descriptor tests scoped to cdef classes

ff862ef

leofang modified the milestones: cuda.core next, cuda.core v1.0.0 May 8, 2026

leofang added bug Something isn't working P0 High priority - Must do! labels May 8, 2026

leofang assigned kkraus14 May 8, 2026

leofang requested review from leofang, mdboom and shwina May 8, 2026 16:26

Use decorator for Cython Python properties

bd7251a

Convert cuda-core cdef properties to Python properties

20e6f06

mdboom requested changes May 8, 2026

View reviewed changes

Address cuda-core property review feedback

c2dab7f

kkraus14 commented May 8, 2026

View reviewed changes

mdboom reviewed May 8, 2026

View reviewed changes

Comment thread cuda_core/cuda/core/_memoryview.pyx Outdated

leofang reviewed May 8, 2026

View reviewed changes

Fix tests

4ff946b

leofang requested changes May 9, 2026

View reviewed changes

Comment thread cuda_core/tests/test_cython_property_descriptors.py Outdated

Comment thread cuda_core/tests/test_cython_property_descriptors.py Outdated

fix test error

6b3a3cb

leofang reviewed May 9, 2026

View reviewed changes

Fix _tensor_bridge exclusion for versioned subpackages

9eb4a8f

In CI, _tensor_bridge lives under a versioned subpackage (cuda.core.cu12._tensor_bridge), but the exclusion set only matched the unversioned name. Switch to suffix matching so that all versioned variants are excluded.

Fix _tensor_bridge exclusion for versioned subpackages

6e182ef

In CI, _tensor_bridge lives under a versioned subpackage (cuda.core.cu12._tensor_bridge), but the exclusion set only matched the unversioned name. Switch to suffix matching so that all versioned variants are excluded.

Fix read-only property error message match across Python versions

7c52713

Python 3.10 uses "property '...' of '...' object has no setter" while 3.11+ uses "can't set attribute '...'". Relax the regex to accept both.

Conversation

kkraus14 commented May 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing

Uh oh!

github-actions Bot commented May 8, 2026

Preview will be ready when the GitHub Pages deployment is complete.

Uh oh!

mdboom left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mdboom May 8, 2026

Choose a reason for hiding this comment

Uh oh!

kkraus14 May 8, 2026

Choose a reason for hiding this comment

Uh oh!

mdboom May 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leofang commented May 8, 2026

Uh oh!

kkraus14 May 8, 2026

Choose a reason for hiding this comment

Uh oh!

kkraus14 May 8, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

leofang commented May 8, 2026

Uh oh!

leofang May 8, 2026

Choose a reason for hiding this comment

Uh oh!

leofang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

leofang left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Review

Approach

The _layout segfault (resolved)

Issues

Not an issue

Uh oh!

leofang commented May 9, 2026

Uh oh!

leofang left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

copy-pr-bot Bot commented May 9, 2026

Uh oh!

leofang commented May 9, 2026

Uh oh!

leofang commented May 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kkraus14 commented May 8, 2026 •

edited

Loading

mdboom May 8, 2026 •

edited

Loading

leofang left a comment •

edited

Loading

The `_layout` segfault (resolved)

leofang left a comment •

edited

Loading