[TRTLLM-9644][infra] Update isolation test - #12491
Conversation
WalkthroughThe CI pipeline now uses ChangesCI test runner integration
Estimated code review effort: 4 (Complex) | ~50 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Groovy as L0_Test.groovy
participant Slurm as slurm_run.sh
participant Runner as run_tests.py
participant Pytest as pytest
participant Results as JUnit and rerun artifacts
Groovy->>Slurm: Export test, shard, rerun, and duration inputs
Slurm->>Runner: Invoke with runner arguments
Runner->>Pytest: Collect and shard tests
Runner->>Pytest: Execute regular and isolated tests
Pytest-->>Runner: Return XML and failure output
Runner->>Pytest: Rerun eligible failures
Runner->>Results: Merge XML and generate rerun report
Runner-->>Slurm: Return execution status
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 4
🧹 Nitpick comments (3)
jenkins/scripts/run_tests.py (3)
297-299: Consider handling missing XML more gracefully.When
result_xmldoesn't exist, returning(True, [])signals "rerun failed" but with no XML files. This may cause confusion downstream sinceTruetypically indicates failure. Consider returning(False, [])to indicate "nothing to rerun, no failure" or add a comment clarifying the semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jenkins/scripts/run_tests.py` around lines 297 - 299, The early-exit branch that checks os.path.exists(result_xml) currently returns (True, []), which is misleading; update the branch in the os.path.exists(result_xml) check to return (False, []) to indicate "no rerun needed / no failure" (or alternatively add an explicit comment documenting that True means failure) so downstream callers aren't confused by a True value when no XML exists—look for the result_xml existence check in run_tests.py and change the return semantics accordingly.
478-483: Narrow the broad exception handling.Catching bare
Exceptioncan mask unexpected errors. Consider catching more specific exceptions (e.g.,IOError,OSError) or at minimum re-raise if it's an unexpected error type.Proposed refinement
if os.path.exists(xml_file): try: content = Path(xml_file).read_text() content = content.replace('testsuite name="pytest"', f'testsuite name="{stage_name}"') Path(xml_file).write_text(content) - except Exception as e: + except (IOError, OSError) as e: print(f"Warning: Failed to fix testsuite name in {xml_file}: {e}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jenkins/scripts/run_tests.py` around lines 478 - 483, The current broad except Exception in the XML testsuite name fix should be narrowed: wrap the Path(xml_file).read_text()/write_text() and replace call in a try block and catch only file/IO and decoding errors (e.g., OSError, IOError, UnicodeError) to print the warning; if any other exception occurs re-raise it so unexpected bugs aren’t swallowed. Update the except clause referencing the same xml_file handling block in run_tests.py accordingly (use specific exception tuple for logging, and allow other exceptions to propagate).
102-109: Acknowledged:shell=Trueusage in subprocess calls.The static analysis flags S602 for
shell=True. In this CI context wherepytest_cmdandcollect_cmdare constructed internally (not from untrusted user input), this is acceptable. However, ensure these commands are never constructed from external/untrusted sources.Also applies to: 193-195
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jenkins/scripts/run_tests.py` around lines 102 - 109, subprocess.run is currently invoked with shell=True for collect_cmd (and also for pytest_cmd around the other call), which triggers S602; to fix this either (preferred) construct collect_cmd and pytest_cmd as argument lists and call subprocess.run(..., shell=False, capture_output=True, text=True, cwd=working_dir, env=env_vars) or (if a shell is absolutely required) add an explicit, nearby sanity check that collect_cmd and pytest_cmd are only ever built from internal constants (no user/external input) and add a clear comment documenting why shell=True is safe in this CI context; update both invocations that use shell=True to follow one of these approaches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@jenkins/scripts/run_tests.py`:
- Line 420: Rename the ambiguous variable `l` to `line` in the list
comprehension that builds `isolate_tests` (the expression currently
"isolate_tests = [l.strip() for l in Path(isolate_list).read_text().splitlines()
if l.strip()]"); update all uses inside that comprehension to `line` (i.e.,
"line.strip()" and "if line.strip()") to satisfy the linter and improve
readability.
- Around line 321-324: Rename the ambiguous variable `l` in the list
comprehension that reads rerun_file to a descriptive name (e.g., `line`) to
resolve ruff E741; specifically update the expression in the block using
`Path(rerun_file).read_text().splitlines()` so it becomes `lines = [line for
line in ... if line.strip()]`, leaving the surrounding variables (`rerun_file`,
`rerun_tag`, `times`, `valid_count`) and logic unchanged.
- Around line 636-639: Comprehensions that compute regular_count and
isolate_count use the single-letter variable name `l`, causing E741 ambiguous
variable-name failures; update both comprehensions to use a clear name like
`line` (e.g., replace `l` with `line` in the list comprehensions that read from
Path(regular_list).read_text().splitlines() and
Path(isolate_list).read_text().splitlines()) so the variables regular_count and
isolate_count are computed without the ambiguous-named iterator.
In `@jenkins/scripts/slurm_run.sh`:
- Around line 109-130: The runTestsArgs array currently includes an unquoted
variable $perfModeFlag which, when empty, can produce word-splitting issues;
update the array construction (around runTestsArgs and perfModeFlag) to either
add perfModeFlag conditionally (only push/perfModeFlag when perfMode is "true")
or ensure it's quoted/handled so the empty value doesn't expand into extra empty
array elements—modify the code that sets perfModeFlag and the runTestsArgs array
population (references: perfModeFlag, runTestsArgs, runTestsScript) to perform a
conditional append or use a safe quoted expansion.
---
Nitpick comments:
In `@jenkins/scripts/run_tests.py`:
- Around line 297-299: The early-exit branch that checks
os.path.exists(result_xml) currently returns (True, []), which is misleading;
update the branch in the os.path.exists(result_xml) check to return (False, [])
to indicate "no rerun needed / no failure" (or alternatively add an explicit
comment documenting that True means failure) so downstream callers aren't
confused by a True value when no XML exists—look for the result_xml existence
check in run_tests.py and change the return semantics accordingly.
- Around line 478-483: The current broad except Exception in the XML testsuite
name fix should be narrowed: wrap the Path(xml_file).read_text()/write_text()
and replace call in a try block and catch only file/IO and decoding errors
(e.g., OSError, IOError, UnicodeError) to print the warning; if any other
exception occurs re-raise it so unexpected bugs aren’t swallowed. Update the
except clause referencing the same xml_file handling block in run_tests.py
accordingly (use specific exception tuple for logging, and allow other
exceptions to propagate).
- Around line 102-109: subprocess.run is currently invoked with shell=True for
collect_cmd (and also for pytest_cmd around the other call), which triggers
S602; to fix this either (preferred) construct collect_cmd and pytest_cmd as
argument lists and call subprocess.run(..., shell=False, capture_output=True,
text=True, cwd=working_dir, env=env_vars) or (if a shell is absolutely required)
add an explicit, nearby sanity check that collect_cmd and pytest_cmd are only
ever built from internal constants (no user/external input) and add a clear
comment documenting why shell=True is safe in this CI context; update both
invocations that use shell=True to follow one of these approaches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a8df67de-4df8-45e6-a4e6-e206deed442f
📒 Files selected for processing (3)
jenkins/L0_Test.groovyjenkins/scripts/run_tests.pyjenkins/scripts/slurm_run.sh
|
/bot run --stage-list "GB200-8_GPUs-2_Nodes-PyTorch-Post-Merge-1, A100X-PyTorch-1" --disable-fail-fast |
|
PR_Github #40108 [ run ] triggered by Bot. Commit: |
|
/bot run --stage-list "GB200-8_GPUs-2_Nodes-PyTorch-Post-Merge-1, A100X-PyTorch-1" --disable-fail-fast |
|
PR_Github #40113 [ run ] triggered by Bot. Commit: |
|
PR_Github #40108 [ run ] completed with state |
|
PR_Github #40113 [ run ] completed with state |
|
/bot run |
|
PR_Github #40889 [ run ] triggered by Bot. Commit: |
|
PR_Github #40889 [ run ] completed with state
|
|
/bot run |
|
PR_Github #41160 [ run ] triggered by Bot. Commit: |
|
PR_Github #41160 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
1 similar comment
|
/bot run --disable-fail-fast |
|
PR_Github #41354 [ run ] triggered by Bot. Commit: |
|
PR_Github #41354 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #42071 [ run ] triggered by Bot. Commit: |
|
PR_Github #42071 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #42221 [ run ] triggered by Bot. Commit: |
|
PR_Github #62373 [ run ] completed with state
|
|
/bot run --stage-list "GB200-8_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU1-GEN1-NODE1-GPU2-Post-Merge-1" |
|
PR_Github #62463 [ run ] triggered by Bot. Commit: |
|
PR_Github #62463 [ run ] completed with state
|
|
/bot run --stage-list "GB200-8_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU1-GEN1-NODE1-GPU2-Post-Merge-1" |
|
PR_Github #62524 [ run ] triggered by Bot. Commit: |
|
PR_Github #62524 [ run ] completed with state
|
|
/bot run --stage-list "GB200-8_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU1-GEN1-NODE1-GPU2-Post-Merge-1" |
|
PR_Github #62675 [ run ] triggered by Bot. Commit: |
|
PR_Github #62675 [ run ] completed with state
|
|
/bot run --stage-list "GB200-8_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU1-GEN1-NODE1-GPU2-Post-Merge-1" |
|
PR_Github #62731 [ run ] triggered by Bot. Commit: |
|
PR_Github #62731 [ run ] completed with state
|
|
/bot run --stage-list "GB200-8_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU1-GEN1-NODE1-GPU2-Post-Merge-1" |
|
PR_Github #62759 [ run ] triggered by Bot. Commit: |
|
PR_Github #62759 [ run ] completed with state
|
|
/bot run --stage-list "GB200-8_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU1-GEN1-NODE1-GPU2-Post-Merge-1" |
|
PR_Github #62931 [ run ] triggered by Bot. Commit: |
|
PR_Github #62931 [ run ] completed with state
|
|
Reviewed the full change (groovy delta, 1. A collection error no longer fails the shard; it silently runs a partial one.
if result.returncode != 0 and not output:
print(f"Error: pytest --collect-only failed with exit code {result.returncode}")
sys.exit(1)On } catch (Exception e) {
error "Test collection failed for shard ${splitId}/${splits}. Cannot proceed without valid test list."
}
2. On the failure path the rerun report is no longer produced or uploaded.
On Also worth noting the timeout case specifically: 3.
4. The whole pytest command now round-trips through single-quoted shell.
Minor: the sbatch path escapes Scope: the fifth file adds 5 Not blocking on 3, 4, or the scope note. 1 and 2 are behaviour that exists on |
…RM paths Introduce jenkins/scripts/run_tests.py as a unified test runner that handles render, regular tests, isolation tests, rerun, and XML merge in a single invocation for both K8s (Blossom) and SLURM (sbatch/agent) CI paths. Key changes: - jenkins/scripts/run_tests.py: new -- unified runner; Popen tail-capture for collection errors, fail-signatures rerun eligibility, XML merge - jenkins/scripts/slurm_run.sh: replace eval $pytestCommand with run_tests.py runTestsArgs array; MPI launcher wraps run_tests.py for multi-node - jenkins/L0_Test.groovy: K8s path calls run_tests.py; sbatch path passes test-list/splits/group/durations via env vars to slurm_run.sh; markExpr fix (double-quote syntax + 'and not disabled' for CPU stages); adopt main's !testFilter[(DETAILED_LOG)] guard for S3 upload args Signed-off-by: EmmaQiaoCh <qqiao@nvidia.com>
6b8f48c to
a23cad3
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
jenkins/scripts/run_tests.py (1)
314-318: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCatch
OSErrorin addition toET.ParseError.
merge_resultscatches(OSError, ET.ParseError)at Line 706. Here onlyET.ParseErroris caught. If the XML file cannot be read, this diagnostics helper raises and masks the original test failure.Proposed fix
try: root = ET.parse(xml_path).getroot() - except ET.ParseError as exc: + except (OSError, ET.ParseError) as exc: print(f" [Could not parse result XML: {exc}]") return🤖 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/run_tests.py` around lines 314 - 318, Update the XML parsing error handler in merge_results to catch OSError alongside ET.ParseError, matching the existing handling at the later merge_results call site. Keep the diagnostic message and return behavior unchanged so unreadable result files do not mask the original test failure.jenkins/scripts/slurm_run.sh (1)
98-102: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the conditional-append pattern for
--durations-pathtoo.Line 98 relies on unquoted
${var:+...}expansion inside the array. This works, but it is inconsistent with the explicitifblock used for--perf-modeat Lines 100-102, and shellcheck flags unquoted array elements. Prefer one pattern.Proposed fix
--max-rerun-tests 5 - ${testDurationsPath:+--durations-path "$testDurationsPath"} ) +if [ -n "${testDurationsPath:-}" ]; then + runTestsArgs+=(--durations-path "$testDurationsPath") +fi if [ "$perfMode" = "true" ]; then runTestsArgs+=(--perf-mode) fi🤖 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 98 - 102, Update the runTestsArgs construction in slurm_run.sh to append --durations-path through an explicit conditional block, matching the existing perfMode pattern. Remove the inline ${testDurationsPath:+...} array expansion and append the option and value only when testDurationsPath is set, preserving the current argument behavior.Source: Linters/SAST tools
🤖 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 4535-4552: Update run_tests.py’s pytest collection handling to
abort immediately whenever the collection subprocess result has a nonzero
returncode, before parsing or using stdout. Do not allow nonempty collection
output to bypass this failure path, so partial shards cannot continue or report
success.
- Around line 4535-4552: The stage currently lets the nonzero run_tests.py exit
abort before the rerun report upload. Update the shell flow around run_tests.py
and the subsequent artifact upload to capture and preserve its exit status,
complete the report upload, then propagate the failure; alternatively move
report generation and upload into run_tests.py before its final nonzero exit.
- Around line 1538-1542: Update the non-CPU branch of unittestMarkExpr in the
stage command setup to exclude tests marked disabled, while preserving the CPU
expression behavior and existing test command construction.
In `@jenkins/scripts/run_tests.py`:
- Around line 173-176: Update the pytest --collect-only result handling to exit
with failure for every non-zero result.returncode, regardless of whether output
is present. Preserve the existing error messages using result.returncode and
result.stderr, and ensure no collected-test execution proceeds after any
collection failure.
- Line 1: Update the NVIDIA copyright header in run_tests.py to use 2026,
reflecting the year of its latest meaningful modification; leave the rest of the
header unchanged.
- Around line 493-501: Enforce max_rerun_tests in check_and_rerun using the
existing valid_count total before the rerun loop, limiting reruns to at most the
configured cap across both rerun files. Ensure the selected rerun entries are
truncated consistently and skip or stop processing once the cap is reached;
preserve normal behavior when the total is within the limit.
---
Nitpick comments:
In `@jenkins/scripts/run_tests.py`:
- Around line 314-318: Update the XML parsing error handler in merge_results to
catch OSError alongside ET.ParseError, matching the existing handling at the
later merge_results call site. Keep the diagnostic message and return behavior
unchanged so unreadable result files do not mask the original test failure.
In `@jenkins/scripts/slurm_run.sh`:
- Around line 98-102: Update the runTestsArgs construction in slurm_run.sh to
append --durations-path through an explicit conditional block, matching the
existing perfMode pattern. Remove the inline ${testDurationsPath:+...} array
expansion and append the option and value only when testDurationsPath is set,
preserving the current argument 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: 4554102f-1cf8-41a4-b212-c68da07e470d
📒 Files selected for processing (3)
jenkins/L0_Test.groovyjenkins/scripts/run_tests.pyjenkins/scripts/slurm_run.sh
| def unittestMarkExpr = (stageName.startsWith("CPU-")) ? "cpu_only and not disabled" : "not cpu_only" | ||
| testCmdLine += ["--unittest-markexpr=\"${unittestMarkExpr}\""] | ||
| if (ENABLE_UPLOAD_TEST_RESULTS) { | ||
| testCmdLine += ["-o console_output_style=progress-even-when-capture-no"] | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Exclude disabled tests for non-CPU stages.
The non-CPU expression is not cpu_only. It does not exclude tests marked disabled. GPU stages can run tests that this change intends to disable.
Proposed fix
- def unittestMarkExpr = (stageName.startsWith("CPU-")) ? "cpu_only and not disabled" : "not cpu_only"
+ def unittestMarkExpr = (stageName.startsWith("CPU-")) ? "cpu_only and not disabled" : "not cpu_only and not disabled"📝 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.
| def unittestMarkExpr = (stageName.startsWith("CPU-")) ? "cpu_only and not disabled" : "not cpu_only" | |
| testCmdLine += ["--unittest-markexpr=\"${unittestMarkExpr}\""] | |
| if (ENABLE_UPLOAD_TEST_RESULTS) { | |
| testCmdLine += ["-o console_output_style=progress-even-when-capture-no"] | |
| } | |
| def unittestMarkExpr = (stageName.startsWith("CPU-")) ? "cpu_only and not disabled" : "not cpu_only and not disabled" | |
| testCmdLine += ["--unittest-markexpr=\"${unittestMarkExpr}\""] | |
| if (ENABLE_UPLOAD_TEST_RESULTS) { | |
| testCmdLine += ["-o console_output_style=progress-even-when-capture-no"] | |
| } |
🤖 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 1538 - 1542, Update the non-CPU branch
of unittestMarkExpr in the stage command setup to exclude tests marked disabled,
while preserving the CPU expression behavior and existing test command
construction.
| // Use unified run_tests.py for render + regular + isolated + rerun + merge | ||
| sh """ | ||
| rm -rf ${stageName}/ && \ | ||
| cd ${llmSrc}/tests/integration/defs && \ | ||
| python3 ${llmSrc}/jenkins/scripts/run_tests.py \ | ||
| --render \ | ||
| --test-db-list ${testDBList} \ | ||
| --splits ${splits} \ | ||
| --group ${splitId} \ | ||
| ${perfMode ? '--perf-mode' : ''} \ | ||
| --pytest-base-cmd '${pytestCommand.join(" ")}' \ | ||
| --stage-name ${stageName} \ | ||
| --output-dir ${WORKSPACE}/${stageName} \ | ||
| --working-dir ${llmSrc}/tests/integration/defs \ | ||
| --fail-signatures '${failSignaturesList}' \ | ||
| --max-rerun-tests 5 \ | ||
| ${clusterDurationsPath ? "--durations-path ${clusterDurationsPath}" : ''} | ||
| """ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail on every test-collection error.
run_tests.py continues when pytest --collect-only returns nonzero and stdout is nonempty. This invocation can then run a partial shard and report success. Make the runner fail before it parses collection output whenever result.returncode != 0.
🤖 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 4535 - 4552, Update run_tests.py’s
pytest collection handling to abort immediately whenever the collection
subprocess result has a nonzero returncode, before parsing or using stdout. Do
not allow nonempty collection output to bypass this failure path, so partial
shards cannot continue or report success.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Upload the rerun report before propagating runner failure.
When run_tests.py exits nonzero after failed reruns, sh aborts before the report upload at Line 4566. Preserve the runner status, upload generated artifacts, then fail the stage. Alternatively, generate and upload the rerun report inside run_tests.py before its final nonzero exit.
Also applies to: 4565-4571
🤖 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 4535 - 4552, The stage currently lets
the nonzero run_tests.py exit abort before the rerun report upload. Update the
shell flow around run_tests.py and the subsequent artifact upload to capture and
preserve its exit status, complete the report upload, then propagate the
failure; alternatively move report generation and upload into run_tests.py
before its final nonzero exit.
Signed-off-by: EmmaQiaoCh <qqiao@nvidia.com>
Signed-off-by: EmmaQiaoCh <qqiao@nvidia.com>
|
/bot run --stage-list "GB200-8_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU1-GEN1-NODE1-GPU2-Post-Merge-1" |
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/scripts/slurm_run.sh (1)
1-1: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the NVIDIA copyright header.
Line 1 starts a modified source file, but the file has no NVIDIA copyright header. Add the standard SPDX header after the shebang. Set the copyright year to the latest meaningful modification year.
As per coding guidelines:
**/*: Add the NVIDIA copyright header to all new files and update the copyright year on modified files;**/*: Source files must contain the NVIDIA copyright header with the year of the latest meaningful modification.Proposed header placement
#!/bin/bash +# SPDX-FileCopyrightText: Copyright (c) YYYY NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0🤖 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` at line 1, Add the standard NVIDIA SPDX copyright header immediately after the shebang in the script, using the latest meaningful modification year. Preserve the existing shebang and script behavior.Source: Coding guidelines
🤖 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 118-119: The spool-drain command currently discards failures;
capture its status in a spool_exit_code variable instead of using || true.
Update the final_exit_code selection so spool_exit_code is the final fallback
after test and performance results pass, preserving the existing precedence of
those checks.
- Around line 99-101: Add the standard NVIDIA copyright header at the beginning
of slurm_run.sh, using the latest meaningful modification year for the copyright
range or year. Preserve the existing script logic, including the
testDurationsPath argument handling.
- Line 97: Update run_isolated_tests and its check_and_rerun flow so the maximum
of five reruns is tracked across the entire isolated-test stage rather than
reset for each test; preserve per-test execution while sharing one stage-level
counter or limit. Also add the required NVIDIA copyright header at the top of
slurm_run.sh.
---
Outside diff comments:
In `@jenkins/scripts/slurm_run.sh`:
- Line 1: Add the standard NVIDIA SPDX copyright header immediately after the
shebang in the script, using the latest meaningful modification year. Preserve
the existing shebang and 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: 575f0c64-792b-4f63-b534-8d007cccaf49
📒 Files selected for processing (3)
jenkins/L0_Test.groovyjenkins/scripts/run_tests.pyjenkins/scripts/slurm_run.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- jenkins/L0_Test.groovy
- jenkins/scripts/run_tests.py
| --output-dir "$jobWorkspace" | ||
| --working-dir "$llmSrcNode/tests/integration/defs" | ||
| --fail-signatures "${failSignaturesList:-}" | ||
| --max-rerun-tests 5 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 12 'def run_(regular|isolated)_tests|max_rerun_tests|rerun' jenkins/scripts/run_tests.pyRepository: NVIDIA/TensorRT-LLM
Length of output: 23060
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- slurm invocation ---'
sed -n '1,130p' jenkins/scripts/slurm_run.sh
printf '%s\n' '--- rerun-list generation ---'
rg -n -C 20 'def generate_rerun_tests_list|rerun_1|rerun_2' jenkins/scripts/test_rerun.py
printf '%s\n' '--- copyright headers ---'
sed -n '1,25p' jenkins/scripts/slurm_run.sh
sed -n '1,20p' jenkins/scripts/run_tests.pyRepository: NVIDIA/TensorRT-LLM
Length of output: 15574
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import ast
source = Path("jenkins/scripts/run_tests.py").read_text()
tree = ast.parse(source)
functions = {}
for node in tree.body:
if isinstance(node, ast.FunctionDef) and node.name in {"check_and_rerun", "run_regular_tests", "run_isolated_tests"}:
functions[node.name] = ast.get_source_segment(source, node)
for name in ("check_and_rerun", "run_regular_tests", "run_isolated_tests"):
print(f"--- {name} ---")
print(functions.get(name, "<missing>"))
PYRepository: NVIDIA/TensorRT-LLM
Length of output: 9166
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import ast
path = Path("jenkins/scripts/run_tests.py")
source = path.read_text()
tree = ast.parse(source)
class CheckAndRerunVisitor(ast.NodeVisitor):
def __init__(self):
self.calls = []
self.loops = []
self.in_loop = 0
def visit_For(self, node):
self.in_loop += 1
self.generic_visit(node)
self.in_loop -= 1
def visit_Call(self, node):
if isinstance(node.func, ast.Name) and node.func.id == "check_and_rerun":
self.calls.append((node.lineno, self.in_loop))
self.generic_visit(node)
visitor = CheckAndRerunVisitor()
visitor.visit(tree)
print("check_and_rerun call sites (line, enclosing-loop-depth):", visitor.calls)
for node in tree.body:
if isinstance(node, ast.FunctionDef) and node.name == "check_and_rerun":
checks = [
n for n in ast.walk(node)
if isinstance(n, ast.Compare)
and isinstance(n.left, ast.Name)
and n.left.id == "valid_count"
]
print("check_and_rerun valid_count comparisons:",
[(n.lineno, ast.unparse(n)) for n in checks])
for node in tree.body:
if isinstance(node, ast.FunctionDef) and node.name == "run_isolated_tests":
loops = [
n for n in ast.walk(node)
if isinstance(n, ast.For) and isinstance(n.target, ast.Tuple)
]
print("run_isolated_tests per-test loop:",
[(n.lineno, ast.unparse(n.target), ast.unparse(n.iter)) for n in loops])
PY
printf '%s\n' '--- modified-file summary ---'
git diff --stat -- jenkins/scripts/slurm_run.shRepository: NVIDIA/TensorRT-LLM
Length of output: 435
Apply the rerun cap across isolated tests and add the NVIDIA copyright header.
run_isolated_tests calls check_and_rerun inside its per-test loop. The stage can therefore exceed five isolated reruns. Track the cap across the full isolated-test stage. Add the required NVIDIA copyright header to jenkins/scripts/slurm_run.sh.
🤖 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` at line 97, Update run_isolated_tests and its
check_and_rerun flow so the maximum of five reruns is tracked across the entire
isolated-test stage rather than reset for each test; preserve per-test execution
while sharing one stage-level counter or limit. Also add the required NVIDIA
copyright header at the top of slurm_run.sh.
| if [ -n "${testDurationsPath:-}" ]; then | ||
| runTestsArgs+=(--durations-path "$testDurationsPath") | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 '\b(testDurationsPath|clusterDurationsPathNode)\b' \
jenkins/L0_Test.groovy jenkins/scriptsRepository: NVIDIA/TensorRT-LLM
Length of output: 2579
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
groovy = Path("jenkins/L0_Test.groovy").read_text()
runner = Path("jenkins/scripts/slurm_run.sh").read_text()
producer = 'export testDurationsPath="$clusterDurationsPathNode"'
consumer = 'runTestsArgs+=(--durations-path "$testDurationsPath")'
print("producer_present:", producer in groovy)
print("consumer_present:", consumer in runner)
print("producer_context:")
for i, line in enumerate(groovy.splitlines(), 1):
if "testDurationsPath" in line or "clusterDurationsPathNode" in line:
print(f"{i}: {line}")
print("consumer_context:")
for i, line in enumerate(runner.splitlines(), 1):
if "testDurationsPath" in line or "--durations-path" in line:
print(f"{i}: {line}")
lines = runner.splitlines()
header = "\n".join(lines[:12])
print("nvidia_header_present_in_first_12_lines:",
"NVIDIA CORPORATION" in header or "Copyright (c)" in header and "NVIDIA" in header)
print("file_prefix:")
for i, line in enumerate(lines[:8], 1):
print(f"{i}: {line}")
PYRepository: NVIDIA/TensorRT-LLM
Length of output: 922
Add the NVIDIA copyright header. jenkins/scripts/slurm_run.sh has no NVIDIA copyright header. Add the header and set its year to the latest meaningful modification.
🤖 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 99 - 101, Add the standard NVIDIA
copyright header at the beginning of slurm_run.sh, using the latest meaningful
modification year for the copyright range or year. Preserve the existing script
logic, including the testDurationsPath argument handling.
| python3 "$llmSrcNode/tests/test_common/s3_output.py" \ | ||
| --drain-spool "$jobWorkspace" || true |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not discard spool-drain failures.
Line 119 ignores every non-zero result from s3_output.py --drain-spool. The stage can therefore report success while required deferred-upload artifacts are missing. Capture the drain status and include it in final_exit_code when test and performance checks pass.
Proposed status handling
+spool_exit_code=0
python3 "$llmSrcNode/tests/test_common/s3_output.py" \
- --drain-spool "$jobWorkspace" || true
+ --drain-spool "$jobWorkspace" || spool_exit_code=$?Then add spool_exit_code as the final fallback in the exit-code selection at Lines 145-151.
🤖 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 118 - 119, The spool-drain command
currently discards failures; capture its status in a spool_exit_code variable
instead of using || true. Update the final_exit_code selection so
spool_exit_code is the final fallback after test and performance results pass,
preserving the existing precedence of those checks.
|
PR_Github #65541 [ run ] triggered by Bot. Commit: |
|
PR_Github #65541 [ run ] completed with state
|
7 participants
Dev Engineer Review
Refactor
run_tests.py.Follow-up required
pytest --collect-onlyreturns a non-zero exit code, even when collection produces output.--max-rerun-testslimit.QA Engineer Review
No test changes.
Description
Test Coverage
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)
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.