Repository navigation
fix(proxy): preserve configured chat template defaults #1725
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
ace84b1
8bcf640
e27d251
f332351
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -900,6 +900,42 @@ async def _call_client_create( | |
| raise HTTPException(status_code=500, detail=message) from e | ||
| kwargs["messages"] = prepared_messages | ||
|
|
||
| defaults = dict( | ||
| getattr(_engine.config.agent, "chat_template_kwargs", {}) if _engine else {} | ||
| ) | ||
| extra_body = dict(kwargs.get("extra_body") or {}) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This assumes extra_body remains nested in the request body. The pinned OpenAI SDK flattens extra_body into top-level JSON fields, so only chat_template_kwargs is recovered here and every unrelated extension field is later removed by the unsupported-argument filter. The claimed preservation therefore does not hold for the standard client path.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed the SDK flattening with an actual AsyncOpenAI serialized request passed through the proxy request-preparation function. I narrowed the PR description and scope: this change reconstructs chat_template_kwargs only, not arbitrary SDK extension fields. Arguments supported by the AReaL client signature retain their existing forwarding behavior; unsupported top-level fields remain filtered. Other fields already present in an explicitly nested raw extra_body are left untouched, but that is not a generic preservation guarantee for the standard SDK wire format. I am keeping generic extension passthrough outside this PR rather than bypassing the existing argument filter. |
||
| session_template = session_data.metadata.get("chat_template_kwargs") or {} | ||
| thinking_keys = ("thinking_option", "enable_thinking", "thinking") | ||
| template_kwargs = {} | ||
| for layer in ( | ||
| defaults, | ||
| session_template, | ||
| extra_body.get("chat_template_kwargs") or {}, | ||
| kwargs.pop("chat_template_kwargs", None) or {}, | ||
| ): | ||
| effective = { | ||
| key: value | ||
| for key, value in layer.items() | ||
| if key not in thinking_keys or value is not None | ||
| } | ||
| # Thinking aliases share precedence, even when their names differ. | ||
| if any(key in effective for key in thinking_keys): | ||
| for key in thinking_keys: | ||
| template_kwargs.pop(key, None) | ||
| template_kwargs.update(effective) | ||
| session_thinking = { | ||
| key: value | ||
| for key, value in session_template.items() | ||
| if key in thinking_keys and value is not None | ||
| } | ||
| if session_thinking: | ||
| for key in thinking_keys: | ||
| template_kwargs.pop(key, None) | ||
| template_kwargs.update(session_thinking) | ||
| if template_kwargs: | ||
| extra_body["chat_template_kwargs"] = template_kwargs | ||
| kwargs["extra_body"] = extra_body | ||
|
|
||
| dropped_args = [] | ||
| for k, v in kwargs.items(): | ||
| if k not in areal_client_allowed_args: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The advertised configured defaults are unreachable through supported configuration paths. AgentConfig does not declare chat_template_kwargs, so structured YAML and CLI loading reject the key, while dataclass RPC serialization drops dynamically attached attributes. This lookup therefore resolves to an empty mapping in production even though the new test injects the missing field through SimpleNamespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in e27d251, included in the new head f332351. The field is now declared on AgentConfig and both CLI references are regenerated, so it is part of structured configuration and dataclass serialization rather than a dynamically attached attribute. I verified real AgentConfig construction through OmegaConf, in addition to the existing proxy suite (41 passed). The previous mock-based check did hide this omission; no additional UT file has been added.