Repository navigation
Python: feat(core): add tool concurrency groups and sequential execution order - #8204
pratik wayase (PratikWayase) wants to merge 21 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Hosted-call isolation, context isolation, and approval replay ordering remain incorrect.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds configurable sequential tool execution while preserving concurrent execution by default.
Changes:
- Adds batch-wide invocation ordering and termination handling.
- Hardens hosted-tool and approval replay handling.
- Adds sequential execution coverage and formatting updates.
File summaries
| File | Description |
|---|---|
python/packages/core/agent_framework/_tools.py |
Implements execution policy, hosted-tool filtering, and approval replay ordering. |
python/packages/core/tests/core/test_tools.py |
Tests sequential invocation order. |
python/packages/core/agent_framework/_workflows/_agent.py |
Formatting-only update. |
python/packages/core/agent_framework/_harness/_loop.py |
Formatting-only updates. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Eduard van Valkenburg (eavanvalkenburg)
left a comment
There was a problem hiding this comment.
A few concerns to address before this is ready.
|
Thanks for the update. Before this is ready, could you please:
Once those are addressed, please re-request review. Thanks! |
…github.com/PratikWayase/agent-framework into feat/serialize-same-message-function-calls
|
Thanks for the follow-up, Eduard van Valkenburg (@eavanvalkenburg) |
|
Thanks for the update. The earlier review discussions and merge conflicts are now resolved. The remaining blocker is that the PR body is missing the breaking-change checklist declaration from the current repository template. Could you please restore and complete that item? The approval-gated workflows can then be run and the PR reviewed. |
Thanks, Eduard! |
|
Thanks for the update. I verified that this head already contains the current |
|
pratik wayase (@PratikWayase) please fix the file conflicts. |
…github.com/PratikWayase/agent-framework into feat/serialize-same-message-function-calls
|
Thank you, pratik wayase (@PratikWayase), for investigating this issue and for the work across #7523, #7881, and this PR. Your iterations established the batch-wide |
PR Description
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, models routinely emit dependent calls in one batch (e.g., "write the file, then read it"). Because the framework lacked a batch-wide execution control, dependent reads could race still-running writes, leading to "not found" errors and contradictory agent states.
This PR closes that gap by providing a declarative, framework-level boolean to control batch-wide execution order, preventing stateful tool race conditions without relying on fragile, tool-side
asyncio.Lockworkarounds.Note on PR History: This PR supersedes and closes #7523 and #7881. It incorporates all maintainer feedback from those previous iterations, specifically narrowing the scope to batch-wide execution control (removing
concurrency_groupper #7914), centralizing the configuration, and resolving all architectural and CI blockers raised by reviewers.Description & Review Guide
What are the major changes?
allow_concurrent_invocation: booltoFunctionInvocationConfiguration(defaulting toTrueto preserve existing parallel behavior). When set toFalse, tools in the same batch execute one-by-one._try_execute_function_call_groups, whenallow_concurrent_invocationisFalseand a call requests termination, the loop now injects"Skipped: a prior tool call in this batch requested termination."results for all subsequent calls, ensuring provider continuation history remains fully resolved.config_provider: Callable[[], FunctionInvocationConfiguration]in_execute_function_calls. This ensures dynamically registered tools (e.g., AG-UI server-managed placeholders) are respected during execution._is_server_managed_tool()filter to_get_tool_map()and a guard in_execute_single_function_callto guarantee hosted tools are never executed locally.original_indexin a parallel array for hidden already-approved requests. This ensures that resumed calls merge by their original batch position, restoring model order instead of reversing dependent operations.serialized_requestsand avoiding polluting theContentdictionaries with framework metadata.test_try_execute_function_call_groups_sequential_configto verify execution order, and applied minor formatting fixes to_loop.pyand_agent.py.What is the impact of these changes?
This is fully backward compatible. The default behavior remains parallel execution (
True). It provides tool authors a safe, declarative way to handle stateful dependencies, fixes a critical server-tool execution bug, and ensures strict type safety.What do you want reviewers to focus on?
Please review the execution logic in
_try_execute_function_call_groups(_tools.py). Specifically, verify that:config_providerinFunctionInvocationLayer.get_responsecleanly evaluates live state without leaking options to the underlying chat client._is_server_managed_toolfilter cleanly prevents theget_time_zonelocal execution bug.allow_concurrent_invocationisFalse, the loop correctly breaks, stops dequeuing calls, and injects terminal "Skipped" results for the suffix.original_indextracking in_store_already_approved_approval_requestsand_pop_already_approved_approval_responsescorrectly restores model order during sequential approval resumes.Related Issue
Fixes #7386
Supersedes and closes #7523
Supersedes and closes #7881
Design for deferred per-tool controls: #7914
Contribution Checklist