[https://nvbugs/6561778][fix] Fence all ranks before pytest launch in multi-node slurm_run.sh - #17372
[https://nvbugs/6561778][fix] Fence all ranks before pytest launch in multi-node slurm_run.sh#17372trtllm-agent wants to merge 1 commit into
Conversation
The only install lock (slurm_install.sh) lives under $resourcePathNode, which is /tmp -- node-local, and a per-step tmpfs under pyxis -- so its wait loop fences just the $SLURM_LOCALID peers of one 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 die between pytest setup and teardown, which leaves the nodeid in unfinished_test.txt and makes generate_timeout_xml.py synthesize the "Test terminated unexpectedly" this bug reports -- there is no traceback for the test body. 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 that 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. The barrier is byte-identical to the one reviewed on the sibling attribution of this same defect (6541343), so whichever lands first collapses the other's hunk on rebase. Also un-waive this bug's test, whose body was already healthy: the reproduce log passes all three accuracy phases (MMLU 87.013 vs 85.513, GSM8K 95.375 vs 92.217, CnnDailymail rouge1 30.271 vs 26.716) and fails only on an fsspecIO thread leaked by HuggingFace's downloader, an environment artifact unrelated to the launch defect. 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 (2)
💤 Files with no reviewable changes (1)
WalkthroughThe Slurm launcher now waits for all multi-rank processes before pytest. The change also removes the waiver for the DeepSeekR1 NVFP4 multi-GPU latency test. ChangesSlurm test execution
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SlurmRank
participant SharedFilesystem
participant Pytest
SlurmRank->>SharedFilesystem: Create job and step readiness marker
SlurmRank->>SharedFilesystem: Wait for all rank markers
SharedFilesystem-->>SlurmRank: Report ready-rank count
SlurmRank->>Pytest: Start pytest after barrier completion
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
brnguyen2
left a comment
brnguyen2
left a comment
There was a problem hiding this comment.
The mechanism checks out: slurm_install.sh's lock lives under $resourcePathNode (node-local), the aggregated path launches one srun with --mpi=pmix for multi-node, and $jobWorkspace (/home/svc_tensorrt/bloom/scripts/...) is in getMountListForSlurmTest, so it is visible to every rank — the barrier is placed and keyed correctly (job + step). Timeout failure propagates through the ERR trap into --kill-on-bad-exit=1, which is the right blast radius.
Two things before merge:
-
The PR states the fence can't be exercised by the reproduction environment (a warm container never runs
slurm_run.sh), so nothing has run the changed code on the path it's meant to fix. Since the waiver comes out in the same PR, please run the actual 2-node post-merge stage —/bot run --extra-stage "GB200-8_GPUs-2_Nodes-PyTorch-Post-Merge-1"— and link the result. -
waives.txtstill carries neighbouring multi-node DeepSeekR1 entries (6561775, 6561777). If this is an infra-level fence fix, are those expected to stay waived, or is the scope deliberately one test?
Also: the 11-line preamble at slurm_run.sh:75 is a post-mortem, not a code comment. 3–4 lines plus the bug ID would age better.
| "all $numRanks ranks to be ready; ready: $ready/$numRanks" | ||
| return 1 | ||
| fi | ||
| # One rank reports progress; all of them would spam the log every 10s. |
There was a problem hiding this comment.
This guard doesn't do what the comment says. The script runs under set -xEeuo pipefail (line 4) and never disables tracing before here, so every rank already emits the full trace of the loop body — the glob assignment, both [ tests, the sleep — to the single #SBATCH --output= log every 10s. Suppressing the echo on non-zero ranks removes one line out of ~6 per rank per iteration.
Either wrap the loop in set +x / set -x (and keep the rank-0 echo as the only progress signal), or drop the guard and the comment.
| mkdir -p "$readyDir" | ||
| touch "$readyDir/rank_${SLURM_PROCID}.ready" | ||
|
|
||
| # Bounded so a dead rank fails the stage loudly instead of hanging until the |
There was a problem hiding this comment.
The stated justification for 3600s doesn't hold. slurm_install.sh has several retry_command --timeout 2700 calls plus a --timeout 1800 wget, so its worst-case budget is well over an hour, and the non-LOCALID-0 branch waits on the lock with no timeout at all. "Exceeds the 2700s pip3 retry budget" is a per-command bound, not a bound on install duration.
What 3600s actually bounds is arrival skew — each rank starts its own deadline after finishing its own install — which is the right thing to bound and comfortably above the ~10min skew in the bug. Worth saying that instead, since the current wording invites someone to "fix" the number against the wrong quantity later.
| @@ -21,7 +21,6 @@ accuracy/test_llm_api_autodeploy.py::TestQwen3_5_397B_MoE::test_bf16_small[4] SK | |||
| accuracy/test_llm_api_pytorch.py::TestDeepSeekR1::test_fp8_blockscale[throughput] SKIP (https://nvbugs/6561775) | |||
There was a problem hiding this comment.
This removal is the only user-visible part of the PR and it is the least-verified. The PR body says the fence "can't be exercised by the pytest verify run at all (a warm container never executes slurm_run.sh)" — so the code path being fixed was never executed end-to-end, only an extracted copy of the helper was.
Please post /bot run --extra-stage "GB200-8_GPUs-2_Nodes-PyTorch-Post-Merge-1" and link the green run before merging; otherwise this re-enables a test in post-merge on a mechanism argument alone.
3 participants
Summary
jenkins/scripts/slurm_run.shhas no cross-rank synchronization betweenslurm_install_setupandeval $pytestCommand— the install lock inslurm_install.shlives under$resourcePathNode(/tmp), so it is node-local and can only fenceSLURM_LOCALIDpeers on the same host, never another node. Per-rank work amplifies the skew (non-zero ranks cover the coverage-config write with a blindsleep 30, andslurm_setup_runtime_envshells out to pip3), so one node reaches pytest while another is still installing. Pytest's first action isimport tensorrt_llm, whose module-scope MPI collective runs under--mpi=pmix(added exactly when nodeCount > 1) with a 300s ring fence timeout, so the skew aborts every rank instead of merely running late — the same defect already solved for sibling bug 6541343.slurm_wait_all_ranksbarrier on the shared$jobWorkspaceimmediately beforeeval $pytestCommand: each rank touches a marker under a directory keyed by bothSLURM_JOB_IDandSLURM_STEP_ID(so markers from another job or an earlier step of the same job cannot satisfy the count) and polls a glob count every 10s until allSLURM_NTASKSranks are present. The wait is bounded at 3600s — above the 2700s pip3 retry budget inslurm_install.shso a merely slow rank still releases the barrier, while a dead rank fails the stage loudly instead of hanging to the partition walltime — and it no-ops for single-rank or unset-jobWorkspaceruns. The function was validated separately (a warm container never executesslurm_run.sh, so the pytest verify cannot exercise it): the function was extracted verbatim (md5-matched) and all 8 cases were run live on 2 nodes, confirming cross-host blocking at the same step, one-poll release for concurrent ranks, correct no-ops,rc=1at exactly the timeout for a missing rank, and correct rejection of stale earlier-step and other-job markers. The waiver forTestDeepSeekR1::test_nvfp4_multi_gpus[latency]is removed accordingly.pytest tests/integration/defs/accuracy/test_llm_api_pytorch.py::TestDeepSeekR1::test_nvfp4_multi_gpus[latency] -vTest plan
Links
Dev Engineer Review
jobWorkspacebarrier before pytest execution.SLURM_NTASKSmarkers.jobWorkspaceskip the barrier.accuracy/test_llm_api_pytorch.py::TestDeepSeekR1::test_nvfp4_multi_gpus[latency].QA Engineer Review
tests/integration/test_lists/waives.txt.test-db/orqa/files were modified.