[Frontend][Rust] Support mm_processor_kwargs in chat completions - #50164
almogtavor wants to merge 3 commits into
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbb6f29953
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
This pull request has merge conflicts that must be resolved before it can be |
The Rust frontend rejected `mm_processor_kwargs` outright. Accept it and merge it onto the model's resolved `PreProcessorConfig` before the image and video preprocessors run, so per-request overrides such as `min_pixels` and `max_pixels` take effect. Known keys are type checked by deserializing the request map as a `PreProcessorConfig`, and only keys the request sets override the model default. Keys the struct does not name land in its `extra` catch-all, which is where model-specific keys from `preprocessor_config.json` already go, so they still reach the processor. A bad value is a request validation error and surfaces as HTTP 400. `media_io_kwargs` stays rejected: it has to reach the media loader, and `MediaConnectorConfig` in `llm-multimodal` carries no per-request IO kwargs to forward it into. Part of vllm-project#44280. Signed-off-by: almogtavor <almogtavor@gmail.com>
fbb6f29 to
d6eb2f6
Compare
|
@BugenZhao Rebased on main, and all three review points are addressed. Cache key. This was a real bug. The overrides change the tensors, but the item hash came straight from the raw image or clip hash, so the same image sent twice with different Audio. Rejected rather than silently ignored, since the audio preprocessor takes a different config type and would drop the overrides. Error conversion. Now uses One |
|
@BugenZhao this implements the Verified at runtime rather than only in unit tests: built |
vllm-project#50368 added a prepare_multimodal call for the gRPC and /inference/generate paths after this branch changed the signature. Those callers carry no per-request kwargs, so the model config applies unchanged. Signed-off-by: almogtavor <almogtavor@gmail.com>
|
@BugenZhao ping on this when you get a chance. all three codex findings are fixed and the threads are resolved, and i just merged main which needed one line because #50368 added a prepare_media call site that now passes None for the kwargs |
|
This pull request has merge conflicts that must be resolved before it can be |
Purpose
Roadmap item from #44280:
mm_processor_kwargs.The Rust frontend rejected it with
mm_processor_kwargs is not supported.on/v1/chat/completions. This accepts it and makes it take effect.Implementation
The kwargs are merged onto the model's resolved
PreProcessorConfiginvllm-chat, right before the image and video preprocessors run, so both modalities honour a per-request override such asmin_pixelsormax_pixels.The merge deserializes the request map as a
PreProcessorConfig, which type checks known keys, then copies over only the keys the request actually set, leaving the rest of the model default intact. Keys the struct does not name land in its#[serde(flatten)] extramap, which is where model-specific keys frompreprocessor_config.jsonalready go, so they still reach the processor rather than being dropped. A bad value (for example"max_pixels": "lots") is a request validation error and returns HTTP 400.Test
All green. New tests in
vllm-chatcover: unset kwargs keep the model config, only requested keys are overridden, unknown keys survive throughextra, and a wrong value type producesInvalidMmProcessorKwargsclassified as a request validation error. Avllm-servertest covers the convert layer forwarding the field.cargo clippy -p vllm-chat -p vllm-server --all-targetsis clean andcargo fmtapplied.Runtime check on an H100
Built
vllm-rsfrom this branch against the image's own vLLM commit and served Qwen2-VL-2B withVLLM_USE_RUST_FRONTEND=1, confirming the Rust frontend was the process answering (it exits on startup if it is not). Same 1024x1024 image in every request, onlymm_processor_kwargsdiffers:{"max_pixels": 200704}{"max_pixels": 50176}{"merge_kernel_size": [2, 2]}(unknown key){"max_pixels": "lots"}returns HTTP 400 withinvalid mm_processor_kwargs: invalid type: string "lots", expected usizeand"type": "invalid_request_error".So the override reaches the image processor and changes the token count, an unknown key is forwarded through
extrawithout breaking the request, and a bad value is a client error rather than a 500.AI assistance was used for this change.