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 `--baseline-arm`
+ option `--ceiling`
+ option `--floor`
+ option `--help/-h`
+ option `--min-reps`
+ option `--out`
~ `dokimi runs list` parameters changed:
~ changed option `--kind`: choices: ["'answers'", "'arms'", "'cli_routes'", "'eval'", "'explore_runs'", "'guidance_reach'", "'matrix_rollup'", "'novelty'", "'plan'", "'plugin_delta'", "'questions'", "'repo_budget'", "'repo_environment'", "'repo_readiness'", "'result'", "'subject_scorecard'", "'task_inspect'", "'task_runs'", "'tool_preference'"] -> ["'answers'", "'arms'", "'cli_routes'", "'eval'", "'explore_runs'", "'guidance_reach'", "'headroom'", "'matrix_rollup'", "'novelty'", "'plan'", "'plugin_delta'", "'questions'", "'repo_budget'", "'repo_environment'", "'repo_readiness'", "'result'", "'subject_scorecard'", "'task_inspect'", "'task_runs'", "'tool_preference'"]Updated on every push; the |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #279 +/- ##
==========================================
+ Coverage 94.05% 94.09% +0.04%
==========================================
Files 297 299 +2
Lines 38018 38366 +348
Branches 6424 6496 +72
==========================================
+ Hits 35756 36102 +346
- Misses 1452 1453 +1
- Partials 810 811 +1
🚀 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. |
Inputs count as repetitions, but runs of different models, judges, run configs or libraries were averaged per question id and lifted the provisional flag. Compare report.model_compare.replicate_key and the library, as model compare does, and exit 2 on a mismatch. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
|
| or library: averaging such runs per question id would mix different measurements.""" | ||
| from dokimi.report.model_compare import replicate_key | ||
|
|
||
| keys = [(replicate_key(dict(run.meta)), run.meta.get("library_name")) for run in runs] |
There was a problem hiding this comment.
Must-fix: records.load preserves top-level metadata without validating run_config, but replicate_key calls .items() on it. For example, running eval headroom bad.json with {"baseline_arm":"baseline","run_config":"bad","evaluations":[{"question_id":"1","scores":{"baseline":{"aggregate":95}}}]} raises an uncaught AttributeError, even with only one input. This call is outside the command's error handlers, so malformed input produces a traceback rather than a diagnostic and exit 2. Validate that each non-null run_config is an object before computing the key, report the offending input path, and add a command regression test.
There was a problem hiding this comment.
Confirmed on 976f06a: eval headroom with an input whose run_config is a string ("bad") crashed in replicate_key with an uncaught AttributeError and a traceback, exit 1.
Fixed in ea85151. Right after each input loads, the command checks that a non-null run_config is a JSON object and otherwise exits 2 with Error: <path>: run_config must be a JSON object, not str (the path names the offending input). Regression test test_command_exits_2_when_run_config_is_not_an_object covers a string, a number and a list; it failed before the change and passes after.
There was a problem hiding this comment.
Fixed in aa49e53 (a non-object run_config exits 2) and eabdcda (any top-level arms or run_config of the wrong shape exits 2, naming the file and field). Tests: test_command_exits_2_when_top_level_metadata_has_the_wrong_shape, test_command_exits_2_when_run_config_is_not_an_object. Head c574718. The earlier reply citing ea85151 is not on this branch.
ea85151 to
aa49e53
Compare
|
Correction to my reply on the |
records.load keeps the top-level metadata as is, and replicate_key calls .items() on run_config, so a scalar or list run_config crashed eval headroom with a traceback. The command now exits 2 and names the input. The check is a helper, which keeps cmd_eval_headroom under the complexity cap of the growth ratchet. Thread: PRRT_kwDOVBg7g86yOcdc Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| or library: averaging such runs per question id would mix different measurements.""" | ||
| from dokimi.report.model_compare import replicate_key | ||
|
|
||
| keys = [(replicate_key(dict(run.meta)), run.meta.get("library_name")) for run in runs] |
There was a problem hiding this comment.
Must-fix: validate top-level arms before calling replicate_key. For example, an input containing {"baseline_arm":"baseline","arms":5,"evaluations":[{"question_id":"1","scores":{"baseline":{"aggregate":95}}}]} passes records.load, which preserves the malformed metadata, but replicate_key executes tuple(5) and raises an uncaught TypeError. Even a single input therefore produces a traceback instead of the expected exit-2 diagnostic. Extend the metadata validation to reject a non-null arms value that is not a list of arm names, and add a command regression test.
| first_seen[digest] = str(path) | ||
| runs.append(run) | ||
| inputs.append(str(path)) | ||
| _exit_unless_replicates(runs, inputs) |
There was a problem hiding this comment.
Must-fix: replicate_key does not include judge_context, so this check accepts runs scored under different judge protocols. For example, two otherwise identical scored arms files, one with judge_context: "treatment_material.v1" and one without that field, pass this guard and have their scores averaged. The latter judge saw only report snippets, not the stored treatment material, so these are different measurements; report/model_compare.py::_check_judge_context explicitly refuses this pairing. Check the protocol stamps here as well, including refusing to pool treatment_material.v1+snippet_fallback runs, and add a CLI regression expecting exit 2 for incompatible protocols.
There was a problem hiding this comment.
Fixed in c574718: judge_context joins the replicate key, so differently stamped runs are refused as not repetitions, and a mixed-protocol input exits 2 (the rule of report.model_compare._check_judge_context). Tests: test_command_exits_2_when_runs_were_judged_under_different_contexts, test_command_exits_2_on_a_mixed_judge_context.
…hape replicate_key reads run_config as a mapping and arms as a list, so an arms file with "arms": 5 (or a scalar run_config) crashed eval headroom with a traceback. The command now checks both fields before grouping and exits 2, naming the file and the field. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
… why replicate_key calls .items() on run_config and tuple() on arms, so a wrong type there crashes it, or a string or object arms is silently split. The comment names both, instead of calling every wrong shape a traceback. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…droom replicate_key does not read judge_context, so runs judged on the stored treatment material and runs judged on report snippets were averaged as repetitions. judge_context now joins the replicate key, and a run with the mixed protocol exits 2, as report.model_compare already refuses it. A list of arms holding a non-string now exits 2 too. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
@xaleryb mapping your items (head c574718):
|
| first_seen[digest] = str(path) | ||
| runs.append(run) | ||
| inputs.append(str(path)) | ||
| _exit_unless_replicates(runs, inputs) |
There was a problem hiding this comment.
Must-fix: matching configuration metadata does not establish that shared question IDs refer to the same questions. For example, give two otherwise identical arms files question_id "1", but question_text "What is X?" with baseline aggregate 100 in one and "What is Y?" with baseline aggregate 0 in the other. This guard accepts both, and headroom pools them into one item with mean 0.5 and two reps, hiding the first question's ceiling. Before pooling, reject conflicting non-empty question text (and available ground truth) for shared IDs, naming the input and ID; add a command regression test. Partial runs can still be allowed without requiring identical ID sets.
There was a problem hiding this comment.
Fixed in 6699e21. eval headroom now exits 2 when two inputs give one question id a different non-empty question text or ground truth, and names the id and both files. Partial runs with different id sets still pool.
Tests: the conflict case (red without the fix) and a partial-run pooling check that asserts the pooled reps.
vlad-perevezentsev
left a comment
vlad-perevezentsev
left a comment
There was a problem hiding this comment.
Review at c574718. The flag registry point is fixed: 6399536 takes the registry's --baseline-arm and --out, and the branch merges cleanly into current main. One point from my first round is still partly open, and I replied in its thread: an item whose no-skill arm is unscored still counts as unflagged.
Agreed with @exai-bot: files that share a question id but not its text are pooled into one item.
What I ran, at c574718:
make ci-local --offline: 0 failed, 19 passed, 13 skipped (network and Docker jobs).- Two arms files with the same config, seeds 1 and 2, and question 1 worded differently (baseline 100 in one, 0 in the other) give one item at 0.5 with 2 reps and no flag.
- A file where item 2's baseline is unscored:
unflagged 1for that item.--out ""exits 2.
Checked, no issues
- Repetition guard: 976f06a and c574718 refuse files whose model, judge, judge context, run config other than the seed, harness, arms or library differ. Runs that differ only in seed still pool. eabdcda refuses a
run_configorarmsof the wrong shape beforereplicate_keyreads them. - Exit codes: an unwritable
--outexits 2 throughexit_on_unwritable(97f0f7a); a schema refusal of our own payload exits 1. The docs say the same. - Earlier rounds: the
--outguard (inputs reached through.., a symlink, the.rejectedsibling, a directory, an empty name), the 0.01 threshold floor,with_docs-named arms and long digit ids all hold, each with a test.
…tions Two arms files that share a question id but not its question text (or ground truth) were pooled into one item, so a ceiling question could hide behind a question with no headroom. The command now exits 2 and names the id and the two inputs. Partial runs with different id sets still pool. Thread: PRRT_kwDOVBg7g86yUIUh Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| @@ -0,0 +1,3 @@ | |||
| ### Added | |||
|
|
|||
| - **`eval headroom` flags arms items with no headroom** — reads one or more `arms.v1` files of the same question set and flags an item as a `ceiling` when the no-skill arm's mean is at least `--ceiling` (default 0.9, on 0-1), or as a `floor` when the no-skill arm was scored, every arm that a run scored has a mean at most `--floor` (default 0), and at least one arm other than the no-skill arm scored. An item with fewer than `--min-reps` runs (default 3) behind it is `provisional`. The human table goes to stdout; `--out` writes a `headroom.v1` artifact with every item. The flags change no score. Inputs that are not repetitions of one configuration (model, judge, run config other than the seed, harness, arms or library differ) exit 2. | |||
There was a problem hiding this comment.
Nit: specify that only runs scoring the item's no-skill arm count toward --min-reps. With three files containing an item but only one scoring its baseline, the implementation reports one rep and keeps it provisional (covered by test_reps_follow_the_baseline_even_when_a_skill_arm_was_scored_in_more_runs). Replace “runs ... behind it” with “runs ... scoring its no-skill arm” to match the detailed documentation.
There was a problem hiding this comment.
Fixed in 02d5d91. The changelog now says the runs scoring the no-skill arm count toward --min-reps, as the detailed docs do.
| keys = [(replicate_key(dict(run.meta)), run.meta.get("library_name"), run.meta.get("judge_context")) | ||
| for run in runs] |
There was a problem hiding this comment.
Must-fix: this checks arm names but not the recorded profile prompt identity. Two files with identical configuration and questions, both listing profile:x, but different answers[].arms['profile:x'].metadata.profile_prompt_sha256 values pass this guard and have their scores averaged. This occurs when a profile is edited without changing its frontmatter ID (AgentProfileTreatment retains the name). It pools different treatments and can hide a floor or, with --baseline-arm profile:x, alter the ceiling verdict. report.model_compare already explicitly rejects this case using prompt_digests; apply an equivalent check to each pooled arm, including nested combined-treatment metadata, and add a regression test.
There was a problem hiding this comment.
Fixed in 02d5d91. Pooling now refuses an arm whose recorded profile_prompt_sha256 differs between inputs or within one, for combined arms too, using model_compare.prompt_digests.
Test: test_command_refuses_a_profile_prompt_that_changed_under_the_same_arm_name.
There was a problem hiding this comment.
🟡 Changes recommended
Headroom results can hide floors or ceilings by pooling incompatible runs and counting cached replays as independent measurements.
8 open issues
What changed in this PR
9 new findings.
This PR adds report-only eval headroom analysis, supporting the measurement work in #220 by identifying questions with ceiling scores, floor scores, or too few repetitions.
Changes:
- Adds configurable headroom analysis and a CLI table without changing scores or gates.
- Introduces the
headroom.v1artifact, schema, and run-discovery integration. - Adds input validation, regression tests, CLI reference updates, and usage documentation.
| File | Description |
|---|---|
dokimi/tests/test_runs_show.py |
Adds expected headroom artifact counts. |
dokimi/tests/test_runs_scan.py |
Covers headroom artifact discovery. |
dokimi/tests/test_headroom.py |
Tests analysis, CLI behavior, validation, and output safety. |
dokimi/tests/fixtures/artifacts/headroom.v1.json |
Provides a representative headroom artifact. |
dokimi/tests/data/cli_flags_allowlist.txt |
Registers the new threshold flags. |
dokimi/dokimi/schemas/headroom.v1.json |
Defines the persisted report format. |
dokimi/dokimi/runs/kinds.py |
Adds headroom counts to run inspection. |
dokimi/dokimi/eval/headroom.py |
Implements scoring aggregation, flags, and table formatting. |
dokimi/dokimi/commands/headroom.py |
Implements input checks, deduplication, and report output. |
dokimi/dokimi/commands/evaluate.py |
Registers the new evaluation command. |
dokimi/dokimi/cli_routes.py |
Classifies headroom as a free command. |
dokimi/dokimi/artifacts.py |
Registers the headroom schema version. |
dokimi/docs/reference/cli-interface.json |
Records the new command and artifact option. |
dokimi/docs/evaluating-treatments.md |
Documents thresholds, repetitions, outputs, and limits. |
dokimi/changelog.d/20261028_120000_napetrov_feat-eval-headroom.md |
Announces the new report-only analysis. |
Review effort: Balanced
| keys = [(replicate_key(dict(run.meta)), run.meta.get("library_name"), run.meta.get("judge_context")) | ||
| for run in runs] |
There was a problem hiding this comment.
Fixed in 02d5d91. A file scored by eval reference next to a judged one is refused (exit 2), since the rescored file keeps the judge's metadata but is a different measurement.
Test: test_command_refuses_a_reference_scored_file_pooled_with_a_judged_one.
| from dokimi.report.model_compare import replicate_key | ||
|
|
||
| keys = [(replicate_key(dict(run.meta)), run.meta.get("library_name"), run.meta.get("judge_context")) | ||
| for run in runs] |
There was a problem hiding this comment.
Fixed in 02d5d91, same check as the profile-prompt reply above: each arm's digests are compared across inputs, combined arms included, and a conflict exits 2.
Test: test_command_refuses_a_profile_prompt_that_changed_under_the_same_arm_name.
| for name, value in (("question text", rec.question_text), ("ground truth", rec.ground_truth)): | ||
| if not value: | ||
| continue |
There was a problem hiding this comment.
Fixed in 02d5d91. A run that records grounded_questions is compared per question id: grounded in one input and ungrounded in another exits 2. A legacy run with no such count has unknown grounding and is not compared.
Test: test_command_refuses_a_grounded_question_judged_ungrounded_in_another_run.
| for path in args.arms: | ||
| try: | ||
| run = records.load(path) | ||
| digest = _content_digest(path) |
There was a problem hiding this comment.
Pending the maintainer's decision. Replay is not recorded in arms artifacts: the replayed flag lives only in the in-process usage record, so headroom cannot tell a cached repetition from a fresh one. Fixing it needs replay provenance stored per answer, which is an arms.v1 schema change. Until then the repeats count as runs, as before.
| data = json.loads(Path(path).read_text(encoding="utf-8")) # records.load has parsed it already | ||
| return hashlib.sha256(json.dumps(data, sort_keys=True).encode("utf-8")).hexdigest() |
There was a problem hiding this comment.
Fixed in 02d5d91. The digest maps integral floats to ints before hashing, so 95 and 95.0 are one run; booleans and strings are not converted.
Test: test_a_copy_with_an_integral_score_written_as_a_float_counts_once.
| if ceiling < _MIN_THRESHOLD: | ||
| raise ValueError(f"ceiling must be at least {_MIN_THRESHOLD}, not {ceiling!r}") | ||
| if 0.0 < floor < _MIN_THRESHOLD: | ||
| raise ValueError(f"floor must be 0 or at least {_MIN_THRESHOLD}, not {floor!r}") |
There was a problem hiding this comment.
Fixed in 02d5d91. The minimum positive threshold is now 1e-6 (far below a score step, far above the float tolerance), so --floor 0.005 is accepted and the docs say so.
Test: test_command_accepts_a_threshold_below_one_score_step.
| f"run {index} lists question_id {dupes[0]!r} more than once; " | ||
| "a run holds each question once (a repeat would count as a second run)") | ||
| for rec in run: | ||
| names = _listed_arms(rec) | set(rec.scores) |
There was a problem hiding this comment.
Fixed in 02d5d91. Each item now lists every arm its run declares, so a declared arm the item lacks shows in unscored_arms and in the table note.
Test: test_an_arm_the_run_declares_but_an_item_lacks_is_unscored_for_that_item.
|
|
||
| def _fmt(value: Optional[float]) -> str: | ||
| # Rounded down, with the flag's tolerance: 0.89996 shows 0.89, an exact 0.9 shows 0.90. | ||
| return "-" if value is None else f"{math.floor((value + _EPS) * 100) / 100:.2f}" |
There was a problem hiding this comment.
Fixed in 02d5d91. Values are floored to six decimals with trailing zeros dropped, so 0.9049 prints below a 0.905 ceiling and 0.00004 prints non-zero.
Test: test_a_threshold_below_one_score_step_splits_scores_the_table_shows_exactly.
… profile prompt Runs scored by eval reference, judged with and without ground truth, or run with a changed profile prompt under the same arm name are different measurements; they are now refused rather than averaged. Declared arms an item lacks are listed as unscored. Integral floats hash as their int. Positive thresholds below one score step are accepted, and the table shows values floored to six decimals so no value reads as a threshold. Also the changelog wording on the reps and the docs. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| id judged grounded in one file and ungrounded in another (a file with no `grounded_questions` count is not | ||
| compared). With `--baseline-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 exits 2: `--ceiling` in (0, 1], |
There was a problem hiding this comment.
Nit: the documented threshold ranges omit the implemented minimum positive threshold of 1e-6. --ceiling 0.0000001 and --floor 0.0000001 are within the stated ranges but exit 2 in check_thresholds. Document ceiling in [1e-6, 1] and floor as zero or in [1e-6, ceiling), so the CLI contract matches validation.
There was a problem hiding this comment.
Fixed by d363baf (Anton Volkov's commit): the threshold ranges now give the 1e-6 minimum, ceiling in [1e-6, 1] and floor 0 or in [1e-6, ceiling). That matches check_thresholds.
| keys = [(replicate_key(dict(run.meta)), run.meta.get("library_name"), run.meta.get("judge_context")) | ||
| for run in runs] |
There was a problem hiding this comment.
Must-fix: the arm comparison only uses the top-level arms field through replicate_key. Supported legacy/wrapped runs without that field therefore pass even when their actual treatments differ. For example, two files with baseline_arm: "baseline" and one evaluation for question "1", the first scoring {baseline: 100, "skill:a": 100} and the second {baseline: 0, "skill:b": 0}, are pooled successfully instead of exiting 2; the resulting baseline mean is 0.5 and hides the first configuration's ceiling. Compare the effective arm identities as well (using observed conditions when no arm list is declared), and add a regression for differing legacy arm sets.
There was a problem hiding this comment.
Fixed in 998f1f5: a run with no top-level arms is now keyed by the arms its records show, so two legacy files whose arms differ exit 2. Red before the fix: test_command_refuses_legacy_runs_without_a_top_level_arms_list_whose_arms_differ.
| def _exit_on_mixed_scoring(runs, inputs) -> None: | ||
| """Exit 2 when some inputs were scored by ``eval reference`` and others by the judge. A rescored file keeps | ||
| the judge's metadata, so it passes the replicate check, yet its scores are a different measurement.""" | ||
| kinds = ["reference" if "reference_scoring" in run.meta else "judge" for run in runs] |
There was a problem hiding this comment.
Must-fix: two reference-scored files are treated as the same measurement regardless of their grading configuration. For example, score otherwise identical arms runs against two question files with the same id/text but different grading targets (baseline answer x, exact target x versus y). Both inputs have the same replicate key and kind here, and the question checks also pass, so scores 100 and 0 pool to 0.5 and hide a ceiling. Validate the reference grading identity as well as the scorer kind; at minimum refuse different recorded reference_scoring.questions sources, and use a grading-content digest to detect edits at the same path.
There was a problem hiding this comment.
Fixed in 998f1f5: two reference scorings must share their scorer and questions file (per-run tallies are left out), else exit 2. Red before: test_command_refuses_reference_scored_runs_graded_against_different_question_files.
Open decision: an edit at the same questions path is still not caught. Catching it needs eval reference to stamp a digest of the questions file (an arms.v1 change). Should I do that in a follow-up, or keep the path check?
There was a problem hiding this comment.
Maintainer chose option (b): a follow-up, not this PR. Issue: #356 (stamp a digest of the questions file in reference_scoring, and compare it in headroom).
No change in this PR: the same-path check stays as it is.
| """Per arm of ``run``, the profile prompt digests its records stamp (none for an arm without a profile).""" | ||
| from dokimi.report.model_compare import prompt_digests | ||
|
|
||
| return {arm: prompt_digests(data, arm) for arm in run.conditions} |
There was a problem hiding this comment.
Must-fix: records.load accepts a bare record list, but prompt_digests calls run_data.get(...), so this raises an uncaught AttributeError for input such as [{"question_id":"1","scores":{"baseline":{"aggregate":100},"skill":{"aggregate":100}}}], even with --baseline-arm baseline. Either extract prompt pins from the parsed Run or explicitly reject non-object input with a diagnostic and exit 2; add a command-level regression test so malformed/unsupported input does not produce a traceback.
There was a problem hiding this comment.
Fixed in 998f1f5: a top-level JSON array exits 2 ("expected a JSON object"), and answer metadata that is not an object, or treatments that is not an array, exits 2 naming the file. Red before: test_command_exits_2_on_a_bare_record_list_instead_of_a_traceback and the metadata-shape cases.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 02d5d91. The pooling guards cover most of the cases the earlier rounds named, but a bare record list still ends in a traceback, two kinds of run are pooled that should not be, and three exit-code paths have no test. One doc nit.
What I ran, on a Linux review host at 02d5d91:
make ci-local: 28 passed, 2 failed, 7 skipped; 12,077 tests passed, 59 skipped; changed-line coverage step passes. The two failures are the gitleaks install and scan steps, which did not run because the tool could not be downloaded; the PR does not touch them.- A mutation run of 48 broken copies of the changed code against
tests/test_headroom.py: 12 passed the 102 tests; 4 of them decide an exit code and are inline. - CLI runs on hand-built arms files (bare list, legacy files with different arms, two
eval referencefiles, answer metadata of the wrong type, grounded/ungrounded mixes,95vs95.0, huge and non-finite numbers), with the LLM transport never reached.
Open bot points I confirm:
- I agree with Copilot that a cached replay of an arms run counts as an independent repetition; the author says arms.v1 does not record replays and holds it for a decision.
Checked, no issues
- Commits: all non-merge commits carry
Signed-off-by. - Pooling guards: differing question text or ground truth, grounded vs ungrounded, judged vs reference-scored, a changed profile prompt across files, a mixed
judge_contextand a wrong-shapedarmsorrun_configall exit 2 with a message. - Table:
_fmtfloors to six decimals: 0.9049 prints 0.9049, 0.89996 prints 0.89996, 0.00004 prints 0.00004; with--ceiling 0.905, 90.5 is flagged and shown as 0.905. - Schema and registry:
headroom.v1is a new kind inSCHEMA_VERSIONS; the CLI snapshot listseval headroomwith--baseline-armand--out; no growth-baseline change is needed.
Bot points I checked that no longer apply at this head (19)
- exai-bot's point on the floor definition does not apply: the docs and changelog now say the no-skill arm must be scored.
- exai-bot's point on a symlink loop in the
--outguard does not apply:_same_filecatchesOSErrorandRuntimeErrorand has a test. - exai-bot's point on the 1e-9 tolerance against tiny thresholds does not apply: positive thresholds below 1e-6 are refused, so the tolerance cannot swallow one.
- exai-bot's point on the documented threshold ranges does not apply: the ranges were rewritten to match
check_thresholds; the 1e-6 minimum is a separate nit inline. - exai-bot's point on the
.rejectedsibling of--outdoes not apply:_clobbers_an_inputchecks it and a test pins it. - exai-bot's point on
with_docsandwithout_docsarms does not apply:_listed_armskeeps them and a test pins it. - exai-bot's point on digit question ids past the int limit does not apply:
_sort_keyno longer callsint(). - exai-bot's point on a non-object
run_configdoes not apply: it exits 2 and names the file. - exai-bot's point on a non-list
armsdoes not apply: it exits 2 and names the file. - exai-bot's point on
judge_contextin the pooling check does not apply: it joins the replicate key and a mixed stamp exits 2. - exai-bot's point on shared ids with different question text does not apply: it exits 2 and names the id and both files.
- exai-bot's point on the profile prompt identity does not apply: a differing
profile_prompt_sha256across files exits 2. - Copilot's point on reference-scored runs pooled with judged runs does not apply: a mix exits 2 (the grading content is the separate inline point).
- Copilot's point on conflicting prompt digests does not apply: a differing digest across files exits 2 for every arm.
- Copilot's point on grounded against ungrounded questions does not apply: a mismatch exits 2 when the runs record
grounded_questions. - Copilot's point on
95against95.0does not apply: integral floats hash as ints and a test pins it. - Copilot's point on the 0.01 minimum threshold does not apply: the minimum is now 1e-6.
- Copilot's point on declared but missing arms does not apply: each item lists every arm its run declares.
- Copilot's point on the displayed score against the threshold does not apply: values are floored to six decimals; I checked 0.9049 and 0.89996.
| """Per arm of ``run``, the profile prompt digests its records stamp (none for an arm without a profile).""" | ||
| from dokimi.report.model_compare import prompt_digests | ||
|
|
||
| return {arm: prompt_digests(data, arm) for arm in run.conditions} |
There was a problem hiding this comment.
A bare record list, or an answer whose metadata is not an object, ends in a traceback instead of exit 2.
records.load accepts a top-level list of records, but _prompt_pins hands the raw list to prompt_digests (report/model_compare.py), which calls .get on it.
eval headroom bare.json --baseline-arm baselinewith[{"question_id":"1","scores":{"baseline":{"aggregate":100},"skill":{"aggregate":100}}}]raisesAttributeError: 'list' object has no attribute 'get'.- An answer with
arms.skill.metadataset to["a"]or"zz"raises the sameAttributeErrorfrommodel_compare.py(metadata.get("treatments")). - Other wrong top levels (string, number, null, bool) exit 2 cleanly, so only this path is open. It comes from the latest commit.
Suggestion: build the pins from the parsed Run records, or skip _prompt_pins and exit 2 with Error: <path>: expected a JSON object when data is not a dict, and catch AttributeError around the digest read for the metadata case. Add a command test for each input.
There was a problem hiding this comment.
Fixed in 998f1f5, same change as the bare-list thread above: a list top level and the metadata shapes you listed (list, string, treatments as a number) now exit 2 with the file named, no traceback. Tests cover each input.
| from dokimi.report.model_compare import replicate_key | ||
|
|
||
| keys = [(replicate_key(dict(run.meta)), run.meta.get("library_name"), run.meta.get("judge_context")) | ||
| for run in runs] |
There was a problem hiding this comment.
Legacy files without a top-level arms are pooled even when their arms differ.
replicate_key reads only meta["arms"]. Two files with baseline_arm: baseline and one question each, the first scoring {baseline: 100, skill:a: 100} and the second {baseline: 50, skill:b: 50}, exit 0 and print one item with reps 2, no-skill 0.75, skill:a 1.00, skill:b 0.50. The baseline mean mixes two treatment sets, and a ceiling can hide.
Suggestion: when meta["arms"] is absent, add the observed run.conditions to the key and refuse a mismatch with the same message. Add a test with two legacy files whose skill arms differ.
There was a problem hiding this comment.
Fixed in 998f1f5, same change as the legacy-arms thread above: legacy runs are compared by their observed arms, and a mismatch exits 2. Test: test_command_refuses_legacy_runs_without_a_top_level_arms_list_whose_arms_differ.
| def _exit_on_mixed_scoring(runs, inputs) -> None: | ||
| """Exit 2 when some inputs were scored by ``eval reference`` and others by the judge. A rescored file keeps | ||
| the judge's metadata, so it passes the replicate check, yet its scores are a different measurement.""" | ||
| kinds = ["reference" if "reference_scoring" in run.meta else "judge" for run in runs] |
There was a problem hiding this comment.
Two eval reference files with different grading are pooled as one measurement.
_exit_on_mixed_scoring tests only whether the reference_scoring key exists. Two otherwise identical files with reference_scoring: {"source": "x1", "grading": "exact"} (score 100) and {"source": "x2", "grading": "fuzzy"} (score 50) exit 0 and print reps 2, no-skill 0.75.
Suggestion: compare the recorded reference_scoring value (canonicalized) across the inputs and exit 2 when it differs. Add a test with two differing values.
There was a problem hiding this comment.
Fixed in 998f1f5, same change as the reference thread above: the scorer and questions file must match across reference-scored inputs, else exit 2. Test: test_command_refuses_reference_scored_runs_graded_against_different_question_files. The same-path edit question is open, see that thread.
| seen: dict[str, tuple[str, str]] = {} # arm -> (digest, first input) | ||
| for pinned, path in zip(pins, inputs, strict=True): | ||
| for arm, digests in pinned.items(): | ||
| for digest in sorted(digests): |
There was a problem hiding this comment.
No test pins that a profile prompt changing within one input exits 2.
Mutation check: change for digest in sorted(digests): to for digest in sorted(digests)[:1]:. All 102 tests of test_headroom.py still pass. The docstring promises a conflict "between inputs, or within one"; with the mutant a single file whose skill records stamp two different profile_prompt_sha256 values is read as one treatment and exits 0. The prompt tests only use one digest per file.
Suggestion: add a case with one arms file of two questions whose skill records carry different digests, and assert exit 2 with ran with a different profile prompt.
There was a problem hiding this comment.
Added in 998f1f5: test_command_refuses_a_profile_prompt_that_changes_between_two_questions_of_one_file. With the [:1] mutant it fails; on the fix it passes.
| save_artifact("headroom", result, args.output) | ||
| except ArtifactValidationError as e: # a payload the schema refuses is an internal failure: exit 1 | ||
| print(f"Error: {e}", file=sys.stderr) | ||
| sys.exit(1) |
There was a problem hiding this comment.
No test pins exit 1 for a payload the schema refuses.
Mutation check: change sys.exit(1) to sys.exit(2) after except ArtifactValidationError. All 102 tests still pass, and the file never references ArtifactValidationError. The exit-1 versus exit-2 split is documented and is what a caller acts on.
Suggestion: monkeypatch dokimi.artifacts.save_artifact to raise ArtifactValidationError and assert exit 1 with Error: on stderr.
There was a problem hiding this comment.
Added in 998f1f5: test_command_exits_1_when_the_payload_fails_the_artifact_schema. With sys.exit(1) changed to 2 it fails.
| raise ValueError(f"ceiling must be in (0, 1], not {ceiling!r}") | ||
| if not 0.0 <= floor < ceiling: | ||
| raise ValueError(f"floor must be in [0, ceiling), not {floor!r} (ceiling {ceiling!r})") | ||
| if ceiling < _MIN_THRESHOLD: |
There was a problem hiding this comment.
The exact _MIN_THRESHOLD boundary is not pinned.
Mutation check: change ceiling < _MIN_THRESHOLD to <= (line 77), or 0.0 < floor < _MIN_THRESHOLD to <= (line 79). Both pass all 102 tests, and --ceiling 1e-6 or --floor 1e-6 would then exit 2, although the docs accept a threshold of that size.
Suggestion: add check_thresholds(1e-6, 0.0, 1) and check_thresholds(1.0, 1e-6, 1) as accepted, and check_thresholds(1e-7, 0.0, 1) and check_thresholds(1.0, 5e-7, 1) as ValueError.
There was a problem hiding this comment.
Added in 998f1f5: test_the_minimum_positive_threshold_is_accepted_and_just_below_it_refused. With <= at either _MIN_THRESHOLD check it fails.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
| # reads as the threshold: 0.89996 shows 0.89996, an exact 0.9 shows 0.90, 0.00004 shows 0.00004. | ||
| if value is None: | ||
| return "-" | ||
| whole, _, frac = f"{math.floor((value + _EPS) * 1e6) / 1e6:.6f}".partition(".") |
There was a problem hiding this comment.
Must-fix: downward truncation protects the ceiling comparison but misrepresents the floor comparison. With baseline and skill aggregates both 30.00004, floor=0.3 and the default min_reps, the provisional row displays both means as 0.30 with no flag, although the footer says floor <= 0.3. The actual means (0.3000004) correctly fail the floor test. Preserve additional digits near either threshold (or use threshold-aware formatting) so an above-floor score does not display as equal to the floor; add a regression alongside the below-ceiling display tests.
There was a problem hiding this comment.
Fixed in 998f1f5: the table prints nine decimals (floored), so 0.3000004 shows as 0.3000004 and no longer as the 0.30 floor. Test: test_a_mean_just_above_the_floor_is_not_shown_as_the_floor.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
I pushed a commit per nit, up to d363baf, which fix the nit from my review:
d363baf:dokimi/docs/evaluating-treatments.md:767: the threshold ranges now read--ceilingin [1e-6, 1] and--floor0 or in [1e-6, ceiling), ascheck_thresholdsenforces.
make ci-local at d363baf: 28 passed, 2 failed (gitleaks install and scan, host), 7 skipped.
Not approving yet. Still open: the bug and the five risks from my review; Copilot on cached (replayed) repetitions count as independent runs; the author holds it for a maintainer decision (arms.v1 change).
…rmed input - A run without a top-level arms list is compared by the arms its records show. - Two reference scorings graded against a different scorer or questions file are refused. - A bare record list, or answer metadata of the wrong shape, exits 2 with a diagnostic, not a traceback. - The table keeps nine decimals, so 0.3000004 is no longer shown as the 0.30 floor. - Tests pin the profile prompt check within one file, the exit 1 path and the minimum threshold. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| in its own column, a note naming any arm no run scored, and the counts. The JSON goes to | ||
| `--out` only, as a `headroom.v1` artifact with every item; if that file cannot be written the | ||
| command prints `Error: cannot write ...` on stderr and exits 2, after the table. Scores are compared unrounded | ||
| and shown rounded down to six decimals, so 0.89996 prints as 0.89996 and a value just under a threshold never |
There was a problem hiding this comment.
Nit: the documented precision differs from the implementation. _fmt floors to nine decimal places, and test_headroom.py explicitly expects 0.3000004 to remain visible (seven decimal places). Change “six decimals” to “up to nine decimal places” so the output contract matches the code.
Suggestion:
| and shown rounded down to six decimals, so 0.89996 prints as 0.89996 and a value just under a threshold never | |
| and shown rounded down to up to nine decimal places, so 0.89996 prints as 0.89996 and a value just under a threshold never |
There was a problem hiding this comment.
Fixed in 61b942d. The sentence now says rounded down to up to nine decimal places, as _fmt floors. test_headroom keeps 0.3000004 (seven places), so no test change.
Keeps the reason (floored, never up) and the float-error note; the examples were the third line that check_comment_blocks rejects. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
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;
--outwrites aheadroom.v1JSON file. Why: a ceiling or floor item shows nothing about a skill, and one run's flag can be noise.Evidence
--baseline-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--baseline-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.judge_contextis refused on its own), an unknown--baseline-arm, a question id repeated within one file, a file where no run scored the no-skill arm, an--outthat cannot be written, names a directory, or is one of the inputs, and any other usage error (for example a threshold out of range). 1 only when--outis refused by the schema (an internal failure).--out. The table always goes to stdout.--baseline-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