-
Notifications
You must be signed in to change notification settings - Fork 419
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
correctly handle raised exception inside function calls #1057
Conversation
🦋 Changeset detectedLatest commit: 4612147 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
logger.error( | ||
f"Failed to get weather data, status code: {response.status}" | ||
) | ||
return f"Failed to get weather data, status code: {response.status}" |
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.
This should raise an exception with a message
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.
Like raise Exception("your message")
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.
Sure, we can approach it that way but I was considering practical use case, like in production, we’d want an LLM to handle errors if function calls fail. Instead of just raising an exception, the LLM should provide meaningful feedback to caller.
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.
Should already be the case here
content = f"Error: {e}" |
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.
Oh it's failing for me, I will check that
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.
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.
So It seems that failed function calls have result=None
called_fncs: [CalledFunction(call_info=FunctionCallInfo(tool_call_id='call_KKypw6MWzZImAeeFVeRwJaMx', function_info=FunctionInfo(name='get_weather', description='', auto_retry=False, callable=<bound method AssistantFnc.get_weather of <__mp_main__.AssistantFnc object at 0x000001D27A8262A0>>, arguments={'location': FunctionArgInfo(name='location', description='The location to get the weather for', type=<class 'str'>, default=<class 'inspect._empty'>, choices=())}), raw_arguments='{"location":"London"}', arguments={'location': 'London'}), task=<Task finished name='Task-69' coro=<AssistantFnc.get_weather() done, defined at C:\Users\Jp\projects\livekit\agents\examples\voice-pipeline-agent\function_calling_weather.py:27> exception=Exception('Failed to get weather data')>, result=None, exception=Exception('Failed to get weather data'))]
and we are ignoring that:
agents/livekit-agents/livekit/agents/pipeline/pipeline_agent.py
Lines 781 to 783 in 74f00c3
# ignore the function calls that returns None | |
if called_fnc.result is None: | |
continue |
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.
are you saying that errors are not being returned to the LLM? if so do you have a proposed path forward?
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.
Yes that's correct. for solution we can comment out the code or add one more condition to handle exception in below code.
agents/livekit-agents/livekit/agents/pipeline/pipeline_agent.py
Lines 781 to 783 in 74f00c3
# ignore the function calls that returns None | |
if called_fnc.result is None: | |
continue |
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've updated PR
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.
approving now, but it'd be great to have a test case for this before merging
Nice catch! |
added
return
instead ofraise
inget_weather
, so llm can handle errors in function callpreviously :
after fix :
PTAL @theomonnom @davidzhao