Python: feat(core): add tool concurrency groups and sequential execution order - #7881
Conversation
GitHub Actions
Active
GitHub Actions
Active
GitHub Actions
Active
GitHub Actions
Active
agent-framework-automation
Bot
added
the
python
github-actions
Bot
changed the title
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: Findings reported
Scope: full PR (5 commit(s)): e3efb1e57d81, b8b1f034fbdc, 7783ec2d9c37, fb4cb4f75eb1, e55413014eb4
Model: gpt-5.6-sol
Overview
The PR adds per-tool concurrency groups and a run-level sequential mode while preserving result order, per-call context propagation, and cancellation of in-flight group tasks. The new grouping loop has gaps around fail-closed middleware, middleware-requested termination, and approval replay ordering, and its configuration/key handling can silently violate the requested execution policy. These issues can start side-effecting calls after a policy stop, reverse dependent operations, or unexpectedly serialize or parallelize a batch.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
5 verified findings remained after source verification (1 high, 4 medium) across 1 file. Details are attached to the affected lines below.
Affected areas: python/packages/core/agent_framework/_tools.py
| live_tools=live_tools, | ||
| ), | ||
| ) | ||
| res = await task |
There was a problem hiding this comment.
When one group raises MiddlewareFailure, another group that has just completed a call can advance to its next queued tool before the outer gather() observes the failure and cancels the group tasks. That lets a side-effecting call start after a fail-closed middleware abort, contrary to the function-loop contract that no further tool call starts. Please propagate a batch-wide abort signal at individual-call completion and check it before dequeuing each subsequent call.
| live_tools=live_tools, | ||
| ), | ||
| ) | ||
| res = await task |
There was a problem hiding this comment.
_execute_single_function_call returns should_terminate=True for MiddlewareTermination, but this loop records the tuple and immediately starts the next call in the same group. With sequential execution, a policy middleware can request approval or termination and a later side-effecting tool still runs before the flag is checked after the entire batch. Please stop dequeuing calls as soon as a result requests termination and prevent other groups from starting additional queued work.
| ordered_results: list[tuple[list[Content], bool] | None] = [None] * len(function_calls) | ||
|
|
||
| async def _execute_group(indices: list[int]) -> None: | ||
| for idx in indices: |
There was a problem hiding this comment.
This assumes function_calls remains in model-request order, but approval resume collects explicit responses before appending the previously hidden safe siblings. A batch emitted as [write, approval-required read] can therefore resume as [read, write], and this serial loop executes the dependency backward. Please preserve each call's original batch ordinal through approval storage and merge resumed calls by that ordinal before grouping.
| ) -> _FunctionExecutionBatch: | ||
|
|
||
| run_config = cast("FunctionInvocationConfiguration", dict(config) if config else {}) | ||
| if custom_args and "tool_execution_order" in custom_args: |
There was a problem hiding this comment.
This assignment runs after the invocation layer has applied the explicit per-run option, so function_invocation_kwargs["tool_execution_order"] silently overwrites options["tool_execution_order"] despite the preceding log saying the explicit option wins. It also lets an ordinary injected tool argument change framework scheduling. Please consume this reserved key when constructing run_config, apply it only when no explicit option was supplied, and avoid forwarding it as a tool runtime argument.
| if tool is not None: | ||
| group_key = getattr(tool, "concurrency_group", None) | ||
| if group_key is None: | ||
| group_key = "__sequential_all__" if execution_order == "sequential" else f"__ungrouped_{idx}" |
There was a problem hiding this comment.
Synthetic keys share the same string namespace as user-provided concurrency_group values. For example, a named group __ungrouped_1 collides with the ungrouped call at index 1, unexpectedly serializing them and potentially deadlocking when the first waits for the second. Please use structurally distinct internal keys or object sentinels so user group names cannot collide with scheduler keys.
|
pratik wayase (@PratikWayase) Thanks for working on this. I think we should narrow the scope of this PR to the batch-wide execution control and make two changes to that API:
I am not yet convinced that we have resolved enough of the design for per-tool I opened #7914 to design the selective tool-level controls separately and tagged Roger Barreto (@rogerbarreto) there for input on whether we should address this in .NET and Python together. I suggest removing |
Motivation & Context
Currently, the framework executes all tool calls requested in a single assistant message concurrently. While this is a great default for independent calls (like parallel document lookups), models routinely emit dependent calls in one batch. Because the tool author has no way to serialize these calls, dependent reads race the still-running writes, leading to "not found" errors and contradictory agent states.
This PR closes that gap by providing declarative, framework-level control over tool execution order, preventing stateful tool race conditions without relying on fragile, tool-side asyncio.Lock workarounds.
Fixes #7386
Description & Review Guide
What are the major changes?
concurrency_group: strparameter toFunctionTooland the@tooldecorator. Tools sharing the sameconcurrency_groupexecute sequentially in call order within a message batch, while ungrouped tools remain fully concurrent.tool_execution_order: Literal["parallel", "sequential"]to_ChatOptionsBaseandFunctionInvocationConfiguration. Setting this to"sequential"forces all tool calls in a batch to execute one-by-one.tool_execution_orderchat option through theFunctionInvocationLayerdown to the execution engine so the setting actually takes effect at runtime.FunctionTool.to_dict()to ensureconcurrency_groupsurvives serialization, and added docstrings documenting the ordering guarantee.task.cancel()+return_exceptions=Truepattern at the group-level gather to preserve fail-closed middleware behavior.What is the impact of these changes?
This is fully backward compatible. The default behavior remains
"parallel"with noconcurrency_groupset, ensuring existing agents behave exactly as before. It provides tool authors a safe, declarative way to handle stateful dependencies.What do you want reviewers to focus on?
Please review the grouping algorithm in
_try_execute_function_call_groups(_tools.py). Specifically, verify that:ordered_resultsarray correctly maps indices to ensure results are returned in the exact order the model requested them.contextvars.copy_context()is still applied correctly per-call inside_execute_groupto preserve agent span observability.asyncio.gather(not individual call tasks).Related Issue
Fixes #7386
Contribution Checklist