[TRTLLMINF-40][fix] Throw typed InfraFailure when SLURM submission yields no job ID - #17255
Conversation
…elds no job ID When sbatch submission fails (or the job ID is never captured), the stage currently dies later with a generic 'script returned exit code N' that FailureClassifier cannot act on, so the stage-level infra retry never fires even though nothing ever ran on a node. A week-long sweep of L0_MergeRequest_PR failures (2026-07-28..08-04) found 179 stage failures in this family. - Run Pytest (sbatch path): validate the captured job ID and throw InfraFailure(TRANSIENT, SLURM, <typed:slurm-submit-no-jobid>) when it is missing or non-numeric, so runLLMTestlistOnSlurm retries the stage. The slurm_job_id.txt read tolerates a missing file so the typed throw is reached instead of a bare cat failure. - cleanUpNodeResources: skip the scancel/sacct/scontrol dump when no numeric job ID exists (mirrors the existing guard in cleanUpSlurmResources). Running it with a null ID emits 'scontrol_print_job error: Invalid job id specified', which then pollutes failure analysis as a bogus infra signature - it was the single most common unmatched 'error' in the sweep. Signed-off-by: Brian Nguyen <brnguyen@nvidia.com>
|
/bot run |
WalkthroughSLURM job IDs now use shared digit-only validation across cleanup, state queries, submission, placement recording, and metadata recovery. Invalid IDs skip diagnostics and placement queries. Missing or invalid submission IDs raise typed transient ChangesSLURM job ID handling
Estimated code review effort: 3 (Moderate) | ~20 minutes 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}
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/L0_Test.groovy`:
- Around line 739-755: Replace every SLURM job-ID guard using
isNumber()—including the guards near the cleanup dump and the typed InfraFailure
path—with a digit-only check equivalent to jobId.toString() ==~ /\d+/. Apply
this consistently at all four locations, and update associated messages to
report missing or invalid non-integer IDs while preserving the existing command
and exception 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: 2884a5f3-7014-41ae-a9d6-7ad9fd2a2ded
📒 Files selected for processing (1)
jenkins/L0_Test.groovy
|
PR_Github #63767 [ run ] triggered by Bot. Commit: |
Groovy's String.isNumber() is an alias for isBigDecimal(), so a decimal like '1.5' passes the guard and still produces invalid scancel/sacct/ scontrol invocations. Use a digit-only match (==~ /\d+/) at all five job-ID guard sites and mention the non-numeric case in the typed InfraFailure message. Addresses CodeRabbit review on PR NVIDIA#17255. Signed-off-by: Brian Nguyen <brnguyen@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
jenkins/L0_Test.groovy (1)
1979-1987: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPreserve the typed failure for submission errors.
Because
set -eEeuo pipefailmakes the submission script exit whensbatchfails or returns no job ID,Utils.execraises before the typedInfraFailurecheck. Capture the submission status without aborting, record an empty ID, and continue to that validation block.🤖 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 1979 - 1987, Update the submission flow surrounding slurmJobId and its typed validation so sbatch failures or missing job IDs do not cause Utils.exec to throw before the InfraFailure check. Capture the submission command’s failure status without aborting, ensure slurmJobId is empty when submission produces no ID, and continue into the existing typed failure validation.
🤖 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 1990-1998: Introduce and reuse a shared digit-only SLURM job ID
validator in the finalizer diagnostics path, immediately before the sacct and
scontrol calls in captureSlurmJobNodeList. Ensure captureSlurmWorkspaceMetadata
does not retain nonempty but invalid IDs in placementContext, and preserve the
existing rejection of unusable IDs in the submission flow.
- Around line 1990-1998: Validate slurmJobId before passing it to
recordSlurmPlacementContext or storing it in placementContext, so rejected
values cannot reach the finally block. Ensure captureSlurmJobNodeList also
accepts only a non-empty numeric job ID before constructing the sacct shell
command, preserving the existing no-job-ID failure behavior and preventing
untrusted payloads from reaching shell sinks.
- Around line 1007-1009: Replace the echo statement in the slurmJobID validation
guard with a throw statement that raises an InfraFailure with InfraFailure.SLURM
to fail fast before reaching the SlurmConfig.checkJobStatus call at Line 1028
and the downstream sacct and scontrol command construction. The condition
checking !slurmJobID or the regex mismatch on slurmJobID.toString() should
remain unchanged, but instead of logging the failure, throw the typed exception
to prevent the wait loop from executing with an invalid job ID.
---
Outside diff comments:
In `@jenkins/L0_Test.groovy`:
- Around line 1979-1987: Update the submission flow surrounding slurmJobId and
its typed validation so sbatch failures or missing job IDs do not cause
Utils.exec to throw before the InfraFailure check. Capture the submission
command’s failure status without aborting, ensure slurmJobId is empty when
submission produces no ID, and continue into the existing typed failure
validation.
🪄 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: 63759a66-b14c-4060-a806-ee068e304aa8
📒 Files selected for processing (1)
jenkins/L0_Test.groovy
|
PR_Github #63767 [ run ] completed with state
|
dpitman-nvda
left a comment
dpitman-nvda
left a comment
There was a problem hiding this comment.
LGTM but please take a look at and resolve the CodeRabbit comments before approval.
|
/bot run |
|
PR_Github #63797 [ run ] triggered by Bot. Commit: |
|
PR_Github #63797 [ run ] completed with state
|
|
/bot run |
|
PR_Github #63820 [ run ] triggered by Bot. Commit: |
…igit-only job-ID validator - runLLMTestlistWithAgent: throw the typed InfraFailure(TRANSIENT, SLURM, <typed:slurm-submit-no-jobid>) instead of only echoing, so the stage retries immediately rather than polling a bogus job ID in the node-wait loop (up to 15h). - Extract isValidSlurmJobId() and use it at all guard sites. - captureSlurmWorkspaceMetadata: drop non-numeric slurm_job_id.txt values instead of recording them in placementContext. - captureSlurmJobNodeList: revalidate the job ID before the sacct/scontrol node-list fallback, so rejected IDs cannot re-enter finalizer diagnostics. Signed-off-by: Brian Nguyen <brnguyen@nvidia.com>
|
/bot run |
|
PR_Github #63822 [ run ] triggered by Bot. Commit: |
|
PR_Github #63820 [ run ] completed with state |
|
PR_Github #63822 [ run ] completed with state
|
|
/bot run |
|
PR_Github #63831 [ run ] triggered by Bot. Commit: |
|
PR_Github #63831 [ run ] completed with state
|
|
/bot skip --comment "This only affects potential infra failure paths; failure seen in latest job is test-only" |
|
PR_Github #63864 [ skip ] triggered by Bot. Commit: |
|
PR_Github #63864 [ skip ] completed with state |
3 participants
Description
When sbatch submission fails (or the job ID is never captured), the stage dies later with a generic
script returned exit code NthatFailureClassifiercannot act on, so the stage-level infra retry (TRTLLMINF-43/-54 machinery) never fires — even though the job never entered the SLURM queue and nothing ran on any node. A week-long sweep of allL0_MergeRequest_PRfailures (2026-07-28..08-04; 978 failed builds, 1124 infra-classified stage failures) found 179 stage failures in this family, and its cleanup side-effect (scontrol show job null→Invalid job id specified) was the single most common bogus "infra signature" in failure analysis.Changes:
runLLMTestlistWithSbatch/ Run Pytest: validate the captured job ID and throwInfraFailure(TRANSIENT, SLURM, <typed:slurm-submit-no-jobid>)when it is missing or non-numeric, so the existing retry loop in the SLURM stage wrapper retries the stage. Theslurm_job_id.txtread now tolerates a missing file (|| true) so the typed throw is reached instead of a barecatfailure.runLLMTestlistWithAgent/ Request Node Via Slurm: same typed throw when the agent-mode submission yields no numeric job ID, instead of echoing and entering the node-wait loop (up to 15h) with a bogus ID.isValidSlurmJobId()digit-only validator (review follow-up): used at every guard site;String.isNumber()is a BigDecimal-style parse that accepts values like1.5.captureSlurmWorkspaceMetadatadrops non-numericslurm_job_id.txtvalues instead of recording them, andcaptureSlurmJobNodeListrevalidates before itssacct/scontrolnode-list fallback.cleanUpNodeResources: skip thescancel/sacct/scontroldump when no numeric job ID exists, mirroring the existing guard incleanUpSlurmResources. This stopsscancel null/scontrol show job nullnoise from polluting per-stage logs and downstream failure analysis.Test Coverage
Jenkins pipeline code — validated by Groovy parse-check and this PR's own L0 run. The typed-throw form matches the existing
<typed:slurm-job-still-running>producer in the same file.PR Checklist
Dev Engineer Review
jenkins/L0_Test.groovy.InfraFailureexceptions when no usable job ID is captured.slurm_job_id.txtfiles are tolerated so the typed failure path executes.Invalid job id specifiederrors.QA Engineer Review
No test changes.