[https://nvbugs/6561777][fix] Add slurm_wait_all_ranks(), a job+step-keyed marker barrier counting… - #17370
Open
trtllm-agent wants to merge 1 commit into
Open
[https://nvbugs/6561777][fix] Add slurm_wait_all_ranks(), a job+step-keyed marker barrier counting…#17370trtllm-agent wants to merge 1 commit into
trtllm-agent wants to merge 1 commit into
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>
trtllm-agent
requested review from
BowenFu,
QiJune,
StanleySun639 and
jieli-matrix
August 6, 2026 17:29
Contributor
WalkthroughChangesSlurm CI execution
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SlurmRank as Slurm rank
participant SharedWorkspace as Shared jobWorkspace
participant Pytest
SlurmRank->>SharedWorkspace: Create readiness marker
SlurmRank->>SharedWorkspace: Wait for all rank markers
SharedWorkspace-->>SlurmRank: Report readiness or timeout
SlurmRank->>Pytest: Execute pytest after readiness
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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
Comment on lines
+75
to
+127
| # The only install lock (slurm_install.sh) lives under $resourcePathNode, a per-step | ||
| # tmpfs in the pyxis container, so it fences just the $SLURM_LOCALID peers on one node | ||
| # -- nothing stops one node from reaching `eval $pytestCommand` below while another is | ||
| # still installing. Pytest's first action is `import tensorrt_llm`, whose module-scope | ||
| # MPI collective must be entered by every rank, and under --mpi=pmix (added exactly | ||
| # when nodeCount > 1) that collective has a 300s fence timeout -- so the skew aborts | ||
| # every rank instead of merely running late. Fence on the shared $jobWorkspace so all | ||
| # ranks enter pytest together. Must stay below the SLURM_* unset block above, which is | ||
| # what makes this a no-op for single-node and disaggregated runs. | ||
| slurm_wait_all_ranks() { | ||
| # SLURM_NTASKS rather than a node count: this is exactly the set of ranks that | ||
| # enters the aborting collective. | ||
| local numRanks="${SLURM_NTASKS:-1}" | ||
| if [ "$numRanks" -le 1 ] || [ -z "${jobWorkspace:-}" ]; then | ||
| return 0 | ||
| fi | ||
|
|
||
| # Keyed per job *and* per step: $jobWorkspace outlives a single step, so markers | ||
| # from another job or from an earlier step must not satisfy the count. Slurm gives | ||
| # every rank of a step the same step id, so all of them agree on this path. | ||
| local readyDir="$jobWorkspace/run_ready_job_${SLURM_JOB_ID:-local}_step_${SLURM_STEP_ID:-0}" | ||
| mkdir -p "$readyDir" | ||
| touch "$readyDir/rank_${SLURM_PROCID}.ready" | ||
|
|
||
| # Bounded above the 2700s pip3 retry budget in slurm_install.sh: a merely slow rank | ||
| # still releases the barrier, while a dead one fails the stage loudly instead of | ||
| # hanging until the partition walltime kills it. | ||
| local timeoutSecs=3600 | ||
| local deadline=$((SECONDS + timeoutSecs)) | ||
| local markers ready | ||
| while true; do | ||
| # Globbed, not `ls | wc -l`: under `set -Eeuo pipefail` a failing `ls` fires the | ||
| # ERR trap. The touch above guarantees a match, so no nullglob is needed. | ||
| markers=("$readyDir"/*.ready) | ||
| ready=${#markers[@]} | ||
| if [ "$ready" -ge "$numRanks" ]; then | ||
| return 0 | ||
| fi | ||
| if [ "$SECONDS" -ge "$deadline" ]; then | ||
| echo "ERROR: rank ${SLURM_PROCID} timed out after ${timeoutSecs}s waiting for" \ | ||
| "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. | ||
| if [ "$SLURM_PROCID" -eq 0 ]; then | ||
| echo "(Waiting for all $numRanks ranks to be ready) ready: $ready/$numRanks" | ||
| fi | ||
| sleep 10 | ||
| done | ||
| } | ||
|
|
||
| slurm_wait_all_ranks | ||
|
|
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
header="$(sed -n '1,30p' jenkins/scripts/slurm_run.sh)"
printf '%s\n' "$header"
if ! printf '%s\n' "$header" | rg -q 'Copyright.*(2026.*NVIDIA|NVIDIA.*2026)'; then
echo "ERROR: jenkins/scripts/slurm_run.sh does not show an NVIDIA copyright header for 2026."
exit 1
fiRepository: NVIDIA/TensorRT-LLM
Length of output: 1309
Add the NVIDIA copyright header
jenkins/scripts/slurm_run.sh has no NVIDIA copyright header. Add the header with 2026.
🤖 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/slurm_run.sh` around lines 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.
Source: Coding guidelines
brnguyen2
approved these changes
Aug 7, 2026
brnguyen2
left a comment
brnguyen2
left a comment
Collaborator
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.
| mkdir -p "$readyDir" | ||
| touch "$readyDir/rank_${SLURM_PROCID}.ready" | ||
|
|
||
| # Bounded above the 2700s pip3 retry budget in slurm_install.sh: a merely slow rank |
Collaborator
There was a problem hiding this comment.
The justification doesn't hold: 2700s is the timeout of a single retry_command, and slurm_install_setup runs several in sequence (wget 1800, apt, optional ray 2700, opencv 2700, requirements-dev 2700, wheel 2700). Worst-case legal install time is well above 3600s, so a slow-but-alive node can trip the barrier and fail the stage — the outcome this change exists to prevent, just at 3600s instead of 300s.
Either derive the bound from the sum of the install steps (or the job walltime minus a margin), or keep 3600 and reword the comment to say it's a walltime-shaped backstop rather than "bounded above the retry budget".
| # MPI collective must be entered by every rank, and under --mpi=pmix (added exactly | ||
| # when nodeCount > 1) that collective has a 300s fence timeout -- so the skew aborts | ||
| # every rank instead of merely running late. Fence on the shared $jobWorkspace so all | ||
| # ranks enter pytest together. Must stay below the SLURM_* unset block above, which is |
Collaborator
There was a problem hiding this comment.
"no-op for ... disaggregated runs" is only true for DISAGG_SERVING_TYPE in {BENCHMARK, DISAGG_SERVER} — those are the only two values the wipe block above matches. A multi-node *Disagg* stage with the variable unset or set to a CTX_*/GEN_* role keeps SLURM_NTASKS and will actually execute the barrier. That looks harmless here (this script is launched by a single srun covering every task of the step, so SLURM_NTASKS agrees with the number of ranks reaching the barrier), but the comment as written would let a future reader assume the barrier can never run on a disagg path, which is the assumption that would break if a role-specific srun --overlap step ever sources this file.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
slurm_wait_all_ranks()with job- and step-specific markers in the shared$jobWorkspace.SLURM_NTASKSmarkers and times out after 3600 seconds.accuracy/test_llm_api_pytorch.py::TestDeepSeekR1::test_nvfp4_multi_gpus[latency_adp_lmtp].nvbugs/6561777; the entry format and test path are valid based on the change summary.QA Engineer Review
tests/integration/test_lists/waives.txt.accuracy/test_llm_api_pytorch.py::TestDeepSeekR1::test_nvfp4_multi_gpus[latency_adp_lmtp].