feat(mm-routing): lightseek-mm Rust-frontend MM-aware KV routing - #9272
Conversation
WalkthroughThis PR implements Lightseek-based multimodal-aware KV cache routing via a Rust frontend. It adds image token counting, content-addressed hashing ( ChangesLightseek MM-aware KV Routing via Rust Frontend
🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (2)
lib/llm/Cargo.toml (1)
87-87: ⚡ Quick winMake
mokaan optional dependency and add it to thelightseek-mmfeature.
mokais only used inside#[cfg(feature = "lightseek-mm")]blocks inpreprocessor.rs(line 983), yet it's compiled unconditionally. The feature documentation promises a slimmer build with--no-default-features --features block-manager, but this build still incurs the cost of compilingmokaand its tokio integration despite never using them.♻️ Suggested fix
-moka = { workspace = true } +moka = { workspace = true, optional = true }-lightseek-mm = ["dep:llm-multimodal", "dep:openssl"] +lightseek-mm = ["dep:llm-multimodal", "dep:openssl", "dep:moka"]🤖 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/Cargo.toml` at line 87, The crate currently compiles moka unconditionally; mark the moka dependency as optional and include it under the lightseek-mm feature so it only builds when that feature is enabled. Update Cargo.toml to make the moka dependency optional (optional = true) and add "moka" to the feature list for "lightseek-mm" (so the feature enables moka), matching the cfg usage in preprocessor.rs (the blocks gated by #[cfg(feature = "lightseek-mm")]). Ensure the feature name exactly matches "lightseek-mm" so the conditional compilation and feature flag behave consistently.lib/llm/src/preprocessor/lightseek_mm.rs (1)
31-44: ⚡ Quick win
try_newperforms blocking filesystem I/O and is called from async contexts.
std::fs::read_to_stringblocks the calling thread. The function is invoked fromOpenAIPreprocessor::new_with_parts, which is called during model initialization fromasync fn do_worker_set_registrationinlib/llm/src/discovery/watcher.rs. This can stall a tokio runtime worker thread. Usetokio::fs::read_to_stringfor async I/O, or wrap the call withtokio::task::spawn_blocking.🤖 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/src/preprocessor/lightseek_mm.rs` around lines 31 - 44, The try_new function in lightseek_mm.rs does blocking disk I/O via std::fs::read_to_string and is called from async contexts (e.g., OpenAIPreprocessor::new_with_parts -> do_worker_set_registration); replace the blocking call with an async-aware version: either call tokio::fs::read_to_string awaiting the result (and make try_new async) or keep try_new sync but offload the read_to_string into tokio::task::spawn_blocking and await that JoinHandle; also ensure the subsequent PreProcessorConfig::from_json parsing is performed on the async path or inside the spawn_blocking closure to avoid blocking the runtime thread.
🤖 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 `@examples/backends/vllm/launch/agg_multimodal_router_chat_processor.sh`:
- Around line 22-27: Replace the hand-rolled GPU/lifecycle logic in
agg_multimodal_router_chat_processor.sh (the manual handling of
_PROFILE_OVERRIDE_VLLM_KV_CACHE_BYTES, readiness polling, cleanup, and final
wait path) by sourcing the shared helpers gpu_utils.sh and launch_utils.sh, call
build_vllm_gpu_mem_args to construct vLLM memory args, use print_launch_banner
for consistent startup logs, and terminate the script with wait_any_exit so the
test/serve harness gets VRAM-safe parallel runs and proper fail-fast shutdown;
update all similar blocks (lines around 46-52, 114-149, 297) to the same pattern
and remove the duplicated readiness/cleanup/wait logic.
In `@examples/backends/vllm/launch/agg_multimodal_router.sh`:
- Around line 25-30: Replace the script's custom cleanup/wait logic by sourcing
the repo-standard launch_utils.sh near the top (using SCRIPT_DIR to locate it,
just like gpu_utils.sh), retain set -euo pipefail and the EXIT trap placement,
remove the hand-rolled trap/wait handling, and replace any bare wait calls with
wait_any_exit to ensure immediate failure detection and cleanup; apply the same
change to the other occurrences referenced (around the other ranges).
- Around line 127-148: The script is passing a manual --gpu-memory-utilization
flag that conflicts with the helper build_vllm_gpu_mem_args; remove the explicit
--gpu-memory-utilization "${GPU_MEMORY_UTILIZATION}" token from the python -m
dynamo.vllm invocation so the GPU_MEM_ARGS (returned by build_vllm_gpu_mem_args)
are the single source of truth for VRAM sizing; ensure GPU_MEM_ARGS remains
included in the command alongside VLLM_EXTRA_ARGS and PASSTHRU_ARGS.
In `@lib/llm/src/preprocessor.rs`:
- Around line 189-203: The struct OpenAIPreprocessor currently declares
kv_cache_block_size twice (once as an unconditional usize and again as a
feature-gated u32), causing a duplicate field error when lightseek-mm is
enabled; fix by consolidating into a single field named kv_cache_block_size with
a single type (prefer u32 to match the feature-gated declaration) and remove the
duplicate feature-gated declaration (or alternatively rename the feature-gated
field to kv_cache_block_size_mm if you need both), and update any usages of
OpenAIPreprocessor::kv_cache_block_size to the chosen type (and adjust casts)
or, if renaming, update all call sites that reference the old feature-gated
name.
- Around line 154-162: The type MmImageEntry is feature-gated but
gather_multi_modal_data() returns Vec<MmImageEntry> unconditionally, causing
compile failures when lightseek-mm is disabled; either remove the #[cfg(feature
= "lightseek-mm")] from the MmImageEntry definition so the struct is always
available, or gate the gather_multi_modal_data() signature and every call site
(e.g., calls at gather_mm_exact_routing_info, and the callers around lines
noted) with the same #[cfg(feature = "lightseek-mm")] so the function and its
uses are only compiled when the feature is enabled; update the signature and all
call sites (or the struct visibility) consistently to eliminate the
unconditional dependency.
In `@lib/llm/src/preprocessor/image_token.rs`:
- Around line 72-77: The current conditional in image_token.rs uses chained
.or(...) on ConfigJson fields (image_token_id, image_token_index,
media_placeholder_token_id) before validating >= 0, so a sentinel -1 on an
earlier field blocks valid later values; change the logic after
load_json::<ConfigJson> to explicitly inspect each field in order (inspect
image_token_id, if Some and >= 0 use it; otherwise inspect image_token_index, if
Some and >= 0 use it; otherwise inspect media_placeholder_token_id and accept it
only if >= 0) and only then proceed when you have a non-negative id; refer to
load_json, ConfigJson, image_token_id, image_token_index, and
media_placeholder_token_id to locate and update the check.
In `@tests/mm_router/test_router_rust_mm_frontend_decode_e2e.py`:
- Around line 55-63: The module-level pytestmark lacks a profiled_vram_gib
marker; update the pytestmark list (where VLLM_MM_MODEL and
pytest.mark.requested_vllm_kv_cache_bytes(...) are declared) to include
pytest.mark.profiled_vram_gib(<appropriate_gib_value>) so the parallel scheduler
can size runs correctly; place the new marker alongside the existing markers in
the pytestmark list for tests in
tests/mm_router/test_router_rust_mm_frontend_decode_e2e.py.
- Around line 91-94: _prepare_log_dir currently writes into a repo-relative path
and mm_runtime_services mutates os.environ directly; change _prepare_log_dir to
create and return a temp directory (use pytest's tmp_path or tempfile.mkdtemp
and create a subdir named with suffix) instead of using shutil.rmtree on a repo
path, and update callers to pass tmp_path (or make _prepare_log_dir accept
tmp_path) so logs are written under the temp dir; in mm_runtime_services replace
direct os.environ[...] = ... with pytest.MonkeyPatch (or the monkeypatch
fixture) calls like monkeypatch.setenv("NATS_SERVER", value) and
monkeypatch.setenv("ETCD_ENDPOINTS", value) so env changes are scoped and
cleaned up automatically (also apply the same tmp_path/monkeypatch pattern for
the other occurrence mentioned at lines ~175-182).
In `@tests/mm_router/test_router_rust_mm_router_e2e.py`:
- Around line 94-97: The _prepare_log_dir function and mm_runtime_services
fixture must avoid writing into the repo and mutating os.environ directly:
change _prepare_log_dir to create/return a temporary directory using pytest's
tmp_path or tempfile (e.g., use request and tmp_path to build a unique dir and
rmtree only within that temp), and update mm_runtime_services to set
NATS/ETCD-related env vars via pytest.MonkeyPatch (monkeypatch.setenv) instead
of assigning os.environ[...] directly; ensure teardown uses tmp_path cleanup and
that log paths point into the provided temp dir so tests are hermetic and leave
no repo state behind.
- Around line 53-61: The file-level pytestmark list is missing a profiled VRAM
marker; add a pytest.mark.profiled_vram_gib(<N>) entry to the pytestmark array
alongside pytest.mark.model(VLLM_MM_MODEL) and
pytest.mark.requested_vllm_kv_cache_bytes(...) so the scheduler can size the
test correctly; choose the <N> value equal to the profiled VRAM (in GiB) for the
VLLM_MM_MODEL used in this test and place the new
pytest.mark.profiled_vram_gib(...) element in the existing pytestmark list.
In `@tests/serve/multimodal_profiles/vllm.py`:
- Around line 108-141: The three TopologyConfig entries "agg_router",
"agg_router_chat_processor", and "agg_router_frontend_decode" are missing the
required profiled_vram_gib marker; run tests/utils/profile_pytest.py to measure
the VRAM (GiB) used by Qwen/Qwen3-VL-2B-Instruct for each topology and add
profiled_vram_gib=<measured_value> to each TopologyConfig (alongside the
existing requested_vllm_kv_cache_bytes and env where present) so the parallel
scheduler and --max-vram-gib filtering can size and select these tests
correctly.
---
Nitpick comments:
In `@lib/llm/Cargo.toml`:
- Line 87: The crate currently compiles moka unconditionally; mark the moka
dependency as optional and include it under the lightseek-mm feature so it only
builds when that feature is enabled. Update Cargo.toml to make the moka
dependency optional (optional = true) and add "moka" to the feature list for
"lightseek-mm" (so the feature enables moka), matching the cfg usage in
preprocessor.rs (the blocks gated by #[cfg(feature = "lightseek-mm")]). Ensure
the feature name exactly matches "lightseek-mm" so the conditional compilation
and feature flag behave consistently.
In `@lib/llm/src/preprocessor/lightseek_mm.rs`:
- Around line 31-44: The try_new function in lightseek_mm.rs does blocking disk
I/O via std::fs::read_to_string and is called from async contexts (e.g.,
OpenAIPreprocessor::new_with_parts -> do_worker_set_registration); replace the
blocking call with an async-aware version: either call tokio::fs::read_to_string
awaiting the result (and make try_new async) or keep try_new sync but offload
the read_to_string into tokio::task::spawn_blocking and await that JoinHandle;
also ensure the subsequent PreProcessorConfig::from_json parsing is performed on
the async path or inside the spawn_blocking closure to avoid blocking the
runtime thread.
🪄 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: 0f39e4ac-e4d5-411e-9d87-26b7b54ab460
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locklib/bindings/python/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
Cargo.tomlcomponents/src/dynamo/vllm/handlers.pydocs/features/multimodal/multimodal-kv-routing.mdexamples/backends/vllm/launch/agg_multimodal_router.shexamples/backends/vllm/launch/agg_multimodal_router_chat_processor.shlib/bindings/python/Cargo.tomllib/llm/Cargo.tomllib/llm/examples/lightseek_count.rslib/llm/src/preprocessor.rslib/llm/src/preprocessor/image_token.rslib/llm/src/preprocessor/lightseek_mm.rslib/llm/src/preprocessor/media/rdma.rstests/mm_router/test_router_rust_mm_frontend_decode_e2e.pytests/mm_router/test_router_rust_mm_router_e2e.pytests/serve/multimodal_profiles/vllm.py
Some VLM families (Kimi-K2.5) declare their image modality as "vision_chunk" rather than "image". vLLM's openai entrypoint mirrors this at chat_utils time (use_unified_vision_chunk_modality), but dynamo bypasses chat_utils and builds `multi_modal_data` directly, so image_url requests landed under multi_modal_data["image"] and Kimi rejected with `At most 0 image(s) may be provided in one prompt.` Read hf_config.use_unified_vision_chunk at handler init; when true, forward image_url items as VisionChunkImage TypedDicts under the vision_chunk key. e2e verified on prenyx (Kimi-K2.5 TP=8): "two tabby cats on a pink couch" response on COCO val/39769, warm-request cached_tokens 432/433 (99.77%) on repeat. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…able preprocessors Promote `MEDIA_FETCHER` and `HTTP_CLIENT` (used by `fetch_image_dims_uncached`) from function-scope `LazyLock`s to module-scope `DIM_FETCH_MEDIA_FETCHER` / `DIM_FETCH_HTTP_CLIENT`. Force eager init from `OpenAIPreprocessor::new_with_parts` whenever either lightseek hook resolved — so TLS-root / reqwest-init / env- misconfig surfaces at deployment startup instead of crashing the first MM request 20 minutes in. Text-only preprocessors leave the `LazyLock`s dormant; no wasted client allocation. Addresses #9272 review thread (Indrajit, discussion_r3222494711). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ed_content non-image semantics Two related review fixes to `may_be_fix_msg_content`: 1. Guard the flatten branch with `!content_array.is_empty()` so a request with `"content": []` is preserved as the array `[]`, matching pre-PR behavior. Without the guard, an empty array reached the placeholder-flatten path and silently became `""`. New test `test_may_be_fix_msg_content_empty_array_with_placeholder_template` pins the bug-fix shape. 2. Document at the `flatten_mixed_content` boundary that `img_idx` increments for every non-text part (videos / audios too). The currently supported families (Phi-3, LLaVA-1.5) are image-only so it doesn't bite today, but the doc names the caveat so a future image+video family doesn't silently misnumber slots. Addresses #9272 review threads (Indrajit, discussion_r3222451647 and discussion_r3222437490). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Single-line type annotation for the LazyLock<MediaFetcher> static so rustfmt's max-width rule passes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Unit-test doubles construct BaseWorkerHandler via __new__ to skip the __init__ that needs engine_client.vllm_config.model_config.hf_config. Without a class-level default, every test that reaches extract_multimodal_data crashes with AttributeError on the new _use_unified_vision_chunk attr. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
46d52a7 to
58d6adb
Compare
Different query strings now produce different mm_hash values (`?v=1` ≠ `?v=2`). Workloads with rotating signed URLs over a stable object should use --frontend-decoding, which hashes decoded RGB bytes and is URL-agnostic by construction. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…xture Strengthens the pre_merge agg_router smoke to send the same MM image twice and assert usage.prompt_tokens_details.cached_tokens >= 1 on the 2nd response — catches silent regressions to text-prefix-only routing. Range-aware image_server fixture (returns 206 Partial Content on Range: bytes=...) is required so the frontend's strict-206 dim-fetch probe actually engages MM-routing-entry creation, instead of falling back to text-prefix routing on the test's 200-OK responses. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Qwen-VL P/D grid_thw failure path is back to yielding
{"status": "error", "message": msg}, which downstream vllm_processor.py
converts to the standard OpenAI {"error": {...}} envelope and HTTP 500.
The BackendOutput-shape change was driven by aiperf's streaming parser;
filing a follow-up issue to teach aiperf the OpenAI-standard error
shape rather than bend dynamo's error format.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The image-placeholder token id resolution delegates to lightseek's per-model ModelProcessorSpec (in lib/llm/src/preprocessor/lightseek_mm.rs), not a 3-tier resolver we own. Update the launch script comment and the user-facing routing doc to reflect that. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
DYNAMO_MM_TRANSFER and mm_kwargs pre-render only apply to --dyn-chat-processor=vllm (agg_multimodal_router_chat_processor.sh). The default Rust frontend path forwards mm_hashes only — each worker re-processes its own images. Rename the section heading and clarify the intro so readers don't expect transfer config to affect the default launch path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
For models with use_unified_vision_chunk=True (Kimi-K2.5) images live under vllm_mm_data["vision_chunk"], not ["image"]. vLLM matches UUIDs to modality strings, so hardcoding "image" silently fails to bind and forces vLLM back to its content-derived hash — defeating router/worker KV-cache key alignment for Kimi. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…mismatch comment - fetch_image_dims warn log was emitting the full URL, which for data: URIs is the entire base64 image payload. Redact to "data:<mime>;base64, <redacted>" so logs don't bloat / leak request content. - total_image_count guard comment now clarifies the mismatch is reachable only on the URL-passthrough path; the decoded-loader path propagates any dim-fetch error via `?` before mm_hashes forwarding runs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The chat-processor variant implicitly uses DYNAMO_MM_TRANSFER=shm by default but didn't surface it in the env-var block or startup banner. Operators looking at the script couldn't tell the channel was active or how to switch to nixl for cross-node deployments. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Revert f7ecc03 rolled back handlers.py to yield {"status": "error", "message": msg} but didn't catch the two test_vllm_worker_handler tests that the original BackendOutput commit had added. Flip assertions to match the actual chunk shape now being yielded. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Switch Phi-3-vision, LLaVA-1.5-7b, and LLaVA-NeXT-mistral-7b post_merge agg_router profiles to use make_image_payload_cached_tokens — asserts cached_tokens >= 1 on a repeated identical request, so silent regressions to text-prefix-only routing fail the post_merge smoke instead of masquerading as a successful "green"-keyword match. Verified locally (NUM_WORKERS=1 SINGLE_GPU=true): - Phi-3-vision-128k: 2368/2377 cached, MM cache hit 50% - LLaVA-1.5-7b: 592/602 cached, 37 effective cached blocks - LLaVA-NeXT-mistral: 1968/1973 cached, 123 effective cached blocks Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Overview:
Adds a Rust-frontend MM-aware KV routing path for multimodal workloads, using the new
lightseek-mmcargo feature for image token expansion.Closes DIS-1930
Details:
How MM-aware routing works.
The frontend now extracts a structured
mm_routing_infofrom each multimodal request before it picks a worker:URL-passthrough vs frontend-decoding.
Two paths share the same routing logic but differ on where image bytes live:
image_url. Frontend hashes the normalized URL string (cache-buster query params stripped), routes the request, and forwards the raw URL to the chosen worker. Worker fetches and decodes the image.mm_processor_cacheis keyed on decoded-bytes hash, so re-requests of the same URL hit; cross-URL same-content requests don't.--frontend-decoding). Frontend'sMediaLoader(LRU-cached) fetches and decodes the image once, ships the decoded RGB tensor to the chosen worker via NIXL/RDMA, and routes by content hash (xxh3_64over the registeredSystemStoragebytes — seeRdmaMediaDataDescriptor::content_hash()). Worker skips both fetch and decode entirely. Same image content reached through different signed URLs collides on the same routing key.The dim-cache primitive.
Image-token counts depend on the dimensions of each image — but the routing decision happens before the worker has decoded anything. So the frontend has to know
(W, H)for each image at request-dispatch time. To avoid an HTTP Range fetch per image per request, dimensions are cached:moka::future::Cache<u64, (u32, u32)>with 100 k-entry capacity and 24 h TTL. Lock-free reads, sharded write locks — handles concurrent dispatch with no contention bottleneck.try_get_withcollapses concurrent in-flight fetches for the samemm_hashinto a single HTTP request — multiple workers can dispatch the same request in parallel without 4 threads racing for the same 4 KB Range read.The cache is feature-gated and bounded so even unbounded URL pools (e.g. signed-URL refresh, image proxies) don't blow up frontend memory.
Backend behavior unchanged.
This PR only touches the Rust frontend's routing decision. vLLM workers are unmodified — they still receive
image_url(or the decoded RDMA tensor in FD mode) and run their normal preprocessing pipeline. Themm_processor_cachekeying that was already content-based is what makes the routing decision deliver actual cache hits.Where should the reviewer start?
lib/llm/src/preprocessor.rs(660-line diff) — the routing-decision code path, all undercfg(feature = "lightseek-mm"). Key call sites:fetch_image_dims— the moka cache + singleflight pattern; this is what avoids per-request HTTP overhead.mm_routing_infoconstruction in the URL-passthrough branch and the FD branch; the only difference is themm_hashsource (URL string vsdescriptor.content_hash()).lib/llm/src/preprocessor/media/rdma.rs— small (~23 lines added) but load-bearing:content_hash()is what makes FD-path routing collide across (signed) URL changes.lib/llm/src/preprocessor/lightseek_mm.rs(new file) —model_id→ per-VLM-family token-count function dispatch (Qwen3-VL etc.). Thin wrapper overllm-multimodal.lib/llm/Cargo.toml+ workspaceCargo.toml— confirm thelightseek-mmfeature flag is wired the way the team prefers, and thatdefault = ["block-manager", "lightseek-mm"](default-on) is acceptable. Flip todefault = ["block-manager"]for opt-in.components/src/dynamo/vllm/handlers.py— 11-line FD-path error-yield fix; not part of routing logic but blocked aiperf parsing on FD failures, so it's in this PR.docs/features/multimodal/multimodal-kv-routing.md— feature description for users.Upstream PR: smg-project/smg#1459 to avoid pulling the openssl/native-tls toolchain.
Related Issues:
Summary by CodeRabbit
Release Notes
New Features
Documentation
Bug Fixes
Tests