feat(Chat): tool_request and tool_result callbacks#493
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
gadenbuie
changed the title
tool_request and tool_result callbackstool_request and tool_result callbacks
gadenbuie
changed the title
tool_request and tool_result callbackstool_request and tool_result callbacks
gadenbuie
force-pushed
the
feat/tool-callbacks
branch
from
0e9f0a1 to
fe0eaf9
Compare
gadenbuie
force-pushed
the
feat/tool-callbacks
branch
from
fe0eaf9 to
25e6cc8
Compare
| # Invoke callbacks in reverse insertion order | ||
| for (id in rev(as.integer(names(private$callbacks)))) { |
There was a problem hiding this comment.
Should $invoke() catch errors in callbacks? I didn't implement it that way because we're looking for the ellmer_tool_reject error in the tool_request callback, so we'd have to carve out exceptions. On the other hand, it feels aggressive for a callback to break the main flow.
There was a problem hiding this comment.
I think we can cop out on this and just say that callbacks should never error.
| #' * `tool_result` receives a `ContentToolResult` object. | ||
| #' | ||
| #' @return Returns a function that can be called to remove the callback. | ||
| register_callback = function(event, callback) { |
There was a problem hiding this comment.
Even here, I think there's some advantage to having separate register callbacks, since then you could check the function arguments here, which I think is a good idea because then you can force specific names for the callback args. You might find check_function2() helpful: https://github.com/r-lib/httr2/blob/main/R/utils.R#L179-L235
There was a problem hiding this comment.
check_fucntion2() is helpful, thanks! I actually originally had an arg check as part of the CallbackManager class, which would look like this
private$callbacks <- list(
"tool_request" = CallbackManager$new(args = "request"),
"tool_result" = CallbackManager$new(args = "result")
)
# or as separate fields
private$callback_on_tool_request <- CallbackManager$new(args = "request"),
private$callback_on_tool_result <- CallbackManager$new(args = "result")
I'm not sure if that nudges you back towards having a single register_callback() method. (I'll split them out and we can see how that feels.)
There was a problem hiding this comment.
I implemented individual callback managers in 881cd2d (or see all changes in this second round)
hadley
left a comment
hadley
left a comment
There was a problem hiding this comment.
Looks great! It's super clear and easy to follow. But I still gave you a bunch of minor comments 😄
| # Invoke callbacks in reverse insertion order | ||
| for (id in rev(as.integer(names(private$callbacks)))) { |
There was a problem hiding this comment.
I think we can cop out on this and just say that callbacks should never error.
Co-authored-by: Hadley Wickham <h.wickham@gmail.com>
* main: (26 commits) Drop `completed` (tidyverse#495) Add PortkeyAI support (tidyverse#471) feat: Adds `stream = "content"` option to `$stream()` and `$stream_async()` (tidyverse#494) feat(Chat): `tool_request` and `tool_result` callbacks (tidyverse#493) feat: Add `tool_reject()` (tidyverse#490) fix: Tweak simple tools test to avoid Claude's empty strings (tidyverse#492) feat(chat_async): Adds `tool_mode` to choose between sequential or concurrent tool calling (tidyverse#488) And azure :| Update snapshot tests; fix bad bedrock model choice chore: Remove trailing whitespace from NEWS (tidyverse#489) Add (some) functions for listing models (tidyverse#454) Move `chat$extract_data_parallel()` to `parallel_chat_structured()` (tidyverse#486) refactor: Use generators for tool invocation (tidyverse#487) refactor: Remove private invoke_tools methods (tidyverse#485) Automatically convert tool inputs (tidyverse#463) Allow `$extract_data_parallel()` to return tokens + cost (tidyverse#449) Start work on programming vignette (tidyverse#458) Revise type coercion (tidyverse#484) Tweak `chat_openai_test()` (tidyverse#483) refactor(tools): `match_tools()` and `tool_results_as_turn()` (tidyverse#480) ...
2 participants
Fixes #401
Adds
Chat$register_callback()to register callbacks fortool_requestandtool_resultevents. Each take a function with a single argument that will receive aContentToolRequestor aContentToolResult. Callbacks are run in last-added first-run order and are invoked directly before and after the tool function is evaluated.If
tool_reject()is called in thetool_requestcallback, the tool call is cancelled and the tool function is not called.