[https://nvbugs/6417488][fix] Rename M3 override to _prepare_page_table_tensor(index_mapper_capacity) and…#15984
[https://nvbugs/6417488][fix] Rename M3 override to _prepare_page_table_tensor(index_mapper_capacity) and…#15984trtllm-agent wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughThe MiniMax-M3 sparse KV cache manager now prepares page-table tensors from ChangesMiniMax-M3 Page Table Preparation
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Running upstream PR NVIDIA#15984's variant (which keeps the base V-K stride derivation for kv_offset) against MiniMax-M3 at TP4 showed the derivation does NOT assert on coalesced pools — our comment's claimed justification was wrong. The real reason for pinning zero: no M3 consumer reads the value, and zero keeps init independent of the base's stride assumptions. Signed-off-by: Zheyu Fu <zheyuf@NVIDIA.com>
2f0c920 to
b9d6414
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #58453 [ run ] triggered by Bot. Commit: |
|
My local try shows this change does not solve all the problem. MiniMaxM3KVCacheManagerV2._get_batch_cache_indices_by_pool_id() |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@tensorrt_llm/_torch/attention_backend/sparse/minimax_m3/cache_manager.py`:
- Around line 435-443: The MinimaxM3 cache manager override is bypassing the
base SWA scratch-copy setup, so the scratch-reuse tensors are never initialized
when enable_swa_scratch_reuse is enabled. In CacheManager’s override where
host_kv_cache_block_offsets is allocated, make sure to preserve the base hook
behavior by calling _prepare_swa_scratch_copy_tensors(index_mapper_capacity) on
the enabled path, or otherwise delegate to the base initialization before
returning, so later scratch-reuse code in CacheManager and related SWA paths can
find the expected tensors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9dce3103-99a0-4840-98a3-6ed2447e3d1a
📒 Files selected for processing (1)
tensorrt_llm/_torch/attention_backend/sparse/minimax_m3/cache_manager.py
|
/bot run --disable-fail-fast |
|
PR_Github #58461 [ run ] triggered by Bot. Commit: |
|
PR_Github #58453 [ run ] completed with state |
|
PR_Github #58461 [ run ] completed with state
|
|
Waiting #16218 for a more proper fix. |
59133cb to
339132d
Compare
4803c16 to
70034ff
Compare
Signed-off-by: Jin Li <59594262+liji-nv@users.noreply.github.com>
70034ff to
89ffcfd
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #59844 [ run ] triggered by Bot. Commit: |
|
PR_Github #59844 [ run ] completed with state
|
BowenFu
left a comment
There was a problem hiding this comment.
LGTM — the M3-specific override rename fixes a silently-dead hook (exact_div assert); the base kv_cache_manager_v2 change swaps try/except-KeyError for an all_buffer_ids membership check that is behavior-preserving — default non-sparse models still return None. Covered by a new unit test.
|
[by Codex] @yizhang-nv Could you please review this PR? Thank you! |
Signed-off-by: Jin Li <59594262+liji-nv@users.noreply.github.com>
Signed-off-by: Jin Li <59594262+liji-nv@users.noreply.github.com>
Signed-off-by: Jin Li <59594262+liji-nv@users.noreply.github.com>
89ffcfd to
27dfb12
Compare
Summary
Test plan
Links
Summary by CodeRabbit