Repository navigation
Conversation
/v1/responses dropped chat-template arguments on the way to the chat request: NvCreateResponse had no field for them, and the conversion hard-coded chat_template_args: None. Template-driven behaviour such as reasoning and tool formatting could therefore be controlled from /v1/chat/completions but not from /v1/responses, even though the same model families depend on it. Add the field to the Responses request, mirroring the chat spelling including the chat_template_kwargs alias so one client payload behaves the same on both endpoints, and forward it in the conversion. Wire the aggregator backstop at the same time. The Responses handler left move_reasoning_to_content_when_empty unset, which a NOTE there recorded as safe only because the conversion hard-coded None, and which asked for the flag to be wired if the conversion ever forwarded the args. It does now: the streaming stage gates on the request's own args through wants_reasoning_as_content_when_empty, but a non-streaming Responses request reaches the aggregating branch, which reads the flag instead. Without this, force_nonempty_content would work when streaming and silently not when not. Fixes ai-dynamo#12284 Signed-off-by: ayaangazali <ayaangazali.work@gmail.com>
|
👋 Hi ayaangazali! 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 Responses API now accepts optional ChangesResponses chat-template argument propagation
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This PR forwards chat-template arguments through Responses conversion and preserves the existing unset behavior when omitted; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
rmccorm4
left a comment
There was a problem hiding this comment.
Thanks for the contribution @ayaangazali 🙏
Adding chat_template_args to NvCreateResponse object instead of upstream on CreateResponse object in frontend-crates repo matches existing customization of chat_template_args on NvCreateChatCompletionRequest instead of frontend-crates CreateChatCompletionRequest object
|
/ok to test e58f28a |
|
Correcting something I said in this PR's validation notes, now that this has merged. I described The failure is the same every time, and it is not the status assertion: So the 413 status arrives, and the JSON body behind it is what gets lost. The test posts a 2 MB payload against a 1 MB limit; I am not proposing a change. The server behaviour is ordinary HTTP, a server may respond and close before a request body is fully sent, and making the response reliably readable would mean draining some bounded amount of a body that was rejected for being too large, which has obvious cost implications and is a call for whoever owns that path. The test, added in #13102, asserts a slightly stronger guarantee than HTTP gives it. Flagging it rather than fixing it because a coin-flip test in the merge gate is worth knowing about, and because my earlier note here understated it. Happy to open a proper issue if that is the preferred home. |
Summary
Fixes #12284, which @michaelfeil confirmed on the issue.
/v1/responsesdropped chat-template arguments on the way to the chat request.NvCreateResponsehad no field for them at all, and the conversion hard-codedchat_template_args: None, so template-driven behaviour such as reasoning and tool formatting could be controlled from/v1/chat/completionsbut not from/v1/responses, even for the model families that depend on it.The field is added to the Responses request mirroring the chat spelling, including the
chat_template_kwargsalias, so one client payload behaves the same on both endpoints, and the conversion forwards it.The coupled change, and why it is in the same PR
The Responses handler left
move_reasoning_to_content_when_emptyunset, and a NOTE there explained why:Forwarding the args is exactly the condition that comment names, so the flag is wired here as well, mirroring the six lines the chat path already uses. The streaming stage gates on the request's own args through
wants_reasoning_as_content_when_empty, but a non-streaming Responses request reaches the aggregating branch, which reads the flag instead. Shipping the forwarding alone would have madeforce_nonempty_contentwork when streaming and silently not when not, which is the failure mode the NOTE anticipated. The comment is rewritten to describe the wiring rather than the old exemption.Adding a field to
NvCreateResponsealso requiredchat_template_args: Noneat 22 existing struct literals, all in test code and one test helper ininput_trigger.rs. That is mechanical and separate from the behaviour change.Validation
cargo test -p dynamo-llm --no-default-features --lib— 1935 passed.cargo clippy --no-default-features --all-targetsclean,cargo fmtapplied.Three tests, following the repro shape in the issue:
chat_template_argsreaches the chat request withenable_thinking == truechat_template_kwargsalias deserializes the same way, so the endpoints do not diverge on spellingNonerather than an empty map, which would change downstream template renderingThe first two fail against the previous behaviour, verified by restoring
chat_template_args: Nonein the conversion and keeping the tests:One caveat worth stating:
http::service::service_v2::tests::test_oversized_body_returns_json_413is flaky in my environment under the full parallel suite. It passed and failed on two consecutive runs of identical code, passes in isolation, and passes in isolation with this change stashed, so it is unrelated to this diff. It touchesservice_v2.rs, which this PR does not.Not verified here: no live model, so I have not observed a template actually consuming a forwarded argument end to end. The change is at the conversion and wiring layer and the tests cover those.
Summary by CodeRabbit
New Features
chat_template_argsand thechat_template_kwargsalias.Bug Fixes