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.
|
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
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)