Python: fix LocalEvaluator reporting zero-check items as passed - #7399
Draft
CTWalk wants to merge 1 commit into
Draft
Python: fix LocalEvaluator reporting zero-check items as passed#7399CTWalk wants to merge 1 commit into
CTWalk wants to merge 1 commit into
Conversation
LocalEvaluator.evaluate initialized item_passed to True and only ever cleared it inside the loop over check results. With no checks configured the loop never runs, so an item with zero scores was recorded as passed: result_counts reported one pass, all_passed was True, and raise_for_status() did not raise. Initialize item_passed from bool(check_results) so an item with no evaluated checks fails closed. This matches the .NET contract in this repository, where AgentEvaluationResults.ItemPassed ends with 'return result.Metrics.Count > 0' and is pinned by LocalEvaluator_WithZeroChecks_ItemsHaveZeroMetricsAndFailAsync. Add a focused regression covering the counts, all_passed, the empty score list, and raise_for_status(). Fixes microsoft#7397
agent-framework-automation
Bot
added
the
python
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes Python LocalEvaluator behavior so an item evaluated with zero checks fails closed (rather than being reported as passed with no score evidence), aligning Python semantics with the existing .NET contract and preventing silent false passes in quality gates.
Changes:
- Initialize
item_passedfrombool(check_results)so zero-check evaluations are counted as failed. - Add a regression test asserting result counts,
all_passed, empty scores, andraise_for_status()behavior for the zero-check case.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| python/packages/core/agent_framework/_evaluation.py | Changes pass/fail aggregation to fail closed when no checks were evaluated. |
| python/packages/core/tests/core/test_local_eval.py | Adds a regression test covering zero-check LocalEvaluator behavior. |
Comment on lines
1575
to
1578
| for item_idx, item in enumerate(items): | ||
| check_results = await asyncio.gather(*[_run_check(fn, item) for fn in self._checks]) | ||
| item_passed = True | ||
| item_passed = bool(check_results) | ||
| item_scores: list[EvalScoreResult] = [] |
Author
|
@microsoft-github-policy-service agree |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation & Context
LocalEvaluator()with no checks reports every item as passed. The itemcarries zero
EvalScoreResultevidence, yet the run reports{"passed": 1, "failed": 0, "errored": 0},all_passedisTrue, andraise_for_status()does not raise.That combination is a silent false pass on exactly the surfaces a caller wires
into a quality gate. An empty check sequence is realistic when checks come from
configuration, optional plugins, or a filter that matches nothing.
.NET in this repository already treats this case as a failure:
AgentEvaluationResults.ItemPassedends withreturn result.Metrics.Count > 0;and
LocalEvaluator_WithZeroChecks_ItemsHaveZeroMetricsAndFailAsyncassertsPassed == 0, Failed == 1for an item with no metrics. This change bringsPython in line with that contract.
Description & Review Guide
What are the major changes?
Two files. In
LocalEvaluator.evaluate,item_passedis initialized frombool(check_results)instead of an unconditionalTrue, so an item with noevaluated checks fails closed instead of inheriting vacuous truth from a loop
that never runs. One regression test is added beside the existing local
evaluator tests.
No API, signature, dependency, serialization, or documentation change.
What is the impact of these changes?
A caller running
LocalEvaluator()with zero checks now getspassed=0, failed=1,all_passed is False, and a raisingraise_for_status(). Behavior for any evaluator with at least one check isunchanged, because the initializer is equivalent to
Truewhenevercheck_resultsis non-empty.This is an observable behavior change for the zero-check path. I have left
"not a breaking change" checked, on the grounds that the affected callers are
receiving a verdict with no evidence behind it and the evals surface is
marked
@experimental(feature_id=ExperimentalFeature.EVALS). If you wouldrather treat it as breaking, say so and I will add the label and title
prefix.
What do you want reviewers to focus on?
Whether fail-closed is the semantics you want on the Python side, or whether
you would prefer an explicit error at construction/evaluation time for an
evaluator with no checks. I chose to mirror the existing .NET guard rather
than introduce a new exception type, but the alternative is reasonable and
would be a larger change.
Opened as a draft so the direction can be settled against real code. Say the
word and I will mark it ready.
Related Issue
Fixes #7397
Small triage note: issue automation inferred
.NETfrom the parity contextand added it alongside
python; the label-prefix workflow then changed thetitle to
.NET: Python: [Bug]: .... I restored the Python-only title. This fixchanges only Python — .NET appears solely as the precedent being matched.
Could a maintainer remove the stale
.NETlabel when convenient?Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.