[None][fix] Don't re-run the SLURM monitor on a terminal job failure#15669
[None][fix] Don't re-run the SLURM monitor on a terminal job failure#15669dpitman-nvda wants to merge 9 commits into
Conversation
📝 WalkthroughWalkthroughThe SLURM tracking step now always records the final status and exit code to ChangesSLURM result handling
Sequence Diagram(s)sequenceDiagram
participant Controller
participant scriptTrack
participant ResultFile as "slurm_job_result.txt"
participant sacct
Controller->>scriptTrack: submit track step
scriptTrack->>ResultFile: write "<STATUS> <EXIT_CODE>"
scriptTrack-->>Controller: exit 0
Controller->>ResultFile: read and parse result
alt recorded result needs authoritative state
Controller->>sacct: query terminal SLURM state
sacct-->>Controller: return status and exit code
end
Controller->>Controller: classify failure from recorded verdict
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
jenkins/L0_Test.groovy (1)
1674-1676: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueVerdict parsing is ambiguous for multi-token SLURM states.
STATUSat loop break is the rawsacctState, which for cancellations renders asCANCELLED by <uid>. Written as"<STATUS> <EXIT_CODE>"(Line 1634) this becomes e.g.CANCELLED by 1000 0, soresultFields[1](Line 1676) resolves toby, not the exit code. The failure path is still taken (correct), but the loggedexit=...is wrong.querySlurmJobStatealready normalizes to the leading token (Line 624); the track script should do the same so the file stays a clean two-field record.♻️ Normalize the recorded state to its leading token
- printf '%s %s\n' "\$STATUS" "\$EXIT_CODE" > "${jobWorkspace}/slurm_job_result.txt" + # Keep only the leading state token (sacct renders cancellations as + # "CANCELLED by <uid>") so the verdict stays a clean two-field record. + STATE_TOKEN=\$(printf '%s' "\$STATUS" | awk '{print \$1}') + printf '%s %s\n' "\$STATE_TOKEN" "\$EXIT_CODE" > "${jobWorkspace}/slurm_job_result.txt"🤖 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/L0_Test.groovy` around lines 1674 - 1676, The verdict parsing in the SLURM tracking script is ambiguous because raw sacct states can contain spaces, which breaks the two-field record written by the track script and later parsed in L0_Test.groovy. Update the state recording logic in the track script to normalize STATUS to its leading token before writing the file, matching the behavior of querySlurmJobState, and keep the result format as a clean "<STATE> <EXIT_CODE>" pair so jobState and jobExit are parsed correctly.
🤖 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/L0_Test.groovy`:
- Around line 1670-1696: The verdict-file handling in the SLURM result check is
too strict when `readSlurmWorkspaceFile` returns an empty string, causing a
successful `COMPLETED/0` job to be treated as failure. Update the logic around
`jobResult`, `jobState`, `jobExit`, and the `querySlurmJobState` fallback so a
missing/unreadable verdict can be revalidated as success instead of only being
used to detect `TIMEOUT`. Keep the typed timeout path in the `UserFailure`
branch, but only throw the generic failure in `L0_Test.groovy` after confirming
the job was not actually successful.
---
Nitpick comments:
In `@jenkins/L0_Test.groovy`:
- Around line 1674-1676: The verdict parsing in the SLURM tracking script is
ambiguous because raw sacct states can contain spaces, which breaks the
two-field record written by the track script and later parsed in L0_Test.groovy.
Update the state recording logic in the track script to normalize STATUS to its
leading token before writing the file, matching the behavior of
querySlurmJobState, and keep the result format as a clean "<STATE> <EXIT_CODE>"
pair so jobState and jobExit are parsed correctly.
🪄 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: 0524e351-97d2-4a25-8a6d-75ffddb5a742
📒 Files selected for processing (1)
jenkins/L0_Test.groovy
| // Read the verdict the monitor recorded. "<STATE> <EXIT_CODE>"; | ||
| // success is COMPLETED with exit 0 (srun --kill-on-bad-exit=1 | ||
| // means any rank failure surfaces as a non-COMPLETED state). | ||
| def jobResult = readSlurmWorkspaceFile(pipeline, remote, "${jobWorkspace}/slurm_job_result.txt", stageName) | ||
| def resultFields = jobResult ? jobResult.tokenize(' ') : [] | ||
| def jobState = resultFields ? resultFields[0] : null | ||
| def jobExit = resultFields.size() > 1 ? resultFields[1] : null | ||
| if (jobState != "COMPLETED" || jobExit != "0") { | ||
| // Prefer the monitor's captured state; fall back to an | ||
| // authoritative sacct query if the verdict file was missing. | ||
| // SLURM aggregates multi-node state into one allocation row | ||
| // (TIMEOUT if any node hit the walltime). A TIMEOUT is a | ||
| // walltime kill -- raise a typed UserFailure so neither the | ||
| // SLURM retry loop nor the outer K8s pod retry re-runs a job | ||
| // that would just time out again. | ||
| def slurmState = jobState ?: querySlurmJobState(pipeline, cluster, partition.clusterName, slurmJobId) | ||
| if (slurmState == "TIMEOUT") { | ||
| throw new UserFailure( | ||
| "SLURM job ${slurmJobId} for ${stageName} ended in state TIMEOUT " + | ||
| "(hit partition walltime ${partition?.time}min); treating as a test timeout, not retrying. " + | ||
| "Original failure: ${e.message}", | ||
| e) | ||
| "(state=${slurmState}, exit=${jobExit ?: 'unknown'})", | ||
| null) | ||
| } | ||
| echo "[INFRA-RETRY] ${stageName}: SLURM job ${slurmJobId} terminal state=${slurmState ?: 'unknown'}; " + | ||
| "deferring to failure classifier." | ||
| throw e | ||
| echo "[INFRA-RETRY] ${stageName}: SLURM job ${slurmJobId} terminal state=${slurmState ?: 'unknown'}, " + | ||
| "exit=${jobExit ?: 'unknown'}; deferring to failure classifier." | ||
| throw new Exception("Pytest failed in SLURM job ${slurmJobId} for ${stageName}") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Missing/unreadable verdict file misclassifies a successful job as a failure.
readSlurmWorkspaceFile (called at Line 1673 with the default numRetries=1) swallows SSH/transport errors and returns "". When that happens, jobState/jobExit are null, so the if (jobState != "COMPLETED" || jobExit != "0") block at Line 1677 is entered. Inside it, the querySlurmJobState fallback at Line 1685 is only consulted to detect TIMEOUT — it is not used to re-derive success. So a job that actually finished COMPLETED/exit 0 but whose verdict could not be read (a transient controller/SSH blip — exactly the failure mode this file tolerates elsewhere) is thrown as a generic Exception at Line 1695 and fails the stage.
Since the track script always writes the verdict before exit 0, the read should be hardened and the fallback should be able to confirm success, not just rule out TIMEOUT.
🛠️ Suggested hardening
- def jobResult = readSlurmWorkspaceFile(pipeline, remote, "${jobWorkspace}/slurm_job_result.txt", stageName)
+ def jobResult = readSlurmWorkspaceFile(pipeline, remote, "${jobWorkspace}/slurm_job_result.txt", stageName, 3)
def resultFields = jobResult ? jobResult.tokenize(' ') : []
def jobState = resultFields ? resultFields[0] : null
def jobExit = resultFields.size() > 1 ? resultFields[1] : null
if (jobState != "COMPLETED" || jobExit != "0") {
...
def slurmState = jobState ?: querySlurmJobState(pipeline, cluster, partition.clusterName, slurmJobId)
+ // Verdict unreadable but controller now reports COMPLETED: the job
+ // succeeded; don't fail the stage on a transient read blip.
+ if (jobState == null && slurmState == "COMPLETED") {
+ echo "[INFRA-RETRY] ${stageName}: verdict file unreadable but sacct reports COMPLETED for ${slurmJobId}; treating as success."
+ return
+ }
if (slurmState == "TIMEOUT") {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Read the verdict the monitor recorded. "<STATE> <EXIT_CODE>"; | |
| // success is COMPLETED with exit 0 (srun --kill-on-bad-exit=1 | |
| // means any rank failure surfaces as a non-COMPLETED state). | |
| def jobResult = readSlurmWorkspaceFile(pipeline, remote, "${jobWorkspace}/slurm_job_result.txt", stageName) | |
| def resultFields = jobResult ? jobResult.tokenize(' ') : [] | |
| def jobState = resultFields ? resultFields[0] : null | |
| def jobExit = resultFields.size() > 1 ? resultFields[1] : null | |
| if (jobState != "COMPLETED" || jobExit != "0") { | |
| // Prefer the monitor's captured state; fall back to an | |
| // authoritative sacct query if the verdict file was missing. | |
| // SLURM aggregates multi-node state into one allocation row | |
| // (TIMEOUT if any node hit the walltime). A TIMEOUT is a | |
| // walltime kill -- raise a typed UserFailure so neither the | |
| // SLURM retry loop nor the outer K8s pod retry re-runs a job | |
| // that would just time out again. | |
| def slurmState = jobState ?: querySlurmJobState(pipeline, cluster, partition.clusterName, slurmJobId) | |
| if (slurmState == "TIMEOUT") { | |
| throw new UserFailure( | |
| "SLURM job ${slurmJobId} for ${stageName} ended in state TIMEOUT " + | |
| "(hit partition walltime ${partition?.time}min); treating as a test timeout, not retrying. " + | |
| "Original failure: ${e.message}", | |
| e) | |
| "(state=${slurmState}, exit=${jobExit ?: 'unknown'})", | |
| null) | |
| } | |
| echo "[INFRA-RETRY] ${stageName}: SLURM job ${slurmJobId} terminal state=${slurmState ?: 'unknown'}; " + | |
| "deferring to failure classifier." | |
| throw e | |
| echo "[INFRA-RETRY] ${stageName}: SLURM job ${slurmJobId} terminal state=${slurmState ?: 'unknown'}, " + | |
| "exit=${jobExit ?: 'unknown'}; deferring to failure classifier." | |
| throw new Exception("Pytest failed in SLURM job ${slurmJobId} for ${stageName}") | |
| } | |
| // Read the verdict the monitor recorded. "<STATE> <EXIT_CODE>"; | |
| // success is COMPLETED with exit 0 (srun --kill-on-bad-exit=1 | |
| // means any rank failure surfaces as a non-COMPLETED state). | |
| def jobResult = readSlurmWorkspaceFile(pipeline, remote, "${jobWorkspace}/slurm_job_result.txt", stageName, 3) | |
| def resultFields = jobResult ? jobResult.tokenize(' ') : [] | |
| def jobState = resultFields ? resultFields[0] : null | |
| def jobExit = resultFields.size() > 1 ? resultFields[1] : null | |
| if (jobState != "COMPLETED" || jobExit != "0") { | |
| // Prefer the monitor's captured state; fall back to an | |
| // authoritative sacct query if the verdict file was missing. | |
| // SLURM aggregates multi-node state into one allocation row | |
| // (TIMEOUT if any node hit the walltime). A TIMEOUT is a | |
| // walltime kill -- raise a typed UserFailure so neither the | |
| // SLURM retry loop nor the outer K8s pod retry re-runs a job | |
| // that would just time out again. | |
| def slurmState = jobState ?: querySlurmJobState(pipeline, cluster, partition.clusterName, slurmJobId) | |
| // Verdict unreadable but controller now reports COMPLETED: the job | |
| // succeeded; don't fail the stage on a transient read blip. | |
| if (jobState == null && slurmState == "COMPLETED") { | |
| echo "[INFRA-RETRY] ${stageName}: verdict file unreadable but sacct reports COMPLETED for ${slurmJobId}; treating as success." | |
| return | |
| } | |
| if (slurmState == "TIMEOUT") { | |
| throw new UserFailure( | |
| "SLURM job ${slurmJobId} for ${stageName} ended in state TIMEOUT " + | |
| "(hit partition walltime ${partition?.time}min); treating as a test timeout, not retrying. " + | |
| "(state=${slurmState}, exit=${jobExit ?: 'unknown'})", | |
| null) | |
| } | |
| echo "[INFRA-RETRY] ${stageName}: SLURM job ${slurmJobId} terminal state=${slurmState ?: 'unknown'}, " + | |
| "exit=${jobExit ?: 'unknown'}; deferring to failure classifier." | |
| throw new Exception("Pytest failed in SLURM job ${slurmJobId} for ${stageName}") | |
| } |
🤖 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/L0_Test.groovy` around lines 1670 - 1696, The verdict-file handling
in the SLURM result check is too strict when `readSlurmWorkspaceFile` returns an
empty string, causing a successful `COMPLETED/0` job to be treated as failure.
Update the logic around `jobResult`, `jobState`, `jobExit`, and the
`querySlurmJobState` fallback so a missing/unreadable verdict can be revalidated
as success instead of only being used to detect `TIMEOUT`. Keep the typed
timeout path in the `UserFailure` branch, but only throw the generic failure in
`L0_Test.groovy` after confirming the job was not actually successful.
|
/bot run |
|
PR_Github #56117 [ run ] triggered by Bot. Commit: |
|
PR_Github #56117 [ run ] completed with state
|
|
/bot run |
|
PR_Github #56378 [ run ] triggered by Bot. Commit: |
|
PR_Github #56378 [ run ] completed with state
|
|
/bot run |
|
PR_Github #56391 [ run ] triggered by Bot. Commit: |
|
PR_Github #56391 [ run ] completed with state
|
|
/bot run |
|
PR_Github #56401 [ run ] triggered by Bot. Commit: |
|
PR_Github #56401 [ run ] completed with state
|
|
/bot run |
|
PR_Github #56437 [ run ] triggered by Bot. Commit: |
|
PR_Github #56437 [ run ] completed with state
|
|
/bot run |
|
PR_Github #56454 [ run ] triggered by Bot. Commit: |
|
PR_Github #56454 [ run ] completed with state
|
|
/bot run |
|
PR_Github #56456 [ run ] triggered by Bot. Commit: |
|
PR_Github #56456 [ run ] completed with state
|
|
/bot run |
|
PR_Github #57020 [ run ] triggered by Bot. Commit: |
|
PR_Github #60773 [ run ] completed with state
|
|
/bot run |
|
PR_Github #60988 [ run ] triggered by Bot. Commit: |
|
PR_Github #60988 [ run ] completed with state
|
|
/bot run |
|
PR_Github #61064 [ run ] triggered by Bot. Commit: |
|
PR_Github #61064 [ run ] completed with state |
…retry-on-terminal-state Resolve jenkins/L0_Test.groovy: fold the device-fault log scrape (NVIDIA#16557) into this PR's verdict-file-based SLURM result handling. The non-terminal-state (job-still-running) infra classification runs first, then on a terminal FAILED verdict the log is scraped for a device/interconnect signature and surfaced to the classifier; a miss falls through to the "Pytest failed" rethrow. Dropped the old catch-block's `e`/`e.message` reference, which no longer exists in the verdict-file structure. Signed-off-by: Derek Pitman <dpitman@nvidia.com>
|
/bot run --disable-fail-fast |
|
PR_Github #61128 [ run ] triggered by Bot. Commit: |
|
PR_Github #61128 [ run ] completed with state
|
|
/bot run |
|
PR_Github #61301 [ run ] triggered by Bot. Commit: |
|
PR_Github #61301 [ run ] completed with state
|
|
/bot run |
|
PR_Github #61319 [ run ] triggered by Bot. Commit: |
|
PR_Github #61319 [ run ] completed with state
|
|
/bot run |
|
PR_Github #61336 [ run ] triggered by Bot. Commit: |
|
PR_Github #61336 [ run ] completed with state
|
|
/bot run |
|
PR_Github #61346 [ run ] triggered by Bot. Commit: |
|
PR_Github #61346 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #61398 [ run ] triggered by Bot. Commit: |
|
PR_Github #61398 [ run ] completed with state
|
|
/bot run |
|
PR_Github #61578 [ run ] triggered by Bot. Commit: |
|
PR_Github #61578 [ run ] completed with state
|
3 participants
Summary by CodeRabbit
Description
Make the track script record its verdict to slurm_job_result.txt and always exit 0 once it has observed a terminal state. numRetries now only re-runs the monitor on an SSH/transport failure (no verdict written) -- never on a job that already reached a terminal state, which a re-run cannot change. The controller reads the verdict, treats COMPLETED+exit-0 as success, raises the existing typed UserFailure on TIMEOUT, and otherwise defers to the classifier. Behavior is unchanged for every outcome; only the redundant re-monitor cycles are removed.
Test Coverage
N/A, this is a CI change
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.