feat: Implement the core system of typed hooks & callbacks #304
Conversation
Relates to strands-agents#231 Add the HookRegistry and a small subset of events (AgentInitializedEvent, StartRequestEvent, EndRequestEvent) as a POC for how hooks will work.
src/strands/hooks/registry.py
Outdated
| ``` | ||
| """ | ||
|
|
||
| def __call__(self, event: Any) -> None: |
There was a problem hiding this comment.
nit: can we now have this as event: HookEvent
There was a problem hiding this comment.
whelp - this should be TEvent.
Fixed in the latest commit
| registry.invoke_callbacks(event) | ||
| ``` | ||
| """ | ||
| for callback in self.get_callbacks_for(event): |
There was a problem hiding this comment.
we have #306, is there a use case for async callbacks?
Should all callbacks be async? If they are, should they always or conditionally be blocking?
Use case I'm considering is a callback where a metric wants to be emitted. I expect a large number of telemetry sdks are async native. So customers would have the extra work of modeling them synchrnously.
There was a problem hiding this comment.
Hmmm... probably.
I'm going to bring this up to the team as a double-check, as I think this makes sense.
I'm going to defer to a follow-up CR, however, as I need to think on this.
| @@ -320,6 +322,10 @@ def __init__( | |||
| self.name = name | |||
| self.description = description | |||
|
|
|||
| self._hooks = HookRegistry() | |||
| # Register built-in hook providers (like ConversationManager) here | |||
There was a problem hiding this comment.
should this comment be here? Or should this be more of a todo to refactor conversational manager? Seems like the comment is really linked to the line below
There was a problem hiding this comment.
Yeah - it's intended to a be a placeholder that is "Add built-in hook providers here" before Initialize is called. That way the conversation manager gets the AgentInitializedEvent
| @@ -320,6 +322,10 @@ def __init__( | |||
| self.name = name | |||
| self.description = description | |||
|
|
|||
| self._hooks = HookRegistry() | |||
There was a problem hiding this comment.
Why _hooks and not hooks? Are you planning on exposing this in a later pr?
There was a problem hiding this comment.
Yes, exposing later after the the exact contracts are figured out. I figure as long as we keep it internal, we can tweak things before making it publicly accessible. LMK if you think a bigger "all-at-once" approach would be more ideal
|
|
||
|
|
||
| @dataclass | ||
| class StartRequestEvent(HookEvent): |
There was a problem hiding this comment.
When specifically is this event triggered? Is it just agent("Hello!"), or are there other entry points like direct tool calling or structured_output?
|
|
||
| def __init__(self) -> None: | ||
| """Initialize an empty hook registry.""" | ||
| self._registered_callbacks: dict[Type, list[HookCallback]] = {} |
There was a problem hiding this comment.
Should this be:
| self._registered_callbacks: dict[Type, list[HookCallback]] = {} | |
| self._registered_callbacks: dict[TEvent, list[HookCallback]] = {} |
?
There was a problem hiding this comment.
In this case it would be TYPE[TEvent] but it doesn't make sense as there's nothing that indicates what TEvent would be bound to (Python typechecker complains).
It's equivalent to Java where a method might be generic, but a map that stores data would type it as non-generic Object
Description
Add the HookRegistry and a small subset of events (AgentInitializedEvent, StartRequestEvent, EndRequestEvent) as a POC for how hooks will work.
This is intentionally only a subset of events listed as called out in #231 (comment) because I wanted to focus on the overall idea rather than the specific events. Follow-up PRs will focus on adding/implementing additional events.
Hooks are not yet publicly exposed to prevent builders from starting to rely on it.
Related Issues
#231
Documentation PR
N/A - will work on docs after more events are added/plumbed through
Type of Change
New feature
Testing
How have you tested the change? Verify that the changes do not break functionality or introduce warnings in consuming repositories: agents-docs, agents-tools, agents-cli
hatch run prepareChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.