Skip to content

[V1] Hybrid allocator without prefix caching #20661

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jul 13, 2025
+33 −0
Merged

[V1] Hybrid allocator without prefix caching #20661

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e5d6309
skip strict HybridKVCacheCoordinator verification if prefix caching i…
nopperl Jul 9, 2025
6323189
Revert "skip strict HybridKVCacheCoordinator verification if prefix c…
nopperl Jul 11, 2025
76e0c05
introduce kv cache coordinator for settings where prefix caching is n…
nopperl Jul 11, 2025
8d1aa20
cache number of single type managers
nopperl Jul 11, 2025
88254df
Merge branch 'main' into v1-allow-swa-mamba
nopperl Jul 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion vllm/v1/core/kv_cache_coordinator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,8 @@ def __init__(self, kv_cache_config: KVCacheConfig, max_model_len: int,
super().__init__(kv_cache_config, max_model_len, use_eagle,
enable_caching, caching_hash_fn,
enable_kv_cache_events)
self.verify_and_split_kv_cache_groups()
if enable_caching:
self.verify_and_split_kv_cache_groups()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This change correctly makes the setup for prefix caching conditional. However, it leads to a situation where a group of instance attributes (e.g., self.full_attention_group_ids, self.other_group_ids) are only defined if enable_caching is true.

This can make the class harder to maintain, as it may lead to AttributeError exceptions if other methods are added in the future that access these attributes without also checking self.enable_caching. While the new assertion in find_longest_cache_hit is a good safeguard for that specific method, the pattern of conditionally defining attributes can be fragile.

A more robust approach is to ensure all instance attributes are initialized in __init__, for example to None. This makes the object's structure consistent and improves readability and static analysis.

For example, you could add the following before this if statement:

self.full_attention_group_ids = None
self.other_group_ids = None

... and so on for all attributes set in verify_and_split_kv_cache_groups

Since this change would be outside the current diff, I'm leaving it as a suggestion for you to consider for improving future maintainability.


def verify_and_split_kv_cache_groups(self) -> None:
"""
Expand Down Expand Up @@ -307,6 +308,9 @@ def find_longest_cache_hit(
- A list of the cache hit blocks for each single type manager.
- The number of tokens of the longest cache hit.
"""
assert self.enable_caching, (
"find_longest_cache_hit can't be used if prefix caching is disabled"
)
# First, find the longest cache hit for full attention.
hit_blocks_full_attn = (
self.full_attention_manager_cls.find_longest_cache_hit(
Expand Down