Skip to content

[BREAKING] Python: Add factory pattern to GroupChat and Magentic #3224

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.

Sign up for GitHub

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

Open
TaoChenOSU wants to merge 7 commits into
base: main
Choose a base branch
from
+1,309 −371
Open

[BREAKING] Python: Add factory pattern to GroupChat and Magentic #3224

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c940751
group chat
TaoChenOSU Jan 14, 2026
d13eb65
magentic
TaoChenOSU Jan 14, 2026
ec8d4b4
Merge branch 'main' into local-branch-python-group-chat-participant-f…
TaoChenOSU Jan 14, 2026
f245e4d
Fix tests
TaoChenOSU Jan 14, 2026
10137ef
AI comments
TaoChenOSU Jan 14, 2026
623e409
Unifiy error message and add warning
TaoChenOSU Jan 15, 2026
b25aa97
misc
TaoChenOSU Jan 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 6 additions & 11 deletions python/packages/core/agent_framework/_workflows/_concurrent.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,9 +292,7 @@ class MyCustomExecutor(Executor): ...
wf2 = ConcurrentBuilder().register_participants([create_researcher, MyCustomExecutor]).build()
"""
if self._participants:
raise ValueError(
"Cannot mix .participants([...]) and .register_participants() in the same builder instance."
)
raise ValueError("Cannot mix .participants() and .register_participants() in the same builder instance.")

if self._participant_factories:
raise ValueError("register_participants() has already been called on this builder instance.")
Expand Down Expand Up @@ -330,9 +328,7 @@ def participants(self, participants: Sequence[AgentProtocol | Executor]) -> "Con
wf2 = ConcurrentBuilder().participants([researcher_agent, my_custom_executor]).build()
"""
if self._participant_factories:
raise ValueError(
"Cannot mix .participants([...]) and .register_participants() in the same builder instance."
)
raise ValueError("Cannot mix .participants() and .register_participants() in the same builder instance.")

if self._participants:
raise ValueError("participants() has already been called on this builder instance.")
Expand Down Expand Up @@ -498,6 +494,10 @@ def with_request_info(

def _resolve_participants(self) -> list[Executor]:
"""Resolve participant instances into Executor objects."""
if not self._participants and not self._participant_factories:
raise ValueError("No participants provided. Call .participants() or .register_participants() first.")
# We don't need to check if both are set since that is handled in the respective methods

participants: list[Executor | AgentProtocol] = []
if self._participant_factories:
# Resolve the participant factories now. This doesn't break the factory pattern
Expand Down Expand Up @@ -549,11 +549,6 @@ def build(self) -> Workflow:

workflow = ConcurrentBuilder().participants([agent1, agent2]).build()
"""
if not self._participants and not self._participant_factories:
raise ValueError(
"No participants provided. Call .participants([...]) or .register_participants([...]) first."
)

# Internal nodes
dispatcher = _DispatchToAllParticipants(id="dispatcher")
aggregator = (
Expand Down
241 changes: 130 additions & 111 deletions python/packages/core/agent_framework/_workflows/_group_chat.py
View file
Open in desktop

Large diffs are not rendered by default.

41 changes: 15 additions & 26 deletions python/packages/core/agent_framework/_workflows/_handoff.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -599,7 +599,7 @@ def __init__(
self._participant_factories: dict[str, Callable[[], AgentProtocol]] = {}
self._start_id: str | None = None
if participant_factories:
self.participant_factories(participant_factories)
self.register_participants(participant_factories)

if participants:
self.participants(participants)
Expand All @@ -619,7 +619,7 @@ def __init__(
# Termination related members
self._termination_condition: Callable[[list[ChatMessage]], bool | Awaitable[bool]] | None = None

def participant_factories(
def register_participants(
Copy link
Contributor

@moonbox3 moonbox3 Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we align the HandoffBuilder.register_participants() signature with GroupChatBuilder and MagenticBuilder?

The HandoffBuilder uses Mapping[str, Callable[[], AgentProtocol]] (dict with explicit names) while the other two use Sequence[Callable[[], AgentProtocol | Executor]] (list)

Copy link
Contributor Author

@TaoChenOSU TaoChenOSU Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is by designed.

Agents in Handoff could potentially be configured to handoff only to a subset of other agents, thus requiring identifiers.

self, participant_factories: Mapping[str, Callable[[], AgentProtocol]]
) -> "HandoffBuilder":
"""Register factories that produce agents for the handoff workflow.
Expand All @@ -637,7 +637,7 @@ def participant_factories(
Self for method chaining.

Raises:
ValueError: If participant_factories is empty or `.participants(...)` or `.participant_factories(...)`
ValueError: If participant_factories is empty or `.participants(...)` or `.register_participants(...)`
has already been called.

Example:
Expand Down Expand Up @@ -666,17 +666,14 @@ def create_billing_agent() -> ChatAgent:

# Handoff will be created automatically unless specified otherwise
# The default creates a mesh topology where all agents can handoff to all others
builder = HandoffBuilder().participant_factories(factories)
builder = HandoffBuilder().register_participants(factories)
builder.with_start_agent("triage")
"""
if self._participants:
raise ValueError(
"Cannot mix .participants([...]) and .participant_factories() in the same builder instance."
)
raise ValueError("Cannot mix .participants() and .register_participants() in the same builder instance.")

if self._participant_factories:
raise ValueError("participant_factories() has already been called on this builder instance.")

raise ValueError("register_participants() has already been called on this builder instance.")
if not participant_factories:
raise ValueError("participant_factories cannot be empty")

Expand All @@ -694,8 +691,8 @@ def participants(self, participants: Sequence[AgentProtocol]) -> "HandoffBuilder
Self for method chaining.

Raises:
ValueError: If participants is empty, contains duplicates, or `.participants(...)` or
`.participant_factories(...)` has already been called.
ValueError: If participants is empty, contains duplicates, or `.participants()` or
`.register_participants()` has already been called.
TypeError: If participants are not AgentProtocol instances.

Example:
Expand All @@ -714,9 +711,7 @@ def participants(self, participants: Sequence[AgentProtocol]) -> "HandoffBuilder
builder.with_start_agent(triage)
"""
if self._participant_factories:
raise ValueError(
"Cannot mix .participants([...]) and .participant_factories() in the same builder instance."
)
raise ValueError("Cannot mix .participants() and .register_participants() in the same builder instance.")

if self._participants:
raise ValueError("participants have already been assigned")
Expand Down Expand Up @@ -896,7 +891,7 @@ def with_start_agent(self, agent: str | AgentProtocol) -> "HandoffBuilder":
if agent not in self._participant_factories:
raise ValueError(f"Start agent factory name '{agent}' is not in the participant_factories list")
else:
raise ValueError("Call participant_factories(...) before with_start_agent(...)")
raise ValueError("Call register_participants(...) before with_start_agent(...)")
self._start_id = agent
elif isinstance(agent, AgentProtocol):
resolved_id = self._resolve_to_id(agent)
Expand Down Expand Up @@ -1039,15 +1034,6 @@ def build(self) -> Workflow:
ValueError: If participants or coordinator were not configured, or if
required configuration is invalid.
"""
if not self._participants and not self._participant_factories:
raise ValueError(
"No participants or participant_factories have been configured. "
"Call participants(...) or participant_factories(...) first."
)

if self._start_id is None:
raise ValueError("Must call with_start_agent(...) before building the workflow.")

# Resolve agents (either from instances or factories)
# The returned map keys are either executor IDs or factory names, which is need to resolve handoff configs
resolved_agents = self._resolve_agents()
Expand All @@ -1058,6 +1044,8 @@ def build(self) -> Workflow:
executors = self._resolve_executors(resolved_agents, resolved_handoffs)

# Build the workflow graph
if self._start_id is None:
raise ValueError("Must call with_start_agent(...) before building the workflow.")
start_executor = executors[self._resolve_to_id(resolved_agents[self._start_id])]
builder = WorkflowBuilder(
name=self._name,
Expand Down Expand Up @@ -1096,8 +1084,9 @@ def _resolve_agents(self) -> dict[str, AgentProtocol]:
Returns:
Map of executor IDs or factory names to `AgentProtocol` instances
"""
if self._participants and self._participant_factories:
raise ValueError("Cannot have both executors and participant_factories configured")
if not self._participants and not self._participant_factories:
raise ValueError("No participants provided. Call .participants() or .register_participants() first.")
# We don't need to check if both are set since that is handled in the respective methods

if self._participants:
return self._participants
Expand Down
Loading
Loading