-
Notifications
You must be signed in to change notification settings - Fork 139
fix(backends): merge intrinsic extra_body once #1300
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
base: main
Are you sure you want to change the base?
Changes from all commits
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 | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -433,6 +433,39 @@ def _make_backend_specific_and_remove( | |||||||||
|
|
||||||||||
| return model_opts | ||||||||||
|
|
||||||||||
| @staticmethod | ||||||||||
| def _merge_user_extra_body( | ||||||||||
| base: dict[str, Any], user: dict[str, Any] | None | ||||||||||
| ) -> dict[str, Any]: | ||||||||||
| """Merges a user-supplied `extra_body` into the one Mellea built. | ||||||||||
|
|
||||||||||
| Both must end up in a single `extra_body` value; passing two spreads that | ||||||||||
| each contain one raises `TypeError` at call time. | ||||||||||
|
|
||||||||||
| Args: | ||||||||||
| base: the `extra_body` Mellea assembled for this request | ||||||||||
| user: `extra_body` taken from the caller's model_options, or None | ||||||||||
|
|
||||||||||
| Returns: | ||||||||||
| a new dict; `base` and `user` are left unmodified | ||||||||||
| """ | ||||||||||
| if user is None: | ||||||||||
| return base | ||||||||||
|
|
||||||||||
| # shallow copy so .pop() below doesn't mutate the caller's dict | ||||||||||
| user = dict(user) | ||||||||||
| merged = dict(base) | ||||||||||
| user_ctk = user.pop("chat_template_kwargs", None) | ||||||||||
| # shallow merge is safe: chat_template_kwargs is the only nested dict key | ||||||||||
| # Mellea writes into extra_body; it is deep-merged separately below | ||||||||||
|
Comment on lines
+459
to
+460
Contributor
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. NIT: "deep-merged separately below" reads like a forward reference to a later step, but it's actually the very next
Suggested change
|
||||||||||
| merged.update(user) | ||||||||||
| if user_ctk is not None: | ||||||||||
| merged["chat_template_kwargs"] = { | ||||||||||
| **merged.get("chat_template_kwargs", {}), | ||||||||||
| **user_ctk, | ||||||||||
| } | ||||||||||
| return merged | ||||||||||
|
|
||||||||||
| async def _generate_from_context( | ||||||||||
| self, | ||||||||||
| action: Component[C] | CBlock, | ||||||||||
|
|
@@ -661,6 +694,7 @@ async def _generate_from_intrinsic( | |||||||||
| user_api_params = self._make_backend_specific_and_remove( | ||||||||||
| model_options, is_chat_context=True | ||||||||||
| ) | ||||||||||
| user_extra_body = user_api_params.pop("extra_body", None) | ||||||||||
| api_params.update(user_api_params) | ||||||||||
|
|
||||||||||
| # Map THINKING to the correct backend parameter(s). Two mechanisms: | ||||||||||
|
|
@@ -680,6 +714,8 @@ async def _generate_from_intrinsic( | |||||||||
| else: | ||||||||||
| api_params["reasoning_effort"] = thinking | ||||||||||
|
|
||||||||||
| extra_body = self._merge_user_extra_body(extra_body, user_extra_body) | ||||||||||
|
Contributor
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. When |
||||||||||
|
|
||||||||||
| # --- call the OpenAI-compatible API -------------------------------- | ||||||||||
| # The rewriter may add instruction messages where 'role' is a default | ||||||||||
| # (e.g. UserMessage with role="user"). exclude_unset would drop it, | ||||||||||
|
|
@@ -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: | ||||||||||
|
Contributor
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. NIT: this path still guards the merge with |
||||||||||
| # shallow copy so .pop() below doesn't mutate the caller's dict | ||||||||||
| user_extra_body = dict(user_extra_body) | ||||||||||
| eb = dict(extra_params.get("extra_body") or {}) | ||||||||||
| user_ctk = user_extra_body.pop("chat_template_kwargs", None) | ||||||||||
| # shallow merge is safe: chat_template_kwargs is the only nested dict | ||||||||||
| # key Mellea writes into extra_body; it is deep-merged separately below | ||||||||||
| eb.update(user_extra_body) | ||||||||||
| if user_ctk is not None: | ||||||||||
| eb["chat_template_kwargs"] = { | ||||||||||
| **eb.get("chat_template_kwargs", {}), | ||||||||||
| **user_ctk, | ||||||||||
| } | ||||||||||
| extra_params["extra_body"] = eb | ||||||||||
| extra_params["extra_body"] = self._merge_user_extra_body( | ||||||||||
| extra_params.get("extra_body") or {}, user_extra_body | ||||||||||
| ) | ||||||||||
|
|
||||||||||
| chat_response: Coroutine[ | ||||||||||
| Any, Any, ChatCompletion | openai.AsyncStream[ChatCompletionChunk] | ||||||||||
|
|
@@ -1204,15 +1230,20 @@ async def _generate_from_raw( | |||||||||
|
|
||||||||||
| prompts = [self.formatter.print(action) for action in actions] | ||||||||||
|
|
||||||||||
| backend_specific = self._make_backend_specific_and_remove( | ||||||||||
| model_opts, is_chat_context=False | ||||||||||
| ) | ||||||||||
| extra_body = self._merge_user_extra_body( | ||||||||||
| extra_body, backend_specific.pop("extra_body", None) | ||||||||||
| ) | ||||||||||
|
|
||||||||||
| try: | ||||||||||
| completion_response: Completion = ( | ||||||||||
| await self._async_client.completions.create( | ||||||||||
| model=self._model_id, | ||||||||||
| prompt=prompts, | ||||||||||
| extra_body=extra_body, | ||||||||||
| **self._make_backend_specific_and_remove( | ||||||||||
| model_opts, is_chat_context=False | ||||||||||
| ), | ||||||||||
| **backend_specific, | ||||||||||
| ) | ||||||||||
| ) # type: ignore | ||||||||||
| except openai.BadRequestError as e: | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,9 +4,13 @@ | |||||||
| _simplify_and_merge, and _make_backend_specific_and_remove. | ||||||||
| """ | ||||||||
|
|
||||||||
| from unittest.mock import AsyncMock, MagicMock, PropertyMock, patch | ||||||||
|
|
||||||||
| import pytest | ||||||||
| from openai.types import Completion | ||||||||
| from openai.types.chat import ChatCompletion, ChatCompletionChunk, ChatCompletionMessage | ||||||||
| from openai.types.chat.chat_completion import Choice | ||||||||
| from openai.types.completion_choice import CompletionChoice | ||||||||
|
|
||||||||
| from mellea.backends import ModelOption | ||||||||
| from mellea.backends.openai import OpenAIBackend | ||||||||
|
|
@@ -315,5 +319,85 @@ async def test_processing_reasoning_content_takes_precedence_over_reasoning(back | |||||||
| assert mot._underlying_value == "answer" | ||||||||
|
|
||||||||
|
|
||||||||
| # --- _merge_user_extra_body --- | ||||||||
|
|
||||||||
|
|
||||||||
| def test_merge_user_extra_body_none_returns_base(backend): | ||||||||
| """A missing user extra_body leaves the base untouched.""" | ||||||||
| base = {"documents": ["d"]} | ||||||||
| assert backend._merge_user_extra_body(base, None) is base | ||||||||
|
|
||||||||
|
|
||||||||
| def test_merge_user_extra_body_user_keys_win(backend): | ||||||||
| """User keys overlay the base, and unrelated base keys survive.""" | ||||||||
| merged = backend._merge_user_extra_body( | ||||||||
| {"documents": ["d"], "guided_json": {"type": "integer"}}, | ||||||||
| {"guided_json": {"type": "string"}}, | ||||||||
| ) | ||||||||
| assert merged == {"documents": ["d"], "guided_json": {"type": "string"}} | ||||||||
|
|
||||||||
|
|
||||||||
| def test_merge_user_extra_body_deep_merges_chat_template_kwargs(backend): | ||||||||
| """chat_template_kwargs merges key-wise rather than being replaced wholesale.""" | ||||||||
| merged = backend._merge_user_extra_body( | ||||||||
| {"chat_template_kwargs": {"adapter_name": "answerability"}}, | ||||||||
| {"chat_template_kwargs": {"caller_key": "caller-value"}}, | ||||||||
| ) | ||||||||
| assert merged["chat_template_kwargs"] == { | ||||||||
| "adapter_name": "answerability", | ||||||||
| "caller_key": "caller-value", | ||||||||
| } | ||||||||
|
|
||||||||
|
|
||||||||
| def test_merge_user_extra_body_does_not_mutate_inputs(backend): | ||||||||
| """Neither argument is modified; .pop() operates on a copy.""" | ||||||||
| base = {"chat_template_kwargs": {"adapter_name": "answerability"}} | ||||||||
| user = {"chat_template_kwargs": {"caller_key": "caller-value"}} | ||||||||
| backend._merge_user_extra_body(base, user) | ||||||||
| assert base == {"chat_template_kwargs": {"adapter_name": "answerability"}} | ||||||||
| assert user == {"chat_template_kwargs": {"caller_key": "caller-value"}} | ||||||||
|
|
||||||||
|
|
||||||||
| async def test_generate_from_raw_merges_user_extra_body(backend): | ||||||||
| """The completions path passes one extra_body, not two spreads (#1241).""" | ||||||||
| import pydantic | ||||||||
|
|
||||||||
| from mellea.core.base import CBlock | ||||||||
| from mellea.stdlib.context import ChatContext | ||||||||
|
|
||||||||
| class Answer(pydantic.BaseModel): | ||||||||
| value: int | ||||||||
|
|
||||||||
| mock_create = AsyncMock( | ||||||||
| return_value=Completion( | ||||||||
| id="raw-test", | ||||||||
| created=0, | ||||||||
| model="fake", | ||||||||
| object="text_completion", | ||||||||
| choices=[CompletionChoice(index=0, finish_reason="stop", text="ok")], | ||||||||
| ) | ||||||||
| ) | ||||||||
| mock_client = MagicMock() | ||||||||
| mock_client.completions.create = mock_create | ||||||||
|
|
||||||||
| with patch.object( | ||||||||
| OpenAIBackend, | ||||||||
| "_async_client", | ||||||||
| new_callable=PropertyMock, | ||||||||
| return_value=mock_client, | ||||||||
| ): | ||||||||
| await backend._generate_from_raw( | ||||||||
| [CBlock(value="what is 1+1?")], | ||||||||
| ChatContext(), | ||||||||
| format=Answer, | ||||||||
| model_options={"extra_body": {"caller_key": "caller-value"}}, | ||||||||
| ) | ||||||||
|
|
||||||||
| 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 | ||||||||
|
Contributor
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. NIT: worth a short comment on why this is an
Suggested change
|
||||||||
|
|
||||||||
|
|
||||||||
| if __name__ == "__main__": | ||||||||
| pytest.main([__file__, "-v"]) | ||||||||
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.
NIT: the docstring says the return value leaves
baseunmodified, but whenuser is Nonethis returnsbaseby identity (lines 452-453), andtest_merge_user_extra_body_none_returns_baseasserts exactly that withis base. Worth tightening the wording so it doesn't overpromise a copy in that branch.