diff --git a/protest/evals/wrapper.py b/protest/evals/wrapper.py index 93b27cb..d5b7d42 100644 --- a/protest/evals/wrapper.py +++ b/protest/evals/wrapper.py @@ -13,6 +13,7 @@ from typing import Annotated, Any, get_args, get_origin from protest.di.hints import get_type_hints_compat +from protest.di.markers import From from protest.entities.events import EvalPayload, EvalScoreEntry from protest.evals.evaluator import ( EvalCase, @@ -39,18 +40,23 @@ def make_eval_wrapper( ) -> Any: """Wrap a function to run evaluators on its return value.""" - _validate_single_evalcase_param(func) + # Resolve which parameter carries the EvalCase once, statically. The name + # is then used for a direct kwargs lookup below - the case is never + # rediscovered by scanning values. + case_param = _resolve_case_param(func) validate_evaluators(evaluators) @functools.wraps(func) async def eval_wrapper(**kwargs: Any) -> EvalPayload: - expected = _extract_expected(kwargs) - case_name = _extract_case_name(kwargs, func.__name__) - inputs = _extract_inputs(kwargs) - metadata = _extract_metadata(kwargs) + case = kwargs.get(case_param) if case_param is not None else None + expected = case.expected if case is not None else None + case_name = case.name if case is not None else func.__name__ + inputs = case.inputs if case is not None else None + metadata = (case.metadata or None) if case is not None else None all_evaluators = list(evaluators) - all_evaluators.extend(_extract_per_case_evaluators(kwargs)) + if case is not None and case.evaluators: + all_evaluators.extend(case.evaluators) # Both guards run before the task itself: the evaluator list is # fully known from the kwargs alone, and the task is typically the @@ -140,35 +146,59 @@ async def eval_wrapper(**kwargs: Any) -> EvalPayload: # --------------------------------------------------------------------------- -def _validate_single_evalcase_param(func: Any) -> None: - """Raise MultipleEvalCaseParamsError if `func` has > 1 EvalCase parameter. +def _resolve_case_param(func: Any) -> str | None: + """Return the name of the single parameter that carries the EvalCase. - Runs at decorator time. The runtime contract (`_find_case`) silently - picks the first EvalCase in kwargs, which would drop the second one's - name/expected/inputs/metadata/per-case evaluators downstream. We catch - that here so the failure is loud and pinpoints the offending eval. + Resolved once at decoration time, from the signature alone. A parameter + is the case parameter when either signal holds: - Subclasses of EvalCase count: the runtime uses isinstance(_, EvalCase), - so any subclass would trigger the same silent drop. + - its declared type is an EvalCase (subclass) - ``case: EvalCase`` or + ``Annotated[EvalCase, From(cases)]``; or + - it is bound via ``From(source)`` whose source yields EvalCase instances - + ``Annotated[Any, From(cases)]``, where the type is deliberately loose. + + The returned name drives a direct ``kwargs[name]`` lookup at runtime. This + is the whole point: the case is identified by *which parameter it is*, not + by scanning kwargs values for an EvalCase instance. A fixture that merely + returns an EvalCase on an unrelated parameter therefore cannot be mistaken + for the case (the silent misattribution the old isinstance scan allowed). + + Raises MultipleEvalCaseParamsError if more than one parameter qualifies - + only one case per eval defines its identity (name, expected, inputs, + metadata, per-case evaluators). Returns None when no parameter qualifies + (a static eval, or one parametrized over non-EvalCase values). """ hints = get_type_hints_compat(func) - offending: list[str] = [] - for param_name, annotation in hints.items(): - if param_name == "return": - continue - underlying = ( - get_args(annotation)[0] - if get_origin(annotation) is Annotated - else annotation - ) + matches = [ + name + for name, annotation in hints.items() + if name != "return" and _is_case_param(annotation) + ] + if len(matches) > 1: + raise MultipleEvalCaseParamsError(func.__name__, matches) + return matches[0] if matches else None + + +def _is_case_param(annotation: Any) -> bool: + if get_origin(annotation) is Annotated: + args = get_args(annotation) + underlying = args[0] if isinstance(underlying, type) and issubclass(underlying, EvalCase): - offending.append(param_name) - if len(offending) > 1: - raise MultipleEvalCaseParamsError(func.__name__, offending) + return True + return any( + isinstance(meta, From) and _source_yields_evalcase(meta.source) + for meta in args[1:] + ) + return isinstance(annotation, type) and issubclass(annotation, EvalCase) + + +def _source_yields_evalcase(source: Any) -> bool: + """True if a From source yields EvalCase instances (ForEach is non-empty).""" + return isinstance(next(iter(source), None), EvalCase) # --------------------------------------------------------------------------- -# Extract helpers - pull EvalCase from kwargs +# Evaluator list helpers # --------------------------------------------------------------------------- @@ -204,49 +234,6 @@ def _flatten_evaluators( return flat -def _find_case(kwargs: dict[str, Any]) -> EvalCase | None: - """Find the EvalCase instance in kwargs.""" - for v in kwargs.values(): - if isinstance(v, EvalCase): - return v - return None - - -def _extract_expected(kwargs: dict[str, Any]) -> Any: - case = _find_case(kwargs) - if case is None: - return None - return case.expected - - -def _extract_case_name(kwargs: dict[str, Any], fallback: str) -> str: - case = _find_case(kwargs) - if case is None: - return fallback - return case.name - - -def _extract_inputs(kwargs: dict[str, Any]) -> Any: - case = _find_case(kwargs) - if case is None: - return None - return case.inputs - - -def _extract_metadata(kwargs: dict[str, Any]) -> Any: - case = _find_case(kwargs) - if case is None: - return None - return case.metadata or None - - -def _extract_per_case_evaluators(kwargs: dict[str, Any]) -> list[Any]: - case = _find_case(kwargs) - if case is None or not case.evaluators: - return [] - return list(case.evaluators) - - # --------------------------------------------------------------------------- # Evaluator execution # --------------------------------------------------------------------------- diff --git a/tests/evals/test_multiple_evalcase_params.py b/tests/evals/test_multiple_evalcase_params.py index 65f8b43..af0a5f8 100644 --- a/tests/evals/test_multiple_evalcase_params.py +++ b/tests/evals/test_multiple_evalcase_params.py @@ -1,24 +1,33 @@ -"""Tests for `_validate_single_evalcase_param` - D1 registration-time check. +"""Tests for case-parameter resolution (`_resolve_case_param`). -The runtime contract (`_find_case`) picks the first `EvalCase` in kwargs and -silently drops any others. The wrapper detects > 1 EvalCase param at -registration and raises a clear error pointing at the offending parameters. +The wrapper identifies the EvalCase by *which parameter carries it*, resolved +once from the signature at decoration time, not by scanning kwargs values at +runtime. This file covers: + +- single case parameter accepted (typed, subclass, or loose-typed via `From`); +- more than one case parameter rejected loudly at registration; +- a parameter that merely *holds* an EvalCase at runtime, without being the + declared case parameter, is ignored - not misattributed (issue #120). """ from __future__ import annotations -from typing import Annotated +import asyncio +from typing import Annotated, Any import pytest from protest import ForEach, From, ProTestSession -from protest.evals import EvalCase +from protest.evals import EvalCase, EvalContext, evaluator from protest.evals.suite import EvalSuite +from protest.evals.wrapper import make_eval_wrapper from protest.exceptions import MultipleEvalCaseParamsError # Module-level case sources so `get_type_hints()` can resolve Annotated args. _cases_a = ForEach([EvalCase(inputs="a", name="a1")]) _cases_b = ForEach([EvalCase(inputs="b", name="b1")]) +_loose_cases = ForEach([EvalCase(inputs="loose", name="loose1")]) +_dict_cases = ForEach([{"inputs": "d"}]) class _MyCase(EvalCase): @@ -28,6 +37,11 @@ class _MyCase(EvalCase): _subclass_cases = ForEach([_MyCase(inputs="x", name="x1")]) +@evaluator +def _ok(ctx: EvalContext) -> bool: + return True + + class TestSingleEvalCaseParamAccepted: def test_one_evalcase_param_via_annotated_from(self) -> None: session = ProTestSession() @@ -63,6 +77,32 @@ def good(case: Annotated[_MyCase, From(_subclass_cases)]) -> str: _ = good session.add_suite(suite) + def test_loose_typed_from_source_is_recognized_as_case(self) -> None: + """`Annotated[Any, From(cases)]` over EvalCase items is the case param. + + The type is deliberately loose; resolution keys off the From source + yielding EvalCase instances, so case identity is still wired up. + """ + + def task(case: Annotated[Any, From(_loose_cases)]) -> str: + return str(case.inputs) + + wrapped = make_eval_wrapper(task, [_ok]) + payload = asyncio.run(wrapped(case=EvalCase(inputs="z", name="z1"))) + assert payload.case_name == "z1" + + def test_from_source_of_non_evalcase_is_not_a_case_param(self) -> None: + """A `From` over plain dicts is parametrization, not a case: the eval + falls back to the function name and carries no expected/inputs.""" + + def task(case: Annotated[dict, From(_dict_cases)]) -> str: + return str(case["inputs"]) + + wrapped = make_eval_wrapper(task, [_ok]) + payload = asyncio.run(wrapped(case={"inputs": "d"})) + assert payload.case_name == "task" + assert payload.expected_output is None + class TestMultipleEvalCaseParamRejected: def test_two_evalcase_params_raise(self) -> None: @@ -97,3 +137,51 @@ def bad( assert "case_a" in str(excinfo.value) assert "case_b" in str(excinfo.value) + + def test_loose_from_collides_with_typed_case(self) -> None: + """A loose `From(EvalCase items)` param and a typed EvalCase param both + qualify - two case params, rejected at registration.""" + suite = EvalSuite("evals") + + with pytest.raises(MultipleEvalCaseParamsError) as excinfo: + + @suite.eval() + def bad( + case_a: Annotated[EvalCase, From(_cases_a)], + case_b: Annotated[Any, From(_loose_cases)], + ) -> str: + return str(case_a.inputs) + + assert "case_a" in str(excinfo.value) + assert "case_b" in str(excinfo.value) + + +class TestRuntimeEvalCaseOnUnrelatedParamIgnored: + """Issue #120, resolved structurally. The case is identified by parameter + name (resolved at decoration), not by scanning values. A fixture that + returns an EvalCase on an unrelated parameter is therefore simply ignored - + it cannot shadow or be misattributed as the declared case. + """ + + def test_evalcase_on_unrelated_param_is_ignored(self) -> None: + def task(case: EvalCase, sneaky: object) -> str: + return str(case.inputs) + + wrapped = make_eval_wrapper(task, [_ok]) + payload = asyncio.run( + wrapped( + case=EvalCase(inputs="a", name="a1"), + sneaky=EvalCase(inputs="b", name="b1"), + ) + ) + # The declared case wins; `sneaky` is not consulted. + assert payload.case_name == "a1" + assert payload.expected_output is None + + def test_single_runtime_evalcase_kwarg_ok(self) -> None: + def task(case: EvalCase) -> str: + return str(case.inputs) + + wrapped = make_eval_wrapper(task, [_ok]) + payload = asyncio.run(wrapped(case=EvalCase(inputs="a", name="a1"))) + assert payload.case_name == "a1"