Skip to content

[https://nvbugs/6462303][fix] Import MpiPoolSession under a second private alias (_RealMpiPoolSession)…#16497

Closed
trtllm-agent wants to merge 1 commit into
NVIDIA:mainfrom
tensorrt-cicd:repair-bot-bug6462303
Closed

[https://nvbugs/6462303][fix] Import MpiPoolSession under a second private alias (_RealMpiPoolSession)…#16497
trtllm-agent wants to merge 1 commit into
NVIDIA:mainfrom
tensorrt-cicd:repair-bot-bug6462303

Conversation

@trtllm-agent

@trtllm-agent trtllm-agent commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Root cause: isinstance(self.mpi_session, MpiPoolSession) used the module-level MpiPoolSession name, which the test-suite session-reuse infra monkey-patches to a factory function; isinstance() against a non-class raises TypeError.
  • Fix: Import MpiPoolSession under a second private alias (_RealMpiPoolSession) that the test infra never patches, and use it for the isinstance check; construction still uses the patched name so pool reuse is preserved. Waiver removed.
  • Automated fix generated by repair-bot

Test plan

  • Verify fix on the same GPU type as the original failure
  • Check for regressions in related tests

Links

Summary by CodeRabbit

  • Bug Fixes
    • Improved executor worker initialization to remain reliable when runtime session components are replaced during testing.
    • Removed an outdated integration-test waiver, allowing the cache eviction and block reuse scenario to run normally.

…instance check

The recently added `isinstance(self.mpi_session, MpiPoolSession)` check in
proxy._start_executor_workers relied on the module-level `MpiPoolSession`
name. However, the test-suite session-reuse infra (tests/test_common/
session_reuse.py) monkey-patches that same module-level symbol to a
factory *function* for pool reuse. isinstance() against a non-class then
raises "TypeError: isinstance() arg 2 must be a type, a tuple of types,
or a union", breaking every LLM(...) construction while the pool cache
is active.

Bind the real class under a private alias that the test infra never
patches, and use it for the isinstance check. Construction still goes
through the patched `MpiPoolSession` name, so pool reuse continues to
work. Also remove the now-obsolete waiver.

Signed-off-by: trtllm-agent <296075020+trtllm-agent@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 56c09db4-31ca-4a73-b037-4194baf78e4a

📥 Commits

Reviewing files that changed from the base of the PR and between b602fa6 and 081d499.

📒 Files selected for processing (2)
  • tensorrt_llm/executor/proxy.py
  • tests/integration/test_lists/waives.txt
💤 Files with no reviewable changes (1)
  • tests/integration/test_lists/waives.txt

📝 Walkthrough

Walkthrough

The worker startup path now checks against the original MpiPoolSession class despite module-level monkey-patching. The waiver for the KV-cache block-reuse eviction test is removed.

Changes

MPI worker initialization

Layer / File(s) Summary
Preserve real MPI session type and update startup detection
tensorrt_llm/executor/proxy.py, tests/integration/test_lists/waives.txt
Aliases the original MpiPoolSession for the _start_executor_workers type check while retaining the runtime-facing import, and removes the KV-cache eviction test waiver.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested reviewers: cascade812

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title is specific and matches the main fix: aliasing MpiPoolSession to avoid monkey-patched isinstance() failures.
Description check ✅ Passed The description covers the root cause, fix, tests, and bug link, with only minor template differences from the repo format.
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.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@brnguyen2

Copy link
Copy Markdown
Collaborator

Closing this as it's superseded by #16444

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.

2 participants