[https://nvbugs/6450338][fix] Replace class-based isinstance check with the existing…#16419
[https://nvbugs/6450338][fix] Replace class-based isinstance check with the existing…#16419trtllm-agent wants to merge 1 commit into
isinstance check with the existing…#16419Conversation
… rebind The test suite's session-reuse helper (tests/test_common/session_reuse.py) rebinds the module-level MpiPoolSession symbol in tensorrt_llm.executor.proxy to a factory function so cached pools can be handed to subsequent bare LLM(...) tests. Once the symbol is a function object, the isinstance(self.mpi_session, MpiPoolSession) check in _start_executor_workers raises TypeError: isinstance() arg 2 must be a type, a tuple of types, or a union. Switch to the polymorphic predicate self.mpi_session.is_comm_session() already defined on MpiSession. Method resolution goes through the real instance (or the _ReusableSession wrapper's __getattr__ delegation), so this works whether the module symbol is rebound or not, and it is more robust than a class-based check for a state that already has an abstract API on the base class. Also unwaive the previously-skipped TestGPTOSS::test_w4_4gpus[v1_kv_cache-ep4-trtllm-fp8]. Signed-off-by: trtllm-agent <296075020+trtllm-agent@users.noreply.github.com>
|
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 (2)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThe executor now identifies non-communication MPI sessions polymorphically when processing worker initialization status. The related GPT-OSS integration test waiver is removed. ChangesMPI worker initialization
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Thanks for the automated attempt, but this doesn't address the failure tracked in https://nvbugs/6450338. That failure is a plain accuracy assertion (GPQA 55.051 vs threshold 55.734) — there is no TypeError anywhere in the CI logs — and |
Summary
isinstance(self.mpi_session, MpiPoolSession)raises TypeError becausetests/test_common/session_reuse.pyrebinds proxy.MpiPoolSession to a factory functionisinstancecheck with the existingself.mpi_session.is_comm_session()predicate; unwaive the previously-skipped testTest plan
Links
Summary by CodeRabbit
Bug Fixes
Tests