Skip to content

[https://nvbugs/6426847][fix] Invert the check to `not isinstance(self.mpi_session, (MpiCommSession… - #16417

Closed
trtllm-agent wants to merge 3 commits into
NVIDIA:mainfrom
tensorrt-cicd:repair-bot-bug6426847
Closed

[https://nvbugs/6426847][fix] Invert the check to `not isinstance(self.mpi_session, (MpiCommSession…#16417
trtllm-agent wants to merge 3 commits into
NVIDIA:mainfrom
tensorrt-cicd:repair-bot-bug6426847

Conversation

@trtllm-agent

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

Copy link
Copy Markdown
Collaborator

Summary

  • Root cause: session_reuse test hook monkey-patches MpiPoolSession in tensorrt_llm.executor.proxy with a factory function, so isinstance(self.mpi_session, MpiPoolSession) raises TypeError: isinstance() arg 2 must be a type because the second arg is a function.
  • Fix: Invert the check to not isinstance(self.mpi_session, (MpiCommSession, RemoteMpiCommSessionClient)) — those two types are never monkey-patched and their absence uniquely identifies the local pool-session case (same fix pattern used earlier in the same function).
  • 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 worker process monitoring across supported execution session types.
    • Ensured worker identities are registered consistently when initialization provides the expected status information.

@coderabbitai

coderabbitai Bot commented Jul 15, 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: 068cf9b1-282d-4bbd-bb69-670d14929b49

📥 Commits

Reviewing files that changed from the base of the PR and between 3b330ca and 87aec0e.

📒 Files selected for processing (1)
  • tensorrt_llm/executor/proxy.py

📝 Walkthrough

Walkthrough

The executor worker startup logic now extracts and registers worker process identities for any three-element initialization status unless the MPI session is a communication session or remote communication client.

Changes

Worker identity registration

Layer / File(s) Summary
Broaden worker identity registration guard
tensorrt_llm/executor/proxy.py
Worker process identities are registered when initialization returns a three-element status for MPI sessions other than MpiCommSession and RemoteMpiCommSessionClient.

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

Possibly related PRs

Suggested reviewers: cascade812, tongyuantongyu, shixiaowei02

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title matches the PR’s main fix and follows the required [ticket][type] pattern.
Description check ✅ Passed The description covers the bug, fix, tests, and link, so it is mostly complete despite template section name mismatches.
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.

@QiJune

QiJune commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

/bot run

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #59436 [ run ] triggered by Bot. Commit: 87aec0e Link to invocation

@QiJune
QiJune enabled auto-merge (squash) July 15, 2026 10:42
…patch

The session-reuse test infrastructure (tests/test_common/session_reuse.py)
monkey-patches MpiPoolSession in tensorrt_llm.executor.proxy with a factory
function to enable pool reuse across LLM instances. This broke
isinstance(self.mpi_session, MpiPoolSession) in _start_executor_workers with
TypeError: isinstance() arg 2 must be a type, a tuple of types, or a union.

Switch to a negative check against the never-patched MpiCommSession /
RemoteMpiCommSessionClient types, matching the pattern already used earlier
in the same function.

Signed-off-by: trtllm-agent <296075020+trtllm-agent@users.noreply.github.com>
Signed-off-by: trtllm-agent <296075020+trtllm-agent@users.noreply.github.com>
auto-merge was automatically disabled July 15, 2026 12:14

Head branch was pushed to by a user without write access

@trtllm-agent
trtllm-agent force-pushed the repair-bot-bug6426847 branch from 87aec0e to 333a4fb Compare July 15, 2026 12:14
@trtllm-agent
trtllm-agent requested review from a team as code owners July 15, 2026 12:14
@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #59436 [ run ] completed with state SUCCESS. Commit: 87aec0e
/LLM/main/L0_MergeRequest_PR pipeline #47905 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
chienchunhung enabled auto-merge (squash) July 15, 2026 15:48
@chienchunhung

Copy link
Copy Markdown
Collaborator

FYI I am preparing PR#16444 as the fix and test unwaiving.

@yufeiwu-nv
yufeiwu-nv removed their request for review July 16, 2026 05:53
@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 (they touch the same line and would conflict; only one should land). #16490 uses the polymorphic self.mpi_session.is_comm_session(), which resolves the class refs inside mpi_session's namespace and is immune to the test-infra rebinding of the module-level MpiPoolSession symbol (the root cause). Suggest consolidating on #16490 and closing this one, folding its nvbug un-waive into #16490.

@brnguyen2

Copy link
Copy Markdown
Collaborator

Superseded by #16444, which has merged.

@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.

7 participants