[None][fix] Suppress agent-path junit for monitor-detected SLURM infra retries - #17130
Conversation
…URM infra retries A single-node SLURM stage (agent path) that infra-fails on attempt 1 and passes on retry was still marked UNSTABLE: the failed attempt's junit was reported even though the retry succeeded. The suppression decision in cacheErrorAndUploadResult classified the raw pytest error, but the retryable-infra determination (slurm-job-still-running / walltime-timeout) is made by the monitor in runLLMTestlistWithAgent *after* cacheErrorAndUploadResult already called junit(). So a machine failure whose raw error didn't match a catalog pattern was reported, then retried anyway -> the passing retry couldn't remove attempt 1's results. Move the SLURM-state classification into a closure applied inside the task runner (via executeLLMTestOnSlurm), so cacheErrorAndUploadResult suppresses this attempt's junit for the same typed failure the retry loop acts on. The outer catch trusts that label (only classifying failures raised outside the task runner), keeping suppression and retry consistent -- a suppressed attempt is always the one that retries, so there is no risk of hiding a non-retried failure. The sbatch path already behaves this way (it throws the typed InfraFailure inside the function that owns junit). querySlurmJobState self-manages its frontend SSH and returns null on any failure, so running it inside the on-node container degrades gracefully to the prior behavior rather than breaking classification. Signed-off-by: Derek Pitman <dpitman@nvidia.com>
dpitman-nvda
changed the title
|
/bot run --disable-fail-fast |
|
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)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughSLURM failures are classified before result caching and JUnit reporting. Timeout states become non-retryable ChangesSLURM failure classification
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant SLURM
participant TaskRunner
participant ResultCache
participant JUnitReporter
SLURM->>TaskRunner: return job state and execution failure
TaskRunner->>TaskRunner: classify failure
TaskRunner->>ResultCache: cache classified result
ResultCache->>JUnitReporter: report result
🚥 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)
1278-1290: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unreachable
FlowInterruptedExceptioncheck.
FlowInterruptedExceptionextendsjava.lang.InterruptedException. The precedingcatch (InterruptedException e) { throw e }at Line 1278-1279 already intercepts anyFlowInterruptedExceptionbefore control can reachcatch (Exception e)at Line 1280. Thee.getClass().name.contains("FlowInterruptedException")check at Line 1286 can therefore never be true; the comment's stated goal ("Pipeline aborts ... must not be relabeled") is already achieved by the first catch clause alone.This is dead code with no functional effect today, but it misleads a future reader into thinking this line provides protection that it does not. Remove it, or add a short comment noting the
InterruptedExceptioncatch already covers this case.♻️ Proposed cleanup
} catch (Exception e) { // Label the failure against the SLURM job's terminal state before // cacheErrorAndUploadResult decides junit suppression, so a monitor- // detected infra failure (e.g. slurm-job-still-running) suppresses this - // attempt's results and a passing retry stays green. Pipeline aborts - // (FlowInterruptedException) must not be relabeled. - if (classifySlurmFailure == null || e.getClass().name.contains("FlowInterruptedException")) { + // attempt's results and a passing retry stays green. Pipeline aborts + // (FlowInterruptedException) are already excluded by the InterruptedException + // catch above, since FlowInterruptedException extends InterruptedException. + if (classifySlurmFailure == null) { throw e } throw classifySlurmFailure(e) }As per the verified Jenkins plugin API,
FlowInterruptedExceptionispublic final class FlowInterruptedException extends InterruptedException, so any genericExceptioncatch block placed after anInterruptedExceptioncatch block never sees it.🤖 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 1278 - 1290, Remove the unreachable FlowInterruptedException class-name check from the catch (Exception e) guard in the surrounding try/catch flow. Retain the preceding catch (InterruptedException e) rethrow, which already preserves pipeline abort behavior, and keep the classifySlurmFailure handling for other exceptions unchanged.
🤖 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 1233-1248: Track whether classifySlurmFailure has already
processed the current SLURM attempt, including the deferred path that returns
the original exception, and update the outer catch around executeLLMTestOnSlurm
to skip reclassification when that flag is set. Preserve propagation of the
already-classified or deferred exception while still classifying failures raised
outside the task runner exactly once.
---
Nitpick comments:
In `@jenkins/L0_Test.groovy`:
- Around line 1278-1290: Remove the unreachable FlowInterruptedException
class-name check from the catch (Exception e) guard in the surrounding try/catch
flow. Retain the preceding catch (InterruptedException e) rethrow, which already
preserves pipeline abort behavior, and keep the classifySlurmFailure handling
for other exceptions unchanged.
🪄 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: 6ea18a1e-801b-46f4-8002-0b503a002ac8
📒 Files selected for processing (1)
jenkins/L0_Test.groovy
|
PR_Github #63065 [ run ] triggered by Bot. Commit: |
|
PR_Github #63065 [ run ] completed with state
|
Address CodeRabbit: on the deferred path classifySlurmFailure returns the raw exception (not a TrtllmCiException), so the outer catch reclassified it a second time -- the common path for ordinary test failures. The duplicate call adds an avoidable querySlurmJobState SSH round trip and, because elapsed time and the job state advance while cacheErrorAndUploadResult uploads results and runs junit(), the second verdict could differ and flip the retry/suppression decision the first classification already made. Track classification with a flag so the outer catch only classifies failures raised outside the task runner. Signed-off-by: Derek Pitman <dpitman@nvidia.com>
|
/bot run --disable-fail-fast |
|
PR_Github #63131 [ run ] triggered by Bot. Commit: |
|
PR_Github #63131 [ run ] completed with state
|
|
/bot run |
|
PR_Github #63482 [ run ] triggered by Bot. Commit: |
|
PR_Github #63482 [ run ] completed with state
|
|
/bot run |
|
PR_Github #63537 [ run ] triggered by Bot. Commit: |
|
PR_Github #63537 [ run ] completed with state
|
|
/bot run |
|
PR_Github #63777 [ run ] triggered by Bot. Commit: |
|
PR_Github #63777 [ run ] completed with state
|
|
/bot run |
|
PR_Github #63793 [ run ] triggered by Bot. Commit: |
|
PR_Github #63793 [ run ] completed with state
|
…gated junit
A suppressed infra-retry attempt still uploads its result tar for forensics.
The top-level Collect Test Result stage re-ingests every results-*.tar.gz via
junit('**/results*.xml'), so a superseded attempt's results-timeout.xml (left
by a monitor-cut still-running job) flipped the build UNSTABLE even though the
stage passed on retry. Move the superseded attempt's result XMLs aside before
taring so neither junit path re-counts them.
Signed-off-by: Derek Pitman <dpitman@nvidia.com>
|
/bot run --disable-fail-fast |
|
PR_Github #63825 [ run ] triggered by Bot. Commit: |
|
PR_Github #63825 [ run ] completed with state
|
|
/bot run |
|
PR_Github #64059 [ run ] triggered by Bot. Commit: |
|
PR_Github #64059 [ run ] completed with state |
3 participants
Dev Engineer Review
UserFailure.InfraFailure.querySlurmJobStateSSH calls.nullwhenquerySlurmJobStatecannot classify a failure.executeLLMTestOnSlurmwith the optionalclassifySlurmFailureclosure while preserving compatibility through the default value.QA Engineer Review
No test changes.
Description
A single-node SLURM stage (agent path) that infra-fails on attempt 1 and passes on retry was still marked UNSTABLE: the failed attempt's junit was reported even though the retry succeeded. The suppression decision in cacheErrorAndUploadResult classified the raw pytest error, but the retryable-infra determination (slurm-job-still-running / walltime-timeout) is made by the monitor in runLLMTestlistWithAgent after cacheErrorAndUploadResult already called junit(). So a machine failure whose raw error didn't match a catalog pattern was reported, then retried anyway -> the passing retry couldn't remove attempt 1's results.
Move the SLURM-state classification into a closure applied inside the task runner (via executeLLMTestOnSlurm), so cacheErrorAndUploadResult suppresses this attempt's junit for the same typed failure the retry loop acts on. The outer catch trusts that label (only classifying failures raised outside the task runner), keeping suppression and retry consistent -- a suppressed attempt is always the one that retries, so there is no risk of hiding a non-retried failure. The sbatch path already behaves this way (it throws the typed InfraFailure inside the function that owns junit).
querySlurmJobState self-manages its frontend SSH and returns null on any failure, so running it inside the on-node container degrades gracefully to the prior behavior rather than breaking classification.
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.