[https://nvbugs/6461799][fix] Import MpiPoolSession from its source module…#16499
Closed
trtllm-agent wants to merge 1 commit into
Closed
[https://nvbugs/6461799][fix] Import MpiPoolSession from its source module…#16499trtllm-agent wants to merge 1 commit into
MpiPoolSession from its source module…#16499trtllm-agent wants to merge 1 commit into
Conversation
…sinstance check The test-side session-reuse plugin (tests/test_common/session_reuse.py) monkey-patches the module-level MpiPoolSession attribute in tensorrt_llm.executor.proxy with a factory function to intercept pool construction. This is fine for the constructor call site, but breaks the isinstance(self.mpi_session, MpiPoolSession) guard added by e05790a (nvbugs/6435642) — isinstance() rejects a function as its second argument with TypeError. Import MpiPoolSession from tensorrt_llm.llmapi.mpi_session at the use site so the class reference is resolved from the source module and is immune to attribute-level patching on the proxy module. Signed-off-by: trtllm-agent <296075020+trtllm-agent@users.noreply.github.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesMpiPoolSession type-check handling
Estimated code review effort: 2 (Simple) | ~5 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Member
|
Close in favor of #16444. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
tests/test_common/session_reuse.pymonkey-patchesMpiPoolSessionin thetensorrt_llm.executor.proxymodule namespace with a factory function, soisinstance(self.mpi_session, MpiPoolSession)at proxy.py:576 raisesTypeError: isinstance() arg 2 must be a typebecause the second argument is a function, not a class.MpiPoolSessionfrom its source module (tensorrt_llm.llmapi.mpi_session) at the use site so the class reference is resolved fresh from the source module (immune to attribute-level monkeypatching intensorrt_llm.executor.proxy); leaves the constructor-call site at proxy.py:130 unchanged so session-reuse still intercepts pool creation.Test plan
Links
Summary by CodeRabbit
Bug Fixes
Chores