Repository navigation
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
github-actions
Bot
added
the
cuda.core
|
/ok to test |
|
|
/ok to test |
| "embedsignature": True, | ||
| "warn.deprecated.IF": False, | ||
| "freethreading_compatible": True, | ||
| "annotation_typing": False, |
|
@mdboom we should discuss this. I am huge +1 in merging this. |
mdboom
added
the
PR review get-together
|
/ok to merge e04d1b9 |
|
The one thing I don't like (but could live with) is under this flag we have to do:
That all makes sense when we don't want them to match, but when we do, it seems like it would be easy to forget the second one. |
|
As we discussed on Slack a while ago, I think it's the right way to go with def foo(T bar: T): ...and I am +1 to move on. The first cc @Andy-Jost for vis/blessing |
📝 Summary
Priority: ➖ Normal Change: Refactor Merge Risk: 🟠 High · up to Creating a stream with an explicit context now fails, which breaks a core workflow. Passing None as a graph kernel or as a system device can crash at the native level instead of raising a clear argument error. Fix these issues before merging. ✨ Finishing Touches 💡 1
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: NVIDIA/cuda-python/.coderabbit.yaml
- Review profile: CHILL
- Plan: Enterprise
- Run ID:
798365be-b1a0-4b10-94b2-999dde12bee0
📒 Files selected for processing (21)
cuda_core/build_hooks.pycuda_core/cuda/core/_context.pyxcuda_core/cuda/core/_device.pyxcuda_core/cuda/core/_event.pyxcuda_core/cuda/core/_launcher.pyxcuda_core/cuda/core/_layout.pyxcuda_core/cuda/core/_memory/_buffer.pyxcuda_core/cuda/core/_memory/_copy_ops.pyxcuda_core/cuda/core/_memory/_device_memory_resource.pyxcuda_core/cuda/core/_memory/_managed_memory_resource.pyxcuda_core/cuda/core/_memory/_peer_access_utils.pyicuda_core/cuda/core/_memory/_peer_access_utils.pyxcuda_core/cuda/core/_memory/_pinned_memory_resource.pyxcuda_core/cuda/core/_module.pyxcuda_core/cuda/core/_stream.pyxcuda_core/cuda/core/_tensor_bridge.pyxcuda_core/cuda/core/graph/_graph_node.pyxcuda_core/cuda/core/graph/_subclasses.pyxcuda_core/cuda/core/system/_device.pyxcuda_core/cuda/core/system/_field_values.pxicuda_core/cuda/core/system/_system_events.pyx
💤 Files with no reviewable changes (4)
- cuda_core/cuda/core/_memory/_managed_memory_resource.pyx
- cuda_core/cuda/core/_memory/_pinned_memory_resource.pyx
- cuda_core/cuda/core/_memory/_device_memory_resource.pyx
- cuda_core/cuda/core/_context.pyx
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
| @cython.annotation_typing(False) | ||
| def _init(cls, obj: IsStreamType | None = None, options: StreamOptions | None = None, | ||
| device_id: int | None = None, ctx: Context | None = None) -> Stream: | ||
| device_id: int | None = None, Context ctx: Context | None = None) -> Stream: |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
important: Replace the remaining context reference with ctx.
The removed local variable leaves context.is_green at Line 175 unresolved. When _init creates a stream with a context, that check raises NameError before create_stream_handle runs. Use ctx.is_green there. Cython resolves names by scope and can fall back to Python lookup for an unresolved name. (cython.readthedocs.io)
| s.update(value) | ||
|
|
||
| def launch(self, config: LaunchConfig, kernel: Kernel, *args) -> KernelNode: | ||
| def launch(self, config: LaunchConfig, Kernel kernel: Kernel, *args) -> KernelNode: |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
important: Reject None before the kernel handle access. A Cython-typed Kernel argument accepts None. If a caller passes None, GN_launch reads ker._h_kernel without a null check. Require a non-None kernel at this public entrypoint to preserve a safe argument error. (cython.readthedocs.io)
|
|
||
|
|
||
| def get_topology_common_ancestor(device1: Device, device2: Device) -> GpuTopologyLevel: | ||
| def get_topology_common_ancestor(Device device1: Device, Device device2: Device) -> GpuTopologyLevel: |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
important: Reject None before accessing NVML device handles. Cython-typed Device arguments accept None. Both public functions then read _handle without a null check, so a None argument can fail at the C-attribute access. (cython.readthedocs.io)
cuda_core/cuda/core/system/_device.pyx#L1113-L1113: validate both devices beforeget_topology_common_ancestorreads their handles.cuda_core/cuda/core/system/_device.pyx#L1139-L1139: validate both devices beforeget_p2p_statusreads their handles.
📍 Affects 1 file
cuda_core/cuda/core/system/_device.pyx#L1113-L1113(this comment)cuda_core/cuda/core/system/_device.pyx#L1139-L1139
Andy-Jost
left a comment
Andy-Jost
left a comment
There was a problem hiding this comment.
The direction looks good, but two things need fixing first:
- The build fails:
_stream.pyx:175usescontext, which this PR removes. #2920 added that use on main, and the merge of main brought it in without a conflict. Usectx. - With annotation typing,
arg: TrejectsNonewhenTis acdefclass. With this change,T arg: TacceptsNone, and an annotation-onlyarg: Taccepts anything. Where the code then reaches C,Nonecrashes instead of raisingTypeError. CodeRabbit flagged three cases, but the same pattern appears in more than twenty public entry points, includinglaunch, theGraphNodebuilders, theupdatemethods andBuffer.copy_from.
It looks like we need the following:
- Write
T arg not None: Twhere Cython knowsT, andarg not None: Twhere it does not. KeepT | NonewhereNoneis valid. - Add a pre-commit check that rejects a
defparameter annotated with a singlecdefclass unless it has anot Noneclause or its annotation includesNone. - Add a few tests that pass
Nonetolaunch,GraphNode.launchandBuffer.copy_from, and expect aTypeError.
This branch has not been deployed
4 participants
A possible implementation of #2894.
This should give us something to discuss -- about whether this is an improvement or not.
My 2c -- keeping "semantically meaningful typing" separate from "annotation-only typing" has some upsides, but it creates additional redundancy that could be confusing.
As an aside, this did identify a few places where runtime type checking is performed multiple times unnecessarily, and in any event we should fix that.