Skip to content

Python: Fix spurious Magentic custom manager warning#6261

Open
moonbox3 wants to merge 5 commits into
microsoft:mainfrom
moonbox3:fix-4306-magentic-manager-warning
Open

Python: Fix spurious Magentic custom manager warning#6261
moonbox3 wants to merge 5 commits into
microsoft:mainfrom
moonbox3:fix-4306-magentic-manager-warning

Conversation

@moonbox3
Copy link
Copy Markdown
Contributor

@moonbox3 moonbox3 commented Jun 2, 2026

Motivation and Context

Fixes #4306.

MagenticBuilder logged Custom manager provided; all other manager arguments will be ignored. whenever a custom manager or manager_factory was supplied, even when the caller did not pass any standard-manager options. This happened because max_stall_count defaulted to 3, so the warning check could not distinguish an omitted value from an explicit override.

Description

Updates MagenticBuilder to use the existing _MISSING sentinel for max_stall_count so omitted values remain distinguishable from explicitly supplied values. The builder resolves omitted max_stall_count to 3 only when constructing a StandardMagenticManager.

Adds regression coverage for custom manager and manager factory configurations that should not warn, preserves the warning when a standard-manager option is explicitly supplied with a custom manager, and verifies manager_agent_factory still receives the default max_stall_count.

Contribution Checklist

  • The code builds clean without any errors or warnings
  • The PR follows the Contribution Guidelines
  • All unit tests pass, and I have added new tests where possible
  • Is this a breaking change? If yes, add "[BREAKING]" prefix to the title of the PR.

Copilot AI review requested due to automatic review settings June 2, 2026 09:06
@moonbox3 moonbox3 added the python label Jun 2, 2026
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Automated Code Review

Reviewers: 4 | Confidence: 94% | Result: All clear

Reviewed: Correctness, Security Reliability, Test Coverage, Design Approach


Automated review by moonbox3's agents

Copy link
Copy Markdown
Contributor

Copilot AI left a 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 a misleading warning in the Python orchestration MagenticBuilder where supplying a custom manager/manager_factory always triggered “Custom manager provided; all other manager arguments will be ignored.” due to max_stall_count having a non-sentinel default. It makes omitted max_stall_count distinguishable from an explicit override, and adds regression tests around the warning behavior and default resolution.

Changes:

  • Update MagenticBuilder / _set_manager to use the existing _MISSING sentinel for max_stall_count, resolving it to 3 only when constructing StandardMagenticManager.
  • Adjust the warning condition so it only fires when standard-manager options were actually provided alongside a custom manager.
  • Add tests to verify (1) no warning for custom manager/manager_factory without standard options, (2) warning still occurs when a standard option is explicitly set, and (3) manager_agent_factory still uses the default max_stall_count=3.

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.

File Description
python/uv.lock Updates locked dependencies (notably azure-ai-agentserver-responses to 1.0.0b7 and related transitive additions) consistent with current workspace requirements.
python/packages/orchestrations/tests/test_magentic.py Adds regression tests covering the warning behavior and default max_stall_count resolution for agent factory builds.
python/packages/orchestrations/agent_framework_orchestrations/_magentic.py Introduces _MISSING sentinel handling for max_stall_count and refines warning logic; resolves default only when instantiating StandardMagenticManager.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 2, 2026

Python Test Coverage

Python Test Coverage Report •
FileStmtsMissCoverMissing
packages/bedrock/agent_framework_bedrock
   _chat_client.py4459877%304–305, 321–330, 336, 404, 413, 424, 426, 428, 433, 452–453, 477, 490, 502, 505, 513–514, 517–518, 520–521, 526–528, 530, 540–541, 563, 570, 579–580, 582–583, 585–587, 589, 591–592, 598–600, 603–604, 610–613, 619–629, 632, 651, 656, 701–702, 715, 741, 753, 758, 786, 790–791, 794, 812, 836, 848, 852, 866, 874–875, 879, 881–888
packages/orchestrations/agent_framework_orchestrations
   _concurrent.py1472483%59, 68–69, 97–98, 103, 121, 126, 131–132, 153, 163, 170, 244, 260, 263, 320, 350, 352–353, 355, 360, 373, 377
   _group_chat.py3327078%183, 352, 359, 392, 403–404, 410, 415, 436, 440, 453–454, 467, 482–483, 485, 501, 528–533, 535, 569–572, 574, 579–583, 677, 680, 719, 722, 725, 728, 736, 748–749, 751–752, 754–755, 757, 762, 765, 774, 780, 824–825, 829–830, 844–845, 847–848, 879–880, 946, 965, 973, 978–980, 987, 997
   _handoff.py3395284%112–113, 115, 168–178, 180, 182, 184, 189, 320, 345, 372, 398, 462, 505, 513, 517–518, 549–551, 556–558, 688, 691, 704, 766, 771, 778, 788, 790, 809, 811, 893–894, 926–927, 1039, 1046, 1118–1119, 1121
   _magentic.py5969184%73–82, 87, 91–102, 267, 278, 282, 302, 363, 372, 374, 416, 433, 442–443, 445–447, 449, 460, 603, 605, 645, 695, 731–733, 735, 745, 753–754, 821–824, 915, 921, 927, 969, 1007, 1039, 1056, 1067, 1124–1125, 1129–1131, 1155, 1179–1180, 1193, 1213, 1236, 1281–1282, 1320–1321, 1490, 1493, 1502, 1505, 1510, 1561–1562, 1603–1604, 1652, 1683, 1741, 1755, 1766
   _participant_output_config.py85692%35, 41, 53, 56, 112, 166
   _sequential.py83692%56, 132, 143, 149, 197, 225
TOTAL37783442088% 

Python Unit Test Overview

Tests Skipped Failures Errors Time
7512 34 💤 0 ❌ 0 🔥 2m 0s ⏱️

@moonbox3 moonbox3 enabled auto-merge June 2, 2026 09:17
Comment thread python/packages/orchestrations/agent_framework_orchestrations/_magentic.py Outdated
Copilot and others added 3 commits June 2, 2026 11:05
Replace the bare object() sentinel with typing_extensions.Sentinel per
PEP 661 (now final). Sentinel provides a proper name and repr
('<_MISSING>') and is the idiomatic approach going forward.

Refs microsoft#4306

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…osoft#6261)

Use int | Sentinel for max_stall_count parameter type annotation instead
of int with cast(Any, _MISSING) to properly express that the parameter
can hold either an int or the _MISSING sentinel value. This fixes the
pyright reportUnnecessaryComparison errors caused by the types int and
Sentinel having no overlap.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment thread python/packages/orchestrations/agent_framework_orchestrations/_magentic.py Outdated
The sentinel is user-visible as a default in public init signatures, so
use UNSET (no leading underscore) instead of the private _MISSING name.
Drop the now-unnecessary reportPrivateUsage ignores on the UNSET imports.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
enable_plan_review: bool = False,
checkpoint_storage: CheckpointStorage | None = None,
output_from: Sequence[_ParticipantOutputSpecifier] | Literal["all"] | None = cast(Any, _MISSING),
output_from: Sequence[_ParticipantOutputSpecifier] | Literal["all"] | None = cast(Any, UNSET),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we need this cast?

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Python: MagenticBuilder logs spurious custom-manager warning with defaults

4 participants