From 0494bac8a7a2703c6f67b003a139f51989ce9eb1 Mon Sep 17 00:00:00 2001 From: Anton Volkov Date: Mon, 26 Oct 2026 13:02:34 +0100 Subject: [PATCH 1/2] fix(eval): cross-judge exits 2 on a result entry of the wrong shape, before any judge call In the #189 review, wrong-shaped contents inside a `--pairwise` file were left for a follow-up: `{"verdicts": {"skill": {"q1": {}}}}` ended in `KeyError: 'score'`. The same holds for a verdict that is not an object, a non-numeric score, an arm that is not an object of verdicts, and in a `--checklist` file for a question with no `question_id`, an `arms` value or an arm that is not an object, or a coverage that is not a number. The measures were also built only after the `--judges` loop, so the traceback came after the paid judge calls, although the comment above the file reads promised that a bad file costs none. `pairwise_measure` and `coverage_measure` now raise ValueError naming the first bad entry (`verdicts.skill.q1`, `questions[1]`), and cross-judge builds both measures when it reads the files, before the first judge call, printing `Error: : ...` with exit 2. A numeric string score is still accepted. `cmd_eval_cross_judge` drops from complexity 32 to 30, and its growth entry follows. Signed-off-by: Anton Volkov --- ...00_antonwolfy_cross-judge-verdict-shape.md | 8 +++++ dokimi/dokimi/commands/judge_reliability.py | 25 +++++++++---- dokimi/dokimi/eval/cross_judge.py | 31 +++++++++++++--- dokimi/scripts/growth_baseline.json | 2 +- dokimi/tests/test_record_readers.py | 35 +++++++++++++++++++ 5 files changed, 88 insertions(+), 13 deletions(-) create mode 100644 dokimi/changelog.d/20261026_120000_antonwolfy_cross-judge-verdict-shape.md diff --git a/dokimi/changelog.d/20261026_120000_antonwolfy_cross-judge-verdict-shape.md b/dokimi/changelog.d/20261026_120000_antonwolfy_cross-judge-verdict-shape.md new file mode 100644 index 000000000..824ca3d5b --- /dev/null +++ b/dokimi/changelog.d/20261026_120000_antonwolfy_cross-judge-verdict-shape.md @@ -0,0 +1,8 @@ +### Fixed + +- **`eval cross-judge` exits 2 on a `--pairwise` or `--checklist` entry of the wrong shape, + before any judge call** — a verdict with no numeric `score`, an arm that is not an object + of verdicts, or a checklist question without a `question_id`, an `arms` object or a number + (or null) `coverage` now prints `Error: : ...` and exits 2 instead of ending + in a Python traceback. The entries are read with the file, before `--judges` makes its + first paid call; until now a wrong shape ended the run after those calls. diff --git a/dokimi/dokimi/commands/judge_reliability.py b/dokimi/dokimi/commands/judge_reliability.py index 762da883b..5eb95837e 100644 --- a/dokimi/dokimi/commands/judge_reliability.py +++ b/dokimi/dokimi/commands/judge_reliability.py @@ -51,6 +51,19 @@ def _judge_name(doc: dict, path: str) -> str: return judge if isinstance(judge, str) and judge else Path(path).stem +def _result_measure(path: str, key: str, kind: type, build): + """The measure an `eval pairwise` or `eval checklist` file gives, or exit 2 naming the file. + + *build* is ``(doc, name) -> Measure``; it raises ValueError on an entry of the wrong shape. + """ + doc = _load_json_object(path, key, kind) + try: + return build(doc, _judge_name(doc, path)) + except ValueError as exc: + print(f"Error: {path}: {exc}", file=sys.stderr) + sys.exit(2) + + def _load_answers(path: str): """The parsed file and its answers records (dokimi/records.py), or a clean exit.""" from dokimi.records import RecordError, read @@ -361,9 +374,10 @@ def score(library, question, answer, ground_truth, context): sys.exit(2) report.measures.append(m) - # Read every result file before the first judge call, so a bad path costs no paid calls. - pairwise = [(path, _load_json_object(path, "verdicts")) for path in args.pairwise or []] - checklists = [(path, _load_json_object(path, "questions", list)) for path in args.checklist or []] + # Read every result file, and its entries, before the first judge call, so a bad file costs no paid calls. + from_files = [_result_measure(p, "verdicts", dict, lambda doc, name: pairwise_measure(doc, name, baseline)) + for p in args.pairwise or []] + from_files += [_result_measure(p, "questions", list, coverage_measure) for p in args.checklist or []] validity = [_load_json_object(v, "summary")["summary"] for v in args.validity or []] specs = [j.strip() for j in (args.judges or "").split(",") if j.strip()] if specs: @@ -385,10 +399,7 @@ def score(library, question, answer, ground_truth, context): concurrency=args.concurrency, progress=_progress(), ground_truth=grounding, store=materials, )) - for path, pw in pairwise: - report.measures.append(pairwise_measure(pw, _judge_name(pw, path), baseline)) - for path, ck in checklists: - report.measures.append(coverage_measure(ck, _judge_name(ck, path))) + report.measures += from_files answers_model = data.get("model") if isinstance(data, dict) else None for v in validity: if v.get("answers_model") and answers_model and v["answers_model"] != answers_model: diff --git a/dokimi/dokimi/eval/cross_judge.py b/dokimi/dokimi/eval/cross_judge.py index df9c18ae2..0f36889df 100644 --- a/dokimi/dokimi/eval/cross_judge.py +++ b/dokimi/dokimi/eval/cross_judge.py @@ -230,19 +230,40 @@ def stored_measure(data: Dict[str, Any], name: Optional[str] = None) -> Optional def coverage_measure(checklist: Dict[str, Any], name: str) -> Measure: + """An `eval checklist` report as a measure: each arm scores its coverage. + + Raises ValueError naming the first question that is not ``{question_id, arms: {arm: + {coverage}}}`` with a number or null coverage. + """ m = Measure(name=name, kind="coverage") - for q in checklist.get("questions") or []: - for arm, v in (q.get("arms") or {}).items(): - m.scores[(q["question_id"], arm)] = v.get("coverage") + for i, q in enumerate(checklist.get("questions") or []): + try: + for arm, v in (q.get("arms") or {}).items(): + coverage = v.get("coverage") + if coverage is not None and not isinstance(coverage, (int, float)): + raise TypeError(coverage) + m.scores[(q["question_id"], arm)] = coverage + except (AttributeError, KeyError, TypeError): + raise ValueError(f"questions[{i}] is not a checklist question: a question_id and " + "an arms object of {arm: {coverage: number or null}}") from None return m def pairwise_measure(pairwise: Dict[str, Any], name: str, baseline_arm: str) -> Measure: - """An `eval pairwise` report as a measure: the arm scores its verdict, the baseline 0.""" + """An `eval pairwise` report as a measure: the arm scores its verdict, the baseline 0. + + Raises ValueError naming the first verdict that is not an object with a numeric ``score``. + """ m = Measure(name=name, kind="pairwise") for arm, by_q in (pairwise.get("verdicts") or {}).items(): + if not isinstance(by_q, dict): + raise ValueError(f"verdicts.{arm} is not an object of per-question verdicts") for qid, v in by_q.items(): - m.scores[(qid, arm)] = float(v["score"]) + try: + score = float(v["score"]) + except (KeyError, TypeError, ValueError): + raise ValueError(f"verdicts.{arm}.{qid} has no numeric score") from None + m.scores[(qid, arm)] = score m.scores[(qid, baseline_arm)] = 0.0 return m diff --git a/dokimi/scripts/growth_baseline.json b/dokimi/scripts/growth_baseline.json index d0d35275e..631cc9a53 100644 --- a/dokimi/scripts/growth_baseline.json +++ b/dokimi/scripts/growth_baseline.json @@ -22,7 +22,7 @@ "dokimi/commands/benchmark.py::cmd_benchmark_preflight": {"complexity": 22}, "dokimi/commands/catalog.py::cmd_catalog_trigger_set": {"complexity": 16}, "dokimi/commands/evaluate.py::cmd_eval_judge_validity": {"complexity": 29, "rationale": "grew on main in #73 (judge-validity --context stored) from 25 to 29 before the ratchet landed; split it in its own PR"}, - "dokimi/commands/judge_reliability.py::cmd_eval_cross_judge": {"complexity": 32, "rationale": "pre-existing when the gate was introduced"}, + "dokimi/commands/judge_reliability.py::cmd_eval_cross_judge": {"complexity": 30, "rationale": "pre-existing when the gate was introduced"}, "dokimi/commands/novelty.py::cmd_novelty_routing": {"complexity": 13, "rationale": "W8 adds the --repeats guard after main's no_queries handling raised the file; the command is split with the rest of novelty.py in a separate PR"}, "dokimi/commands/repo.py::cmd_repo_scan": {"complexity": 13}, "dokimi/commands/repo.py::cmd_repo_validate": {"complexity": 11, "rationale": "grew on main in #150 (repo validate --fail-on-warning) from 10 to 11 before the ratchet landed; split it in its own PR"}, diff --git a/dokimi/tests/test_record_readers.py b/dokimi/tests/test_record_readers.py index b67d45160..628672910 100644 --- a/dokimi/tests/test_record_readers.py +++ b/dokimi/tests/test_record_readers.py @@ -659,3 +659,38 @@ def test_cross_judge_names_a_result_file_measure_after_its_summary_judge(tmp_pat cli.main() md = out.with_suffix(".md").read_text(encoding="utf-8") assert " · judge-a |" in md and " · result |" not in md + + +_GOOD_QUESTION = {"question_id": "q0", "arms": {"skill": {"coverage": 1}}} + + +@pytest.mark.parametrize("flag, content, where", [ + ("--pairwise", {"verdicts": {"skill": {"q1": {}}}}, "verdicts.skill.q1"), + ("--pairwise", {"verdicts": {"skill": {"q1": 5}}}, "verdicts.skill.q1"), + ("--pairwise", {"verdicts": {"skill": {"q1": {"score": "x"}}}}, "verdicts.skill.q1"), + ("--pairwise", {"verdicts": {"skill": {"q1": {"score": None}}}}, "verdicts.skill.q1"), + ("--pairwise", {"verdicts": {"skill": 5}}, "verdicts.skill"), + ("--checklist", {"questions": [5]}, "questions[0]"), + ("--checklist", {"questions": [_GOOD_QUESTION, {"arms": {"skill": {"coverage": 1}}}]}, "questions[1]"), + ("--checklist", {"questions": [{"question_id": "q1", "arms": [1]}]}, "questions[0]"), + ("--checklist", {"questions": [{"question_id": "q1", "arms": {"skill": 5}}]}, "questions[0]"), + ("--checklist", {"questions": [{"question_id": "q1", "arms": {"skill": {"coverage": "x"}}}]}, "questions[0]"), + ("--checklist", {"questions": [{"question_id": ["q1"], "arms": {"skill": {"coverage": 1}}}]}, "questions[0]"), +], ids=["no-score", "verdict-not-an-object", "score-not-a-number", "null-score", "arm-not-an-object", + "question-not-an-object", "no-question-id", "arms-not-an-object", "arm-not-an-object", + "coverage-not-a-number", "unhashable-question-id"]) +def test_cross_judge_names_a_result_entry_of_the_wrong_shape_before_any_judge_call( + tmp_path, capsys, monkeypatch, flag, content, where): + from tests.test_judge_reliability import _cross_judge_args, _material_arms_run, _recording_judge + + seen: dict = {} + _recording_judge(monkeypatch, seen) + wrong = tmp_path / "wrong.json" + wrong.write_text(json.dumps(content), encoding="utf-8") + args = _cross_judge_args(tmp_path, _material_arms_run(), "--no-stored", "--judges", "amazon-bedrock/m", + flag, str(wrong)) + with pytest.raises(SystemExit) as exc: + args.func(args) + assert exc.value.code == 2 + assert capsys.readouterr().err.startswith(f"Error: {wrong}: {where} ") + assert seen == {} # no paid judge call was made From cb23dd5901482fcb0849850d78e4d2095badaefc Mon Sep 17 00:00:00 2001 From: Anton Volkov Date: Mon, 26 Oct 2026 21:41:30 +0100 Subject: [PATCH 2/2] fix(eval): a result file's score and coverage are finite numbers; a checklist question needs both keys From the reviews of #233: - vlad-perevezentsev: `q.get("arms") or {}` skipped a question with no `arms`, a null or a list one, and read `question_id` only inside the arm loop, so `{"question_id": "q1"}` and `{"arms": {}}` passed and two null ids overwrote each other, while the changelog says such a question exits 2. A question now needs a `question_id` that is not null or a bool, and an `arms` object, before its arms are read. - vlad-perevezentsev and napetrov: "numeric" meant two things. A pairwise score went through `float()`, which takes "1", NaN and infinity and raises OverflowError, uncaught, on an integer past a float's range; a coverage of `true` or NaN passed the type check. Both now go through `_number`: a finite JSON number, not a bool or a string. `eval pairwise` writes an int score and `eval checklist` a float or null coverage, so no file they wrote changes reading. - vchamarthi: the test id `arm-not-an-object` was used twice; the checklist one is `arm-entry-not-an-object`. A checklist arm with a null coverage, which `eval checklist` writes for an arm with no scored point, has a test of its own. Signed-off-by: Anton Volkov --- ...00_antonwolfy_cross-judge-verdict-shape.md | 13 ++-- dokimi/dokimi/eval/cross_judge.py | 39 ++++++---- dokimi/tests/test_record_readers.py | 72 +++++++++++++++---- 3 files changed, 93 insertions(+), 31 deletions(-) diff --git a/dokimi/changelog.d/20261026_120000_antonwolfy_cross-judge-verdict-shape.md b/dokimi/changelog.d/20261026_120000_antonwolfy_cross-judge-verdict-shape.md index 824ca3d5b..e9af067ea 100644 --- a/dokimi/changelog.d/20261026_120000_antonwolfy_cross-judge-verdict-shape.md +++ b/dokimi/changelog.d/20261026_120000_antonwolfy_cross-judge-verdict-shape.md @@ -1,8 +1,11 @@ ### Fixed - **`eval cross-judge` exits 2 on a `--pairwise` or `--checklist` entry of the wrong shape, - before any judge call** — a verdict with no numeric `score`, an arm that is not an object - of verdicts, or a checklist question without a `question_id`, an `arms` object or a number - (or null) `coverage` now prints `Error: : ...` and exits 2 instead of ending - in a Python traceback. The entries are read with the file, before `--judges` makes its - first paid call; until now a wrong shape ended the run after those calls. + before any judge call** — a verdict whose `score` is not a finite number (missing, null, + a string, a bool, NaN, infinite or past a float's range), an arm that is not an object of + verdicts, or a checklist question without a `question_id` (or with a null or bool one), + without an `arms` object, or with a `coverage` that is neither a finite number nor null + now prints `Error: : ...` and exits 2 instead of ending in a Python + traceback or passing a bad value on. The entries are read with the file, before + `--judges` makes its first paid call; until now a wrong shape ended the run after those + calls. diff --git a/dokimi/dokimi/eval/cross_judge.py b/dokimi/dokimi/eval/cross_judge.py index 0f36889df..967de4a00 100644 --- a/dokimi/dokimi/eval/cross_judge.py +++ b/dokimi/dokimi/eval/cross_judge.py @@ -46,6 +46,7 @@ from __future__ import annotations +import math import re from concurrent.futures import ThreadPoolExecutor from dataclasses import dataclass, field @@ -229,30 +230,44 @@ def stored_measure(data: Dict[str, Any], name: Optional[str] = None) -> Optional return m if m.scores else None +def _number(x: Any) -> float: + """A score or coverage from a result file: a finite JSON number, not a bool or a string. + + Raises TypeError, ValueError, or OverflowError for an integer past a float's range.""" + if isinstance(x, bool) or not isinstance(x, (int, float)): + raise TypeError(x) + value = float(x) + if not math.isfinite(value): + raise ValueError(x) + return value + + def coverage_measure(checklist: Dict[str, Any], name: str) -> Measure: """An `eval checklist` report as a measure: each arm scores its coverage. Raises ValueError naming the first question that is not ``{question_id, arms: {arm: - {coverage}}}`` with a number or null coverage. + {coverage}}}`` with a finite number or null as the coverage. """ m = Measure(name=name, kind="coverage") for i, q in enumerate(checklist.get("questions") or []): try: - for arm, v in (q.get("arms") or {}).items(): + qid, arms = q["question_id"], q["arms"] + if qid is None or isinstance(qid, bool): + raise TypeError(qid) + for arm, v in arms.items(): coverage = v.get("coverage") - if coverage is not None and not isinstance(coverage, (int, float)): - raise TypeError(coverage) - m.scores[(q["question_id"], arm)] = coverage - except (AttributeError, KeyError, TypeError): - raise ValueError(f"questions[{i}] is not a checklist question: a question_id and " - "an arms object of {arm: {coverage: number or null}}") from None + m.scores[(qid, arm)] = None if coverage is None else _number(coverage) + except (AttributeError, KeyError, TypeError, ValueError, OverflowError): + raise ValueError(f"questions[{i}] is not a checklist question: a question_id and an arms " + "object of {arm: {coverage: a finite number or null}}") from None return m def pairwise_measure(pairwise: Dict[str, Any], name: str, baseline_arm: str) -> Measure: """An `eval pairwise` report as a measure: the arm scores its verdict, the baseline 0. - Raises ValueError naming the first verdict that is not an object with a numeric ``score``. + Raises ValueError naming the first verdict that is not an object with a finite number as + its ``score``. """ m = Measure(name=name, kind="pairwise") for arm, by_q in (pairwise.get("verdicts") or {}).items(): @@ -260,9 +275,9 @@ def pairwise_measure(pairwise: Dict[str, Any], name: str, baseline_arm: str) -> raise ValueError(f"verdicts.{arm} is not an object of per-question verdicts") for qid, v in by_q.items(): try: - score = float(v["score"]) - except (KeyError, TypeError, ValueError): - raise ValueError(f"verdicts.{arm}.{qid} has no numeric score") from None + score = _number(v["score"]) + except (KeyError, TypeError, ValueError, OverflowError): + raise ValueError(f"verdicts.{arm}.{qid} has no score that is a finite number") from None m.scores[(qid, arm)] = score m.scores[(qid, baseline_arm)] = 0.0 return m diff --git a/dokimi/tests/test_record_readers.py b/dokimi/tests/test_record_readers.py index 628672910..c6948d8a6 100644 --- a/dokimi/tests/test_record_readers.py +++ b/dokimi/tests/test_record_readers.py @@ -664,21 +664,48 @@ def test_cross_judge_names_a_result_file_measure_after_its_summary_judge(tmp_pat _GOOD_QUESTION = {"question_id": "q0", "arms": {"skill": {"coverage": 1}}} +def _pairwise(verdict): + return {"verdicts": {"skill": {"q1": verdict}}} + + +def _checklist(question): + return {"questions": [question]} + + @pytest.mark.parametrize("flag, content, where", [ - ("--pairwise", {"verdicts": {"skill": {"q1": {}}}}, "verdicts.skill.q1"), - ("--pairwise", {"verdicts": {"skill": {"q1": 5}}}, "verdicts.skill.q1"), - ("--pairwise", {"verdicts": {"skill": {"q1": {"score": "x"}}}}, "verdicts.skill.q1"), - ("--pairwise", {"verdicts": {"skill": {"q1": {"score": None}}}}, "verdicts.skill.q1"), - ("--pairwise", {"verdicts": {"skill": 5}}, "verdicts.skill"), - ("--checklist", {"questions": [5]}, "questions[0]"), - ("--checklist", {"questions": [_GOOD_QUESTION, {"arms": {"skill": {"coverage": 1}}}]}, "questions[1]"), - ("--checklist", {"questions": [{"question_id": "q1", "arms": [1]}]}, "questions[0]"), - ("--checklist", {"questions": [{"question_id": "q1", "arms": {"skill": 5}}]}, "questions[0]"), - ("--checklist", {"questions": [{"question_id": "q1", "arms": {"skill": {"coverage": "x"}}}]}, "questions[0]"), - ("--checklist", {"questions": [{"question_id": ["q1"], "arms": {"skill": {"coverage": 1}}}]}, "questions[0]"), -], ids=["no-score", "verdict-not-an-object", "score-not-a-number", "null-score", "arm-not-an-object", - "question-not-an-object", "no-question-id", "arms-not-an-object", "arm-not-an-object", - "coverage-not-a-number", "unhashable-question-id"]) + pytest.param("--pairwise", _pairwise({}), "verdicts.skill.q1", id="no-score"), + pytest.param("--pairwise", _pairwise(5), "verdicts.skill.q1", id="verdict-not-an-object"), + pytest.param("--pairwise", _pairwise({"score": "x"}), "verdicts.skill.q1", id="score-not-a-number"), + pytest.param("--pairwise", _pairwise({"score": "1"}), "verdicts.skill.q1", id="score-a-numeric-string"), + pytest.param("--pairwise", _pairwise({"score": None}), "verdicts.skill.q1", id="null-score"), + pytest.param("--pairwise", _pairwise({"score": True}), "verdicts.skill.q1", id="bool-score"), + pytest.param("--pairwise", _pairwise({"score": float("nan")}), "verdicts.skill.q1", id="nan-score"), + pytest.param("--pairwise", _pairwise({"score": float("inf")}), "verdicts.skill.q1", id="infinite-score"), + pytest.param("--pairwise", _pairwise({"score": 10 ** 400}), "verdicts.skill.q1", id="score-past-float-range"), + pytest.param("--pairwise", {"verdicts": {"skill": 5}}, "verdicts.skill", id="arm-not-an-object"), + pytest.param("--checklist", {"questions": [5]}, "questions[0]", id="question-not-an-object"), + pytest.param("--checklist", {"questions": [_GOOD_QUESTION, {"arms": {"skill": {"coverage": 1}}}]}, + "questions[1]", id="no-question-id"), + pytest.param("--checklist", _checklist({"question_id": None, "arms": {"skill": {"coverage": 1}}}), + "questions[0]", id="null-question-id"), + pytest.param("--checklist", _checklist({"question_id": True, "arms": {"skill": {"coverage": 1}}}), + "questions[0]", id="bool-question-id"), + pytest.param("--checklist", _checklist({"question_id": ["q1"], "arms": {"skill": {"coverage": 1}}}), + "questions[0]", id="unhashable-question-id"), + pytest.param("--checklist", _checklist({"question_id": "q1"}), "questions[0]", id="no-arms"), + pytest.param("--checklist", _checklist({"question_id": "q1", "arms": None}), "questions[0]", id="null-arms"), + pytest.param("--checklist", _checklist({"question_id": "q1", "arms": [1]}), "questions[0]", id="arms-not-an-object"), + pytest.param("--checklist", _checklist({"question_id": "q1", "arms": {"skill": 5}}), "questions[0]", + id="arm-entry-not-an-object"), + pytest.param("--checklist", _checklist({"question_id": "q1", "arms": {"skill": {"coverage": "x"}}}), + "questions[0]", id="coverage-not-a-number"), + pytest.param("--checklist", _checklist({"question_id": "q1", "arms": {"skill": {"coverage": True}}}), + "questions[0]", id="bool-coverage"), + pytest.param("--checklist", _checklist({"question_id": "q1", "arms": {"skill": {"coverage": float("nan")}}}), + "questions[0]", id="nan-coverage"), + pytest.param("--checklist", _checklist({"question_id": "q1", "arms": {"skill": {"coverage": 10 ** 400}}}), + "questions[0]", id="coverage-past-float-range"), +]) def test_cross_judge_names_a_result_entry_of_the_wrong_shape_before_any_judge_call( tmp_path, capsys, monkeypatch, flag, content, where): from tests.test_judge_reliability import _cross_judge_args, _material_arms_run, _recording_judge @@ -694,3 +721,20 @@ def test_cross_judge_names_a_result_entry_of_the_wrong_shape_before_any_judge_ca assert exc.value.code == 2 assert capsys.readouterr().err.startswith(f"Error: {wrong}: {where} ") assert seen == {} # no paid judge call was made + + +def test_cross_judge_takes_a_checklist_arm_with_a_null_coverage(tmp_path, monkeypatch): + """`eval checklist` writes a null coverage for an arm with no scored point.""" + from dokimi import cli + + arms = tmp_path / "arms.json" + arms.write_text(json.dumps({"answers": [{"question_id": "q1", "arms": {"skill": {"answer": "s"}}}]}), + encoding="utf-8") + result = tmp_path / "result.json" + result.write_text(json.dumps(_checklist({"question_id": "q1", "arms": {"skill": {"coverage": None}}})), + encoding="utf-8") + out = tmp_path / "out.json" + monkeypatch.setattr("sys.argv", ["dokimi", "eval", "cross-judge", "--answers", str(arms), "--no-stored", + "--checklist", str(result), "--output", str(out)]) + cli.main() + assert " · result |" in out.with_suffix(".md").read_text(encoding="utf-8")