fix(backends): merge intrinsic extra_body once by gaoflow · Pull Request #1300 · generative-computing/mellea

gaoflow · 2026-06-19T01:42:28Z

Summary

Pop user-supplied extra_body out of intrinsic OpenAI API params before spreading them into create().
Merge that body into the intrinsic extra_body, preserving documents and combined chat_template_kwargs.
Add a regression test covering THINKING=True with user extra_body.

Tests

uv run pytest test/backends/test_openai_intrinsics_unit.py test/backends/test_openai_ollama.py -q
uv run ruff check .
uv run ruff format --check .
uv run python -m compileall -q mellea/backends/openai.py test/backends/test_openai_intrinsics_unit.py
git diff --check

The intrinsic OpenAI path passed extra_body explicitly and also let user-supplied extra_body remain in api_params, so model_options containing extra_body raised TypeError before the request was sent. Pop user extra_body before updating api_params and merge it into the intrinsic extra_body, preserving chat_template_kwargs from both sides. Fixes generative-computing#1241 Assisted-by: Codex Signed-off-by: Vincent Gao <gaobing1230@gmail.com>

ajbozarth

Thank you for your submission. LGTM on the fix; matches #1241 and the standard-path treatment from #1204. A couple of cleanup items below before merge.

planetf1 · 2026-07-20T15:50:32Z

Thanks for this, @gaoflow — logic looks correct and matches the #1204 pattern. Still needs the items from @ajbozarth's review above before merging though.

Also found the same extra_body double-spread bug in _generate_from_raw (unfixed) — worth folding into this PR alongside the helper extraction, or I'm happy to raise it as a follow-up issue if you'd rather keep this PR scoped.

Let me know if you'd like a hand getting this over the line.

Both the intrinsic and standard chat paths merged user extra_body with duplicated logic; the intrinsic copy had dropped the comments explaining the shallow copy and why a shallow merge is safe. Extract one helper so they stay in sync and the reasoning lives in a single place. Apply it to _generate_from_raw as well, which passed extra_body explicitly while also spreading backend-specific params that can contain it - the same TypeError as generative-computing#1241 on the completions endpoint. Add tests for the merge helper, for the intrinsic path without THINKING, and for the completions path. Assisted-by: Claude Code Signed-off-by: Vincent Gao <gaobing1230@gmail.com>

gaoflow · 2026-07-27T07:30:43Z

All of @ajbozarth's items are in a1a58b2, and I folded in the _generate_from_raw one rather than deferring it — same helper, so it was a two-line change.

Confirmed it's real and still unfixed on main: _generate_from_raw passes extra_body=extra_body and also spreads **self._make_backend_specific_and_remove(...), which can carry extra_body. With model_options={"extra_body": {...}} and a format, it raises TypeError: got multiple values for keyword argument 'extra_body' before the request goes out — the #1241 failure on the completions endpoint.

Added test_generate_from_raw_merges_user_extra_body, which reproduces that exact TypeError against the current PR head and passes with the fix, plus four unit tests for the helper itself. Against main's openai.py 8 of the tests in these two files fail; all 45 pass now.

planetf1

All three review comments addressed and verified — helper extraction, docstring rationale, and new tests all check out. Ruff, mypy, and the full openai backend test suite pass. LGTM.

planetf1

Follow-up pass after a second review surfaced a few consistency/documentation nits on top of the already-resolved threads. Nothing blocking here — flagging for optional follow-up.

planetf1 · 2026-07-27T11:51:00Z

+            user: `extra_body` taken from the caller's model_options, or None
+
+        Returns:
+            a new dict; `base` and `user` are left unmodified


NIT: the docstring says the return value leaves base unmodified, but when user is None this returns base by identity (lines 452-453), and test_merge_user_extra_body_none_returns_base asserts exactly that with is base. Worth tightening the wording so it doesn't overpromise a copy in that branch.

Suggested change

a new dict; `base` and `user` are left unmodified

a new dict when `user` is not None; when `user` is None, `base` is returned unmodified (same object), skipping the copy

planetf1 · 2026-07-27T11:51:00Z

            else:
                api_params["reasoning_effort"] = thinking

+        extra_body = self._merge_user_extra_body(extra_body, user_extra_body)


When ModelOption.THINKING is set, api_params["reasoning_effort"] is derived from it, but a caller overriding chat_template_kwargs.enable_thinking through their own extra_body only touches the CTK side — reasoning_effort still reflects the original thinking value. E.g. THINKING: True plus extra_body.chat_template_kwargs.enable_thinking: False ends up sending reasoning_effort="medium" alongside enable_thinking=False. Not a blocker, but a short comment on the precedence (or a test pinning down the intended behaviour) would help future readers.

planetf1 · 2026-07-27T11:51:00Z

@@ -925,19 +961,9 @@ async def _generate_from_chat_context_standard(
        )
        user_extra_body = backend_specific.pop("extra_body", None)
        if user_extra_body is not None:


NIT: this path still guards the merge with if user_extra_body is not None:, while the other two call sites call _merge_user_extra_body unconditionally and rely on its own None short-circuit. Removing the guard here would make all three call sites consistent — only caveat is it would change extra_params["extra_body"] from absent to an always-present (possibly empty) dict when there's nothing to merge, so worth checking nothing downstream depends on the key being absent.

planetf1 · 2026-07-27T11:51:00Z

+        # shallow merge is safe: chat_template_kwargs is the only nested dict key
+        # Mellea writes into extra_body; it is deep-merged separately below


NIT: "deep-merged separately below" reads like a forward reference to a later step, but it's actually the very next if block (462-466).

Suggested change

# shallow merge is safe: chat_template_kwargs is the only nested dict key

# Mellea writes into extra_body; it is deep-merged separately below

# shallow update is safe here: chat_template_kwargs is the only nested

# dict key, and it gets its own key-wise merge in the next block

planetf1 · 2026-07-27T11:51:00Z

+    call_kwargs = mock_create.call_args.kwargs
+    extra_body = call_kwargs["extra_body"]
+    assert extra_body["caller_key"] == "caller-value"
+    assert "guided_json" in extra_body or "structured_outputs" in extra_body


NIT: worth a short comment on why this is an or — the key name depends on self._use_structured_output_for_raw (openai.py:1220-1223), so it's correct, just non-obvious at a glance.

Suggested change

assert "guided_json" in extra_body or "structured_outputs" in extra_body

# key name depends on _use_structured_output_for_raw (vLLM vs. guided-decoding backends)

assert "guided_json" in extra_body or "structured_outputs" in extra_body

ajbozarth · 2026-07-27T15:55:12Z

Re-reviewed and my concerns were all addressed, I'll defer to Nigels review above for the remaining nits.

planetf1 · 2026-07-29T15:11:10Z

We're ok to merge this @gaoflow - ok with you?

gaoflow requested a review from a team as a code owner June 19, 2026 01:42

gaoflow requested review from AngeloDanducci, ajbozarth and akihikokuroda June 19, 2026 01:42

github-actions Bot added the bug Something isn't working label Jun 19, 2026

ajbozarth reviewed Jun 19, 2026

View reviewed changes

Comment thread mellea/backends/openai.py Outdated

Comment thread mellea/backends/openai.py Outdated

Comment thread test/backends/test_openai_intrinsics_unit.py

ajbozarth assigned gaoflow Jun 19, 2026

planetf1 approved these changes Jul 27, 2026

View reviewed changes

planetf1 reviewed Jul 27, 2026

View reviewed changes

	a new dict; `base` and `user` are left unmodified
	a new dict when `user` is not None; when `user` is None, `base` is returned unmodified (same object), skipping the copy

		# shallow merge is safe: chat_template_kwargs is the only nested dict key
		# Mellea writes into extra_body; it is deep-merged separately below

	assert "guided_json" in extra_body or "structured_outputs" in extra_body
	# key name depends on _use_structured_output_for_raw (vLLM vs. guided-decoding backends)
	assert "guided_json" in extra_body or "structured_outputs" in extra_body

Uh oh!

Conversation

gaoflow commented Jun 19, 2026

Summary

Tests

Uh oh!

ajbozarth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

planetf1 commented Jul 20, 2026

Uh oh!

gaoflow commented Jul 27, 2026

Uh oh!

planetf1 left a comment

Choose a reason for hiding this comment

Uh oh!

planetf1 left a comment

Choose a reason for hiding this comment

Uh oh!

planetf1 Jul 27, 2026

Choose a reason for hiding this comment

Uh oh!

planetf1 Jul 27, 2026

Choose a reason for hiding this comment

Uh oh!

planetf1 Jul 27, 2026

Choose a reason for hiding this comment

Uh oh!

planetf1 Jul 27, 2026

Choose a reason for hiding this comment

Uh oh!

planetf1 Jul 27, 2026

Choose a reason for hiding this comment

Uh oh!

ajbozarth commented Jul 27, 2026

Uh oh!

planetf1 commented Jul 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants