Repository navigation
feat(llm): add extra_body passthrough to media request structs - #13817
Conversation
Signed-off-by: Atindra Jha <atindra@cs.stanford.edu>
|
👋 Hi merceod! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughThe OpenAI audio, image, and video request types now accept optional ChangesOpenAI extra body support
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized change adds optional passthrough fields to media requests without any identified merge-blocking risk; it is merge-ready after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
|
Thanks for working on this, @merceod . how about streamlining on [P2: Real correctness/reliability issue under scoped configurations or conditions; normally fix before merge] I think a literal extra_body={"generate_sound": True}arrives at the server as a top-level Could media requests follow the existing
For audio and video, that would be approximately: #[serde(default, flatten)]
pub passthrough: serde_json::Map<String, serde_json::Value>,Image already has a flattened The Python media request models also need to preserve and forward these unknown fields; otherwise the Rust frontend can capture them but the worker boundary will still discard them. |
Signed-off-by: Atindra Jha <atindra@cs.stanford.edu>
|
you're right, thanks for catching it. The sdk merges extra_body into the top level so the nested field would never see traffic from a normal openai client. Ireworked it to follow the GenerateRequest pattern, video and audio now carry a flattened passthrough map, and since the image request already flattens its inner openai struct (a second flattened map would capture the typed keys too) it got hand rolled serdee that keeps the same external contract, typed fields at the top level, unknowns retained, typed fields winning on conflict. |
There was a problem hiding this comment.
Thanks for reworking the Rust side so the public wire shape now matches OpenAI extra_body semantics. I think the latest revision still stops one layer too early at the Python boundary: ConfigDict(extra="allow") preserves unknown fields in Pydantic model_extra, but the Omni handlers do not read or forward model_extra to the engine.
Could we make this an explicit extra_args contract between the Rust frontend and Python worker instead?
Ask
Concretely, I am asking for:
-
Keep the flattened Rust
passthroughon the public media request types. That correctly captures the top-level fields produced by the OpenAI SDK. -
In the Rust frontend, before
engine.generate(...), convert the public request to an internal worker request. Keep the normal typed media fields available to the worker, but place the captured backend-owned fields under a namespaced entry, for example:{ "prompt": "a cat", "model": "wan", "size": "832x480", "extra_args": { "vllm_omni": { "generate_sound": true } } }If this needs to remain backend-neutral, a neutral namespace such as
media_passthroughis fine; the important part is that the internal extension contract is explicit and nested. -
Make
extra_argsan explicit field in the Python worker request contract, rather than depending onBaseModel.model_extra. -
Have
OmniHandlerconsume that namespace and forward the supported values intoOmniDiffusionSamplingParams.extra_argsand/or the appropriate engine kwargs. Stable fields can still be promoted to typed fields over time. -
Add a boundary test showing that a top-level SDK
extra_bodyfield reaches the actual Omni sampling/engine input, not only that Pydantic retains it.
Why
Why I prefer this:
- We should not require every Python receiver to inherit from and configure Pydantic
BaseModel. For example, the common backendGenerateRequestis only aTypedDict, and it explicitly exposesextra_args. - A nested namespace makes ownership clear and prevents backend-specific extensions from colliding with Dynamo's canonical fields or extensions from another backend.
- It gives us one recognizable Rust↔Python pattern instead of one path using explicit
extra_argsand media paths relying on Pydantic-specific implicit storage. extra="allow"by itself only retains the value; it does not forward it to vLLM-Omni.
Example
The Rust frontend side of /inference/v1/generate is the precedent I have in mind:
- Public
GenerateRequestcaptures future top-level fields with#[serde(flatten)] passthrough. VllmTitoEnveloperetains the backend-owned request shape.preprocessed_from_generateruns in the Rust HTTP frontend and stores that envelope inPreprocessedRequest.extra_args["vllm_tito"]before dispatch.- The handler performs that conversion before constructing
Context<PreprocessedRequest>and calling the engine:handler_generate.
So the requested separation is: public top-level compatibility at the HTTP edge, then an explicit namespaced extra_args contract across the Rust-to-Python worker boundary.
Signed-off-by: Atindra Jha <atindra@cs.stanford.edu>
|
Done! Went with the full separation you described. @biswapanda let me know what you think of the PR and if its ready to merge. |
|
/ok to test 62b88f9 |
|
/ok to test 7350323 |
Signed-off-by: Atindra Jha <atindra@cs.stanford.edu>
|
@biswapanda Addressed all your concerns! Lmk if there are any more. |
|
/ok to test b9dabff |
Signed-off-by: Ryan McCormick <rmccormick@nvidia.com>
|
/ok to test 399a66e |
|
Fixed the pre-commit failures and triggered CI |
Overview:
Adds passthrough of unknown top level fields on the media request structs so backend specific knobs can reach the backend without a typed field for each one. Follows the recent discussion where we agreed typed fields stay preferred for anything generic and a passthrough covers the rest.
Details:
The media request structs are closed today, so any field the frontend doesn't know about gets silently dropped. That blocks things like audio generation toggles on video models, image size overrides and TTS emotion controls.
Two layers, matching how
/inference/v1/generatealready separates its public shape from its worker contract.At the public edge,
NvCreateImageRequest,NvCreateVideoRequestandNvCreateAudioSpeechRequestkeep unknown top level fields, matching how the OpenAI client's extra_body option merges into the top level of the body. Video and audio use a flattenedpassthroughmap like the existingGenerateRequest. The image request already flattens its inner OpenAI struct, and a second flattened map would capture the typed keys too, so it gets hand rolled serde that keeps the same external contract.At the worker boundary, the handlers call
nest_passthrough()before dispatch, which moves the captured fields under an explicitextra_args["media_passthrough"]entry. So the worker sees the normal typed fields plus one nested, namespaced bucket instead of loose top level unknowns, and backend extensions can't collide with canonical fields. The Python media protocol models declareextra_argsas an explicit field rather than relying on pydantic extra field storage, and the Omni handlers consume the namespace: knobs matching a diffusion sampling params attribute apply directly, the rest rideOmniDiffusionSamplingParams.extra_args(TTS merges them into its engine params). Knobs that prove out can be promoted to typed fields over time.Tests pin the SDK wire shape at the edge, the nesting at the boundary, and that a passthrough knob reaches the actual sampling params in the Omni handler.
Where should the reviewer start?
lib/llm/src/protocols/openai.rs has the nesting helper and the namespace key. videos.rs shows the plain flattened form plus nesting tests, images.rs the hand rolled serde. On the worker side, components/src/dynamo/vllm/omni/omni_handler.py consumes the namespace and the boundary test lives in test_omni_handler.py.
Related Issues
🚫 This PR is NOT linked to an issue: