fix(vllm): support unified multimodal routing - #10375
Conversation
|
👋 Hi maheshrbapatu! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
b0a45db to
42f9522
Compare
|
Linter diff in the way? Review this PR in Change Stack to focus on meaningful changes and expand context only when needed. WalkthroughThis PR adds multimodal feature metadata grouping and single-modality transfer selection to the frontend, and backend support for consuming grouped metadata with fallback UUID namespacing. Frontend vLLM processor extracts per-modality hashes/placeholders and gates mm_kwargs transfer to single-modality requests. Backend handlers normalize modality keys, parse modality-specific values from extra_args, reconstruct fallback UUIDs with namespace prefixes, and validate placeholder embed-mask correctness. ChangesMultimodal Feature Metadata Extraction and Transfer Selection
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
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: 3
🧹 Nitpick comments (1)
components/src/dynamo/frontend/tests/test_vllm_processor_unit.py (1)
144-216: ⚡ Quick winMove the new
vllm_processorimports to the top of the file.The added tests introduce several unguarded method-local imports of
dynamo.frontend.vllm_processor. These are not the optional-dependency pattern called out in repo learnings, so they should follow the normal module-level import rule instead.As per coding guidelines, "Keep all imports at the top of the file; flag any import inside functions/classes/methods."
🤖 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 `@components/src/dynamo/frontend/tests/test_vllm_processor_unit.py` around lines 144 - 216, Multiple tests perform local imports of dynamo.frontend.vllm_processor (e.g., _group_mm_feature_metadata, _single_transfer_modality) inside test methods; move these imports to the module level at the top of components/src/dynamo/frontend/tests/test_vllm_processor_unit.py, replacing the per-test "from dynamo.frontend.vllm_processor import _group_mm_feature_metadata" (and any other similar lines) with a single top-of-file import listing the needed symbols, and remove the in-function imports so tests use the module-level names.
🤖 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/frontend/tests/test_vllm_processor_unit.py`:
- Around line 130-132: The default argument for data in _feature is using
object() at import time (Ruff B008); replace it with a module-level sentinel
(e.g., _SENTINEL = object()) and change the signature to use that sentinel, then
inside _feature treat _SENTINEL as “no value” and set the intended default
runtime value. Also consolidate repeated local imports from
dynamo.frontend.vllm_processor used in TestMultimodalFeatureMetadata (and other
tests) by moving them to the top of the file so tests reuse the top-level
imports instead of importing inside individual test methods.
In `@components/src/dynamo/frontend/vllm_processor.py`:
- Around line 76-77: The logging in _prepare_mm_routing uses f.modality directly
which can raise AttributeError for legacy MM features; update
_prepare_mm_routing to call the helper _mm_feature_modality(feature) (or getattr
fallback) instead of accessing f.modality, and use that returned string in the
log and any subsequent logic (replace uses of f.modality with the result of
_mm_feature_modality for features named e.g., f or feature).
In `@components/src/dynamo/vllm/handlers.py`:
- Around line 2058-2061: The branch currently only calls
_compute_mm_uuids(multi_modal_data) when self.embedding_loader is None, which
incorrectly assumes presence of an embedding_loader means the request didn't
include raw MM payload; change the logic so that when forwarded_mm_uuids is
falsy you always compute mm_uuids from the actual payload (multi_modal_data)
regardless of self.embedding_loader (i.e., replace the conditional on
self.embedding_loader with an unconditional else that calls
_compute_mm_uuids(multi_modal_data)), ensuring
_extract_multimodal_data/image_loader paths still produce router-aligned MM
cache identities.
---
Nitpick comments:
In `@components/src/dynamo/frontend/tests/test_vllm_processor_unit.py`:
- Around line 144-216: Multiple tests perform local imports of
dynamo.frontend.vllm_processor (e.g., _group_mm_feature_metadata,
_single_transfer_modality) inside test methods; move these imports to the module
level at the top of
components/src/dynamo/frontend/tests/test_vllm_processor_unit.py, replacing the
per-test "from dynamo.frontend.vllm_processor import _group_mm_feature_metadata"
(and any other similar lines) with a single top-of-file import listing the
needed symbols, and remove the in-function imports so tests use the module-level
names.
🪄 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: c4217d9f-1c48-4345-a475-785f65f38df7
📒 Files selected for processing (4)
components/src/dynamo/frontend/tests/test_vllm_processor_unit.pycomponents/src/dynamo/frontend/vllm_processor.pycomponents/src/dynamo/vllm/handlers.pycomponents/src/dynamo/vllm/tests/test_vllm_video_handler.py
42f9522 to
f0f9c5d
Compare
6d6ae70 to
557ad08
Compare
|
@krishung5 to help review |
There was a problem hiding this comment.
Pull request overview
This PR fixes Dynamo’s vLLM multimodal routing/forwarding logic to support unified multimodal models (e.g., Gemma4-12B) by preserving canonical multimodal hash strings while carrying modality information via modality-keyed metadata, with a legacy image-only fallback.
Changes:
- Frontend: groups forwarded multimodal hashes/placeholders by modality (while keeping legacy flat
mm_hashes/mm_placeholdersfor image-only requests). - Backend: consumes modality-keyed multimodal UUIDs/hashes and normalizes the unified vision-chunk modality mapping (
image→vision_chunkwhen configured). - Tests: adds focused unit coverage for grouping behavior, legacy fallback behavior, unified vision-chunk remapping, and placeholder
is_embedpreservation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| components/src/dynamo/frontend/vllm_processor.py | Builds and forwards modality-keyed multimodal metadata; skips mm_kwargs transfer for mixed-modality requests. |
| components/src/dynamo/vllm/handlers.py | Adds helpers to normalize modality and consume grouped multimodal hashes/placeholders in prompt building and mm_kwargs reception. |
| components/src/dynamo/frontend/tests/test_vllm_processor_unit.py | Adds unit tests for modality grouping, legacy fallback behavior, and mixed-modality transfer behavior. |
| components/src/dynamo/vllm/tests/test_vllm_video_handler.py | Adds unit tests validating backend prompt UUID construction, unified vision-chunk fallback, image UUID computation, and placeholder is_embed handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/ok to test d4a2807 |
krishung5
left a comment
There was a problem hiding this comment.
Thanks for the PR! Left some comments.
Preserve modality-keyed multimodal routing metadata for vLLM unified multimodal requests instead of treating every forwarded multimodal hash as an image hash. This is needed for Gemma4-12B style unified multimodal models, where image, video, and audio features can share the same vLLM request path. The forwarded multimodal hash/UUID is cache identity for routing, so it must remain keyed by the actual modality instead of being collapsed into the legacy image bucket. Keep the legacy image-only flat metadata path for existing callers while forwarding grouped multi_modal_uuids to the vLLM backend. Update the vLLM handler to consume modality-keyed multimodal UUIDs and add focused coverage for frontend metadata grouping plus backend video routing behavior. Validation: targeted frontend/vLLM unit tests pass. AIPerf validation on Qwen/Qwen3-VL-2B-Instruct-FP8 shows existing image workloads remain within run-to-run variance; baseline video/unified multimodal workloads fail on origin/main and pass with this patch. This commit does not claim a throughput improvement. Signed-off-by: maheshrbapatu <maheshbapatu@gmail.com>
test_vllm_processor_unit.py imported dynamo.frontend.vllm_processor at module top level to cover the new unified-multimodal helpers. The pytest-marker-report pre-commit hook collects every test without executing it, so that top-level import ran vllm_processor's `from vllm.tasks import ...` and `from vllm.v1.engine.parallel_sampling import ...` during collection. Those submodules are not in the hook's vllm stub list, so collection failed with ModuleNotFoundError, failing pre-commit / pre-merge-status-check. Move the helper imports back inside the individual test methods (matching how the file and the vllm_processor_module fixture already import the module), so collection no longer triggers the unstubbed vllm submodule imports. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: maheshrbapatu <maheshbapatu@gmail.com>
The black pre-commit hook reformats the _get_modality_extra_values assertion (removing redundant parentheses). Formatting only; no behavior change. Fixes the remaining pre-commit / pre-merge-status-check failure. Signed-off-by: maheshrbapatu <maheshbapatu@gmail.com>
Signed-off-by: maheshrbapatu <maheshbapatu@gmail.com>
d4a2807 to
120b344
Compare
|
/ok to test 120b344 |
|
/ok to test b01e7c0 |
Signed-off-by: maheshrbapatu <maheshbapatu@gmail.com>
|
@krishung5 The branch is now synced with current |
|
/ok to test f34688e |
dynamo-ops
left a comment
There was a problem hiding this comment.
Code LGTM. The only changes since last approval are merge-from-main commits — no functional code changes. CI is green across all 64 checks.
|
Thanks so much for all the help with the PR, @krishung5! |
krishung5
left a comment
There was a problem hiding this comment.
Thanks for the contribution!
Summary
Support modality-aware multimodal routing metadata for Dynamo's vLLM backend path.
Gemma 4 unified multimodal models can carry image, video, and audio features through the same request path. The previous forwarding path treated multimodal cache identity as image-only, which could produce incorrect KV-cache routing and backend metadata.
multi_modal_uuidsdictionary key.is_embedfeature mask in the shared routing utility and make the multimodal router E2E model-independent.Validation
git diff --check: passed.33 passed.google/gemma-4-12B-itrouter E2E,test_vllm_mm_overlap_all[shm]:1 passedwith all 29 overlap observations succeeding.Where should the reviewer start?
components/src/dynamo/common/multimodal/routing_utils.pycomponents/src/dynamo/frontend/vllm_processor.pycomponents/src/dynamo/vllm/handlers.pytests/mm_router/test_vllm_mm_router_e2e.pyRelated Issues