[https://nvbugs/6561777][fix] Add slurm_wait_all_ranks(), a job+step-keyed marker barrier counting… - #17370
Conversation
TestDeepSeekR1::test_nvfp4_multi_gpus[latency_adp_lmtp] is the fourth test row attributed to a single multi-node launch defect, alongside nvbugs/6541343, 6561775 and 6561778 -- all from build 2885 on the same GB200 8-GPU 2-node stage family, all reporting the same "Test terminated unexpectedly" with no traceback. That string is synthesized, not a crash: generate_timeout_xml.py fabricates a testcase with it for every nodeid left in unfinished_test.txt, so it only means the srun step was killed while the test was in flight. The only install lock lives under $resourcePathNode (/tmp) in slurm_install.sh. In a pyxis container /tmp is a per-step tmpfs, so that lock is node-local: its wait loop fences just the $SLURM_LOCALID peers on the same node, and a node can never observe another node's lock. Nothing then stops slurm_run.sh from reaching `eval $pytestCommand` on one node while another is still installing, and the per-rank work in between skews the ranks further: non-zero ranks cover rank 0's coverage-config write with a blind `sleep 30`, and slurm_setup_runtime_env shells out to pip3. Pytest's first action is `import tensorrt_llm`, whose module-scope MPI collective must be entered by every rank. Under --mpi=pmix, which is added exactly when nodeCount > 1, that collective has a 300s fence timeout, so a node whose pip3 install stalls (up to the 2700s retry budget) makes the collective abort every rank rather than merely run late. The ranks then die between pytest setup and teardown, which is what leaves the nodeid in unfinished_test.txt. Note that PMIX_MCA_gds=hash does not mitigate this: a fence that times out never exchanges the modex regardless of GDS mode, and the pml_ucx errors seen alongside it are downstream of the same missing exchange. Add a marker barrier on the shared $jobWorkspace immediately before `eval $pytestCommand`. It counts SLURM_NTASKS rank markers rather than nodes, so the fenced set is exactly the set that enters the aborting collective, and placing it after the block that wipes SLURM_* keeps it a no-op for single-node and disaggregated benchmark/server runs, which reach it with SLURM_NTASKS unset. The marker directory is keyed per job and per step because $jobWorkspace outlives a step, so a later step must not be released by an earlier one's markers. The wait is bounded above the 2700s pip3 budget so a genuinely dead rank fails the stage with a clear message instead of hanging until the partition walltime. This is the same change already proposed for nvbugs/6541343; it is carried here byte-identically so whichever lands first makes the other a no-op. This variant's ISOLATION marker in l0_gb200_multi_nodes.yml does not change any of the above: runIsolatedTests is only reachable from runLLMTestlistOnPlatformImpl, the Docker path, so on the Slurm path the marker is merely stripped and the test shares one slurm_run.sh pytest with its shard. Verified by asserting elapsed seconds, since a no-op barrier also returns 0: single rank, unset workspace and unset SLURM_NTASKS return at 0s; a full marker set releases at 0s; markers from another job, from an earlier step of the same job, and a dead rank each fail bounded with rc=1 through the ERR trap. On a live 2-node 8-rank pmix launch with rank 7 delayed 20s, ranks on both hosts blocked 20-30s and every rank agreed on the same SLURM_STEP_ID, showing the fence crosses the node boundary; with no delay all 8 released within one poll interval. Also un-waive the test, whose body was already healthy. Signed-off-by: trtllm-agent <296075020+trtllm-agent@users.noreply.github.com>
|
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 (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review. WalkthroughThe integration waiver list adds skips for affected Llama, DeepSeek, Cosmos3 Nano, and LTX2 tests. It removes obsolete waivers for Step3.7 FP8 block scales, overlap-scheduler consistency, and advanced sampling. ChangesIntegration test waivers
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to The PR adds a cross-rank synchronization point for multi-node Slurm tests and removes the associated waiver; no actionable merge-blocking risk remains beyond normal checks and review. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 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/slurm_run.sh`:
- Around line 75-127: Add the standard NVIDIA copyright header for 2026 at the
beginning of slurm_run.sh, before the existing script content. Do not alter the
slurm_wait_all_ranks implementation or other script behavior.
🪄 Autofix
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: 2158a1a7-f8f8-4ba4-9b7f-ab6064d0c30a
📒 Files selected for processing (2)
jenkins/scripts/slurm_run.shtests/integration/test_lists/waives.txt
💤 Files with no reviewable changes (1)
- tests/integration/test_lists/waives.txt
brnguyen2
left a comment
brnguyen2
left a comment
There was a problem hiding this comment.
Approving — the comments below are optional touch-ups, not blockers.
The mechanism holds up: $jobWorkspace is under /home/svc_tensorrt/bloom/scripts, which is in the container mount list (getMountListForSlurmTest), so it really is shared across nodes, and the install lock it complements is keyed ..._node_${SLURM_NODEID} on the container-local path. Placement after the SLURM_* wipe does keep single-node stages a 0s no-op. Worth noting srun --kill-on-bad-exit=1 (L0_Test.groovy:1952) already covers the dead-rank case, so the barrier only has to handle slow ranks.
Two things beyond the inline notes:
- Scope:
jenkins/scripts/perf/local/slurm_run.shhas the same structure andperf/local/slurm_install.shhas the same per-node install lock (its build lock is cross-node, the install lock isn't). Multi-node aggregated perf stages should still be able to hit this. Fine as a follow-up, but say so in the description rather than leaving it implicit. - Validation: the un-waived test only runs in a post-merge multi-node GB200 stage, so a plain
/bot runwon't exercise it. Please run that stage explicitly (--stage-list/--extra-stage) and link the result. The siblingtest_nvfp4_multi_gpus[latency]waiver (nvbugs/6561778) has a similar signature — worth checking whether it clears too.
StanleySun639
left a comment
StanleySun639
left a comment
There was a problem hiding this comment.
LGTM if the target test case can pass.
ZhanruiSunCh
left a comment
ZhanruiSunCh
left a comment
There was a problem hiding this comment.
LGTM for infra part. The barrier design and root-cause analysis are solid. Two nits worth addressing (not blocking, both already noted by brnguyen2 inline):
-
The 3600s timeout comment claims it is bounded above the 2700s pip retry budget, but slurm_install_setup runs several retry_command calls in sequence (wget, apt, ray, opencv, requirements-dev, wheel), so worst-case legal install time can exceed 3600s. Consider deriving the bound from the sum of install steps, or reword the comment to describe it as a walltime-shaped backstop rather than a guaranteed bound.
-
The "no-op for disaggregated runs" comment is only accurate for DISAGG_SERVING_TYPE in {BENCHMARK, DISAGG_SERVER}. Harmless today since this script is launched by a single srun covering every task, but worth rewording so a future reader does not assume the barrier can never run on a disagg path.
Signed-off-by: Emma Qiao <qqiao@nvidia.com>
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/bot run |
|
PR_Github #68649 [ run ] triggered by Bot. Commit: |
|
PR_Github #68649 [ run ] completed with state
|
|
/bot run |
|
PR_Github #68703 [ run ] triggered by Bot. Commit: |
|
PR_Github #68703 [ run ] completed with state |
Signed-off-by: Emma Qiao <qqiao@nvidia.com>
EmmaQiaoCh
added
the
ci: post-merge approved
|
/bot run --stage-list "GB200-8_GPUs-2_Nodes-PyTorch-Post-Merge-*" |
|
PR_Github #68763 [ run ] triggered by Bot. Commit: |
|
PR_Github #68763 [ run ] completed with state |
|
/bot run --post-merge |
|
PR_Github #68976 [ run ] triggered by Bot. Commit: |
|
PR_Github #68976 [ run ] completed with state |
7 participants
Summary
eval $pytestCommandwhile another still installs, and the pmix 300s fence timeout then aborts all 8 ranks instead of merely running late.pytest tests/integration/defs/accuracy/test_llm_api_pytorch.py::TestDeepSeekR1::test_nvfp4_multi_gpus[latency_adp_lmtp] -vTest plan
Links
Dev Engineer Review
tests/integration/test_lists/waives.txt.nvbugsreferences, and contain no duplicates.slurm_wait_all_ranks()implementation and the related waiver removal are included in the PR.QA Engineer Review
test-db/orqa/files were reported as modified.tests/integration/test_lists/waives.txtcontains test-list-only changes.