Repository navigation
fix(servicer): media-processing follow-ups to #2624, #2626 and #2627 - #2645
hello-alexmcc wants to merge 4 commits into
Conversation
…edia vllm.multimodal.video is not a module on the vLLM this servicer targets (VideoMediaIO is re-exported from vllm.multimodal.media), so the default frame count was never found and the budget was written in its place, which for a budget above vLLM's default of 32 raised sampling instead of capping it. The import is corrected, an unknown default with no explicit count now leaves sampling untouched with one warning, and the test stub registers the module production imports. Signed-off-by: Alex McC <319643551+hello-alexmcc@users.noreply.github.com>
…r probes smg-grpc-proto 0.4.21 is the first release with GetServerInfoResponse.mm_processor_source (18) and GenerateComplete.media_identity (15); the servicer now requires it, as 0.13.0, and sets both fields without probing the descriptor. The proto contract tests cover both fields, and GetServerInfo names the processor mode's source in a servicer test. Also from that review: a blank flag or environment value is unset, consistently; client-visible messages lead with --mm-processor; the sidecar's --mm-sidecar-timeout-ms, which applied to nothing (each job carries its deadline), is gone with the README claim built on it. Signed-off-by: Alex McC <319643551+hello-alexmcc@users.noreply.github.com>
…not build build_media_identity ran after the engine had answered and could raise on shapes it did not anticipate (a missing prompt_token_ids key, mixed grid dtypes, a non-tensor element), turning a served request into a client error. The identity is an optimisation with a fallback, decode reprocessing the media, so the build is now guarded: a failure logs one warning with the request id and the leg answers without it. Signed-off-by: Alex McC <319643551+hello-alexmcc@users.noreply.github.com>
…identity vLLM's Qwen-VL processors emit the video timing as second_per_grid_ts, the Qwen-Omni family and the gateway's own processors as video_second_per_grid; the identity relay listed only the first, so a worker-processed video's timing could be dropped from the decode leg and its M-RoPE positions rebuilt without it. The servicer's GRID_KEYS carries both, with a test per spelling; the router's mirror (VLLM_MROPE_GRID_KEYS) lands with the router follow-ups. Signed-off-by: Alex McC <319643551+hello-alexmcc@users.noreply.github.com>
github-actions
Bot
added
documentation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (11)
Included review availability: 6 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 8 reviews per hour. 📝 SummarySummary by CodeRabbit
WalkthroughThe changes update processor-setting resolution and video-frame handling, adjust media-identity construction and proto-field handling, and remove sidecar timeout configuration. ChangesMedia Processing and Identity
Sidecar Deadline Settings
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to No actionable issue is established that should block merging. Normal validation remains appropriate for the updated media-processing paths. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 26.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 50 functions across 9 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
👋 The PR description doesn't fully follow
Please update the PR description so reviewers have the context they need. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| help="the worker's job timeout, for the startup log (falls back to " | ||
| "SMG_VLLM_MM_SIDECAR_TIMEOUT_MS)", | ||
| ) | ||
| parser.add_argument("--concurrency", type=int, default=2) |
There was a problem hiding this comment.
🟡 Nit: Dropping --mm-sidecar-timeout-ms from the parser outright is a hard break for existing sidecar launch commands: argparse exits at startup with "unrecognized arguments" after an upgrade. The flag was informational-only, so the removal is semantically safe, but elsewhere this PR gives env vars a deprecation release ("env support ends in the next minor release"). Consider accepting the flag as a suppressed no-op for one release (e.g. parser.add_argument("--mm-sidecar-timeout-ms", type=int, default=None, help=argparse.SUPPRESS) plus a deprecation warning when set) before removing it.
| # the video timing that goes with them, in both spellings: `second_per_grid_ts` | ||
| # from the Qwen-VL processors, `video_second_per_grid` from the Qwen-Omni | ||
| # family and the router's own processors. The router's VLLM_MROPE_GRID_KEYS | ||
| # mirrors this list. | ||
| GRID_KEYS = ("image_grid_thw", "video_grid_thw", "second_per_grid_ts", "video_second_per_grid") |
There was a problem hiding this comment.
🟡 Nit: "The router's VLLM_MROPE_GRID_KEYS mirrors this list" is ahead of the tree: model_gateway/src/routers/grpc/proto_wrapper.rs:1278 still has the three-key list (video_second_per_grid is in the router follow-ups PR, per the description). Since merge order isn't guaranteed, consider phrasing it as intent ("the router's VLLM_MROPE_GRID_KEYS is kept in sync with this list") or referencing the follow-up, so the comment doesn't read as a broken cross-reference if this lands first. No functional concern either way — I verified apply_media_identity relays the identity batches verbatim, so an old router tolerates the new key.
There was a problem hiding this comment.
Full review of the servicer-side media-processing follow-ups. No blocking issues found: 0 🔴 Important, 2 🟡 Nit, 0 🟣 Pre-existing (inline).
Verified along the way:
- The guarded
build_media_identitycorrectly initializesmedia_identity = Noneand theexcept Exceptioncannot swallow cancellation (CancelledErroris aBaseException); the new PD test covers the fallback path. - Dropping the descriptor probes is safe given the
smg-grpc-proto>=0.4.21floor, and the router-side WARN for older servicers is kept. clamp_video_frameswith an unknown vLLM default now leaves sampling untouched (a cap must never raise sampling) and warns once at construction — both the worker and the sidecar call it only at build time, so no log spam.- Blank flag/env handling (
_blank,resolve,resolve_mm_processor_mode) is consistent: blank falls through flag → env → default, with tests for both layers. - The new error message's
--mm-processingcross-reference exists on the router (model_gateway/src/main.rs), and an old router relays the newvideo_second_per_grididentity key verbatim (apply_media_identityusesclone_from), so the cross-PR ordering with the router follow-ups is safe.
Nits: the hard removal of the sidecar's --mm-sidecar-timeout-ms flag breaks existing launch commands at argparse without the deprecation grace the PR gives env vars, and the VLLM_MROPE_GRID_KEYS mirror comment is ahead of the tree until the router follow-up lands.
Also posted separately: the PR description is missing the template's ### Problem / ### Solution headers.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Closing in favour of one-concern-per-PR splits, at the maintainer's request. The requested changes were also re-judged on their merits rather than carried over wholesale; what survived:
Dropped, with reasons: the servicer capability-probe changes (#2640's author's call); |
1 participant
Description
Split out of #2642 (items 2–5, servicer side) from the reviews of #2624, #2626 and #2627 that merged without them. Every live fact below is the reviewer's observation quoted from the write-up; none of this was run against a cluster. The capability probe from #2640 (
mm_salt.py) is left exactly as merged.2.
VideoMediaIOimport and the frame budget (from #2624)The default was never found, so with no explicit
num_framesthe budget was written in its place, which for a budget above vLLM's 32 raised sampling.vllm_default_video_framesimports fromvllm.multimodal.media(theexcept Exceptionfallback stays); with the default unknown and nothing set,clamp_video_framesleaves sampling untouched and logs one WARNING. The construction-test stub registersvllm.multimodal.media/.media.video, so the test exercises the production import; a new test covers the unknown-default case withnum_framesleft absent.3. Proto floor, servicer version, field tests (from #2626)
smg-grpc-proto>=0.4.21with the annotated comment (0.4.21 is the first release with bothGetServerInfoResponse.mm_processor_source(18) andGenerateComplete.media_identity(15): #2627 did not bump, pergit log -- crates/grpc_client/proto/vllm_engine.proto), servicer 0.13.0. Both descriptor probes are gone:GetServerInfosetsmm_processor_sourcewhenever a processor is advertised,_complete_responsesetsmedia_identitywhenever one was built,media_identity_supported()is deleted (the router-side WARN for an older servicer stays, that one is legitimate).test_media_refs_proto_fields.pycovers fields 18 and 15 (number, default, round trip); a servicer test assertsGetServerInfo(...).mm_processor_source == "flag"/"env"(vLLM lane).Also from that review: a blank flag or env value (
--mm-redis-url "",SMG_VLLM_MM_PROCESSOR=" ") is unset, consistently; client-visible strings lead with--mm-processor; the sidecar's--mm-sidecar-timeout-msapplied to nothing (each job carries itsdeadline_ms), so it is removed together with the README sentence built on it. Kept on purpose:e2e_test/infra/worker.pystill setsSMG_VLLM_MM_PROCESSOR: the upstream launcher (vllm/entrypoints/launchers/grpc_server.pyon vLLM main, checked 2026-09-23) has no--mm-*flags yet, so the e2e deprecation line stays until the launcher change lands, and env support cannot end in the next minor release before it does.4. Guarded identity build (from #2627)
The call is wrapped: a failure logs
Request %s: media identity not built (%s); the decode leg will reprocess the mediaand the leg answers without an identity. Test intest_vllm_pd_media_identity.py(vLLM lane):build_media_identityraises, the request still completes.5. Both video timing names, servicer side (from #2627)
Verified in vLLM main: the Qwen-VL processors (
qwen2_5_vl.py,qwen3_vl.py) usesecond_per_grid_ts, the Qwen-Omni family (qwen2_5_omni_thinker.py,qwen3_omni_moe_thinker.py) and this gateway's processorsvideo_second_per_grid.GRID_KEYScarries both, with a video case per spelling. The router's mirror (VLLM_MROPE_GRID_KEYS) is in the router follow-ups PR; each side tolerates the other's version.Changes
grpc_servicer/smg_grpc_servicer/vllm/mm_processor.py(2, 3),vllm/servicer.py(3, 4),vllm/media_identity.py(3, 5),vllm/mm_sidecar.py,README.md,pyproject.toml(3).test_vllm_mm_processor.py,test_media_refs_proto_fields.py,test_vllm_mm_sidecar.py,test_vllm_media_identity.py,test_vllm_pd_media_identity.py.Test Plan
Each item's test was written first and watched fail before the fix.
uv venv .venv && uv pip install … -e crates/grpc_client/python -e grpc_servicer …): 413 passed, 12 skipped (the vLLM-gated tests, including the three new ones intest_vllm_mm_processor.py::TestServicerWiringandtest_vllm_pd_media_identity.py, run on the vLLM e2e lane).media_refs=1, decode logmedia_refs=0, preprocessed_mm=True, only the prefill sidecar sees the job; the same with a short video;SMG_VLLM_MM_MAX_VIDEO_FRAMES=16on the sidecar container caps decoding at 16 frames, unset leaves it as vLLM decides;/workersshowsmm_processor_sourcefor an env-configured servicer.Checklist
cargo +nightly fmtpasses (no Rust change)cargo clippy --all-targets -- -D warningspasses (no Rust change)