From 351a19fd1c9935d7c8b8fecda49dfe70f896921c Mon Sep 17 00:00:00 2001 From: Anton Volkov Date: Fri, 23 Oct 2026 16:39:00 +0200 Subject: [PATCH 1/4] test(records): hold readers of with_docs/without_docs to dokimi.records Settles the review point on #80 that "every reader goes through records.py" was too strong, with nothing to keep it true for the next reader. - The pipeline summary in orchestrator/pipeline.py read the evaluation dicts' "with_docs"/"without_docs"/"delta" keys; it now reads them through records.records(), Record.score and Record.delta. A test pins the summary numbers (means over the judged questions, a failed question skipped). - tests/test_records_guard.py scans dokimi/ with ast for x["with_docs"] loads and x.get("with_docs") calls outside records.py, against a baseline of (module, function) -> reason. A new read fails, naming dokimi.records; a baseline entry that no longer matches fails as stale, so it only shrinks. - AGENTS.md names the test next to the rule. Signed-off-by: Anton Volkov --- AGENTS.md | 3 +- ..._160100_antonwolfy_records-reader-guard.md | 7 + dokimi/dokimi/orchestrator/pipeline.py | 8 +- dokimi/tests/test_pipeline_summary.py | 57 ++++++ dokimi/tests/test_records_guard.py | 184 ++++++++++++++++++ 5 files changed, 255 insertions(+), 4 deletions(-) create mode 100644 dokimi/changelog.d/20261023_160100_antonwolfy_records-reader-guard.md create mode 100644 dokimi/tests/test_pipeline_summary.py create mode 100644 dokimi/tests/test_records_guard.py diff --git a/AGENTS.md b/AGENTS.md index ca6b0e2c..16ad3d41 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -87,7 +87,8 @@ Registries, bundled in the package (`dokimi/dokimi/`) so they ship in the wheel: - Code that reads answers, eval or arms records goes through `dokimi/dokimi/records.py` (`read`/`load`/`records`), not `.get("with_docs")` on the dict: it is the one place that knows the keys - the writers write. + the writers write. `dokimi/tests/test_records_guard.py` fails a new such read + and lists the exceptions, which only shrink. ## What goes in the tree diff --git a/dokimi/changelog.d/20261023_160100_antonwolfy_records-reader-guard.md b/dokimi/changelog.d/20261023_160100_antonwolfy_records-reader-guard.md new file mode 100644 index 00000000..edfc2bd6 --- /dev/null +++ b/dokimi/changelog.d/20261023_160100_antonwolfy_records-reader-guard.md @@ -0,0 +1,7 @@ +### Changed + +- **Pipeline summary reads through `dokimi.records`** — the per-condition + averages and the delta average that `EvaluationPipeline.run` prints and returns + are now read with `dokimi.records` instead of the `with_docs`/`without_docs` + keys of the evaluation dicts. The numbers are unchanged. A test now holds every + other reader of those keys to a list of exceptions that only shrinks. diff --git a/dokimi/dokimi/orchestrator/pipeline.py b/dokimi/dokimi/orchestrator/pipeline.py index a732b71f..5badfda3 100644 --- a/dokimi/dokimi/orchestrator/pipeline.py +++ b/dokimi/dokimi/orchestrator/pipeline.py @@ -11,6 +11,7 @@ from dokimi.defaults import DEFAULT_ANSWER_MODEL, DEFAULT_ANSWER_PROVIDER, DEFAULT_JUDGE_MODEL, DEFAULT_JUDGE_PROVIDER, DEFAULT_RETRIEVAL_TOP_K from dokimi.mcp.factory import create_doc_source_client +from dokimi.records import BASELINE, CONTEXT, records as read_records logger = logging.getLogger(__name__) @@ -380,9 +381,10 @@ def run(self, concurrency: int = 5) -> Dict[str, Any]: } # Compute summary stats - with_scores = [e["with_docs"]["aggregate"] for e in evaluations if e.get("with_docs")] - without_scores = [e["without_docs"]["aggregate"] for e in evaluations if e.get("without_docs")] - deltas = [e["delta"] for e in evaluations if e.get("delta") is not None] + scored = read_records(evaluations, source="evaluations", kind="evaluations") + with_scores = [s for s in (r.score(CONTEXT) for r in scored) if s is not None] + without_scores = [s for s in (r.score(BASELINE) for r in scored) if s is not None] + deltas = [r.delta for r in scored if r.delta is not None] if with_scores and without_scores and deltas: results["steps"]["evaluation"]["summary"] = { diff --git a/dokimi/tests/test_pipeline_summary.py b/dokimi/tests/test_pipeline_summary.py new file mode 100644 index 00000000..f07b2beb --- /dev/null +++ b/dokimi/tests/test_pipeline_summary.py @@ -0,0 +1,57 @@ +# Copyright (C) 2026 Intel Corporation +# SPDX-License-Identifier: Apache-2.0 + +"""The summary ``EvaluationPipeline.run`` returns for the judged questions.""" + +from __future__ import annotations + +import json + +import pytest + +from dokimi.orchestrator.pipeline import EvaluationPipeline + + +def _evaluation(qid, with_score, without_score): + delta = None if with_score is None or without_score is None else with_score - without_score + return { + "question_id": qid, + "question_text": f"Question {qid}?", + "with_docs": None if with_score is None else {"aggregate": with_score}, + "without_docs": None if without_score is None else {"aggregate": without_score}, + "delta": delta, + } + + +def _run(tmp_path, monkeypatch: pytest.MonkeyPatch, evaluations): + """Run the pipeline on a cached question set with the LLM steps replaced.""" + monkeypatch.setattr(EvaluationPipeline, "_generate_answers", lambda self, *a, **k: []) + monkeypatch.setattr(EvaluationPipeline, "_evaluate_answers", lambda self, *a, **k: evaluations) + monkeypatch.setattr(EvaluationPipeline, "_generate_report", lambda self, *a, **k: "report") + output_dir = tmp_path / "run" + (output_dir / "personas").mkdir(parents=True) + (output_dir / "questions").mkdir() + (output_dir / "personas" / "oneTBB.json").write_text(json.dumps({"personas": []}), encoding="utf-8") + questions = [{"id": e["question_id"], "question": e["question_text"], "source_type": "manual"} + for e in evaluations] + (output_dir / "questions" / "oneTBB.json").write_text(json.dumps({"questions": questions}), encoding="utf-8") + pipeline = EvaluationPipeline(product="oneTBB", output_dir=output_dir, description="Threading Building Blocks") + return pipeline.run(concurrency=1) + + +def test_summary_averages_the_judged_scores_and_skips_a_failed_question(tmp_path, monkeypatch): + """Each average is over the questions that have that score; a question whose judge + failed (null conditions, no delta) adds to none of them.""" + result = _run(tmp_path, monkeypatch, [ + _evaluation("q1", 80.5, 50), + _evaluation("q2", 59.5, 40), + _evaluation("q3", None, None), + ]) + + assert result["steps"]["evaluation"]["summary"] == {"with_avg": 70.0, "without_avg": 45.0, "delta_avg": 25.0} + + +def test_summary_is_left_out_when_no_question_was_scored(tmp_path, monkeypatch): + result = _run(tmp_path, monkeypatch, [_evaluation("q1", None, None)]) + + assert "summary" not in result["steps"]["evaluation"] diff --git a/dokimi/tests/test_records_guard.py b/dokimi/tests/test_records_guard.py new file mode 100644 index 00000000..4f48027a --- /dev/null +++ b/dokimi/tests/test_records_guard.py @@ -0,0 +1,184 @@ +# Copyright (C) 2026 Intel Corporation +# SPDX-License-Identifier: Apache-2.0 + +"""Answer and eval records are read through ``dokimi.records``, not by their persisted keys. + +``dokimi/records.py`` is the one reader of the ``with_docs`` and ``without_docs`` +conditions of an answers, eval or arms record (see ``AGENTS.md``). This test scans +``dokimi/`` with ``ast`` for a read of either key outside it: a ``x["with_docs"]`` +subscript or a ``x.get("with_docs")`` call. A store (``x["with_docs"] = ...``), a +delete and a dict-literal key are writes and are not reads. + +Each read that may stay is in :data:`READ_BASELINE`, keyed by module and enclosing +function, with the reason it is not a read of a persisted record or why it has not +moved. A new read fails; a baseline entry whose read is gone fails as stale, so the +baseline only shrinks. Never add an entry to make a new read pass: read the record +through ``dokimi.records`` (``records()``/``record()``/``load()``, then ``Record.score``, +``Record.answer`` and the ``Score``/``Answer`` fields). + +Not covered: a read through the constants (``rec[records.CONTEXT]``), a ``"with_docs" in x`` +test, and ``.raw`` reads of record fields (``rec.raw.get("question_text")``). The last cannot +be told from the other ``.raw`` attributes by syntax: ``Answer.raw`` and ``Score.raw`` are +condition payloads, and ``Task.raw`` of ``dokimi.repos`` is a task file, not a record. +""" + +from __future__ import annotations + +import ast +from collections.abc import Iterable, Mapping +from pathlib import Path + +PROJECT = Path(__file__).resolve().parents[1] +PACKAGE = PROJECT / "dokimi" +READER = "dokimi.records" + +#: The persisted condition keys of a binary answers or eval record. +KEYS = frozenset({"with_docs", "without_docs"}) + +ReadSite = tuple[str, str] + +#: ``(module, enclosing function) -> reason`` for each read that may stay. Only shrinks. +READ_BASELINE: dict[ReadSite, str] = { + ("dokimi.eval.answerer", "Answerer.generate_answers"): + "a writer counting the answers it just built, before they are persisted", + ("dokimi.eval.question_text", "has_answer_to_judge"): + "takes the persisted record dict its judges pass as Record.raw; moving it onto Record " + "changes every judge's call", + ("dokimi.report.generator", "ReportGenerator._format_markdown"): + "reads the statistics dict the generator computed from the evaluations, not a record", + ("dokimi.report.model_compare", "pool_replicate_scores"): + "reads the per-question score rows extract_scores built, which use the binary score names", + ("dokimi.report.model_compare", "generate_section_report"): + "reads the per-question score rows extract_scores built, which use the binary score names", +} + + +def _enclosing_names(tree: ast.AST) -> dict[ast.AST, str]: + """Each node's innermost enclosing function or class qualname (```` at top level).""" + names: dict[ast.AST, str] = {} + + def visit(node: ast.AST, scope: tuple[str, ...]) -> None: + for child in ast.iter_child_nodes(node): + inner = scope + (child.name,) if isinstance( + child, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)) else scope + names[child] = ".".join(scope) or "" + visit(child, inner) + + visit(tree, ()) + return names + + +def _is_key(node: ast.AST) -> bool: + return isinstance(node, ast.Constant) and node.value in KEYS + + +def key_reads(source: str) -> list[tuple[ReadSite, int]]: + """``((, ), line)`` of each ``x["with_docs"]`` load and ``x.get("with_docs")``.""" + tree = ast.parse(source) + scopes = _enclosing_names(tree) + found = [] + for node in ast.walk(tree): + subscript_load = (isinstance(node, ast.Subscript) and isinstance(node.ctx, ast.Load) + and _is_key(node.slice)) + get_call = (isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute) + and node.func.attr == "get" and bool(node.args) and _is_key(node.args[0])) + if subscript_load or get_call: + found.append((scopes.get(node, ""), node.lineno)) + return found + + +def package_reads(package: Path = PACKAGE, reader: str = READER) -> dict[ReadSite, list[int]]: + """Every read of a condition key outside ``reader``: ``(module, scope) -> lines``.""" + found: dict[ReadSite, list[int]] = {} + for path in sorted(package.rglob("*.py")): + rel = path.relative_to(package.parent).with_suffix("") + module = ".".join(rel.parts[:-1] if rel.name == "__init__" else rel.parts) + if module == reader: + continue + for scope, line in key_reads(path.read_text(encoding="utf-8")): + found.setdefault((module, scope), []).append(line) + return {site: sorted(lines) for site, lines in found.items()} + + +def stale_entries(baseline: Iterable[ReadSite], found: Iterable[ReadSite]) -> list[ReadSite]: + """Baseline entries that no longer match a read: they must be deleted.""" + return sorted(set(baseline) - set(found)) + + +def new_reads(baseline: Mapping[ReadSite, str], found: Mapping[ReadSite, list[int]]) -> list[str]: + return sorted( + f"{module}:{lines} reads a record's with_docs/without_docs key in {scope}; " + f"read the record through {READER} (records()/record(), Record.score)" + for (module, scope), lines in found.items() if (module, scope) not in baseline) + + +# --- the package ---------------------------------------------------------------- + + +def test_records_are_read_through_the_records_module(): + found = package_reads() + assert found, f"the scan found no read under {PACKAGE}: it would pass every check" + problems = new_reads(READ_BASELINE, found) + assert not problems, "\n".join(problems) + + +def test_a_read_that_moved_leaves_the_baseline(): + stale = stale_entries(READ_BASELINE, package_reads()) + assert not stale, f"{stale} no longer read a condition key; delete them from READ_BASELINE" + + +def test_every_baseline_entry_has_a_reason_and_names_a_real_module(): + assert all(reason.strip() for reason in READ_BASELINE.values()), "every entry carries its reason" + for module, _ in READ_BASELINE: + assert (PACKAGE.parent / (module.replace(".", "/") + ".py")).is_file(), f"{module} names no module" + + +# --- the scanner finds what it should ------------------------------------------- + + +def test_a_subscript_load_and_a_get_are_reads_in_their_function(): + source = ( + "def total(rows):\n" + " return [r['with_docs'] for r in rows if r.get('without_docs')]\n" + "class Report:\n" + " def line(self, r):\n" + " return r.get(\"with_docs\", {})\n" + ) + assert sorted(key_reads(source)) == [("Report.line", 5), ("total", 2), ("total", 2)] + + +def test_a_store_a_delete_and_a_dict_literal_are_writes_not_reads(): + source = ( + "def write(r):\n" + " r['with_docs'] = {'answer': 'a'}\n" + " del r['without_docs']\n" + " return {'with_docs': None, 'without_docs': None}\n" + ) + assert key_reads(source) == [] + + +def test_other_keys_and_non_literal_keys_are_not_reads(): + source = "def read(r, key):\n return r['aggregate'], r.get('delta'), r[key], r.get(key)\n" + assert key_reads(source) == [] + + +def test_the_reader_module_is_exempt_and_a_package_read_is_found(tmp_path): + package = tmp_path / "pkg" + (package / "sub").mkdir(parents=True) + (package / "records.py").write_text("def f(r):\n return r.get('with_docs')\n") + (package / "sub" / "__init__.py").write_text("def g(r):\n return r['without_docs']\n") + (package / "sub" / "mod.py").write_text("X = 1\n") + assert package_reads(package, reader="pkg.records") == {("pkg.sub", "g"): [2]} + + +def test_a_read_missing_from_the_baseline_is_reported_with_the_fix(): + found = {("pkg.a", "f"): [3], ("pkg.b", "g"): [9]} + problems = new_reads({("pkg.a", "f"): "a writer"}, found) + assert len(problems) == 1 and problems[0].startswith("pkg.b:[9] ") + assert "read the record through dokimi.records" in problems[0] + + +def test_a_stale_baseline_entry_is_reported(): + baseline = {("a", "f"): "x", ("b", "g"): "y"} + assert stale_entries(baseline, {("b", "g"): [1]}) == [("a", "f")] + assert stale_entries({}, {("b", "g"): [1]}) == [] From c6a3987e44752504224aa5423b9a72dd7ae78193 Mon Sep 17 00:00:00 2001 From: Anton Volkov Date: Fri, 23 Oct 2026 18:46:04 +0200 Subject: [PATCH 2/4] test(records): the guard counts reads per baselined function Review of #203 (vchamarthi, exai-bot): the baseline was keyed by (module, function) only, so a new `row.get("without_docs")` inside one of the five listed functions passed, and the stale check could not see a function that lost some of its reads. Each entry now holds the number of reads there (2, 2, 6, 2 and 15 today). A read beyond it fails as new; fewer reads than it counts fail as stale with the count to lower to. Signed-off-by: Anton Volkov --- dokimi/tests/test_records_guard.py | 83 +++++++++++++++++++----------- 1 file changed, 53 insertions(+), 30 deletions(-) diff --git a/dokimi/tests/test_records_guard.py b/dokimi/tests/test_records_guard.py index 4f48027a..8ff82648 100644 --- a/dokimi/tests/test_records_guard.py +++ b/dokimi/tests/test_records_guard.py @@ -10,9 +10,10 @@ delete and a dict-literal key are writes and are not reads. Each read that may stay is in :data:`READ_BASELINE`, keyed by module and enclosing -function, with the reason it is not a read of a persisted record or why it has not -moved. A new read fails; a baseline entry whose read is gone fails as stale, so the -baseline only shrinks. Never add an entry to make a new read pass: read the record +function, with the number of reads there and the reason they are not reads of a persisted +record or why they have not moved. A read beyond that number fails, in a function the +baseline lists as anywhere else; an entry that counts more reads than are left fails as +stale, so the baseline only shrinks. Never add an entry to make a new read pass: read the record through ``dokimi.records`` (``records()``/``record()``/``load()``, then ``Record.score``, ``Record.answer`` and the ``Score``/``Answer`` fields). @@ -25,7 +26,7 @@ from __future__ import annotations import ast -from collections.abc import Iterable, Mapping +from collections.abc import Mapping from pathlib import Path PROJECT = Path(__file__).resolve().parents[1] @@ -37,19 +38,19 @@ ReadSite = tuple[str, str] -#: ``(module, enclosing function) -> reason`` for each read that may stay. Only shrinks. -READ_BASELINE: dict[ReadSite, str] = { - ("dokimi.eval.answerer", "Answerer.generate_answers"): - "a writer counting the answers it just built, before they are persisted", - ("dokimi.eval.question_text", "has_answer_to_judge"): +#: ``(module, enclosing function) -> (reads, reason)`` for the reads that may stay. Only shrinks. +READ_BASELINE: dict[ReadSite, tuple[int, str]] = { + ("dokimi.eval.answerer", "Answerer.generate_answers"): (2, + "a writer counting the answers it just built, before they are persisted"), + ("dokimi.eval.question_text", "has_answer_to_judge"): (2, "takes the persisted record dict its judges pass as Record.raw; moving it onto Record " - "changes every judge's call", - ("dokimi.report.generator", "ReportGenerator._format_markdown"): - "reads the statistics dict the generator computed from the evaluations, not a record", - ("dokimi.report.model_compare", "pool_replicate_scores"): - "reads the per-question score rows extract_scores built, which use the binary score names", - ("dokimi.report.model_compare", "generate_section_report"): - "reads the per-question score rows extract_scores built, which use the binary score names", + "changes every judge's call"), + ("dokimi.report.generator", "ReportGenerator._format_markdown"): (6, + "reads the statistics dict the generator computed from the evaluations, not a record"), + ("dokimi.report.model_compare", "pool_replicate_scores"): (2, + "reads the per-question score rows extract_scores built, which use the binary score names"), + ("dokimi.report.model_compare", "generate_section_report"): (15, + "reads the per-question score rows extract_scores built, which use the binary score names"), } @@ -100,16 +101,28 @@ def package_reads(package: Path = PACKAGE, reader: str = READER) -> dict[ReadSit return {site: sorted(lines) for site, lines in found.items()} -def stale_entries(baseline: Iterable[ReadSite], found: Iterable[ReadSite]) -> list[ReadSite]: - """Baseline entries that no longer match a read: they must be deleted.""" - return sorted(set(baseline) - set(found)) +def stale_entries(baseline: Mapping[ReadSite, tuple[int, str]], + found: Mapping[ReadSite, list[int]]) -> list[str]: + """Baseline entries that count more reads than are left: lower the count, or delete the entry.""" + stale = [] + for (module, scope), (allowed, _) in sorted(baseline.items()): + left = len(found.get((module, scope), [])) + if left < allowed: + fix = f"lower its count to {left}" if left else "delete the entry" + stale.append(f"{module}:{scope} has {left} of its {allowed} baselined reads left; {fix}") + return stale -def new_reads(baseline: Mapping[ReadSite, str], found: Mapping[ReadSite, list[int]]) -> list[str]: - return sorted( - f"{module}:{lines} reads a record's with_docs/without_docs key in {scope}; " - f"read the record through {READER} (records()/record(), Record.score)" - for (module, scope), lines in found.items() if (module, scope) not in baseline) +def new_reads(baseline: Mapping[ReadSite, tuple[int, str]], found: Mapping[ReadSite, list[int]]) -> list[str]: + """Sites with more reads than the baseline allows (none, for a site it does not list).""" + problems = [] + for (module, scope), lines in sorted(found.items()): + allowed = baseline.get((module, scope), (0, ""))[0] + if len(lines) > allowed: + over = f" ({len(lines)} reads, the baseline allows {allowed})" if allowed else "" + problems.append(f"{module}:{lines} reads a record's with_docs/without_docs key in {scope}{over}; " + f"read the record through {READER} (records()/record(), Record.score)") + return problems # --- the package ---------------------------------------------------------------- @@ -124,11 +137,12 @@ def test_records_are_read_through_the_records_module(): def test_a_read_that_moved_leaves_the_baseline(): stale = stale_entries(READ_BASELINE, package_reads()) - assert not stale, f"{stale} no longer read a condition key; delete them from READ_BASELINE" + assert not stale, "READ_BASELINE counts reads that are gone:\n" + "\n".join(stale) def test_every_baseline_entry_has_a_reason_and_names_a_real_module(): - assert all(reason.strip() for reason in READ_BASELINE.values()), "every entry carries its reason" + assert all(count >= 1 and reason.strip() for count, reason in READ_BASELINE.values()), ( + "every entry counts at least one read and carries its reason") for module, _ in READ_BASELINE: assert (PACKAGE.parent / (module.replace(".", "/") + ".py")).is_file(), f"{module} names no module" @@ -173,12 +187,21 @@ def test_the_reader_module_is_exempt_and_a_package_read_is_found(tmp_path): def test_a_read_missing_from_the_baseline_is_reported_with_the_fix(): found = {("pkg.a", "f"): [3], ("pkg.b", "g"): [9]} - problems = new_reads({("pkg.a", "f"): "a writer"}, found) + problems = new_reads({("pkg.a", "f"): (1, "a writer")}, found) assert len(problems) == 1 and problems[0].startswith("pkg.b:[9] ") assert "read the record through dokimi.records" in problems[0] -def test_a_stale_baseline_entry_is_reported(): - baseline = {("a", "f"): "x", ("b", "g"): "y"} - assert stale_entries(baseline, {("b", "g"): [1]}) == [("a", "f")] +def test_a_new_read_in_a_function_the_baseline_lists_is_reported(): + problems = new_reads({("pkg.a", "f"): (1, "a writer")}, {("pkg.a", "f"): [3, 7]}) + assert len(problems) == 1 and "pkg.a:[3, 7] " in problems[0] + assert "(2 reads, the baseline allows 1)" in problems[0] + + +def test_a_baseline_entry_counting_reads_that_are_gone_is_reported(): + baseline = {("a", "f"): (1, "x"), ("b", "g"): (2, "y"), ("c", "h"): (1, "z")} + assert stale_entries(baseline, {("b", "g"): [1], ("c", "h"): [4]}) == [ + "a:f has 0 of its 1 baselined reads left; delete the entry", + "b:g has 1 of its 2 baselined reads left; lower its count to 1", + ] assert stale_entries({}, {("b", "g"): [1]}) == [] From 1b1e718ef26bfb58a1bb7d66c05b2d3190cad225 Mon Sep 17 00:00:00 2001 From: Anton Volkov Date: Fri, 23 Oct 2026 18:47:09 +0200 Subject: [PATCH 3/4] test(records): the guard also counts .pop and .setdefault of a condition key Review of #203 (vchamarthi): `x.pop("with_docs")` and `x.setdefault("with_docs", ...)` read the key as `get` does; the scanner matched only `get`. No such read exists in the package today. Signed-off-by: Anton Volkov --- dokimi/tests/test_records_guard.py | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/dokimi/tests/test_records_guard.py b/dokimi/tests/test_records_guard.py index 8ff82648..b74c6786 100644 --- a/dokimi/tests/test_records_guard.py +++ b/dokimi/tests/test_records_guard.py @@ -6,8 +6,8 @@ ``dokimi/records.py`` is the one reader of the ``with_docs`` and ``without_docs`` conditions of an answers, eval or arms record (see ``AGENTS.md``). This test scans ``dokimi/`` with ``ast`` for a read of either key outside it: a ``x["with_docs"]`` -subscript or a ``x.get("with_docs")`` call. A store (``x["with_docs"] = ...``), a -delete and a dict-literal key are writes and are not reads. +subscript, or a ``x.get("with_docs")``, ``.pop`` or ``.setdefault`` call. A store +(``x["with_docs"] = ...``), a delete and a dict-literal key are writes and are not reads. Each read that may stay is in :data:`READ_BASELINE`, keyed by module and enclosing function, with the number of reads there and the reason they are not reads of a persisted @@ -36,6 +36,9 @@ #: The persisted condition keys of a binary answers or eval record. KEYS = frozenset({"with_docs", "without_docs"}) +#: Dict methods that read the value of the key they are given. +READ_METHODS = frozenset({"get", "pop", "setdefault"}) + ReadSite = tuple[str, str] #: ``(module, enclosing function) -> (reads, reason)`` for the reads that may stay. Only shrinks. @@ -74,7 +77,7 @@ def _is_key(node: ast.AST) -> bool: def key_reads(source: str) -> list[tuple[ReadSite, int]]: - """``((, ), line)`` of each ``x["with_docs"]`` load and ``x.get("with_docs")``.""" + """``(, line)`` of each ``x["with_docs"]`` load and ``x.get/pop/setdefault("with_docs")``.""" tree = ast.parse(source) scopes = _enclosing_names(tree) found = [] @@ -82,7 +85,7 @@ def key_reads(source: str) -> list[tuple[ReadSite, int]]: subscript_load = (isinstance(node, ast.Subscript) and isinstance(node.ctx, ast.Load) and _is_key(node.slice)) get_call = (isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute) - and node.func.attr == "get" and bool(node.args) and _is_key(node.args[0])) + and node.func.attr in READ_METHODS and bool(node.args) and _is_key(node.args[0])) if subscript_load or get_call: found.append((scopes.get(node, ""), node.lineno)) return found @@ -150,15 +153,17 @@ def test_every_baseline_entry_has_a_reason_and_names_a_real_module(): # --- the scanner finds what it should ------------------------------------------- -def test_a_subscript_load_and_a_get_are_reads_in_their_function(): +def test_a_subscript_load_get_pop_and_setdefault_are_reads_in_their_function(): source = ( "def total(rows):\n" " return [r['with_docs'] for r in rows if r.get('without_docs')]\n" "class Report:\n" " def line(self, r):\n" " return r.get(\"with_docs\", {})\n" + "def take(r):\n" + " return r.pop('with_docs'), r.setdefault('without_docs', None)\n" ) - assert sorted(key_reads(source)) == [("Report.line", 5), ("total", 2), ("total", 2)] + assert sorted(key_reads(source)) == [("Report.line", 5), ("take", 7), ("take", 7), ("total", 2), ("total", 2)] def test_a_store_a_delete_and_a_dict_literal_are_writes_not_reads(): From 0b3af098b8ca7e23b4b8366fe6c883e31c51be12 Mon Sep 17 00:00:00 2001 From: Anton Volkov Date: Fri, 23 Oct 2026 21:18:25 +0200 Subject: [PATCH 4/4] test(records): the guard counts reads per module with one pattern, no AST Review of #203 (napetrov): 200 lines of AST scope tracking was a lot to hold one naming rule. A regex over `["with_docs"]` subscripts not assigned to and `.get/.pop/.setdefault("with_docs")` calls finds the same 27 reads on the same lines, and counting them per module keeps both properties the review asked for: a new read fails wherever it is, inside an already-listed module too, since its count goes up; and a count above what is left fails as stale. The baseline is four modules (2, 2, 6, 17). The file goes from 212 to 121 lines. Moving the remaining readers instead would mean changing `report/model_compare.py`, whose 17 reads are on the per-question score rows it builds itself, not on stored records. Signed-off-by: Anton Volkov --- dokimi/tests/test_records_guard.py | 237 +++++++++-------------------- 1 file changed, 73 insertions(+), 164 deletions(-) diff --git a/dokimi/tests/test_records_guard.py b/dokimi/tests/test_records_guard.py index b74c6786..03c8efff 100644 --- a/dokimi/tests/test_records_guard.py +++ b/dokimi/tests/test_records_guard.py @@ -3,182 +3,100 @@ """Answer and eval records are read through ``dokimi.records``, not by their persisted keys. -``dokimi/records.py`` is the one reader of the ``with_docs`` and ``without_docs`` -conditions of an answers, eval or arms record (see ``AGENTS.md``). This test scans -``dokimi/`` with ``ast`` for a read of either key outside it: a ``x["with_docs"]`` -subscript, or a ``x.get("with_docs")``, ``.pop`` or ``.setdefault`` call. A store -(``x["with_docs"] = ...``), a delete and a dict-literal key are writes and are not reads. - -Each read that may stay is in :data:`READ_BASELINE`, keyed by module and enclosing -function, with the number of reads there and the reason they are not reads of a persisted -record or why they have not moved. A read beyond that number fails, in a function the -baseline lists as anywhere else; an entry that counts more reads than are left fails as -stale, so the baseline only shrinks. Never add an entry to make a new read pass: read the record -through ``dokimi.records`` (``records()``/``record()``/``load()``, then ``Record.score``, -``Record.answer`` and the ``Score``/``Answer`` fields). +``dokimi/records.py`` is the one reader of the ``with_docs`` and ``without_docs`` conditions of +an answers, eval or arms record (see ``AGENTS.md``). This test counts, in each module of +``dokimi/`` but that one, the reads of either key: an ``x["with_docs"]`` subscript that is not +assigned to, and an ``x.get("with_docs")``, ``.pop`` or ``.setdefault`` call. A dict-literal key +and an assignment are writes and do not match. + +:data:`READ_BASELINE` holds the modules whose reads may stay, with their count and the reason +they are not reads of a persisted record. More reads than the count fail, so a new read fails +wherever it is; fewer fail as stale, so the baseline only shrinks. Never raise a count to make a +new read pass: read the record through ``dokimi.records``. Not covered: a read through the constants (``rec[records.CONTEXT]``), a ``"with_docs" in x`` -test, and ``.raw`` reads of record fields (``rec.raw.get("question_text")``). The last cannot -be told from the other ``.raw`` attributes by syntax: ``Answer.raw`` and ``Score.raw`` are -condition payloads, and ``Task.raw`` of ``dokimi.repos`` is a task file, not a record. +test, and ``.raw`` reads of record fields, which a pattern cannot tell from other ``.raw`` +attributes (``Task.raw`` of ``dokimi.repos`` is a task file, not a record). """ from __future__ import annotations -import ast +import re from collections.abc import Mapping from pathlib import Path -PROJECT = Path(__file__).resolve().parents[1] -PACKAGE = PROJECT / "dokimi" +PACKAGE = Path(__file__).resolve().parents[1] / "dokimi" READER = "dokimi.records" -#: The persisted condition keys of a binary answers or eval record. -KEYS = frozenset({"with_docs", "without_docs"}) - -#: Dict methods that read the value of the key they are given. -READ_METHODS = frozenset({"get", "pop", "setdefault"}) - -ReadSite = tuple[str, str] - -#: ``(module, enclosing function) -> (reads, reason)`` for the reads that may stay. Only shrinks. -READ_BASELINE: dict[ReadSite, tuple[int, str]] = { - ("dokimi.eval.answerer", "Answerer.generate_answers"): (2, - "a writer counting the answers it just built, before they are persisted"), - ("dokimi.eval.question_text", "has_answer_to_judge"): (2, - "takes the persisted record dict its judges pass as Record.raw; moving it onto Record " - "changes every judge's call"), - ("dokimi.report.generator", "ReportGenerator._format_markdown"): (6, - "reads the statistics dict the generator computed from the evaluations, not a record"), - ("dokimi.report.model_compare", "pool_replicate_scores"): (2, - "reads the per-question score rows extract_scores built, which use the binary score names"), - ("dokimi.report.model_compare", "generate_section_report"): (15, - "reads the per-question score rows extract_scores built, which use the binary score names"), +_KEY = r"""["'](?:with|without)_docs["']""" +#: A subscript of a condition key not followed by ``=`` (``==`` is a read), or a dict method call on one. +READ = re.compile(rf"\[\s*{_KEY}\s*\](?!\s*=[^=])|\.(?:get|pop|setdefault)\(\s*{_KEY}") + +#: ``module -> (reads, reason)`` for the reads that may stay. Only shrinks. +READ_BASELINE: dict[str, tuple[int, str]] = { + "dokimi.eval.answerer": (2, "a writer counting the answers it just built, before they are persisted"), + "dokimi.eval.question_text": ( + 2, "takes the persisted record dict its judges pass as Record.raw; moving it onto Record changes " + "every judge's call"), + "dokimi.report.generator": (6, "reads the statistics dict it computed from the evaluations, not a record"), + "dokimi.report.model_compare": ( + 17, "reads the per-question score rows extract_scores built, which use the binary score names"), } -def _enclosing_names(tree: ast.AST) -> dict[ast.AST, str]: - """Each node's innermost enclosing function or class qualname (```` at top level).""" - names: dict[ast.AST, str] = {} - - def visit(node: ast.AST, scope: tuple[str, ...]) -> None: - for child in ast.iter_child_nodes(node): - inner = scope + (child.name,) if isinstance( - child, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)) else scope - names[child] = ".".join(scope) or "" - visit(child, inner) - - visit(tree, ()) - return names - - -def _is_key(node: ast.AST) -> bool: - return isinstance(node, ast.Constant) and node.value in KEYS - - -def key_reads(source: str) -> list[tuple[ReadSite, int]]: - """``(, line)`` of each ``x["with_docs"]`` load and ``x.get/pop/setdefault("with_docs")``.""" - tree = ast.parse(source) - scopes = _enclosing_names(tree) - found = [] - for node in ast.walk(tree): - subscript_load = (isinstance(node, ast.Subscript) and isinstance(node.ctx, ast.Load) - and _is_key(node.slice)) - get_call = (isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute) - and node.func.attr in READ_METHODS and bool(node.args) and _is_key(node.args[0])) - if subscript_load or get_call: - found.append((scopes.get(node, ""), node.lineno)) - return found +def reads(source: str) -> list[int]: + """The line of each read of a condition key in ``source``.""" + return [source.count("\n", 0, m.start()) + 1 for m in READ.finditer(source)] -def package_reads(package: Path = PACKAGE, reader: str = READER) -> dict[ReadSite, list[int]]: - """Every read of a condition key outside ``reader``: ``(module, scope) -> lines``.""" - found: dict[ReadSite, list[int]] = {} +def package_reads(package: Path = PACKAGE, reader: str = READER) -> dict[str, list[int]]: + """``module -> lines`` of every read of a condition key outside ``reader``.""" + found: dict[str, list[int]] = {} for path in sorted(package.rglob("*.py")): rel = path.relative_to(package.parent).with_suffix("") module = ".".join(rel.parts[:-1] if rel.name == "__init__" else rel.parts) - if module == reader: - continue - for scope, line in key_reads(path.read_text(encoding="utf-8")): - found.setdefault((module, scope), []).append(line) - return {site: sorted(lines) for site, lines in found.items()} - - -def stale_entries(baseline: Mapping[ReadSite, tuple[int, str]], - found: Mapping[ReadSite, list[int]]) -> list[str]: - """Baseline entries that count more reads than are left: lower the count, or delete the entry.""" - stale = [] - for (module, scope), (allowed, _) in sorted(baseline.items()): - left = len(found.get((module, scope), [])) - if left < allowed: - fix = f"lower its count to {left}" if left else "delete the entry" - stale.append(f"{module}:{scope} has {left} of its {allowed} baselined reads left; {fix}") - return stale - - -def new_reads(baseline: Mapping[ReadSite, tuple[int, str]], found: Mapping[ReadSite, list[int]]) -> list[str]: - """Sites with more reads than the baseline allows (none, for a site it does not list).""" - problems = [] - for (module, scope), lines in sorted(found.items()): - allowed = baseline.get((module, scope), (0, ""))[0] - if len(lines) > allowed: - over = f" ({len(lines)} reads, the baseline allows {allowed})" if allowed else "" - problems.append(f"{module}:{lines} reads a record's with_docs/without_docs key in {scope}{over}; " - f"read the record through {READER} (records()/record(), Record.score)") - return problems + if module != reader and (lines := reads(path.read_text(encoding="utf-8"))): + found[module] = lines + return found -# --- the package ---------------------------------------------------------------- +def problems(baseline: Mapping[str, tuple[int, str]], found: Mapping[str, list[int]]) -> list[str]: + """A module with more reads than its baseline allows (0 if not listed), or fewer than it counts.""" + out = [] + for module in sorted(set(baseline) | set(found)): + lines, allowed = found.get(module, []), baseline.get(module, (0, ""))[0] + if len(lines) > allowed: + out.append(f"{module}:{lines} reads a record's with_docs/without_docs key ({len(lines)} reads, " + f"{allowed} allowed); read the record through {READER} (records()/record(), Record.score)") + elif len(lines) < allowed: + fix = f"lower its count to {len(lines)}" if lines else "delete its entry" + out.append(f"{module} has {len(lines)} of its {allowed} baselined reads left; {fix} in READ_BASELINE") + return out def test_records_are_read_through_the_records_module(): found = package_reads() assert found, f"the scan found no read under {PACKAGE}: it would pass every check" - problems = new_reads(READ_BASELINE, found) - assert not problems, "\n".join(problems) - - -def test_a_read_that_moved_leaves_the_baseline(): - stale = stale_entries(READ_BASELINE, package_reads()) - assert not stale, "READ_BASELINE counts reads that are gone:\n" + "\n".join(stale) + found_problems = problems(READ_BASELINE, found) + assert not found_problems, "\n".join(found_problems) def test_every_baseline_entry_has_a_reason_and_names_a_real_module(): - assert all(count >= 1 and reason.strip() for count, reason in READ_BASELINE.values()), ( - "every entry counts at least one read and carries its reason") - for module, _ in READ_BASELINE: + for module, (count, reason) in READ_BASELINE.items(): + assert count >= 1 and reason.strip(), module assert (PACKAGE.parent / (module.replace(".", "/") + ".py")).is_file(), f"{module} names no module" -# --- the scanner finds what it should ------------------------------------------- - - -def test_a_subscript_load_get_pop_and_setdefault_are_reads_in_their_function(): - source = ( - "def total(rows):\n" - " return [r['with_docs'] for r in rows if r.get('without_docs')]\n" - "class Report:\n" - " def line(self, r):\n" - " return r.get(\"with_docs\", {})\n" - "def take(r):\n" - " return r.pop('with_docs'), r.setdefault('without_docs', None)\n" - ) - assert sorted(key_reads(source)) == [("Report.line", 5), ("take", 7), ("take", 7), ("total", 2), ("total", 2)] +def test_subscript_loads_and_dict_method_calls_are_reads(): + source = ('a = r["with_docs"]["aggregate"]\nb = r.get("without_docs")\nc = r.pop(\'with_docs\')\n' + 'd = r.setdefault("without_docs", {})\nif r[\n "with_docs"\n] == x:\n pass\n') + assert reads(source) == [1, 2, 3, 4, 5] -def test_a_store_a_delete_and_a_dict_literal_are_writes_not_reads(): - source = ( - "def write(r):\n" - " r['with_docs'] = {'answer': 'a'}\n" - " del r['without_docs']\n" - " return {'with_docs': None, 'without_docs': None}\n" - ) - assert key_reads(source) == [] - - -def test_other_keys_and_non_literal_keys_are_not_reads(): - source = "def read(r, key):\n return r['aggregate'], r.get('delta'), r[key], r.get(key)\n" - assert key_reads(source) == [] +def test_assignments_dict_literals_and_other_keys_are_not_reads(): + source = ('r["with_docs"] = {"answer": "a"}\nrow = {"with_docs": None, "without_docs": None}\n' + 'r["aggregate"], r.get("delta"), r.get(key), r[key]\n') + assert reads(source) == [] def test_the_reader_module_is_exempt_and_a_package_read_is_found(tmp_path): @@ -187,26 +105,17 @@ def test_the_reader_module_is_exempt_and_a_package_read_is_found(tmp_path): (package / "records.py").write_text("def f(r):\n return r.get('with_docs')\n") (package / "sub" / "__init__.py").write_text("def g(r):\n return r['without_docs']\n") (package / "sub" / "mod.py").write_text("X = 1\n") - assert package_reads(package, reader="pkg.records") == {("pkg.sub", "g"): [2]} - - -def test_a_read_missing_from_the_baseline_is_reported_with_the_fix(): - found = {("pkg.a", "f"): [3], ("pkg.b", "g"): [9]} - problems = new_reads({("pkg.a", "f"): (1, "a writer")}, found) - assert len(problems) == 1 and problems[0].startswith("pkg.b:[9] ") - assert "read the record through dokimi.records" in problems[0] - - -def test_a_new_read_in_a_function_the_baseline_lists_is_reported(): - problems = new_reads({("pkg.a", "f"): (1, "a writer")}, {("pkg.a", "f"): [3, 7]}) - assert len(problems) == 1 and "pkg.a:[3, 7] " in problems[0] - assert "(2 reads, the baseline allows 1)" in problems[0] - - -def test_a_baseline_entry_counting_reads_that_are_gone_is_reported(): - baseline = {("a", "f"): (1, "x"), ("b", "g"): (2, "y"), ("c", "h"): (1, "z")} - assert stale_entries(baseline, {("b", "g"): [1], ("c", "h"): [4]}) == [ - "a:f has 0 of its 1 baselined reads left; delete the entry", - "b:g has 1 of its 2 baselined reads left; lower its count to 1", + assert package_reads(package, reader="pkg.records") == {"pkg.sub": [2]} + + +def test_a_new_read_an_extra_read_and_a_stale_count_are_reported(): + baseline = {"pkg.a": (1, "x"), "pkg.b": (2, "y"), "pkg.c": (1, "z")} + found = {"pkg.a": [3, 7], "pkg.b": [1], "pkg.new": [9]} + assert problems(baseline, found) == [ + "pkg.a:[3, 7] reads a record's with_docs/without_docs key (2 reads, 1 allowed); " + "read the record through dokimi.records (records()/record(), Record.score)", + "pkg.b has 1 of its 2 baselined reads left; lower its count to 1 in READ_BASELINE", + "pkg.c has 0 of its 1 baselined reads left; delete its entry in READ_BASELINE", + "pkg.new:[9] reads a record's with_docs/without_docs key (1 reads, 0 allowed); " + "read the record through dokimi.records (records()/record(), Record.score)", ] - assert stale_entries({}, {("b", "g"): [1]}) == []