[None][feat] Reserve one more slots for attention_dp in mixed mamba cache manager - #14853
Conversation
📝 WalkthroughWalkthroughThis PR introduces a dedicated constant ChangesAttention-DP dummy slot management
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unittest/disaggregated/test_mamba_transfer.py (1)
350-357: 💤 Low valueOptional: assert reserved-slot stability after
free_resourcesto match the unit test.This test verifies dummy routing but, unlike
test_attention_dp_dummy_has_reserved_slot_with_batch_size_oneintest_mamba_cache_manager.py, it doesn't confirm the reserved slot stays out of the free pool after the dummy is freed — the exact lifecycle the disagg + ADP +max_batch_size=1fix targets.💚 Optional lifecycle assertion
assert mgr._impl.mamba_cache_index[100] != mgr._impl._attention_dp_dummy_slot + + mgr._impl.free_resources(SimpleNamespace(py_request_id=ATTENTION_DP_DUMMY_REQUEST_ID)) + assert mgr._impl._attention_dp_dummy_slot not in mgr._impl.mamba_cache_free_blocks finally: mgr.shutdown()This also requires importing
SimpleNamespace(from types import SimpleNamespace).🤖 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/disaggregated/test_mamba_transfer.py` around lines 350 - 357, After adding the dummy request, also verify the reserved dummy slot remains excluded from the free pool after freeing the dummy: call mgr.free_resources([ATTENTION_DP_DUMMY_REQUEST_ID]) (or the appropriate free method on mgr), then assert that mgr._impl.mamba_cache_free_blocks does not contain mgr._impl._attention_dp_dummy_slot and that mgr._impl.mamba_cache_index either no longer maps the request id or still maps the id to the reserved slot as per the other test; also add "from types import SimpleNamespace" to imports if needed for parity with test_mamba_cache_manager.py.
🤖 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/unittest/disaggregated/test_mamba_transfer.py`:
- Around line 350-357: After adding the dummy request, also verify the reserved
dummy slot remains excluded from the free pool after freeing the dummy: call
mgr.free_resources([ATTENTION_DP_DUMMY_REQUEST_ID]) (or the appropriate free
method on mgr), then assert that mgr._impl.mamba_cache_free_blocks does not
contain mgr._impl._attention_dp_dummy_slot and that mgr._impl.mamba_cache_index
either no longer maps the request id or still maps the id to the reserved slot
as per the other test; also add "from types import SimpleNamespace" to imports
if needed for parity with test_mamba_cache_manager.py.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f43be582-9614-457c-8576-0a3c6e0af29d
📒 Files selected for processing (5)
tensorrt_llm/_torch/pyexecutor/llm_request.pytensorrt_llm/_torch/pyexecutor/mamba_cache_manager.pytensorrt_llm/_torch/pyexecutor/py_executor.pytests/unittest/_torch/executor/test_mamba_cache_manager.pytests/unittest/disaggregated/test_mamba_transfer.py
…ache manager * Will fix `out-of-mamba-block` error with ADP + max_batch_size=1 + disagg scenario. Signed-off-by: Wanli Jiang <35160485+Wanli-Jiang@users.noreply.github.com>
e8da248 to
d2e0805
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #51542 [ run ] triggered by Bot. Commit: |
|
PR_Github #51542 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #51690 [ run ] triggered by Bot. Commit: |
|
PR_Github #51690 [ run ] completed with state |
Feature
out-of-mamba-blockerror with ADP + max_batch_size=1 + disagg scenario.Summary by CodeRabbit
New Features
Tests
Description
Test Coverage
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.