-
Notifications
You must be signed in to change notification settings - Fork 314
tool executors #658
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?
tool executors #658
Conversation
@@ -306,122 +304,6 @@ async def recurse_event_loop(agent: "Agent", invocation_state: dict[str, Any]) - | |||
recursive_trace.end() | |||
|
|||
|
|||
async def run_tool(agent: "Agent", tool_use: ToolUse, invocation_state: dict[str, Any]) -> ToolGenerator: |
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.
Moved logic into strands.executors._executor.Executor._stream
|
||
async def acall() -> ToolResult: | ||
# Pass kwargs as invocation_state | ||
async for event in run_tool(self._agent, tool_use, kwargs): |
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.
Moved run_tool
logic into new ToolExecutor._stream
method.
return wrapper | ||
|
||
@_trace | ||
async def stream( |
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.
Logic copied from the now removed run_tool
method in event_loop.py
.
|
||
validate_and_prepare_tools(message, tool_uses, tool_results, invalid_tool_use_ids) | ||
tool_uses = [tool_use for tool_use in tool_uses if tool_use.get("toolUseId") not in invalid_tool_use_ids] |
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.
optional nit: any reason this needs to be modified in place? I know we do it in the code base but I find it harder to follow
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.
We could follow up on this (and a lot of code in this PR). For now though, I figured I would just try to copy and paste as much as possible rather than fully alter the existing logic to simplify the PR. In other words, this PR more so focuses on code organization and placement rather than the logic.
…nto tool-execution
src/strands/agent/agent.py
Outdated
@@ -35,6 +35,8 @@ | |||
from ..session.session_manager import SessionManager | |||
from ..telemetry.metrics import EventLoopMetrics | |||
from ..telemetry.tracer import get_tracer, serialize | |||
from ..tools.executors import ConcurrentToolExecutor | |||
from ..tools.executors._executor import Executor as ToolExecutor |
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.
Per discussion, indicating that the base Executor class is private rather than experimental to discourage any use.
|
||
async def acall() -> ToolResult: | ||
# Pass kwargs as invocation_state | ||
async for event in run_tool(self._agent, tool_use, kwargs): | ||
async for event in ToolExecutor._stream(self._agent, tool_use, tool_results, invocation_state): |
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.
Per discussion, member methods of the executor classes are also marked private to discourage customer use. We want to first cement the interface before exposing.
"""Abstract base class for tool executors.""" | ||
|
||
@staticmethod | ||
async def _stream( |
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.
Logic is copy and pasted from run_tool
in event_loop.py
.
tool_results.append(after_event.result) | ||
|
||
@staticmethod | ||
async def _stream_with_trace( |
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.
Previously I tried setting this up as a decorator. For more clarity, I went ahead and made it a separate method. Note, this logic is copied from tools/executor.py
which is removed in this PR.
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.
Can you update the PR with:
- The new public APIs being exposed
- Expected way that the customer would use the new apis?
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.
Also add a section about the expected follow-ups that are decided on - if this is a stepping stone to the long-term solution
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.
Added "Usage" and "Follow Up" sections to the overview.
…nto tool-execution
Description
For more details, please see comments in file diff.
Usage
Sequential
Assuming the model returns
screenshot_tool
andemail_tool
use requests, theSequentialToolExecutor
will execute both sequentially in the order given.Concurrent
Assuming the model returns
weather_tool
andtime_tool
use requests, theConcurrentToolExecutor
will execute both concurrently. Note, this is the default executor and soAgent(tools=[weather_tool, time_tool])
will result in the same behavior.Related Issues
#614
Documentation PR
Will follow up
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 prepare
: Wrote new unit and integration testsChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Follow Up
Will document the following items in an issue tracker after PR approval:
Executor
class implements_stream
and_stream_with_trace
methods to wrap the tool execution with additional logic that includes tracing, hooks, exception handling, etc. It is a bit cumbersome and would maybe make more sense to be placed in theAgentTool
implementations.execute
method interface.tool_results
.execute
could instead work as a generator that yields results upon completion that the caller (insideevent_loop.py
) collects. For this to work however, we require strongly typed events.invocation_state
as this parameter is on a deprecation path.Executor
as a public interface after clean up.