fix(evaluation): raise when evaluate_eval_set evaluates zero cases - #6952
Open
chelsealong wants to merge 1 commit into
Open
fix(evaluation): raise when evaluate_eval_set evaluates zero cases#6952chelsealong wants to merge 1 commit into
chelsealong wants to merge 1 commit into
Conversation
evaluate_eval_set builds a failures list only from eval_results_by_eval_id, then asserts not failures. If eval_results_by_eval_id is empty -- an EvalSet with no eval_cases, or num_runs=0 on a non-empty one -- the loop never runs, failures stays [], and the assert passes even though nothing was evaluated. Fixes google#6951
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.
Related
Fixes #6951
🔴 Required Information
Describe the Bug:
AgentEvaluator.evaluate_eval_setbuildseval_results_by_eval_idfrom theeval set, iterates it to populate a
failureslist, and finishes withassert not failures. Ifeval_results_by_eval_idends up empty, the loopbody never runs,
failuresstays[], and the assert passes — the functionreturns normally with no signal that zero cases were evaluated.
Two ordinary, non-exceptional calls reach this:
EvalSetwitheval_cases=[].evaluate_eval_set(..., num_runs=0)on a non-empty eval set — theinference requests list becomes
[], so nothing is inferred or evaluated.Fix:
After step 1 (building
eval_results_by_eval_id), raise aValueErrorif itis empty, before any pass/fail reporting happens. A "pass" from this function
should mean "every requested case was checked and none failed," not "nothing
was checked."
Testing Plan
Unit Tests:
test_evaluate_eval_set_raises_when_no_eval_cases_were_evaluated,which mocks
_get_eval_results_by_eval_idto return{}and assertsevaluate_eval_setraisesValueErrorinstead of returning silently.test_evaluate_eval_set_threads_artifact_service(which exercisedan empty
EvalSetand previously asserted a silent, no-op success) toassert the new
ValueErrorinstead._get_eval_results_by_eval_idto return{}purely as a "don't care" placeholder(
test_evaluate_eval_set_registers_custom_metrics,test_evaluate_eval_set_keeps_evaluators_from_the_default_registry,test_evaluate_eval_set_forwards_results_manager_and_app_name) to return aminimal passing
EvalCaseResultinstead, since they are not testing theempty-results case.
Verified the new/updated tests fail without the fix:
Full test run with the fix applied:
Lint / formatting:
AI Assistance Disclosure
This PR was written with the assistance of an AI coding agent (Claude). The
change was reviewed, tested, and verified by a human-supervised automated
workflow before submission.