fix(backends): deep-merge extra_body.chat_template_kwargs in merge_model_options - #1617
Open
planetf1 wants to merge 2 commits into
Open
fix(backends): deep-merge extra_body.chat_template_kwargs in merge_model_options#1617planetf1 wants to merge 2 commits into
planetf1 wants to merge 2 commits into
Conversation
…del_options model_options-level extra_body defaults (the pattern documented for setting a persistent thinking-mode toggle) were silently clobbered by any unrelated per-call extra_body, because merge_model_options did a flat dict.update on the whole extra_body value. Special-case extra_body to deep-merge its chat_template_kwargs sub-dict, mirroring the deep-merge OpenAIBackend._merge_user_extra_body already does; every other key still flat-overwrites as before. Adds regression tests at the merge-layer (test_model_options.py, test_options.py) and through an actual intrinsic/adapter call (test_openai_intrinsics_unit.py), plus a defensive fallback for a malformed non-dict chat_template_kwargs. Updates the openai.md docs section that previously documented this as a known limitation. Fixes generative-computing#1539 Assisted-by: Claude Code Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
Comments and test docstrings should document current behavior, not point back at the issue that motivated it. Also tightens the _merge_extra_body/merge_model_options docstrings. Assisted-by: Claude Code Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
jakelorocco
reviewed
Sep 3, 2026
Comment on lines
+288
to
+295
| if ( | ||
| k == "extra_body" | ||
| and isinstance(v, dict) | ||
| and isinstance(new_options.get(k), dict) | ||
| ): | ||
| new_options[k] = ModelOption._merge_extra_body(new_options[k], v) | ||
| else: | ||
| new_options[k] = v |
Contributor
There was a problem hiding this comment.
For openai backends at least, I believe this causes construction-time enable_thinking to silently override per-call ModelOption.THINKING.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
Issue
Fixes #1539
Description
Impact
A backend-level
extra_bodydefault (e.g. a thinking-mode toggle set at construction time) gets silently dropped, with no error or warning, the moment a per-callextra_bodyis also present — even one that's about something else entirely:This isn't a rare edge case: it fires on every intrinsic/adapter call, because
OpenAIBackend._generate_from_intrinsicalways builds its ownextra_body(to writechat_template_kwargs["adapter_name"]) — so a session that sets a thinking default and also uses any adapter loses that default the first time the adapter is called.Root cause
merge_model_optionsmerges option dicts with a flat, top-leveldict.update. That's correct for scalar options, but wrong forextra_body, whose value is itself a dict that different call sites write into independently.Fix
merge_model_optionsnow special-casesextra_body: when both sides have a dict there, theirchat_template_kwargssub-dicts deep-merge instead of one replacing the other (newModelOption._merge_extra_body, mirroring the deep-mergeOpenAIBackend._merge_user_extra_bodyalready does one layer down). Every other option key — and every other key insideextra_body— still flat-overwrites exactly as before; only the one broken case changes.Also updates
docs/docs/integrations/openai.md, which documented this exact clobbering as a known limitation — that caveat is now gone.Testing
New tests:
test_model_options.py,test_options.py— unit coverage on the merge functions: unrelated-key preservation,chat_template_kwargsdeep-merge, conflict resolution, non-mutation, non-dict fallback.test_openai_intrinsics_unit.py::test_construction_time_extra_body_default_survives_intrinsic_call— the impact scenario end to end through a real intrinsic call: a construction-time thinking default, an unrelated per-callextra_body, and the adapter's ownadapter_nameall survive together in the final request. Confirmed this test fails on pre-fix code (KeyError: 'enable_thinking') and passes post-fix.Full run:
uv run pytest test/ -m "not qualitative"→ 4286 passed, 20 skipped, 4 xpassed, 1 unrelated pre-existing flake (test_tracing_backend.py::test_stream_e2e, a live-Ollama timing assertion — passes on rerun).ruff format/ruff check/mypyclean;markdownlint-cli2clean on the docs change.Attribution
Adding a new component, requirement, sampling strategy, or tool?
If your PR adds or modifies one of the types below, check the matching box. A checklist of type-specific review items will be posted as a comment.
NOTE: Please ensure you have an issue that has been acknowledged by a core contributor and routed you to open a pull request against this repository. Otherwise, please open an issue before continuing with this pull request.