feat(cuda.core): add cluster scheduling policy to LaunchConfig - #2703
feat(cuda.core): add cluster scheduling policy to LaunchConfig#2703atiaomar1978-hub wants to merge 12 commits into
Conversation
Expose CU_LAUNCH_ATTRIBUTE_CLUSTER_SCHEDULING_POLICY_PREFERENCE on LaunchConfig via ClusterSchedulingPolicyType, with validation, CC >= 9.0 gating, and tests mapping to the native launch attribute. Closes NVIDIA#2629 Co-authored-by: Cursor <[email protected]> Signed-off-by: Omar Atie <[email protected]>
Assert LaunchConfig emits both CU_LAUNCH_ATTRIBUTE_CLUSTER_DIMENSION and CU_LAUNCH_ATTRIBUTE_CLUSTER_SCHEDULING_POLICY_PREFERENCE when set. Co-authored-by: Cursor <[email protected]> Signed-off-by: Omar Atie <[email protected]>
Use init_cuda with an inline noop kernel so test_launcher.py does not depend on the get_saxpy_kernel_cubin fixture from test_module.py. Co-authored-by: Cursor <[email protected]> Signed-off-by: Omar Atie <[email protected]>
…rough Align NVIDIA#2629 tests with reviewer guidance: round-trip each policy on LaunchConfig and exercise launch() for DEFAULT/SPREAD/LOAD_BALANCING. Co-authored-by: Cursor <[email protected]> Signed-off-by: Omar Atie <[email protected]>
b00cd6f to
c6c283b
Compare
|
Hi @lijinf2 — when you have a moment, could you please review this PR? This is the signed re-issue of #2702 (GitHub would not reopen #2668). It implements the plan we aligned on in #2629: Tests follow your guidance (getter/setter plus a I do not have permission to set assignee or milestone on this account; Sorry for the force-push on this branch. I know rewriting published history is against the project rules. I did it only so the commits would be SSH-signed and show as Verified on GitHub. I will not force-push again. |
|
/ok to test c6c283b |
|
CI test_subpackage_exports_are_documented[typing] failed because the new typing export was missing from docs/source/*.rst. Signed-off-by: Omar Atie <[email protected]>
|
Posted a docs fix for the CI failure on
RunPod (RTX 2000 Ada, CC 8.9 — L4 community was out of stock; same compute capability as the failing L4 jobs):
PR metadata is now set ( |
|
Hi @lijinf2 — could you please trigger CI again on The previous Why CI failedEvery GPU job died on the same assertion — not on
Why it works again
That name is now present in RunPod replay (pod
|
|
/ok to test 39c9fc0 |
|
@atiaomar1978-hub Hey, there are 7 test cases, and it looks like some are redundant. Could we remove redundancy and merge them into at most 3 cases to reduce code maintenance overhead going forward? |
|
I will do now @lijinf2 |
Merge seven overlapping LaunchConfig cluster-scheduling tests into mapping, rejection, and Hopper launch smoke to cut maintenance. Signed-off-by: Omar Atie <[email protected]>
|
@lijinf2 folded the seven cluster-policy tests into three in
Could you |
|
/ok to test 1a78ad5 |
CUDA 13.0.2's CUlaunchAttributeValue setter reads .value, so a raw int breaks native LaunchConfig conversion in CI. Signed-off-by: Omar Atie <[email protected]>
|
@lijinf2 CUDA 13.0.2 jobs failed on RunPod replay (RTX 2000 Ada, bindings 13.0.3): mapping + reject PASSED; smoke SKIPPED on CC 8.9 (already passed on CI H100). Could you please |
…-2629-signed Signed-off-by: Omar Atie <[email protected]>
|
/ok to test 3a90298 |
@lijinf2, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
Drop ClusterSchedulingPolicyType so callers pass DEFAULT, SPREAD, or LOAD_BALANCING without importing a new enum. Signed-off-by: Omar Atie <[email protected]>
Cython types the argument as str, so only DEFAULT/SPREAD/LOAD_BALANCING are accepted. Signed-off-by: Omar Atie <[email protected]>
|
@lijinf2 addressed the review on RunPod H100 SXM (CC 9.0), HEAD
3 passed, 0 skipped. Could you please |
Signed-off-by: Omar Atie <[email protected]>
|
/ok to test c15bf56 |
@lijinf2, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
|
/ok to test d185a86 |
|
Thanks @lijinf2 for triggering the CI builds and tests 👍 |
| 'cluster_scheduling_policy_preference', | ||
| ) | ||
|
|
||
| _CLUSTER_SCHED_POLICY_NAMES = ( |
There was a problem hiding this comment.
Let's remove this variable and reuse _CLUSTER_SCHED_POLICY_TO_DRIVER dictionary keys. In future maintainance we hope less variables.
There was a problem hiding this comment.
Done in e85fafe — removed _CLUSTER_SCHED_POLICY_NAMES; validation now uses _CLUSTER_SCHED_POLICY_TO_DRIVER.keys().
| ) | ||
|
|
||
|
|
||
| def _cluster_sched_policy_to_driver(value): |
There was a problem hiding this comment.
delete this function to use _CLUSTER_SCHED_POLICY_TO_DRIVER[value] directly.
There was a problem hiding this comment.
Done in e85fafe — removed _cluster_sched_policy_to_driver; lookup is _CLUSTER_SCHED_POLICY_TO_DRIVER[value] via LaunchConfig._cluster_sched_policy_driver_value().
| "LOAD_BALANCING", | ||
| ) | ||
|
|
||
| _CLUSTER_SCHED_POLICY_TO_DRIVER = { |
There was a problem hiding this comment.
Let's combine _CLUSTER_SCHED_POLICY_TO_DRIVER, and _validate_cluster_scheduling_policy_preference into a unified function (maybe _validate_cluster_scheduling_policy_preference or better name)?
Also, please put the unified function under LaunchConfig as a member function not global.
There was a problem hiding this comment.
Done in e85fafe — merged validation into LaunchConfig._validate_cluster_scheduling_policy_preference (member method).
| "cluster launch attributes are not supported on devices with " | ||
| f"compute capability < 9.0 (got {cc})" | ||
| ) | ||
| if cluster is not None: |
There was a problem hiding this comment.
let's keep lines 142-150 unchanged and add compute capability checking to the unified function.
There was a problem hiding this comment.
Done in e85fafe — lines 142–150 unchanged; CC check for policy is also in the member validator.
| attr.value.clusterSchedulingPolicyPreference = _cluster_sched_policy_to_driver( | ||
| config.cluster_scheduling_policy_preference | ||
| ) | ||
| attrs.append(attr) |
There was a problem hiding this comment.
Why do we set twice? one at line 217-223, and the other at line 285-292?
There was a problem hiding this comment.
The attribute is appended in two conversion paths until all modules are cythonized: the cdef _to_native_launch_config (cydriver structs) and the cpdef module helper (Python driver.CUlaunchAttribute objects). e85fafe factors shared driver lookup into LaunchConfig._cluster_sched_policy_driver_value().
| assert (attr.value.clusterDim.x, attr.value.clusterDim.y, attr.value.clusterDim.z) == (2, 2, 2) | ||
|
|
||
|
|
||
| _CLUSTER_SCHED_POLICIES = ( |
There was a problem hiding this comment.
Let's remove _CLUSTER_SCHED_POLICIES and import.
There was a problem hiding this comment.
Done in e85fafe — tests iterate _CLUSTER_SCHED_POLICY_TO_DRIVER keys instead of a local tuple.
@atiaomar1978-hub Hey, are you a human contributor or a code agent contributor? Your replies and code style read like automatic agent-generated content, with little human review/refinement. |
|
human + agent together and integration with RunPod to Test @lijinf2 |
…onfig Move validation and driver lookup onto LaunchConfig members, drop duplicate policy name constants, and reuse _CLUSTER_SCHED_POLICY_TO_DRIVER in tests. Signed-off-by: Omar Atie <[email protected]>
|
Addressed latest review feedback in e85fafe:
RunPod (H100, CC 9.0, cuda.bindings 13.3.1): 3/3 cluster scheduling policy tests passed. Ready for |
|
/ok to test e85fafe |
|
The failing job Test win-64 / Python 3.14, CUDA 13.3.0 (wheels), GPU h100 (x2) (MCDM) (run) does not look related to this PR's code changes. It failed during Configure driver mode ( On the same commit ( Happy to re-run that job if helpful. Cluster policy coverage was verified separately on RunPod H100 (3/3 tests passed). |
|
@lijinf2 When you have a moment, could you please take another look at the latest review feedback addressed in e85fafe? The only failing check on that commit was the Windows dual-H100 (MCDM) job, which failed during Configure driver mode before any cuda.core tests ran. That looks like a runner/infra issue rather than a regression from this PR. If you agree, a re-run of that job (or a fresh Cluster scheduling policy coverage was verified on RunPod H100 (3/3 tests passed). Thanks again for the detailed review. |
|
@lijinf2 Friendly follow-up on All review feedback from the last round has been addressed, and CI is now green on that commit — including the Windows dual-H100 (MCDM) job, which passed on re-run (previously failed during Configure driver mode before tests started). Summary of
When you have time, could you please take another look and let me know if anything else is needed before merge? Thank you. |
Description
closes #2629
Add
cluster_scheduling_policy_preferencetoLaunchConfig, mapping toCU_LAUNCH_ATTRIBUTE_CLUSTER_SCHEDULING_POLICY_PREFERENCEviaClusterSchedulingPolicyType(DEFAULT,SPREAD,LOAD_BALANCING).Unlike stream synchronization policy (#2628), cluster scheduling policy is
documented as valid for graph nodes and kernel launches, so
LaunchConfigisthe correct surface.
This is a signed re-issue of #2702 (same change, linear history on current
main). Commits are SSH-signed. GitHub would not reopen #2668.Checklist
Test plan
cluster+ policy attributeslaunch()smoke for all three policies