From c2131b70e4dab0a18f529b8d4aa9f53ac0547bfa Mon Sep 17 00:00:00 2001 From: Gaurav Gandhi Date: Sat, 15 Aug 2026 01:04:15 +0530 Subject: [PATCH 1/2] fix(evaluation): honor each metric's own eval_status in AgentEvaluator.evaluate() _process_metrics_and_get_failures recomputed PASSED/FAILED itself via overall_score >= threshold, hardcoding a higher-is-better convention for every registered metric uniformly. This is backwards for any Evaluator that defines its metric as lower-is-better (a cost, latency, or error-rate metric, PASSED when score <= threshold): the metric's own correct eval_status was silently discarded and replaced with an inverted one, so AgentEvaluator.evaluate() misclassified a genuinely-passing run as failed for every real threshold value. Fix: aggregate from each invocation's own eval_status (already set correctly by the Evaluator and copied verbatim onto EvalMetricResult by LocalEvalService._evaluate_metric) instead of re-deriving a possibly- wrong one from the mean score. Mirrors LocalEvalService._generate_final_eval_status's existing aggregation convention (FAILED takes precedence, then PASSED if any passed, else NOT_EVALUATED), applied across a metric's own invocations instead of across an eval case's metrics. adk eval/LocalEvalService were never affected -- they already read eval_status directly. --- src/google/adk/evaluation/agent_evaluator.py | 48 ++++- .../evaluation/test_agent_evaluator.py | 179 ++++++++++++++++++ 2 files changed, 217 insertions(+), 10 deletions(-) diff --git a/src/google/adk/evaluation/agent_evaluator.py b/src/google/adk/evaluation/agent_evaluator.py index 40e1906f4d..0357e2fb7a 100644 --- a/src/google/adk/evaluation/agent_evaluator.py +++ b/src/google/adk/evaluation/agent_evaluator.py @@ -818,17 +818,45 @@ def _process_metrics_and_get_failures( for m in eval_metric_results_with_invocations if m.eval_metric_result.score is not None ] - - if scores: - overall_score = statistics.mean(scores) - overall_eval_status = ( - EvalStatus.PASSED - if overall_score >= threshold - else EvalStatus.FAILED + overall_score = statistics.mean(scores) if scores else None + + # Aggregate PASSED/FAILED/NOT_EVALUATED from each invocation's own + # `eval_status` -- set by the registered Evaluator itself (see + # LocalEvalService._evaluate_metric, which copies + # `PerInvocationResult.eval_status` verbatim onto each + # `EvalMetricResult`) -- rather than recomputing a fresh verdict here + # via `overall_score >= threshold`. That recomputation hardcoded a + # higher-is-better convention for every metric uniformly, which is + # backwards for any metric an Evaluator defines as lower-is-better + # (e.g. a cost or latency metric, PASSED when `score <= threshold`): + # such a metric's own correct eval_status was silently discarded and + # replaced with an inverted one, misclassifying a genuinely-passing + # run as failed (and vice versa) for every real threshold value. + # `Evaluator`/`EvalStatus` have no separate "polarity" concept + # anywhere else in this module either -- eval_status is already the + # one place a metric's own pass/fail semantics are recorded, so this + # reads that instead of re-deriving a possibly-wrong one. Mirrors + # `LocalEvalService._generate_final_eval_status`'s existing + # aggregation convention (FAILED takes precedence over everything; + # otherwise PASSED if any result passed; else NOT_EVALUATED) -- + # applied here across an eval metric's own invocations instead of + # across an eval case's metrics, but the same three-way logic. + overall_eval_status = EvalStatus.NOT_EVALUATED + for ( + eval_metric_result_with_invocation + ) in eval_metric_results_with_invocations: + invocation_status = ( + eval_metric_result_with_invocation.eval_metric_result.eval_status ) - else: - overall_score = None - overall_eval_status = EvalStatus.NOT_EVALUATED + if invocation_status == EvalStatus.FAILED: + overall_eval_status = EvalStatus.FAILED + break + elif invocation_status == EvalStatus.PASSED: + overall_eval_status = EvalStatus.PASSED + elif invocation_status == EvalStatus.NOT_EVALUATED: + continue + else: + raise ValueError(f"Unknown eval status: {invocation_status}.") # Gather all the failures. if overall_eval_status != EvalStatus.PASSED: diff --git a/tests/unittests/evaluation/test_agent_evaluator.py b/tests/unittests/evaluation/test_agent_evaluator.py index b5f4dfec0d..268875428e 100644 --- a/tests/unittests/evaluation/test_agent_evaluator.py +++ b/tests/unittests/evaluation/test_agent_evaluator.py @@ -508,6 +508,185 @@ def test_get_results_as_rows_handles_missing_expected_invocation(): assert rows[0]["actual_response"] == "hello" +class TestProcessMetricsAndGetFailures: + """_process_metrics_and_get_failures must honor each invocation's own + eval_status (set by the registered Evaluator) rather than recomputing a + fresh verdict from `overall_score >= threshold`, which hardcodes a + higher-is-better convention that is backwards for any metric an Evaluator + defines as lower-is-better (a cost, latency, or error-rate metric -- + PASSED when `score <= threshold`).""" + + def test_lower_is_better_metric_genuinely_passing_is_not_reported_as_failure( + self, + ): + """The directionality bug's core repro: a lower-is-better metric whose + own eval_status is PASSED (score below threshold, by that metric's own + correct accounting) used to be recomputed via `10.0 >= 100.0` (False) + and reported as a failure. It no longer is.""" + eval_metric_results = { + "latency_ms": [ + _make_result_with_invocation( + metric_name="latency_ms", + score=10.0, + threshold=100.0, + eval_status=EvalStatus.PASSED, + prompt="hi", + expected_response="", + actual_response="ok", + ), + ], + } + + failures = AgentEvaluator._process_metrics_and_get_failures( + eval_metric_results=eval_metric_results, + print_detailed_results=False, + agent_module="my_agent", + ) + + assert failures == [] + + def test_lower_is_better_metric_genuinely_failing_is_still_reported(self): + """The same lower-is-better metric, but genuinely over threshold (its own + eval_status is FAILED) -- must still be reported. This is the case a + permissive "just always trust the metric" non-fix would have broken.""" + eval_metric_results = { + "latency_ms": [ + _make_result_with_invocation( + metric_name="latency_ms", + score=500.0, + threshold=100.0, + eval_status=EvalStatus.FAILED, + prompt="hi", + expected_response="", + actual_response="ok", + ), + ], + } + + failures = AgentEvaluator._process_metrics_and_get_failures( + eval_metric_results=eval_metric_results, + print_detailed_results=False, + agent_module="my_agent", + ) + + assert len(failures) == 1 + assert "latency_ms for my_agent Failed" in failures[0] + + def test_higher_is_better_metric_still_works_as_before(self): + """Regression safety net: an ordinary higher-is-better metric (the + existing, common case -- PASSED when score >= threshold) must keep + working exactly as it did before this fix, since it was never the + metric shape this bug affected.""" + eval_metric_results = { + "response_match_score": [ + _make_result_with_invocation( + metric_name="response_match_score", + score=1.0, + threshold=0.8, + eval_status=EvalStatus.PASSED, + prompt="What is 2 + 2?", + expected_response="4", + actual_response="4", + ), + ], + } + + failures = AgentEvaluator._process_metrics_and_get_failures( + eval_metric_results=eval_metric_results, + print_detailed_results=False, + agent_module="my_agent", + ) + + assert failures == [] + + def test_higher_is_better_metric_failure_still_reported(self): + eval_metric_results = { + "response_match_score": [ + _make_result_with_invocation( + metric_name="response_match_score", + score=0.0, + threshold=0.8, + eval_status=EvalStatus.FAILED, + prompt="Capital of France?", + expected_response="Paris", + actual_response="London", + ), + ], + } + + failures = AgentEvaluator._process_metrics_and_get_failures( + eval_metric_results=eval_metric_results, + print_detailed_results=False, + agent_module="my_agent", + ) + + assert len(failures) == 1 + assert "response_match_score for my_agent Failed" in failures[0] + + def test_failed_takes_precedence_over_passed_across_invocations(self): + """Mirrors LocalEvalService._generate_final_eval_status's own + aggregation convention: one FAILED invocation fails the whole metric + even if another invocation of the same metric passed.""" + eval_metric_results = { + "latency_ms": [ + _make_result_with_invocation( + metric_name="latency_ms", + score=10.0, + threshold=100.0, + eval_status=EvalStatus.PASSED, + prompt="q1", + expected_response="", + actual_response="ok", + ), + _make_result_with_invocation( + metric_name="latency_ms", + score=500.0, + threshold=100.0, + eval_status=EvalStatus.FAILED, + prompt="q2", + expected_response="", + actual_response="ok", + ), + ], + } + + failures = AgentEvaluator._process_metrics_and_get_failures( + eval_metric_results=eval_metric_results, + print_detailed_results=False, + agent_module="my_agent", + ) + + assert len(failures) == 1 + + def test_not_evaluated_with_no_scores_is_still_reported_as_failure(self): + """Unchanged from before this fix -- a metric that never produced a + real score (NOT_EVALUATED, no scores at all) is still treated as a + failure by this function, exactly as it was pre-fix. This function does + not change that; only the PASSED-vs-FAILED polarity for a metric that + genuinely did produce a score.""" + eval_metric_results = { + "latency_ms": [ + _make_result_with_invocation( + metric_name="latency_ms", + score=None, + threshold=100.0, + eval_status=EvalStatus.NOT_EVALUATED, + prompt="hi", + expected_response="", + actual_response="ok", + ), + ], + } + + failures = AgentEvaluator._process_metrics_and_get_failures( + eval_metric_results=eval_metric_results, + print_detailed_results=False, + agent_module="my_agent", + ) + + assert len(failures) == 1 + + def test_write_results_to_csv_writes_expected_file(tmp_path): rows = [ { From 9842b84e0b334d26fa769122efc70338e35d49df Mon Sep 17 00:00:00 2001 From: Gaurav Gandhi Date: Mon, 17 Aug 2026 02:02:58 +0530 Subject: [PATCH 2/2] fix(evaluation): preserve mean-vs-threshold aggregation, fix only polarity Revises #6739 per review from varunbiluri: the original diff replaced mean-vs-threshold aggregation with an any-invocation-fails rule, a backwards-incompatible behavior change for multi-invocation evals independent of polarity. This keeps overall_score = mean(scores) and the mean-vs-threshold comparison exactly as before, correcting only which comparison operator applies -- inferred from one invocation's own (score, eval_status) pair, since Evaluator/EvalMetric carry no explicit polarity field anywhere in this module. Adds the reviewer's own counter-example as a test (scores [0.0, 1.0], threshold 0.5, higher-is-better -- mean clears the threshold, must pass), its lower-is-better mirror, and the corresponding mean-genuinely-fails cases for both. Replaces the prior mixed-invocation test, whose name asserted an any-invocation-fails rule that is no longer the actual contract (it happened to still pass under the reverted behavior for an unrelated reason: its failing score was extreme enough to also fail the mean). --- src/google/adk/evaluation/agent_evaluator.py | 71 +++++---- .../evaluation/test_agent_evaluator.py | 148 ++++++++++++++++-- 2 files changed, 178 insertions(+), 41 deletions(-) diff --git a/src/google/adk/evaluation/agent_evaluator.py b/src/google/adk/evaluation/agent_evaluator.py index 0357e2fb7a..37ea30c272 100644 --- a/src/google/adk/evaluation/agent_evaluator.py +++ b/src/google/adk/evaluation/agent_evaluator.py @@ -820,43 +820,52 @@ def _process_metrics_and_get_failures( ] overall_score = statistics.mean(scores) if scores else None - # Aggregate PASSED/FAILED/NOT_EVALUATED from each invocation's own - # `eval_status` -- set by the registered Evaluator itself (see + # Determine this metric's pass/fail POLARITY (higher-is-better vs. + # lower-is-better) from one invocation's own (score, eval_status) + # pair -- set correctly by the registered Evaluator itself (see # LocalEvalService._evaluate_metric, which copies # `PerInvocationResult.eval_status` verbatim onto each - # `EvalMetricResult`) -- rather than recomputing a fresh verdict here - # via `overall_score >= threshold`. That recomputation hardcoded a - # higher-is-better convention for every metric uniformly, which is - # backwards for any metric an Evaluator defines as lower-is-better - # (e.g. a cost or latency metric, PASSED when `score <= threshold`): - # such a metric's own correct eval_status was silently discarded and - # replaced with an inverted one, misclassifying a genuinely-passing - # run as failed (and vice versa) for every real threshold value. - # `Evaluator`/`EvalStatus` have no separate "polarity" concept - # anywhere else in this module either -- eval_status is already the - # one place a metric's own pass/fail semantics are recorded, so this - # reads that instead of re-deriving a possibly-wrong one. Mirrors - # `LocalEvalService._generate_final_eval_status`'s existing - # aggregation convention (FAILED takes precedence over everything; - # otherwise PASSED if any result passed; else NOT_EVALUATED) -- - # applied here across an eval metric's own invocations instead of - # across an eval case's metrics, but the same three-way logic. - overall_eval_status = EvalStatus.NOT_EVALUATED + # `EvalMetricResult`) -- rather than hardcoding a higher-is-better + # `>=` comparison for every metric uniformly, which is backwards for + # any metric an Evaluator defines as lower-is-better (e.g. a cost or + # latency metric, PASSED when `score <= threshold`). + # `Evaluator`/`EvalMetric`/`BaseCriterion` have no separate + # "polarity" field anywhere in this module, so this recovers it from + # the one place a metric's own correct pass/fail semantics are + # already recorded per invocation, then applies that SAME direction + # to the aggregate mean -- preserving the original mean-vs-threshold + # aggregation exactly, only correcting which comparison operator it + # uses. A sample exactly AT the threshold is skipped when inferring + # direction (both directions agree there, so it's uninformative); + # if every invocation sits exactly at the threshold, the mean does + # too, and the arbitrary default below still yields the correct + # verdict either way. + higher_is_better = True for ( eval_metric_result_with_invocation ) in eval_metric_results_with_invocations: - invocation_status = ( - eval_metric_result_with_invocation.eval_metric_result.eval_status - ) - if invocation_status == EvalStatus.FAILED: - overall_eval_status = EvalStatus.FAILED - break - elif invocation_status == EvalStatus.PASSED: - overall_eval_status = EvalStatus.PASSED - elif invocation_status == EvalStatus.NOT_EVALUATED: + result = eval_metric_result_with_invocation.eval_metric_result + if ( + result.score is None + or result.score == threshold + or result.eval_status not in (EvalStatus.PASSED, EvalStatus.FAILED) + ): continue - else: - raise ValueError(f"Unknown eval status: {invocation_status}.") + higher_is_better = (result.score > threshold) == ( + result.eval_status == EvalStatus.PASSED + ) + break + + if overall_score is None: + overall_eval_status = EvalStatus.NOT_EVALUATED + elif ( + overall_score >= threshold + if higher_is_better + else overall_score <= threshold + ): + overall_eval_status = EvalStatus.PASSED + else: + overall_eval_status = EvalStatus.FAILED # Gather all the failures. if overall_eval_status != EvalStatus.PASSED: diff --git a/tests/unittests/evaluation/test_agent_evaluator.py b/tests/unittests/evaluation/test_agent_evaluator.py index 268875428e..5f71e6d7dd 100644 --- a/tests/unittests/evaluation/test_agent_evaluator.py +++ b/tests/unittests/evaluation/test_agent_evaluator.py @@ -509,12 +509,18 @@ def test_get_results_as_rows_handles_missing_expected_invocation(): class TestProcessMetricsAndGetFailures: - """_process_metrics_and_get_failures must honor each invocation's own - eval_status (set by the registered Evaluator) rather than recomputing a - fresh verdict from `overall_score >= threshold`, which hardcodes a - higher-is-better convention that is backwards for any metric an Evaluator - defines as lower-is-better (a cost, latency, or error-rate metric -- - PASSED when `score <= threshold`).""" + """_process_metrics_and_get_failures aggregates over the MEAN of an eval + metric's per-invocation scores compared to its threshold -- unchanged + from before this fix -- but must apply the CORRECT comparison direction + for that metric's polarity, inferred from each invocation's own + eval_status (set by the registered Evaluator). The original bug hardcoded + a higher-is-better `overall_score >= threshold` comparison for every + metric uniformly, which is backwards for any metric an Evaluator defines + as lower-is-better (a cost, latency, or error-rate metric -- PASSED when + `score <= threshold`). This fix corrects ONLY the comparison direction; + it does not change mean-vs-threshold aggregation into an + any-invocation-fails rule (see the mixed-invocation tests below, which + exist specifically to prove the mean-based contract survives).""" def test_lower_is_better_metric_genuinely_passing_is_not_reported_as_failure( self, @@ -623,10 +629,132 @@ def test_higher_is_better_metric_failure_still_reported(self): assert len(failures) == 1 assert "response_match_score for my_agent Failed" in failures[0] - def test_failed_takes_precedence_over_passed_across_invocations(self): - """Mirrors LocalEvalService._generate_final_eval_status's own - aggregation convention: one FAILED invocation fails the whole metric - even if another invocation of the same metric passed.""" + def test_mixed_invocations_where_the_mean_still_clears_the_threshold(self): + """Aggregation is over the MEAN of scores compared to the threshold -- + NOT "any individual invocation failed" -- exactly the pre-existing + contract this fix must preserve, and exactly the gap a prior version of + this test suite failed to cover (it asserted "any-invocation-fails" + behavior instead of proving mean-aggregation survives). Two invocations + of a higher-is-better metric at threshold=0.5: scores 0.0 (individually + FAILED) and 1.0 (individually PASSED). The MEAN (0.5) clears the + threshold, so the metric as a whole must PASS -- an any-invocation- + fails rule would incorrectly report this as a failure since one + invocation individually failed.""" + eval_metric_results = { + "response_match_score": [ + _make_result_with_invocation( + metric_name="response_match_score", + score=0.0, + threshold=0.5, + eval_status=EvalStatus.FAILED, + prompt="q1", + expected_response="", + actual_response="wrong", + ), + _make_result_with_invocation( + metric_name="response_match_score", + score=1.0, + threshold=0.5, + eval_status=EvalStatus.PASSED, + prompt="q2", + expected_response="", + actual_response="right", + ), + ], + } + + failures = AgentEvaluator._process_metrics_and_get_failures( + eval_metric_results=eval_metric_results, + print_detailed_results=False, + agent_module="my_agent", + ) + + assert failures == [] + + def test_mixed_invocations_where_the_mean_genuinely_fails_the_threshold( + self, + ): + """The counterpart to the above: when the mean genuinely does NOT clear + the threshold, the metric must still be reported as a failure -- a + permissive "just always pass on any individual success" non-fix would + have broken this. Higher-is-better metric, threshold=0.5: scores 0.0 + and 0.4 -- mean is 0.2, below threshold, so this must fail even though + neither score individually triggers a different rule.""" + eval_metric_results = { + "response_match_score": [ + _make_result_with_invocation( + metric_name="response_match_score", + score=0.0, + threshold=0.5, + eval_status=EvalStatus.FAILED, + prompt="q1", + expected_response="", + actual_response="wrong", + ), + _make_result_with_invocation( + metric_name="response_match_score", + score=0.4, + threshold=0.5, + eval_status=EvalStatus.FAILED, + prompt="q2", + expected_response="", + actual_response="close", + ), + ], + } + + failures = AgentEvaluator._process_metrics_and_get_failures( + eval_metric_results=eval_metric_results, + print_detailed_results=False, + agent_module="my_agent", + ) + + assert len(failures) == 1 + + def test_mixed_invocations_lower_is_better_mean_still_clears_threshold( + self, + ): + """Same mean-vs-threshold contract, for a lower-is-better metric + (the actual shape this whole fix exists for): threshold=100.0, one + invocation well under budget (score=10, individually PASSED) and one + modestly over (score=150, individually FAILED). Mean is 80.0, which + clears (is under) the threshold, so the metric as a whole must PASS.""" + eval_metric_results = { + "latency_ms": [ + _make_result_with_invocation( + metric_name="latency_ms", + score=10.0, + threshold=100.0, + eval_status=EvalStatus.PASSED, + prompt="q1", + expected_response="", + actual_response="ok", + ), + _make_result_with_invocation( + metric_name="latency_ms", + score=150.0, + threshold=100.0, + eval_status=EvalStatus.FAILED, + prompt="q2", + expected_response="", + actual_response="ok", + ), + ], + } + + failures = AgentEvaluator._process_metrics_and_get_failures( + eval_metric_results=eval_metric_results, + print_detailed_results=False, + agent_module="my_agent", + ) + + assert failures == [] + + def test_mixed_invocations_lower_is_better_mean_genuinely_fails(self): + """Counterpart for a lower-is-better metric: one invocation cheap + (score=10, PASSED) and one wildly over budget (score=500, FAILED). + Mean is 255.0, well over the threshold=100.0, so this must still be + reported as a failure.""" eval_metric_results = { "latency_ms": [ _make_result_with_invocation(