-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat: Add items and title to ToolParameter/ToolParamDefinition #3003
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
feat: Add items and title to ToolParameter/ToolParamDefinition #3003
Conversation
Hi @TamiTakamiya! 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 [email protected]. Thanks! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
Can you show the errors that happens when you don't apply this change? The MCP specification does not talk about these attributes specifically. Can you explain exactly how the failures happen and what is specific about Gemini that doesn't affect GPT or Claude? |
@ashwinb When a Gemini LLM (e.g.
The same error is reported against litellm (BerriAI/litellm#12222, BerriAI/litellm#10251), while they may be found without using Llama Stack. As you see in the message like |
I think this is not standard in any way, possible that this is a litellm or gemini model issue. We cannot add this to the tool definitions unless the MCP standard specifies it. |
@ashwinb Currently llama-stack stores the type for an array-type property and ignores |
@ashwinb I think this is issue need to be fixed. I have the same problem with OpenAI GPT 4O.
The input schema is a JSON schema. According to the spec a schema of |
Note that both the Longer-term, we might want to consider whether we really need to strongly type all possible fields of a JSON Schema here or use some other way to shuffle that information from the MCP server to the inference call. |
Re-opening this PR for further review / discussion. |
b6ed79c
to
0dc0cf4
Compare
I didn't realize (immediately) that these fields flow through to the JSON Schema parameters. It just seems like a better fix (although not needed here for now) is to make sure that the Llama Stack tool stuff is explicitly thought of as being part of a JSON schema formally (which it was only loosely modeled until now.) I will review and merge this soon as soon as CI is green. |
You will need to re-record the individual test by using https://github.com/llamastack/llama-stack/blob/main/tests/README.md#remote-re-recording-recommended
Pre-commit is also failing. Please fix that. |
0dc0cf4
to
2f5ca4e
Compare
if possible, I'd pass the whole input and output JSON schemas to the ToolParameter. |
2f5ca4e
to
3373ab1
Compare
3373ab1
to
ae975f9
Compare
@ashwinb @boazdavid @bbrowning What do you suggest we move forward with this PR? My original intention was to provide a small change to fix an issue with array-type parameters with Gemini provider, but if we are going to support full JSON schema, this will be included in that new feature. Pls advise. |
@TamiTakamiya The CI test is failing due to |
yeah I will do that as a follow-up. The root cause is what Ben pointed -- it seems un-ideal to transport via our ToolParameter, etc. definitions which likely just need to go now. We can work with JSON schemas instead and convert back into ToolParameter, etc. wherever needed. |
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.
CI clean now, landing this. Thanks for the work @TamiTakamiya and the discussion everyone. Sorry for all the delay in getting this merged.
@boazdavid @bbrowning @TamiTakamiya @wukaixingxp see #3627 (still work in progress) for killing all these MCP tool schema related errors in one go. |
This is a sweeping change to clean up some gunk around our "Tool" definitions. First, we had two types `Tool` and `ToolDef`. The first of these was a "Resource" type for the registry but we had stopped registering tools inside the Registry long back (and only registered ToolGroups.) The latter was for specifying tools for the Agents API. This PR removes the former and adds an optional `toolgroup_id` field to the latter. Secondly, as pointed out by @bbrowning in #3003 (comment), we were doing a lossy conversion from a full JSON schema from the MCP tool specification into our ToolDefinition to send it to the model. There is no necessity to do this -- we ourselves aren't doing any execution at all but merely passing it to the chat completions API which supports this. By doing this (and by doing it poorly), we encountered limitations like not supporting array items, or not resolving $refs, etc. To fix this, we replaced the `parameters` field by `{ input_schema, output_schema }` which can be full blown JSON schemas. Finally, there were some types in our llama-related chat format conversion which needed some cleanup. We are taking this opportunity to clean those up. This PR is a substantial breaking change to the API. However, given our window for introducing breaking changes, this suits us just fine. I will be landing a concurrent `llama-stack-client` change as well since API shapes are changing.
…stack#3627) This is a sweeping change to clean up some gunk around our "Tool" definitions. First, we had two types `Tool` and `ToolDef`. The first of these was a "Resource" type for the registry but we had stopped registering tools inside the Registry long back (and only registered ToolGroups.) The latter was for specifying tools for the Agents API. This PR removes the former and adds an optional `toolgroup_id` field to the latter. Secondly, as pointed out by @bbrowning in llamastack#3003 (comment), we were doing a lossy conversion from a full JSON schema from the MCP tool specification into our ToolDefinition to send it to the model. There is no necessity to do this -- we ourselves aren't doing any execution at all but merely passing it to the chat completions API which supports this. By doing this (and by doing it poorly), we encountered limitations like not supporting array items, or not resolving $refs, etc. To fix this, we replaced the `parameters` field by `{ input_schema, output_schema }` which can be full blown JSON schemas. Finally, there were some types in our llama-related chat format conversion which needed some cleanup. We are taking this opportunity to clean those up. This PR is a substantial breaking change to the API. However, given our window for introducing breaking changes, this suits us just fine. I will be landing a concurrent `llama-stack-client` change as well since API shapes are changing.
What does this PR do?
Add items and title to ToolParameter/ToolParamDefinition. Adding items will resolve the issue that occurs with Gemini LLM when an MCP tool has array-type properties.
Test Plan
Unite test cases will be added.