You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
refactor(eval): give answer and eval records one reader (eval/records.py) - #80
Readers of answers.v1 / eval.v1 / arms.v1 records each picked their own keys. The judge, panel, reports, gates and judge-reliability readers listed below now go through one module, eval/records.py. It is the only code that knows the binary shape (with_docs/without_docs) and the arm shape (arms/scores, legacy inline evaluation.aggregate_score):
dokimi/eval/
+├── records.py # read(data) / load(path) -> Run(shape, conditions, baseline, meta, records)+│ # Record: question_text, ground_truth, expected_skills, retrieval_hit,+│ # answer(cond) -> Answer(text, docs, context), score(cond) -> aggregate | None+│ # arm_answers / arm_scores: only `arms` / `scores` (the arm judges read these)+│ # RecordError(ValueError): names the file and the record
├── judge.py - answer["with_docs"].get("retrieved_docs") + rec.answer(CONTEXT).context
├── panel.py - answer.get("question_text") or .get("question") or .get("text")
├── diagnoser.py - retrieval_hit(answer dict) (moved into records)
├── ragas_eval.py, grounding.py, degrade.py, pairwise.py, checklist.py, cross_judge.py
├── arm_runner.py, arm_summary.py, plugin_delta.py, stats.py, interactions.py
gate/trust_gate.py
report/generator.py, eval_report.py, model_compare.py, arms_report.py
dashboard/aggregator.py
commands/evaluate.py, judge_reliability.py, baseline.py, questions.py, orchestrator/pipeline.py
That is about 40 reader sites in 24 files. Some are easy to miss: the judge's console summary, the report eval Q&A sidecar, the dashboard, baseline save/compare, stats.paired_vectors/arm_significance, the interaction contrasts, the arm runner's judge_context source count, and the answers branch of questions panel-review. A few raw-key reads remain and give the same result today: orchestrator/pipeline.py's step summary, judge_context.py's chunk text, and the raw metadata in model_compare.py and eval_report.py. AGENTS.md makes records.py the rule for new readers.
Values are read as written. An id stays the str/int it was (0 is an id) and an aggregate stays the int/float it was, so reports print what they printed before. The persisted names with_docs/without_docs do not change. The arm judges (eval pairwise, checklist, cross-judge) still read only arms answers and evaluations[].scores, as on main. The averages are untouched: reconciling them is the eval/compare.py PR.
Evidence
No number moves on well-formed files. I ran every migrated reader on main and on this branch with the same fixtures. The eval.v1 file has int and string ids, an id of 0, int and float aggregates, one question scored in one arm only, an errored question, panel agreement and a stamped retrieval_hit. There is also its answers.v1 file and three arms.v1 runs (current, a second model, legacy inline scores). Outputs compared: report generate (md and json), report eval (md and Q&A sidecar), the trust gate, the dashboard snapshot, the baseline summary, grounding, model-compare, plugin delta, cross-judge, pairwise, checklist, arm summaries, the degradation pool, and the judge/panel/arm runner outputs and prompts with a stubbed LLM. diff -r before after is empty. Both reviewers' differential runs agree.
Readers that read the wrong key (tests/test_record_readers.py). On main this file stops at collection because it imports dokimi.eval.records. With that module present, each test fails on main's reader as shown and passes now:
AttributeError: 'NoneType' object has no attribute 'get'
Review fixes. Each test fails on the commit before its fix:
baseline save wrote the file and then crashed on an id-less record: test_baseline_save_refuses_a_malformed_eval_file_before_writing.
One bad answers record blanked the report eval sidecar: test_report_eval_sidecar_drops_only_the_malformed_answers_record.
cross-judge raised on unknown_q ids: test_stored_scores_read_an_arms_run_with_the_fallback_question_id.
An unscored arm was averaged as 0: test_baseline_summary_leaves_out_a_question_with_an_unscored_arm / …_unscored_baseline.
Unscored arms broke the report's top/bottom tables: test_report_top_deltas_leave_out_a_bool_aggregate, test_report_top_and_bottom_deltas_are_ordered.
A stray scores: null / arms: null hid a binary record's conditions: test_a_binary_record_with_a_stray_null_*.
The arm judges paid for binary files and took legacy inline scores as the stored judge:
test_the_arm_judges_refuse_a_binary_answers_file_before_any_call[pairwise|checklist|cross-judge]
at 6f1962b: with_docs/without_docs judged as two arms, exit 0
now: exit 2 naming the file, 0 judge calls, no output written
test_cross_judge_on_a_legacy_inline_score_run_has_nothing_to_compare
at 6f1962b: exit 0, inline scores taken as the stored judge
now: exit 2 "nothing to compare", as on main
Also test_the_arm_judges_send_no_binary_answer_to_a_judge and test_cross_judge_takes_no_stored_measure_from_*.
The reader itself.tests/test_records.py covers the binary, arms and legacy shapes, ids and values as written, and every RecordError. It also pins join precedence: the evaluation's score over an inline one, metadata when both sides differ, conditions and raw order, eval-only questions, and the stamped retrieval_hit. It pins question_id over id too. Every join mutant from the review now fails the suite, and so do the _top_deltas order, baseline filter and arms: null mutants. records.py passes mypy --strict.
Checks. The full suite passes on two random seeds (31337, 2718): 5250 passed, 2 skipped. make ci-local CI_LOCAL_ARGS=--offline gives 0 failed, 14 passed, 7 skipped (network/docker). Changed-line coverage is 97% and records.py is at 100%. schema_compat is OK: the only schema change is the retrieval_hit description, which pointed at the removed diagnoser.retrieval_hit. The changelog fragment check passes.
Merge Danger
Door: two-way. No persisted format, schema version or flag changes, and no score changes for well-formed files, so a revert restores today's readers.
Blast Radius: malformed-input
A malformed record is an error that names it (answers.json answers[3]: missing question_id). This covers a missing id, an id that is neither a string nor an integer (question_id must be a string or an integer, got 1.5), a condition/arm payload that is neither an object nor null, and a bad expected_skill. Before, such a record was skipped, given a made-up id, or crashed a paid run part-way. eval score and arms run check before their first call. report generate/eval, model-compare and plugin-delta exit 1 naming the file. baseline save writes nothing. The report eval sidecar drops only the bad record. The dashboard leaves the file out with a warning.
question_id wins over id when a hand-merged record carries both (utils.question_id preferred id).
A non-numeric aggregate ("80", a bool) is unscored everywhere. The dashboard used to parse "80". baseline save and the report's top/bottom tables leave out a question with an unscored arm.
baseline compare matches ids as strings: an int 1 and a string "1" are the same question.
eval cross-judge --judges on a binary answers file exits 2 before any call. It used to print an empty table. pairwise and checklist already exited 2 on such a file.
Signatures:diagnose(context_answer, delta) takes the context Answer and the delta. diagnoser.retrieval_hit moved to Record.retrieval_hit. The private stats._aggregate_of and plugin_delta._scores_by_question are gone. A patch on Judge._evaluate_answer_pair receives a Record.
Busy files for the next wave-4 PR (eval/compare.py): report/*, eval/stats.py, gate/trust_gate.py.
Checklist
Tests added/updated and pytest passes
ruff, mypy, and schema checks pass
Docs updated (or N/A) — see the docs-drift checklist in CONTRIBUTING.md
No ad-hoc run artifacts committed
New files carry the SPDX license header (or are exempt in .github/.licenserc.yaml)
….py)
No module owned the answers.v1 / eval.v1 / arms.v1 record shape, so every
consumer picked its own keys, and some picked keys no writer writes.
eval/records.py is now the one reader: read()/load() take any artifact
(wrapped or bare list, binary or N-arm, arms.v1 answers and evaluations
joined by question id, the legacy inline evaluation.aggregate_score) and
return a Run (shape, conditions, baseline, meta) of Records: question text,
ground truth, expected_skills, per-condition Answer (text, docs, context)
and Score (aggregate, agreement), retrieval_hit (an evaluation's stamped
value wins over what an answer implies). Values are read, not normalised:
an id stays the str or int it was written as, an aggregate the int or float.
Malformed input is one RecordError (a ValueError) naming the file and the
record. Persisted formats are unchanged; with_docs/without_docs stay the
binary condition names.
Readers migrated: judge (and its summary), panel, RAGAS, grounding,
diagnoser, trust gate, report generate, report eval (and its Q&A sidecar),
dashboard aggregator, baseline save/compare, model-compare (both formats),
arms report, plugin delta, cross-judge, pairwise, checklist, degradation
pool, arm runner judge/summary/judge_context sources, arm summaries
(agentic usage, skill selection, cost), stats.paired_vectors /
arm_significance, interaction contrasts, the answers loaders of eval
score / panel-score / ragas / grounding / judge-validity, judge_reliability
and the pipeline, and questions panel-review's answers branch.
No published number moves: a before/after run of every migrated reader on
fixture eval.v1, answers.v1 and arms.v1 files (report generate md+json,
report eval md+sidecar, trust gate, dashboard, model-compare, plugin delta,
arm summaries, judge and panel outputs with a stubbed LLM, ...) is
byte-identical.
Deliberate behaviour change: a record with no question_id (or a payload
that is neither an object nor null, or an expected_skill that is not a
name, list of names or null) is a RecordError instead of being skipped,
renamed or crashing later; the judge checks every record, question text
included, before its first call.
Fixed (wrong keys or values read):
- report generate / report eval read question_text only; the panel writes
`question`, so a panel run's tables were blank.
- report generate summed a panel aggregate of null (every vote failed)
and raised TypeError.
- questions panel-review read question/text from an answers file, whose
records carry question_text, and reviewed nothing.
- model-compare's legacy path called .get on a null arm payload.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
❌ Patch coverage is 96.74355% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.71%. Comparing base (6c6d5b9) to head (c06126a).
The reason will be displayed to describe this comment to others. Learn more.
Review at 2fe185b. The reader is sound and the migration matches main on well-formed files: I diffed report, gate, baseline, grounding, ragas, cross-judge and arm outputs against main on the same fixtures and found no drift. The problems are in the new strictness. Two commands now lose data or leave state behind on an id-less record, and the reader's join is almost untested. I'd fix the two bugs and the cross-judge crash before merge.
make ci-local: 19 passed, 0 failed, 4 skipped (docker/network/semgrep, job conditions). 4,713 tests, changed-line coverage 97%. Python 3.13, so the matrix legs on 3.11/3.12/3.14 did not run.
Differential runs of main and the PR head on the same fixtures (ids of 0, int and string ids, null arms, legacy inline scores, duplicate and missing ids), with the LLM transport mocked, for the reproductions below.
A mutation run on records.py: 119 mutants against test_records.py and test_record_readers.py, 73 survive; three of the join ones also pass the full suite (see the inline comment).
schema_compat vs the base: OK (the retrieval_hit description change is allowed in place). The changelog fragment name, categories and headers are valid. ruff, mypy and mypy --strict on records.py are clean.
Beyond the inline comments
The "every reader goes through records.py" claim in the PR text and AGENTS.md is too strong. A few sites still pick their own keys: orchestrator/pipeline.py:304 reads e["with_docs"]["aggregate"], eval/judge_context.py:227 reads context_chunks/snippet from answer.raw, and report/model_compare.py:567 and report/eval_report.py:444 read rec.raw.get("question_text") and friends instead of the Record fields. None of them gives a wrong result today. Nothing enforces the rule for the next reader, though, so a test that greps dokimi/ for these keys outside records.py would keep it true.
Nit: the five "fails on main" tests do not fail with the quoted messages as committed.test_record_readers.py:20 imports dokimi.eval.records at the top, so on main the file stops at collection with ModuleNotFoundError. The quoted assertions only appear once the import exists.
Checked, no issues
Numbers:report generate (md and json), report eval markdown, the trust gate verdict and every check, baseline save/compare on valid files, model-compare, arms report, eval ragas/grounding/panel-score and the arm runner's judge/summaries give the same output as main on well-formed files, including id 0, int vs float aggregates, one-arm-scored questions and NaN deltas.
Panel files: the five listed regression tests pass on the head and fail on main once the import exists; the panel question key and an all-failed panel question now read correctly.
Schema and docs: only the retrieval_hit description changed in eval.v1.json; no stale references to diagnoser.retrieval_hit, stats._aggregate_of, plugin_delta._scores_by_question or the old diagnose( signature remain in docs, comments or tests. No generated artifacts, new files carry the SPDX header, no new doc page is needed for the nav.
Tests are offline and test_record_readers.py builds its eval file with the real panel writer.
Review follow-ups for eval/records.py:
- baseline save computes its summary before writing and exits 1 on a
RecordError, so a bad file leaves no orphan baseline.
- report eval's Q&A sidecar reads answers per record; a bad one is left
out with a warning instead of blanking every answer.
- cross-judge's stored scores read answers and evaluations without a
join, so arms files with the unknown_q fallback id still read; the
command turns a RecordError into a clean exit.
- eval score exits 1 naming the record when evaluate_answers refuses it.
- a question_id of the wrong type says so instead of 'missing'.
- tests for the binary join's precedence and fallbacks.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RW8mAicb3ibXdzHMV83wDn
The records reader refuses such an id, so `arms run` paid for every
answer and then lost the run in its own summaries, writing nothing.
The questions loader now refuses it before the first call.
Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
`report generate` and `report eval` let RecordError escape as a
traceback; `report generate`, `report model-compare` and
`arms plugin-delta` named the record but not the file, though the
last two read several. Each now exits 1 with `<file> <section>[i]: ...`.
Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
Differential run main vs 895727b over the fixtures, 199 stored artifacts and pipeline artifacts produced with a stub LLM: well-formed files give identical output (dashboard identical, 58 model-compare runs byte-identical), eval score checks every record before the first judge call, #10's retrieval_hit semantics hold over a 3000-record fuzz, and the four bugs from the earlier review stay fixed.
Pushed (each test fails on this head):
fd82b7d: arms run paid for every answer (8 calls) and then lost the run on a float or bool question id; the id is now refused before the first call, which also covers the matrix run.
72246ec: report generate and report eval ended in a raw traceback on a malformed record, and model_compare and arms plugin-delta named the record but not the file; all four now exit 1 with Error: <file> ....
Open, your call:
baseline save on an eval with an aggregate null or "80" next to a numeric delta: exit 0 with with=52.33, while the trust gate on the same file uses 62.8 (n=5); report generate markdown on a bool aggregate raises TypeError at generator.py:395 (main rendered it). Unscored arms should stay out of the mean.
eval cross-judge on a legacy inline-score arms file: exit 2 becomes 0 with stored-judge verdicts, and with --judges the Holm p goes 0.727 to 1; binary answers files now also get paid checklist, pairwise and cross-judge calls. Keep those readers arms/evaluations-only, or announce it.
A binary record with a stray scores: null silently loses its with/without scores: trust gate PASS n=12 becomes FAIL n=4.
#69 and #73 conflict with this PR in cross_judge.py, judge_reliability.py and judge_context.py, and some hunks git merges cleanly still break: rec["question_id"] in arm_runner._score raises TypeError (16 tests) and data in cmd_eval_judge_validity raises NameError (6 tests).
Resolve conflicts with intel#78/intel#69/intel#43:
- arms: keep main's questions.v2 loader; keep the id-type check before the run.
- cross-judge: keep main's judge_materials gate; stored scores and the
pairwise/checklist gate read records without a join, and a RecordError
exits 1 naming the record (tests updated from exit 2).
- arm_runner: digest lookup uses Record.question_id.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RW8mAicb3ibXdzHMV83wDn
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Review follow-ups (xaleryb):
- baseline save averaged a null/"80" aggregate as 0 next to a numeric
delta; it now counts only questions with both arms scored.
- report generate's top/bottom delta rows left out unscored arms, which
raised TypeError on a bool aggregate.
- a binary record with a stray `scores: null` was read as an arms record
and lost its with/without scores.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RW8mAicb3ibXdzHMV83wDn
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
e6290bc covers items 1 and 3, so we dropped our own commits for them. Item 2 (eval cross-judge on legacy inline-score arms files, and paid checklist, pairwise and cross-judge calls on binary answers files) is still your call. The merge with main took in #69; the conflict with #73 is still ahead.
The reason will be displayed to describe this comment to others. Learn more.
Review at e6290bc. The follow-up commits fix the reported bugs: baseline save refuses a bad file before writing, the Q&A sidecar drops only the bad record, cross-judge reads unknown_q files and exits 1 on a bad record, and eval score exits 1 instead of a traceback. Two things are still worth fixing before merge: eval cross-judge, eval checklist and eval pairwise accept more file shapes than on the base and the changelog does not say so (@xaleryb's item 2), and the join precedence rules are still not pinned.
make ci-local: 19 passed, 0 failed, 4 skipped (docker/network/semgrep, job conditions). 5,066 tests, changed-line coverage 97%. Python 3.13, so the matrix legs on 3.11/3.12/3.14 did not run.
Differential runs of the base and the head on the same fixtures (well-formed and malformed eval, answers and arms files, with the LLM transport mocked): baseline save/compare, trust gate, report generate and report eval, dashboard, model-compare, arms report, arms plugin-delta, grounding, cross-judge. Well-formed files give identical output.
Mutation runs on records.py and the follow-up code: 134 mutants, 68 of which pass the full suite (5,065 passed).
schema_compat vs the base: OK (only the retrieval_hit description changed). The changelog fragment passes the checker, new files carry the SPDX header.
1. I agree with @xaleryb's open item 2: legacy inline-score and binary answers files now go through eval cross-judge, eval checklist and eval pairwise, and the changelog does not say so. It still holds at this head; details in the inline comment at cross_judge.py:152.
2. Tests for the follow-up code leave real regressions through. Mutation check on the head, full suite each time (5,065 passed, 2 skipped):
records.py:280: "arms" in item or item.get("scores") is not None passes. test_a_binary_record_with_a_stray_null_scores_keeps_its_scores pins scores: null only. A binary answers record with "arms": null and with_docs/without_docs payloads would be read as an arms record with no conditions and lose both answers.
generator.py:209: deltas.sort(reverse=False, ...) in _top_deltas passes, so "top improvements" would list the worst questions. test_report_top_deltas_leave_out_a_bool_aggregate only checks which ids appear, with one valid row. Add a test with three scored rows, limit=2, and assert the order in both _top_deltas and _bottom_deltas.
baseline.py:38-42: dropping the e.score(BASELINE) is not None half of the new filter passes. test_baseline_summary_leaves_out_a_question_with_an_unscored_arm nulls only the context side. Add a row with without_docs.aggregate: null and a numeric delta.
Still open from my earlier review:
records.py:370 (join tests): the new test pins the stamped retrieval_hit, the eval-only question and the one-sided metadata fallbacks. scores={**e.scores, **a.scores} still passes the full suite, and so do the union order and one-sided sources of conditions, the raw merge order, and the persona/ground_truth/error/diagnosis/expected_skill preferences when both sides carry a different value. Add one test where the answers record has an inline legacy score and the evaluation a different one for the same arm, and one where both sides carry different metadata.
judge.py:297 (empty question_text): the eval score traceback is gone. cross-judge --judges, pairwise, checklist and ArmRunner.judge still send "" to the judge for the same record, and degrade.py skips it. The changelog states the rule for eval score only; #81 takes it up for the other paths.
Checked, no issues
Earlier bugs:baseline save with an id-less record exits 1 naming the file and writes nothing; the report eval sidecar keeps the other answers and warns; an arms file with unknown_q for every question reads in cross-judge (stored and --judges) as on the base; a RecordError in cross-judge exits 1 without a traceback in the stored, --judges, --pairwise and --checklist paths.
Numbers:report generate md and json, report eval, the trust gate, baseline save/compare, model-compare, arms report, the dashboard and plugin-delta match the base on well-formed files. A "80" or bool aggregate is unscored, as the changelog says.
Question ids:question_id that is present but not a string or an integer now reports question_id must be a string or an integer, got 1.5. The question_id-over-id rule is in the changelog.
Repo rules: changelog fragment valid, no generated artifacts, no new doc page needed for mkdocs.yml, tests offline.
…s-judge/checklist/pairwise
- join: tests for the evaluation's score over an inline legacy score, one-sided
scores, conditions order, raw merge order, and every metadata preference
when both sides differ; answers-only/eval-only order; duplicate answers.
- records: a binary record with arms: null keeps its answers.
- report: top/bottom deltas order with limit; baseline summary leaves out a
question with an unscored baseline.
- changelog and --answers help: binary answers files are read as arms
with_docs/without_docs by cross-judge, checklist and pairwise, and incur
paid judge calls; a CLI test covers pairwise on such a file.
- generator._columns docstring: the deltas are the persisted ones.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RW8mAicb3ibXdzHMV83wDn
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…#81)
* fix(judge): refuse a record with no question text on every judge path
evaluate_answers, the panel, ArmRunner.judge, pairwise, checklist,
score_measure and the degradation pairs disagreed on a record whose
question text is missing or blank: some sent "" to the judge, degrade
skipped the record, evaluate_answers recorded a per-question error only
for a missing key. A shared helper (dokimi.eval.question_text) now checks
every record before the first judge call and raises EmptyQuestionError
naming the record; the CLI prints "Error: ..." and exits 1.
Follow-up to review on #80.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RW8mAicb3ibXdzHMV83wDn
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
* fix(arms): refuse a blank question before any answer is paid for
The question-set reader accepts a whitespace-only question, and
`arms run --judge` reached the judge's refusal only after every arm had
answered every question; the run then exited 1 without writing its
output, so the paid answers were lost. Check the questions where the
judge is built, before the sampling probe and the first answer.
Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
* fix(eval): refuse a blank question only where it is judged, before the probe; exit 2
- RAGAS: check rows before the first RAGAS call; the command wrapper lets
EmptyQuestionError through instead of turning it into a warning.
- pairwise/checklist/cross-judge: the jobs are built (and checked) before the
--judge-temperature sampling probe, so no billed call precedes the refusal.
- A record that would never be judged (answer-error, empty answers, no points,
no baseline, under the length floor) is skipped, not refused.
- Exit 2, the input-error code of these commands; 1 stays the verdict code.
- arms run/matrix: the refusal is no longer reported as an arm setup error;
the runner reads the question with the guard's key order; ArmRunner.judge
reads the text once for the call and the row.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RW8mAicb3ibXdzHMV83wDn
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
---------
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: Alexander Rybkin <alexander.rybkin@intel.com>
Resolve conflicts with intel#81 (empty-question rule): the judge paths keep the
record objects and call require_question_texts / require_question_text on
the parsed records before the first judge call; the duplicate RecordError
question check in Judge.evaluate_answers is replaced by intel#81's
EmptyQuestionError (exit 2). checklist_jobs, pairwise_jobs and
scored_answers read records. A record without question_id stays a
RecordError (intel#80) in the degradation pairs; unreproducible_arms keeps
intel#87's skip of malformed records.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RW8mAicb3ibXdzHMV83wDn
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
cross-judge, checklist and pairwise read answers under `arms` and stored
scores under `evaluations[].scores` before this PR. Through the shared
reader they also took a binary answers file's with_docs/without_docs as
two arms (paid judge calls where the base made none) and a legacy inline
`arms[name].evaluation` as the stored judge (cross-judge exit 2 became 0,
and the extra measure moved the Holm-adjusted p of --judges).
- records: arm_answers / arm_scores / has_arms read only those fields.
- scored_answers, pairwise_jobs, checklist_jobs, stored_measure and
unreproducible_arms use them.
- eval pairwise, eval checklist and eval cross-judge --judges exit 2
naming the file, before any judge call, when no record has `arms`.
- changelog: the widening entry is replaced by the refusal; the
--answers help is the base text again.
- test: question_id wins over id, id is the fallback (pins the rule
the changelog states).
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
@antonwolfy@xaleryb, thanks for both rounds. Every point is now addressed at 1892bf4 (main is merged in at 6f1962b, #81 included). Could you re-review?
Point
Status
baseline save wrote the file, then crashed on an id-less record
Cross-judge/checklist/pairwise take binary and legacy inline-score files (xaleryb 2)
1892bf4: refused, not documented. The arm judges read arms and evaluations[].scores only, as on main. A file with no arms exits 2 before any judge call, and a legacy inline-score file has no stored measure again.
"Every reader goes through records.py" is too strong
PR text now names the remaining raw-key reads; AGENTS.md states the rule for new readers. I did not add the grep guard: writers and other formats share the keys, so it needs an allow-list. It can be a follow-up if you want it.
"Fails on main" tests stop at collection
PR text now says so
The one change left vs main for the refusal is in the fragment: cross-judge --judges on a binary file exits 2, where it printed an empty table with no judge call.
Locally the full suite passes on two seeds (5250 passed, 2 skipped). make ci-local --offline has 0 failed, changed-line coverage is 97%, and records.py is at 100%.
The reason will be displayed to describe this comment to others. Learn more.
Review at 1892bf4. The refusal in 1892bf4 does what the thread asked: pairwise, checklist and cross-judge --judges exit 2 before any call on a file with no arms, and a legacy inline-score file has no stored measure again. One regression is left from the same family: report model-compare now treats a binary file's with_docs/without_docs as arms.
make ci-local: 19 passed, 0 failed, 4 skipped (docker/network/semgrep, job conditions). 5,251 tests, changed-line coverage 97%. Python 3.13, so the matrix legs on 3.11/3.12/3.14 did not run.
Differential runs of the base and the head with the LLM transport mocked: the arm judges' pure functions (scored_answers, pairwise_jobs, checklist_jobs, stored_measure, unreproducible_arms) over 25 cases, and the three arm-judge commands over 17 file shapes. Output, exit code and judge-call count match the base on well-formed arms files, unknown_q ids, id 0, null arms and duplicate ids.
Reproductions for the two code findings below, and a check of report generate, trust, baseline save and the dashboard against the base on well-formed files: no drift.
Still open from my earlier review: nothing. The eval score traceback, the join mutants, the arm judges' file shapes and the _columns docstring are all fixed at this head.
Checked, no issues
Arm judges: all three call sites of _require_arms sit after _load_answers, so a RecordError still exits 1 first. Binary files exit 2 with 0 judge calls on pairwise, checklist and cross-judge --judges. Making arm_answers or arm_scores return the binary fields fails the new tests.
Join: swapping the precedence of text, category, scores, raw order, the stamped hit or the duplicate-id checks fails the suite.
Repo rules:with_docs/without_docs keys and schema versions are unchanged (only the retrieval_hit description), the changelog fragment passes, new files carry the SPDX header, no generated artifacts, tests are offline.
non_baseline_arms and scored_arms read rec.conditions which for binary files
includes with_docs/without_docs, conditions not treatment arms. This caused
check_run_consistency to falsely report two binary files as measuring different
treatments.
Fix: read only arm_scores and arm_answers, which exclude binary conditions.
Add test that runs check_run_consistency on two binary files.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
ArmRunner.judge looped over rec.conditions, which for binary records includes
with_docs/without_docs conditions that are not treatment arms. This caused
binary records to be judged as two-armed.
Fix: loop over arm_answers(rec) instead, which returns only treatment arms.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The record goes through EmptyQuestionError handler which exits 2, not 1.
RecordError exits 1.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The reason will be displayed to describe this comment to others. Learn more.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
No open issues 🎉
What changed in this PR
Refactors all readers of answers.v1 / eval.v1 / arms.v1 artifacts to go through a single canonical parser (dokimi.eval.records), ensuring consistent key handling, shape detection, and error reporting across the judge/panel/reports/gates/dashboard/CLI.
Changes:
Added dokimi/eval/records.py as the single source of truth for reading/joining/validating record shapes and values.
Migrated ~40 call sites (reports, gates, CLI commands, eval helpers) to consume Record/Run via records.read/load/records.
Added extensive regression and reader-shape tests + updated docs/schema/changelog to reflect the new contract.
Whitespace-only question text bypasses valid fallback
dokimi/dokimi/eval/records.py:501
A whitespace-only question_text masks a valid fallback question or text. require_question_texts() accepts the fallback, but the single judge, panel, arm runner and RAGAS then use rec.question_text, sending only whitespace to grading calls. Match question_text_of()'s nonblank check so validation and the prompt use the same question.
The reason will be displayed to describe this comment to others. Learn more.
Review at 5573776. The three fixes from my last review hold: model-compare no longer sees with_docs/without_docs as arms, ArmRunner.judge no longer judges a binary record, and the changelog says exit 2. The same fixes introduced two regressions on null payloads, and neither new test would catch them. I'd fix these before merge.
Differential runs of the base, 1892bf4 and the head for non_baseline_arms, scored_arms, check_run_consistency, extract_scores and report model-compare over 15 artifact shapes (current and legacy arms files, null arms, binary files, answers-only, evaluations-only), with the LLM transport mocked. Only the all-null-treatment shape differs, see the inline comment.
ArmRunner.judge with a mocked judge on records with a null skill arm and a null baseline arm, base vs head.
Mutation runs: the model_compare.py fix reverted fails the new test; the arm_answers(rec) loop reverted to rec.conditions passes the full suite (5,251 passed, 2 skipped).
I agree with @copilot-pull-request-reviewer on arm_runner.py:451 (null arms dropped), records.py:296/:366 (scores.X: null does not clear a legacy inline score) and records.py:501 (whitespace-only question_text). New evidence:
skill: null: the head drops skill from scores, so extract_scores(force_treatment_arm="skill") exits with "not found"; with baseline: null the question leaves the comparison (base 1 row, head 0).
paired_vectors on the legacy-inline shape gives ([], []) on the base and ([60.0], [80.0]) on the head.
question_text: " " with question: "Real?": the base sent Real?, the head sends ' ' to the judge, because require_question_texts strips and Record.question_text does not.
Checked, no issues
Exit codes:EmptyQuestionError ends in exit 2 (cli.py:97-101) and a RecordError from _load_answers in exit 1, as the fragment now says.
Legacy inline scores in model-compare: base scored_arms and non_baseline_arms already counted legacy arms[x] payloads through the answers section, and the head still does through arm_answers. Output is identical on the legacy shapes.
Repo rules: both changelog fragments keep a valid format, eval.v1.json changes one description only, schema_compat is OK, no generated artifacts, tests are offline.
…p null arms
An explicit null under scores clears a legacy inline score for that arm, and a
null evaluation in a join no longer keeps the answer's inline score. A new
arm_names reader lists every arm an arms record ran, null ones included, so the
arm judge writes an unscored row for a failed arm and non_baseline_arms still
sees a treatment that produced nothing. Adds the missing judge and
model-compare regression tests.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
trust_gate (components) imported dokimi.eval.records (evaluation), an
upward import. records.py depends only on the standard library, so it
moves to dokimi/records.py in the foundation layer, which both the gate
and the evaluation engine may import. Imports, docs, schema description,
and the changelog fragment now name the new path.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The reason will be displayed to describe this comment to others. Learn more.
Still open from the Copilot review: this accepts a whitespace-only question_text, while require_question_texts() skips it and checks question instead. A record with question_text " " and question "How do I X?" passes the guard, and then the judge, panel, ArmRunner.judge, RAGAS and degrade get " " as the question. Please use the same check as question_text_of(): if isinstance(value, str) and value.strip():, and add a test.
The reason will be displayed to describe this comment to others. Learn more.
A RecordError exits 1 here (and in judge_reliability.py:33 and :270), but cli.py says bad input exits 2 because 1 is the verdict of eval judge-validity and of the --strict gates. eval judge-validity on a file with a record without question_id now exits 1, the same code as a failed validity gate. In eval score a missing id exits 1 and a blank question exits 2. Please exit 2 for a RecordError too, and say so in the fragment.
The reason will be displayed to describe this comment to others. Learn more.
For a Run this returns all its records and ignores key, so stored_measure() on a Run loaded with section="answers" finds no stored measure. Either accept only the dict here, or check run.section against key.
The fragment kept from an earlier commit is replaced by a new file named for
this branch, with the new dokimi.records path and the null-score fix from
4502d26.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
This branch has not been deployed
No deployments
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
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.
antonwolfy
left a comment
Part of #4.
Summary
Readers of
answers.v1/eval.v1/arms.v1records each picked their own keys. The judge, panel, reports, gates and judge-reliability readers listed below now go through one module,eval/records.py. It is the only code that knows the binary shape (with_docs/without_docs) and the arm shape (arms/scores, legacy inlineevaluation.aggregate_score):That is about 40 reader sites in 24 files. Some are easy to miss: the judge's console summary, the
report evalQ&A sidecar, the dashboard,baseline save/compare,stats.paired_vectors/arm_significance, the interaction contrasts, the arm runner'sjudge_contextsource count, and the answers branch ofquestions panel-review. A few raw-key reads remain and give the same result today:orchestrator/pipeline.py's step summary,judge_context.py's chunk text, and therawmetadata inmodel_compare.pyandeval_report.py.AGENTS.mdmakes records.py the rule for new readers.Values are read as written. An id stays the
str/intit was (0is an id) and an aggregate stays theint/floatit was, so reports print what they printed before. The persisted nameswith_docs/without_docsdo not change. The arm judges (eval pairwise,checklist,cross-judge) still read onlyarmsanswers andevaluations[].scores, as on main. The averages are untouched: reconciling them is theeval/compare.pyPR.Evidence
No number moves on well-formed files. I ran every migrated reader on main and on this branch with the same fixtures. The eval.v1 file has int and string ids, an id of 0, int and float aggregates, one question scored in one arm only, an errored question, panel agreement and a stamped
retrieval_hit. There is also its answers.v1 file and three arms.v1 runs (current, a second model, legacy inline scores). Outputs compared:report generate(md and json),report eval(md and Q&A sidecar), the trust gate, the dashboard snapshot, the baseline summary, grounding,model-compare, plugin delta, cross-judge, pairwise, checklist, arm summaries, the degradation pool, and the judge/panel/arm runner outputs and prompts with a stubbed LLM.diff -r before afteris empty. Both reviewers' differential runs agree.Readers that read the wrong key (
tests/test_record_readers.py). On main this file stops at collection because it importsdokimi.eval.records. With that module present, each test fails on main's reader as shown and passes now:test_report_generate_shows_the_question_of_a_panel_eval_file['', '', ''] == ['How do I use feature 0?', …]test_report_eval_shows_the_question_of_a_panel_eval_filequestion_text: nulltest_report_generate_skips_a_question_whose_panel_votes_all_failedTypeError: unsupported operand type(s) for +: 'float' and 'NoneType'test_question_panel_review_reads_the_questions_of_an_answers_file[] == ['How do I use feature 0?', …]test_model_compare_reads_a_legacy_run_with_a_null_armAttributeError: 'NoneType' object has no attribute 'get'Review fixes. Each test fails on the commit before its fix:
baseline savewrote the file and then crashed on an id-less record:test_baseline_save_refuses_a_malformed_eval_file_before_writing.report evalsidecar:test_report_eval_sidecar_drops_only_the_malformed_answers_record.cross-judgeraised onunknown_qids:test_stored_scores_read_an_arms_run_with_the_fallback_question_id.test_baseline_summary_leaves_out_a_question_with_an_unscored_arm/…_unscored_baseline.test_report_top_deltas_leave_out_a_bool_aggregate,test_report_top_and_bottom_deltas_are_ordered.scores: null/arms: nullhid a binary record's conditions:test_a_binary_record_with_a_stray_null_*.test_the_arm_judges_send_no_binary_answer_to_a_judgeandtest_cross_judge_takes_no_stored_measure_from_*.The reader itself.
tests/test_records.pycovers the binary, arms and legacy shapes, ids and values as written, and everyRecordError. It also pinsjoinprecedence: the evaluation's score over an inline one, metadata when both sides differ,conditionsandraworder, eval-only questions, and the stampedretrieval_hit. It pinsquestion_idoveridtoo. Everyjoinmutant from the review now fails the suite, and so do the_top_deltasorder, baseline filter andarms: nullmutants.records.pypassesmypy --strict.Checks. The full suite passes on two random seeds (31337, 2718): 5250 passed, 2 skipped.
make ci-local CI_LOCAL_ARGS=--offlinegives 0 failed, 14 passed, 7 skipped (network/docker). Changed-line coverage is 97% andrecords.pyis at 100%.schema_compatis OK: the only schema change is theretrieval_hitdescription, which pointed at the removeddiagnoser.retrieval_hit. The changelog fragment check passes.Merge Danger
Door: two-way. No persisted format, schema version or flag changes, and no score changes for well-formed files, so a revert restores today's readers.
Blast Radius: malformed-input
answers.json answers[3]: missing question_id). This covers a missing id, an id that is neither a string nor an integer (question_id must be a string or an integer, got 1.5), a condition/arm payload that is neither an object nornull, and a badexpected_skill. Before, such a record was skipped, given a made-up id, or crashed a paid run part-way.eval scoreandarms runcheck before their first call.report generate/eval,model-compareandplugin-deltaexit 1 naming the file.baseline savewrites nothing. Thereport evalsidecar drops only the bad record. The dashboard leaves the file out with a warning.question_idwins overidwhen a hand-merged record carries both (utils.question_idpreferredid)."80", a bool) is unscored everywhere. The dashboard used to parse"80".baseline saveand the report's top/bottom tables leave out a question with an unscored arm.baseline comparematches ids as strings: an int1and a string"1"are the same question.eval cross-judge --judgeson a binary answers file exits 2 before any call. It used to print an empty table.pairwiseandchecklistalready exited 2 on such a file.diagnose(context_answer, delta)takes the contextAnswerand the delta.diagnoser.retrieval_hitmoved toRecord.retrieval_hit. The privatestats._aggregate_ofandplugin_delta._scores_by_questionare gone. A patch onJudge._evaluate_answer_pairreceives aRecord.eval/compare.py):report/*,eval/stats.py,gate/trust_gate.py.Checklist
pytestpassesruff,mypy, and schema checks pass.github/.licenserc.yaml)🤖 Generated with Claude Code