Repository navigation
fix: map empty Gemini tool-call arguments to {} instead of {"type":"object"} - #43162
apex-mochen wants to merge 1 commit into
Conversation
|
| "post_message": " [/INST]\n", | ||
| }, | ||
| "user": {"pre_message": "[INST] ", "post_message": " [/INST]\n"}, | ||
| "assistant": {"pre_message": " |
There was a problem hiding this comment.
Mid-conversation system messages fail Supported Anthropic models retain these messages, but the converter no longer handles them. It raises
BadRequestError before the request reaches Anthropic.
Knowledge Base Used: Provider adapters and capabilities
| out = _gemini_tool_call_invoke_helper( | ||
| function_call_params={"name": "list_directory", "arguments": ""} | ||
| ) | ||
| assert out["args"] == {} |
There was a problem hiding this comment.
Existing Gemini test fails The new assertion expects
{}, while the existing factory test still expects {"type": "object"}. Update that test; the repository guide requires bug fixes to extend the existing mapped file.
Context Used: AGENTS.md (source)
| "post_message": " [/INST]\n", | ||
| }, | ||
| "user": {"pre_message": "[INST] ", "post_message": " [/INST]\n"}, | ||
| "assistant": {"pre_message": " |
There was a problem hiding this comment.
Template parameter loses its type Both template helpers replace
str | None with Any | None. The repository guide requires strongly typed parameters; restore that requirement before merging.
Context Used: AGENTS.md (source)
| initial_prompt_value="<s>", | ||
| role_dict={ | ||
| "system": { | ||
| "pre_message": "[INST] \n", | ||
| "post_message": " [/INST]\n", | ||
| }, | ||
| "user": {"pre_message": "[INST] ", "post_message": " [/INST]\n"}, | ||
| "assistant": {"pre_message": " |
There was a problem hiding this comment.
Duplicate thinking-block explanation This docstring repeats the shared predicate’s explanation, creating text to maintain twice. The repository guide restricts unnecessary code comments; remove the duplicate before merging.
Context Used: AGENTS.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| "post_message": " [/INST]\n", | ||
| }, | ||
| "user": {"pre_message": "[INST] ", "post_message": " [/INST]\n"}, | ||
| "assistant": {"pre_message": " |
Merging this PR will not alter performance
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
lets-order-some-fries
left a comment
There was a problem hiding this comment.
The substantive change here is right and matches the description — _gemini_tool_call_invoke_helper handing back {"type": "object"} as runtime args instead of {} is a real bug, and the three-line fix at factory.py:1144 is the correct shape.
The diff around it is the problem: +9456/−9479, with 5586 changed lines in factory.py. I pulled the head and normalised it against the merge-base (c19ce71bcd), and three separate things are riding along that I don't think you intended.
1. The file was converted to CRLF. Every line:
base: 5582 lines, 0 with CR
head: 5586 lines, 5586 with CR
Once \r is stripped, the real diff in factory.py is 62 lines, not 5586. Same in the test file: 52 real lines inside 3847.
2. Five comments were corrupted, and the pattern says the encoding is the cause. Em-dashes became 鈥?, which is UTF-8 — (E2 80 94) read back as GBK/CP936:
1631: # translate to Anthropic document blocks, not image blocks 鈥?
2318: # conversational-turn boundary 鈥?reset tracking.
2376: # because there is no way to "pass through" an empty text block 鈥?the
3708: # Fallback: no objects extracted 鈥?use empty dict.
3. It removes live code that is currently on main. These are deletions in the normalised diff, not additions:
from litellm.litellm_core_utils.prompt_templates.mid_conversation_system import anthropic_system_messages, together with its call sitenew_messages.extend(anthropic_system_messages(messages[msg_i]))is_unsignable_thinking_blockfrom the imports and both of its call sites, includingreturn [block for block in thinking_blocks if not is_unsignable_thinking_block(block)]TypeAliasfrom thetypingimport and_AnthropicMessageList: TypeAlias = list[AllAnthropicPassThroughMessageValues]- two signatures weakened from
chat_template: str | Nonetochat_template: Any | None
anthropic_system_messages is still referenced on main, so merging this as-is would drop that handling and the unsignable-thinking-block filtering.
All three are consistent with one cause: the file being opened and rewritten by an editor with a non-UTF-8 default codepage and CRLF endings, on a base that had drifted.
What I would suggest, rather than fixing it in place: branch fresh from current main, apply only the arguments_dict change and the new tests/unit/test_gemini_tool_call_convert.py, and leave factory.py's other 5580 lines untouched. Setting git config core.autocrlf input and making sure the editor writes UTF-8 should stop it recurring. The fix itself is worth landing — it just needs to be a three-line diff so a reviewer can see that it is one.
I have not run the test suite against this head; this is a read of the diff against the merge-base.
4706ae2 to
def0f8b
Compare
|
@lets-order-some-fries Thank you — you were right about the corrupted, drifted diff. I rebuilt this PR from current main rather than trying to repair that branch in place. The current head (d5f4912) is +25/-6 across three files only: the three-line empty-object correction, the existing expectation now updated to an empty object, and three focused empty/missing/non-empty argument tests. It contains no CRLF/BOM or encoding rewrite, and it leaves the Anthropic system-message handling, thinking-block filter, and type annotations untouched. The repository workflows have been triggered on this rebuilt head; I will address any CI result that identifies a remaining issue. |
|
@mateo-berri @Shivam - small ping for review when you have a moment. This fixes #43156: when a Gemini/Vertex tool call has zero arguments, the helper currently emits {"type":"object"}, which differs from the Anthropic/Bedrock path (which passes {}) and can trip Gemini's tool schema validation. Diff is ~25 lines with regression coverage. Happy to adjust anything. |
lets-order-some-fries
left a comment
There was a problem hiding this comment.
Re-checked at bf7d9d610e. This looks right now. The only change to factory.py is line 1144, the existing expectation is updated, and nothing else comes along.
What I ran:
- The repro from #43156 through
convert_to_gemini_tool_call_invokereturnsargs={}for thearguments=""call at this head, and{'type': 'object'}onmain(14f4c34c61). - With only this PR's test files applied to
main,test_empty_arguments_maps_to_empty_object,test_missing_arguments_maps_to_empty_objectand the updatedtest_vertex_ai_transform_empty_function_call_argumentsfail withassert {'type': 'object'} == {}. The non-empty case passes on both, as it should. All four pass here. tests/unit/litellm_core_utils/prompt_templates/plus the Vertex Gemini transformation tests give the same results asmain, apart from the three new passes.
The red misc / Run tests is not caused by this PR. Its only failures are tests/unit/test_model_prices_schema.py::test_price_map_has_no_duplicate_keys[main] and [backup], both on a duplicate openrouter/perceptron/perceptron-mk1.5 key in the price maps, which this PR does not touch. main failed the same job on the same two tests at your base, 99655b6f86. #43273 removed the duplicate at 06:15Z, after this run had started, and main has passed that job on every commit since, through 14f4c34c61. A re-run checks out the same merge commit (71a4e6a), so the check will stay red until the branch is updated from main.
One correction to my earlier review: I suggested keeping a separate test file, but AGENTS.md (line 30) asks bug fixes to extend the existing mapped test file. test_empty_arguments_maps_to_empty_object also repeats test_vertex_ai_transform_empty_function_call_arguments (test_litellm_core_utils_prompt_templates_factory.py:595), with the same empty arguments and the same assertion. Moving the missing-key and non-empty cases into that file and dropping test_gemini_empty_arguments.py would fit the guide. That is not a blocker, and it's a maintainer's call.
Also, the @shivam above resolves to an account with no activity on this repo. The maintainer you meant is likely shivamrawat1.
bf7d9d6 to
bd92a0f
Compare
|
@lets-order-some-fries Thanks for the re-check! I've rebased the branch onto the latest upstream main ( The core three-line fix in |
|
Checked at |
|
Thanks for re-checking at To confirm the state you verified is the state we are leaving it in: no further changes are planned on this branch. The test move stays where it is, and the three assertions are deliberate — the two that flip pin the fix, and CI is 97/97 at that head with nothing outstanding from our side. |
|
Hi @lets-order-some-fries, thanks for the re-check at |
lets-order-some-fries
left a comment
There was a problem hiding this comment.
Approving at bd92a0fd, the head I checked on 09-26; nothing has been pushed since. The diff is still the one-line change at factory.py:1144 plus the updated expectation and two new cases in the mapped test file. In the revert control I ran then, with factory.py reverted to main, the empty and missing-arguments tests failed with assert {'type': 'object'} == {} and the non-empty case passed either way. CI is green at this head, and main at b370996b9d still has {"type": "object"} in that branch, so the fix is still needed.
|
@mateo-berri @Shivam Could you take a maintainer look when convenient? The current head |
|
Merge request, same state as #43293: head |
|
Hi @litellm-engineering, gentle follow-up on this one. #43162 was approved by @lets-order-some-fries on Sep 30, and I've kept it rebased onto the latest |
|
Hi @mateo-berri — could you take a look at this PR for a final maintainer merge decision when convenient? The current HEAD (bd92a0f) has community approval, CI is green, and the fix is limited to mapping empty Gemini tool-call arguments to {}, with regression coverage. The five remaining review threads are historical CRLF/encoding artifacts from older revisions. Their underlying issues have been addressed, and the current HEAD has been confirmed clean. For context, I previously contributed the cancellation-race analysis and shielded-cleanup suggestions acknowledged in the recently merged #43061. I appreciate your work getting that fix upstream. Could you let me know if anything else is needed before this PR can be merged? Happy to address any remaining concerns promptly. Thanks! |
…bject"} Rebase onto latest upstream main. Fold the missing-key / non-empty regression cases into the mapped test file (per AGENTS.md) and drop the standalone test_gemini_empty_arguments.py. See BerriAI#43156 Signed-off-by: ding <2756823972@qq.com>
bd92a0f to
10511df
Compare
|
Rebased onto latest upstream main. The two failing jobs were from an old run: |
Fixes #43156.
Zero-arg OpenAI tool calls arrive with
arguments=""._gemini_tool_call_invoke_helperinjected a schema stub{"type":"object"}as runtimeargsinstead of{}, diverging from the Anthropic/Bedrock path. Maps empty/missing arguments to{}and parses non-empty as before. Adds unit tests.