[None][fix] Fix KV cache V2 OOM with separate draft KV cache (EAGLE3/MTP) - #12188
Conversation
📝 WalkthroughWalkthroughUpdates KV-cache management for draft models including budget splitting logic, explicit layer count propagation, and resource reset methods. Modifies KV-cache size calculation signatures to accept optional layer counts, affecting both Python executor utilities and resource manager classes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip You can customize the high-level summary generated by CodeRabbit.Configure the |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tensorrt_llm/_torch/pyexecutor/_util.py (1)
754-779:⚠️ Potential issue | 🟠 MajorDraft KV budget can remain unsplit while a draft manager is still created on non-last PP ranks.
At Line 754-779,
draft_kv_cache_configmay stayNone(whendraft_kv <= 0), but one-model draft manager creation still proceeds with the unsplit target config. Combined with the fallback non-empty PP layer behavior inget_pp_layers(tensorrt_llm/_torch/pyexecutor/resource_manager.py, Line 147-150), this can still reserve extra KV budget and reintroduce OOM pressure.💡 Proposed fix
@@ - draft_kv_cache_config = None - if (not estimating_kv_cache - and self._should_create_separate_draft_kv_cache() - and issubclass(self._kv_cache_manager_cls, KVCacheManagerV2)): + draft_kv_cache_config = None + should_host_one_model_draft_on_this_rank = ( + self._speculative_config.spec_dec_mode.is_external_drafter() + or self._mapping.is_last_pp_rank()) + if (not estimating_kv_cache + and self._should_create_separate_draft_kv_cache() + and issubclass(self._kv_cache_manager_cls, KVCacheManagerV2) + and should_host_one_model_draft_on_this_rank): draft_kv_cache_config = self._split_kv_cache_budget_for_draft() @@ - elif self._should_create_separate_draft_kv_cache(): - draft_kv_cache_manager = self._create_one_model_draft_kv_cache_manager( - estimating_kv_cache, - kv_cache_config_override=draft_kv_cache_config) + elif self._should_create_separate_draft_kv_cache(): + if (issubclass(self._kv_cache_manager_cls, KVCacheManagerV2) + and not should_host_one_model_draft_on_this_rank): + draft_kv_cache_manager = None + else: + draft_kv_cache_manager = self._create_one_model_draft_kv_cache_manager( + estimating_kv_cache, + kv_cache_config_override=draft_kv_cache_config)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tensorrt_llm/_torch/pyexecutor/_util.py` around lines 754 - 779, The code may create a one-model draft KV manager even when draft_kv_cache_config is None (e.g., draft_kv <= 0), causing unintentional KV reservations; update the branch that calls _create_one_model_draft_kv_cache_manager so it only creates draft_kv_cache_manager when draft_kv_cache_config is not None (or >0) — i.e., add an explicit guard checking draft_kv_cache_config before invoking _create_one_model_draft_kv_cache_manager (and similarly ensure _split_kv_cache_budget_for_draft returns None for no-split cases); reference symbols: draft_kv_cache_config, _split_kv_cache_budget_for_draft, _create_one_model_draft_kv_cache_manager, _create_kv_cache_manager, and consider interplay with get_pp_layers behavior to avoid reserving extra KV on non-last PP ranks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tensorrt_llm/_torch/pyexecutor/resource_manager.py`:
- Around line 2121-2122: The failure path after creating the draft KV cache
leaks the per-request draft resources because release_resources(req) does not
call draft_kv_cache_manager.free_resources(current_request); update the error
handling where kv_cache.resize(new_capacity) can fail (the block that calls
release_resources(req) and returns None) to explicitly call
draft_kv_cache_manager.free_resources(current_request) (or the equivalent
cleanup method) before calling release_resources(req) and returning, ensuring
draft_kv resources for current_request are freed even on resize failure.
---
Outside diff comments:
In `@tensorrt_llm/_torch/pyexecutor/_util.py`:
- Around line 754-779: The code may create a one-model draft KV manager even
when draft_kv_cache_config is None (e.g., draft_kv <= 0), causing unintentional
KV reservations; update the branch that calls
_create_one_model_draft_kv_cache_manager so it only creates
draft_kv_cache_manager when draft_kv_cache_config is not None (or >0) — i.e.,
add an explicit guard checking draft_kv_cache_config before invoking
_create_one_model_draft_kv_cache_manager (and similarly ensure
_split_kv_cache_budget_for_draft returns None for no-split cases); reference
symbols: draft_kv_cache_config, _split_kv_cache_budget_for_draft,
_create_one_model_draft_kv_cache_manager, _create_kv_cache_manager, and consider
interplay with get_pp_layers behavior to avoid reserving extra KV on non-last PP
ranks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5ed31748-802c-44d3-953b-174d66262207
📒 Files selected for processing (3)
cpp/tensorrt_llm/batch_manager/kvCacheManagerV2Utils.cutensorrt_llm/_torch/pyexecutor/_util.pytensorrt_llm/_torch/pyexecutor/resource_manager.py
…MTP) Fix OOM failures when KV cache manager V2 is used with one-model speculative decoding (EAGLE3/MTP) that requires a separate draft KV cache. Changes: - Split max_gpu_total_bytes budget proportionally between target and draft KV cache managers to prevent double-counting the total budget. - Fix get_cache_size_per_token to accept explicit num_layers for draft models where HF config layer count differs from runtime (EAGLE3). - Handle PP correctly: draft KV size only counted on last PP rank. - Pass sparse_attention_config to draft KV cache manager for MoE models (DeepSeek V3 with DSA). - Fix BAD_PAGE_INDEX handling in copyBatchBlockOffsetsToDeviceKernel: use 0 instead of BAD_PAGE_INDEX to avoid out-of-bounds memory access. - Gracefully handle resize failures in dummy request preparation instead of raising ValueError (returns None to let scheduler retry). Signed-off-by: Yi Zhang <187001205+yizhang-nv@users.noreply.github.com>
3e982f1 to
b945ae1
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #38972 [ run ] triggered by Bot. Commit: |
|
PR_Github #38972 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #39006 [ run ] triggered by Bot. Commit: |
|
PR_Github #39006 [ run ] completed with state |
…MTP) (NVIDIA#12188) Signed-off-by: Yi Zhang <187001205+yizhang-nv@users.noreply.github.com>
…MTP) (NVIDIA#12188) Signed-off-by: Yi Zhang <187001205+yizhang-nv@users.noreply.github.com>
…MTP) (NVIDIA#12188) Signed-off-by: Yi Zhang <187001205+yizhang-nv@users.noreply.github.com>
@coderabbitai summary
Description
Fix OOM failures when KV cache manager V2 is used with one-model speculative decoding (EAGLE3/MTP) that requires a separate draft KV cache.
Root cause: When V2 creates both a target and a draft KV cache manager, both managers independently claim the full
max_gpu_total_bytesbudget, leading to OOM duringprepare_resources.Changes:
_util.py): Splitmax_gpu_total_bytesproportionally between target and draft KV cache managers based on per-token KV cache sizes. Only applies to final manager creation (not estimation)._util.py,resource_manager.py): Addnum_layersparameter toget_cache_size_per_tokenfor draft models where the HF config layer count differs from runtime (e.g. EAGLE3: config says 1 layer, runtime uses 4). Preventspp_layers()from giving wrong results for draft layers._util.py): Only count draft KV cache cost on the last PP rank where EAGLE3/MTP draft layers reside._util.py): Passsparse_attention_configfromeffective_draft_configto draft KV cache manager for MoE models (e.g. DeepSeek V3 with DSA).kvCacheManagerV2Utils.cu): Use 0 instead ofBAD_PAGE_INDEXfor padding incopyBatchBlockOffsetsToDeviceKernelto avoid out-of-bounds memory access in attention kernels.resource_manager.py): Handle resize failures in dummy request preparation by returning None instead of raising ValueError, letting the scheduler retry.Test Coverage
TBD — V2 is not yet enabled by default; will be covered by the enable-v2-by-default PR.
PR Checklist