diff --git a/docs/evals.md b/docs/evals.md index 77d3584..6a68368 100644 --- a/docs/evals.md +++ b/docs/evals.md @@ -147,6 +147,11 @@ protest eval evals.session:session --no-tag slow An evaluator is a function decorated with `@evaluator` that receives an `EvalContext` and returns a verdict. The decorator is mandatory: passing a plain function in `evaluators=[...]` raises `TypeError` at registration. The wrapping is what gives the evaluator its identity (used for hashing, history, reporting) and a typed `run(ctx)` method - there's no implicit conversion. +Every eval case must have **at least one evaluator** at runtime - suite-level +(`@suite.eval(evaluators=[...])`) or per-case (`EvalCase(evaluators=[...])`). +Zero evaluators raises `NoEvaluatorsError`: `passed` is `all(verdicts)` and +`all([])` is `True`, so a mis-wired eval would otherwise pass silently forever. + !!! info "If your eval task returns a non-string output" The built-in evaluators (`contains_keywords`, `not_empty`, `max_length`, diff --git a/protest/evals/evaluator.py b/protest/evals/evaluator.py index 141720c..7640895 100644 --- a/protest/evals/evaluator.py +++ b/protest/evals/evaluator.py @@ -186,6 +186,12 @@ class ShortCircuit: """ def __init__(self, evaluators: list[Evaluator]) -> None: + if not evaluators: + raise ValueError( + "ShortCircuit requires at least one evaluator. An empty " + "group contributes zero scores, feeding the always-green " + "trap (all([]) is True)." + ) validate_evaluators(evaluators, _inside_short_circuit=True) self.evaluators = evaluators diff --git a/protest/evals/wrapper.py b/protest/evals/wrapper.py index 33d0c15..93b27cb 100644 --- a/protest/evals/wrapper.py +++ b/protest/evals/wrapper.py @@ -27,6 +27,7 @@ from protest.exceptions import ( FixtureError, MultipleEvalCaseParamsError, + NoEvaluatorsError, ScoreNameCollisionError, ) @@ -48,6 +49,19 @@ async def eval_wrapper(**kwargs: Any) -> EvalPayload: inputs = _extract_inputs(kwargs) metadata = _extract_metadata(kwargs) + all_evaluators = list(evaluators) + all_evaluators.extend(_extract_per_case_evaluators(kwargs)) + + # Both guards run before the task itself: the evaluator list is + # fully known from the kwargs alone, and the task is typically the + # expensive LLM call. Zero evaluators would silently pass + # (all([]) is True), and duplicate names guarantee colliding score + # keys - fail before spending any tokens on a doomed case. + flat_evaluators = _flatten_evaluators(all_evaluators) + if not flat_evaluators: + raise NoEvaluatorsError(case_name) + _check_duplicate_evaluator_names(flat_evaluators, case_name) + start = time.perf_counter() if asyncio.iscoroutinefunction(func): raw_output = await func(**kwargs) @@ -67,14 +81,6 @@ async def eval_wrapper(**kwargs: Any) -> EvalPayload: else: output = raw_output - all_evaluators = list(evaluators) - per_case = _extract_per_case_evaluators(kwargs) - all_evaluators.extend(per_case) - - # Duplicate evaluator names are knowable before running anything - - # fail here rather than after burning judge tokens on a doomed case. - _check_duplicate_evaluator_names(all_evaluators, case_name) - scores, eval_ctx = await run_evaluators( all_evaluators, case_name, @@ -167,18 +173,18 @@ def _validate_single_evalcase_param(func: Any) -> None: def _check_duplicate_evaluator_names( - evaluators: list[Evaluator | ShortCircuit], case_name: str + evaluators: list[Evaluator], case_name: str ) -> None: """Raise ScoreNameCollisionError if an evaluator name appears twice. Score names are namespaced per evaluator, so a duplicate evaluator name (the same @evaluator attached twice, possibly rebound with different - kwargs) guarantees colliding score keys. Runs before any evaluator - - including judge calls - executes. + kwargs) guarantees colliding score keys. Takes the flattened list; + runs before the task and any evaluator executes. """ seen: set[str] = set() duplicates: list[str] = [] - for ev in _flatten_evaluators(evaluators): + for ev in evaluators: if ev.name in seen and ev.name not in duplicates: duplicates.append(ev.name) seen.add(ev.name) diff --git a/protest/exceptions.py b/protest/exceptions.py index c4e54cb..e4df84f 100644 --- a/protest/exceptions.py +++ b/protest/exceptions.py @@ -115,6 +115,26 @@ def __init__(self, func_name: str, param_names: list[str]): ) +class NoEvaluatorsError(ProTestError): + """Raised when an eval case ends up with zero evaluators at runtime. + + `passed` is computed as `all(s.passed for s in scores)` and `all([])` + is `True` - an eval with no evaluators would silently pass no matter + what the task returned, making a wiring mistake (forgotten + `evaluators=`, per-case evaluators not attached) indistinguishable + from a healthy eval. The guard runs at execution time because + per-case evaluators are only known then. + """ + + def __init__(self, case_name: str): + super().__init__( + f"Eval '{case_name}' has no evaluators. An eval with zero " + f"evaluators would always pass (all([]) is True), hiding wiring " + f"mistakes. Pass evaluators= to @suite.eval(...) or attach " + f"per-case evaluators via EvalCase(evaluators=[...])." + ) + + class ScoreNameCollisionError(ProTestError): """Raised when two evaluators in the same eval emit scores with the same name. diff --git a/tests/evals/test_no_evaluators.py b/tests/evals/test_no_evaluators.py new file mode 100644 index 0000000..6ab1327 --- /dev/null +++ b/tests/evals/test_no_evaluators.py @@ -0,0 +1,100 @@ +"""Tests for `NoEvaluatorsError` - fail loud on the zero-evaluators trap. + +`passed` is `all(s.passed for s in scores)` and `all([])` is `True`: an +eval that ends up with no evaluators at runtime would always pass, +hiding wiring mistakes. The guard runs at execution time because +per-case evaluators are only known then. +""" + +from __future__ import annotations + +import asyncio +from typing import Annotated + +import pytest + +from protest import ForEach, From, ProTestSession +from protest.evals import ( + EvalCase, + EvalContext, + EvalSuite, + ShortCircuit, + evaluator, +) +from protest.evals.wrapper import make_eval_wrapper +from protest.exceptions import NoEvaluatorsError + + +@evaluator +def _ok(ctx: EvalContext) -> bool: + return True + + +async def _invoke(evaluators: list, case: EvalCase): + def task(case: EvalCase) -> str: + return str(case.inputs) + + wrapped = make_eval_wrapper(task, evaluators) + return await wrapped(case=case) + + +class TestNoEvaluatorsRaises: + def test_empty_list_raises(self) -> None: + with pytest.raises(NoEvaluatorsError) as excinfo: + asyncio.run(_invoke([], EvalCase(inputs="x", name="c1"))) + assert "c1" in str(excinfo.value) + + def test_empty_short_circuit_rejected_at_construction(self) -> None: + """ShortCircuit([]) would contribute zero evaluators - rejected + even earlier, when the group is built.""" + with pytest.raises(ValueError, match="at least one evaluator"): + ShortCircuit([]) + + def test_session_path_fails_with_no_evaluators_error(self) -> None: + """Through the runner, the guard surfaces as a NoEvaluatorsError on + the case, not a silently green session.""" + from protest.api import run_session # noqa: PLC0415 - heavy import + from protest.plugin import PluginBase # noqa: PLC0415 - heavy import + + captured = [] + + class Capture(PluginBase): + name = "capture" + + def on_test_fail(self, result) -> None: + captured.append(result) + + cases = ForEach([EvalCase(inputs="x", name="c1")]) + session = ProTestSession() + session.register_plugin(Capture()) + suite = EvalSuite("evals") + + @suite.eval() + def forgot_evaluators(case: Annotated[EvalCase, From(cases)]) -> str: + return str(case.inputs) + + # Twin eval with an evaluator: must stay green, proving the + # failure below is the guard and not collateral wiring breakage. + @suite.eval(evaluators=[_ok]) + def wired_correctly(case: Annotated[EvalCase, From(cases)]) -> str: + return str(case.inputs) + + _ = forgot_evaluators, wired_correctly + session.add_suite(suite) + result = run_session(session) + assert result.success is False + assert len(captured) == 1 + assert isinstance(captured[0].error, NoEvaluatorsError) + assert "no evaluators" in str(captured[0].error) + + +class TestEvaluatorsPresentPasses: + def test_suite_level_evaluator_passes_guard(self) -> None: + payload = asyncio.run(_invoke([_ok], EvalCase(inputs="x", name="c1"))) + assert "_ok" in payload.scores + + def test_per_case_evaluators_satisfy_guard(self) -> None: + """Empty suite-level list is fine when the case brings its own.""" + case = EvalCase(inputs="x", name="c1", evaluators=[_ok]) + payload = asyncio.run(_invoke([], case)) + assert "_ok" in payload.scores