-
Notifications
You must be signed in to change notification settings - Fork 421
feat(multiagent): Add stream_async #961
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?
Conversation
|
||
# Yield final result (consistent with Agent's AgentResultEvent format) | ||
result = self._build_result() | ||
yield {"result": result} |
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.
Yield final result (consistent with Agent's AgentResultEvent format)
Is this an AgentResult as it is for single-agent streaming? If not, can we rename so that it doesn't conflict with different types
Yields: | ||
Dictionary events containing graph execution information including: | ||
- MultiAgentNodeStartEvent: When a node begins execution |
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.
These types aren't exposed to customers, so we should either remove these docs or document the shape of the dictionaries being emited
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 also list out the the new events to the PR description (similar to #788) along with the signatures of the new apis being added. This well help the PR be more akin to the spec of what's being proposed
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.
so we should either remove these docs AND document the shape of the dictionaries being emitted*
try: | ||
event = await asyncio.wait_for(async_generator.__anext__(), timeout=timeout) | ||
yield event | ||
except StopAsyncIteration: |
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.
Is this always thrown at the end and thus part of normal execution?
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.
nit: seems like theres might be a more pythonic way to do this without the while
async with asyncio.timeout(timeout):
async for event in async_generator:
# because a failure would throw exception and code would not make it here | ||
ready_nodes.extend(self._find_newly_ready_nodes(current_batch)) | ||
|
||
async def _execute_nodes_parallel( |
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.
Does this follow the pattern of how tools are executed in parallel - I know we worked out some wrinkles/bugs with that, so it'd be ideal to not have to redo those
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, does this have to be in this PR? Can we do the streaming then add parallel execution as a different commit to simplify the review?
start_event = MultiAgentNodeStartEvent( | ||
node_id=node.node_id, node_type="agent" if isinstance(node.executor, Agent) else "multiagent" | ||
) | ||
yield start_event.as_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.
Could we do this at a higher level instead of in here? That way we can ensure this method is always returning TypedEvent
s
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.
Same for below
wrapped_event = MultiAgentNodeStreamEvent(node.node_id, event) | ||
yield wrapped_event.as_dict() | ||
# Capture the final result event | ||
if "result" in event: |
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 we just do an isinstance
check here?
if isinstance(node.executor, MultiAgentBase): | ||
# For nested multi-agent systems, stream their events and collect result | ||
multi_agent_result = None | ||
if self.node_timeout is not None: |
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.
Rather than having two code paths, can _stream_with_timeout
special_case no timeout to infinite?
If not, perhaps a conditional like so:
events = self._stream_with_timeout(...) if timeout else self.stream_async()
So that the if blocks are unified
elif isinstance(node.executor, Agent): | ||
# For agents, stream their events and collect result | ||
agent_response = None | ||
if self.node_timeout is not None: |
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.
Same comments as above - these if blocks have the same content, so let's find a way to unify them
to_node=self.state.current_node.node_id, | ||
message=self.state.handoff_message or "Agent handoff occurred", | ||
) | ||
yield handoff_event.as_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.
As noded for graphs, can we do the as_dict() stuff higher up? I'd like to be strongly typed at the lower-levels and only do dict conversion at the api layer
|
||
except Exception: | ||
logger.exception("node=<%s> | node execution failed", current_node.node_id) | ||
except Exception as e: |
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.
Why can't we use an exception type for this? This seems hacky
status=Status.EXECUTING, | ||
task=task, | ||
total_nodes=len(self.nodes), | ||
edges=[(edge.from_node, edge.to_node) for edge in self.edges], |
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.
note related to this PR, but why doesn't GraphState take Iterable[GraphEdge] instead of edges: list[Tuple["GraphNode", "GraphNode"]] = field(default_factory=list)
Did GraphEdge come later?
try: | ||
event = await asyncio.wait_for(async_generator.__anext__(), timeout=timeout) | ||
yield event | ||
except StopAsyncIteration: |
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.
nit: seems like theres might be a more pythonic way to do this without the while
async with asyncio.timeout(timeout):
async for event in async_generator:
# because a failure would throw exception and code would not make it here | ||
ready_nodes.extend(self._find_newly_ready_nodes(current_batch)) | ||
|
||
async def _execute_nodes_parallel( |
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, does this have to be in this PR? Can we do the streaming then add parallel execution as a different commit to simplify the review?
self, async_generator: AsyncIterator[dict[str, Any]], timeout: float, timeout_message: str | ||
) -> AsyncIterator[dict[str, Any]]: | ||
"""Wrap an async generator with timeout functionality.""" | ||
while True: |
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.
same nit as in graph
logger.exception("node=<%s> | node execution failed", current_node.node_id) | ||
except Exception as e: | ||
# Check if this is a timeout exception | ||
if "timed out after" in str(e): |
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.
nit: can we create a variable for this so we don't accidentally change the message in exception
Description
This PR adds streaming support to the Swarm and Graph multi-agent systems, enabling real-time event emission during multi-agent execution. This brings multi-agent systems to feature parity with the single Agent class streaming capabilities.
Key Changes
New Event Types (
src/strands/types/_events.py
):MultiAgentNodeStartEvent
: Emitted when a node begins executionMultiAgentNodeCompleteEvent
: Emitted when a node completes executionMultiAgentNodeStreamEvent
: Forwards agent events with node contextMultiAgentHandoffEvent
: Emitted during agent handoffs in Swarm (includes from_node, to_node, and message)Swarm Streaming (
src/strands/multiagent/swarm.py
):stream_async()
method that yields events during executioninvoke_async()
to usestream_async()
internally (maintains backward compatibility)Graph Streaming (
src/strands/multiagent/graph.py
):stream_async()
method for real-time event streaminginvoke_async()
to consumestream_async()
eventsTesting:
Benefits
Related Issues
Documentation PR
Type of Change
New feature
Testing
How have you tested the change?
tests/strands/multiagent/test_swarm.py
,tests/strands/multiagent/test_graph.py
)Verify that the changes do not break functionality or introduce warnings in consuming repositories: agents-docs, agents-tools, agents-cli
hatch run prepare
Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.