Reset a failed tool call #5666
Reset a failed tool call #5666
Conversation
| @@ -334,6 +334,10 @@ async def _handle_message_action(self, action: MessageAction) -> None: | |||
| def _reset(self) -> None: | |||
| """Resets the agent controller""" | |||
|
|
|||
| # if the pending action has a tool call, reset the tool call | |||
| if self._pending_action and hasattr(self._pending_action, 'tool_call_metadata'): | |||
| self._pending_action.tool_call_metadata = None | |||
There was a problem hiding this comment.
Any idea why we want to do this? Why not directly remove the pending_action?
There was a problem hiding this comment.
The failed tool calls are gathered in the pending queue in the agent otherwise, and never get sent to the LLM. They are still in the stream, but the agent never sees them, even though they appear real to the user (and to us, when sent in trajectories/logs etc).
Actually, I think we should give it an error observation. It appears to work, I'm still testing that. What do you think?
There was a problem hiding this comment.
What does "failed tool calls" means? Is it a tool call that somehow never get executed?
There was a problem hiding this comment.
Ah, yes, sorry. When there's a runtime error like one raised here, it's possible it never gets an observation. Like this:
There was a problem hiding this comment.
ahh - then how would remove the tool call metadata here help though?
There was a problem hiding this comment.
Oh, I think you're right to question it - it should fail some asserts in the agent if it was removed. No, this is no good.
There was a problem hiding this comment.
Here in reset, we still have the self._pending_action, and we know this action isn't actually going to finish normally. We can try to create on spot an error obs, linked to that action? 🤔
I'm not sure there are many alternatives right now, I can think of others but they won't work... Does the agent really need to support multiple pending tool calls, when in fact we're processing them by response (ModelResponse)? And we override the tool call to have one per ModelResponse. We're executing it, it doesn't actually wait for another?
There was a problem hiding this comment.
We can try to create on spot an error obs, linked to that action? 🤔
I think this is probably a better way! This also preserve enough info so we can look into details when it happens again
|
@xingyaoww This is ready for review. Not tested with UI yet, but headless it works fine and looks ok. |
|
|
||
| # make a new ErrorObservation with the tool call metadata | ||
| if not found_observation: | ||
| obs = ErrorObservation(content='The action has not been executed.') |
There was a problem hiding this comment.
| obs = ErrorObservation(content='The action has not been executed.') | |
| obs = ErrorObservation(content='The action has not been executed. Please try again.') |
There was a problem hiding this comment.
Should we? The agent also adds "[Error occurred in processing last action]"
The agent is in STOPPED or in ERROR at this point. So with the UI, control is back to the user. What the agent should do might be different after the user message?
| # make a new ErrorObservation with the tool call metadata | ||
| if not found_observation: | ||
| obs = ErrorObservation(content='The action has not been executed.') | ||
| obs.tool_call_metadata = self._pending_action.tool_call_metadata |
There was a problem hiding this comment.
Does that means this ErrorObservation will trigger the execution of the same action or no? Will be nice if we can add a test for this..
There was a problem hiding this comment.
It won't, because
- next lines will make
self._pending_action = Nonein the controller, so it won't keep waiting for an obs, - it's an obs with
tool_id, so in the agent too, the pending tool call is now "complete"
On the contrary, leaving it pending is problematic 🤔
There was a problem hiding this comment.
openhands-agent added some nice unit tests! They show the behavior here
|
@openhands-agent-exp Read the diff of this PR. You do not need to fix anything, just add unit tests for this. IMPORTANT: you need to be careful, and read about each of the variables used in the reset() function in agent_controller.py, to understand their purpose, how to define them and how they work! You can read some related unit tests too, like those testing agent controller. |
|
Overview of Changes: Status: All requested changes have been successfully completed with no remaining issues. The implementation provides thorough test coverage and addresses all key aspects of the reset() functionality. |
…nHands into enyst/message-separator
End-user friendly description of the problem this fixes or functionality that this introduces
Give a summary of what the PR does, explaining any non-trivial design decisions
This PR proposes a quick fix to:
Link of any specific issues this addresses
To run this PR locally, use the following command: