Repository navigation
Conversation
690a449 to
af96e4d
Compare
|
cc @hickeyma |
shimib
left a comment
There was a problem hiding this comment.
The new chat branch rebuilds the engine input from MultiModalFeatures without the validations the generate path runs on the same payload. Findings in priority order.
Blocking
1. SHM cache handles are forwarded unauthenticated (vllm/renderers/online_renderer.py:839)
token_in_token_out/serving.py runs _validate_mm_cache_handles / cache.validate_input_item before kwargs reach the engine. The chat branch skips it. With --mm-processor-cache-type shm, a client can put forged address / monotonic_id / signature fields in prompt_features.kwargs_data and the engine dereferences an unauthenticated shared-memory handle. That is the hole #59357 closed on generate, reopened on chat.
2. Metadata-only items accepted without ec_transfer_params (online_renderer.py:839)
No equivalent of GenerateRequest._require_ec_for_metadata_only. A null kwargs_data (whole or per item) with mm_metadata set builds MultiModalKwargsItems with no pixel_values; the request passes the frontend and dies in engine core instead of returning the 400 generate gives.
3. Placeholder ranges are not bounds-checked (online_renderer.py:832)
No equivalent of _validate_multimodal_feature_bounds. prompt_token_ids of length 100 with mm_placeholders: {"image": [{"offset": 200, "length": 64}]} is accepted; the scheduler never reaches the placeholder and the request produces garbage or hangs rather than a 400.
4. Harmony regression (online_renderer.py:563)
The protocol change keeps prompt_token_ids for media requests when prompt_features is present, but _make_request_with_harmony takes the ids and drops the features. A GPT-OSS chat request with an image plus forwarded ids now submits placeholder-bearing ids with no mm_kwargs. Before this PR the messages were re-rendered. Either thread the features through the Harmony path or keep dropping the ids there.
5. mypy no-redef (online_renderer.py:831 and :858)
conversation: list[ConversationMessage] = [] is annotated in both sibling branches. pre-commit run mypy-3.12 --all-files --hook-stage manual fails with Name "conversation" already defined on line 831. The pre-commit CI job was skipped by the pre-run check, which is why it is green here. Declare it once above the if.
Should fix
docs/features/disagg_prefill.md:80still says prompt ids are always ignored whenmessageshas non-text content, andkv_transfer_params["prompt_features"]and its schema are undocumented. Router authors will not find this feature.online_renderer.py:129:prompt_featuresis popped and silently discarded whenprompt_token_idsis absent (hides a router bug; at least log it), and it is validated before theechoearly return, so a malformedprompt_features400s a request that was going to ignore it anyway.
Suggestion that covers 1–3 at once
The PlaceholderRange conversion + mm_kwargs_from_features + mm_input block is a byte-for-byte copy of token_in_token_out/serving.py:269-290. If you lift that block and its three validations into one helper next to mm_kwargs_from_features in mm_features.py (something like engine_input_from_features(token_ids, features, cache_salt, mm_cache)) and call it from both endpoints, the missing checks come for free and the two paths cannot drift again.
Minor: the function-local vllm.inputs / vllm.multimodal.inputs imports at online_renderer.py:825 are not needed for layering (vllm.inputs is already imported at module top); only the token_in_token_out imports need to be lazy. A TYPE_CHECKING import of MultiModalFeatures would also let reuse_features be typed instead of Any.
|
@shimib Thanks for the review. Let me fix your comments. |
|
Thanks @shimib — excellent review. All findings addressed in a1aad6f (hardening) + b1d347b (tests): Blocking
Should fix
The covering refactor — done as you suggested: Tests (b1d347b) — new |
|
Documentation preview: https://vllm--64721.org.readthedocs.build/en/64721/ |
|
This pull request has merge conflicts that must be resolved before it can be |
…prompt token ids When a router renders a prompt (including multimodal preprocessing) and forwards the result, the chat path can now skip templating, tokenization and multimodal preprocessing for media requests too, not just text. The render result is forwarded through the existing kv_transfer_params channel as two keys: - prompt_token_ids: the rendered token ids (existing, from vllm-project#48145) - prompt_features: the multimodal render result, using the same MultiModalFeatures schema that /inference/v1/generate already accepts (mm_hashes, mm_placeholders, kwargs_data, mm_metadata) When both are present, the chat path builds the engine input with the generate path's existing builder (mm_kwargs_from_features -> MultiModalKwargsItems -> mm_input) instead of rendering from messages. When prompt_features is absent, behavior is unchanged: the vllm-project#55771 media guard fires and the engine renders from messages. Addresses the multimodal gap in vllm-project#55770 and the router use case in vllm-project#54937. Validated on DeepSeek-V4.1-Flash with a sentinel test: blue messages + red image's forwarded payload -> 'Red' (the engine used the forwarded tensors, not the messages' image). Signed-off-by: Pravein Govindan Kannan <pravein.govindan.kannan@ibm.com>
Address review findings on the chat reuse branch: - Share the engine-input builder with the generate path. The PlaceholderRange conversion + mm_kwargs_from_features + mm_input block was a copy of token_in_token_out/serving.py; both endpoints now call engine_input_from_features() in mm_features.py, so the paths cannot drift. - Validate SHM cache handles on chat (blocking). With --mm-processor-cache-type shm, forged address/monotonic_id/signature fields in prompt_features.kwargs_data previously reached the engine unauthenticated on chat, reopening the hole vllm-project#59357 closed on generate. The shared builder runs cache.validate_input_item on every deserialized item before mm_input is built. - Reject metadata-only items without ec_transfer_params on chat (blocking). Mirrors GenerateRequest._require_ec_for_metadata_only via the shared validate_metadata_only(); a null kwargs_data with mm_metadata now fails with 400 instead of dying in engine core. - Bounds-check placeholder ranges on chat (blocking). Mirrors GenerateRequest._validate_multimodal_feature_bounds via the shared validate_feature_bounds(); out-of-range offsets now fail with 400 instead of hanging or producing garbage. - Fix the Harmony regression (blocking). _make_request_with_harmony kept placeholder-bearing prompt_token_ids while dropping prompt_features. Harmony is not a vision path: when features are forwarded the ids are dropped too and messages are re-rendered, as before this feature. - Fix mypy no-redef (blocking). conversation is now declared once above the reuse branches in preprocess_chat. - Validate prompt_features only after the echo / non-text early returns, so a malformed features payload no longer 400s a request that was going to ignore it; warn when prompt_features arrives without prompt_token_ids instead of silently discarding it (hides router bugs). - Type _reused_prompt_token_ids with MultiModalFeatures under TYPE_CHECKING instead of Any; drop the redundant function-local vllm.inputs / vllm.multimodal.inputs imports (module-top imports already exist). - Document kv_transfer_params['prompt_features'] and its schema in docs/features/disagg_prefill.md, including the render -> forward round trip and the validation guarantees. The GenerateRequest parse-time validators now delegate to the same shared helpers (lazy import to break the protocol <-> mm_features cycle), keeping identical error messages and timing for generate. Signed-off-by: Pravein Govindan Kannan <pravein.govindan.kannan@ibm.com>
The forwarded-features validations (SHM cache-handle authentication, metadata-only/EC consistency, placeholder bounds) were only exercised through the generate endpoint. Adds coverage for the chat side: - _reused_prompt_token_ids: features-without-ids warns and is ignored, echo skips feature validation, malformed features raise a 400-shaped VLLMValidationError, non-text messages keep the reuse path only with features and drop the ids without them. - engine_input_from_features: success shape, out-of-bounds placeholders, metadata-only without ec_transfer_params (accepted with it), forged SHM handle rejected with the (item, hash) call the cache expects. - preprocess_chat integration: forwarded ids + features skip rendering and build a multimodal engine input; forged handles and out-of-bounds placeholders fail with VLLMValidationError before the engine sees the request. - Harmony: ids forwarded with features are dropped and messages are re-rendered (regression test for the pre-feature behavior). Importing this module is also the smoke test that vllm.renderers.online_renderer imports cleanly with the TYPE_CHECKING-only MultiModalFeatures annotation. Verified: 13/13 pass; 119/119 including the existing token_in_token_out unit suite (generate stream, serde, protocol, logprobs). Signed-off-by: Pravein Govindan Kannan <pravein.govindan.kannan@ibm.com>
b1d347b to
a84f705
Compare
|
Fixes look right, tests and mypy pass locally. |
…branch - F823 (online_renderer.py): the lazy runtime import of MultiModalFeatures inside _reused_prompt_token_ids shadowed the TYPE_CHECKING name used in annotations. The local import is now aliased (_MultiModalFeatures) so the annotation binding resolves to the module-level TYPE_CHECKING import. - SIM102 (chat_completion/protocol.py): the nested ifs in drop_prompt_token_ids_with_media are combined into one condition (same short-circuit order and semantics). - ruff-format: three files reformatted (line-wrapping only). Verified with the pinned ruff 0.14.0 (check + format clean) and the chat-reuse test suite (84/84) in a live vLLM environment. Signed-off-by: Pravein Govindan Kannan <pravein.govindan.kannan@ibm.com>
|
@shimib I have pushed the fixes for the lint errors. |
|
Could you elaborate on why couldn't you pass the rendered |
gty111
left a comment
There was a problem hiding this comment.
Two issues with the forwarded-feature path:
|
|
||
| # Convert PlaceholderRangeInfo → PlaceholderRange per modality. | ||
| mm_placeholders: MultiModalPlaceholders = { | ||
| modality: [PlaceholderRange(offset=p.offset, length=p.length) for p in ranges] |
There was a problem hiding this comment.
Could we preserve PlaceholderRange.is_embed through serialization and reconstruction? Models such as Gemma 3 include text/structural tokens inside the placeholder range. Restoring only offset and length treats every position as an embedding position, causing a mismatch with the encoder output. The new chat fast path inherits this existing Generate API limitation; previously chat re-rendered the messages and retained the mask.
| mm_cache=renderer.mm_processor_cache, | ||
| ec_transfer_params=getattr(request, "ec_transfer_params", None), | ||
| ) | ||
| except ValueError as e: |
There was a problem hiding this comment.
Could malformed serialized items be converted to a validation error? A base64-encoded MessagePack payload such as {"pixel_values": {}} passes MultiModalFeatures validation but raises KeyError('data') during deserialization. Since only ValueError is caught, the request returns HTTP 500 instead of 400.
@DarkLight1337 Fair question — I initially did consider this. Mechanically it works: 1. The response contract. Clients speak OpenAI chat: streaming SSE with content/reasoning deltas, tool_call fragments, usage. The chat endpoint produces that directly; through 2. No topology change. The forwarding rides 3. Feature state stays in one place. On the chat path, 4. Scope. Same endpoint, same request/response contract, no new components; the chat path falls back to rendering whenever features are absent. The shared builder keeps the two paths from drifting on multimodal engine-input construction, so the token-in-token-out option stays fully open — for |
Per review, defer multimodal feature forwarding to its own PR once vllm-project/vllm#64721 merges: the wire-feature layer retained multi-MB kwargs on TokenizedRequest by default, and against current vLLM the forwarded prompt_features would be left in kv_transfer_params after prompt_token_ids is popped. Multimodal prompts are now skipped outright (prompt.MultiModalFeatures != nil -> return nil), with the deferral noted in the README and the skip comment. Also from review: - Construct the PreRequest debug logger only in the non-mutable-payload branch (avoids a per-request child-logger allocation on the hot path). - Promote FieldPromptTokenIDs and FieldPrompt to pkg/common/request/constants.go; forwardtokens, the P/D sidecar and the OpenAI parser now share them instead of three private copies. - Drop the review-comment reference from the nixlv2 test comment. Signed-off-by: Pravein Govindan Kannan <pravein.govindan.kannan@ibm.com>
Per review, defer multimodal feature forwarding to its own PR once vllm-project/vllm#64721 merges: the wire-feature layer retained multi-MB kwargs on TokenizedRequest by default, and against current vLLM the forwarded prompt_features would be left in kv_transfer_params after prompt_token_ids is popped. Multimodal prompts are now skipped outright (prompt.MultiModalFeatures != nil -> return nil), with the deferral noted in the README and the skip comment. Also from review: - Construct the PreRequest debug logger only in the non-mutable-payload branch (avoids a per-request child-logger allocation on the hot path). - Promote FieldPromptTokenIDs and FieldPrompt to pkg/common/request/constants.go; forwardtokens, the P/D sidecar and the OpenAI parser now share them instead of three private copies. - Drop the review-comment reference from the nixlv2 test comment. Signed-off-by: Pravein Govindan Kannan <pravein.govindan.kannan@ibm.com>
…ures; 400 on malformed items Address review feedback on the multimodal feature forwarding: - PlaceholderRangeInfo carries is_embed (list[bool] | None, validated against length) so models whose placeholders interleave text or structural tokens (Gemma 3, Qwen-VL) keep their sparse-embedding mask across the render -> forward round-trip. The shared engine_input_from_features reconstructs the torch bool mask, so both the generate path and the chat reuse branch align encoder output with the masked positions; previously the chat path re-rendered and retained the mask while forwarding lost it. - Malformed serialized items now fail with 400 instead of 500: a payload that is valid base64 and valid MessagePack can still fail MultiModalKwargsItem reconstruction (e.g. a tensor field carrying a non-tensor value raises KeyError). The decode is wrapped to map any such failure to ValueError, which both the generate and chat error handlers convert to HTTP 400. Tests: is_embed round-trip (mask preserved, get_num_embeds), None for plain ranges, length-mismatch validation, malformed base64-MessagePack payload -> ValueError. 17/17 pass. Signed-off-by: Pravein Govindan Kannan <pravein.govindan.kannan@ibm.com>
…decar One prompt tokenization per request. The EPP's token-producer already renders every request for routing; this hands the rendered IDs to the model server so it does not tokenize again: - /v1/chat/completions, /v1/messages, /v1/responses: written as kv_transfer_params.prompt_token_ids, which vLLM's chat path reads to skip templating and tokenization (vLLM #48145). - /v1/completions: the text prompt is replaced with its token IDs. The plugin skips: requests without a tokenization result, the token-producer's legacy /v1/messages and /v1/responses translations (marked LegacyTranslated on TokenizedRequest — no token parity guarantee), stateful /v1/responses fields, echo and return_prompt_text (both need the server to render), multimodal prompts (feature forwarding deferred pending vllm-project/vllm#64721), multi-prompt requests, non-mutable payloads, and client-supplied prompt_token_ids (an explicit null counts as absent and is replaced). The kv_transfer_params fast path peeks json.RawMessage for an existing prompt_token_ids before materializing the array (long-context hot path). The P/D sidecar keeps the client's IDs as raw JSON end to end (kv_transfer_params is no longer decoded on ingest) and every connector — offloading, mooncake, NIXLv2 — carries them onto both the prefill and decode requests, including when the prefill response omits kv_transfer_params. Constants FieldPromptTokenIDs and FieldPrompt are shared from pkg/common/request. Rebased onto the requestcontrol→requesthandling package rename. Signed-off-by: Pravein Govindan Kannan <pravein.govindan.kannan@ibm.com>
Thanks @gty111 — both addressed in dc2cc52: 1. 2. Malformed items → 400. The decode is now wrapped: bad base64 ( |
…decar One prompt tokenization per request. The EPP's token-producer already renders every request for routing; this hands the rendered IDs to the model server so it does not tokenize again: - /v1/chat/completions, /v1/messages, /v1/responses: written as kv_transfer_params.prompt_token_ids, which vLLM's chat path reads to skip templating and tokenization (vLLM #48145). - /v1/completions: the text prompt is replaced with its token IDs. The plugin skips: requests without a tokenization result, the token-producer's legacy /v1/messages and /v1/responses translations (marked LegacyTranslated on TokenizedRequest — no token parity guarantee), stateful /v1/responses fields, echo and return_prompt_text (both need the server to render), multimodal prompts (feature forwarding deferred pending vllm-project/vllm#64721), multi-prompt requests, non-mutable payloads, and client-supplied prompt_token_ids (an explicit null counts as absent and is replaced). The kv_transfer_params fast path peeks json.RawMessage for an existing prompt_token_ids before materializing the array (long-context hot path). The P/D sidecar keeps the client's IDs as raw JSON end to end (kv_transfer_params is no longer decoded on ingest) and every connector — offloading, mooncake, NIXLv2 — carries them onto both the prefill and decode requests, including when the prefill response omits kv_transfer_params. Constants FieldPromptTokenIDs and FieldPrompt are shared from pkg/common/request. Rebased onto the requestcontrol→requesthandling package rename. Signed-off-by: Pravein Govindan Kannan <pravein.govindan.kannan@ibm.com>
…decar One prompt tokenization per request. The EPP's token-producer already renders every request for routing; this hands the rendered IDs to the model server so it does not tokenize again: - /v1/chat/completions, /v1/messages, /v1/responses: written as kv_transfer_params.prompt_token_ids, which vLLM's chat path reads to skip templating and tokenization (vLLM #48145). - /v1/completions: the text prompt is replaced with its token IDs. The plugin skips: requests without a tokenization result, the token-producer's legacy /v1/messages and /v1/responses translations (marked LegacyTranslated on TokenizedRequest — no token parity guarantee), stateful /v1/responses fields, echo and return_prompt_text (both need the server to render), multimodal prompts (feature forwarding deferred pending vllm-project/vllm#64721), multi-prompt requests, non-mutable payloads, and client-supplied prompt_token_ids (an explicit null counts as absent and is replaced). The kv_transfer_params fast path peeks json.RawMessage for an existing prompt_token_ids before materializing the array (long-context hot path). The P/D sidecar keeps the client's IDs as raw JSON end to end (kv_transfer_params is no longer decoded on ingest) and every connector — offloading, mooncake, NIXLv2 — carries them onto both the prefill and decode requests, including when the prefill response omits kv_transfer_params. Constants FieldPromptTokenIDs and FieldPrompt are shared from pkg/common/request. Rebased onto the requestcontrol→requesthandling package rename. Signed-off-by: Pravein Govindan Kannan <pravein.govindan.kannan@ibm.com>
hickeyma
left a comment
There was a problem hiding this comment.
@DarkLight1337 sorry for the slow reply. Part of the reasoning above has changed but I still think there's a case for the chat path. With #63103 merged, output_mode: "derender" streams parsed output from generate which means the caller doesn't carry stream state or need a separate derender frontend. It does need the tokenizer and parsers on the generate server. What generate doesn't return is OpenAI chat chunks, so the router has to rebuild them. llm-d-router#3656 went with forwarded IDs on the client's route as its default which keeps the native response.
The bigger concern for me is that I don't see a gate. This accepts client supplied tensors on /v1/chat/completions with no KV connector or --enable-scale-out check which means it's open on a plain vllm serve. The blue/red test in the description shows the model answering from something other than messages which is #62307 with images. I think this should follow whatever we decide on #62333.
@praveingk one question on payload. Forwarded tensors are about 8 KB per image token unless they hit the cache and the original image stays in messages. llm-d-router#3656 lists no rendered tensors across the gateway in its acceptance and llm-d-router#3706 needs vllm.omitMMKwargs: false to send them. How do you see those fitting together?
What type of PR is this?
/kind feature
What this PR does / why we need it:
Cache-aware routers (llm-d, Ray's kv-aware router, SGLang's router) render every prompt to pick an endpoint, then forward the original
messagesand the engine renders again. On text prompts,kv_transfer_params["prompt_token_ids"](#48145) already lets the engine skip that second render. On multimodal prompts, #55771 deliberately drops the forwarded IDs because they carry no multimodal data — the engine must rendermessagesto process the images.This PR closes that gap by letting the router forward the multimodal render result alongside the IDs, through the same
kv_transfer_paramschannel:{ "messages": [{"role": "user", "content": [ {"type": "text", "text": "What is in this image?"}, {"type": "image_url", "image_url": {"url": "data:image/png;base64,..."}} ]}], "kv_transfer_params": { "prompt_token_ids": [151644, 872, ...], "prompt_features": { "mm_hashes": {"image": ["<content-hash>"]}, "mm_placeholders": {"image": [{"offset": 57, "length": 192}]}, "kwargs_data": {"image": ["<base64-encoded MultiModalKwargsItem>"]}, "mm_metadata": null } } }prompt_featuresuses the sameMultiModalFeaturesschema that/inference/v1/generatealready accepts (the token-in/token-out format from the scale-out work). When both keys are present, the chat path's ID-reuse hook builds the engine input with the generate path's existing builder (mm_kwargs_from_features→MultiModalKwargsItems→mm_input) — no new engine semantics, just connecting two existing code paths.Why this addresses the #55771 guard's concern: the guard exists because "the ids stand in for the rendered prompt, but they carry no multimodal data or hashes." With
prompt_featuresforwarded, the IDs are no longer standing in alone — they come with the complete render result (hashes for prefix-cache disambiguation, processed tensors, placeholder layout). Withoutprompt_features, the guard fires exactly as today.Changes:
vllm/renderers/online_renderer.py:_reused_prompt_token_idsreturns(ids, features)and pops both keys; the chat-path call site gains a multimodal reuse branch that mirrors the generate path's input builder; the Harmony call site unpacks the tuple (features unsupported there)vllm/entrypoints/openai/chat_completion/protocol.py:drop_prompt_token_ids_with_mediaonly pops the IDs whenprompt_featuresis absentNo new API surface. Both keys travel through the existing
kv_transfer_paramsdict. The feature degrades gracefully: withoutprompt_features, behavior is identical tomain.Validation (DeepSeek-V4.1-Flash, 2×8 H100, vision tower loaded, on-cluster):
Sentinel proof that the engine uses the forwarded payload:
Full-path regression (18 tests, all pass): text chat (streaming, non-streaming, multi-turn, tools, reasoning), completions, Messages, Responses, images (short, long/forwarded, streaming), and edge cases (
tool_choice=none,reasoning_effort=medium, empty content).Which issue(s) this PR fixes:
Addresses the multimodal gap in #55770 and the router use case in #54937.
Special notes for your reviewer:
The
mm_input(...)call in the new multimodal branch is byte-identical to the generate path's usage intoken_in_token_out/serving.py. The lazy imports (mm_kwargs_from_features,mm_input,MultiModalKwargsItems,PlaceholderRange) keep the entrypoints layering acyclic. The router-side implementation is llm-d-router #3706.Release note: