fix(evaluation): NOT_EVALUATED metric no longer masked by a passing one - #6682
Open
gaurav-gandhi-2411 wants to merge 2 commits into
Open
Conversation
_generate_final_eval_status looped over an eval case's per-metric results and set the final status to PASSED whenever a PASSED result was seen, using a bare `continue` on NOT_EVALUATED that left an already-set PASSED untouched. A metric that crashed mid-evaluation (caught in _evaluate_metric_for_eval_case and recorded as NOT_EVALUATED) therefore had no effect on the final verdict as long as some other metric in the same eval case passed: [NOT_EVALUATED, PASSED] and [PASSED, NOT_EVALUATED] both reported PASSED, silently dropping the fact that one of the requested metrics never actually produced a verdict. This is order-dependent in a way that has no principled justification -- the two orderings represent the same set of per-metric outcomes and must produce the same final status. FAILED already dominated NOT_EVALUATED correctly in both orderings (it breaks the loop immediately), which is what confirms this was specifically a PASSED-vs-NOT_EVALUATED bug rather than intended behavior. Fix uses the existing EvalStatus values only: if any metric was NOT_EVALUATED and no metric FAILED, the final status is NOT_EVALUATED rather than PASSED. A genuine FAILED still takes precedence over NOT_EVALUATED, since it's real evidence rather than a missing verdict. Added test_generate_final_eval_status_not_evaluated_then_passed_is_not_evaluated and the reverse-order counterpart to make the order-dependence visible, plus a same-precedence test confirming FAILED still wins over NOT_EVALUATED regardless of order. All four generate_final_eval_status tests, including the pre-existing doesn_t_throw_on one, and the full evaluation/ suite (786 tests) pass after the fix.
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
4 participants
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.
🔴 Required Information
Describe the Bug:
LocalEvalService._generate_final_eval_statuscomputes an eval case's overall status from its per-metric results. It setsPASSEDwhenever aPASSEDresult is seen, and uses a barecontinueonNOT_EVALUATEDthat leaves an already-setPASSEDuntouched._evaluate_metric_for_eval_casecatches any exception during a single metric's evaluation (a judge-model API failure, a rate limit, etc.) and records that metric asNOT_EVALUATEDrather than letting the exception propagate — by design, so one metric's failure doesn't take down the others. But nothing downstream then treats thatNOT_EVALUATEDas reducing confidence in the final verdict.Steps to Reproduce:
Run an eval case with two requested metrics where one crashes (→
NOT_EVALUATED) and the other passes.Expected Behavior: An eval case where a requested metric never produced a verdict should not be reported as a clean
PASSED.Observed Behavior:
[NOT_EVALUATED, PASSED]and[PASSED, NOT_EVALUATED](same set of outcomes, opposite order) both currently returnPASSED— the crashed metric is silently absorbed.FAILEDwas already handled correctly in both orderings (itbreaks the loop immediately), which is what pins this down as specifically aPASSED-vs-NOT_EVALUATEDbug rather than intended behavior: there's no principled reason two orderings of the same outcomes should disagree.Why this fix
Track whether any metric was
NOT_EVALUATED; if so, and the loop would otherwise have concludedPASSED, reportNOT_EVALUATEDinstead.FAILEDstill takes precedence overNOT_EVALUATEDin all orderings, since a genuine failure is real evidence, not a missing verdict. This uses only the three existingEvalStatusvalues (PASSED/FAILED/NOT_EVALUATED) — no new status is introduced.Testing Plan
Added two tests exercising both orderings of
[NOT_EVALUATED, PASSED], named to make the order-dependence explicit, plus a same-shape test confirmingFAILEDstill dominatesNOT_EVALUATEDregardless of order. Confirmed the two new PASSED/NOT_EVALUATED-ordering tests fail onmainwith the exact bug described above, and pass after the fix.