Skip to content

Python: fix(orchestrations): forward workflow run kwargs to the GroupChat orchestrator - #8314

Open
Yufeng He (he-yufeng) wants to merge 3 commits into
microsoft:mainfrom
he-yufeng:fix/groupchat-orchestrator-run-kwargs
Open

Python: fix(orchestrations): forward workflow run kwargs to the GroupChat orchestrator#8314
Yufeng He (he-yufeng) wants to merge 3 commits into
microsoft:mainfrom
he-yufeng:fix/groupchat-orchestrator-run-kwargs

Conversation

@he-yufeng

Copy link
Copy Markdown
Contributor

Motivation & Context

workflow.run(..., function_invocation_kwargs=..., client_kwargs=...) is stored in workflow state and forwarded to participant agents via AgentExecutor, but the GroupChat orchestrator's own agent.run() never received them. Hosts that put per-run values there (user_id for tool ACLs, request-scoped ids for metrics) worked for participants and silently lost them on the orchestrator, which for a tool-bearing orchestrator means the manager agent runs with fewer rights than the team it manages.

Description & Review Guide

  • What are the major changes? _invoke_agent on the GroupChat orchestrator now takes the WorkflowContext its callers already hold, resolves the run kwargs from WORKFLOW_RUN_KWARGS_KEY state, and forwards them to the orchestrator's agent.run. The resolution logic (_prepare_agent_run_args / _resolve_executor_kwargs) is extracted to module-level helpers in _agent_executor.py so the executor and the orchestrator share one implementation; the executor's methods are now thin delegates.
  • What is the impact of these changes? The orchestrator's agent receives the same run kwargs participants get, including the per-executor and __global__ resolution semantics. No change for runs that don't pass kwargs.
  • What do you want reviewers to focus on? Whether resolving with the orchestrator's own executor id is the right keying (vs. the participant's id); and the deliberate scope cut below.

Scope cut: the Magentic manager's _complete has the same gap, but plan/replan/create_progress_ledger/prepare_final_answer are public on MagenticManagerBase, and threading a context through them breaks custom manager subclasses that override those methods (I hit exactly that in the existing test doubles). I'd rather land the GroupChat fix and do Magentic as a follow-up with a design you prefer (context var vs. signature change) than guess. Noted on the issue.

Related Issue

Toward #8304 (GroupChat half; Magentic half deliberately deferred as described above)

Contribution Checklist

  • The code builds clean without any errors or warnings
  • All unit tests pass, and I have added new tests where possible
  • The PR follows the Contribution Guidelines
  • The PR is linked to an issue and there is no other open PR for this issue (see Related Issue above).
  • This is not a breaking change. If it is a breaking change, add the breaking change label (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.

…Chat orchestrator

workflow.run(..., function_invocation_kwargs=..., client_kwargs=...) reaches
participant agents through AgentExecutor, but the GroupChat orchestrator's
own agent.run never saw them. _invoke_agent now takes the WorkflowContext
its callers already hold, resolves the run kwargs through the same
resolution AgentExecutor uses (extracted into module-level helpers so both
call sites share it), and forwards them.

Magentic's manager._complete has the same gap but public signatures on
MagenticManagerBase make threading the context a bigger contract change;
deferred deliberately, noted on the issue.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Per-executor orchestrator routing needs direct coverage, and the helper docstring incorrectly claims Magentic support.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Forwards workflow run kwargs to GroupChat orchestrator agents and centralizes kwargs resolution.

Changes:

  • Extracts shared run-kwargs resolution helpers.
  • Passes resolved kwargs to the GroupChat orchestrator.
  • Adds forwarding coverage.
File summaries
File Description
_agent_executor.py Adds shared kwargs-resolution helpers.
_group_chat.py Forwards kwargs to orchestrator runs.
test_group_chat.py Tests orchestrator kwargs forwarding.
Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 2
  • Review effort level: Balanced

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +318 to +319
function_invocation_kwargs={"user_id": "u-123"},
client_kwargs={"trace_id": "t-456"},
Comment on lines +78 to +80
Shared by ``AgentExecutor`` and the orchestrator/manager call sites that
invoke an agent inside a workflow (GroupChat orchestrator, Magentic
manager), so run kwargs reach every agent run, not just participants.
…e orchestrator

Copilot review on microsoft#8314: the helper docstring claimed Magentic as a caller
while that side is explicitly deferred, and the kwargs test only exercised
the global form, so a wrong executor id at the orchestrator call site would
still have passed. Narrow the docstring to the actual callers and add a
per-executor case keyed by the orchestrator's own executor id, with a
participant-keyed entry as the negative control.
@he-yufeng

Copy link
Copy Markdown
Contributor Author

Addressed both Copilot comments in 604ae57:

  • The prepare_agent_run_kwargs docstring no longer lists the Magentic manager as a caller; it now describes only AgentExecutor and the GroupChat orchestrator call site, matching the deferred scope called out in the PR description.
  • Added test_orchestrator_agent_receives_per_executor_run_kwargs: kwargs keyed by the orchestrator's own executor id (resolve_agent_id(manager)) reach agent.run, while an entry keyed by a participant executor does not leak into the orchestrator's kwargs. The global-form test stays as coverage for the __global__ path.

Verified locally: both kwargs tests plus the full test_group_chat.py and the core agent-executor tests pass; ruff check and format are clean on the touched files.

The conflict in _resolve_executor_kwargs is resolved by keeping the
helper delegation and moving upstream microsoft#7963's mixed-kwargs merge
semantics (global fills, the executor entry wins per key) into
resolve_executor_run_kwargs, so the shared helper matches current main
behavior instead of the winner-takes-all routing it was extracted from.
@he-yufeng

Copy link
Copy Markdown
Contributor Author

Conflict with main resolved in the merge just pushed. The interesting bit: main landed #7963 (mixed __global__ + per-executor kwargs with per-key override) after this branch was cut, so the shared helper now carries that merge semantics — global fills, the executor entry wins per key — instead of the winner-takes-all routing it was originally extracted from. The orchestrator forwarding behavior this PR adds is unchanged; the group-chat tests and the workflow kwargs suites pass against the merged head (105 passed locally), and pyright/ruff are clean on the touched file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

python Usage: [Issues, PRs], Target: Python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants