-
Notifications
You must be signed in to change notification settings - Fork 865
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
Tool calling support. Part I #102
base: main
Are you sure you want to change the base?
Conversation
Add tool calling support for below providers - OpenAI, Groq, Anthropic, AWS, & Mistral. OpenAI compatible SDKs need to changes for tool calling support. Adding utility ToolManager for users to easily supply tools, and parse model's request for tool usage.
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.
Looks good @rohitprasad15
Nothing blocking, just a few suggestions in the review.
if isinstance(msg, dict): | ||
return self._convert_dict_message(msg) | ||
return self._convert_message_object(msg) |
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 looks like both _convert_dict_message
and _convert_message_object
logically do the same things. Would it be better to convert to one format and keep one of the convert message methods.
if isinstance(msg, dict):
msg = Message(msg)
return self._convert_message_object(msg)
or the other way
if isinstance(msg, Message):
msg = dict(msg)
return self._convert_dict_message(msg)
tool_input = ( | ||
tool_call["function"]["arguments"] | ||
if isinstance(tool_call, dict) | ||
else tool_call.function.arguments | ||
) | ||
message_content.append( | ||
{ | ||
"type": "tool_use", | ||
"id": ( | ||
tool_call["id"] if isinstance(tool_call, dict) else tool_call.id | ||
), | ||
"name": ( | ||
tool_call["function"]["name"] | ||
if isinstance(tool_call, dict) | ||
else tool_call.function.name | ||
), | ||
"input": json.loads(tool_input), | ||
} | ||
) |
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.
Similar thought, converting to either an object or dict at the beginning of this would create a consistency that would not require the if/else
statements throughout the function call.
aisuite/utils/tool_manager.py
Outdated
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.
Can you add unit tests for this?
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.
outside of unit tests, lgtm
@rohitprasad15 When can we expect tool calling in |
Add tool calling support for below providers -
OpenAI, Groq, Anthropic, AWS, & Mistral.
OpenAI compatible SDKs need to changes for
tool calling support.
Adding utility ToolManager for users to easily
supply tools, and parse model's request for
tool usage.