feat(multimodal): vLLM video via shared Modality enum - #1895
Conversation
github-actions
Bot
added
grpc
📝 WalkthroughWalkthroughThis PR adds shared multimodal modality routing across protobufs, gateway assembly, and Python servicers, and adds structural prefix accounting for multimodal placeholder expansion and Qwen3-VL video token layout changes. ChangesShared modality and placeholder prefix propagation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for video inputs in the vLLM multimodal path. It adds an is_video flag to the MultimodalInputs proto and propagates this flag through the Rust gateway and Python servicer. The servicer now dynamically maps tensor keys (e.g., renaming pixel_values to pixel_values_videos) and configures field modalities as "video" when the flag is set. Additionally, the Rust gateway's validation is updated to allow video inputs, and unit tests are added to verify the round-trip serialization of the new flag. There are no review comments, so I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
👋 The PR description doesn't fully follow
Please update the PR description so reviewers have the context they need. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3abdb31627
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # expects the pixel tensor under a modality-specific key. The router sends | ||
| # the generic ``pixel_values`` field; rename it to ``pixel_values_videos`` | ||
| # for the video path (grid/size tensors already carry video-specific keys). | ||
| mm_modality = "video" if mm_proto.is_video else "image" |
There was a problem hiding this comment.
When smg-grpc-servicer is upgraded in an environment that already has smg-grpc-proto==0.4.11 installed, the declared dependency in grpc_servicer/pyproject.toml still considers that old generated proto package valid, but its MultimodalInputs class does not have this newly added is_video field. This access then raises AttributeError for any vLLM multimodal request, including image-only requests, before the servicer can build inputs; bump the proto package/version floor (and release version) or use a compatible fallback.
Useful? React with 👍 / 👎.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| backend: &str, | ||
| ) -> Result<()> { | ||
| match intermediate.modality { | ||
| Modality::Image | Modality::ImageEmbeds | Modality::Video => Ok(()), |
There was a problem hiding this comment.
🟡 Nit: The previous ensure_image_only used != Modality::Image, which rejected ImageEmbeds on the vLLM path. This new match now accepts ImageEmbeds in addition to Video — a silent relaxation that isn't mentioned in the PR description. Other backends (TokenSpeed) do treat Image | ImageEmbeds together, so this is likely correct, but worth confirming it was intentional rather than accidental.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
slin1237
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6354aa65d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| message MultimodalItem { | ||
| Modality modality = 1; | ||
| smg.grpc.common.Modality modality = 1; |
There was a problem hiding this comment.
When the TokenSpeed encode-stage servicer handles any EncodeRequest, this field now comes from smg.grpc.common.Modality, so the generated tokenspeed_scheduler_pb2 module no longer defines IMAGE, VIDEO, or MODALITY_UNSPECIFIED; those constants live in common_pb2. The unchanged grpc_servicer/smg_grpc_servicer/tokenspeed/encoder_servicer.py still reads tokenspeed_scheduler_pb2.IMAGE/VIDEO in _items_from_proto, so the first encode RPC raises AttributeError before it can enqueue the item. Please update that servicer to use the common enum constants (or numeric-compatible comparisons) together with this proto move.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@grpc_servicer/smg_grpc_servicer/tokenspeed/servicer.py`:
- Around line 435-436: Remove the fallback literals in the shared-enum lookups
inside the token speed servicer; in the code paths that set `image_modality`,
`video_modality`, and `audio_modality` in `servicer.py`, use direct
`common_pb2.IMAGE` / `common_pb2.VIDEO` / `common_pb2.AUDIO` access instead of
`getattr(..., default)`. This keeps `TokenspeedServicer` aligned with the direct
enum access pattern used elsewhere (for example in `vllm/servicer.py`) and
ensures any proto/stub mismatch fails immediately rather than being silently
masked.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 912f5bf7-d502-461d-96d2-805e975110a7
📒 Files selected for processing (7)
crates/grpc_client/proto/common.protocrates/grpc_client/proto/tokenspeed_scheduler.protocrates/grpc_client/proto/vllm_engine.protogrpc_servicer/smg_grpc_servicer/tokenspeed/servicer.pygrpc_servicer/smg_grpc_servicer/vllm/servicer.pymodel_gateway/src/routers/grpc/multimodal/assemble.rsmodel_gateway/src/routers/grpc/proto_wrapper.rs
Enable video inputs on the vLLM gRPC path, previously rejected by ensure_image_only. Video pixel tensors ride the same inline/SHM transport as images (#1893), routed to vLLM's video modality. Instead of a one-off is_video bool, hoist the Modality enum into common.proto (like ShmHandle in #1) so the single-modality, precomputed-tensor engines (vLLM + TokenSpeed) share one modality type; TokenSpeed's proto now references smg.grpc.common.Modality. (SGLang keeps its string `modalities` for mixed-modality inputs; converging it onto the common enum is a follow-up.) - proto: Modality enum -> common.proto; tokenspeed references it; vLLM MultimodalInputs gets `common.Modality modality = 10` - assemble: vLLM accepts image or video; assemble_vllm maps the modality and takes mm_hashes from videos vs images. Mixed rejected upstream in process. - servicer: video -> pixel_values_videos under vLLM's `video` MultiModalFieldConfig; tokenspeed servicer reads modality from common_pb2 - test: modality proto round-trip Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com>
|
Addressed the CI failure + review feedback (pushed
Local: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@grpc_servicer/smg_grpc_servicer/vllm/servicer.py`:
- Around line 617-624: The modality routing in `servicer.py` currently treats
any non-VIDEO value as image, so add explicit validation in the
`mm_modality`/`is_video` branch and fail fast for unsupported `common_pb2`
modalities. Mirror the behavior used by `_modality_from_proto` and
`encoder_servicer.py` by checking only the supported image/video enum values
before assigning the modality-specific key, and raise an error for anything else
instead of defaulting to image.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f61ba6d3-6dc9-4dc1-99ea-36f81aec7a2c
📒 Files selected for processing (11)
crates/grpc_client/proto/common.protocrates/grpc_client/proto/tokenspeed_scheduler.protocrates/grpc_client/proto/vllm_engine.protocrates/grpc_client/python/pyproject.tomlcrates/grpc_client/src/lib.rsgrpc_servicer/pyproject.tomlgrpc_servicer/smg_grpc_servicer/tokenspeed/encoder_servicer.pygrpc_servicer/smg_grpc_servicer/tokenspeed/servicer.pygrpc_servicer/smg_grpc_servicer/vllm/servicer.pymodel_gateway/src/routers/grpc/multimodal/assemble.rsmodel_gateway/src/routers/grpc/proto_wrapper.rs
| num_items = len(mm_proto.mm_placeholders) | ||
|
|
||
| # Image vs video: vLLM routes each modality to a different encoder and | ||
| # expects the pixel tensor under a modality-specific key. The router sends | ||
| # the generic ``pixel_values`` field; rename it to ``pixel_values_videos`` | ||
| # for the video path (grid/size tensors already carry video-specific keys). | ||
| is_video = mm_proto.modality == common_pb2.VIDEO | ||
| mm_modality = "video" if is_video else "image" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Silently treats unknown/unsupported modality as image instead of failing fast.
is_video only checks for VIDEO; any other value (e.g. AUDIO, or a future enum member) falls through to mm_modality = "image" without validation. TokenSpeed's _modality_from_proto and encoder_servicer.py's dispatch both explicitly raise for unsupported modalities — this path should follow the same convention rather than silently mis-routing.
🛡️ Proposed fix
- is_video = mm_proto.modality == common_pb2.VIDEO
+ if mm_proto.modality not in (
+ common_pb2.MODALITY_UNSPECIFIED,
+ common_pb2.IMAGE,
+ common_pb2.VIDEO,
+ ):
+ raise ValueError(
+ f"vLLM multimodal path modality={mm_proto.modality} is not supported"
+ )
+ is_video = mm_proto.modality == common_pb2.VIDEO
mm_modality = "video" if is_video else "image"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| num_items = len(mm_proto.mm_placeholders) | |
| # Image vs video: vLLM routes each modality to a different encoder and | |
| # expects the pixel tensor under a modality-specific key. The router sends | |
| # the generic ``pixel_values`` field; rename it to ``pixel_values_videos`` | |
| # for the video path (grid/size tensors already carry video-specific keys). | |
| is_video = mm_proto.modality == common_pb2.VIDEO | |
| mm_modality = "video" if is_video else "image" | |
| num_items = len(mm_proto.mm_placeholders) | |
| # Image vs video: vLLM routes each modality to a different encoder and | |
| # expects the pixel tensor under a modality-specific key. The router sends | |
| # the generic ``pixel_values`` field; rename it to ``pixel_values_videos`` | |
| # for the video path (grid/size tensors already carry video-specific keys). | |
| if mm_proto.modality not in ( | |
| common_pb2.MODALITY_UNSPECIFIED, | |
| common_pb2.IMAGE, | |
| common_pb2.VIDEO, | |
| ): | |
| raise ValueError( | |
| f"vLLM multimodal path modality={mm_proto.modality} is not supported" | |
| ) | |
| is_video = mm_proto.modality == common_pb2.VIDEO | |
| mm_modality = "video" if is_video else "image" |
🤖 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 `@grpc_servicer/smg_grpc_servicer/vllm/servicer.py` around lines 617 - 624, The
modality routing in `servicer.py` currently treats any non-VIDEO value as image,
so add explicit validation in the `mm_modality`/`is_video` branch and fail fast
for unsupported `common_pb2` modalities. Mirror the behavior used by
`_modality_from_proto` and `encoder_servicer.py` by checking only the supported
image/video enum values before assigning the modality-specific key, and raise an
error for anything else instead of defaulting to image.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3401545d29
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| message MultimodalItem { | ||
| Modality modality = 1; | ||
| smg.grpc.common.Modality modality = 1; |
There was a problem hiding this comment.
This moves the public modality field to smg.grpc.common.Modality, but the Python package's lazy public surface still omits common_pb2 (crates/grpc_client/python/smg_grpc_proto/__init__.py:8-23). For Python clients that previously used tokenspeed_scheduler_pb2.IMAGE/VIDEO, the generated scheduler module no longer defines those names, and from smg_grpc_proto import common_pb2 raises AttributeError, so they have no documented named constants to populate the field. Please add common_pb2 to _GENERATED_MODULES/__all__.
Useful? React with 👍 / 👎.
| requires-python = ">=3.10" | ||
| dependencies = [ | ||
| "smg-grpc-proto>=0.4.11", | ||
| "smg-grpc-proto>=0.4.13", |
There was a problem hiding this comment.
At this dependency floor change, the project version above remains 0.6.0. The checked release workflow runs on grpc_servicer/pyproject.toml changes (.github/workflows/release-grpc.yml:7-9,38-39) and uploads with twine upload ... --skip-existing (.github/workflows/release-grpc.yml:163-165); if 0.6.0 is already published, the new wheel is skipped, so pip install -U smg-grpc-servicer will not get the proto floor or servicer fixes. Please bump the servicer package version with this change.
Useful? React with 👍 / 👎.
vLLM's Qwen3-VL derives mrope positions by walking one `<|vision_start|>..<|vision_end|>` block per temporal frame (`_iter_mm_grid_hw` loops `for _ in range(t)`), so a video needs `t` per-frame vision blocks and its placeholder range must start on the leading `<|vision_start|>`. Two gaps broke this: - The per-frame video layout was gated to the Qwen3.5 family; base Qwen3-VL fell back to a single flat pad block, so any multi-frame video crashed with "vision_start_token_id not in list" on frame 2. Apply the per-frame layout to the whole family (same processor since #1563) and rename the helper accordingly. - expand_tokens recorded the placeholder range starting *after* the template's leading `<|vision_start|>`, so vLLM's per-frame scan skipped the first marker (breaking even single-frame video). Add `PromptReplacement::structural_prefix` so a spec can fold N preceding template tokens into the reported range; Qwen3-VL video sets it to 1. Verified live on a Qwen3-VL-8B vLLM worker: a red->blue test video now returns "red, blue" over both inline and /dev/shm transport, no crash. Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
https://github.com/lightseekorg/smg/blob/f85e336e2c1b3b52b55e894be94a6aa0e4959ed3/crates/multimodal/src/registry/qwen3_vl.rs#L115
When the decoder clamps sampling to min_frames/max_frames (for example, default clips longer than 384s are capped at 768 frames), the effective sampled FPS is no longer 2.0. Hard-coding 2.0 here makes the <x.y seconds> tokens understate or overstate the real frame times, so Qwen3-VL temporal prompts point at the wrong moments for short or long videos; carry the sampled timing/frame indices from decode instead of assuming the default.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| /// template already supplies the outer `<|vision_start|>`/`<|vision_end|>`, so | ||
| /// this emits only the inner per-frame structure (hence the `grid_idx > 0` | ||
| /// guards that reuse the template's opener/closer for the first/last frame). |
There was a problem hiding this comment.
For Qwen3-VL video requests with grid_t > 1, reusing the chat template's outer <|vision_start|>/<|vision_end|> drops the inner start marker for the first frame and the inner end marker for the last frame. The Qwen3VL processor expands the bare video pad into timestamp + vision_start + video pads + vision_end for every temporal frame while preserving the outer wrapper, so this produces different prompt tokens/MRoPE positions and can corrupt all vLLM video results for this model family.
Useful? React with 👍 / 👎.
1 participant
What this changes
This lets chat requests include video when using the vLLM inference engine. Before this change the vLLM path accepted still images only and rejected any request that contained a video, even though the rest of the system was already able to handle video.
Why it was needed
The image and video paths were almost identical, but two problems stopped video from working:
Videos have to be described frame by frame. The engine expects one marked section per moment in time. The code only produced this frame-by-frame layout for one newer model variant and fell back to a single combined block for the main Qwen video model. A single block made the engine fail the moment a video had more than one frame.
The marked region for the video started one step too late. It began just after the "start of picture" marker instead of on it. The engine looks for that marker to line up each frame, so it never found the first one. This broke even single-frame videos.
What was changed
Scope: this focuses on the vLLM engine. The other engine (SGLang) is intentionally left for a later change, audio is still not handled on this path, and it is one video per request.
Testing
Verified live on a machine with four graphics cards running the Qwen vision model (Qwen3-VL):
How the two data-delivery methods compare
The image or video data has to be handed from the front door of the system to the engine. There are two ways to do it: packed into the request (the data is copied into the message), or shared memory (the data is written once to an area both sides can see, and only a small pointer is sent). While testing, I measured both.
One request at a time — shared memory is faster, and the gap grows with data size:
Shared memory is consistently about 15% faster per request, and the time saved grows with the amount of data, because it skips copying that data into and out of the message.
Many requests at once:
Takeaway: shared memory gives a clear per-request speed-up that grows with data size and frees up processor time. A gain in total throughput only appears when moving the data — rather than preparing the image or the graphics card itself — is the bottleneck. That is most likely with very large media (long videos) or when the engine runs on a separate machine.
Summary by CodeRabbit
modalitysupport to route image vs video through the gRPC pipeline (including vLLM assembly).