feat(agents): add max_stalls parameter to optimize config and API - #9314
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). 6 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Adds max_stalls configuration support for optimizer early stopping.
Changes:
- Adds YAML and API model fields.
- Maps the field into optimization requests.
- Adds YAML parsing and request-conversion tests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
optimize_api/models.go |
Adds the API request field. |
opt_eval/yaml.go |
Adds YAML configuration support. |
opt_eval/yaml_test.go |
Tests YAML parsing. |
optimize_config.go |
Maps configuration to API requests. |
optimize_config_test.go |
Tests request conversion. |
c91b5ec to
7dea97c
Compare
glharper
left a comment
There was a problem hiding this comment.
This looks good overall - just a couple of things to consider.
The change is cleanly scoped to #9316, purely additive, and backward compatible. I confirmed optimize_config.go is the only production site constructing optimize_api.OptimizeOptions, so nothing else needed wiring.
7dea97c to
e838d11
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
cli/azd/extensions/azure.ai.agents/internal/cmd/optimize.go:352
- azd-code-reviewer:
--max-stalls=0(or a negative value) is silently treated as omitted because only positive values reach the config. This bypasses the new validation and can even leave a config-file value in place despite the explicit CLI override, although the help text requires a value of at least 1. Reject an explicitly set invalid flag before running the action, and add coverage for this CLI path.
if a.flags.maxStalls > 0 {
cfg.Options.MaxStalls = &a.flags.maxStalls
}
jongio
left a comment
There was a problem hiding this comment.
--max-stalls 0 is silently ignored while max_stalls: 0 from YAML is now rejected. An explicit --max-stalls 0 also drops a config-file value instead of overriding it. Details inline, not blocking.
e838d11 to
3aa2920
Compare
3aa2920 to
d648c25
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
cli/azd/extensions/azure.ai.agents/internal/cmd/optimize_config_test.go:385
- azd-code-reviewer: This temporary exists only so its address can be taken, but this Go 1.26 codebase requires
new(value)for that pattern (cli/azd/AGENTS.md:308-313). Usenew(-1)directly so the modernization check passes.
MaxStalls: &neg,
cli/azd/extensions/azure.ai.agents/internal/cmd/optimize.go:354
- azd-code-reviewer: The new CLI override branch is not exercised. The added validation tests construct
Optionsdirectly, so they still pass if this assignment stops overriding YAML or an omitted flag accidentally clears it. AddapplyOverridescases for explicitly set and unsetmaxStallsSetvalues.
if a.flags.maxStallsSet {
cfg.Options.MaxStalls = &a.flags.maxStalls
}
cli/azd/extensions/azure.ai.agents/internal/cmd/optimize.go:172
- azd-code-reviewer: This added line exceeds the repository's 125-character Go limit (
cli/azd/AGENTS.md:97-108), so thelllcheck will fail. Wrap theIntVarcall.
cmd.Flags().IntVar(&flags.maxStalls, "max-stalls", 0, "Maximum consecutive non-improving iterations before early stopping (must be >= 1; default: 5)")
jongio
left a comment
There was a problem hiding this comment.
Previous comments addressed. --max-stalls 0 and negatives now route through the single Validate() call, and max_candidates staying lax is a separate discussion per your reply, so not blocking on it.
Three nits, none blocking:
- optimize.go:172 - help text says
default: 5, but nothing in the repo applies that default formax_stalls - optimize_config.go:103 - error message names only the YAML key, though
--max-stallsreaches it now - optimize_config_test.go:370 - comment claims CLI-path coverage the test doesn't have
🤖 agent jongio
Head branch was pushed to by a user without write access
d648c25 to
d91f35e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 75 out of 78 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- cli/azd/test/evals/package-lock.json: Generated file
Comments suppressed due to low confidence (3)
eng/pipelines/eval-e2e.yml:71
- azd-code-reviewer: Filtering to
tasks/lifecycle/does not make this an E2E run: both lifecycle tasks only ask for walkthrough text and usetext,action_sequence, andbehaviorgraders. No task executes the suggested commands or references any of the newcodegraders, so this job cannot provision resources or validate ARM/HTTP state despite the pipeline and README claiming that it does. Add executable lifecycle tasks plus the infrastructure, health, and cleanup graders before scheduling this as E2E.
eng/pipelines/eval-waza.yml:1 - azd-code-reviewer: This pipeline is outside the stated
max_stallschange, as are the new eval framework, registry-policy rollback, and protocol-version change. The PR description and linked issue only cover the threemax_stallswiring changes, so this scope cannot be reviewed or released as described. Split the unrelated work into dedicated PRs (or fully retitle and document the intended scope).
eng/pipelines/eval-report.yml:71 - azd-code-reviewer: This scheduled “report” pipeline never generates a report; it only downloads and republishes artifacts. The referenced
scripts/generate-report.tsdoes not exist, so no weekly summary orregression-issues.jsoncan be produced. Implement and invoke the generator before publishing, or remove the placeholder pipeline until it is functional.
…I flag)
Exposes the max_stalls early-stopping option via eval-config YAML and the
optimize API. This parameter is intentionally not surfaced as a CLI flag
because it is an advanced tuning knob best suited for config-file workflows.
Changes:
- opt_eval.Options: add MaxStalls *int (yaml:max_stalls,omitempty)
- optimize_api.OptimizeOptions: add MaxStalls *int (json:max_stalls,omitempty)
- OptimizeConfig.ToRequest(): wire MaxStalls from YAML config to API request
- Validate(): reject MaxStalls < 1 with a clear error message
Usage in eval-config.yaml:
options:
max_stalls: 10 # stop early after 10 consecutive non-improving iterations
Omitting max_stalls uses the service default (5).
Closes Azure#9316
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
d91f35e to
d5a3248
Compare
jongio
left a comment
There was a problem hiding this comment.
The --max-stalls flag removal resolves all four of my earlier findings: the sentinel bypass of Validate(), the help text default azd never applied, the error message naming only the YAML key, and the test comment claiming CLI-path coverage. YAML is the only input path now, so max_stalls: 0 and negatives both route through the single Validate() call at optimize_config.go:101.
Verified against d5a3248: no leftover maxStalls references in optimize.go, and the extension test suite passes locally.
max_candidates staying lax is unchanged and still separate from this PR.
🤖 agent jongio
jongio
left a comment
There was a problem hiding this comment.
The description still says omitting max_stalls uses "the service default (5)". That 5 comes from defaultOptimizeConfig() at optimize_config.go:118 and applies to max_candidates, not to the service, so it doesn't carry over. Nothing sets MaxStalls, so the service picks the value and we don't actually know it's 5.
The doc comments at yaml.go:485 and models.go:117 get this right by not naming a number. Worth dropping the "(5)" from the description unless you've confirmed it against the FAOS default.
|
@jongio yes the service default both for max-stalls and max-candidates is currently 5, if it will change you are right I will need to update the YAML again |
Closes #9316
Summary
Exposes the
max_stallsearly-stopping option in the YAML eval-config and the optimize API. When N consecutive full validation-set evaluations produce no improvement, the optimizer stops early to save cost.The
max_stallsparameter was added to the FAOS service in Vienna PR #2189426. This PR wires it through the azd CLI extension (YAML → API only).Changes
opt_eval.Options: addMaxStalls *intwithyaml:"max_stalls,omitempty"optimize_api.OptimizeOptions: addMaxStalls *intwithjson:"max_stalls,omitempty"OptimizeConfig.ToRequest(): wireMaxStallsfrom YAML config to API requestValidate(): rejectmax_stalls < 1with a clear error messageUsage (YAML config file only)
Omitting
max_stallsuses the service default (5).Testing
TestOptions_MaxStalls— verifies YAML keymax_stallspopulatesMaxStallsTestToRequest_MaxStalls— verifiesMaxStallsis passed throughToRequest()to the APITestOptimizeRequest_RoundTrip— verifies JSON tagmax_stallsround-trips correctlyTestValidate_MaxStallsZeroIsRejected— verifies YAMLmax_stalls: 0errorsTestValidate_MaxStallsNegativeIsRejected— verifies YAMLmax_stalls: -1errorsTestValidate_MaxStallsPositiveIsAccepted— verifies valid values pass