Repository navigation
feat(sglang): support --frontend-decoding for aggregated multimodal - #9405
Conversation
0abd902 to
7b63048
Compare
WalkthroughThis PR implements frontend decoding support for multimodal inference across multiple backends. It introduces a shared configuration module, integrates the feature into SGLang with conditional image loading, refactors TRT-LLM and vLLM to use shared configuration helpers, and adds comprehensive tests and example scripts. ChangesFrontend Decoding Multi-Backend Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
🧹 Nitpick comments (3)
components/src/dynamo/sglang/register.py (2)
42-44: ⚡ Quick winMove
DYN_MM_ALLOW_INTERNALto argparse-backed config in SGLang.This new env read introduces another runtime tunable outside
DynamoSGLangArgGroup/DynamoSGLangConfig. Please thread it through config instead of reading env directly here.Based on learnings, "In the SGLang Dynamo wrapper (components/src/dynamo/sglang), replace inline environment reads (os.environ.get()) of tunables with argparse-driven configuration."
🤖 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/sglang/register.py` around lines 42 - 44, Replace the direct os.getenv read of DYN_MM_ALLOW_INTERNAL in register.py by adding a boolean option to the existing argparse/config flow: add a field to DynamoSGLangArgGroup (e.g. --dyn-mm-allow-internal) and expose it on DynamoSGLangConfig, have the code that constructs/receives the config pass config.dyn_mm_allow_internal into register.py instead of reading the env, and then call media_fetcher.allow_direct_ip(config.dyn_mm_allow_internal) and media_fetcher.allow_direct_port(config.dyn_mm_allow_internal); remove the os.getenv usage and the local allow_internal variable and ensure the new arg has a default matching the previous behavior.
85-86: ⚡ Quick winUse direct config field access instead of
getattrforfrontend_decoding.
dynamo_argsis a known typed config object wherefrontend_decoding: bool = Falseis explicitly defined; direct attribute access is safer and keeps schema changes visible.♻️ Suggested local fix
- if getattr(dynamo_args, "frontend_decoding", False): + if dynamo_args.frontend_decoding:🤖 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/sglang/register.py` around lines 85 - 86, Replace the getattr check with direct attribute access on the typed config object: use dynamo_args.frontend_decoding instead of getattr(dynamo_args, "frontend_decoding", False). Update the conditional that calls _build_media_decoder_and_fetcher() to read dynamo_args.frontend_decoding so the schema is explicit and type-safe (this surfaces type-checking and schema changes via dynamo_args.frontend_decoding).components/src/dynamo/sglang/request_handlers/llm/decode_handler.py (1)
515-515: 💤 Low valuePrefer explicit check over assert for invariant.
The Python guideline recommends avoiding
assertfor runtime validation in favor of explicitif ...: raise ..., even for invariants. This provides a clearer error message and is not disabled by-Ooptimization.♻️ Suggested refactor
- assert self._image_loader is not None + if self._image_loader is None: + raise RuntimeError( + "ImageLoader is None despite frontend_decoding=True; " + "this is a programming error in DecodeWorkerHandler.__init__" + )As per coding guidelines: "Do not use
assertfor runtime validation (especially for config/request validation); use explicitif ...: raise ...."🤖 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/sglang/request_handlers/llm/decode_handler.py` at line 515, Replace the runtime assert with an explicit check: in the class/method where the line "assert self._image_loader is not None" appears (e.g., DecodeHandler or its decode method), change it to an if statement that raises a clear exception (ValueError or RuntimeError) with a helpful message like "image loader not initialized" when self._image_loader is None; this ensures the invariant is enforced at runtime and not skipped under -O optimization.
🤖 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.
Nitpick comments:
In `@components/src/dynamo/sglang/register.py`:
- Around line 42-44: Replace the direct os.getenv read of DYN_MM_ALLOW_INTERNAL
in register.py by adding a boolean option to the existing argparse/config flow:
add a field to DynamoSGLangArgGroup (e.g. --dyn-mm-allow-internal) and expose it
on DynamoSGLangConfig, have the code that constructs/receives the config pass
config.dyn_mm_allow_internal into register.py instead of reading the env, and
then call media_fetcher.allow_direct_ip(config.dyn_mm_allow_internal) and
media_fetcher.allow_direct_port(config.dyn_mm_allow_internal); remove the
os.getenv usage and the local allow_internal variable and ensure the new arg has
a default matching the previous behavior.
- Around line 85-86: Replace the getattr check with direct attribute access on
the typed config object: use dynamo_args.frontend_decoding instead of
getattr(dynamo_args, "frontend_decoding", False). Update the conditional that
calls _build_media_decoder_and_fetcher() to read dynamo_args.frontend_decoding
so the schema is explicit and type-safe (this surfaces type-checking and schema
changes via dynamo_args.frontend_decoding).
In `@components/src/dynamo/sglang/request_handlers/llm/decode_handler.py`:
- Line 515: Replace the runtime assert with an explicit check: in the
class/method where the line "assert self._image_loader is not None" appears
(e.g., DecodeHandler or its decode method), change it to an if statement that
raises a clear exception (ValueError or RuntimeError) with a helpful message
like "image loader not initialized" when self._image_loader is None; this
ensures the invariant is enforced at runtime and not skipped under -O
optimization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4af8733b-1478-4342-b801-412894e9cfd6
📒 Files selected for processing (12)
components/src/dynamo/common/configuration/groups/frontend_decoding_args.pycomponents/src/dynamo/sglang/backend_args.pycomponents/src/dynamo/sglang/init_llm.pycomponents/src/dynamo/sglang/register.pycomponents/src/dynamo/sglang/request_handlers/llm/decode_handler.pycomponents/src/dynamo/sglang/tests/test_sglang_frontend_decoding.pycomponents/src/dynamo/trtllm/backend_args.pycomponents/src/dynamo/trtllm/workers/llm_worker.pycomponents/src/dynamo/vllm/backend_args.pycomponents/src/dynamo/vllm/main.pyexamples/backends/sglang/launch/agg_vision.shtests/serve/test_sglang.py
7b63048 to
e1cb939
Compare
a14a4c6 to
3027e60
Compare
Mirrors the dynamo.vllm flag. When set, the backend registers a MediaDecoder + MediaFetcher with the Rust frontend so it decodes images itself and ships pre-decoded pixels to the worker via NIXL RDMA. The aggregated DecodeWorkerHandler arm reads Decoded variants via the shared ImageLoader and hands PIL Images to sgl.Engine.async_generate(image_data=[...]). Out of scope: EPD topology (--multimodal-encode-worker), where the encode worker still needs URLs to run MMEncoder — validation rejects the combination at parse time. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…imodal Wires the new `agg_vision_fd.sh` launch script into `tests/serve/test_sglang.py` via a `multimodal_agg_fd_qwen` SGLangConfig that round-trips a base64 PNG through the Rust frontend's NIXL RDMA media pipeline and `ImageLoader` → `sgl.Engine`, mirroring the vLLM Qwen3.5-0.8B FD test in `tests/serve/multimodal_profiles/vllm.py`. The launch script also gains a `--page-size <n>` flag (default 16) so hybrid Mamba/full-attention VLs like Qwen3.5-0.8B — whose `MambaRadixCache` asserts `page_size == 1` — can be exercised without a hardcode swap. Verified end-to-end on H100 with Qwen/Qwen3.5-0.8B: 1 passed in 59s, response "Green" matched the b64 green-image color prompt. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tend-decoding The FD variant was structurally identical to `agg_vision.sh` plus a NIXL LD_LIBRARY_PATH preamble, a `--frontend-decoding` worker flag, and a `--page-size` parameterization. Collapse the two into a single script behind opt-in `--frontend-decoding` and `--page-size` flags: - Non-FD callers see no behavior change (NIXL preamble gated; worker invocation identical to before). - FD callers pass `--frontend-decoding` to enable both the NIXL setup and the worker flag. - `--page-size <n>` parameterization is kept regardless of FD so Mamba models (Qwen3.5-0.8B) can pass `--page-size 1`. Also bumps the default MODEL to `Qwen/Qwen3-VL-2B-Instruct` (the existing TODO; the FD path already assumed it). `tests/serve/test_sglang.py` `multimodal_agg_fd_qwen` config: switch to `agg_vision.sh` and append `--frontend-decoding` to `script_args`. Verified end-to-end on H100 with Qwen/Qwen3.5-0.8B via the unified script: 1 passed in 53s, response "Green", NIXL UCX backend init + agent registered. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ort guard `--frontend-decoding` was defined inline in each backend's `backend_args.py` (vllm, sglang, trtllm) with identical help text and a near-identical `add_negatable_bool_argument` block — only the env-var prefix differed (`DYN_VLLM_*` vs `DYN_SGL_*` vs `DYN_TRTLLM_*`). Replace the three copies with a single helper at `dynamo.common.configuration.groups.frontend_decoding_args.add_frontend_decoding_arg(g, env_prefix=...)` that each backend calls. Engine-specific topology validation (sglang rejects FD + EPD) stays in each backend's `ConfigBase.validate()`. While here, drop the `try/except ImportError` guard around `from dynamo.llm import MediaDecoder, MediaFetcher` in all three backends. The Rust bindings always export these two classes unconditionally (`lib/bindings/python/rust/lib.rs:186-187`, no `#[cfg(feature = ...)]`). The only media-related Cargo feature is `media-ffmpeg`, which gates video decoding inside MediaDecoder's internals — not the class export. The comment claiming a "media" feature gate was stale; there is no such feature in `lib/llm/Cargo.toml`. Both changes are pure refactors: re-ran the sglang FD e2e (`multimodal_agg_fd_qwen` on Qwen3.5-0.8B, H100): 1 passed in 49.6s, response "Green", same NIXL UCX init + agent registration. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`_image_loader` is non-None iff `_enable_frontend_decoding` is True (set together in `__init__` at lines 85-89). The runtime `and self._image_loader is not None` clause in the aggregated path was just narrowing the Optional for the type checker. Replace with an `assert` so the invariant is documented and the hot path doesn't re-check it on every request. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…test ConfigBase has no kwargs-accepting __init__ — instantiating DynamoSGLangConfig(use_sglang_tokenizer=False, ...) raises TypeError: DynamoSGLangConfig() takes no arguments. Switch to the sibling vLLM test pattern (test_backend_args.py:create_config): build with the no-arg constructor and set each field via attribute assignment. All 6 tests in test_sglang_frontend_decoding.py now pass.
3027e60 to
39cd494
Compare
|
/ok to test 39cd494 |
Why
The
dynamo.sglangbackend ignored--frontend-decoding: every multimodalrequest went through SGLang's URL → HTTP → PIL decode path in the worker's
P0 thread. The
dynamo.vllmbackend has handled this for a while — theRust frontend already decodes images and ships pixels via NIXL RDMA when
the backend advertises a
MediaDecoderat registration. This PR teachesthe SGLang backend to advertise the same and consume
Decodeditems viathe shared
ImageLoader, removing the GIL-bound decode hop on the workerside.
What Change
--frontend-decoding/DYN_SGL_FRONTEND_DECODINGflag mirroring vLLM.MediaDecoder+MediaFetcherwith the Rust frontend.DecodeWorkerHandleraggregated arm: loadDecodeditems viaImageLoader, hand PIL list tosgl.Engine.async_generate(image_data=...).--multimodal-{encode-worker,worker}(EPD needs URLs for MMEncoder).Test Plan
examples/backends/sglang/launch/agg_vision_fd.shreturns a coherent caption.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
--frontend-decodingconfiguration option across SGLang, TRT-LLM, and vLLM backends for frontend-based image decoding in multimodal models with NIXL RDMA support.Tests