Repository navigation
feat(router): add --mm-per-request-image-limit override for spec image limits - #2381
Conversation
…e limits The multimodal model specs hardcode per-request modality limits (e.g. qwen3_vl caps images at 10) with no router-level override. A vLLM engine configured with --limit-mm-per-prompt.image 128 still gets every >10-image request rejected at the gateway with HTTP 400 invalid_multimodal_request, so the gateway silently contradicts the engine's configured capability. Add a --mm-per-request-image-limit flag (Rust binary and Python launcher) that replaces each spec's built-in image limit for all models, taking precedence over the SMG_IMAGE_MAX_COUNT env override. The registry trait gains validate_media_request_with_limits, an object-safe variant taking per-modality caller overrides, and the router passes its configured map through MultimodalComponents into the single validation call site. Unset keeps today's spec-default behavior; zero is rejected at clap parse time and in RouterConfig::validate. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
github-actions
Bot
added
python-bindings
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| /// [`Self::validate_media_request`] with caller-supplied per-modality limit | ||
| /// overrides; each replaces the spec limit and beats the env override. | ||
| fn validate_media_request_with_limits( |
There was a problem hiding this comment.
🟡 Nit: The trait now has two validation entry points, and the only production caller (prepare_placeholder_tokens) uses the new one. Today no spec overrides validate_media_request, so this is fine — but the "obvious" method for a future spec author to override is validate_media_request, and such an override would be silently bypassed by the router, since the default validate_media_request_with_limits calls modality_limits/check_media_counts directly.
Cheap ways to keep that from rotting: note in the validate_media_request doc comment that specs must override validate_media_request_with_limits (not this one) because the gateway only calls the _with_limits variant, or drop the default body of validate_media_request and make it a non-trait helper so there's only one overridable hook.
| model_registry: Arc::new(ModelRegistry::default()), | ||
| config_registry, | ||
| pixel_cache: pixel_cache_from_env(), | ||
| modality_limit_overrides: image_limit_override |
There was a problem hiding this comment.
🟡 Nit: This Option<usize> → HashMap<Modality, usize> mapping is the one link in the chain with no test. The PR covers the ends (traits.rs unit test for the override semantics, main.rs for CLI → RouterConfig, validation.rs for Some(0)), but nothing asserts that MultimodalComponents::new(reg, Some(128)) yields {Image: 128} and that None yields an empty map. A silent regression here (e.g. someone keying it on Modality::ImageEmbeds, or an unwrap_or_default() that swallows the value) would make the flag a no-op with every existing test still green.
MultimodalComponents::new builds a reqwest client and a MediaConnector, so a direct test is a bit heavy — extracting the two-line mapping into a small free function (fn image_limit_overrides(limit: Option<usize>) -> HashMap<Modality, usize>) would make it a two-case unit test.
| ) | ||
| parser.add_argument( | ||
| f"--{prefix}mm-per-request-image-limit", | ||
| type=int, |
There was a problem hiding this comment.
🟡 Nit: The help text promises "Must be >= 1", but type=int accepts 0 and negatives, so the two entry points reject bad input at different places with different messages:
- Rust CLI:
claprange(1..)→ clean parse-time error naming the flag. - Python launcher
0: accepted by argparse, accepted by the pyo3 constructor, and only rejected atRouter.start()asConfiguration validation failed: .... - Python launcher
-1:OverflowErrorout of the pyo3Option<usize>extraction at_Router(**args_dict).
Both do fail loudly, so nothing is silently wrong — but a small type= validator would give the Python path the same parse-time rejection as the Rust one:
def _positive_int(value: str) -> int:
parsed = int(value)
if parsed < 1:
raise argparse.ArgumentTypeError("must be >= 1")
return parsed
| /// Per-request image-count limit applied to every model, replacing each | ||
| /// spec's built-in limit (e.g. to match the engine's `--limit-mm-per-prompt`). | ||
| #[arg(long, value_parser = clap::value_parser!(u64).range(1..), help_heading = "Multimodal")] | ||
| mm_per_request_image_limit: Option<u64>, |
There was a problem hiding this comment.
🟡 Nit: Naming is off-pattern for this help group. The two sibling flags under help_heading = "Multimodal" spell the prefix out (--multimodal-tensor-transport, --multimodal-shm-min-bytes), and the env override this flag supersedes is SMG_IMAGE_MAX_COUNT. --mm-per-request-image-limit introduces a third spelling (mm) for the same subsystem.
Worth settling now rather than later — the name is baked into RouterConfig's serde field, the pyo3 signature, and the frozen RouterArgs field-order list, so renaming after release is a breaking change on three surfaces. --multimodal-per-request-image-limit would match the neighbours; if the mm prefix is deliberate (it does echo vLLM's --limit-mm-per-prompt), a line in the doc comment saying so would stop the next person from "fixing" it.
|
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: 9 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 10 reviews per hour. 📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds an optional per-request image-count limit to Python and server configuration. The limit is validated, propagated through multimodal router initialization, and applied during media request validation. ChangesMultimodal image limits
Sequence Diagram(s)sequenceDiagram
participant PythonRouter
participant RouterConfig
participant GrpcRouter
participant MultimodalComponents
participant ModelProcessorSpec
PythonRouter->>RouterConfig: Set mm_per_request_image_limit
RouterConfig->>GrpcRouter: Pass configured image limit
GrpcRouter->>MultimodalComponents: Initialize image override
MultimodalComponents->>ModelProcessorSpec: Validate media request with limits
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning Your free Security trial is over. An organization admin can upgrade to Advanced for continuous pull request security review or dismiss this notice. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
2 participants
Description
Problem
The router's multimodal model specs hardcode per-request modality limits — e.g.
crates/multimodal/src/registry/qwen3_vl.rssetsModality::Image → 10— with no router-level override. A vLLM engine configured with--limit-mm-per-prompt.image 128still gets every >10-image request rejected at the gateway:HTTP 400 invalid_multimodal_request: model spec qwen3_vl supports at most 10 image inputs; got 100. Live-hit on the moirai OMENative PD campaign (Qwen3-VL, multi-turn conversations legitimately accumulate 100+ images). The gateway should not silently contradict the engine's configured capability.SMG_IMAGE_MAX_COUNT(#2153) can already raise the limit via env, but there was no first-class router flag plumbed throughRouterConfigand the Python launcher.Solution
Add
--mm-per-request-image-limit Nto the Rust binary and the Python launcher. When set, it replaces every model spec's built-in image limit (raising or lowering it), and takes precedence overSMG_IMAGE_MAX_COUNT. It never enables a modality a spec does not declare. Unset keeps today's spec-default behavior. The value must be >= 1: clap rejects 0 at parse time, andRouterConfig::validate()rejects it on the pyo3 path.Enforcement stays at the single validation call site (
prepare_placeholder_tokensinmodel_gateway/src/routers/grpc/multimodal/plan.rs, backed bycheck_media_countsincrates/multimodal/src/registry/traits.rs). The registry trait gainsvalidate_media_request_with_limits, an object-safe variant taking a per-modality override map, so the mechanism generalizes to video/audio if a flag is ever needed there (env overrides already cover them).Future work (deeper fix): workers should advertise their engine's
--limit-mm-per-promptvia registration labels, and the router should takemin(spec, engine)per worker instead of a deployment-wide flag.Changes
crates/multimodal/src/registry/traits.rs: newvalidate_media_request_with_limitstrait method (caller override map > env override > spec limit);validate_media_requestdelegates to it with no overrides. Unit test for both directions of the override plus unoverridden modalities.model_gateway/src/config/{types,builder,validation}.rs:RouterConfig.mm_per_request_image_limit: Option<usize>+ builder method +Some(0)rejected invalidate_server_settings.model_gateway/src/main.rs:--mm-per-request-image-limitCLI flag (range(1..)), plumbed into the builder; extended the multimodal config-plumbing guard test.model_gateway/src/routers/grpc/multimodal/{config,plan}.rs,router.rs:MultimodalComponentscarriesmodality_limit_overridesbuilt fromRouterConfig; the validation call site passes it through.bindings/python/src/lib.rs:mm_per_request_image_limitappended at the pyo3 signature/struct tail, forwarded to theRouterConfigbuilder (validated by the existingrouter_config.validate()call instart()).bindings/python/src/smg/router_args.py: dataclass field appended at the tail +--mm-per-request-image-limitargparse flag.bindings/python/tests/test_arg_parser.py: parse test + frozen field-order list updated.Test Plan
Ran locally:
cargo test -p llm-multimodal --lib traits::— 8 passed (includes newcaller_limit_override_replaces_spec_limit)cargo test -p smg --bin smg multimodal_transport_flows_into_both_configs— passes (now also asserts the new flag reaches bothRouterConfigandServerConfig, and that clap rejects0)cargo test -p smg --lib config::validation::tests::zero_mm_per_request_image_limit_is_rejected— passescargo test -p smg --lib routers::grpc::multimodal— 53 passedcargo check -p llm-multimodal / -p smg --tests / -p smg-python— cleancargo clippy -p llm-multimodal -p smg -p smg-python --no-deps— no warningsmaturin develop+pytest bindings/python/tests/test_arg_parser.py— 58 passed, 1 skipped; also verifiedRouter.from_argsaccepts the new field end-to-end through the pyo3 boundarycargo +nightly fmt— cleanHonest note: the full
cargo clippy --workspace --all-targets --all-featurespre-commit hook needs system opencv (theopencv-videofeature) which is not installed on this machine, so that exact invocation is deferred to CI; the touched crates were clippy-clean with default features.Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspasses (needs system opencv; deferred to CI — touched crates clippy-clean locally)