- Notifications
You must be signed in to change notification settings - Fork1k
Addbuiltin_tools toAgent#2102
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 ourterms of service andprivacy 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?
Conversation
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
…dantic#1752)Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
- Added builtin_tools field to ModelRequestParameters- Merged new output_mode and output_object fields from main- Updated test snapshots to include all fields- Resolved import conflicts to include both builtin tools and profiles
I'll make sure the For curiosity, do you work at Duolingo? |
Awesome and Dm’ed! |
| # TODO(Marcelo): We need to handle custom tool definitions per model base. | ||
| # def handle_custom_tool_definition(self, model: str) -> Any: ... |
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.
I don't understand this, what does it mean?
| city:str | ||
| country:str | ||
| region:str |
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.
what isregion?
More generally, are the required contents (of this fieldand the others on this class) vendor-specific in any way? Should we include examples?
| part:ServerToolCallPart | ||
| """The server tool call to make.""" | ||
| event_kind:Literal['server_tool_call']='server_tool_call' |
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 makes me sad that some of our discriminators are snake_case (here) and some are kebab-case (part_kind). I guess you probably didn't introduce this inconsistency in this PR, but it feels bad. Maybe we should change it in v1 and just do value normalization during validation (i.e., replace any_ with- or vice versa).
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| ifpart.executable_codeisnotNone: | ||
| items.append(ServerToolCallPart(args=part.executable_code.model_dump(),tool_name='code_execution')) | ||
| elifpart.code_execution_resultisnotNone: | ||
| # TODO(Marcelo): Is the idea to generate the tool_call_id on the `executable_code`, and then pass it 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.
I feel like we can/should answer this question before merging?
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
@Kludex should we include code interpreter from openai as its accessible on the responses API or save that for a follow up given its a fairly complex set of types |
Co-authored-by: David Montague <35119617+dmontagu@users.noreply.github.com>
Uh oh!
There was an error while loading.Please reload this page.
Fixes test and merge conflicts for#1722
Closes#840