[https://nvbugs/6480574][fix] seed attrs bypassed by __new__ in pool session shutdown test by JunyiXu-nv · Pull Request #16630 · NVIDIA/TensorRT-LLM

JunyiXu-nv · 2026-07-20T15:02:57Z

Summary by CodeRabbit

Tests
- Strengthened coverage for pool session shutdown behavior.
- Verified that shutdown remains non-blocking after exit joins are released, avoiding waits on an inactive pool manager.

Description

test_pool_session_shutdown_never_blocks_after_release constructs a MpiPoolSession with __new__() and only sets mpi_pool, then calls release_exit_joins() and shutdown(). MpiPoolSession.shutdown() reads self.n_workers (log line) and self._wait_shutdown (post-shutdown worker barrier); on the raw-constructed session both are absent and the second attribute access raises AttributeError before the code under test runs.

Seed the two attributes on the mock session so the test exercises the release/shutdown contract instead of failing on attribute lookup. Keep _wait_shutdown=False because the test targets the blocking-join guard, not the identity-based worker-exit wait.

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)
If PR introduces API changes, an appropriate PR label is added - either api-compatible or api-breaking. For api-breaking, include BREAKING in 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.

coderabbitai · 2026-07-20T15:05:31Z

📝 Walkthrough

Walkthrough

The shutdown test now initializes n_workers and _wait_shutdown on a partially constructed MpiPoolSession, allowing it to verify non-blocking shutdown behavior after release_exit_joins().

Changes

Proxy fast-death test coverage

Layer / File(s)	Summary
Initialize shutdown test state `tests/unittest/executor/test_proxy_fast_death.py`	The test seeds internal session attributes before invoking shutdown and continues asserting that `pool.shutdown(wait=False)` is used.

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

Possibly related PRs

NVIDIA/TensorRT-LLM#16613: Updates MpiPoolSession.shutdown() handling for partially initialized or released sessions.

Suggested reviewers: jiaganc

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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.
Title check	✅ Passed	The title matches the PR's bug fix and follows the required [ticket][type] summary format.
Description check	✅ Passed	The description clearly explains the bug and fix, and the checklist is present, but the Test Coverage section is left empty.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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

JunyiXu-nv · 2026-07-21T02:08:20Z

/bot run --disable-fail-fast

tensorrt-cicd · 2026-07-21T02:14:28Z

PR_Github #60529 [ run ] triggered by Bot. Commit: e5fb79e Link to invocation

tensorrt-cicd · 2026-07-21T02:50:39Z

PR_Github #60529 [ run ] completed with state SUCCESS. Commit: e5fb79e
/LLM/main/L0_MergeRequest_PR pipeline #48850 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

Please check the failed tests and fix your PR
If you cannot view the failures, ask the CI triggerer to share details
Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

JunyiXu-nv · 2026-07-21T05:03:39Z

/bot run

tensorrt-cicd · 2026-07-21T05:10:24Z

PR_Github #60581 [ run ] triggered by Bot. Commit: e5fb79e Link to invocation

tensorrt-cicd · 2026-07-21T05:27:21Z

PR_Github #60581 [ run ] completed with state FAILURE. Commit: e5fb79e
/LLM/main/L0_MergeRequest_PR pipeline #48891 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

Please check the failed tests and fix your PR
If you cannot view the failures, ask the CI triggerer to share details
Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

…session shutdown test test_pool_session_shutdown_never_blocks_after_release constructs a MpiPoolSession with __new__() and only sets mpi_pool, then calls release_exit_joins() and shutdown(). MpiPoolSession.shutdown() reads self.n_workers (log line) and self._wait_shutdown (post-shutdown worker barrier); on the raw-constructed session both are absent and the second attribute access raises AttributeError before the code under test runs. Seed the two attributes on the mock session so the test exercises the release/shutdown contract instead of failing on attribute lookup. Keep _wait_shutdown=False because the test targets the blocking-join guard, not the identity-based worker-exit wait. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>

…ver_blocks_after_release The test now seeds the attributes that MpiPoolSession.shutdown() reads, so the AttributeError raised on the raw-constructed session is gone and the test passes. Remove the waive. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>

JunyiXu-nv · 2026-07-21T10:40:10Z

/bot run --disable-fail-fast

tensorrt-cicd · 2026-07-21T10:46:18Z

PR_Github #60678 [ run ] triggered by Bot. Commit: 1571acd Link to invocation

tensorrt-cicd · 2026-07-21T11:25:03Z

PR_Github #60678 [ run ] completed with state SUCCESS. Commit: 1571acd
/LLM/main/L0_MergeRequest_PR pipeline #48972 completed with status: 'SUCCESS'

CI Report

Link to invocation

BowenFu

LGTM — test-only. Seeds n_workers/_wait_shutdown on the new-built MpiPoolSession (mirroring init defaults) so shutdown()'s wait=False path is correctly exercised, and un-waives the case. No production code touched, copyright header current, CI green — a valid regression guard for nvbugs/6480574.

niukuo

LGTM

…ool-session test test_proxy_fast_death builds GenerationExecutorProxy / MpiPoolSession via __new__ (bypassing __init__), then drives shutdown(). A recent main change (NVIDIA#16523, multi-process HTTP frontends) made GenerationExecutorProxy.shutdown() -> _cleanup_multi_frontend_ipc_dir() read self._multi_frontend_ipc_dir unguarded, so the __new__-built proxies raise AttributeError at GC/teardown (test_shutdown_does_not_block_on_dead_engine and the two sibling shutdown tests). This is a cross-PR merge-skew: NVIDIA#16312 added the __new__-based tests, NVIDIA#16523 later added the unguarded attribute access; neither PR tested the combination. Seed _multi_frontend_ipc_dir / _multi_frontend_hmac in the shared _bare_proxy() helper so the teardown path is a clean no-op, mirroring how NVIDIA#16630 (nvbugs/6480574) handled the earlier n_workers/_wait_shutdown variant. Also re-align with main: restore the n_workers/_wait_shutdown seeding in test_pool_session_shutdown_never_blocks_after_release and drop its now-removed waive (main dropped it in NVIDIA#16630). Whole file: 23 passed (was 3 failed + 1 waived/broken). Signed-off-by: Yao Yao <lowsfer@users.noreply.github.com>

…roxy_fast_death __new__ seeding Two base-branch (main) regressions surfaced by rebasing onto latest main; neither is in the KVCacheManagerV2 change set. 1) tensorrt_llm import break (nvbugs/6442074): SpecWorkerBase.__init_subclass__ forbids subclasses from overriding forward() -- they must implement _forward_impl so the base forward() wrapper can guarantee spec-dec attn-metadata cleanup. MTPEagleDynamicTreeWorker still overrode forward(), so 'import tensorrt_llm' raised TypeError at class-definition time via the normal _torch.models -> speculative -> mtp_dynamic_tree import chain, breaking all test collection / package sanity. Rename its forward() to _forward_impl(), matching every sibling worker (MTPEagleWorker, DSparkWorker, eagle3, eagle3_dynamic_tree); callers use the base forward() wrapper so no caller changes. Verified: 'import tensorrt_llm' succeeds again. 2) test_proxy_fast_death cross-PR merge-skew: NVIDIA#16312 added __new__-built proxy tests; NVIDIA#16523 later made GenerationExecutorProxy.shutdown() -> _cleanup_multi_frontend_ipc_dir() read self._multi_frontend_ipc_dir unguarded, so the __new__ proxies raised AttributeError at teardown. Seed _multi_frontend_ipc_dir/_multi_frontend_hmac in the shared _bare_proxy() helper so GC-time teardown is a clean no-op, mirroring NVIDIA#16630 (nvbugs/6480574) for the earlier n_workers variant. Signed-off-by: Yao Yao <lowsfer@users.noreply.github.com>

…new__ proxy mock (drop after upstream fix) NVIDIA#16523 added _multi_frontend_ipc_dir (set in __init__) and shutdown() now always calls _cleanup_multi_frontend_ipc_dir(), including the workers_started=False early path. test_proxy_fast_death.py builds proxies via GenerationExecutorProxy.__new__ without __init__, so every shutdown() (and the __del__ GC path) raises AttributeError on main. Seed the attr in _bare_proxy(), same pattern as NVIDIA#16630. Temporary carry so this PR's CI can go green; becomes a no-op once the equivalent fix lands on main. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: longcheng-nv <243710427+longcheng-nv@users.noreply.github.com>

…e re-added during rebase (fixed by NVIDIA#16630) Signed-off-by: Chuang Zhu <111838961+chuangz0@users.noreply.github.com>

JunyiXu-nv requested a review from a team as a code owner July 20, 2026 15:02

JunyiXu-nv requested review from YihuiLu512 and zhaoyangwang-nvidia July 20, 2026 15:02

github-actions Bot assigned JunyiXu-nv Jul 20, 2026

lowsfer mentioned this pull request Jul 21, 2026

[https://nvbugs/6480574][fix] mpi_session: guard MpiPoolSession.shutdown against partial init #16613

Closed

JunyiXu-nv requested review from a team as code owners July 21, 2026 10:38

JunyiXu-nv requested review from BowenFu, QiJune, crazydemo, dpitman-nvda, jieli-matrix and niukuo July 21, 2026 10:39

JunyiXu-nv added 2 commits July 21, 2026 18:39

JunyiXu-nv force-pushed the dev-junyix-fix-nvbug-6480574 branch from 0a09355 to 1571acd Compare July 21, 2026 10:39

BowenFu approved these changes Jul 21, 2026

View reviewed changes

jieli-matrix approved these changes Jul 22, 2026

View reviewed changes

JunyiXu-nv enabled auto-merge (squash) July 22, 2026 02:29

niukuo approved these changes Jul 22, 2026

View reviewed changes

zhaoyangwang-nvidia approved these changes Jul 22, 2026

View reviewed changes

JunyiXu-nv merged commit c54538b into NVIDIA:main Jul 22, 2026
8 checks passed

This was referenced Jul 22, 2026

[TRTLLM-14510][feat] serve: multi-process HTTP frontends on the classic IPC executor path #16523

Merged

[None][perf] GVR top-K decode: enable R0 histogram-ladder admission by default #16457

Open

qiaoxj07 mentioned this pull request Jul 22, 2026

[None][perf] Avoid implicit device-scalar syncs in DeepSeek-V4 ctx sparse metadata #16734

Merged

chzblych mentioned this pull request Jul 23, 2026

[https://nvbugs/6426834][fix] Deflake test_kv_transfer: cap NIXL progress threads, pin UCX env #16708

Merged

1 task

Uh oh!

Conversation

JunyiXu-nv commented Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Description

Test Coverage

PR Checklist

GitHub Bot Help

Uh oh!

coderabbitai Bot commented Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

JunyiXu-nv commented Jul 21, 2026

Uh oh!

tensorrt-cicd commented Jul 21, 2026

Uh oh!

tensorrt-cicd commented Jul 21, 2026

Uh oh!

JunyiXu-nv commented Jul 21, 2026

Uh oh!

tensorrt-cicd commented Jul 21, 2026

Uh oh!

tensorrt-cicd commented Jul 21, 2026

Uh oh!

JunyiXu-nv commented Jul 21, 2026

Uh oh!

tensorrt-cicd commented Jul 21, 2026

Uh oh!

tensorrt-cicd commented Jul 21, 2026

Uh oh!

BowenFu left a comment

Choose a reason for hiding this comment

Uh oh!

niukuo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

JunyiXu-nv commented Jul 20, 2026 •

edited

Loading

coderabbitai Bot commented Jul 20, 2026 •

edited

Loading