[TRTLLM-12714][feat] KVCacheManagerV2: wire PyExecutor rebalance hook (single GPU, aggregated for now) - #14578
Conversation
…-GPU agg) The V2 KVCacheManager.adjust() auto-tuner is implemented but had zero production callers. This patch wires a minimal between-iteration hook in PyExecutor._executor_loop that suspends every active request, calls adjust(), and resumes -- gated to single-GPU aggregated serving with no beam search, drafter, or disagg transceiver. The fast path costs one property read per iteration; adjust() itself is guarded by V2's existing 2000-sample + 120s cooldown gate. Pool VAs are preserved by the CUDA VMM-backed pool design, so captured CUDA graphs remain valid across resize. Resume failures stay suspended and the scheduler reactivates them through the existing prepare_context / try_allocate_generation path on the next iteration. Adds a public resume_request() on KVCacheManagerV2 as a thin wrapper around the existing _resume_and_restore helper. Out of scope (deferred): overlap scheduler, PP collective vote, disagg transceiver quiescence, draft V2 mirror, PEFT cleanup on suspend. Signed-off-by: Thor Johnsen <41591019+thorjohnsen@users.noreply.github.com>
…uler Adds the same between-iteration hook to PyExecutor._executor_loop_overlap that the prior commit added to _executor_loop, plus an always-drain step inside _maybe_rebalance_kv_pools so it works correctly in both loops. The overlap loop runs iter N+1's GPU work in parallel with CPU consumption of iter N's results, so at the top of any iteration previous_batch may hold an unconsumed in-flight iteration. Suspending its _KVCache instances mid-flight would race against running kernels. The new _consume_previous_batch_for_rebalance helper drains GPU work via torch.cuda.current_stream().synchronize() and processes previous_batch inline (mirroring the same _update_requests / _send_kv_async / _flush_pending_transfer_responses / _process_previous_batch sequence the loop performs). In the non-overlap loop previous_batch is always None, so the helper is a no-op there -- no special case. Cost: one pipeline-bubble per rebalance, fired at most once every 120s (the V2 auto-tuner's existing cooldown). Gates are unchanged from the prior commit; the drain mechanism is what makes overlap-mode safe, not narrower scope. Signed-off-by: Thor Johnsen <41591019+thorjohnsen@users.noreply.github.com>
…hook Adds ``KvCacheConfig.enable_kv_pool_rebalance`` (default True, status="prototype") as a kill switch for the PyExecutor rebalance hook introduced in the prior two commits. Plumbed through _util.py:create_py_executor_instance into PyExecutor's constructor and checked at the head of _can_pause_for_rebalance. When False, the hook is skipped entirely -- pool ratios remain at their warmup-derived values. Useful as an escape hatch for workloads where the suspend/adjust/resume cost or the 120s-cadence pipeline bubbles are undesirable, and for debugging regressions to isolate whether the hook is responsible. The flag only takes effect when use_kv_cache_manager_v2=True; the V1 manager has no rebalance code path. Signed-off-by: Thor Johnsen <41591019+thorjohnsen@users.noreply.github.com>
The rebalance hook is a beta feature -- ship it opt-in rather than opt-out. Users must now set ``kv_cache_config.enable_kv_pool_rebalance = True`` to invoke the V2 auto-tuner from PyExecutor; the prior default (True) silently activated it for everyone on V2. Docstring and ``_can_pause_for_rebalance`` comment updated to reflect opt-in semantics. Signed-off-by: Thor Johnsen <41591019+thorjohnsen@users.noreply.github.com>
…nce hook
Adds tests for the rebalance hook landed in the prior commits on this
branch.
* tests/unittest/_torch/executor/test_kv_pool_rebalance.py
-- 14 functional tests covering every short-circuit branch of
_can_pause_for_rebalance, the suspend/adjust/resume cycle of
_maybe_rebalance_kv_pools (including the kill-switch and the
"adjust failed but resume still runs" path), and the overlap-mode
drain helper _consume_previous_batch_for_rebalance. No real
engine: calls methods as unbound class lookups on a
MagicMock(spec=PyExecutor), following the existing test_py_executor
pattern. All pass locally.
* tests/integration/defs/accuracy/test_kv_pool_rebalance_accuracy.py
-- Greedy-decode token-exact comparison on Gemma-3-1B with explicit
VSWA (so we get >=2 pool groups and adjust() has real shrink work
to do). Inline _inject_pool_ratio_mismatch helper bypasses the
2000-sample / 120s cooldown gates and skews target ratios past
the 1.25x mismatch threshold. Parametrized on overlap mode.
Requires TLLM_WORKER_USE_SINGLE_PROCESS=1 so the test process can
reach the in-proc PyExecutor for injection.
The accuracy test currently fails: when the hook fires for real,
KVCacheManagerV2._storage_manager.shrink_pool_group asserts at L668 on
an overflow-slot count mismatch. This is a pre-existing bug in the V2
shrink path that has gone undetected because adjust() had zero
production callers before this branch. The hook plumbing itself is
correct (the unit tests prove the call chain).
Per the team's decision, the two accuracy parametrizations are added
to waives.txt as SKIP with a tag pointing at the V2 bug; the tests are
kept in-repo as a repro and unwaived when the bug is fixed.
Also wires the new tests into l0_a10 (unit) and l0_h100 (accuracy)
test lists.
Signed-off-by: Thor Johnsen <41591019+thorjohnsen@users.noreply.github.com>
…acy waives to NVBug Swaps the placeholder tag (kvcm-v2-shrink-pool-group-assert) on the two test_rebalance_matches_baseline parametrizations for the real https://nvbugs/6225866 URL, matching the rest of waives.txt's convention. Signed-off-by: Thor Johnsen <41591019+thorjohnsen@users.noreply.github.com>
… shrink The shrink path in `_storage_manager.shrink_pool_group` asserted that `len(_overflow_slots) == _num_active_slots - _target_capacity`, and `finish_shrink` gated finalization on the same equation. Both implicitly assumed _num_active_slots > _target_capacity. When the auto-tuner asks to shrink a pool whose new size is still above the high-water mark of slot IDs ever issued, the RHS goes negative and the assertion fires. `_num_active_slots` is a strict monotone high-water mark of issued slot IDs, so slot IDs in the to-be-removed range have never been handed out. There is genuinely no migration to do. Two changes: - `_storage/_core.py:finish_shrink` -- relax the gate to `max(0, _num_active_slots - _target_capacity)`, which collapses to `0 == 0` in the underused case. The body already handled this case correctly via `min(_num_active_slots, _capacity)`. - `_storage_manager.py:shrink_pool_group` -- add an early-return fast path when `_num_active_slots <= new_num_slots`: call `prepare_for_shrink` + `finish_shrink` + `resize_pools` and return, skipping the migration scan entirely. Added `TestSlotAllocatorShrink` with a direct repro of the NVBug condition and a sanity check that the non-trivial migration path still finalizes correctly. Signed-off-by: Thor Johnsen <41591019+thorjohnsen@users.noreply.github.com>
…curacy tests The shrink-path bug tracked in NVBug 6225866 is fixed in the previous commit. Both parametrizations of test_rebalance_matches_baseline pass locally on H100 with Gemma-3-1B + VSWA + injected ratio mismatch (token-exact match between rebalance=off and rebalance=on, no_overlap and overlap scheduler paths). Signed-off-by: Thor Johnsen <41591019+thorjohnsen@users.noreply.github.com>
…sertion Pre-commit's ruff-format hook prefers the trailing-message-as-call style for assert with a long expression. Signed-off-by: Thor Johnsen <41591019+thorjohnsen@users.noreply.github.com>
📝 WalkthroughWalkthroughThis PR implements automatic KV cache pool rebalance tuning in PyExecutor. A new ChangesKV Pool Rebalance Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 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.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/unittest/kv_cache_manager_v2_tests/test_kv_cache_manager_v2.py (1)
2423-2458: QA list update is unnecessary for this PR scope.These changes are unittest-only and the added coverage is aligned with the storage shrink regression scope, so no
tests/integration/test_lists/qa/*update is needed here.
As per coding guidelines: "If the PR only touches unittest/ or narrow unit scope, say explicitly whether QA list updates are unnecessary or optional."🤖 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 2423 - 2458, The PR needs an explicit statement that QA list updates are unnecessary because the change is limited to unit tests; update the PR description (or commit message) to say something like "QA list update unnecessary: changes are unittest-only (tests/unittest/kv_cache_manager_v2_tests::TestSlotAllocatorShrink -> test_shrink_underused_pool, test_shrink_touched_pool) and only add coverage for storage shrink regression," so reviewers can easily see this follows the coding guideline about unit-scope changes.tests/unittest/_torch/executor/test_kv_pool_rebalance.py (1)
180-192: ⚡ Quick winRemove unused
caplogparameter.The test function declares
caplogas a parameter but never uses it. Remove it to avoid confusion and keep the test signature clean.♻️ Proposed fix
- def test_adjust_failure_does_not_skip_resume(self, monkeypatch, caplog): + def test_adjust_failure_does_not_skip_resume(self, monkeypatch):🤖 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/_torch/executor/test_kv_pool_rebalance.py` around lines 180 - 192, The test test_adjust_failure_does_not_skip_resume currently includes an unused caplog parameter; remove caplog from the function signature so the test becomes def test_adjust_failure_does_not_skip_resume(self, monkeypatch): and update any callers or decorators if present; keep the rest of the body intact (references to _make_request, _make_executor, exe, monkeypatch, and assertions on exe.kv_cache_manager) and ensure imports/mocks are unchanged so PyExecutor._maybe_rebalance_kv_pools behavior is still validated.tensorrt_llm/llmapi/llm_args.py (1)
2681-2693: 💤 Low valueAlign description terminology with status field.
The description text says "Beta: enable at your own risk" but the
statusfield is set to"prototype". According to theField()docstring (line 90),"prototype"means "Not yet stable and subject to breaking changes; intended for experimentation only", while"beta"means "Recommended for use per the latest documentation".For consistency, consider changing "Beta:" to "Prototype:" in the description, or simply remove the "Beta:" prefix since the
status="prototype"already conveys the stability level.📝 Suggested fix to align terminology
enable_kv_pool_rebalance: bool = Field( default=False, status="prototype", description= "Opt in to the KVCacheManagerV2 auto-tuner (``adjust()``) for " "rebalancing pool-group ratios between iterations. When True the " "PyExecutor calls ``adjust()`` opportunistically; the auto-tuner " "itself remains gated by V2's internal 2000-sample / 120s cooldown. " "When False (default) the rebalance hook is skipped entirely and " - "pool ratios remain at their warmup-derived values. Beta: enable at " - "your own risk. Only used when using KV cache manager v2 " + "pool ratios remain at their warmup-derived values. Only used when " + "using KV cache manager v2 (experimental)." - "(experimental)." )🤖 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 `@tensorrt_llm/llmapi/llm_args.py` around lines 2681 - 2693, The description for the Field declaration of enable_kv_pool_rebalance is inconsistent with its status="prototype": update the text to match the status by replacing the "Beta:" prefix with "Prototype:" (or remove the stability prefix entirely) so the description aligns with the Field(status="prototype") metadata; locate the enable_kv_pool_rebalance Field in llm_args.py and edit its description string accordingly to reflect "Prototype:" (or omit the prefix) and keep the rest of the wording intact.
🤖 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/pyexecutor/py_executor.py`:
- Around line 2647-2668: The _can_pause_for_rebalance gate currently only
excludes pipeline parallelism; update it to enforce true single-rank execution
by adding a world-size check: in the _can_pause_for_rebalance method (and any
callers that assume single-rank semantics such as
_consume_previous_batch_for_rebalance), return False when self.dist.world_size
!= 1 (or > 1) so multi-rank TP/DP/CP runs cannot enter the rebalance path;
locate the function _can_pause_for_rebalance and add the explicit
self.dist.world_size check alongside the existing pp_size check.
- Around line 2721-2724: The current broad except around mgr.impl.adjust()
swallows all exceptions; narrow it to only catch the expected rebalance failure
by replacing the generic handler with an except OutOfPagesError as e: and call
logger.warning(...) there, allowing any other exceptions (ValueError,
AssertionError, etc.) to propagate and fail fast; reference the
mgr.impl.adjust() invocation and the logger.warning call when making this change
and ensure OutOfPagesError is imported/available in the module.
---
Nitpick comments:
In `@tensorrt_llm/llmapi/llm_args.py`:
- Around line 2681-2693: The description for the Field declaration of
enable_kv_pool_rebalance is inconsistent with its status="prototype": update the
text to match the status by replacing the "Beta:" prefix with "Prototype:" (or
remove the stability prefix entirely) so the description aligns with the
Field(status="prototype") metadata; locate the enable_kv_pool_rebalance Field in
llm_args.py and edit its description string accordingly to reflect "Prototype:"
(or omit the prefix) and keep the rest of the wording intact.
In `@tests/unittest/_torch/executor/test_kv_pool_rebalance.py`:
- Around line 180-192: The test test_adjust_failure_does_not_skip_resume
currently includes an unused caplog parameter; remove caplog from the function
signature so the test becomes def test_adjust_failure_does_not_skip_resume(self,
monkeypatch): and update any callers or decorators if present; keep the rest of
the body intact (references to _make_request, _make_executor, exe, monkeypatch,
and assertions on exe.kv_cache_manager) and ensure imports/mocks are unchanged
so PyExecutor._maybe_rebalance_kv_pools behavior is still validated.
In `@tests/unittest/kv_cache_manager_v2_tests/test_kv_cache_manager_v2.py`:
- Around line 2423-2458: The PR needs an explicit statement that QA list updates
are unnecessary because the change is limited to unit tests; update the PR
description (or commit message) to say something like "QA list update
unnecessary: changes are unittest-only
(tests/unittest/kv_cache_manager_v2_tests::TestSlotAllocatorShrink ->
test_shrink_underused_pool, test_shrink_touched_pool) and only add coverage for
storage shrink regression," so reviewers can easily see this follows the coding
guideline about unit-scope changes.
🪄 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: 6604d89d-6a85-4540-9c7d-0acc873e6078
📒 Files selected for processing (11)
tensorrt_llm/_torch/pyexecutor/_util.pytensorrt_llm/_torch/pyexecutor/py_executor.pytensorrt_llm/_torch/pyexecutor/resource_manager.pytensorrt_llm/llmapi/llm_args.pytensorrt_llm/runtime/kv_cache_manager_v2/_storage/_core.pytensorrt_llm/runtime/kv_cache_manager_v2/_storage_manager.pytests/integration/defs/accuracy/test_kv_pool_rebalance_accuracy.pytests/integration/test_lists/test-db/l0_a10.ymltests/integration/test_lists/test-db/l0_h100.ymltests/unittest/_torch/executor/test_kv_pool_rebalance.pytests/unittest/kv_cache_manager_v2_tests/test_kv_cache_manager_v2.py
|
/bot run --disable-fail-fast |
…sError The previous broad `except Exception` around `mgr.impl.adjust()` would swallow programmer errors (AssertionError, ValueError, etc.) and turn them into a warning log line, hiding real bugs. Only `OutOfPagesError` represents an expected runtime condition where the auto-tuner cannot fit the requested ratios; everything else should propagate and fail fast. Signed-off-by: Thor Johnsen <41591019+thorjohnsen@users.noreply.github.com>
…or-rebalance-hook Signed-off-by: Thor Johnsen <41591019+thorjohnsen@users.noreply.github.com> # Conflicts: # tensorrt_llm/_torch/pyexecutor/py_executor.py # tests/integration/test_lists/test-db/l0_a10.yml # tests/unittest/kv_cache_manager_v2_tests/test_kv_cache_manager_v2.py
Pre-commit picked up new docstring lint rules (D205, D301) and ruff-format style updates that landed on main since this PR opened. No behaviour change -- pure formatting / docstring reflow on three files this PR touched. Signed-off-by: Thor Johnsen <41591019+thorjohnsen@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #50823 [ run ] triggered by Bot. Commit: |
|
@lowsfer @yizhang-nv could you review this? Thanks |
|
/bot run --disable-fail-fast |
1 similar comment
|
/bot run --disable-fail-fast |
|
/bot run --disable-fail-fast |
|
PR_Github #51376 [ run ] triggered by Bot. Commit: |
|
PR_Github #51376 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #51444 [ run ] triggered by Bot. Commit: |
|
PR_Github #51444 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #51620 [ run ] triggered by Bot. Commit: |
|
PR_Github #51620 [ run ] completed with state |
Superjomn
left a comment
There was a problem hiding this comment.
LGTM on the llmapi changes.
|
@lowsfer @yizhang-nv to review this. thanks |
schetlur-nv
left a comment
There was a problem hiding this comment.
Approving for runtime devs
… (single GPU, aggregated for now) (NVIDIA#14578) Signed-off-by: Thor Johnsen <41591019+thorjohnsen@users.noreply.github.com> Signed-off-by: NVFB <186336021+NVFB@users.noreply.github.com>
… (single GPU, aggregated for now) (NVIDIA#14578) Signed-off-by: Thor Johnsen <41591019+thorjohnsen@users.noreply.github.com>
…-GPU agg)
The V2 KVCacheManager.adjust() auto-tuner is implemented but had zero production callers. This patch wires a minimal between-iteration hook in PyExecutor._executor_loop that suspends every active request, calls adjust(), and resumes -- gated to single-GPU aggregated serving with no beam search, drafter, or disagg transceiver.
The fast path costs one property read per iteration; adjust() itself is guarded by V2's existing 2000-sample + 120s cooldown gate. Pool VAs are preserved by the CUDA VMM-backed pool design, so captured CUDA graphs remain valid across resize. Resume failures stay suspended and the scheduler reactivates them through the existing prepare_context / try_allocate_generation path on the next iteration.
Adds a public resume_request() on KVCacheManagerV2 as a thin wrapper around the existing _resume_and_restore helper.
Out of scope (deferred): overlap scheduler, PP collective vote, disagg transceiver quiescence, draft V2 mirror, PEFT cleanup on suspend.
Summary by CodeRabbit
Release Notes
New Features
Performance
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.