fix(sidecar): rename P2P kv_transfer_params sub-dict keys to match vLLM#2140
Conversation
The OffloadingConnector P2P branch renamed the request-level sub-dict keys for clarity (liranschour/vllm@530fa0d7 + @5bcf5b31): "prefill" -> "remote_prefiller", "decode" -> "remote_decoder", "p2p" -> "remote_kv_source", each named for the remote party it describes. Inner fields (kv_request_id, remote_host, remote_port) and the secondary-tier type "p2p" are unchanged. A sidecar emitting the old keys against a renamed engine is silently inert - the engine ignores unknown sub-dicts and every request recomputes - so the sidecar and engine must move together. Signed-off-by: nilig <nili.ifergan@gmail.com>
| | `offloading` | `OffloadingConnector` | KV transfer over the vLLM CPU offloading tier. The decoder pulls KV from the prefiller via the `p2p` secondary tier. | | ||
|
|
||
| With `offloading`, the sidecar dispatches prefill and decode concurrently. It injects role-keyed `kv_transfer_params`: the prefiller receives `{"decode": {"kv_request_id": <id>}}` (no peer address), and the decoder receives `{"prefill": {"kv_request_id": <id>, "remote_host": <prefiller host>, "remote_port": <p2p-connector-port>}}` so it can pull KV from the prefiller. The prefiller host comes from the `x-prefiller-host-port` header; the port is `--p2p-connector-port`. | ||
| With `offloading`, the sidecar dispatches prefill and decode concurrently. It injects role-keyed `kv_transfer_params`, each key named for the remote party it describes: the prefiller receives `{"remote_decoder": {"kv_request_id": <id>}}` (no peer address), and the decoder receives `{"remote_prefiller": {"kv_request_id": <id>, "remote_host": <prefiller host>, "remote_port": <p2p-connector-port>}}` so it can pull KV from the prefiller. The prefiller host comes from the `x-prefiller-host-port` header; the port is `--p2p-connector-port`. |
There was a problem hiding this comment.
While new lines in paragraphs as this one in Markdown files are meaningless, they make it much easier to review changes.
Please break this up into separate lines in the file.
| With `offloading`, the sidecar dispatches prefill and decode concurrently. It injects role-keyed `kv_transfer_params`, each key named for the remote party it describes: the prefiller receives `{"remote_decoder": {"kv_request_id": <id>}}` (no peer address), and the decoder receives `{"remote_prefiller": {"kv_request_id": <id>, "remote_host": <prefiller host>, "remote_port": <p2p-connector-port>}}` so it can pull KV from the prefiller. The prefiller host comes from the `x-prefiller-host-port` header; the port is `--p2p-connector-port`. | ||
|
|
||
| When the request also carries the `x-kv-cache-source-host-port` header (set by the EPP `p2p-source-producer` to a peer holding more cached prefix than the pod computing the prefix), the sidecar injects an additional `p2p` key so vLLM pulls that cached prefix over the P2P tier instead of recomputing it. Under disaggregation the prefiller leg carries `{"decode": {...}, "p2p": {"kv_request_id": <own id>, "remote_host": <source host>, "remote_port": <p2p-connector-port>}}` (the only supported multi-key combination); without a prefiller the decoder-only request carries `{"p2p": {...}}` alone. A malformed or disallowed source header is ignored and the request proceeds unchanged, as is any source header on a connector that cannot pull over the P2P tier: only `offloading`, or NIXLv2 with `--enable-p2p-pull`, honors it. For the pulled blocks to be servable, the source pod must offload its generated (decode-phase) KV: set `offload_prompt_only: false` in its `kv_connector_extra_config` (the default `true` offloads only prefill blocks). | ||
| When the request also carries the `x-kv-cache-source-host-port` header (set by the EPP `p2p-source-producer` to a peer holding more cached prefix than the pod computing the prefix), the sidecar injects an additional `remote_kv_source` key so vLLM pulls that cached prefix over the P2P tier instead of recomputing it. Under disaggregation the prefiller leg carries `{"remote_decoder": {...}, "remote_kv_source": {"kv_request_id": <own id>, "remote_host": <source host>, "remote_port": <p2p-connector-port>}}` (the only supported multi-key combination); without a prefiller the decoder-only request carries `{"remote_kv_source": {...}}` alone. A malformed or disallowed source header is ignored and the request proceeds unchanged, as is any source header on a connector that cannot pull over the P2P tier: only `offloading`, or NIXLv2 with `--enable-p2p-pull`, honors it. For the pulled blocks to be servable, the source pod must offload its generated (decode-phase) KV: set `offload_prompt_only: false` in its `kv_connector_extra_config` (the default `true` offloads only prefill blocks). |
|
@nilig While the vLLM PR that changed the KV related JSON fileds has been merged. Is there is a version of vLLM available with the PR? Having users needing to build vLLM from source is not a good idea... |
Review feedback on llm-d#2140: the two protocol paragraphs were single long lines; wrapped for reviewable diffs. No content change. Signed-off-by: nilig <nili.ifergan@gmail.com>
|
The vLLM side is not merged yet - the rename lives in vllm-project/vllm#48021, which is approved (orozery) and carries the Sequencing-wise I'd merge this PR together with or after the vLLM merge. Until then the guide pins engine and sidecar as a matched pair, and mixing old/new sides degrades to recompute rather than failing, so there is no crash window - just the silently-inert pairing this PR exists to prevent. |
There was a problem hiding this comment.
Pull request overview
This PR updates the sidecar’s OffloadingConnector P2P kv_transfer_params sub-dict key names to match the current vLLM OffloadingConnector branch, ensuring cached-prefix pulls are actually recognized by the engine instead of being silently ignored.
Changes:
- Renamed
kv_transfer_paramsnesting keys emitted by the sidecar toremote_decoder,remote_prefiller, andremote_kv_source(via the existingrequestFieldP2P*constants). - Updated docs and release note text to reflect the new wire keys.
- Updated unit-test comments/assertion expectations that reference the renamed keys.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| release-notes.d/unreleased/1937.md | Updates release note wording to the new kv_transfer_params.remote_kv_source key. |
| pkg/sidecar/proxy/proxy.go | Renames the P2P-related kv_transfer_params field constants to the new vLLM key names. |
| pkg/sidecar/proxy/connector_p2p.go | Updates inline documentation around the injected kv_transfer_params block names. |
| pkg/sidecar/proxy/connector_p2p_test.go | Updates test expectations/comments to reference remote_decoder / remote_prefiller. |
| docs/disaggregation.md | Updates disaggregation documentation examples to use the renamed keys. |
| // p2pSourceParams builds the kv_transfer_params.remote_kv_source block for a | ||
| // pull from sourceHostPort's OffloadingConnector P2P tier. The kv_request_id is its | ||
| // own fresh UUID: in P2P mode it is consumer-side only. |
The kv_transfer_params nesting key is remote_kv_source; comments and test descriptions that still call it the p2p key or p2p params read as if the secondary-tier type (p2p, unchanged) were the request-field key. Signed-off-by: nilig <nili.ifergan@gmail.com>
| } | ||
|
|
||
| // addP2PPullToPrefill adds the OffloadingConnector p2p pull block to a prefill | ||
| // addP2PPullToPrefill adds the OffloadingConnector P2P pull block to a prefill |
There was a problem hiding this comment.
There is some inconsistency in the names and comments P2P vs remote_kv_source, as in the comment https://github.com/llm-d/llm-d-router/pull/2140/changes#r3639185910
There was a problem hiding this comment.
Renamed the constants after their wire keys in 43aea48: requestFieldRemoteDecoder, requestFieldRemotePrefiller, requestFieldRemoteKVSource, matching the sibling request-field constants, and swept the test comments that still said decode/prefill/p2p. P2P now only names the mechanism: the OffloadingConnector tier (type "p2p", unchanged in vLLM) and its protocol functions.
The requestFieldP2P* constants held remote_* wire keys, so the names no longer described the values. Naming them for their values matches the sibling request-field constants and leaves P2P to name only the OffloadingConnector tier and its protocol functions. Also sweeps test comments and one spec title that still named the sub-dicts by the old decode/prefill/p2p keys. Signed-off-by: nilig <nili.ifergan@gmail.com>
What type of PR is this?
/kind bug
What this PR does / why we need it:
Renames the sidecar's P2P
kv_transfer_paramssub-dict keys to match the vLLM OffloadingConnector branch after liranschour/vllm@530fa0d7 + @5bcf5b31:prefill->remote_prefiller,decode->remote_decoder,p2p->remote_kv_source, each named for the remote party it describes. Inner fields (kv_request_id,remote_host,remote_port) and thep2psecondary-tier type are unchanged. Without this, a sidecar paired with the current branch injects keys the engine ignores, so cached-prefix pulls are silently inert - requests serve, everything recomputes.The three literals live only in the
requestFieldP2P*constants, so the change is the constant values plus comments anddocs/disaggregation.md; all injection sites and tests flow through the constants. Constant identifiers are kept to minimize churn. Sidecar and engine must move together: mixing old and new sides on either arrangement leaves pulls silently disabled (both engine generations ignore foreign keys, so nothing crashes).Which issue(s) this PR fixes:
Fixes #2133
Release note:
Test plan:
make lintmake presubmitup to vulncheck; vulncheck fails identically on cleanmain(pre-existing GO-2026-5970, unrelated x/text)nightly-dcfebf93, Llama-8B 1P1D): with the source header the prefill worker pulled 1,216 tokens / 152 MB of the decoder's generated history (125 KB/token, byte-exact for Llama-8B bf16 KV); control without the header: no pull. Verified in-pod that the loaded engine parsesremote_kv_sourceand no longer parsesp2p.