fix(kvcache): decode lora_id/medium/lora_name/group_idx in BlockStored (#2285) - #2384
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request updates the KV-cache msgpack decoder to correctly decode newer optional BlockStored fields emitted by vLLM (notably lora_id, medium, lora_name, and group_idx) while remaining backward compatible with older, shorter event arrays.
Changes:
- Extend
BlockStoreddecoding to conditionally parse optional fields by positional index with bounds checks. - Add
toStringPtrhelper to decode nullable string fields that may arrive asstringor[]byte. - Add targeted decoder tests that marshal raw positional arrays to validate the wire contract and backward compatibility.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/cache/kvcache/msgpack_decoder.go | Decodes optional BlockStored fields (positions 5–7, 9) and adds toStringPtr for nullable string decoding. |
| pkg/cache/kvcache/msgpack_decoder_test.go | Adds raw-wire-format tests covering full/partial/omitted optional fields and extra_keys positioning behavior. |
| pkg/cache/kvcache/event_types.go | Updates BlockStoredEvent documentation and struct to expose decoded optional fields to downstream consumers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
vllm-project#2285) Signed-off-by: pjdurden <prajjwalchittori1@gmail.com>
e858c36 to
1430d3e
Compare
There was a problem hiding this comment.
Code Review
This pull request updates the BlockStoredEvent struct and its msgpack decoder to support new optional fields (LoraID, Medium, LoraName, and GroupIdx) introduced in newer vLLM builds. The decoder reads these fields by position with bounds checks to maintain backward compatibility with older vLLM builds that send shorter arrays. Additionally, comprehensive unit tests have been added to verify the decoding behavior. There are no review comments, so I have no feedback to provide.
Review Notes & SuggestionsThanks for the PR! Here are a few observations and minor suggestions. Most of these are non-blockers or notes for the follow-up PR, but they are worth a quick review. Actionable Suggestions
Observations & Notes for PR2
|
|
Thanks for the review.
|
Signed-off-by: pjdurden <prajjwalchittori1@gmail.com>
d72baba to
c2101e4
Compare
Problem
The ZMQ decoder reads only the first four positional fields of vLLM's
BlockStored(block_hashes, parent_block_hash, token_ids, block_size) and drops the rest. Newer vLLM (vllm/distributed/kv_events.py) also carrieslora_id,medium,lora_name,extra_keys, andgroup_idx. Droppinggroup_idx/medium/lora_nameis what causes the false prefix matches described in #2285:group_idx: hybrid-attention models (sliding-window + full layers) hash the same token range into different KV-cache groups. Collapsing all groups into one hash space produces false prefix matches.medium: with KV offloading, blocks can live on CPU/remote tiers, so counting every block as a GPU hit over-credits pods that only hold the prefix on a slower tier.lora_name: canonical adapter id (lora_idis deprecated upstream), needed to isolate adapters in the index.This PR (1 of 2)
Decode the dropped fields and expose them on
BlockStoredEvent. No routing/keying behavior change yet, this just stops dropping the data.BlockStoredis a msgspecarray_likestruct withomit_defaults, so trailing fields equal to their default may be absent from the wire array. The decoder therefore reads positions 5-9 by index with bounds checks, and tolerates shorter arrays from older vLLM builds:extra_keys(position 8) is left for the follow-up that consumes it for block-hash reconstruction;group_idxis still read at its fixed position 9.Follow-up (PR2)
Key prefix entries by
(model, lora_name, group_idx)and score non-GPU mediums lower, plumbing the fields through the kvevent handler into the indexer. That touches theMatchPrefix/AddPrefixsignature across the indexers, so it is kept separate per the discussion on #2285.Tests
New decoder tests build raw vLLM-layout msgpack arrays (full field set, partial length, nil/None values, backward-compat short array, and group_idx read past a non-nil extra_keys). Verified locally:
Part 1 of 2 for #2285.