Skip to content

[None][test] Make the session-reuse pool seam isinstance-transparent - #16520

Closed
sunnyqgg wants to merge 1 commit into
NVIDIA:mainfrom
sunnyqgg:fix_reuse_seam_isinstance
Closed

[None][test] Make the session-reuse pool seam isinstance-transparent#16520
sunnyqgg wants to merge 1 commit into
NVIDIA:mainfrom
sunnyqgg:fix_reuse_seam_isinstance

Conversation

@sunnyqgg

@sunnyqgg sunnyqgg commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Summary

Changes

  • tests/test_common/session_reuse.py: install an isinstance-transparent metaclass shim at the seams instead of a bare function.
  • tests/unittest/llmapi/test_session_reuse.py: shim unit test (construction routing + instance/subclass transparency) and an end-to-end regression test against the real patched tensorrt_llm.executor.proxy seam (raises TypeError on pre-fix code).

Test plan

  • tests/unittest/llmapi/test_session_reuse.py — 23 passed (21 existing + 2 new) with the session-reuse plugin active
  • Pre-merge CI

Summary by CodeRabbit

  • Bug Fixes

    • Improved session reuse compatibility with Python type checks.
    • Prevented errors when validating pooled sessions with isinstance or issubclass.
  • Tests

    • Added coverage for transparent session construction and type-checking behavior.
    • Added end-to-end validation for the session reuse integration.

The pool-creation seams held a plain FUNCTION in place of
MpiPoolSession. Library code doing isinstance(x, MpiPoolSession)
against the patched module attribute then raises 'TypeError:
isinstance() arg 2 must be a type' — proxy.py's killed-worker detection
added exactly such a check and every bare LLM() creation failed until
it was worked around with an exclusion-based match in the consumer.

Remove the hazard class at its source: the seam now holds a real class
whose metaclass routes construction to the reuse factory and
instance/subclass checks to the real MpiPoolSession, so both usage
patterns keep working — including isinstance consumers added after this
layer was written. The exclusion-based check in
_register_worker_processes can be reverted to the natural
isinstance(self.mpi_session, MpiPoolSession) form at leisure.

Signed-off-by: qgai <qgai@nvidia.com>
@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The session reuse test seam now replaces MpiPoolSession with a type-preserving shim that forwards construction to factories while keeping isinstance and issubclass checks valid. Tests cover both the shim directly and the patched proxy seam.

Changes

Session reuse seam

Layer / File(s) Summary
Transparent MpiPoolSession shim
tests/test_common/session_reuse.py
Adds a metaclass-based shim that routes construction to the selected factory and delegates type checks to the real session class; seam installation now uses the shim.
Shim behavior validation
tests/unittest/llmapi/test_session_reuse.py
Adds unit and integration checks for factory construction and isinstance/issubclass compatibility after patching the proxy seam.

Estimated code review effort: 3 (Moderate) | ~15–30 minutes

Possibly related PRs

Suggested reviewers: lori-ren

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title is clear, concise, and directly reflects the main change to make the session-reuse pool seam isinstance-transparent.
Description check ✅ Passed The description covers the issue, the solution, and testing, though it doesn't follow the template headings exactly.
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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
tests/test_common/session_reuse.py (1)

206-234: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add precise annotations to the new functions.

  • tests/test_common/session_reuse.py#L206-L234: annotate the shim factory and each metaclass hook.
  • tests/unittest/llmapi/test_session_reuse.py#L300-L317: add -> None.
  • tests/unittest/llmapi/test_session_reuse.py#L320-L329: annotate reuse_cache precisely and add -> None.

As per coding guidelines, “Annotate every function.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_common/session_reuse.py` around lines 206 - 234, The new shim
functions lack required annotations. In tests/test_common/session_reuse.py lines
206-234, annotate _isinstance_transparent_shim and each _SeamMeta hook with
precise parameter and return types; in
tests/unittest/llmapi/test_session_reuse.py lines 300-317, add -> None; and in
lines 320-329, precisely annotate reuse_cache and add -> None.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@tests/test_common/session_reuse.py`:
- Around line 206-234: The new shim functions lack required annotations. In
tests/test_common/session_reuse.py lines 206-234, annotate
_isinstance_transparent_shim and each _SeamMeta hook with precise parameter and
return types; in tests/unittest/llmapi/test_session_reuse.py lines 300-317, add
-> None; and in lines 320-329, precisely annotate reuse_cache and add -> None.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 7628505a-949c-46a0-9886-5b091a8fd000

📥 Commits

Reviewing files that changed from the base of the PR and between 1228fed and 0ba6d84.

📒 Files selected for processing (2)
  • tests/test_common/session_reuse.py
  • tests/unittest/llmapi/test_session_reuse.py

@sunnyqgg

Copy link
Copy Markdown
Collaborator Author

Closing: this change has been folded into #15908 (cherry-picked as-is, then generalized — the shim now lives in the shared _session_utils and is applied at BOTH the session-reuse and session-prefetch seams). Tracking continues there.

@sunnyqgg sunnyqgg closed this Jul 17, 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.

1 participant