-
Notifications
You must be signed in to change notification settings - Fork 228
Update jsonschema #1389
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
Update jsonschema #1389
Conversation
These tests only test provider schemas so no need for them to be in tools Signed-off-by: Djordje Lukic <[email protected]>
Signed-off-by: Djordje Lukic <[email protected]>
With this update jsonschema decided that slices and such have a type of [null, string], which is correct in golang world, but it breaks Gemini so we need to tweak and remove all the array-defined types and take only the type itself. Signed-off-by: Djordje Lukic <[email protected]>
| out := cagentExec(t, "testdata/fs_tools.yaml", "--model=mistral/mistral-small", "How many files in testdata/working_dir? Only output the number.") | ||
|
|
||
| require.Equal(t, "\n--- Agent: root ---\n\nCalling list_directory(path: \"testdata/working_dir\")\n\nlist_directory response → \"FILE README.me\\n\"\n1", out) | ||
| // NOTE: If you look at the LLM response, Mistral says it sees 2 files, yours truly got tired of re-running this test to get it to say "1". |
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.
😂
| } | ||
|
|
||
| // The Response API requires every parameter to be required | ||
| parameters = makeAllRequired(parameters) |
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.
is this safe to remove? wasn't expecting this change here
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.
back then it wasn't. @rumpl have you tested it?
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.
It wasn't removed, I moved these to ConvertParametersToSchema so both Responses and Chat APIs will get the same treatment.
Yes I have tested this, our e2e tests still work and I tested with the responses api too
With this update jsonschema decided that slices and such have a type of [null, string], which is correct in golang world, but it breaks Gemini so we need to tweak and remove all the array defined types and take only the type itself.
Also moved the code a bit and unified the way we convert parameters in openai