Repository navigation
fix(sglang): reject media for text-only engines - #15520
Conversation
A text-only chat template can silently discard media input. Publish a text-only capability only when the resolved SGLang engine explicitly reports is_multimodal=False, and reject undeclared media before dispatch in the Rust, SGLang and vLLM frontend paths. Missing or malformed declarations preserve existing behavior; multimodal and unknown engines make no declaration. Co-authored-by: Neil <neil@reflection.ai> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Travis Wilson <travis@reflection.ai> Signed-off-by: Neil <neil@reflection.ai>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: ai-dynamo/dynamo/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (9)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 1 remain after this review. WalkthroughThe change adds runtime input-modality metadata and validation in SGLang and vLLM frontend processors and the Rust preprocessor. When a modality declaration is available, these checks reject media inputs that it does not include. ChangesInput modality validation
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change rejects media sent to models declared text-only and leaves existing behavior unchanged otherwise. No merge-blocking risk was identified. 🚥 Pre-merge checks | ✅ 3 | ❌ 1 | ❓ 1❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 51.61% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 31 functions across 8 files. (1 skipped: 1 too large.)
Comment |
|
👋 Hi neilkg! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
Include canonical input modality declarations in the model-card checksum so incompatible workers cannot reuse a frontend processor's admission policy. Preserve legacy checksums for missing or malformed declarations and ignore ordering and duplicate entries in valid modality sets. Cover checksum boundaries and discovery admission, and run SGLang's missing engine-metadata assertions once outside the flag matrix. Signed-off-by: Neil <neil@reflection.ai>
Keep Null for malformed non-array input and a mixed-type array for the separate malformed-array path. The string input duplicated the Null case. Signed-off-by: Neil <neil@reflection.ai>
Signed-off-by: Neil <neil@reflection.ai>
Signed-off-by: Neil <neil@reflection.ai>
|
/ok to test bd8d601 |
|
chatted with AI and it seems that there is a rollout issue that we need to think of -- Because the declaration participates in |
✅ Dynamo PR CI passed — run 37542152048 (attempt 2) on
|
|
@neilkg following up on the third item in my earlier comment (rollout note) with the concrete impact, so it can go into the PR description / release notes. Which models are affected The worker publishes
Request experience For affected models, a chat request containing Rollout experience The declaration participates in
This is a one-time transition per deployment. Two ways to avoid it:
|
Remove the worker-published input_modalities runtime key, its model-card checksum entry, and the Rust and Python frontend rejection. The checksum entry changes the card of every text-only SGLang worker on upgrade, which splits same-namespace rollouts. The next commit moves the rejection into the SGLang worker instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: furionw <qiwa@nvidia.com>
SGLang's Engine API skips the HTTP server's text-only media check and silently drops image, video, and audio input when the model is not multimodal, so a request returned a 200 answer that never saw its media. The aggregated, prefill, and decode handlers now raise InvalidArgument before loading media when the engine reports is_multimodal=False. The diffusion LM handler never forwards media, so it rejects media unconditionally. Multimodal and unknown engines are unchanged, and no model-card field changes, so upgrades do not split worker sets. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: furionw <qiwa@nvidia.com>
|
/ok to test b98028e |
|
@neilkg heads-up: to land this before today's code freeze, I pushed two commits to your branch that change the approach.
Why: the checksum entry changed the card of every text-only SGLang worker, which splits same-namespace rolling upgrades for most popular text-only models (new workers parked, then a 404 gap). Rejecting in the worker gives the same user-facing fix, a 400 instead of a 200 that silently ignored the media, without a new card contract, and it works behind any frontend version. Behavior delta vs. your version: non-streaming clients still get the 400. Streaming clients now get an error event after the initial 200 instead of a 400, the same as other worker-side rejections such as TRT-LLM's text-only check. Multimodal and unknown engines are unchanged. This supersedes my earlier review items and the rollout note above, since there is no checksum change anymore. I also updated the title and description to match. The diagnosis and the original fix are yours. Thank you! Verification: pre-commit passes, and the new helper logic was exercised locally. The new unit tests (text-only, multimodal, and unknown engines; extracted and raw media; empty lists) run in CI on |
Signed-off-by: furionw <qiwa@nvidia.com> # Conflicts: # components/src/dynamo/sglang/request_handlers/llm/diffusion_handler.py
|
/ok to test 23b0cdd |
|
@neilkg one more update: GitHub reported a merge conflict with |
KrishnanPrash
left a comment
There was a problem hiding this comment.
Stamped to unblock
Why
SGLang's own HTTP server rejects media for text-only models, but Dynamo calls SGLang's Engine API directly, which skips that check: SGLang drops the image, video, or audio and the request returns a 200 answer that never saw the media. Rejecting in the worker keeps the decision with the engine that knows its capabilities, works behind any frontend version, and adds no model-card field, so upgrades do not split worker sets by checksum.
Effect
What Change
is_multimodalis False.Test Plan
🤖 Generated with Claude Code