-
Notifications
You must be signed in to change notification settings - Fork 23
feat: add pre/post call hooks to ToolSimulator (#167) #168
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| from dataclasses import dataclass, field | ||
| from typing import Any | ||
|
|
||
|
|
||
| @dataclass | ||
| class PreCallHookEvent: | ||
| """ | ||
| Event passed to pre_call_hook before the LLM generates a tool response. | ||
|
|
||
| Attributes: | ||
| tool_name: Name of the tool being called. | ||
| parameters: Parsed parameters for the tool call. | ||
| state_key: Key for the state (tool_name or share_state_id). | ||
| previous_calls: List of previous tool call records from the state registry. | ||
| """ | ||
|
|
||
| tool_name: str | ||
| parameters: dict[str, Any] | ||
| state_key: str | ||
| previous_calls: list[dict[str, Any]] = field(default_factory=list) | ||
|
|
||
|
|
||
| @dataclass | ||
| class PostCallHookEvent: | ||
| """ | ||
| Event passed to post_call_hook after the LLM generates a tool response. | ||
|
|
||
| Attributes: | ||
| tool_name: Name of the tool that was called. | ||
| parameters: Parsed parameters for the tool call. | ||
| state_key: Key for the state (tool_name or share_state_id). | ||
| response: The LLM-generated response dict, which the hook may modify. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also just some food for thoughts:
My worry is these event types are ad-hoc and bonded to TS changes. Say if in the future we want access to more TS internal members and states, one has to touch these types again.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On one class with On passing |
||
| """ | ||
|
|
||
| tool_name: str | ||
| parameters: dict[str, Any] | ||
| state_key: str | ||
| response: dict[str, Any] = field(default_factory=dict) | ||
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 implementation reminds me of decorator pattern, which also applies changes pre and post a decorated function. Wondering if it might be better to refactor this with decorator pattern?
I think the pros are a) self._call_tool is not touched, b) more modularized, c) it seems to me that the pre/post hooks need access to many members of TS. Instead of relying on Event classes we might rather give them full access to self. Though I also worry this seems an overkill instead of just modifying _call_tool. @poshinchen thoughts?
Uh oh!
There was an error while loading. Please reload this page.
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.
Yeah the reason I went with hooks over subclassing was specifically because
_call_toolis private. I didn't want to couple to its internals or commit its signature as public API. The event classes keep the contract narrow so_call_toolcan be refactored freely. But I'm open to a different approach if you have a preference. I would be happy to rework it.Edit: Good point on the decorator pattern. I think the inline approach is simpler here, it avoids re-parsing
parameters_stringacross decorators and keeps the flow in one place. Happy to wait for @poshinchen's take.