Skip to content

[https://nvbugs/6435642][fix] avoid isinstance(MpiPoolSession) crash under session-reuse test infra - #16438

Closed
MrGeva wants to merge 1 commit into
NVIDIA:mainfrom
nv-auto-deploy:debug/proxy-typeerror
Closed

[https://nvbugs/6435642][fix] avoid isinstance(MpiPoolSession) crash under session-reuse test infra#16438
MrGeva wants to merge 1 commit into
NVIDIA:mainfrom
nv-auto-deploy:debug/proxy-typeerror

Conversation

@MrGeva

@MrGeva MrGeva commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

Description

_start_executor_workers() in tensorrt_llm/executor/proxy.py (added by #16338, merged 2026-07-14) does:

if isinstance(self.mpi_session, MpiPoolSession) and len(status) == 3:

tests/test_common/session_reuse.py — an existing, default-on pytest speedup for bare LLM(...) tests that reuses MPI worker pools across tests — monkeypatches this module's MpiPoolSession attribute to a pool-reuse factory function, so it can intercept MpiPoolSession(n_workers=...) construction calls at self.mpi_session = MpiPoolSession(n_workers=model_world_size) (line 130).

Under that patch, isinstance(x, MpiPoolSession) raises:

TypeError: isinstance() arg 2 must be a type, a tuple of types, or a union

because a function isn't a valid isinstance() type argument — crashing every MPI-mode LLM/executor test run under tests/unittest/ (session-reuse is wired in globally via pytest.ini's -p test_common.session_reuse_hooks), not anything specific to one test file.

Fix

Re-import MpiPoolSession from its origin module (tensorrt_llm.llmapi.mpi_session) at the check site, instead of relying on this module's module-level name — which must stay monkeypatchable for the pool-reuse factory to keep intercepting the construction call at line 130. The origin module's own MpiPoolSession attribute is never patched (only tensorrt_llm.executor.proxy and tensorrt_llm.llmapi.llm's references are), so this always resolves to the real class.

Test Coverage

Verified locally on 2×B200:

  • tests/unittest/auto_deploy/multigpu/smoke/test_ad_build_small_multi.py (the test that surfaced the bare TypeError in CI) — both cases pass, including the reused-pool path (2nd invocation is ~4x faster, confirming reuse still works).
  • tests/unittest/auto_deploy/multigpu/custom_ops/test_ad_dist_strategies.py::test_allreduce_strategies[AUTO/TWOSHOT] — pass.
  • tests/unittest/llmapi/test_mpi_session.py, tests/unittest/executor/test_proxy_fast_death.py, tests/unittest/executor/test_base_worker.py — 28/28 pass, no regressions on the non-reuse path.

So this is a classic cross-PR integration bug: #16338's author added a new isinstance check without being aware that #15777 (merged the day before, unrelated area) had already made MpiPoolSession a name that test infrastructure legitimately monkeypatches. Neither PR is
wrong in isolation — the interaction between them is what broke, and it wouldn't have been caught by either PR's own tests since neither one's test suite exercises the other's code path in a way that surfaces the conflict.

Summary by CodeRabbit

  • Bug Fixes
    • Improved executor startup reliability when using externally provided worker sessions.
    • Fixed an edge case that could cause incorrect initialization behavior in test and customized runtime environments.

…under session-reuse test infra

_start_executor_workers() added isinstance(self.mpi_session, MpiPoolSession)
to gate registering worker process identities with the new
WorkerProcessMonitor. tests/test_common/session_reuse.py (an existing,
default-on pytest speedup for bare LLM(...) tests) monkeypatches this
module's MpiPoolSession attribute to a pool-reuse factory *function* so it
can intercept `MpiPoolSession(n_workers=...)` construction calls. Under
that patch, isinstance(x, MpiPoolSession) raises "isinstance() arg 2 must
be a type, a tuple of types, or a union" because a function isn't a valid
isinstance() type argument, crashing every MPI-mode LLM/executor test.

Re-import MpiPoolSession from its origin module (tensorrt_llm.llmapi.mpi_session)
at the check site instead of relying on this module's (intentionally
monkeypatchable) module-level name, which the test-reuse pool-creation
factory at self.mpi_session = MpiPoolSession(...) still needs to keep using.

Signed-off-by: Eran Geva <19514940+MrGeva@users.noreply.github.com>
@MrGeva
MrGeva requested a review from a team as a code owner July 15, 2026 15:47
@MrGeva
MrGeva requested review from YihuiLu512 and lori-ren July 15, 2026 15:47
@MrGeva

MrGeva commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator Author

/bot run

@coderabbitai

coderabbitai Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

GenerationExecutorProxy now re-imports the origin MpiPoolSession class when checking externally provided MPI pool sessions during worker initialization and requires status to contain three elements.

Changes

MPI pool session initialization

Layer / File(s) Summary
Worker initialization type guard
tensorrt_llm/executor/proxy.py
The MPI pool session check uses a re-imported class reference and only enters the specialized branch when the initialization status has length three.

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

Possibly related PRs

Suggested reviewers: lori-ren, yihuilu512, chienchunhung

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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 fix and clearly states the MPI isinstance crash under session-reuse testing.
Description check ✅ Passed The description explains the bug, the fix, and includes relevant test coverage, so it is mostly complete.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #59472 [ run ] triggered by Bot. Commit: d8613d3 Link to invocation

@MrGeva
MrGeva enabled auto-merge (squash) July 15, 2026 16:07
@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #59472 [ run ] completed with state SUCCESS. Commit: d8613d3
/LLM/main/L0_MergeRequest_PR pipeline #47937 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

@MrGeva

MrGeva commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator Author

/bot run

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #59506 [ run ] triggered by Bot. Commit: d8613d3 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #59506 [ run ] completed with state SUCCESS. Commit: d8613d3
/LLM/main/L0_MergeRequest_PR pipeline #47964 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

@chienchunhung

Copy link
Copy Markdown
Collaborator

FYI #16444 has similar fix, plus verifying and unwaiving the previously failing tests.

@MrGeva

MrGeva commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator Author

closing this PR in favor of #16444

@MrGeva MrGeva closed this Jul 16, 2026
auto-merge was automatically disabled July 16, 2026 04:48

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants