Conversation
Signed-off-by: cmdy <zhang_lin66@foxmail.com>
|
👋 Hi cmdy! 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 (4)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughChangesOpenAI request parsing
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The request parsing changes preserve validation behavior while adding the intended null normalization; no merge-blocking risk is currently identified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
38dbb16 to
d102dbb
Compare
d102dbb to
6ea262b
Compare
6ea262b to
afa7770
Compare
Signed-off-by: cmdy <zhang_lin66@foxmail.com>
afa7770 to
5f6478a
Compare
|
/ok to test 5f6478a |
|
/ok to test 757d223 |
|
Null handling looks correct, and existing validation is preserved. I haven’t run the tests. |
|
|
||
| #[derive(Deserialize)] | ||
| struct CompletionRequestWithNullableStreamOptions<T> { | ||
| #[serde(flatten)] |
There was a problem hiding this comment.
[P2] Avoid copying escaped request text through another flatten layer
This still buffers the request twice: NvCreate*Request already contains flattened fields, so the additional wrapper traverses Serde's outer Content through ContentRefDeserializer, copying owned escaped strings into another buffer. The wrapper runs even when stream_options is absent. At this head, a 15 MiB decoded prompt containing newlines (18 MiB JSON body) raises peak live parsing allocation from 46 MiB to 61 MiB compared with direct deserialization of the same request type. That is an extra 15 MiB per concurrently parsing request; the plain-text control was effectively unchanged. These are allocator measurements in a debug build, excluding the input body, not RSS or throughput estimates.
Normalize the two booleans in ChatCompletionStreamOptions with #[serde(default, deserialize_with = "super::deserialize_null_as_default")], reuse the existing null-default helper in the protocol crate, and remove this wrapper so both handlers use parse_json_request directly. I validated that change locally with a patched protocol dependency: 750 tests passed, and the null-containing stream-options probe returned identical parsed output with peak parsing allocation back at 46 MiB. Shipping it needs the corresponding protocol release and dependency bump.
Follow-up to the earlier allocation finding: the explicit JSON-value retry is gone, but the extra payload copy remains in this replacement.
There was a problem hiding this comment.
Thanks for the detailed measurements. Agreed—the wrapper should be removed. The protocol-side null handling is already available in dynamo-protocols 6.0.1 through frontend-crate #239, but this PR is still on 5.4.1 and #14755 owns the protocols 6 migration. I’ll wait for #14755 to land, then update this branch, remove the wrapper, restore direct parse_json_request usage in both handlers, and rerun the targeted tests.
Overview:
Some OpenAI-compatible clients serialize unset optional request parameters as explicit
null. This is accepted by SGLang-compatible request schemas, but Dynamo currently validates several frontend passthrough fields as concrete values before a request reaches the backend.For example, the following request fragment currently returns HTTP 400 with
Validation: cache_salt must be a string:{ "cache_salt": null, "stop_token_ids": null, "stream_options": { "include_usage": null, "continuous_usage_stats": null } }This PR treats these explicit nulls as unset values for both
/v1/chat/completionsand/v1/completions, allowing the request to proceed with Dynamo's existing defaults.Details:
nullas omitted for the six recognized fields inPASSTHROUGH_EXTRA_FIELDS.nullboolean members inside astream_optionsobject and default them tofalse.Passthrough fields
The flattened extra-field deserializer now removes a field only when both conditions are true:
null.PASSTHROUGH_EXTRA_FIELDS.The affected fields are:
cache_saltstop_token_idsdetokenizeallowed_token_idsbad_words_token_idslogprob_token_idsThis normalization happens before validation and backend extraction, so a null value has the same meaning as an omitted optional field. Valid non-null values continue to be forwarded without modification. Invalid non-null values continue to return a validation error.
Unknown fields are deliberately retained, even when their value is
null. They continue to be rejected by default or ignored whenDYN_IGNORE_OPENAI_FE_UNSUPPORTED_FIELDSis enabled.Stream options
The chat and text completion handlers deserialize requests through a typed wrapper that captures
stream_optionsseparately from the flattened completion request. This allows omitted or null values for:stream_options.include_usagestream_options.continuous_usage_statsBoth fields default to
false. Explicittrueandfalsevalues are preserved. Invalid types remain rejected. The request body is deserialized once without materializing an intermediate JSON tree, and duplicate-field validation remains intact.Compatibility behavior
nullnullDYN_IGNORE_OPENAI_FE_UNSUPPORTED_FIELDSstream_optionsomitted or set tonullnullinsidestream_optionsfalsetrueorfalseThe normalization is implemented at the Dynamo OpenAI frontend boundary, so all configured backends receive the same normalized request behavior.
Where should the reviewer start?
lib/llm/src/protocols/openai/validate.rs:deserialize_extra_fieldscontains the allowlisted null normalization and its protocol regression tests.lib/llm/src/http/service/openai.rs:parse_completion_json_requestcontains thestream_optionsfallback normalization and endpoint-level parsing tests.lib/llm/src/protocols/openai/chat_completions.rsandlib/llm/src/protocols/openai/completions.rs: both request types apply the custom flattened-field deserializer.Validation
cargo test -p dynamo-llm --no-default-features --locked --lib protocols::openai::— 471 passed, 0 failed.cargo test -p dynamo-llm --no-default-features --locked --lib http::service::openai::tests::test_parse_— 12 passed, 0 failed.cargo fmt --all -- --checkgit diff --checkThe regression coverage verifies both chat and text completions, all six passthrough fields, unknown-null preservation, valid and invalid non-null values, null and missing stream flags, explicit stream flags, usage-policy overrides, and malformed stream option types.
Related Issues
🚫 This PR is NOT linked to an issue:
Summary by CodeRabbit