fix: llama-stack function tool serialization for Gemini compatibility - #5082
fix: llama-stack function tool serialization for Gemini compatibility#5082dprince wants to merge 2 commits into
Conversation
|
Hi @dprince! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
466ac2f to
12be062
Compare
leseb
left a comment
There was a problem hiding this comment.
Can you leave an inline comment explaining why we exclude "type"? Something like:
# Exclude "type" from the function dict — it belongs on the outer
# ChatCompletionToolParam, not inside the FunctionDefinition.
# OpenAI silently ignores the extra field, but stricter endpoints
# (e.g. Gemini) reject it.This will save future contributors from wondering why the exclude is there.
26a4834 to
ebfb5bf
Compare
Done, in the latest rebase. Thanks |
| # (e.g. Gemini) reject it. | ||
| self.ctx.chat_tools.append( | ||
| ChatCompletionToolParam(type="function", function=input_tool.model_dump(exclude_none=True)) | ||
| ChatCompletionToolParam(type="function", function=input_tool.model_dump(exclude={"type"}, exclude_none=True)) |
There was a problem hiding this comment.
the original code is 1 step too clever using model_dump instead of adding strict to make_openai_tool
also, responses tools have further evolved and allow a defer_loading field, do we need to add that to the exclude too?
two suggestions -
- directly translate the fields we need and don't use
model_dump - file an issue about supporting
defer_loading, aka tool search
There was a problem hiding this comment.
I implemented the switch to make_openai_tool in the latest push
46b3fa6 to
de7dab4
Compare
It used to be possible to run scripts/github/schedule-record-workflow.sh to generate recordings in CI, but I haven't tried it lately, does anybody know if this is it still possible? |
|
@mattf, this is the PR I was talking about (although I see you already here), thanks |
@derekhiggins i've posted gpt recordings and a few ollama, but my current ollama deployment is incomplete |
|
This pull request has merge conflicts that must be resolved before it can be merged. @dprince please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork |
30792e4 to
ce621d8
Compare
|
@mattf thanks for the recordings. I attempted to rebase those as there were conflicts. |
6eff984 to
ce621d8
Compare
ce621d8 to
aefdc37
Compare
|
This pull request has merge conflicts that must be resolved before it can be merged. @dprince please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork |
aefdc37 to
4ae9aa8
Compare
|
This pull request has merge conflicts that must be resolved before it can be merged. @dprince please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork |
This comment was marked as spam.
This comment was marked as spam.
2 similar comments
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
|
@dprince sorry for the lack of attention here, we have a job to do recordings on behalf of users, can you rebase? thanks |
|
This pull request has merge conflicts that must be resolved before it can be merged. @dprince please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork |
0c63bfe to
be287e7
Compare
be287e7 to
d7c272b
Compare
The Responses API function tool conversion used model_dump() to build
the ChatCompletionToolParam function dict, which leaked the "type"
field (and would leak any future fields like defer_loading) into the
FunctionDefinition. OpenAI silently ignores extra fields, but stricter
endpoints (e.g. Gemini) reject them.
Instead of a fragile denylist (exclude={"type"}), reuse
convert_tooldef_to_openai_tool which explicitly picks only the fields
that belong in a FunctionDefinition. Added strict parameter support to
convert_tooldef_to_openai_tool so function tools can pass it through.
Signed-off-by: Dan Prince <dprince@redhat.com>
d7c272b to
d4962f6
Compare
Co-Authored-By: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
|
✅ Recordings committed successfully Recordings from the integration tests have been committed to this PR. |
What does this PR do?
llama-stack's Responses API to Chat Completions conversion in streaming.py leaks a "type" field into the function tool dict via model_dump(). The OpenAI Chat Completions FunctionDefinition schema only allows name, description, parameters, and strict — not type. OpenAI silently ignores the extra field, but Gemini's OpenAI-compatible endpoint strictly validates and rejects it with:
"Unknown name 'type' at 'tools[N].function': Cannot find field."
Fix: exclude type from serialization in streaming.py:
function=input_tool.model_dump(exclude={"type"})
Test Plan
unit tests included