feat: support vLLM 0.23 (platform/worker/attention API changes) - #18
feat: support vLLM 0.23 (platform/worker/attention API changes)#18maci0 wants to merge 2 commits into
Conversation
0.23 churned several internal APIs the plugin patches. Adapt to them: - platform: seed hook renamed seed_everything -> manual_seed_all (no-op for CPU, already seeded); implement num_compute_units (new in 0.23) - worker: CPU worker now inherits GPUWorker.load_model whose memory-pool context assumes a device allocator; return nullcontext for the Vulkan/CPU platform - attention: KV cache uses the HND layout (num_blocks, num_kv_heads, block_size, 2*head_size) via view+chunk instead of unbind; reshape_and_cache and cpu_attention_with_kv_cache gained k_scale/v_scale/kv_cache_dtype args
There was a problem hiding this comment.
Code Review
This pull request introduces compatibility shims and updates for vLLM 0.23, including platform and worker shims, and updates the attention mechanism to handle the new HND layout for the KV cache. However, a critical correctness bug was identified in how the key and value caches are split, which incorrectly interleaves K and V tokens instead of separating them. Additionally, the change to a 4D tensor causes a silent fallback to CPU execution because _get_or_create_vulkan_kv_cache expects a 5D tensor, effectively disabling Vulkan GPU acceleration. Confidence Score: 2/5 (Significant bugs, Needs rework)
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| # vLLM 0.23 HND layout: (num_blocks, num_kv_heads, block_size, 2*head_size) | ||
| _nb, _nkv, _bs, _ = kv_cache.size() | ||
| key_cache, value_cache = kv_cache.view((_nb, _nkv, _bs * 2, -1)).chunk(2, dim=2) |
There was a problem hiding this comment.
1. Incorrect Tensor Splitting (Correctness Bug)
The current implementation of splitting the key and value caches is mathematically incorrect:
key_cache, value_cache = kv_cache.view((_nb, _nkv, _bs * 2, -1)).chunk(2, dim=2)When kv_cache has the shape (num_blocks, num_kv_heads, block_size, 2 * head_size) with a contiguous memory layout, reshaping it to (_nb, _nkv, _bs * 2, head_size) interleaves the key and value tokens along the third dimension (e.g., row 0 is Token 0 K, row 1 is Token 0 V, row 2 is Token 1 K, row 3 is Token 1 V). Chunking this along dim=2 splits the tensor into the first half of the tokens (containing both K and V) and the second half of the tokens (containing both K and V), rather than separating K and V.
The correct and idiomatic PyTorch way to split the concatenated K and V dimensions is:
key_cache, value_cache = kv_cache.unflatten(-1, (2, -1)).unbind(-2)This correctly splits the last dimension of size 2 * head_size into (2, head_size) and unbinds them into separate key_cache and value_cache tensors of shape (num_blocks, num_kv_heads, block_size, head_size).
2. Broken Vulkan GPU Acceleration (Silent Fallback)
Because kv_cache is now a 4D tensor in vLLM 0.23, the helper function _get_or_create_vulkan_kv_cache (which expects a 5D tensor of shape [2, blocks, heads, block, dim]) will raise a ValueError. This exception is silently caught in _try_vulkan_decode and _try_write_tokens_to_vulkan_cache, causing the attention mechanism to silently fall back to CPU execution. As a result, Vulkan GPU acceleration is completely disabled, and the model runs entirely on the CPU (which is why the output was coherent but likely much slower than expected).
To fix this, please also update _get_or_create_vulkan_kv_cache to handle both 4D and 5D shapes:
shape = tuple(int(dim) for dim in kv_cache.shape)
...
if len(shape) == 4:
num_blocks, num_kv_heads, block_size, double_head_size = shape
head_size = double_head_size // 2
shape = (2, num_blocks, num_kv_heads, block_size, head_size)
elif len(shape) == 5 and shape[0] == 2:
_, num_blocks, num_kv_heads, block_size, head_size = shape
else:
raise ValueError(
f"expected KV cache shape [2, blocks, heads, block, dim] or [blocks, heads, block, 2*dim], got {shape}"
)Confidence Score
Score: 2/5 (Significant bugs, Needs rework)
The PR has a critical correctness bug in how it splits the key/value caches, and Vulkan GPU acceleration is completely disabled due to a shape mismatch that silently falls back to CPU execution.
| # vLLM 0.23 HND layout: (num_blocks, num_kv_heads, block_size, 2*head_size) | |
| _nb, _nkv, _bs, _ = kv_cache.size() | |
| key_cache, value_cache = kv_cache.view((_nb, _nkv, _bs * 2, -1)).chunk(2, dim=2) | |
| # vLLM 0.23 HND layout: (num_blocks, num_kv_heads, block_size, 2*head_size) | |
| key_cache, value_cache = kv_cache.unflatten(-1, (2, -1)).unbind(-2) |
|
Thanks for the review. On the two points: 1. K/V split — this mirrors vLLM 0.23's own reference, not a bug. The split is taken verbatim from vLLM's # vllm/v1/attention/backends/cpu_attn.py:342-346
# Make a view [num_blocks, num_kv_heads, block_size * 2, head_size]
kv_cache = kv_cache.view((num_blocks, num_kv_heads, block_size * 2, -1))
key_cache, value_cache = kv_cache.chunk(2, dim=2)The HND cache is written so this read separates K and V correctly (the write side matches this layout, not a per-token 2. Vulkan attention falling back to CPU — correct, and intentional for this PR. You're right that with the 4D HND cache, |
The hasattr guard never fired: vLLM's base Platform always defines manual_seed_all (it raises NotImplementedError), so hasattr was always True and the override was skipped, surfacing NotImplementedError from init_device. Assign it unconditionally. Verified on an RX 6900 XT (RADV NAVI21).
|
Thanks for contributing @maci0 just poking you so you know this one needs work, didn't pass the build |
ReviewThe direction (0.23 compat shims) is reasonable, but there's a correctness gap that I think blocks merging as-is. Blocking: breaks the currently-pinned vLLM version, unconditionally
Since Concretely, merging this today would break the plugin for every current user (still on 0.20.1) until a follow-up bumps Suggestion: either (a) branch on Also blocking (minor): lintSame Mergeability: 3/10 — good problem statement, but the attention.py change as written breaks the only vLLM version this repo currently supports, and CI can't catch it because the relevant tests require real Vulkan hardware to even be collected. |
Review (re-check against current main)Still merges cleanly, still red, and the blocking issue from my July review is unchanged. Build failure
Blocking: breaks the pinned vLLM version
Fix either way:
NoteBranch is ~2 months behind main (which has since gained the compute-engine foundation and shader library). Rebase to get a fresh CI run. |
Summary
Adapt the plugin to vLLM 0.23 (currently pinned to 0.20.1). 0.23 churned
several internal worker/platform/attention APIs the plugin patches; this makes
it load, serve, and generate correctly on 0.23. Verified end-to-end on a
discrete AMD GPU (RADV, gfx1100): coherent output ("The capital of France is
... Paris ...").
Changes (3 files, ~29 lines)
platform.py
seed_everything->manual_seed_all.CPU RNG is already seeded in
set_random_seed, so a no-op override is correct.profile_runqueriesPlatform.num_compute_units(new abstract method);implement it (returns CPU core count — Vulkan exposes CU via
VkPhysicalDeviceShaderCorePropertiesAMD, but_rsdoes not surface it yet).worker.py
GPUWorker.load_model, whose memory-poolcontext assumes a device allocator (cuda/xpu). The Vulkan/CPU platform has
none, so
_maybe_get_memory_pool_contextreturns anullcontext.attention.py
(num_blocks, num_kv_heads, block_size, 2*head_size). Replaceunbind(0)withthe 0.23
view(...).chunk(2, dim=2)split.cpu_attn_reshape_and_cacheandcpu_attention_with_kv_cachegainedk_scale/v_scale/kv_cache_dtypearguments in 0.23; pass them.Notes
Without these, 0.23 fails at:
_get_autobind_cpu_ids(removed — setVLLM_CPU_OMP_THREADS_BIND=nobindor it is unused on this path),manual_seed_allNotImplementedError, the sleep-mode allocator, then the KVunpack.
.vllm-versionis left at 0.20.1; bump separately when ready.