Use Python properties for Cython descriptors#2056
Use Python properties for Cython descriptors#2056kkraus14 wants to merge 5 commits intoNVIDIA:mainfrom
Conversation
|
mdboom
left a comment
mdboom
left a comment
There was a problem hiding this comment.
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.
| descriptor = inspect.getattr_static(cls, cython_property.name) | ||
|
|
||
| assert isinstance(descriptor, property) | ||
| assert not isinstance(descriptor, types.GetSetDescriptorType) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
All Linux-64 CI segfaulted. (Ignored linux-arm64 CIs, there seems to be a widespread outage.) |
| _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"), | ||
| } |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
This also feels quite brittle
| return (<int>device_type, int(device_id)) | ||
|
|
||
| @property | ||
| @python_property |
There was a problem hiding this comment.
Looking at the segfault in CI (which I think is really just infinite recursion) -->
I think what's happening is that this is now masking the cdef _layout on the class for Cython code, whereas before it was somehow only exposing it to Python code. Renaming one or the other should hopefully do the trick. I assume the one expected internally from this file is the cdef and this property was only expected to be used from outside.
There was a problem hiding this comment.
How about making _layout a cdef readonly and just remove this one? We really don't want general users to access it.
There was a problem hiding this comment.
It's used from testing in the test suite, but we can just update the test suite to call get_layout() since that's all this does. The cdef readonly _layout isn't exposed to Python so the tests otherwise fail.
FYI, this should be restored now. |
| import cuda.core.graph | ||
| import cuda.core.system | ||
|
|
||
| pytestmark = pytest.mark.no_cuda |
3 participants
Summary
python_propertydecorator that creates real Pythonpropertydescriptors inside Cythoncdef classbodies.@python_propertyinstead of generatedgetset_descriptors.propertyinstances and notgetset_descriptorobjects.Testing
cuda_core/pixi.tomlon this branch still has the knownpython_version < "3.11"package target parse error; the pixi fix is intentionally left to the separate PR)