-
Notifications
You must be signed in to change notification settings - Fork 7
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 & argref changes #71
Conversation
Hi @sidnarayanan - one issue is in the design PCR, sometimes you need to pass the name of an enzyme. We can change that method to accomodate, but that flexibility for passing in a string or keyname |
src/aviary/env.py
Outdated
@@ -217,7 +217,7 @@ async def _exec_tool_call(tool_call: ToolCall) -> ToolResponseMessage: | |||
logger.debug(str(exc), exc_info=True) | |||
tool_exc = exc | |||
if tool_exc: | |||
s_content: str = f"{logger_msg}:\n{tool_exc}" | |||
s_content = str(tool_exc) |
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.
Let's keep the above logger_msg
, but just change the wording from Failed to execute tool call
to something like Encountered Exception during tool call
.
The reason is:
- Sometimes people don't put a message inside of a
raise
(e.g.raise ValueError
is valid) - Sometimes people don't state in the message if it's a failure or not
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.
Done. I'm keeping logger_msg
separate above, since we report a bit more info (exception type, tool name).
@whitead I see, let me think this through |
@sidnarayanan I can just change that function to have another set of arguments - enzyme_overhang - for example. |
logger_msg = ( | ||
f"Failed to execute tool call for tool {tool.info.name}: {exc!r}" | ||
) | ||
logger_msg = f"Encountered exception during tool call for tool {tool.info.name}: {exc!r}" |
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.
logger_msg = f"Encountered exception during tool call for tool {tool.info.name}: {exc!r}" | |
logger_msg = f"Encountered exception during tool {tool.info.name}: {exc!r}" |
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.
during tool
sounds odd - it's during a tool call
Per offline discussion, will modify |
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.
Nice work!
These are a set of changes to tool call handling and
argref_by_name
that I found to help reduce agent failure rates.Tool calling:
handle_exc=True
. The old message (a) leaked implementation details and (b) prevented us from using exceptions for control flow in tools.argref_by_name
:def func(a: list[int])
, nowfunc(a='foo,bar')
is supported.argref
'dFWIW - I think we should drop
*args
inargref_by_name
methods, sinceToolRequestMessage
s only support kwargs anyway. Would simplify the code significantly.