Repository navigation
Conversation
The headroom command reads arms.v1 runs of one question set and flags an item as a ceiling when its no-skill arm's mean is at least 0.9, or as a floor when every arm's mean is at most 0. An item with fewer than three runs behind it is provisional. The table goes to stdout; -o writes a headroom.v1 artifact. Flags are descriptive: no score or gate changes. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The per-arm mean was rounded to four decimals before the threshold check, so an item at 89.996 (below 0.9) was flagged as a ceiling. The flags now compare the exact mean; the artifact still shows four decimals. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
A mistyped --no-skill-arm read as "no ceiling anywhere" and exited 0. It is now refused (exit 2), naming the arms the runs list. reps counted score entries, so one file passed three times, or a question id repeated within one file, faked three runs and lifted the provisional guard. The CLI now drops repeated input files by content digest with a note, and a question id repeated within one run is an error. The library counts each Run as one run. Also: the JSON keeps the unrounded mean (no 4-decimal rounding that turned 0.89996 into 0.9), the table rounds down with the flag's tolerance, the table lists the control arm once, a non-ASCII digit question id is named in the error instead of crashing, and the docs say the explicit --no-skill-arm skips the baseline agreement check. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…loor reps was the smallest run count over the arms, so one arm the skill missed in a run lowered the item's reps. It is now the number of runs in which the no-skill arm scored the item. An arm no run scored is listed in the item's unscored_arms (JSON and a table note) and is left out of the floor test. The floor also needs at least one non-baseline arm that scored, so the baseline alone at zero is not a floor. -o that cannot be written prints "Error: ..." and exits 2. The duplicate-input note says "byte-identical", as the docs do. The docs state that JSON goes to -o only and the table always goes to stdout. headroom.v1 is not on intel/main yet, so the schema gains unscored_arms in place without a version bump. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
github-actions
Bot
added
the
cli-interface-change
CLI interface change detectedThis PR changes dokimi's user-facing CLI surface (commands and/or options): + added command `dokimi eval headroom`
+ argument `arms`
+ option `--ceiling`
+ option `--floor`
+ option `--help/-h`
+ option `--min-reps`
+ option `--no-skill-arm`
+ option `--output/-o`Updated on every push; the |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #279 +/- ##
==========================================
+ Coverage 93.50% 93.55% +0.04%
==========================================
Files 273 276 +3
Lines 35569 35821 +252
Branches 6040 6085 +45
==========================================
+ Hits 33260 33513 +253
+ Misses 1492 1491 -1
Partials 817 817
🚀 New features to boost your workflow:
|
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 050de2e. The flag logic is sound and the edge cases I probed hold. Two test gaps let a real regression change the verdict; I'd close both before merge. One doc nit.
What I ran, on a Linux review host at 050de2e:
make ci-local: 27 passed, 1 failed, 7 skipped. 9731 tests passed, 4 skipped; the changed-line coverage step passes. The failingpip-auditstep lists advisories againstfilelock 3.32.7in the lock files, which this PR does not touch.- Reproductions of the CLI on hand-built arms files (empty, non-UTF-8, directory, non-arms, mixed question sets, unknown
--no-skill-arm, odd thresholds). No traceback, and exit codes match the docs. - A mutation run of 79 broken copies of the new code: 30 passed the 36 headroom tests. The two that decide a verdict are inline; I re-ran both by hand.
Checked, no issues
- Schema: the CLI's
-ooutput and the fixture validate againstheadroom.v1.json;headroomis registered inSCHEMA_VERSIONSand the schema is new, soschema_compathas nothing to widen. - Commits: all five carry
Signed-off-by. - Dedupe and reps: a repeated or byte-identical file counts once; a repeated question id in one file exits 2; an item missing from some runs gets fewer reps and is provisional.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
I pushed a commit per nit, up to 38d219c, which fix the nit from my review:
38d219c:dokimi/docs/evaluating-treatments.md:757: added the sentence that an out-of-range threshold (--ceilingin (0, 1],--floorin [0, ceiling),--min-repsat least 1) exits 2.
make ci-local at 38d219c: 27 passed, 1 failed (pip-audit, unchanged from the reviewed head), 7 skipped.
Not approving yet. Still open: the two risks from my review.
|
🔍 OpenCodeReview found 1 issue(s) in this PR. By severity: 1 low
Review details
|
…-ceiling-floor Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
An item with baseline 40 and skill 0 must not be a floor. Red when the floor test drops the baseline (all(... if a != control)), green on head. Thread: PRRT_kwDOVBg7g86xhYFw Thread: PRR_kwDOVBg7g88AAAABU4x1TA Thread: PRR_kwDOVBg7g88AAAABU5Bc8g Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…mand Red when the command calls headroom() with the default thresholds; the test asserts the flags and the thresholds of the -o artifact for ceiling 0.5, floor 0.2, min-reps 1. Thread: PRRT_kwDOVBg7g86xhYFz Thread: PRR_kwDOVBg7g88AAAABU4x1TA Thread: PRR_kwDOVBg7g88AAAABU5Bc8g Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Red when an option is renamed, its dest changes or a default drifts from the DEFAULT_* constants; one test also runs the command from parsed arguments. Thread: PRRT_kwDOVBg7g86xlrgs Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Five items cover ceiling+provisional, floor, unflagged+provisional and unflagged+steady. Red when flagged_provisional counts every provisional item, no_flag is hard-coded, or the floor count is read as the ceiling count. Thread: PRRT_kwDOVBg7g86xlrf- Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Numeric ids come first in numeric order, then the other ids. Red when the sort is lexicographic, insertion order, or ignores the numeric key. Thread: PRRT_kwDOVBg7g86xlrgN Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Each rejected threshold now names its own message (ceiling 0, negative and above 1; floor equal to the ceiling and negative; min_reps 0 and negative), and ceiling=1.0, floor=0 and min_reps=1 are accepted. Red when a bound moves. Thread: PRRT_kwDOVBg7g86xlrgX Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
headroom([]) is refused as "no runs to read"; a run with no questions and a run nothing was judged in give no flag, at library and command level. Red when the empty-input refusal is removed or the command stops printing the table. Thread: PRRT_kwDOVBg7g86xlrgc Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
With no_skill_arm="skill" on runs that declare "baseline", no_skill, reps and both flags follow "skill"; the mixed-baseline case now asserts its item too. Red when a given arm is ignored and the declared baseline is returned. Thread: PRRT_kwDOVBg7g86xlrgh Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The patch-coverage report named lines of dokimi/eval/headroom.py (the empty-input refusal) and the output-less branch of dokimi/commands/headroom.py. The tests of 544394e and 4bbc04a cover both: test_headroom.py alone reaches 100% of lines and branches of the two files. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
This review is the empty run marker of the OpenCodeReview bot; it carries no claim. Its six findings were the inline threads, each answered there. |
|
Informational notice, no change needed: the added |
| try: | ||
| run = records.load(path) | ||
| digest = hashlib.sha256(Path(path).read_bytes()).hexdigest() | ||
| except (records.RecordError, OSError) as e: |
There was a problem hiding this comment.
Nit: an arms file that is deeply nested JSON (e.g. 100000 [) makes json.loads raise RecursionError. records.load does not wrap it, and it is not an OSError/RecordError, so the command ends in a traceback instead of Error: and exit 2. Other commands in the repo catch RecursionError for this reason. Add it to the except tuple.
Suggestion:
| except (records.RecordError, OSError) as e: | |
| except (records.RecordError, OSError, RecursionError) as e: |
There was a problem hiding this comment.
Fixed in e5faa92.
Deeply nested JSON raised RecursionError from json.loads, which the command did not catch. It now goes through the same Error: / exit 2 path as the other read errors. Pinned by test_command_exits_2_on_an_input_nested_too_deep_to_read.
vlad-perevezentsev
left a comment
vlad-perevezentsev
left a comment
There was a problem hiding this comment.
Review at 95f84dd. The ceiling, the reps and the threshold rounding are right, and the new tests close the gaps the other reviews found. Two cases still turn an unmeasured no-skill arm into a result (an unflagged item, or a floor); they and a few smaller points are in the line comments.
What I ran, at 95f84dd:
make ci-local --offline: 0 failed, 17 passed, 13 skipped (network and Docker jobs).- Scratch runs of
headroom()andpython cli.py eval headroom: an arms file with noevaluations, a baseline whose judge failed next to a skill at 0, a declaredbaseline_armthe records do not use, a pretty-printed copy of a run,-oonto an input,--min-reps 0, and a deeply nested JSON file.
Checked, no issues
- Ceiling and rounding: the flag compares the unrounded mean with a 1e-9 tolerance, so 89.996 is not a ceiling and an exact 90 from 85 and 95 is; the table rounds down, so it never shows 0.90 for an item that is not flagged.
- Reps: they count the runs that scored the no-skill arm, and a question id repeated within one file is refused, so one file cannot pass as several runs.
- Schema and registration:
headroom.v1is a new kind inSCHEMA_VERSIONSwith a fixture; the command validates its output before writing it. - Docs and changelog: the section in
evaluating-treatments.mdmatches the flags and exit codes, including the threshold checks added in 38d219c; the fragment is underAddedand says the flags change no score.
| "floor": sum(1 for i in items if i.flag == FLOOR), | ||
| "provisional": sum(1 for i in items if i.provisional), | ||
| "flagged_provisional": sum(1 for i in items if i.flag and i.provisional), | ||
| "no_flag": sum(1 for i in items if i.flag is None), |
There was a problem hiding this comment.
An item whose no-skill arm was never scored is counted as unflagged, and a file with no scores at all exits 0.
arms run without --judge writes an arms.v1 file with no evaluations (the schema does not require them). eval headroom on it prints ceiling 0, floor 0, provisional 1 (0 of them flagged), unflagged 1 and exits 0, and summary.no_flag in the artifact counts every item. The same happens when the declared baseline_arm is not the name the records use (baseline_arm: control, records scored under no_skill at 100): the no-skill column is - everywhere, no ceiling is found, exit 0. In both cases "not measured" reads as "has headroom". The new test_a_run_with_no_evaluations_flags_nothing_and_has_no_reps and test_command_reads_a_file_nothing_was_judged_in pin this as intended.
Suggestion: exit 2 when no record of any run has a score for the no-skill arm (this covers an unjudged file and a wrong baseline name), and count items with no_skill: null apart from no_flag (for example an unmeasured count). The two tests would then expect the refusal.
| if no_skill is not None and no_skill >= ceiling - _EPS: | ||
| return CEILING | ||
| scored = {arm: v for arm, v in arms.items() if v is not None} | ||
| if any(arm != control for arm in scored) and all(v <= floor + _EPS for v in scored.values()): |
There was a problem hiding this comment.
An item is flagged floor ("no arm answers it") when the no-skill arm was never scored and the one scored skill arm is at 0.
With scores: {baseline: {aggregate: null, error: "judge failed"}, skill: {aggregate: 0}} the item gets flag: floor and reps: 0, and the summary counts floor 1. Nothing shows the baseline cannot answer it: its judge just failed. The code already refuses a floor from the baseline alone at 0, and d5113e0 now pins that the baseline must be at the floor too; an unscored baseline still passes that term because it is left out.
Suggestion: require the no-skill arm to be scored for a floor, e.g. if no_skill is not None and any(arm != control for arm in scored) and all(...), and add a test with the baseline unscored and the skill at 0.
| print(format_table(result)) | ||
| if args.output: | ||
| try: | ||
| save_artifact("headroom", result, args.output) |
There was a problem hiding this comment.
-o can point at one of the input arms files and overwrites it.
eval headroom run1/arms.json -o run1/arms.json exits 0 and replaces the arms run with the headroom artifact; its answers and evaluations are gone.
Suggestion: refuse an output path that resolves to one of the inputs (Path(args.output).resolve() against the resolved inputs) with exit 2.
| for path in args.arms: | ||
| try: | ||
| run = records.load(path) | ||
| digest = hashlib.sha256(Path(path).read_bytes()).hexdigest() |
There was a problem hiding this comment.
Repeated runs are detected by raw bytes only, so a re-serialized copy of the same run counts as another rep.
jq . run1.json > copy.json, then eval headroom run1.json copy.json --min-reps 2 gives runs: 2, and the item is no longer provisional (ceiling 1, provisional 0) with one real run behind it.
Suggestion: hash a canonical form of the parsed data (json.dumps(data, sort_keys=True)), so the same run in another layout still counts once.
| unscored = [f"{item['question_id']} ({', '.join(item['unscored_arms'])})" | ||
| for item in result["items"] if item["unscored_arms"]] | ||
| if unscored: | ||
| lines.append("note: no run scored these arms, so the floor test leaves them out: " + "; ".join(unscored)) |
There was a problem hiding this comment.
The "no run scored these arms" note puts every item with an unscored arm on one line, including items the table does not show.
When one arm failed everywhere in a 500-question run, the table is followed by one line with 500 qid (arm) entries, which buries the counts below it.
Suggestion: summarize it per arm (skill: unscored in 500 items), or list only the rows the table shows.
| def _sort_key(qid: str): | ||
| if qid.isdigit() and not qid.isascii(): | ||
| # isdigit() accepts superscripts and other digits int() refuses: name the id, do not crash. | ||
| raise ValueError(f"question id {qid!r} uses non-ASCII digits; use ASCII digits or a non-numeric id") | ||
| return (0, int(qid), qid) if qid.isdigit() else (1, 0, qid) |
There was a problem hiding this comment.
Nit: _sort_key refuses the whole analysis (exit 2) for a question id like ², only to sort it.
The comment says int() refuses such digits, but int() accepts other Unicode decimals (int('٣') == 3), and an id the records accept should not stop the command. Sorting such an id as a string is enough, and the new order test still passes with it:
return (0, int(qid), qid) if qid.isascii() and qid.isdigit() else (1, 0, qid)| help=f"Flag an item when the no-skill arm scores at least this, 0-1 (default {DEFAULT_CEILING})") | ||
| h.add_argument("--floor", type=float, default=DEFAULT_FLOOR, | ||
| help=f"Flag an item when every arm scores at most this, 0-1 (default {DEFAULT_FLOOR})") | ||
| h.add_argument("--min-reps", type=int, default=DEFAULT_MIN_REPS, dest="min_reps", |
There was a problem hiding this comment.
Nit: --min-reps uses type=int and a check in check_thresholds, while evaluate.register already has positive_int for this.
--min-reps 0 fails with Error: ... instead of argparse's usage message like the other eval flags. Suggestion: pass positive_int to headroom.register_eval and use it for --min-reps.
json.loads raises RecursionError on a deeply nested file, which the headroom command did not catch, so it ended in a traceback. Catch it with the other read errors. Thread: PRRT_kwDOVBg7g86xtTdU Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
--min-reps used type=int, so 0 reached the analysis and failed as an Error line. Pass the eval flags' positive_int, as the other eval commands do, so argparse names the flag. Thread: PRRT_kwDOVBg7g86xyyV1 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
An id like '²' passes isdigit() but int() refuses it, so the whole analysis exited 2 just to sort it. Only ASCII digit ids are read as numbers; any other id sorts as text after them. Thread: PRRT_kwDOVBg7g86xyyVx Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
-o pointing at an input arms file replaced that run with the headroom artifact, losing its answers and evaluations. Compare the resolved paths and exit 2 before anything is read into the output. Thread: PRRT_kwDOVBg7g86xyyVs Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Repeats were detected by raw bytes, so the same run written with other whitespace or key order counted as a second run and could lift an item out of provisional. Compare a digest of the parsed JSON instead. Thread: PRRT_kwDOVBg7g86xyyVu Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The note listed every 'qid (arm)' pair, so one arm that failed across a 500-question run put 500 entries on one line above the counts. Give one count per arm instead. Thread: PRRT_kwDOVBg7g86xyyVv Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
An item whose baseline judge failed was flagged floor when the skill arm scored 0, since the unscored baseline was left out of the floor test. Require the no-skill arm to have a score for the item. Thread: PRRT_kwDOVBg7g86xyyVq Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
An unjudged file, or a baseline named differently from the scores, left every item unmeasured and the command still exited 0 with the items counted as unflagged. Exit 2 when items exist and none has a no-skill score, and name the arm and the fix. Thread: PRRT_kwDOVBg7g86xyyVn Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
🔍 OpenCodeReview found 2 issue(s) in this PR. By severity: 2 medium
Review details
|
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The command skips a repeated run by the digest of its parsed JSON (see 49a4d54), so a re-serialized copy also counts once. The section said byte-identical, which understated that. Wording only; no code path changed, so no test applies. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| - **floor**: every arm that a run scored has a mean at most `--floor`, and at least one | ||
| non-baseline arm scored. Default 0. An arm no run scored is left out of this test and | ||
| listed in the item's `unscored_arms`; the baseline alone at zero is not a floor. |
There was a problem hiding this comment.
The floor definition omits a condition the code enforces. _flag in eval/headroom.py also requires the no-skill arm to be scored on the item (no_skill is not None). An item where only non-baseline arms scored 0 and the baseline was unscored is not a floor. A reader following this text would expect it to be one. The module docstring states the condition, so add it here too.
Suggestion:
| - **floor**: every arm that a run scored has a mean at most `--floor`, and at least one | |
| non-baseline arm scored. Default 0. An arm no run scored is left out of this test and | |
| listed in the item's `unscored_arms`; the baseline alone at zero is not a floor. | |
| - **floor**: the no-skill arm was scored, every arm that a run scored has a mean at most | |
| `--floor`, and at least one non-baseline arm scored. Default 0. |
| and shown rounded down, so 0.89996 prints as 0.89. Without `--no-skill-arm`, the runs must | ||
| declare the same baseline; the default is that baseline. With `--no-skill-arm`, the arm must | ||
| be one the runs list (exit 2 otherwise, naming the arms that exist), and the declared | ||
| baselines are not checked. A threshold out of range (`--ceiling` in (0, 1], `--floor` in |
There was a problem hiding this comment.
The exit-2 cases listed here are incomplete, and the PR description's own list is too. The command also exits 2 in two cases that this page and the changelog don't mention. First, when no run scored the no-skill arm at all, which is the case of unjudged arms files. Second, when -o names one of the input arms files. Add both. Also document that scores are not range-checked: 0-100 is assumed, so a score of 150 reads as a ceiling. This is listed as a known limit only in the PR description, and users won't see it there.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 95642eb. The commits since my last review close both earlier test gaps and take the points from the other reviews: the unscored-baseline floor, the -o overwrite, content-digest dedupe, the empty/unjudged refusal and the nested-JSON exit. One test gap on the -o guard is worth closing before merge; two doc nits.
What I ran, on a Linux review host at 95642eb:
make ci-local: 29 passed, 0 failed, 7 skipped (network and Docker jobs).tests/test_headroom.py: 60 passed; the run left no files in the tree.- A mutation run of 55 broken copies of the new code: 48 failed the headroom tests, 7 passed. One of the survivors decides an exit code and is inline; the others change a message or are equivalent.
- CLI runs on hand-built arms files (directory, non-UTF-8,
nan/infthresholds, string/bool aggregates, symlinked-o, mixed question sets, differing baselines). No traceback, exit codes as documented.
Checked, no issues
- Commits: all 25 non-merge commits carry
Signed-off-by. - Schema:
headroom.v1is a new kind registered inSCHEMA_VERSIONS, the CLI's-ooutput and the fixture validate, andschema_compathas nothing to widen. - Flags:
--ceiling nan/inf,--floor nanand a percent-style--ceiling 90exit 2; a baseline null in one run gives that item no floor. - Docs build:
mkdocs build --strictpasses and the changelog fragment is underAdded.
| def test_command_refuses_an_output_that_is_one_of_its_inputs(tmp_path, capsys): | ||
| path = _arms_file(tmp_path / "arms.json", {"1": {"baseline": 99, "skill": 99}}) | ||
| before = path.read_bytes() | ||
| with pytest.raises(SystemExit) as exc: | ||
| cmd_eval_headroom(_args([path], output=path)) | ||
| assert exc.value.code == 2 | ||
| assert path.read_bytes() == before | ||
| assert "overwrite" in capsys.readouterr().err |
There was a problem hiding this comment.
No test pins the path resolution of the -o overwrite guard, so it can regress to a literal string match and overwrite a run.
Mutation check: change commands/headroom.py:33 to Path(args.output) in {Path(p).resolve() for p in args.arms} (no .resolve() on the output). The full test_headroom.py still passes (60 passed). With that change eval headroom arms.json -o ./arms.json, or -o through a symlink or a sub/../arms.json spelling, exits 0 and replaces the arms run with the headroom artifact, which is the loss the guard exists to stop. The test only passes the identical absolute tmp_path object as input and output.
Suggestion: add a case where the input is given as str(tmp_path / 'arms.json') and -o as str(tmp_path / 'sub' / '..' / 'arms.json') (create sub), assert exit 2 and that the file bytes are unchanged.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
| from dokimi.artifacts import ArtifactValidationError, save_artifact | ||
| from dokimi.eval.headroom import format_table, headroom, repeated_ids | ||
|
|
||
| if args.output and Path(args.output).resolve() in {Path(p).resolve() for p in args.arms}: |
There was a problem hiding this comment.
Must-fix: path resolution happens outside the error handlers. On supported Python 3.11/3.12, ln -s loop.json loop.json followed by python cli.py eval headroom arms.json -o loop.json raises RuntimeError: Symlink loop here instead of printing Error: ... and exiting 2. An input path containing a link loop has the same problem when -o is supplied. Wrap this guard's resolution in an error handler for OSError and RuntimeError, using the existing stderr/exit-2 behavior, and add a regression test.
| if no_skill is not None and no_skill >= ceiling - _EPS: | ||
| return CEILING |
There was a problem hiding this comment.
Must-fix: the absolute 1e-9 tolerance overwhelms valid small thresholds. For an item with baseline=0 and skill=0, --ceiling 1e-10 --floor 0 passes threshold validation but reports ceiling: 0 >= 1e-10 - 1e-9 is true. The correct flag is floor. The floor comparison has the same problem for small positive scores. Use a magnitude/ULP-scaled tolerance that accommodates floating-point rounding without collapsing distinct thresholds, and add regressions for thresholds below 1e-9 while retaining the existing exact-boundary tests.
There was a problem hiding this comment.
This only matters for thresholds below 1e-9, and no headroom analysis uses those. Scores are 0-100 aggregates divided by 100, so the finest real step is 0.01. --ceiling 1e-10 would make every item a ceiling anyway. A scaled tolerance adds code for a case a judge score cannot produce. If it should be closed, it is simpler to have check_thresholds refuse a positive --ceiling or --floor below 0.01.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
I pushed a commit per nit, up to 9a191ba, which fix the two nits from my review:
2563839:dokimi/docs/evaluating-treatments.md:739: the floor bullet and the changelog fragment now say the no-skill arm must be scored on the item.9a191ba:dokimi/docs/evaluating-treatments.md:759: added the two other exit-2 cases and that scores are the 0-100 aggregate divided by 100, not range-checked.
make ci-local at 9a191ba: 29 passed, 0 failed, 7 skipped.
Tests (py3.11, py3.14) fail in test_cli_flags.py::test_every_spelling_on_the_tree_is_canonical_or_allowlisted, and the mypy job fails at CLI snapshot is current. Both checks came with #254, which merged into main after my review: the new eval headroom flags (--ceiling, --floor, --min-reps, --no-skill-arm) are not in cli_flags.py or tests/data/cli_flags_allowlist.txt, and cli-interface.json lacks the command. The fix commits touch only docs and the changelog fragment. Fix: use the registry spellings or allowlist the flags with a reason, then make cli-snapshot.
Not approving yet. Still open: the risk from my review.
vlad-perevezentsev
left a comment
vlad-perevezentsev
left a comment
There was a problem hiding this comment.
Review at 9a191ba. The fixes since my last review hold: a floor now needs the no-skill arm scored, an unjudged run or a wrong baseline name exits 2, -o cannot overwrite an input, and a re-serialized copy counts once. What is left is in the line comments; the main one is that the new flags fail main's flag registry check after #254.
What I ran, at 9a191ba:
make ci-local --offlineagainst the PR base: 0 failed, 18 passed, 13 skipped (network and Docker jobs).- This head merged with
origin/main:tests/test_cli_flags.pyfails 2 tests (flag spellings and the CLI snapshot);tests/test_headroom.pypasses. - My earlier repros again: the unjudged file and the wrong
baseline_armexit 2; a baseline that is unscored next to a skill at 0 is no longer a floor;-othrough a symlink to an input exits 2. Path.resolve()on a self-looping symlink under Python 3.12.3:RuntimeError.
Checked, no issues
- Tests for the fixes: each fix commit comes with a test for the case it fixes (the unjudged run, the wrong baseline name, the unscored-baseline floor,
-oonto an input, the re-serialized copy,--min-reps 0, nested JSON, non-ASCII ids, the per-arm note). - Docs and changelog: the floor rule, the extra exit-2 cases and the unchecked 0-100 scale now match the code.
| "headroom", | ||
| help="Flag arms items with no headroom: no-skill ceiling, all-zero floor, provisional reps") | ||
| h.add_argument("arms", nargs="+", help="arms.v1 JSON, one per repetition of the question set") | ||
| h.add_argument("--no-skill-arm", default=None, |
There was a problem hiding this comment.
After #254 the new flags fail the flag registry check and the CLI snapshot, so the PR breaks main's tests once merged.
On a merge of this head with origin/main, tests/test_cli_flags.py fails 2 tests: test_every_spelling_on_the_tree_is_canonical_or_allowlisted (--no-skill-arm, --ceiling, --floor, --min-reps) and test_the_cli_interface_snapshot_is_current. @antonwolfy noted this in their review; one addition: the registry already has a concept for this flag. baseline ("the run or arm a comparison is made against") has the canonical --baseline-arm NAME, which arms run uses for the same arm.
Suggestion: merge main, rename --no-skill-arm to --baseline-arm (docs, changelog and tests too), allowlist --ceiling, --floor and --min-reps with a reason (no registry concept for headroom thresholds), then run make cli-snapshot.
Summary
Part of #220.
eval headroomreads one or morearms.v1files of the same question set, one per repetition, and lists the items that a treatment comparison cannot learn from:It only reports. No score, gate or comparison changes. The table always goes to stdout;
-owrites aheadroom.v1JSON file. Why: a ceiling or floor item shows nothing about a skill, and one run's flag can be noise.Evidence
--no-skill-armexits 2 and lists the arms; the same file given twice counts once; a question id repeated in one file exits 2; a non-ASCII digit id is named, not a traceback; 0.89996 prints as 0.89 and is not flagged.FRESH-REVIEW-A1, on 0965221) found two blockers, both fixed in df78ff5: a mistyped--no-skill-armgave zero ceilings with exit 0, and repeated inputs counted as extra runs. Its unscored-arm nit is fixed in 050de2e. An in-session pass found the float-boundary blocker, fixed in 0965221.make ci-local --offline): ruff, schema, registry, guidance scan, schema_compat, mypy (273 files), deptry, mkdocs and the CLI-surface job (which reports the addedeval headroom) pass. Pytest: 9729 passed, 5 skipped, 1 failed:test_container_agent.py::test_probe_reports_a_writable_docker_socket(AF_UNIX path too long). The same test failed on a clean intel/main checkout in an earlier run, so it is not from this change. bandit flagged only files in a local.venvinside the package directory; with that excluded, bandit is clean.Merge Danger
eval headroomand new persisted formatheadroom.v1(schema, fixture, registry entry, widening-only compatibility). Both are new in this PR.--no-skill-arm, a question id repeated within one file, or an-othat cannot be written.-o. The table always goes to stdout.--no-skill-arm, the baselines the runs declare are not compared. Without it, the runs must declare the same baseline (documented).repscounts the runs in which the no-skill arm scored the item. Pass one file per repetition; repeated copies are skipped with a note.dokimi.eval.stats.🤖 Generated with Claude Code