Skip to content

[https://nvbugs/6463812][fix] Import MpiPoolSession from its source module (tensorrt_llm.llmapi.mpi_session)…#16496

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

[https://nvbugs/6463812][fix] Import MpiPoolSession from its source module (tensorrt_llm.llmapi.mpi_session)…#16496
trtllm-agent wants to merge 1 commit into
NVIDIA:mainfrom
tensorrt-cicd:repair-bot-bug6463812

Conversation

@trtllm-agent

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

Copy link
Copy Markdown
Collaborator

Summary

  • Root cause: tests/test_common/session_reuse.py rebinds proxy.MpiPoolSession to a factory function, so isinstance(self.mpi_session, MpiPoolSession) raises TypeError.
  • Fix: Import MpiPoolSession from its source module (tensorrt_llm.llmapi.mpi_session) inline at the check site so isinstance always receives the concrete class, and remove the corresponding waiver.
  • 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 startup reliability when running in environments with customized test configurations.
  • Tests

    • Re-enabled an end-to-end text generation integration test, expanding validation coverage for encoder-decoder models.

…est session-reuse monkey-patch

The BART TP=2 CUDA-graph proxy setup failed with:
  TypeError: isinstance() arg 2 must be a type, a tuple of types, or a union

tests/test_common/session_reuse.py rebinds
tensorrt_llm.executor.proxy.MpiPoolSession to a factory function so the test
suite can intercept pool construction for reuse. Once patched, the
module-level name is a callable, not a class, and the
isinstance(self.mpi_session, MpiPoolSession) check in
_start_executor_workers raises TypeError.

Import the concrete class directly from tensorrt_llm.llmapi.mpi_session
inside the check site so isinstance always receives the real type, while
leaving the module-level import untouched (the constructor call is what
the session-reuse harness intentionally intercepts). Remove the
corresponding 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: d9ccde4f-a8c0-4356-a0f4-5859c55073be

📥 Commits

Reviewing files that changed from the base of the PR and between b602fa6 and 01c457d.

📒 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 executor proxy now re-imports MpiPoolSession before its worker-startup type check. The BART PyTorch encoder-decoder integration test is removed from the skipped-test waiver list.

Changes

MPI session startup and BART integration coverage

Layer / File(s) Summary
Reload MPI session class for worker startup
tensorrt_llm/executor/proxy.py
A local MpiPoolSession import makes the isinstance check use the real class when the module-level reference is monkey-patched.
Remove BART integration waiver
tests/integration/test_lists/waives.txt
Removes the test_bart_pytorch_generate_encoder_decoder_end_to_end skip waiver associated with NVBUG 6463812.

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

Possibly related PRs

Suggested reviewers: brnguyen2

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title is specific and matches the main fix: importing MpiPoolSession at the check site for the nvbugged TypeError.
Description check ✅ Passed It covers the issue, fix, test plan, and bug link, though it doesn't use the template's exact Description/Test Coverage/PR Checklist headings.
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.

@BowenFu

BowenFu commented Jul 16, 2026

Copy link
Copy Markdown

Heads up: this is one of three competing fixes for the same one-line session check in proxy.py#16417, #16490, #16496 (same line, would conflict; only one should land). This re-import approach fixes the symptom but not the root fragility (it still breaks if tensorrt_llm.llmapi.mpi_session.MpiPoolSession itself is patched). #16490's polymorphic self.mpi_session.is_comm_session() is immune to that. Suggest consolidating on #16490 and closing this one, folding its nvbug un-waive into #16490.

@brnguyen2

Copy link
Copy Markdown
Collaborator

Closing as it's superseded by #16444

@brnguyen2 brnguyen2 closed this Jul 16, 2026
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