Added ability to cancel tool calls. #145
Conversation
The framework for cancelling calls is ready. The shell tool is currently the only one that has been modified to allow cancellation. Removed the :stopped status and added the :cleanup status. :cleanup is entered after normal tool completion or after interrupted tool completion. In either case, :cleanup removes the future and resources from the state. This is a failsafe in case the tool is not able to clean up after itself. This introduced another status, :cleanup, and will require removing :stopped as a terminal status. Since a deref of a cancelled future *immediately* results in a CancellationException without waiting for the future to cleanup, we needed to add another promise to the tool call state to ensure that we properly wait for the tool call to cleanup on cancellation. We added that promise to the state, and the associated actions to init and remove it from the state, and to deliver to the promise. Added some debug logging for the new promise. Modified the tests to handle the change in actions. Added :future-cleanup-complete?* to the tool call state and associated actions. The :stopping status is now considered to be an active status. Added a self-transition on :stop-requested. Added actions for cleaning up resource and futures. Changed :set-future to :add-future. Added a :cancel-future action to the :executing status on :stop-requested. Renamed the :set-call-future action to :set-future, for consistency. In the shell tool, add and remove the resource (the spawned process) from the call state. On InterruptedException, destroy the process. Added a tool-call-destroy-resource! multi-method that can be used from inside and outside the tool to destroy any resources. Typically, a tool will clean up after itself. But it might not, for example if it never gets an InterruptedException, or if the tool is not cooperating. Made the shell tool more robust in making sure that it always cleans up and resources that remain at the end. As a failsafe. Marginally improved some logging. Updated and added to the tests.
| (defmethod tool-call-details-after-invocation :default [_name _arguments details _result] | ||
| details) | ||
|
|
||
| (defmulti tool-call-destroy-resource! |
src/eca/features/tools/shell.clj
Outdated
| :err :string | ||
| :timeout timeout | ||
| :continue true} "bash -c" command-args))] | ||
| (when proc |
There was a problem hiding this comment.
what will happen when this comes nil? I believe result will be nil as well? is that expected?
There was a problem hiding this comment.
Good question.
When proc is nil, that would mean that the tool call is in the :stopping status and so we don't want to start another process. As you say, result will be nil. The :else clause of the cond will trigger and an error will be reported. Admittedly, the error message will be less than useful.
Would it be better to have result be set to {:exit 1 :err "Tool call is :stopping, so shell process not started"}? Or would you prefer a different way to handle this?
There was a problem hiding this comment.
hum, not sure, we certainly don't want to respond to LLM right? so it doesn't need to have a response with the exit/err format, maybe we should just throw a exception?
There was a problem hiding this comment.
Sorry, but I don't follow. If we throw an Exception, it will be caught right there and will return an exit/err anyways. What am I missing?
Why should we not respond to the LLM? It asked for a tool call, so should we not at least send a response saying why we are not running the call? I'm not familiar enough with what the LLMs would expect in such a case, but my guess would be that some feedback would be helpful.
My guess is that what I suggested with the exit/err is a reasonable way to proceed. But let me know if you'd prefer a different approach.
There was a problem hiding this comment.
if we are stopping, we don't want to request LLM again right? because the LLM loop are just requests and responses, if we are :stopping, we don't want to continure this loop (answer LLM), does that makes sense?
There was a problem hiding this comment.
I think this code was meant to be defensive in the following situation.
The LLM has requested a call. However, before the process was started, there was a stop request and so the call has transitioned to :stopping. Now, in the tool call future, just before spawning the process, the call checks to see what its status is. If it is :stopping, it will not spawn off the process.
Currently, the only way to request a stop is via stopping the whole prompt and the whole LLM loop.
We have also talked about the situation where the user might want to stop a particular tool call and continue with the rest. In that case, I would guess that we do want to continue the LLM loop with the remaining calls, right?
I was trying to be consistent with what happens if, for whatever reason (say reaching a process limit), the process cannot be spawned.
Perhaps you can explain more what you meant by throwing an Exception. In particular, where you expect this Exception would be handled.
There was a problem hiding this comment.
I would guess that we do want to continue the LLM loop with the remaining calls, right?
Good question, currently we don't have the option to stop one tool call so I don't know what would happen if 3 tool calls are running and user stop 1, what we should do? but yeah, maybe you are right, we want to respond that user stopped the tool call and respond to LLM , so I think it's ok to return that err/exit structure you mentioned.
Perhaps you can explain more what you meant by throwing an Exception. In particular, where you expect this Exception would be handled.
Nvm, I was thinking in a case user stop the prompt, but not stopping a single tool call, I just think that:
- if there is a single tool call and user stop it, we should stop the loop and user types what LLM should do differently.
- but if there are multiple tool calls, not sure we want to stop the loop.
In the case where a tool call has been stopped by user request before the shell process has been spawned, explicitly mention why the process was not started.
|
I made the change as we discussed. I have set this version of eca to be the default one that I use - in order to give it some more manual testing. Would it be of some use if I wrote up a document on the tool call internals and/or the design of the tool interrupt system - from the POV of a developer? In particular, the developer of a new tool. |
|
Not sure why the integration test is failing between 0.64.0 and 0.64.1 The changes between the two should not have impacted this test? Would you take a look? I have been running this version of eca as my primary eca for the last two days and I am not seeing any regressions. Have you had a chance to run any manual tests on this PR? |
I think so, at least to leave described in the PR or issue, that would be helpful We are having flaky issues on integration-tests but AFAIK only for macos for some reason, certainly there are improvements that could be made there. I tested your PR but had to fix other issues, will use again and let you know |
|
Not seeing any issues so far, looks good, will merge and let go into next release, thank you! |
2 participants
The framework for cancelling calls is ready. The shell tool is currently the only one that has been modified to allow cancellation.
Removed the :stopped status and added the :cleanup status. :cleanup is entered after normal tool completion or after interrupted tool completion. In either case, :cleanup removes the future and resources from the state. This is a failsafe in case the tool is not able to clean up after itself.
This introduced another status, :cleanup, and will require removing :stopped as a terminal status.
Since a deref of a cancelled future immediately results in a CancellationException without waiting for the future to cleanup, we needed to add another promise to the tool call state to ensure that we properly wait for the tool call to cleanup on cancellation. We added that promise to the state, and the associated actions to init and remove it from the state, and to deliver to the promise. Added some debug logging for the new promise. Modified the tests to handle the change in actions.
Added :future-cleanup-complete?* to the tool call state and associated actions.
The :stopping status is now considered to be an active status. Added a self-transition on :stop-requested.
Added actions for cleaning up resource and futures. Changed :set-future to :add-future.
Added a :cancel-future action to the :executing status on :stop-requested.
Renamed the :set-call-future action to :set-future, for consistency.
In the shell tool, add and remove the resource (the spawned process) from the call state. On InterruptedException, destroy the process.
Added a tool-call-destroy-resource! multi-method that can be used from inside and outside the tool to destroy any resources. Typically, a tool will clean up after itself. But it might not, for example if it never gets an InterruptedException, or if the tool is not cooperating.
Made the shell tool more robust in making sure that it always cleans up and resources that remain at the end. As a failsafe.
Marginally improved some logging.
Updated and added to the tests.