[None][infra] Select UCX env for perf sanity by cluster name - #16725
[None][infra] Select UCX env for perf sanity by cluster name#16725chuangz0 wants to merge 5 commits into
Conversation
014bcd8 to
8f91714
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (6)
WalkthroughThe PR adds cluster-aware UCX environment selection for local and Jenkins PerfSanity Slurm submissions. It introduces shared cluster/GPU matching helpers, propagates optional cluster names through launchers, detects Slurm cluster names locally, and removes one integration test waiver. ChangesCluster-aware UCX configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Jenkins
participant Submit as perf/submit.py
participant ClusterEnv as cluster_env.py
participant Slurm
Jenkins->>Submit: pass stage name and cluster name
Submit->>ClusterEnv: derive GPU type and select UCX settings
ClusterEnv-->>Submit: return UCX environment prefix
Submit->>Slurm: launch workers with UCX settings
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/bot run --stage-list "DGX_B200-16_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-FUNCTIONAL-ONLY-CTX1-NODE1-GPU4-GEN1-NODE1-GPU8" |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
jenkins/scripts/perf/cluster_env.py (1)
58-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the public helper contracts in Google style.
These imported helpers omit
Args/Returnsdocumentation, including empty-result and wildcard-matching behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jenkins/scripts/perf/cluster_env.py` around lines 58 - 91, Update the public helper docstrings for gpu_type_from_stage_name, gpu_type_from_supported_gpus, and get_ucx_tls_cmd to use Google-style Args and Returns sections. Document accepted inputs, the empty-string fallback behavior, and for get_ucx_tls_cmd explain wildcard matching against cluster and GPU patterns and the returned shell-prefix format.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@jenkins/scripts/perf/local/run_disagg.sh`:
- Line 70: Add the standard NVIDIA copyright header with the current year at the
top of the modified script, before the existing shell content; leave the
cluster_name initialization unchanged.
---
Nitpick comments:
In `@jenkins/scripts/perf/cluster_env.py`:
- Around line 58-91: Update the public helper docstrings for
gpu_type_from_stage_name, gpu_type_from_supported_gpus, and get_ucx_tls_cmd to
use Google-style Args and Returns sections. Document accepted inputs, the
empty-string fallback behavior, and for get_ucx_tls_cmd explain wildcard
matching against cluster and GPU patterns and the returned shell-prefix format.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b568251b-1320-47a1-a913-b4e12a6e6213
📒 Files selected for processing (7)
jenkins/L0_Test.groovyjenkins/scripts/perf/cluster_env.pyjenkins/scripts/perf/local/configs/example.confjenkins/scripts/perf/local/run_disagg.shjenkins/scripts/perf/local/submit.pyjenkins/scripts/perf/submit.pytests/integration/test_lists/waives.txt
💤 Files with no reviewable changes (1)
- tests/integration/test_lists/waives.txt
|
PR_Github #60966 [ run ] triggered by Bot. Commit: |
|
PR_Github #60966 [ run ] completed with state
|
BowenFu
left a comment
BowenFu
left a comment
There was a problem hiding this comment.
CI perf-sanity infra only (cluster-aware UCX env selection + waiver removal); no tensorrt_llm/** or cpp/** paths. LGTM.
chuangz0
changed the title
|
/bot run --stage-list "DGX_B200-16_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-FUNCTIONAL-ONLY-CTX1-NODE1-GPU4-GEN1-NODE1-GPU8" --disable-fail-fast |
|
PR_Github #61243 [ run ] triggered by Bot. Commit: |
|
PR_Github #61243 [ run ] completed with state
|
b9d5992 to
83d14fa
Compare
|
/bot run --stage-list "DGX_B200-16_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-FUNCTIONAL-ONLY-CTX1-NODE1-GPU4-GEN1-NODE1-GPU8" --disable-fail-fast |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@jenkins/scripts/perf/local/submit.py`:
- Around line 354-359: Update the subprocess call in the configuration lookup
try block to invoke scontrol through a deployment-trusted absolute executable
path, or validate the resolved executable path before use. Keep the existing
arguments and timeout behavior unchanged, and do not suppress Ruff S607.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5b447329-5f5f-4081-be7c-2f698e346788
📒 Files selected for processing (7)
jenkins/L0_Test.groovyjenkins/scripts/perf/cluster_env.pyjenkins/scripts/perf/local/configs/example.confjenkins/scripts/perf/local/run_disagg.shjenkins/scripts/perf/local/submit.pyjenkins/scripts/perf/submit.pytests/integration/test_lists/waives.txt
💤 Files with no reviewable changes (1)
- tests/integration/test_lists/waives.txt
🚧 Files skipped from review as they are similar to previous changes (5)
- jenkins/scripts/perf/local/run_disagg.sh
- jenkins/L0_Test.groovy
- jenkins/scripts/perf/cluster_env.py
- jenkins/scripts/perf/local/configs/example.conf
- jenkins/scripts/perf/submit.py
|
PR_Github #61266 [ run ] triggered by Bot. Commit: |
|
PR_Github #61266 [ run ] completed with state
|
83d14fa to
191be00
Compare
|
/bot run --stage-list "DGX_B200-16_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-FUNCTIONAL-ONLY-CTX1-NODE1-GPU4-GEN1-NODE1-GPU8*" --disable-fail-fast |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
jenkins/scripts/perf/local/submit.py (1)
349-350: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the return annotation to the new helper.
Declare this function as
def detect_cluster_name() -> str:. As per coding guidelines, annotate every function. Based on learnings, missing annotations in CI/infra Python may be deferred rather than block this functional PR.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jenkins/scripts/perf/local/submit.py` around lines 349 - 350, Update the detect_cluster_name function signature to include the str return annotation, preserving its existing behavior and implementation.Sources: Coding guidelines, Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@jenkins/scripts/perf/local/submit.py`:
- Around line 687-690: Guard the GPU and cluster detection in the submission
flow so it runs only when runtime_mode equals "disaggregated". Move
supported_gpus/gpu_type derivation and cluster_name assignment under that
condition, preserving their existing sources and leaving aggregated submissions
without Slurm detection.
- Around line 361-363: Update the Slurm config parsing loop to match the exact
ClusterName key and only split and return when the line contains an "="
separator; otherwise skip malformed lines so the existing best-effort fallback
remains reachable.
---
Nitpick comments:
In `@jenkins/scripts/perf/local/submit.py`:
- Around line 349-350: Update the detect_cluster_name function signature to
include the str return annotation, preserving its existing behavior and
implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b2088bed-120a-4696-aab3-b4ad985b791f
📒 Files selected for processing (7)
jenkins/L0_Test.groovyjenkins/scripts/perf/cluster_env.pyjenkins/scripts/perf/local/configs/example.confjenkins/scripts/perf/local/run_disagg.shjenkins/scripts/perf/local/submit.pyjenkins/scripts/perf/submit.pytests/integration/test_lists/waives.txt
💤 Files with no reviewable changes (1)
- tests/integration/test_lists/waives.txt
🚧 Files skipped from review as they are similar to previous changes (5)
- jenkins/L0_Test.groovy
- jenkins/scripts/perf/local/configs/example.conf
- jenkins/scripts/perf/local/run_disagg.sh
- jenkins/scripts/perf/cluster_env.py
- jenkins/scripts/perf/submit.py
|
PR_Github #61279 [ run ] triggered by Bot. Commit: |
|
PR_Github #61279 [ run ] completed with state
|
7912d80 to
27827f0
Compare
|
/bot run --stage-list "DGX_B200-16_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-FUNCTIONAL-ONLY-CTX1-NODE1-GPU4-GEN1-NODE1-GPU8*" --disable-fail-fast |
|
PR_Github #62082 [ run ] triggered by Bot. Commit: |
|
PR_Github #62082 [ run ] completed with state |
…of GPU type UCX transport settings depend on the cluster's network fabric, not just the GPU model, and the same CI stage can land on different clusters (frontend auto:* platforms are load-balanced in bloom's SlurmConfig). Add jenkins/scripts/perf/cluster_env.py with a (cluster, GPU) -> UCX env rule table shared by the CI and local submit scripts: - all clusters: unset UCX_CUDA_IPC_ENABLE_MNNVL UCX_TLS UCX_NET_DEVICES - nsc-svg: pin UCX_NET_DEVICES to the usable mlx5 ports - aws-cmh: pin UCX_TLS=cuda_ipc,cuda_copy,sm,self,tcp - aws-dfw: exclude gdr_copy Cluster patterns are prefix wildcards so both the bloom clusterName (CI) and the cluster's slurm.conf ClusterName (e.g. oci-nrt -> oci-nrt-cs-001) match. L0_Test.groovy passes the resolved partition.clusterName via --cluster-name; the local path takes it from the run_disagg.sh cluster conf or --cluster-name, falling back to detection via SLURM_CLUSTER_NAME / scontrol. Signed-off-by: Chuang Zhu <111838961+chuangz0@users.noreply.github.com>
Remove the waive for disagg_upload-gen_only-b200_deepseek-r1-fp4_8k1k_con1536_ctx1_dep4_gen1_dep8_eplb0_mtp1_ccb-NIXL (https://nvbugs/6478615): the failure is addressed by the cluster-specific UCX env selection introduced in the previous commit. Signed-off-by: Chuang Zhu <111838961+chuangz0@users.noreply.github.com>
- Add NVIDIA copyright header to run_disagg.sh - Use absolute path /usr/bin/scontrol to avoid PATH-dependent lookup - Harden ClusterName parsing with partition() to tolerate malformed lines - Guard GPU/cluster detection to run only for disaggregated mode - Expand cluster_env.py public function docstrings to Google style Signed-off-by: Chuang Zhu <111838961+chuangz0@users.noreply.github.com>
Signed-off-by: Chuang Zhu <111838961+chuangz0@users.noreply.github.com>
Signed-off-by: Chuang Zhu <111838961+chuangz0@users.noreply.github.com>
99f0625 to
0137d47
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #62136 [ run ] triggered by Bot. Commit: |
|
PR_Github #62136 [ run ] completed with state
|
5 participants
UCX transport settings depend on the cluster's network fabric, not just the GPU model, and the same CI stage can land on different clusters (frontend auto:* platforms are load-balanced in bloom's SlurmConfig).
Add jenkins/scripts/perf/cluster_env.py with a (cluster, GPU) -> UCX env rule table shared by the CI and local submit scripts:
Cluster patterns are prefix wildcards so both the bloom clusterName (CI) and the cluster's slurm.conf ClusterName (e.g. oci-nrt -> oci-nrt-cs-001) match. L0_Test.groovy passes the resolved partition.clusterName via --cluster-name; the local path takes it from the run_disagg.sh cluster conf or --cluster-name, falling back to detection via SLURM_CLUSTER_NAME / scontrol.
Dev Engineer Review
jenkins/scripts/perf/cluster_env.py:BASE_UCX_UNSETclearsUCX_CUDA_IPC_ENABLE_MNNVL,UCX_TLS, andUCX_NET_DEVICESto prevent UCX leakage from the parent environment.UCX_ENV_RULESordered first-match, case-insensitive wildcard rules keyed by(cluster_name, gpu_type)using prefix wildcards for cluster patterns to match both bloom cluster names and SlurmClusterNamevariants.gcp-nrt*: pinsUCX_NET_DEVICEStorocep*ports and sets IB/RoCE tuning vars.nsc-svg*: pinsUCX_NET_DEVICESto the correct mlx5 ports (mlx5_*set).aws-cmh*: pinsUCX_TLS=cuda_ipc,cuda_copy,sm,self,tcp.aws-dfw*: excludesgdr_copyviaUCX_TLS=^gdr_copy.KNOWN_GPU_TYPESvia:gpu_type_from_stage_name(stage_name)(substring scan, ordered to avoid collisions like GB200 vs B200)gpu_type_from_supported_gpus(supported_gpus)(membership againstmetadata.supported_gpus)--cluster-namethrough CI and submit flows so UCX selection can use the resolved cluster name:jenkins/L0_Test.groovy: passes--cluster-name ${partition.clusterName}into the generated multi-nodesrun/SLURM launch.jenkins/scripts/perf/submit.py: adds--cluster-name(default""), derivesgpu_typefrom--stage-name, and usesget_ucx_tls_cmd(cluster_name, gpu_type)when constructing the worker/gen/benchmarkpytestCommand*env exports.jenkins/scripts/perf/local/submit.py: adds--cluster-name, implementsdetect_cluster_name()(prefersSLURM_CLUSTER_NAME, otherwise parsesscontrol show config’sClusterName), derivesgpu_typefromconfig.metadata.supported_gpus, and usesget_ucx_tls_cmd(cluster_name, gpu_type).jenkins/scripts/perf/local/run_disagg.sh: introduces optionalcluster_nameconfig and conditionally includes--cluster-name "$cluster_name"in thesubmit.pyinvocation only when set.jenkins/scripts/perf/local/configs/example.conf: documents the newcluster_nameoption and how it relates tocluster_env.pyrules.tests/integration/test_lists/waives.txt: removed the SKIP waiver forperf/test_perf_sanity.py::test_e2e[disagg_upload-gen_only-b200_deepseek-r1-fp4_8k1k_con1536_ctx1_dep4_gen1_dep8_eplb0_mtp1_ccb-NIXL](nvbugs/6478615), per the intended fix from cluster-specific UCX selection.QA Engineer Review
tests/integration/test_lists/waives.txtSKIPentry for:perf/test_perf_sanity.py::test_e2e[disagg_upload-gen_only-b200_deepseek-r1-fp4_8k1k_con1536_ctx1_dep4_gen1_dep8_eplb0_mtp1_ccb-NIXL](nvbugs/6478615)Description
Test Coverage
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.