Skip to content

feat(repo): repo compare --metric judge:recall_judged reads judged recall - #395

Open
antonwolfy wants to merge 13 commits into
intel:mainfrom
antonwolfy:feat/compare-judged-recall
Open

antonwolfy wants to merge 13 commits into
intel:mainfrom
antonwolfy:feat/compare-judged-recall

Conversation

@antonwolfy

Copy link
Copy Markdown
Contributor

Summary

Closes #291. This is its last part: "repo compare reads recall_judged". The first half, repo hits judge writing <cell>/hits_judged.json with recall_judged, merged as #357. This half waited on #332, which moved the per-task values out of repos/compare.py (at its 800-line cap) into compare_values.py. In the decision comment on #291, the maintainer asked that a figure be called uncalibrated unless labels back it, and that recall_judged count same point only.

On main, every --metric comes from grade.json metrics, so judged recall cannot be compared across arms. A batch of two tasks, both arms judged, one treatment cell regraded since judging:

$ python cli.py repo compare b1 --baseline guidance:none --metric recall --metric judge:recall_judged
✗ no finished cell reports a numeric or boolean --metric judge:recall_judged        (exit 2)

With this PR:

  metric:recall                   2      0.500      0.875  +0.375 [-1.213, +1.963]      0.500       -  2/0/0
  metric:judge:recall_judged      1      0.250      0.500  +0.250 -                     1.000       -  1/0/0
unpaired: t2 (m-1, no metric:judge:recall_judged in treatment)
judge:recall_judged, guidance:raw: 1 of 2 cells read (1 stale: regraded since judging); run repo hits judge again
note: judge:recall_judged is a judge's labels of the credited pairs, at most recall; it is uncalibrated unless repo hits judge --labels reported agreement for these arms (contract §9)
Input Exit / effect
--metric judge:recall_judged, cell's file valid and current Read: the row metric:judge:recall_judged gets the usual statistics, and n_baseline/n_treatment count the cells that report it
File missing, stale (its recall ≠ grade.json metrics.recall), another cell's (cell/task/arm/model differ), recall_judged null, not JSON / not an object / wrong schema_version / a directory / not UTF-8 / nested too deep The cell does not report the metric (never 0); it is counted in judged_cells and on the per-arm text line
Every cell of one arm left out for a task The task is unpaired for that measure, as with any metric
--metric judge:recall (any other judge: field) Exit 2: --metric judge:recall: a judge: metric reads hits_judged.json, and the only one accepted is judge:recall_judged
No selected cell reports it Exit 2: no finished cell reports --metric judge:recall_judged (2 no hits_judged.json, 1 stale: regraded since judging): run repo hits judge --judge PROVIDER/MODEL first

Changes

  • New dokimi/repos/compare_judged.py reads judge:<field> metrics from each cell's hits_judged.json and applies the rules above. recall_judged is the only field it accepts. It counts each cell's reason per compared arm and writes the text lines. The file name comes from hits_judge.HITS_JUDGED and the schema version from artifacts.SCHEMA_VERSIONS.
  • dokimi/repos/compare.py only calls into it: in load_batches, before the cells are read (refusing a bad judge: name), next to _check_metrics, in the summary, and in format_compare. The file grows to 781 lines.
  • repo compare --json gains judged_cells. It is null without a judge: metric. Otherwise it holds metrics and arms: per compared arm, cells (of the compared models) and the counts read, no_file, stale, other_cell, unjudged and unreadable.
  • --ledger calls compare with no metrics (ledger.py:177), so a judge: metric does not reach the rule verdicts. The contract says so; the ledger code is unchanged.
  • Docs:
    • Contract §9's repo compare bullet: the judge: reading, which cells count, judged_cells, the per-arm line, the refusal and the uncalibrated note.
    • Contract §4.5 and §9's repo hits judge text: one sentence pointing to the compare reading.
    • reviewing.md "Reading a replay batch": the compare command, and "quote it only next to a calibration".
    • architecture.md: the new module.
    • The --metric help.
  • Changelog fragment (Added).

Risk

A plain --metric NAME still reads grade.json, and its output is unchanged. repo compare --json has one new key, judged_cells, which is null unless a judge: metric is asked for. A --metric starting with judge: gave exit 2 before ("no finished cell reports") unless a grade.json metric had that literal name. Now judge:recall_judged is read from hits_judged.json, a grade.json metric of the same name is ignored, and every other judge: name exits 2 with the new message. No exit code changes otherwise. Compare cannot see whether the judge was calibrated: the text says so, and the figure stays uncalibrated unless repo hits judge --labels reported agreement.

Testing

On a Linux review host, at d72aa27 (on main 5ce3207):

  • make ci-local: 30 passed, 0 failed, 7 skipped (external-catalog-smoke, which needs docker and network; test-latest, docs.yml:deploy and pip-audit.yml:report, whose job conditions are false on a PR; cli-surface "Package the result", an expression the local runner does not know; conda-forge-deps, no micromamba; semgrep secrets, offline). 15633 tests passed, 64 skipped; changed-line coverage 100% (79 lines). Not covered locally: the matrix legs on Python 3.11, 3.12 and 3.14.
  • New tests/test_repo_compare_judged.py (10 tests). The fixtures are written by the real writer (hits_judge.cell_doc + save_artifact). They cover:
    • two arms with a hand-computed per-task delta and n;
    • --metric recall --metric judge:recall_judged giving both rows;
    • stale, misplaced, unjudged and bad files (not JSON, wrong schema_version, a list, deep nesting, not UTF-8, a directory), each left out and counted in judged_cells and the text line;
    • a task whose every treatment cell is out becoming unpaired;
    • judge:recall, judge:n_threads and judge: refused with exit 2;
    • no usable file refused with the repo hits judge hint;
    • --task narrowing;
    • the CLI with --json.
  • The repro above: main exits 2 on --metric judge:recall_judged; this branch prints the rows and the lines shown.
  • Mutants: each of 15 breaks in compare_judged.py and compare.py fails the test that pins it. The breaks: identity check dropped, stale check dropped, null read as 0, schema_version check dropped, RecursionError not caught, a grade.json metric of the judge name kept, other judge: fields accepted, hint refusal dropped, per-arm line for no arm, per-arm line for every arm, note dropped, judged_cells always null, judged_cells counting cells outside --task, hits_judged.json never read, note printed without a judge metric.

Revert

A plain revert removes the reading, judged_cells and the docs. Nothing stored changes: hits_judged.json and its schema are only read.

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
  • Self-reviewed per "Before opening a PR" in AGENTS.md
  • No ad-hoc run artifacts committed
  • New files carry the SPDX license header (or are exempt in .github/.licenserc.yaml)

…call

Second half of intel#291: `repo hits judge` writes `<cell>/hits_judged.json`
with `recall_judged`, and `repo compare` could not read it, since every
`--metric` came from `grade.json` `metrics`.

A `--metric` named `judge:<field>` is now read from the cell's
`hits_judged.json` (new `dokimi/repos/compare_judged.py`; `compare.py`
only calls it). `recall_judged` is the one field taken; any other
`judge:` name is refused with exit 2. A cell reports it only when the
file is a `repo_hits_judged.v1` object naming the cell's own cell, task,
arm and model, its `recall` equals the cell's current `grade.json`
`metrics.recall` (else the cell was regraded since judging), and its
`recall_judged` is a finite number; otherwise the metric is absent for
that cell, as a missing `grade.json` metric is, and a null is never 0.

`--json` gains `judged_cells`: per compared arm, the cells read, with no
file, stale, of another cell, unjudged and unreadable (`null` without a
`judge:` metric). The text prints one line per arm with a cell not read
and one note that compare cannot tell whether the judge was calibrated.
A `judge:recall_judged` no selected cell reports is refused with the
`repo hits judge` command to run first. `--ledger` passes no metric and
is unchanged.

Docs: contract §4.5 and §9, reviewing.md, architecture.md, `--metric`
help. Changelog fragment (Added).

Closes intel#291

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
@antonwolfy
antonwolfy requested a review from a team as a code owner November 4, 2026 18:46
Copilot AI balanced review requested due to automatic review settings November 4, 2026 18:46
@exai-bot

exai-bot commented Nov 4, 2026 •

Copy link
Copy Markdown

OpenCodeReview: Commit a805c63 was not reviewed. This PR has reached its review limit (6 reviews).

CI at `a805c63`: 10 still running, 8 passing, 2 skipped
Check Status
CLI interface diff still running
CodeQL (actions) still running
CodeQL (python) still running
Package install smoke test (py3.11) still running
Tests (py3.11) still running
Tests (py3.14) still running
Windows smoke test (py3.13) still running
self / repo scan still running
self / repo tasks check fix-degrade-pre still running
self / repo tasks check review-pr-conventions still running

To review this commit: re-request review from exai-bot in the Reviewers list.

<details><summary>Last review (6b7e062): no findings</summary>

Open threads from the bot: 0

</details>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The recall-only validity check can accept obsolete judgments when credited thread–finding pairs change without changing recall, so it must validate the judged inputs and cover this case with a regression test.

1 open issue
What changed in this PR

1 new must-fix finding.

This PR completes the comparison support for #291, allowing repo compare to read judged recall produced by repo hits judge alongside location-based recall.

Changes:

  • Adds --metric judge:recall_judged with artifact checks and unsupported-field refusal.
  • Reports per-arm inclusion and exclusion counts, plus an uncalibrated-judge note.
  • Adds regression tests, CLI help, documentation, and a changelog entry.
File Description
dokimi/​tests/​test_repo_compare_judged.py Tests judged metrics, exclusions, diagnostics, and CLI output.
dokimi/​dokimi/​repos/​compare.py Integrates judged metrics into loading and summaries.
dokimi/​dokimi/​repos/​compare_judged.py Reads judged artifacts and reports per-arm counts.
dokimi/​dokimi/​commands/​repo_run.py Documents the new metric in CLI help.
dokimi/​docs/​reviewing.md Explains comparison usage and calibration caveats.
dokimi/​docs/​repo-evaluation-contract.md Defines judged-metric acceptance and output behavior.
dokimi/​docs/​architecture.md Documents the new reader module.
dokimi/​changelog.d/​20261104_120000_antonwolfy_compare-judged-recall.md Announces judged-recall comparison support.

Review effort: Balanced

Comment thread dokimi/dokimi/repos/compare_judged.py Resolved
@antonwolfy antonwolfy self-assigned this Nov 4, 2026
@codecov-commenter

codecov-commenter commented Nov 4, 2026 •

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.80%. Comparing base (0e993b9) to head (a805c63).
⚠️ Report is 1 commits behind head on main.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #395      +/-   ##
==========================================
+ Coverage   94.78%   94.80%   +0.01%     
==========================================
  Files         353      355       +2     
  Lines       46575    46709     +134     
  Branches     7974     7994      +20     
==========================================
+ Hits        44146    44282     +136     
+ Misses       1541     1540       -1     
+ Partials      888      887       -1     
Files with missing lines Coverage Δ
dokimi/dokimi/commands/repo_run.py 95.21% <ø> (ø)
dokimi/dokimi/repos/compare.py 100.00% <100.00%> (ø)
dokimi/dokimi/repos/compare_judged.py 100.00% <100.00%> (ø)
dokimi/dokimi/repos/hits_judge.py 91.84% <ø> (ø)
dokimi/dokimi/repos/report.py 97.94% <100.00%> (+0.08%) ⬆️

... and 12 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…s changed

A regrade that credits other (thread, finding) pairs can keep the same recall,
so the recall check alone read an obsolete judgment as current. Compare the saved
n_threads and ordered pairs with grade.json too.

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Comment thread dokimi/dokimi/repos/compare.py Resolved
…dels

check_reported ran on every task-filtered cell before the arm and model
selection, so a hits_judged.json of an excluded arm or model satisfied the
metric and compare succeeded with no judged-recall row. Run it on the
baseline and treatment cells of the --model selection.

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>

@napetrov napetrov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: no blocking thread; one nit and one question on the inline threads. Tests not run here (scipy and regex are missing).

Question with no line: the refusal runs over all selected arms together, so compare exits 0 when the baseline has no readable file and a treatment does (0 of N cells read on the baseline line). That follows the documented "no selected cell reports it" rule. Was a refusal per compared arm intended?

Coverage: checked the diff, the zero-thread cell on head and base (probes/b1probe.py), prior threads T1 and T2 against head code. Not looked at: the test file (not run), hits_judge.py (outside this diff).

Comment thread dokimi/dokimi/repos/compare_judged.py Resolved
Comment thread dokimi/dokimi/repos/compare_judged.py Resolved
…own reason, a non-object match is skipped

A zero-thread cell has recall_judged null because it has no pair to judge, not because a pair is unjudged, so it is counted as no_threads and the hint to run repo hits judge again no longer names it. _pairs skips non-object matches as report.load_hits does, so a hand-edited grade.json does not leave a cell stale for good.

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
@antonwolfy

Copy link
Copy Markdown
Contributor Author

On the question without a line: yes, the refusal over all selected arms together is intended. It is the rule the contract states for a judge: metric ("when no selected cell reports it"), the same as a grade.json metric that no cell has. A baseline with no readable file while a treatment has one still exits 0, and the baseline line then says 0 of N cells read with the reasons and the command to run. A per-arm refusal would also block comparing a judged arm against a partly judged one, which the per-arm counts are there for. The new no_threads reason (ffa5143) is counted in those lines too.

@antonwolfy

Copy link
Copy Markdown
Contributor Author

Round 2 summary (head ffa5143, CI green):

  • Zero-gold-thread cell: it now has its own reason, no_threads ("with no gold thread"), counted apart from unjudged. It still stays out of the task mean and n. Contract, changelog fragment and a test updated.
  • Non-object details.matches entry: _pairs skips it as report.load_hits does, so a hand-edited grade.json no longer leaves a cell stale for good. Test added.
  • Refusal over all selected arms together: intended, per the contract rule "when no selected cell reports it" (answered above).

Both new tests fail when their fix is reverted. All review threads are resolved.

@vlad-perevezentsev vlad-perevezentsev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review at ffa5143. The reader is right for the cases it checks: another cell's file, a changed recall, n_threads or credited pairs, an excluded arm or model, and a null never read as 0. One kind of regrade still passes as current, and one hint asks for a rerun that changes nothing; both are in the line comments.

What I ran, at ffa5143:

  • make ci-local --offline: 15,638 passed, 0 failed, 63 skipped; every check passed (the network and Docker jobs were skipped).
  • A repro on the test fixtures: a grade.json whose credited pair (0, 0) has a new comment after judging is counted read; a n_threads: 0 cell gives 1 of 2 cells read (1 with no gold thread); run repo hits judge again.
  • Mutation: with the credited-pairs check removed, test_credited_pairs_that_changed_with_the_same_recall_make_the_file_stale fails.

Checked, no issues

  • Earlier threads: the stale check now compares n_threads and the ordered pairs (cb39ae3), check_reported runs on the baseline and treatment cells of --model only (6e1d55e), and a non-object match is skipped as report.load_hits skips it (ffa5143).
  • Refusal over all compared arms together: agreed with the answer to @napetrov. The per-arm line shows a baseline with 0 of N cells read, and a per-arm refusal would block comparing a judged arm with a partly judged one.
  • Docs and changelog: the contract §9, reviewing.md, architecture.md, the --metric help and the Added fragment match the code. The new files have the SPDX header.

Comment thread dokimi/dokimi/repos/compare_judged.py Resolved
Comment thread dokimi/dokimi/repos/compare_judged.py Resolved

@napetrov napetrov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review at ffa5143 (round 6e1d55e..ffa5143). Verdict: no new thread from me.

  • My nit (zero-gold-thread cell reported as unjudged): fixed, own reason no_threads.
  • My question (non-object details.matches entry): answered; entries now skipped as load_hits does.
  • Round pass found one row: "run repo hits judge again" hint printed when every left-out cell is no_threads. Same as @vlad-perevezentsev's open thread on compare_judged.py (line 144); not repeated here.

Coverage: round diff, plain + edge pass, no probes beyond the passes' own. CI green on head.

A regrade can keep the same credited (thread, finding) pairs, recall and
n_threads while the thread's comment changed (a `repo run` that
re-exported _eval/content with a reworded thread), and compare read the
old label as current.

`repo hits judge` now writes each pair's `comment` and `message`, the
grader's cuts the match holds, in hits_judged.json (two optional fields:
repo_hits_judged.v1 only widens). `repo compare --metric
judge:recall_judged` compares them with the grade's matches and counts
the cell stale when they differ. A file written before the pairs
recorded the cuts is checked on (thread, finding) only, as the contract
now says.

Settles the review thread on compare_judged.py:82 (a regrade that keeps
the credited pairs but changes their text).

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
The per-arm line and the refusal said to run `repo hits judge` again
even when every cell left out has no gold thread: a rerun writes the
same null for such a cell and spends judge calls for nothing.

Both now add the hint only when a cell is left out for a reason a rerun
can change (RERUN: no file, stale, another cell's file, an unjudged pair,
unreadable). The contract and the changelog fragment say so.

Settles the review thread on compare_judged.py:144 (the rerun hint for
cells with no gold thread).

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Codecov named four changed lines of compare_judged.py no test ran: the
grade with no `matches`, a `matches` that is not a list, and a file whose
pairs do not line up with the grade's in number.

Each is reachable and now has a test of what it decides: a parse-error
cell (no matches, no pair) is read with its measured 0 in the task's
mean; a grade whose `matches` is not a list (one `report.load_hits`
leaves out, so `repo hits judge` removes its file) and a file holding a
pair fewer than the grade credits, at the same recall, are stale rather
than read or an error.

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
fd7cd1e wrote `HINT ="run ...`; put the missing space before the
string. No behaviour changes.

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
@antonwolfy

Copy link
Copy Markdown
Contributor Author

(commented) Review at ffa5143. The reader is right for the cases it checks: another cell's file, a changed

@vlad-perevezentsev (#395 (review)): Both points from this review are fixed: the reworded regrade in 1b38f3d and the rerun hint in fd7cd1e. Details are in the two line threads.

@antonwolfy

Copy link
Copy Markdown
Contributor Author

(commented) Re-review at ffa5143 (round 6e1d55e..ffa5143). Verdict: no new thread from me.

@napetrov (#395 (review)): The row this pass found (the rerun hint when every cell left out has no gold thread) is the same point as the thread on compare_judged.py:144. fd7cd1e fixes it. The hint is now added only when a cell is left out for a reason a rerun can change.

compare.py keeps both imports: main's compare_interval and this PR's own module.

Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
A grader-failure cell, a grade without matches or another grader's cell
gets no hits_judged.json from repo hits judge, yet compare called it
no_file and said to run hits judge again. It is now not_judged, decided
by report.replay_skip, the rule load_hits itself uses, and kept out of RERUN.

Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
The note said "at most recall", but the recall row averages every cell
and the judged row only the cells read, so with a cell left out the
judged row can print above the recall row. Say the bound is per cell and
that the two rows then cover different cells.

Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
compare checks the comment and message cuts the grade keeps (CUT in
graders/review.py), so a reword past character 200, or of a finding's
rule or evidence, still reads the old label. Say so where the contract,
changelog and module claimed any reword makes the file stale.

Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
Each hits_judged.json records its judge, but compare never read it, so
a baseline labelled by one judge and a treatment by another gave a delta
with no sign of the change. judged_cells now lists each arm's judges,
and compare warns, as it does for other stamps, when they differ.

Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
@xaleryb

xaleryb commented Nov 6, 2026

Copy link
Copy Markdown
Contributor
  • Ran on Python 3.13 at head 6b7e062 plus the commits below: the full suite, ruff, mypy, bandit, the growth and comment-block gates and the benchmark gate, also merged with current main.
  • repo hits judge on the _batch fixture, then compare --metric judge:recall_judged: the grader-failure cell and the cell graded without matches were reported as no hits_judged.json; run repo hits judge again. A rerun spends every judge call again and changes nothing. Expected: a reason of their own and no hint.
  • A baseline judged by one judge and a treatment judged by another gave a delta with no warning. Expected a warning, as the other stamps get.
  • With a stale cell, the judged row printed 1.000 above recall's 0.625, while the note said "at most recall". The bound holds per cell.
  • A reword past the 200-character cut still reads as current, but the docs said any reword makes the file stale.
  • Pushed:
    • aeb6070: a not_judged reason through report.replay_skip, the rule load_hits uses, and no rerun hint for these cells.
    • 9c9016a: the note says the bound is per read cell.
    • d2aa4d7: the docs state the 200-character reword limit.
    • a805c63: judged_cells.judges, and a warning when the judges differ.

@vlad-perevezentsev vlad-perevezentsev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I checked a805c63. Both points from my last round are fixed. 1b38f3d records each pair's comment and message cuts, so a regrade that rewords a credited thread makes the file stale. fd7cd1e prints the rerun hint only when a rerun can change a cell.

Merge: safe now: it merges cleanly into current main, 1 commit behind (#396), and its tests pass on the merge. Once it is in, #392 conflicts with it (compare.py, architecture.md) and needs a rebase.

What I ran, at a805c63:

  • make ci-local --offline: 15,815 passed, 0 failed, 63 skipped; every check passed (the network and Docker jobs were skipped).
  • Mutations, each caught by its new test: ignore the comment/message cuts in _same_pairs; call every cell with no file no_file; never warn about mixed judges.
  • On the merge with current main: the compare, hits judge, report hits, docs refs, CLI surface and flags, and artifact fixture tests, 543 passed.

Checked, no issues

  • @xaleryb's commits: not_judged goes through report.replay_skip, and load_hits behaves as before with it. The note now bounds judged recall per read cell. The 200-character limit on detecting a reword is stated in the contract, the module and the changelog. The judges warning reads only the cells that were read.
  • Schema: comment and message are optional pair fields, so repo_hits_judged.v1 only widens. A file without them is checked on (thread, finding) only, as the contract says.
  • Docs and changelog: contract §9, reviewing.md and the fragment match the new reasons, the hint rule and the judges field.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

review_replay: optional judged recall over the credited hits

7 participants