Skip to content

[None][test] Add a timeout to the subprocess.run call in test_bolt - #20305

Merged
chenfeiz0326 merged 1 commit into
NVIDIA:mainfrom
chenfeiz0326:fix/test-bolt-subprocess-timeout
Oct 15, 2026
Merged

chenfeiz0326 merged 1 commit into
NVIDIA:mainfrom
chenfeiz0326:fix/test-bolt-subprocess-timeout

Conversation

@chenfeiz0326

@chenfeiz0326 chenfeiz0326 commented Oct 14, 2026 •

Copy link
Copy Markdown
Collaborator

Description

tests/unittest/scripts/test_subprocess_timeouts.py::test_subprocess_run_calls_have_timeouts (from #18812) requires every subprocess.run under tests/ to pass a timeout. #20024 added a gcc call in tests/unittest/scripts/test_bolt.py (_compile_so, which builds a scratch shared object) without one. Since then, the unittest/scripts stage has failed on pre-merge runs on CPU-Generic-x86-1/arm-1. In recent L0 builds it failed on 7 unrelated PRs (e.g. #20099, #20151, #19492, #19722, #19275, #20253, #20250):

FAILED unittest/scripts/test_subprocess_timeouts.py::test_subprocess_run_calls_have_timeouts
  AssertionError: subprocess.run calls without timeout:
    unittest/scripts/test_bolt.py:123

This PR adds timeout=60 to that call. It compiles a one-line C file, so 60 s is generous. A hang now fails the test instead of the stage.

Test Coverage

  • I ran the policy test's own checker (_candidate_paths + _missing_timeout_lines) over the patched tree: 119 files scanned, 0 violations. On current main it reports tests/unittest/scripts/test_bolt.py:123.
  • test_bolt.py behaviour is unchanged, apart from the bound on that gcc call.

PR Checklist

  • PR description clearly explains what and why.
  • Follows the TensorRT-LLM coding guidelines.
  • Test cases are provided.

🤖 Generated with Claude Code

Dev Engineer Review

_compile_so now passes timeout=60 to subprocess.run when it invokes gcc. This satisfies the existing policy test, which checks that subprocess.run calls in the test tree specify a timeout. No other behavior change is reported. Current review finding counts are unavailable.

QA Engineer Review

tests/unittest/scripts/test_bolt.py changed. The change does not add or modify test functions; it bounds the compiler subprocess used by existing tests. The policy test checks for missing timeouts, but no test run result is provided. Coverage verdict: sufficient for this timeout-policy change. No CI or manual-QA test-list entry applies to this unit test file.

Per-File QA Perspective

  • tests/unittest/scripts/test_bolt.py: Existing BOLT helper tests compile temporary shared objects through _compile_so; the compiler call now stops after 60 seconds. This unit test file is not listed in the integration CI or manual-QA lists.

test_subprocess_timeouts requires every subprocess.run under tests/ to
pass a timeout. The gcc call that builds a scratch shared object in
test_bolt.py had none, so the policy test fails on every pre-merge run.
Bound it at 60 seconds.

Signed-off-by: chenfeiz0326 <203214996+chenfeiz0326@users.noreply.github.com>
@chenfeiz0326
chenfeiz0326 requested a review from a team as a code owner October 14, 2026 16:55
@coderabbitai

coderabbitai Bot commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: NVIDIA/TensorRT-LLM/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Enterprise
  • Run ID: 49c0fc6b-0ffd-4fee-aba3-bd009292f225
📥 Commits

Reviewing files that changed from the base of the PR and between 02f5556 and 23b8db8.

📒 Files selected for processing: 1
  • tests/unittest/scripts/test_bolt.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.

Walkthrough

The _compile_so helper now sets a 60-second timeout for the GCC subprocess. It continues to return whether compilation succeeded.

Changes

GCC compile timeout

Layer / File(s) Summary
Set GCC subprocess timeout
tests/unittest/scripts/test_bolt.py
The GCC subprocess call now uses a 60-second timeout. The helper still returns whether compilation succeeded.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~3 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 23b8d

The GCC test invocation is now bounded at 60 seconds, and no actionable merge-blocking risk is established.

Pre-merge checks: Passed 5
Passed Passed checks: 5 passed
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: adding a timeout to the subprocess.run call in test_bolt. It follows the repository title format.
Description check Passed The description includes the required Description, Test Coverage, and PR Checklist sections. It explains the issue, the solution, the affected test, and the reported validation results.
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@chenfeiz0326

Copy link
Copy Markdown
Collaborator Author

/bot run

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #77881 [ run ] triggered by Bot. Commit: 23b8db8 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #77881 [ run ] completed with state SUCCESS. Commit: 23b8db8
/LLM/main/L0_MergeRequest_PR pipeline #64200 completed with status: 'SUCCESS'

CI Report

Link to invocation

@chenfeiz0326

Copy link
Copy Markdown
Collaborator Author

CI is green. @mlefeb01 @yihwang-nv, could one of you take a look? It's a one-line timeout=60 on the gcc call #20024 added to test_bolt.py. Without it, test_subprocess_timeouts (#18812) fails the unittest/scripts stage on every pre-merge run against current main, so other PRs (e.g. #20250) can't get a green CI until this lands.

@trtllm-agent

Copy link
Copy Markdown
Collaborator

[Repair Bot] NVBug 6929769 appears to have the same root cause as this PR.
No separate PR was opened.

  • Failing test: pytest tests/unittest/scripts/test_subprocess_timeouts.py::test_subprocess_run_calls_have_timeouts -v
  • Comparison: The existing PR's structured evidence fields are empty, but its description quotes the candidate's exact failure: test_subprocess_timeouts.py::test_subprocess_run_calls_have_timeouts fails with 'subprocess.run calls without timeout: unittest/scripts/test_bolt.py:123', on CPU-Generic-x86-1/arm-1. Both name the same cause: the _compile_so gcc helper added in [TRTLLMINF-336][fix] Stop emitting cortex-a53-843419 veneers in BOLT builds #20024 calls subprocess.run without a timeout. The test_bolt.py change is identical in both (adds timeout=60). The candidate also removes the

If this is a different root cause, an NVIDIA collaborator can request a separate attempt by commenting /repair-bot continue 6929769.

@github-actions

Copy link
Copy Markdown

Automatically added "ci: full pre-merge approved" because this PR has satisfied the required GitHub review approvals. Unresolved review conversations and other required checks remain independent merge requirements.

@chenfeiz0326
chenfeiz0326 merged commit bb18d36 into NVIDIA:main Oct 15, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants