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.07% +0.02%
==========================================
Files 297 299 +2
Lines 38018 38272 +254
Branches 6424 6468 +44
==========================================
+ Hits 35756 36003 +247
- Misses 1452 1457 +5
- Partials 810 812 +2
🚀 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. |
Path() drops a trailing slash, so -o newdir/ with newdir absent passed the directory check and wrote a plain file named newdir while printing "Wrote headroom artifact to newdir/". The check now also refuses a trailing separator, and "", "." and ".." by name. Red first: the absent-with-slash case wrote the file before this change. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Two gaps in the directory refusal added for -o: - "newdir/." (absent) ended in ".", not a separator, so its last component was "newdir" and the save wrote a plain file of that name. The check now reads the last component of the spelling: "", "." or ".." refuses. - is_dir() raises PermissionError under a parent without search permission, which escaped the guard as a traceback. An unstat-able path is now not a directory, and the save reports it as before. The message says "names a directory"; the exit-2 list in evaluating-treatments.md names the directory and .rejected refusals. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…ases A trailing separator on an existing regular file (-o existing.json/) was refused with "names a directory", which is false for a file. The message now says the spelling is not a file name, which holds for both. The directory test pins the wording and that an existing file is left alone, and the stat-error test asserts the save writes the file when the directory check cannot stat its path. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…he -o refusals "Every item is provisional" held for one file only: reps add up across the files of one run, so three files that score an item give reps=3 and it is not provisional. The exit-2 list also names the path-with-separator refusal. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
The CI failures named in the 9a191ba review are fixed on the branch: the flag registry check in |
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> # Conflicts: # dokimi/dokimi/artifacts.py
… row --output/-o becomes --out (the primary artifact is always a file; -o and --output are not canonical), and --no-skill-arm becomes --baseline-arm (the baseline concept's spelling). The route table lists eval headroom as free: it reads arms files and makes no model call. Snapshot regenerated. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The destination checks tested the value's truth, so "" skipped the directory and clobber checks and the save, and the command exited 0 without the artifact. They now test for None; "" is refused with exit 2. Address review: refuse an empty output through the real parser Thread: PRRT_kwDOVBg7g86yKCDW Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Exit 2 is for input and usage errors. A write that the OS refuses is a runtime failure, so eval headroom exits 1 for it, as the other commands do. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
Pushed 156c82d. What changed in this round:
The PR body and |
eval headroom is in the route table as a leaf, so the snapshot lists it. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The runs scan and show tests check every schema version has a row. headroom.v1 was added by this PR without one, so the Tests and Windows smoke jobs failed. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The exit-code table in docs/quickstart.md gives exit 2 to an output that cannot be written, and exit_on_unwritable implements it. Use it; a schema refusal of the payload stays exit 1. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
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.
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.
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