[None][fix] Cherry-pick kv_cache_manager_v2 fixes to main - #14725
Conversation
…s in resize (NVIDIA#14531) Signed-off-by: Lance Liao <108499334+lancelly@users.noreply.github.com> (cherry picked from commit e4a5978) Signed-off-by: Yao Yao <lowsfer@users.noreply.github.com>
NVIDIA#14478) Signed-off-by: Iman Tabrizian <10105175+tabrizian@users.noreply.github.com> (cherry picked from commit 210a290) Signed-off-by: Yao Yao <lowsfer@users.noreply.github.com>
📝 WalkthroughWalkthroughKV cache improvements for precise SWA scratch allocation using range intersection and stale-block lifecycle cleanup, complemented by test infrastructure enabling delayed-commit decode validation across both immediate and deferred commit timing scenarios. ChangesKV Cache Core and Test Enhancements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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/kv_cache_manager_v2_tests/test_kv_cache_manager_v2.py (1)
583-600: ⚡ Quick winCoverage is still insufficient for the scratch-resize regression.
This matrix now exercises delayed-commit decode, but it does not pin the other
_kv_cache.pyfix from this cherry-pick: growing a scratch-enabled cache when the new scratch range overlaps blocks that were already allocated. Please add a focused regression intests/unittest/kv_cache_manager_v2_tests/test_kv_cache_manager_v2.pythat performs tworesize()calls under a tight quota and asserts the second grow succeeds without charging pages for the pre-existing scratch range. Coverage is insufficient fortensorrt_llm/runtime/kv_cache_manager_v2/_core/_kv_cache.py. As per coding guidelines:tests/**: Act as a QA engineer reviewing test changes and coverage for TensorRT-LLM. Keep feedback actionable: suggest concrete list file names and whether coverage is sufficient, insufficient, or needs follow-up outside the PR.🤖 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/kv_cache_manager_v2_tests/test_kv_cache_manager_v2.py` around lines 583 - 600, Add a focused regression test in tests/unittest/kv_cache_manager_v2_tests/test_kv_cache_manager_v2.py that creates a scratch-enabled cache, sets a tight quota, calls resize() to grow the cache into a scratch range, then calls resize() again where the new scratch range overlaps blocks already allocated and assert the second grow succeeds without charging pages for the pre-existing scratch range (use existing helpers like prepare() and run_naive() patterns or directly exercise the KvCacheV2.resize() behavior); target the runtime module tensorrt_llm/runtime/kv_cache_manager_v2/_core/_kv_cache.py to ensure the overlapping-scratch grow path is exercised; add one deterministic unit test (e.g., test_scratch_resize_regression) that performs two resize() calls and asserts quota/accounting unchanged for the already-allocated scratch pages. Files to change: tests/unittest/kv_cache_manager_v2_tests/test_kv_cache_manager_v2.py (add test) and ensure coverage for tensorrt_llm/runtime/kv_cache_manager_v2/_core/_kv_cache.py is sufficient (current coverage: insufficient, add the test) — no other changes required.
🤖 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/kv_cache_manager_v2_tests/test_kv_cache_manager_v2.py`:
- Around line 583-600: Add a focused regression test in
tests/unittest/kv_cache_manager_v2_tests/test_kv_cache_manager_v2.py that
creates a scratch-enabled cache, sets a tight quota, calls resize() to grow the
cache into a scratch range, then calls resize() again where the new scratch
range overlaps blocks already allocated and assert the second grow succeeds
without charging pages for the pre-existing scratch range (use existing helpers
like prepare() and run_naive() patterns or directly exercise the
KvCacheV2.resize() behavior); target the runtime module
tensorrt_llm/runtime/kv_cache_manager_v2/_core/_kv_cache.py to ensure the
overlapping-scratch grow path is exercised; add one deterministic unit test
(e.g., test_scratch_resize_regression) that performs two resize() calls and
asserts quota/accounting unchanged for the already-allocated scratch pages.
Files to change:
tests/unittest/kv_cache_manager_v2_tests/test_kv_cache_manager_v2.py (add test)
and ensure coverage for
tensorrt_llm/runtime/kv_cache_manager_v2/_core/_kv_cache.py is sufficient
(current coverage: insufficient, add the test) — no other changes required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 214a88bd-d246-4d3b-b9db-f118431af457
📒 Files selected for processing (2)
tensorrt_llm/runtime/kv_cache_manager_v2/_core/_kv_cache.pytests/unittest/kv_cache_manager_v2_tests/test_kv_cache_manager_v2.py
|
/bot run --disable-fail-fast --stage-list "*PerfSanity*" |
|
PR_Github #50965 [ run ] triggered by Bot. Commit: |
|
PR_Github #50965 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #51055 [ run ] triggered by Bot. Commit: |
|
PR_Github #51055 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #51177 [ run ] triggered by Bot. Commit: |
|
PR_Github #51177 [ run ] completed with state |
@coderabbitai summary
Description
Cherry-picks two merged
kv_cache_manager_v2fixes fromfeat/deepseek_v4tomain:#14531/e4a5978ca1c81fe304559b251b826350f21b4d7c: count only newly-added scratch blocks during resize.#14478/210a290405ef9f262c49b0c5589f6657e156b012: release stale-range holders after_commit_block.Test Coverage
Relevant coverage is in
tests/unittest/kv_cache_manager_v2_tests/test_kv_cache_manager_v2.py, including the delayed commit path added by#14478. CI will cover this PR.PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit 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.