[None][test] Cover KVCacheManagerV2 C++ pool rebalance path - #17387
Conversation
The C++ KVCacheManagerV2 backend is now the default (TLLM_KV_CACHE_MANAGER_V2_BACKEND=cpp), but the rebalance path had no test that reached it: - TestSlotAllocatorShrink, the NVBug 6225866 regression test, imports SlotAllocator from kv_cache_manager_v2._storage._core, i.e. the Python class, so it never exercised the C++ implementation. - The C++ V2 gtests covered only HostMem / Stats / TypedIndex. - tests/unittest/_torch/executor/test_kv_pool_rebalance.py mocks the PyExecutor hook and constructs no real manager. So the NVBug 6225866 fix shipped unguarded in the default code path. Add two pieces of coverage: 1. kvCacheManagerV2SlotAllocatorTest - ports both cases of the Python TestSlotAllocatorShrink to the C++ SlotAllocator. ShrinkUnderusedPool is the regression case: without the max(0, numActiveSlots - targetCapacity) guard in SlotAllocator::finishShrink the expected-overflow count goes negative and finishShrink throws. ShrinkTouchedPool covers the ordinary migration path. Neither needs a CUDA context. 2. TestPoolRebalance - backend-agnostic coverage of need_adjustment -> adjust() -> adjust_cache_level -> shrink/expand_pool_group, driven through the manager so it runs on whichever backend is selected, and needing no model weights. The tests assert that pool group 0's slot total grows while group 1's shrinks, which is what proves both the expand and the shrink path ran, and that KV committed before the resize still verifies through block reuse afterwards. Note that pool groups are formed per distinct slot layout, not per sliding window size, so the test config uses two attention layers with different buffer sizes to obtain two pool groups. The new gtest was mutation-tested: reverting the max(0, ...) guard makes ShrinkUnderusedPool fail with "cannot finish shrink yet" while ShrinkTouchedPool stays green. Signed-off-by: Thor Johnsen <41591019+thorjohnsen@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdded C++ unit tests for ChangesKV cache pool rebalancing tests
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/bot run --disable-fail-fast |
|
PR_Github #64430 [ run ] triggered by Bot. Commit: |
|
PR_Github #64430 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #64608 [ run ] triggered by Bot. Commit: |
|
PR_Github #64608 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #64648 [ run ] triggered by Bot. Commit: |
|
PR_Github #64648 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #65042 [ run ] triggered by Bot. Commit: |
|
/bot kill |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
PR_Github #65059 [ kill ] triggered by Bot. Commit: |
|
PR_Github #65042 [ run ] completed with state |
|
PR_Github #65059 [ kill ] completed with state |
|
/bot run --disable-fail-fast |
|
PR_Github #65061 [ run ] triggered by Bot. Commit: |
|
PR_Github #65061 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #65126 [ run ] triggered by Bot. Commit: |
|
PR_Github #65126 [ run ] completed with state |
|
@lowsfer @nvpohanh @lori-ren @allisonlim-nv — could you take a look when you get a chance? Need one approval from |
Signed-off-by: Thor Johnsen <41591019+thorjohnsen@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #65342 [ ] completed with state |
|
/bot run --disable-fail-fast |
|
PR_Github #65347 [ run ] triggered by Bot. Commit: |
|
PR_Github #65347 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #65425 [ run ] triggered by Bot. Commit: |
|
PR_Github #65425 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #65458 [ run ] triggered by Bot. Commit: |
|
PR_Github #65458 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #65481 [ run ] triggered by Bot. Commit: |
|
PR_Github #65481 [ run ] completed with state |
Description
The C++
KVCacheManagerV2backend is now the default (TLLM_KV_CACHE_MANAGER_V2_BACKEND, defaultcpp), but the pool-rebalance path had no test that actually reached it:TestSlotAllocatorShrink— the NVBug 6225866 regression test — importsSlotAllocatorfromkv_cache_manager_v2._storage._core, i.e. the Python class, so it never exercised the C++ implementation.tests/unittest/_torch/executor/test_kv_pool_rebalance.pymocks the PyExecutor hook and constructs no real manager.Net effect: the NVBug 6225866 fix was shipping unguarded in the default code path. This PR adds test coverage only — no production code is modified.
1.
kvCacheManagerV2SlotAllocatorTest(new gtest)Ports both cases of the Python
TestSlotAllocatorShrinkto the C++SlotAllocator.ShrinkUnderusedPoolis the regression case. Without themax(0, numActiveSlots - targetCapacity)guard inSlotAllocator::finishShrink, the expected-overflow count goes negative, never matchesmOverflowSlots.size(), andfinishShrink()throws"cannot finish shrink yet".ShrinkTouchedPoolcovers the ordinary migration path.Neither needs a CUDA context, so this runs in the normal C++ unit-test stage.
2.
TestPoolRebalance(new Python test class)Backend-agnostic coverage of
need_adjustment→adjust()→adjust_cache_level→shrink/expand_pool_group, driven through the manager so it runs on whichever backend is selected, and requiring no model weights.The assertions are what keep it non-vacuous: it checks that pool group 0's slot total grows while group 1's shrinks, proving both the expand and the shrink path ran, and that KV committed before the resize still verifies through block reuse afterwards.
Test Coverage
New tests:
cpp/tests/unit_tests/batch_manager/kvCacheManagerV2SlotAllocatorTest.cpp—ShrinkUnderusedPool,ShrinkTouchedPooltests/unittest/kv_cache_manager_v2_tests/test_kv_cache_manager_v2.py::TestPoolRebalance—test_adjust_resizes_pool_groups,test_kv_survives_adjustValidated on 8×H100 (sm90):
kvCacheManagerV2SlotAllocatorTest(2)TestPoolRebalance(2)TestPoolRebalance(2)kv_cache_manager_v2_tests(127)OK (skipped=13)kv_cache_manager_v2_tests(127)OK (skipped=12)tests/unittest/_torch/executor/test_kv_pool_rebalance.py(15)accuracy/test_kv_pool_rebalance_accuracy.py(2)The 13-vs-12 skip differential is accounted for:
test_planned_drop_handle_rejects_partial_coveragecarries@requires_python_backendbecause forcing the state under test needs a direct write into a pure-Python page object. It is unrelated to rebalancing.Mutation-tested. A passing test proves nothing unless it fails when the bug returns, so the fix was temporarily reverted in
storage/core.cpp:Result — exactly the predicted failure, and only the regression case failed:
The revert was undone and rebuilt; both gtests pass again.
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.🤖 Generated with Claude Code
Dev Engineer Review
kvCacheManagerV2SlotAllocatorTestCMake target.QA Engineer Review
Added tests:
ShrinkUnderusedPoolShrinkTouchedPoolTestPoolRebalance.test_adjust_resizes_pool_groupsTestPoolRebalance.test_kv_survives_adjustSupporting methods added:
prepare_two_pool_groups_run_sequenceNo matching entries were found in
tests/integration/test_lists/,test-db/, orqa/. The added tests are not explicitly listed for CI or manual QA coverage.Verdict: needs follow-up.