feat(cuda.core): support CU_LAUNCH_ATTRIBUTE_PRIORITY in LaunchConfig - #2706
Conversation
github-actions
Bot
added
the
cuda.core
|
/ok to test b963404 |
This comment has been minimized.
This comment has been minimized.
| Whether to allow programmatic stream serialization (PDL). When True, | ||
| the kernel may overlap with a previous kernel in the same stream that | ||
| signals completion via programmatic means. | ||
| priority : int, optional |
There was a problem hiding this comment.
Do we have a range for the value we can set? It cannot be any number.
CUDA doc should provide guidance on what value to set. Let's include the guidance in docstring.
There was a problem hiding this comment.
The range depends on the device, so there's no single fixed number to document. It's queried at runtime via cuCtxGetStreamPriorityRange(), which reports the valid bounds for the currently active context. On a device that doesn't support multiple stream priorities, both bounds come back as 0.
c71a56d to
070353d
Compare
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
There is still a semantic gap in the current head around priority=0.
StreamOptions uses None as the sentinel and treats 0 as a real priority. This PR now stores LaunchConfig.priority as an int, but both construction and native conversion use truthiness (if priority: / if self.priority:). That makes an explicit priority=0 indistinguishable from not specifying the launch attribute at all.
That matters when the launch runs on a nonzero-priority stream. For example, if the stream priority is -1 and the caller passes LaunchConfig(priority=0), the expected behavior is to emit CU_LAUNCH_ATTRIBUTE_PRIORITY=0 and override the stream priority. The current code emits no priority attribute, so the launch inherits -1 instead. This also conflicts with the PR description's original distinction that 0 is meaningful rather than an unset sentinel.
Could we keep the public/int storage requested in the earlier review but track whether the option was explicitly supplied separately (for example with a _priority_is_set flag), and add a regression that distinguishes None from 0?
|
/ok to test 070353d |
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Rechecked current head 070353d8. The priority=0 ambiguity I raised is now resolved by making the contract explicit: both the API docs and implementation treat 0 as the unset/inherit-stream-priority value, and the native-conversion tests cover that behavior. This is internally consistent now; no remaining concern from my earlier review.
| prio = priority | ||
| if not (low <= prio <= high): | ||
| raise ValueError(f"{priority=} is out of range {[low, high]}") | ||
| self.priority = prio |
There was a problem hiding this comment.
The above code is identical to that in the stream priority setter. Please merge the common code.
There was a problem hiding this comment.
Can be done in a follow up.
Got it. Yes, priority can be set through either stream or launch config. Stream setting works for all launches. Thank you for helping check the edge case. |
|
…-2629-signed Integrate upstream LaunchConfig priority support (NVIDIA#2706) with cluster scheduling policy while preserving PR commit history. Signed-off-by: Omar Atie <atiaomar1978@gmail.com>
3 participants
Description
issues #2631
Adds a
priorityattribute toLaunchConfigthat maps toCU_LAUNCH_ATTRIBUTE_PRIORITY, following the same pattern used forprogrammatic_stream_serialization(#1334).priority: int | None = None(defaultNone) — when omitted, the launch uses the stream's priority, matching existingLaunchConfigattribute conventions.__init__against the device's stream priority range: a nonzeropriorityoutside[greatestPriority, leastPriority], as returned bycuCtxGetStreamPriorityRange, raisesValueError.intwith0meaning "unset", soconfig.priorityreads back0(notNone) when omitted, and both native-config conversion paths (LaunchConfig._to_native_launch_configand the module-level_to_native_launch_config) use a truthiness check, emitting no launch attribute for0. This makes an explicitpriority=0indistinguishable from an unset priority, which is behaviorally equivalent in practice: on devices without multiple stream priorities both bounds are0, and elsewhere0isleastPriority, so omitting the attribute and inheriting the stream's priority yields the same scheduling.0/nonzero priorities, and an updatedLaunchConfigrepr pattern intest_object_protocols.py.