feat(frontend): pass through vLLM cached image UUIDs#8957
Conversation
The `image_url`/`video_url`/`audio_url` `uuid` field is now parsed as a strict `Uuid` (8-4-4-4-12 hex) at the chat parser. Inside the preprocessor it's downgraded to `String` before being forwarded to vLLM, whose `multi_modal_uuids` is contract-free on format. This gives the public API a tight type while keeping the internal pipeline unchanged (`MultimodalData::UuidOnly(String)`, `MultimodalUuidMap`). Producers that emit `img-<sha256[:16]>`-style cache keys must format them as proper UUIDs before sending — the JSONL benchmark generator on `qiwa/jsonl-uuids` is updated to use `uuid5(NAMESPACE_OID, ref)`. The companion aiperf change (#869) needs the same — flagged in PR #8957 description. E2E (Qwen3-VL-2B-Instruct, mm_processor_cache_gb=4): PASS 01_url_plus_uuid_fill (real model: "the dominant color is red") PASS 02_uuid_only_hit (uuid-only reaches worker) PASS 03_uuid_only_miss (uuid-only reaches worker) PASS 04_five_imgs_one_fresh_four_repeats PASS 05_negative_non_uuid_string (HTTP 400 on `img-not-a-uuid`) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`compute_image_uuid` now returns `uuid5(NAMESPACE_OID, ref)` — a proper hyphenated UUID — so dynamo's chat parser (which validates `image_url.uuid` as a strict `Uuid` at the wire boundary, see PR #8957) accepts it. The deterministic-per-ref guarantee is preserved by UUIDv5's namespace hash. Wire shape changes from `img-ac3921de680bb217` to e.g. `508e1a09-5ecd-528a-8ce5-f7c6720baf17`. The aiperf companion (#869) needs the same swap on the consumer side. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The `image_url`/`video_url`/`audio_url` `uuid` field is now parsed as a strict `Uuid` (8-4-4-4-12 hex) at the chat parser. Inside the preprocessor it's downgraded to `String` before being forwarded to vLLM, whose `multi_modal_uuids` is contract-free on format. This gives the public API a tight type while keeping the internal pipeline unchanged (`MultimodalData::UuidOnly(String)`, `MultimodalUuidMap`). Producers that emit `img-<sha256[:16]>`-style cache keys must format them as proper UUIDs before sending — the JSONL benchmark generator on `qiwa/jsonl-uuids` is updated to use `uuid5(NAMESPACE_OID, ref)`. The companion aiperf change (#869) needs the same — flagged in PR #8957 description. E2E (Qwen3-VL-2B-Instruct, mm_processor_cache_gb=4): PASS 01_url_plus_uuid_fill (real model: "the dominant color is red") PASS 02_uuid_only_hit (uuid-only reaches worker) PASS 03_uuid_only_miss (uuid-only reaches worker) PASS 04_five_imgs_one_fresh_four_repeats PASS 05_negative_non_uuid_string (HTTP 400 on `img-not-a-uuid`) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
01afe80 to
c4a77f8
Compare
WalkthroughThis PR implements vLLM's cached-MM UUID passthrough across the Dynamo stack, enabling multimodal requests to reuse cached processor results via optional UUID fields. Changes span wire types, frontend extraction/batching, backend preprocessing with slot-aligned UUID tracking, defensive media loading, and comprehensive test coverage. ChangesCached Multimodal UUID Passthrough
🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
lib/llm/tests/preprocessor.rs (1)
886-910: ⚡ Quick winAvoid unconditional
extra_argsunwrap in the no-UUID branch.Line 886 unconditionally unwraps
preprocessed.extra_args; this makes theexpected_mm_hashes = Nonecase brittle ifextra_argsis absent. Use optional-safe access for the absence assertion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/llm/tests/preprocessor.rs` around lines 886 - 910, The test unconditionally calls preprocessed.extra_args.as_ref().unwrap(), which causes a panic in the None branch; change the None branch to access extra_args safely (e.g., use preprocessed.extra_args.as_ref().and_then(|extra| extra.get("mm_hashes"))) and assert that that result is None instead of unwrapping; keep the existing Some(...) branch that inspects extra intact and only use safe option/borrow access for extra_args in the expected_mm_hashes == None path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/src/dynamo/common/multimodal/image_loader.py`:
- Around line 228-267: The reconstruction can silently drop trailing inputs
because unsupported item shapes don't append to slot_to_future_idx and
zip(image_mm_items, slot_to_future_idx) isn't strict; update the loop over
image_mm_items so that every branch (including an explicit else fallback)
appends a sentinel (e.g., None) to slot_to_future_idx and logs/raises as
appropriate, and change the pairing to use zip(..., strict=True) when iterating
for results to surface any length mismatches; reference the variables
image_mm_items, slot_to_future_idx, image_futures and the handlers load_image
and _read_and_convert_nixl_image and the keys URL_VARIANT_KEY,
DECODED_VARIANT_KEY, UUID_ONLY_VARIANT_KEY to locate the code to modify.
In `@components/src/dynamo/common/tests/multimodal/test_image_loader.py`:
- Around line 264-334: The tests call an undefined helper _mock_http_client
causing F821; add a local definition or import the existing helper into this
test module so _mock_http_client is available to async tests like
test_load_image_batch_five_images_four_overlapping and
test_load_image_batch_all_uuid_only_no_http_fetch; ensure the helper returns the
same mock object used in the with patch(..., return_value=mock_client) blocks
and exposes awaited methods used in assertions (e.g., get, send) so
mock_client.get.assert_not_awaited() and mock_client.send.assert_not_awaited()
work as expected.
In `@docs/features/multimodal/multimodal-vllm.md`:
- Around line 74-75: Add a single clarifying sentence to the paragraph that
begins "The UUID is optional." stating that any media part missing both `url`
and `uuid` is treated as invalid input and will be rejected; reference the media
part wire contract and contrast this with cache-miss failures so readers know
malformed payloads (neither `url` nor `uuid`) are refused while cache-only
requests fail when the UUID is unknown.
In `@lib/llm/src/protocols/anthropic/types.rs`:
- Around line 1660-1665: The code currently dereferences img.image_url.url
directly but img.image_url is now Option<ImageUrl> and ImageUrl.url is
Option<Url>, causing E0609; update the access in types.rs to first unwrap or map
img.image_url (e.g., use img.image_url.as_ref() or match on img.image_url) and
then unwrap the inner url (e.g., .url.as_ref() or match) before calling
to_string(), and update the expect message to reflect both missing layers (e.g.,
"image_url and image_url.url required for anthropic image conversion") so you
don't index through a None.
In `@lib/protocols/src/types/chat.rs`:
- Around line 263-267: The media-part structs (the fields image_url:
Option<ImageUrl> and uuid: Option<String>, and the analogous video/audio fields)
must reject instances that have neither a URL nor a UUID; add a validation step
in the builder or deserializer for the ImagePart/VideoPart/AudioPart types that
returns an error if both image_url (or video_url/audio_url) and uuid are None.
Locate the builder/build function for these structs (the
derive_builder-generated build path for ImagePart/VideoPart/AudioPart) or
implement a custom Deserialize/try_from for the structs and enforce "if
self.image_url.is_none() && self.uuid.is_none() { return Err(...); }" (and the
same for video_url/audio_url) so malformed payloads fail at schema boundary.
In `@tests/e2e/test_uuid_passthrough_e2e.sh`:
- Around line 51-56: The cleanup() function currently uses unsafe pattern pkill
-f which can kill unrelated processes; update cleanup() to only kill the
captured PIDs (use WORKER_PID and FRONTEND_PID) by testing if each variable is
set and the PID exists, then send a TERM (kill -TERM "$WORKER_PID") and
wait/kill -KILL if necessary, ignore errors (|| true), and clear the variables
afterward; apply the same PID-targeted replacement for the other teardown block
around lines 68-75 that also uses pkill -f.
- Around line 77-104: The readiness polling loops for the frontend (/health
probe using PORT) and worker registration (WARMUP POST to /v1/chat/completions)
lack explicit timeout handling; add timeout failure branches after each for-loop
that print a clear error (e.g., "frontend not ready after timeout" / "worker
registration timed out") and exit non-zero (exit 1) so the script fails fast
when retries are exhausted. Ensure the messages reference the probe being used
(health or chat/completions) and keep behavior otherwise unchanged.
- Around line 95-98: The test currently writes warmup responses to a fixed
/tmp/_warmup.json which can collide in concurrent runs; change the curl call in
test_uuid_passthrough_e2e.sh to write to a unique temp file (e.g., create one
with mktemp or use the test framework's tmp_path/tempfile fixture), use that
temp filename in the -o argument for the curl that posts $WARMUP to
"http://localhost:$PORT/v1/chat/completions", and ensure the temp file is
removed after the test (or use a fixture-managed temp directory) so concurrent
runs don’t clash.
In `@tests/utils/client.py`:
- Around line 42-44: The truncation call can raise TypeError when image_url is a
dict like {"url": None}; update the guard around the _truncate_base64_url call
in the code handling part.get("image_url") so it only calls _truncate_base64_url
when image_url is a dict and image_url.get("url") is not None (i.e., explicitly
check for None before passing to _truncate_base64_url), leaving image_url
unchanged when the URL is null.
---
Nitpick comments:
In `@lib/llm/tests/preprocessor.rs`:
- Around line 886-910: The test unconditionally calls
preprocessed.extra_args.as_ref().unwrap(), which causes a panic in the None
branch; change the None branch to access extra_args safely (e.g., use
preprocessed.extra_args.as_ref().and_then(|extra| extra.get("mm_hashes"))) and
assert that that result is None instead of unwrapping; keep the existing
Some(...) branch that inspects extra intact and only use safe option/borrow
access for extra_args in the expected_mm_hashes == None path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e9576acb-543b-4403-950d-5bcb3f5d4b0d
📒 Files selected for processing (19)
components/src/dynamo/common/multimodal/image_loader.pycomponents/src/dynamo/common/tests/multimodal/test_image_loader.pycomponents/src/dynamo/frontend/sglang_processor.pycomponents/src/dynamo/frontend/tests/test_multimodal_utils.pycomponents/src/dynamo/frontend/utils.pycomponents/src/dynamo/frontend/vllm_processor.pydocs/features/multimodal/multimodal-vllm.mdlib/llm/src/preprocessor.rslib/llm/src/preprocessor/media/loader.rslib/llm/src/protocols/anthropic/types.rslib/llm/src/protocols/common/preprocessor.rslib/llm/src/protocols/openai/responses/mod.rslib/llm/tests/preprocessor.rslib/protocols/src/types/chat.rslib/protocols/src/types/impls.rstests/e2e/test_uuid_passthrough_e2e.shtests/serve/multimodal_profiles/vllm.pytests/utils/client.pytests/utils/multimodal.py
Preserve user-provided cached multimodal UUIDs across Rust and Python preprocessing, backend loading, routing metadata, and vLLM cache reuse. Co-authored-by: Indrajit Bhosale <iamindrajitb@gmail.com> Signed-off-by: Qi Wang <qiwa@nvidia.com>
1cffc66 to
8ca4758
Compare
This comment has been minimized.
This comment has been minimized.
Signed-off-by: Qi Wang <qiwa@nvidia.com>
uuid to backendSigned-off-by: Qi Wang <qiwa@nvidia.com>
|
/ok to test 62ac636 |
Preserve the official dynamo-protocols UUID pin and add aggregate embedding-cache restore coverage while resolving the main branch conflict. Signed-off-by: Zhuangcheng(Jesse) Gu <zcgu@connect.hku.hk>
|
Hi @furionw, I’ve made some updates to this PR. Within the intended aggregated serving scope, vLLM uuid passthrough now should works E2E, including url+uuid, uuid-only, embedding cache, and text-prefix routing fallback. MM-aware routing and cross-replica cache affinity remain out of scope. |
Based on frontend-crates#119.
Why
vLLM extends OpenAI-compatible chat image parts with opaque processor-cache UUIDs, including UUID-only cache hits. Dynamo needs to preserve that contract across Rust/Python preprocessing and distributed workers without treating client keys as router hashes.
What changed
Dependency
Temporarily pins
dynamo-protocols2.0.2 tofrontend-crates@522a09cpending the #119 release. This PR remains draft until that prerequisite lands.Validation
cargo test -p dynamo-llm cached_multimodal_uuid— 5 passed.cargo test -p dynamo-llm preprocessor::media::loader::tests— 15 passed.mm_agg_gemma4-e2b-it_uuid_passthrough-2— passed with both the Rust and vLLM chat processors.cargo fmt --all -- --check, andgit diff --checkpassed.