-
Notifications
You must be signed in to change notification settings - Fork 680
Python: Fix orchestration patterns when using workflow as an agent #1470
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
Conversation
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.
Pull Request Overview
This PR fixes orchestration pattern workflows (Magentic, Sequential, and Concurrent) when used as agents via .as_agent(). The main issue was that workflow start executors couldn't properly handle different input types when wrapped as agents.
- Enhanced
WorkflowAgentwith flexible input type handling and payload encoding for different workflow types - Added support for
MagenticBuilder,SequentialBuilder, andConcurrentBuilderwhen running as agents - Added comprehensive test coverage and sample code demonstrating the patterns
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
python/packages/core/agent_framework/_workflows/_agent.py |
Enhanced WorkflowAgent with sophisticated input type resolution and payload encoding system |
python/packages/core/agent_framework/_workflows/_magentic.py |
Added as_agent() method to MagenticWorkflow for consistency |
python/packages/core/tests/workflow/test_workflow_agent.py |
Added comprehensive tests for Sequential, Concurrent, and Magentic builders as agents |
python/samples/getting_started/workflows/agents/*.py |
Added sample implementations demonstrating each orchestration pattern as an agent |
python/samples/getting_started/workflows/README.md |
Updated documentation table to include new workflow-as-agent samples |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| return self._workflow | ||
|
|
||
| def as_agent(self, name: str | None = None) -> "WorkflowAgent": | ||
| """Expose the underlying workflow as a WorkflowAgent.""" |
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.
In this doc string we should note the message type conversion behavior rather than making this behavior implicit.
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 do we need to call out the message type conversion? This is simply creating a "WorkflowAgent". We don't call anything specifically even in the workflow's method:
def as_agent(self, name: str | None = None) -> WorkflowAgent:
"""Create a WorkflowAgent that wraps this workflow.
Args:
name: Optional name for the agent. If None, a default name will be generated.
Returns:
A WorkflowAgent instance that wraps this workflow.
"""
# Import here to avoid circular imports
from ._agent import WorkflowAgent
return WorkflowAgent(workflow=self, name=name)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.
I thought user may want to understand how their input messages are being handled under the hood to become input to the first executor/agent? The Workflow.as_agent() method requires the first executor to handle list[ChatMessage] type as part of the original WorkflowAgent constructor and raises error if not the case. It may be helpful to document these transformations so when user reading the doc, they can see the underlying behavior if they need to understand it. It doesn't need to be very detailed but having it avoids guesswork, as the new behavior is more like "parsing" an input type rather than simply passing it along.
|
|
||
| raise ValueError("Workflow's start executor cannot be adapted to agent chat inputs.") | ||
|
|
||
| def _candidate_adapters_from_input_types( |
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: I know these are "private" methods, but it'd still be beneficial to have some comments for them. It'd make reading and understanding the code so much easier for others and potential AI. And maybe copilot could detect errors if the method doesn't do what the description says.
| adapters.append(adapter) | ||
| return adapters | ||
|
|
||
| def _flatten_type_annotation(self, annotation: Any) -> list[Any]: |
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.
I wonder if we need to support the cases when the input executor cannot handle chat message.
|
|
||
| return None | ||
|
|
||
| def _adapter_for_specialized_class(self, message_cls: type[Any]) -> Callable[[list[ChatMessage]], Any] | 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.
nit: this is probably ok, but I have observed that we have too many lower-level modules taking dependencies on higher level modules. At some point, we should think about some of the design choices we are making to make the code a bit organized.
|
|
||
| return None | ||
|
|
||
| def _build_magentic_start_adapter( |
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.
Here too, though it's not taking a dependency on the MagenticBuilder but it's logically related. I don't think this module should know anything about the Magentic orchestration.
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.
I think I may abandon this PR. I'm working on introducing the base group chat pattern, which the Magentic pattern will extend (as we talked about a while back). There's too much custom "MagenticWorkflow" handling that I don't like as well. I expect we will be able to move away from these custom methods.
|
Going to close this PR in favor of a new one that does more cleanup for orchestrations. |
Motivation and Context
Workflows supports the ability to be run as an agent using
.as_agent(). There is an issue doing this for magentic orchestrations (leads to an error). Also, making sure to add support for SequentialBuilder and ConcurrentBuilder.Description
.as_agent()).Contribution Checklist