Skip to content

[None][test] Fix Mamba hybrid transceiver helper#15323

Open
chienchunhung wants to merge 1 commit into
NVIDIA:mainfrom
chienchunhung:codex/fix-mamba-hybrid-cache-manager-tests
Open

[None][test] Fix Mamba hybrid transceiver helper#15323
chienchunhung wants to merge 1 commit into
NVIDIA:mainfrom
chienchunhung:codex/fix-mamba-hybrid-cache-manager-tests

Conversation

@chienchunhung

@chienchunhung chienchunhung commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Update the hybrid KV cache transceiver test helper to instantiate MixedMambaHybridCacheManager instead of the marker MambaHybridCacheManager base class.
  • Refresh the helper docstring/comment to match the concrete test path.

Root Cause

MambaHybridCacheManager was refactored into a marker base class used for isinstance checks and type hints. The unit-test helper still attempted to call it with constructor arguments, which raises TypeError: MambaHybridCacheManager() takes no arguments before the hybrid transceiver tests can run. The helper constructs a disaggregated mixed KV+Mamba manager directly, so the matching concrete class is MixedMambaHybridCacheManager.

Dependency

This PR is based directly on main and does not depend on PR #15181.

Validation

  • python3 -m py_compile tests/unittest/others/test_kv_cache_transceiver.py
  • Local pre-commit hooks passed during git commit -s using Python 3.12.

Summary by CodeRabbit

  • Tests
    • Updated KV-cache transceiver test infrastructure to use an improved hybrid cache manager for better test coverage and management of cache operations.

Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
@chienchunhung chienchunhung requested a review from NVShreyas June 12, 2026 22:28
@chienchunhung chienchunhung marked this pull request as ready for review June 12, 2026 22:28
@chienchunhung chienchunhung marked this pull request as draft June 12, 2026 22:30
@chienchunhung chienchunhung marked this pull request as ready for review June 12, 2026 22:32
@coderabbitai

coderabbitai Bot commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

This PR updates the KV-cache transceiver tests to use MixedMambaHybridCacheManager as the hybrid cache manager fixture instead of MambaHybridCacheManager. The change affects the factory function implementation, its docstring, related test comments, and import-adjacent documentation—all targeting the same cache manager type substitution.

Changes

Hybrid Cache Manager Fixture Update

Layer / File(s) Summary
Update hybrid cache manager fixture
tests/unittest/others/test_kv_cache_transceiver.py
create_hybrid_cache_manager factory function returns MixedMambaHybridCacheManager instead of MambaHybridCacheManager. Docstring, import-area comment, and test comment are updated to reflect the new manager type.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related PRs

  • NVIDIA/TensorRT-LLM#14841: Main PR that introduces MixedMambaHybridCacheManager and its constructor/behavior changes; these test updates align the test fixtures with that implementation.

Suggested reviewers

  • Wanli-Jiang
  • yechank-nvidia
🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive The PR description includes Summary, Root Cause, Dependency, and Validation sections that comprehensively explain the issue, solution, and testing. However, it does not follow the repository's required PR description template format with Description, Test Coverage, and PR Checklist sections. Restructure the description to follow the repository's template format with Description, Test Coverage, and PR Checklist sections, even if some items are marked as not applicable.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title '[None][test] Fix Mamba hybrid transceiver helper' clearly describes the main change: fixing the test helper for Mamba hybrid transceiver by updating it to use the correct cache manager class.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
tests/unittest/others/test_kv_cache_transceiver.py (1)

452-479: 🎯 Functional Correctness | 🔴 Critical

Fix invalid is_disagg kwarg passed to MixedMambaHybridCacheManager
tests/unittest/others/test_kv_cache_transceiver.py passes is_disagg=True, but tensorrt_llm/_torch/pyexecutor/mamba_cache_manager.py’s MixedMambaHybridCacheManager.__init__ has no is_disagg parameter, causing a TypeError: unexpected keyword argument 'is_disagg'.

🤖 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/unittest/others/test_kv_cache_transceiver.py` around lines 452 - 479,
The test passes an unsupported kwarg is_disagg=True to
MixedMambaHybridCacheManager which causes a TypeError; open
MixedMambaHybridCacheManager.__init__ and either remove the is_disagg argument
from the instantiation in test_kv_cache_transceiver.py or replace it with the
correct parameter name used in MixedMambaHybridCacheManager.__init__ (verify the
constructor signature in mamba_cache_manager and, if the intent is to enable
disaggregation, set the corresponding parameter such as enable_disagg/disagg
flag that the class actually accepts); ensure the call site (the
MixedMambaHybridCacheManager(...) block) matches the constructor parameters
(KvCacheConfig, kv_cache_type, num_layers, etc.).
🤖 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.

Outside diff comments:
In `@tests/unittest/others/test_kv_cache_transceiver.py`:
- Around line 452-479: The test passes an unsupported kwarg is_disagg=True to
MixedMambaHybridCacheManager which causes a TypeError; open
MixedMambaHybridCacheManager.__init__ and either remove the is_disagg argument
from the instantiation in test_kv_cache_transceiver.py or replace it with the
correct parameter name used in MixedMambaHybridCacheManager.__init__ (verify the
constructor signature in mamba_cache_manager and, if the intent is to enable
disaggregation, set the corresponding parameter such as enable_disagg/disagg
flag that the class actually accepts); ensure the call site (the
MixedMambaHybridCacheManager(...) block) matches the constructor parameters
(KvCacheConfig, kv_cache_type, num_layers, etc.).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: fdb4c575-9a18-415a-b502-3ceeb8bdee63

📥 Commits

Reviewing files that changed from the base of the PR and between 19ae053 and ec44151.

📒 Files selected for processing (1)
  • tests/unittest/others/test_kv_cache_transceiver.py

@chienchunhung

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #53964 [ run ] triggered by Bot. Commit: ec44151 Link to invocation

@chienchunhung chienchunhung enabled auto-merge (squash) June 12, 2026 23:33
@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #53964 [ run ] completed with state SUCCESS. Commit: ec44151
/LLM/main/L0_MergeRequest_PR pipeline #43055 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

Link to invocation

@chienchunhung

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #54142 [ run ] triggered by Bot. Commit: ec44151 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #54142 [ run ] completed with state FAILURE. Commit: ec44151
/LLM/main/L0_MergeRequest_PR pipeline #43225 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

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.

2 participants