Repository navigation
fix(answers,pipeline,judge,gate): Stack P fixes, step 2 of #228 - #274
Conversation
…tep 2) The answerer stored top_k and never sent it, so retrieval asked the doc source for its default max_results. It now sends max_results=top_k, the same request DocTreatment makes on Stack A. With the default top_k of 3 the reranker sees 3 raw docs, not 5, so with-docs answers and their judge scores move. Flips the PINNED rows D1 retrieval.passed and D1 retrieval.max_results. Test fakes that take the doc-source call now accept max_results. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…ntel#228 step 2) benchmark run had no judge temperature flag, so its judge always ran at the provider default while evaluate and arms run took one. The flag is added (default unset, so runs without it are unchanged) and reaches the EvaluationPipeline judge. Flips the PINNED row D6 cli.judge_temperature_flag. benchmark batch does not get the flag yet. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…#228) EvaluationPipeline took the library id from --context7-id or the client's own resolver, so a product whose products.yaml declares context7_id was still looked up by its registry key. Resolve it with the shared resolve_library_id_for_product helper, the same order the arms stack uses: explicit id, then the registry's context7_id, then the client. Question generation and answer generation both go through the helper. The registry is injectable (registry=) and defaults to the bundled products.yaml. Parity row D8 flips from client/resolved to registry/lib. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…el#228) When no document reached the prompt (no hit, or the reranker raised), the answerer records "[FALLBACK: No relevant documentation found]" as the with-docs answer, and the judge scored that text like a model answer. It inflated the with-docs side of the paired delta on questions the docs did not answer. The judge now leaves a context answer whose doc_source is a no-retrieval source (NO_RETRIEVAL_SOURCES) unjudged, so its with-docs score and delta are None and the question drops out of the paired statistics. The record and its retrieval_hit stay. Opt back in with judge_placeholder=True, or --judge-placeholder on benchmark run, orchestrate evaluate and eval score. The trust gate's retrieval_hit_rate read only the both-arms-scored evaluations. Unjudged misses would then vanish from it and an all-miss run would read as "not measured". It now counts every evaluation's retrieval_hit. The thresholds are unchanged. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Answerer built a self.llm client in __init__ that nothing read: the answer call goes through llm_call_with_usage, which picks the provider's client itself. Only the unsupported-provider ValueError is kept; the client construction and its imports are gone. _generate_with_docs had an "if not docs" branch that could not run, because _generate_answer_pair calls it only when retrieval returned documents. The branch is deleted with its unreachable "[No documentation retrieved]" record. No parity row changes: nothing observable on either stack moved. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…intel#228) The Stack P answers artifact and the judge's evaluation output now carry method_version, from defaults.ANSWERING_METHOD_VERSION (2). The steps before this one are version 1 and have no stamp, so a later compare can refuse to put runs of different versions side by side. compare does not read the stamp yet. answers.v1 and eval.v1 document the field. The arms artifact is not stamped in this change; the new parity row records that. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
ruff F821 flagged the annotation on _paired_deltas. The module uses `from __future__ import annotations`, so the missing name did not fail at import time and the parity tests passed without it. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…set-then-read tests (intel#228) With the llm construction removed from Answerer.__init__, test_init_openai and test_init_anthropic only set attributes and read them back, which the test quality audit flags (set_then_read). The new parametrized test checks what init does now: each supported provider is accepted. It is marked as a must-not-raise smoke test. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
) test_calls_mcp_client builds the Answerer with the default top_k, so a literal max_results=5 would pass it. The new test builds it with top_k=3 and asserts the doc source is asked for 3. On intel/main the same probe sends no max_results at all, so the test fails there. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…ons (intel#228) The methodology list and the method_version comment described the trust gate and the stamp more broadly than the code does. The score checks run over the both-arm-scored questions, so a question with no document leaves them. Nothing reads method_version yet; the comment now says so. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…fraction (intel#228) A question whose retrieval missed is no longer judged with docs, so it has no context score. The zero-score check counted only scored questions, so a run where most questions miss passed it: 4 misses of 14 gave 0.0 PASS, where intel/main gave 0.286 FAIL (the misses were judged at zero there). The check now counts a miss (no context score, a baseline score, and retrieval_hit False) as unusable, in the numerator and the denominator. The message says such rows had no document. Thresholds, min_questions and the early return are unchanged. The new fail-severity retrieval check is not added here. Regression test: tests/test_trust_gate_unjudged_misses.py. It fails on the parent commit (4 of 14 passes at 0.0) and passes with this change. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…ons once (intel#228) The methodology bullet still said the zero-score fraction leaves out questions with no document. It counts them now (c517c72). Update it. Also parse the evaluations once in evaluate_trust. The input may be a one-shot iterable, and the old code parsed it again for the retrieval check. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #274 +/- ##
==========================================
+ Coverage 94.05% 94.10% +0.05%
==========================================
Files 297 297
Lines 38018 38106 +88
Branches 6424 6438 +14
==========================================
+ Hits 35756 35858 +102
+ Misses 1452 1438 -14
Partials 810 810
🚀 New features to boost your workflow:
|
github-actions
Bot
added
the
cli-interface-change
CLI interface change detectedThis PR changes dokimi's user-facing CLI surface (commands and/or options): ~ `dokimi answers generate` parameters changed:
~ changed option `--top-k`: type: 'int' -> 'dokimi.cli.build_parser.<locals>.positive_int#faf57db5ff9f'
~ `dokimi benchmark run` parameters changed:
+ added option `--judge-placeholder`
+ added option `--judge-temperature`
~ `dokimi eval score` parameters changed:
+ added option `--judge-placeholder`
~ `dokimi evaluate` parameters changed:
+ added option `--judge-placeholder`Updated on every push; the |
vlad-perevezentsev
left a comment
vlad-perevezentsev
left a comment
There was a problem hiding this comment.
Review at 6bec891. The five Stack P fixes do what step 2 of #228 asks, and the zero-score check keeps failing the same runs as main. Not judging the placeholder has side effects outside the gate's score checks (the retrieval coverage, the diagnosis and the pipeline summary), and the registry lookup misses a custom --registry. These are in the line comments.
What I ran, at 6bec891:
make ci-local --offline: 0 failed, 17 passed, 13 skipped (network and Docker jobs).- Trust gate with 15 judged rows and 5 judge-error rows, on the base and on the head: the retrieval check goes from 100% (15/15) to "not measured".
summarise_diagnoseson 10 hits and 4 misses judged through_evaluate_answer_pair, on the base and on the head:empty_retrieval4 becomes 0 andinsufficient_data0 becomes 4.resolve_library_id_for_product("my_lib", <Context7 client>)returnsmy_libunchanged.
Checked, no issues
top_k:Answerersendsmax_results=top_k, asDocTreatmentdoes.evaluateandanswers generatepass their--top-k, and the new test pins the value at the call site.- Zero-score check: unjudged misses count in both the numerator and the denominator, so 4 of 14 misses still fail at 0.286, as on main. The new test fails on the parent commit.
- Judge temperature:
benchmark run --judge-temperaturereaches theJudgein single and multi-run mode. - Dead code:
Answerer.llmwas never read, and_generate_with_docsis only called with docs. The provider allow-list keeps the same set. - Schemas:
method_versionis an optional new property inanswers.v1andeval.v1. That widens the schemas, and old files still validate.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 6bec891. The five Stack P fixes do what step 2 of #228 asks, and the zero-score check keeps failing the same runs as the base. Three things are worth fixing before merge, all test gaps on the new code (inline). The open problems from @vlad-perevezentsev's review still hold at this head and block the merge.
I agree with @vlad-perevezentsev on the judge-error rows losing the retrieval rate, the insufficient_data diagnosis of misses, the registry lookup in benchmark run --registry, the summary averages over different questions, the unstamped --judge-placeholder runs, the 0-hit gate test and the changelog flag names. New evidence: 10 hits and 4 misses move min_delta from -17.1 (fail) to +0.0 (pass).
What I ran, on a Linux review host at 6bec891:
make ci-local: 27 passed, 1 failed, 7 skipped. The failure ispip-audit, which lists advisories onfilelockin the lock files. This PR does not touch them.- 9,705 tests passed, 4 skipped; the changed-line coverage step passes.
- Reproductions for the code findings, with the LLM transport mocked.
- A mutation run on the new code (33 mutants, 13 of which pass the related tests):
_library_id, theunjudged_missesfilter and the flag wiring inbenchmark run,evaluateandeval scoreeach have mutants that pass every test.
Checked, no issues
- Schemas:
method_versionis an optional property on an open object inanswers.v1.jsonandeval.v1.json.schema_compatagainst the base passes, and no version bump is needed. top_k: every doc-source client'sget_library_docsacceptsmax_results, so the new argument raises nowhere.- Dead code: no caller of
Answerer.llmor of_generate_with_docswith empty docs remains. - Zero-score check: 10 hits and 4 misses give 4/14 = 0.286 and fail, as on the base. Judge-error rows are excluded from the new term.
- Commits: all 12 carry
Signed-off-by; the changelog fragment is present.
|
🔍 OpenCodeReview found 4 issue(s) in this PR. By severity: 4 low
Review details
|
…rs-stack-p-step2 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> # Conflicts: # dokimi/dokimi/orchestrator/pipeline.py
The retrieval coverage floor counted every judge-error row as an unrecorded outcome, so a run with more than 20% judge errors lost the retrieval check. Thread: PRRT_kwDOVBg7g86xgwoY Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
With the no-retrieval placeholder left unjudged its delta is None, and diagnose() returned insufficient_data before it looked at the doc source. Thread: PRRT_kwDOVBg7g86xgwog Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| of one version that differ in it are told apart. A `method_version` that is not a positive | ||
| integer is refused before any judge call (`eval score` exits 2). |
There was a problem hiding this comment.
Nit: method_version: null is accepted as legacy version 1, not refused. answers_method_version() explicitly handles None, and test_a_null_stamp_is_legacy confirms that eval score proceeds. Qualify this claim so it matches the accepted input: an absent or null stamp means version 1; any other value must be a positive integer.
There was a problem hiding this comment.
Fixed in 0f8835a: an absent or null stamp is version 1; any other value must be a positive integer, or eval score exits 2.
There was a problem hiding this comment.
Addressed in fork commit 0f8835aa (PR head 7e57031d). The changelog now says: "An absent or null stamp means version 1; any other value must be a positive integer, or eval score exits 2 before any judge call."
Checked against the code at 7e57031d: answers_method_version() returns 1 for an absent or None stamp, and refuses any other non-positive-integer value with a ValueError, which eval score turns into exit 2 before the Judge is built. test_a_null_stamp_is_legacy covers the null case.
| - minimum `with_docs` average, | ||
| - average delta, | ||
| - score coefficient of variation, | ||
| - minimum evaluated question count (both arms scored; a question with no document is not judged by default, so it is not counted here), |
There was a problem hiding this comment.
Nit: qualify the no-document exclusion as the single Judge default. For example, eval panel-score on an answer with doc_source="fallback_none" and a nonempty [FALLBACK: ...] text still scores both arms: JudgePanel._score_answer_entry() only skips missing or empty answers. That row therefore counts toward the trust gate's minimum question count. This page also covers panels, so the current unqualified statement incorrectly describes their behavior.
Suggestion:
| - minimum evaluated question count (both arms scored; a question with no document is not judged by default, so it is not counted here), | |
| - minimum evaluated question count (both arms scored; the single judge leaves no-document context placeholders unscored by default, so those questions are not counted here; panel scoring still judges them), |
There was a problem hiding this comment.
Fixed in 17d5b40: the single Judge leaves out a question with no document; JudgePanel still judges an answer that is only fallback text, and the line says so.
| - average delta, | ||
| - score coefficient of variation, | ||
| - minimum evaluated question count (both arms scored; a question with no document is not judged by default, so it is not counted here), | ||
| - zero-score fraction (computed when at least one question is scored in both arms; over those paired questions plus retrieval misses with a scored baseline or a judge error, which count as unusable), |
There was a problem hiding this comment.
Nit: only retrieval misses with an unscored context arm are added as unusable. With --judge-placeholder, a miss scored 70 in the context arm and 60 in the baseline contributes no zero, whereas this wording says retrieval misses with a scored baseline count as unusable. evaluate_trust explicitly requires r.score(CONTEXT) is None for the extra rows. Qualify the added misses as unscored to describe the denominator and numerator accurately.
Suggestion:
| - zero-score fraction (computed when at least one question is scored in both arms; over those paired questions plus retrieval misses with a scored baseline or a judge error, which count as unusable), | |
| - zero-score fraction (computed when at least one question is scored in both arms; over those paired questions plus retrieval misses with an unscored context arm and either a scored baseline or a judge error, which count as unusable), |
There was a problem hiding this comment.
Fixed in 17d5b40: the zero-score rows are only retrieval misses whose context arm is unscored (with a scored baseline or a judge error).
There was a problem hiding this comment.
Addressed in fork commit 17d5b405 (PR head 7e57031d). The zero-score bullet now counts "retrieval misses with an unscored context arm and either a scored baseline or a judge error, which count as unusable", and nothing else is added.
Checked against evaluate_trust at 7e57031d: unjudged_misses takes rows with a None context score, retrieval_hit is False, and either a baseline score or an error. A miss whose context arm is scored is not added.
|
Mapping of the items in your 10-31 06:49Z comment to commits on this head (synced with main b4a0a8c in 6b5ca65):
Local: report and dashboard tests 1118 passed; ci-local static and typecheck clean. |
…rs-stack-p-step2 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
A question the context arm never answered, with no retrieval hit and a baseline score, counted as no score, so the snapshot averaged the baseline alone (0 hits gave doc_score equal to the baseline). It now counts as 0.0 for the context arm. Thread: IC_kwDOVBg7g88AAAABhdSHJQ.a Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
With no question scored in both arms, report eval printed a 0.0 arm mean, an Overall delta of +0.0 and a delta over 0 pairs. The Summary cells and the Overall delta line now show an em dash and a note that there are no complete pairs. Thread: IC_kwDOVBg7g88AAAABhdSHJQ.b Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The changelog said a non-positive-integer stamp is refused; a null or absent one is the legacy version 1 and is accepted. Thread: PRRT_kwDOVBg7g86yPRma Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The minimum question count excludes an unjudged question only for the single Judge; a panel judges a fallback-only answer. The zero-score denominator adds retrieval misses with an unscored context arm and either a scored baseline or a judge error, and no other miss. Thread: PRRT_kwDOVBg7g86yPRmb Thread: PRRT_kwDOVBg7g86yPRmc Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
No code change. The body edit (line 22: method_version 1 for an eval of legacy answers; the 14-of-14 row: retrieval_hit_rate 0.0 FAIL) is applied to the PR description. Thread: IC_kwDOVBg7g88AAAABhdSHJQ.c Thread: IC_kwDOVBg7g88AAAABhdSHJQ.d Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
With no complete pair, the note under the Summary still said the means were over 0 complete pairs. It now says there are no complete pairs and that no arm mean or delta is given. Thread: IC_kwDOVBg7g88AAAABhdSHJQ.b Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| without_score = _as_float(ev.score(BASELINE)) | ||
| if (with_score is None and ev.retrieval_hit is False and without_score is not None | ||
| and ev.error is None and (with_s is None or with_s.error is None)): | ||
| with_score = 0.0 # a retrieval miss the context arm never answered: the docs did not help |
There was a problem hiding this comment.
Must-fix: this also turns an unjudged miss into a complete pair in _arm_means, changing the reported A/B delta, not just the documentation score/status. With the new test's inputs (one actual pair at 80/60 and one unjudged miss with baseline 60), the dashboard now reports delta -20, while both eval reports report +20 over the sole judged pair; the miss's own QuestionResult.delta remains None. An all-miss run similarly acquires a -60 delta despite having no scored pairs. Keep the zero penalty separate for the documentation health score, and compute the displayed arm means/delta from actual scored pairs so an unjudged placeholder does not become a measured degradation.
There was a problem hiding this comment.
Agreed, and it also makes the dashboard and report eval disagree on the same eval file, which #286 had just aligned.
Repro at 7e57031, one eval file with one pair (80/60) and one unjudged miss (baseline 60): ResultsAggregator gives context 40.0, baseline 60.0, delta -20.0; report eval on the same file gives | **All** | 2 (1 unpaired) | 80.0 | 60.0 | **+20.0** | and "context arm is helping". The changelog says an unjudged miss "drops out of the paired statistics" and does not mention the dashboard, so a reader of either output gets the wrong idea about the other.
Suggestion: keep the 0.0 only for doc_score/status (the doc health number, as @xaleryb asked), compute avg_with_docs/avg_without_docs/avg_delta from the real pairs as before, and add a changelog line for the dashboard.
There was a problem hiding this comment.
Fixed in 60f5b09. The arm means and delta now leave an unjudged miss out, since it is no judged measurement. The dashboard matches report eval on the same file (+20 over the one complete pair). The doc score and status still count the miss as 0.
Tests: the miss case in test_dashboard.py (red on the old code), and an all-miss run now has no delta. The changelog says so too.
| if not s["unpaired"]: | ||
| return [] | ||
| if not s["valid"]: | ||
| return [f'_No complete pairs: {s["unpaired"]} unpaired questions, so no arm mean or delta is given._', ""] |
There was a problem hiding this comment.
Must-fix: the no-pairs handling only updates the Summary and overall recommendation. With the new test's two unpaired questions, the same report still prints context/baseline 0.0 and delta +0.0 in the Dynamic vs Static breakdown (including Total), and claims “+0.0 vs +0.0 — good consistency, benchmark is stable.” Difficulty rows do the same when a difficulty is present. These pre-existing zero fallbacks now contradict this new statement and the Summary's em dashes. Apply the no-pairs rendering to both breakdown tables and guard the agreement recommendation when either set lacks pairs; extend the regression test to cover these outputs.
There was a problem hiding this comment.
Fixed in ad1f45e. The Dynamic vs Static and difficulty rows print em dashes for a set with no complete pair, as the Summary does. The agreement bullet says there is no delta, instead of "good consistency".
Regression test: test_breakdown_tables_and_agreement_say_no_pair_instead_of_a_zero_delta (red on the old code).
…atchet The miss-as-zero rule and the no-pairs lines pushed _load_snapshot over the cap of 10 and generate_report past its baseline of 18. Move them into _context_score, _overall_line and _delta_summary; no behavior change. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
vlad-perevezentsev
left a comment
vlad-perevezentsev
left a comment
There was a problem hiding this comment.
Review at 7e57031. Everything from my first review is fixed and pinned by tests. The new dashboard change scores an unjudged miss as 0, which also moves the dashboard's arm means and delta. So the dashboard and report eval now disagree on the same eval file. I added a repro in @exai-bot's thread.
Agreed with @exai-bot: the dashboard's arm means and delta now count unjudged misses; with no complete pairs, the report's per-set tables and the agreement line still print +0.0.
What I ran, at 7e57031:
make ci-local --offline: 0 failed, 19 passed, 13 skipped (network and Docker jobs). The suite had 12,042 passed and 58 skipped. The branch is 0 commits behindmain.- One eval file with one pair (80/60) and one unjudged miss (baseline 60). The dashboard shows context 40.0, baseline 60.0, delta -20.0.
report evalshows 80.0, 60.0, +20.0. - Earlier rounds: mutation runs on the gate filter, the
top_koverride and the diagnoser branch. Each mutant is now killed.
Checked, no issues
- Trust gate: in
zero_score_fraction, misses with an unscored context arm count as unusable, and a judge-error miss counts too. Judge-error rows keepretrieval_hit. An old error row without that field stays out of the coverage count, after a6a74c4. With no pair, the gate still shows the retrieval rate. - Diagnosis: an unjudged miss is
empty_retrieval. A miss judged with--judge-placeholderthat has no baseline isinsufficient_data. - Stamps:
eval scoreand the pipeline stamp the eval with the answers'method_version: an absent stamp counts as 1, and a malformed one exits 2 before any judge call (21b3054, f34aa62).judge_placeholderis stamped too. - Retrieval:
top_kreaches the doc source, and a value below 1 is refused (3d7f8c5). The library id comes from--context7-id, then themcp:;id=, then the loaded--registry. The topics cache is keyed on the doc source (b945e44). - Summaries: the run summary and
report evalaverage the complete pairs and say how many questions were left out. With no pair they print no delta. - Changelog and docs: the fragment names only flags and commands that exist, and says old scores are not comparable.
methodology.mdgives the subset each gate check uses.
A no-error retrieval miss scores 0 as the context arm, so the doc score counts it, but it is no judged measurement: pairing it with its baseline made the dashboard report a -20 delta where the eval report gave +20 over the same file. The arm means and delta are now over the measured pairs; the doc health (doc_score, status) still counts the miss as 0. Thread: PRRT_kwDOVBg7g86yUBEO Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
A set with no complete pair printed 0.0 for its context, baseline and delta in the Dynamic vs Static and difficulty tables, and the agreement bullet claimed consistency. Those rows now print an em dash like the Summary, and the agreement bullet says there is no delta when either set has no pair. Thread: PRRT_kwDOVBg7g86yUBES Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
There was a problem hiding this comment.
🟡 Changes recommended
Skipping placeholders exposes a critical all-miss reporting bug that presents zero-valued statistics and concludes minimal impact despite having no paired measurements.
1 open issue
What changed in this PR
1 new must-fix finding.
This PR implements step 2 of #228, correcting Stack P’s answering and judging behavior before consolidating the two evaluation stacks.
Changes:
- Honor retrieval limits, resolve library IDs through the registry, and wire judge controls through CLI entry points.
- Leave placeholders unjudged by default, preserve retrieval misses in trust checks, and stamp artifacts with method metadata.
- Update reports, dashboard calculations, documentation, and regression tests for unpaired results.
| File | Description |
|---|---|
dokimi/tests/test_trust_gate.py |
Covers retrieval rates and revised gate messages. |
dokimi/tests/test_trust_gate_unjudged_misses.py |
Tests missed retrievals and judge-error handling. |
dokimi/tests/test_report_arm_agreement.py |
Tests report sections without complete pairs. |
dokimi/tests/test_provider_call_budgets.py |
Updates the retrieval stub signature. |
dokimi/tests/test_pipeline_summary.py |
Checks paired and excluded question counts. |
dokimi/tests/test_pipeline_library_id.py |
Tests library resolution and topic-cache isolation. |
dokimi/tests/test_llm_cli.py |
Checks early refusal of unsupported judge temperature. |
dokimi/tests/test_judge_placeholder_entry_points.py |
Tests placeholder flags through command handlers. |
dokimi/tests/test_eval_score_method_version.py |
Tests artifact version propagation and validation. |
dokimi/tests/test_eval_answerer.py |
Tests provider acceptance and retrieval limits. |
dokimi/tests/test_diagnoser.py |
Tests diagnoses for unjudged placeholders. |
dokimi/tests/test_dashboard.py |
Checks paired averages and retrieval-miss health scores. |
dokimi/tests/test_benchmark_registry_wiring.py |
Verifies custom registries reach benchmark pipelines. |
dokimi/tests/test_answers_command.py |
Tests invalid retrieval-limit rejection. |
dokimi/tests/test_answering_stack_parity.py |
Updates pinned Stack P behavior and scores. |
dokimi/tests/test_answering_stack_p_step2.py |
Tests judge controls and artifact stamps. |
dokimi/tests/data/cli_flags_allowlist.txt |
Allows the placeholder-judging flag. |
dokimi/tests/answering_stack_parity.py |
Passes registry and placeholder options to fixtures. |
dokimi/dokimi/schemas/eval.v1.json |
Declares method and placeholder-judging metadata. |
dokimi/dokimi/schemas/answers.v1.json |
Declares answering-method metadata. |
dokimi/dokimi/report/eval_report.py |
Renders unavailable results when no pairs exist. |
dokimi/dokimi/orchestrator/pipeline.py |
Integrates registry resolution, judge metadata, and pair counts. |
dokimi/dokimi/gate/trust_gate.py |
Includes retrieval misses in relevant trust checks. |
dokimi/dokimi/eval/judge.py |
Skips placeholders by default and preserves retrieval metadata. |
dokimi/dokimi/eval/diagnoser.py |
Classifies unjudged misses as empty retrievals. |
dokimi/dokimi/eval/answerer.py |
Honors retrieval limits, stamps outputs, and removes dead code. |
dokimi/dokimi/defaults.py |
Defines method versions and stamp validation. |
dokimi/dokimi/dashboard/aggregator.py |
Separates documentation health from paired measurements. |
dokimi/dokimi/commands/orchestrate.py |
Exposes and forwards placeholder judging. |
dokimi/dokimi/commands/evaluate.py |
Validates answer versions and forwards judge options. |
dokimi/dokimi/commands/benchmark.py |
Wires judge controls, sampling checks, and registries. |
dokimi/dokimi/commands/answers.py |
Validates positive retrieval limits. |
dokimi/docs/reference/cli-interface.json |
Refreshes the CLI interface snapshot. |
dokimi/docs/methodology.md |
Clarifies trust-check populations. |
dokimi/changelog.d/20261028_120000_napetrov_stack-p-step2.md |
Documents behavior changes and compatibility limits. |
Review effort: Balanced
| placeholder = context_answer is not None and context_answer.doc_source in NO_RETRIEVAL_SOURCES | ||
| if context_answer is not None and context_answer.text and (self.judge_placeholder or not placeholder): |
There was a problem hiding this comment.
Fixed in 6de55b4. An all-miss run has no complete pair, so the stats carry None instead of zero: the overall, cluster and source rows show an em dash, the advice is suppressed, and the unpaired note says no pairs were made. JSON carries null for those values.
Test: test_an_all_miss_run_prints_no_zero_mean_delta_or_conclusion (tests/test_report_arm_agreement.py).
A run with no complete baseline/skill pair printed zero means, a zero delta and a conclusion. Stats now carry None without a pair; the overall, cluster and source rows show an em dash, the advice is suppressed, and the unpaired note explains that no pairs were made. Also lower the _format_markdown growth baseline to its new complexity. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 6de55b4. Two things are worth fixing before merge, and both are test gaps in the newest commits: the no-pair guards that keep a fabricated zero out of the report's per-topic and per-source tables, and the miss rule behind the dashboard's doc score and status. Two nits inline. The code behaves correctly in every case I ran; the problem is that the tests would not notice if it stopped.
What I ran, on a Linux review host at 6de55b4:
make ci-local: 30 passed, 0 failed, 7 skipped (Docker, network and org-level jobs). 12,043 tests passed, 59 skipped; the changed-line coverage step andschema_compatagainst the PR base pass.- Reproductions with the LLM transport mocked: one pair (80/60) plus one unjudged miss (baseline 60) gives 80.0 / 60.0 / +20.0 in the dashboard,
report evaland thereport generateJSON. An all-miss run gives no delta in each. The gate probe rows from the description reproduce (4 of 14 misses:min_questions10 pass,zero_score_fraction0.286 FAIL,retrieval_hit_rate0.714 pass; 14 of 14:min_questionsFAIL, rate 0.0). - A mutation run on the new code of the dashboard, both reports, the gate, the stamps and the library id: the survivors that decide a reported number are the inline comments below.
Checked, no issues
method_version:True, strings,0, negatives and floats are refused, null or absent is version 1, andeval scoreexits 2 before any judge call. The schemas only add optional properties.- Library id and cache:
--context7-id, then themcp:;id=, then the registry, then the client; the topics cache is keyed on the doc source, so old entries are re-extracted once and not reused wrongly. - Docs: the methodology and changelog claims about the gate, the both-arm subset and the stamps match the code. No generated artifacts, comment blocks within the limit,
growth_baseline.jsononly lowered, no test writes outsidetmp_path. - Commits: every non-merge commit has
Signed-off-by.
Bot points I checked that no longer apply at this head (5)
- exai-bot's point on
eval scorestamping legacy answers as version 2 does not apply:eval scoreand the pipeline now stamp the answers' ownmethod_version(absent means 1), and tests cover both paths. - exai-bot's point on an
mcp:descriptor's;id=losing to the registry does not apply toEvaluationPipeline: the descriptor's id is passed before the registry id, and a test pins it. - exai-bot's point on the dashboard delta counting an unjudged miss does not apply: the arm means and delta now leave the miss out, and the dashboard gives +20 on the same file, as
report evaldoes. - exai-bot's point on the zero deltas in the
report evalbreakdown tables does not apply: those rows print an em dash and the agreement line says there is no delta. - Copilot's point on the pipeline report printing zero means for an all-miss run does not apply: the overall, cluster and source rows print an em dash, the advice is suppressed and the JSON carries null.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
| - minimum `with_docs` average, | ||
| - average delta, | ||
| - score coefficient of variation, | ||
| - minimum evaluated question count (both arms scored; the single `Judge` does not judge a question with no document, so it is not counted here, while `JudgePanel` judges one whose answer is only a fallback text), |
There was a problem hiding this comment.
Nit: this exclusion applies only by default. For example, eval score --judge-placeholder scores a nonempty fallback_none answer, and the trust gate includes that row in its minimum count when the baseline is also scored. Mention the flag exception so users do not interpret an opted-in run's paired count as excluding all retrieval misses.
Suggestion:
| - minimum evaluated question count (both arms scored; the single `Judge` does not judge a question with no document, so it is not counted here, while `JudgePanel` judges one whose answer is only a fallback text), | |
| - minimum evaluated question count (both arms scored; the single `Judge` leaves no-document fallback answers unscored by default, but includes them when `--judge-placeholder` is enabled and both arms receive scores; `JudgePanel` also judges nonempty fallback answers), |
There was a problem hiding this comment.
Fixed in 52b3fa1: the trust-gate line now says the single Judge skips the no-retrieval placeholder by default, unless --judge-placeholder is set, and JudgePanel judges nonempty fallback answers. I took your suggested wording.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
I pushed a commit per nit, up to 85b0ee4, which fix the two nits from my review:
5ea5432:dokimi/tests/test_diagnoser.py:77: added a test that judges onefallback_noneanswer through_evaluate_answer_pairwithjudge_placeholderon and no baseline, and assertsinsufficient_data; it fails withcontext_scored=False.85b0ee4:dokimi/changelog.d/20261028_120000_napetrov_stack-p-step2.md:20: added thatreport evalandreport generateshow—(JSONnull) for the means and delta when no question is scored in both arms, and that the run summary prints no means then.
make ci-local at 85b0ee4: 29 passed, 1 failed (timing flake, passes alone), 7 skipped.
Not approving yet. Still open: the two risks from my review.
- A group with no complete pair has no means in the grouped tables (topic and source), in JSON or markdown. - A retrieval miss counts as a context zero only with a clean baseline score: no baseline score, a top-level error and a with_docs error each leave the doc score alone. - methodology: the single Judge skips the no-retrieval placeholder unless --judge-placeholder is set; JudgePanel judges nonempty fallback answers. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| placeholder again (this restores only that part of the old method). The dashboard's arm | ||
| means and delta leave such a question out too, as `report eval` does; its doc score still | ||
| counts it as 0. `report eval` and `report generate` show `—` for the arm means and delta when | ||
| no question is scored in both arms (the `report generate --format json` `stats` block carries |
There was a problem hiding this comment.
Must-fix: qualify the dashboard's miss-as-zero claim. _context_score only substitutes 0 when the baseline has a score and neither the evaluation nor context arm has an error. For example, a complete 80/60 pair plus an unjudged retrieval miss with a judge error produces doc_score == 80, not 40 (also pinned in test_a_miss_without_a_clean_baseline_score_leaves_the_doc_score_alone). State these conditions so users do not interpret excluded judge-error misses as zero-score contributions.
There was a problem hiding this comment.
Fixed in 989d087: the entry now says the doc score counts a retrieval miss as 0 only when the baseline has a score and neither arm has an error; a miss with an error leaves the doc score alone (test_a_miss_without_a_clean_baseline_score_leaves_the_doc_score_alone). Docs only; full suite 12099 passed, 8 skipped.
| - **Stamped artifacts** — the answers and eval artifacts carry `method_version` (2). Artifacts | ||
| from before this change have no stamp and represent the legacy method (version 1); readers do |
There was a problem hiding this comment.
Must-fix: limit the eval stamping claim to single-Judge artifacts. eval panel-score writes through panel._save_incremental, which includes neither method_version nor judge_placeholder, even when judging newly generated version-2 answers. Consequently, an unstamped eval does not necessarily represent the legacy answering method. Explicitly document the panel exception here and qualify the subsequent claim that eval artifacts carry judge_placeholder.
There was a problem hiding this comment.
Fixed in 989d087: the entry now limits the stamp claim to eval score artifacts (Judge). eval panel-score writes neither method_version nor judge_placeholder, so its artifact reads as version 1; the entry says so.
The doc score counts a miss as 0 only with a baseline score and no error on either arm, and panel-score artifacts carry neither method_version nor judge_placeholder. Say so where the entry claims otherwise. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
LGTM. Both test gaps from my last review are closed, and the delta since 85b0ee4 is tests and wording only. No new findings.
What I ran, on a Linux review host at 989d087:
make ci-local: 30 passed, 0 failed, 7 skipped (Docker, network and org-level jobs). The test run, the changed-line coverage step andschema_compatagainst the PR base pass.- Mutation check on the two new tests: removing any one of the three
None if not s.pairedguards in_arm_meansfails the grouped-table test. Dropping any one ofwithout_score is not None,ev.error is Noneorwith_s.error is Nonein_context_scorefails a case of the dashboard test.
Checked, no issues
- Changelog: the dashboard miss-as-zero conditions match
_context_score.eval panel-scorewrites neithermethod_versionnorjudge_placeholder(panel._save_incremental), as the entry now says. - Methodology: the minimum-count line matches
Judge(placeholder unjudged unless--judge-placeholder) andJudgePanel(skips only empty answers). - Commits: every non-merge commit has
Signed-off-by.
Bot points I checked that no longer apply at this head (6)
- exai-bot's point on
eval scorestamping legacy answers as version 2 does not apply:eval scoreand the pipeline stamp the answers' ownmethod_version(absent means 1), and tests cover both paths. - exai-bot's point on an
mcp:descriptor's;id=losing to the registry does not apply toEvaluationPipeline: the descriptor's id is passed before the registry id, and a test pins it. - exai-bot's point on the dashboard delta counting an unjudged miss does not apply: the arm means and delta leave the miss out, as
report evaldoes. - exai-bot's point on the zero deltas in the
report evalbreakdown tables does not apply: those rows print an em dash and the agreement line says there is no delta. - Copilot's point on the pipeline report printing zero means for an all-miss run does not apply: the overall, cluster and source rows print an em dash, the advice is suppressed and the JSON carries null.
- exai-bot's point on the stamp claim covering
eval panel-scoredoes not apply: the changelog now limits the stamp andjudge_placeholderclaim toeval scoreartifacts and says the panel writes neither.
Brings in main up to 41b3d8d (intel#335, intel#281, intel#343, intel#344, intel#345, intel#274 and dependency updates). Conflict: - dokimi/dokimi/artifacts.py: the module docstring's list of artifact kinds. Main added effective_instructions (intel#335) before need_graph; this branch added repo_hits_judged after task_inspect. Kept both, in the docstring's order. SCHEMA_VERSIONS auto-merged with both sides' kinds. Auto-merged and checked: runs/kinds.py, cli_routes.py, tests/data/cli_flags_allowlist.txt, tests/test_cli_routes.py, tests/test_runs_scan.py, tests/test_runs_show.py and the contract keep both sides' entries. docs/reference/cli-interface.json was regenerated with scripts/cli_surface.py dump and matches the auto-merged file. Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Summary
Step 2 of #228, "Fix Stack P in place". Stack P (
EvaluationPipeline+Answerer) gets five fixes and one stamp, one commit each. Each fix flips the rows it owns in the step-1 characterization table (tests/test_answering_stack_parity.py, PINNED), so the diff of this PR shows the change in numbers. No step 3 work is in this PR.eval/arms_run.py,commands/arms.pyandcommands/subjects.pyare not touched (PR #270 owns them).Which scores move, and why
These are the parity-table values, measured on the six-question fixture. No live benchmark was run for this PR; the size and direction of the live shift are not measured here.
top_kdocs (was the default 5). Retrieved context is smaller, so with-docs answers and their judge inputs change.retrieval.max_results5 to 3;retrieval.passed("max_tokens",)to("max_results", "max_tokens")"[FALLBACK: No relevant documentation found]". It is recorded, not scored.judge.calls12 to 10;judge.contexts.q-emptyandq-reranklose the placeholder with-docs context;delta.per_questionq-empty and q-rerank 0.0 to NoneMIN_PAIRS_FOR_TEST(6). P gets no significance test.stats.delta_mean13.33 to 20.0 (4 pairs, was 6);significance.n6 to None;significance.significantFalse to Nonecontext7_id, the pipeline resolves the id through the registry before the client.library_idclient/resolvedtoregistry/libbenchmark rungains--judge-temperature. The default is still the provider default, so no default score moves.cli.judge_temperature_flagFalse to Truemethod_version: 2in the answers artifact and in the eval artifact of the singleJudge. No score moves.artifact.method_version2 (P), None (A)Direction of the live shift, from the D2/D3 change: the placeholder answer is scored near zero by a judge, so dropping those rows raises the with-docs average and the delta on any live run with retrieval misses. Old and new runs of the same product are therefore not comparable on those numbers.
comparedoes not readmethod_versionyet, so it will not refuse the mix (out of scope, below).Default behaviour that changes live runs:
benchmark run,orchestrate evaluateandeval scoreno longer judge a placeholder with-docs answer. Pass--judge-placeholderto restore the old judging.An eval artifact made by
eval scoreon answers from before this PR is stampedmethod_version: 1, the version of the answers it judges. The stamp records the judging method; the answers artifact keeps its own stamp (absent, so version 1). Read the two together; a single eval stamp does not say which answers it judged.Trust gate: numbers, thresholds not changed
Thresholds are unchanged:
min_retrieval_hit_rate0.60,min_retrieval_coverage0.80,min_questions10,max_zero_fraction0.20, and the rest ofDEFAULTS.What changes is the input of the checks:
min_questionscounts the both-arm-scored questions. Placeholder questions (no document, not judged by default) drop out of that count. Fewer counted questions makes the minimum harder to reach, so this is a stricter number change, not a weakening. On the 4-of-14 fixture the count goes from 14 to 10, which still passes at min 10.zero_score_fraction(fail) counts a question whose retrieval missed as unusable, in the numerator and the denominator. On main a miss was judged at 0 and counted as a zero, so this keeps the fail-severity check failing on the same run. The message says "scored 0 or had no document". Commitc517c727. Regression test:tests/test_trust_gate_unjudged_misses.pyfails on the parent commit (4 of 14 passes at 0.0) and passes here.min_with_docs_avg,min_deltaandscore_cv(warn) run over the both-arm-scored subset only. The placeholder zeros that main counted are gone from that subset, so these three checks are more lenient than on main. Not changed here; see the decision below.Thresholds are not changed, and no retrieval check was added with fail severity.
Probe (
probes/gate_probe.pyin the review scratch; 14 both-arm questions, mock judge scores aFALLBACKanswer 0, otherwise 60; re-run on the head after the fix):min_questions(fail, min 10)zero_score_fraction(fail, max 0.20)retrieval_hit_rate(warn, min 0.60)min_questions(fail, min 10)retrieval_hit_rate0.0, FAILNet effect: the fail-severity zero-score check gives the same answer on the same run as main (FAIL at 0.286). A run where 29% of questions get no document still fails the gate. The retrieval miss shows as a warn-severity rate that passes at 71%, as before. A run where every question misses still fails, through
min_questions, and has no retrieval line.Adding a fail-severity retrieval check, or changing its threshold, is a separate design decision. It is not in this PR.
Changes
fix(answers): the doc source is asked fortop_kdocs (was the default). Dead code deleted in the context arm.fix(benchmark):benchmark run --judge-temperatureis parsed and passed to the judge. Batch is not changed.fix(pipeline): the library id goes throughresolve_library_id_for_product(explicit id, then registrycontext7_id, then client).fix(judge): the no-retrieval placeholder is not judged by default;--judge-placeholderonbenchmark run,orchestrate evaluateandeval scorerestores it. Trust gate counts all records for retrieval (see above).fix(gate):zero_score_fractioncounts retrieval misses as unusable, so the fail-severity check matches main on the 4-of-14 fixture (commitc517c727, regression testtests/test_trust_gate_unjudged_misses.py). Commit6bec8910updates the methodology bullet and parses the evaluations once.refactor(answerer): removed the unusedself.llmclient, the unreachableif not docs:branch, and their imports.feat(artifacts):method_version(defaults.ANSWERING_METHOD_VERSION = 2) on the answers artifact and the eval artifact of the singleJudge; documented inanswers.v1.jsonandeval.v1.json. Artifacts without it read as version 1. Nothing reads it yet.test(parity),test(answerer),test(answerer): lint fix (missingListimport); the init tests that only read back attributes replaced by a provider allow-list smoke test (flagged by the test-quality audit); a direct test thattop_kreaches the doc source.docs: methodology trust-gate list says which checks run on both-arm-scored questions; themethod_versioncomment says nothing reads it yet.changelog.d/20261028_120000_napetrov_stack-p-step2.md.The flag registry (
dokimi/cli_flags.py) does not exist on main, so the new flags were added to the parsers directly.Evidence
top_ktest: fails on intel/main (the probe sends nomax_results), passes on the head.6bec8910; the full run was on3685c29f, and the last commit only changes one comment line, with the targeted gate and docs tests re-run):9703 passed, 6 skipped.tests/test_trust_gate_unjudged_misses.py: fails on the parent commit0fe7452d(value 0.0, expected0.286), passes on the head.ruff check .: clean.ruff format --checkreports 482 files that would be reformatted; the project does not gate onruff format, so this is pre-existing and not changed here.review.mdof the review scratch.Merge Danger
Checked by trial merge against
intel/main(no rebase needed for this branch: it containsintel/main).fix/report-eval-generate-agree, OPEN, BLOCKED): textual conflict indokimi/dokimi/orchestrator/pipeline.py, the import block at intel/main lines 12 to 14. fix(report): report eval, generate and pipeline use one paired arm summary (#229 P1-3) #250 removes thecreate_doc_source_clientimport line; this PR addsresolve_library_id_for_productto it. Resolve by keeping both names. fix(report): report eval, generate and pipeline use one paired arm summary (#229 P1-3) #250 also editspipeline.pyat intel/main lines 383 to 391, near the_generate_*call sites this PR changes; no conflict there, but re-run the parity test after the merge.feat/claude-cli-backend, OPEN, DIRTY): textual conflicts indokimi/dokimi/eval/answerer.py(theChatModelimport at lines 18 to 19, and theAnswerer.__init__llm block at lines 98 to 110, which this PR removes). Resolve by taking this PR's removal and feat(llm): claude-cli provider: run the Q&A track on a logged-in Claude Code CLI #255's provider change.commands/benchmark.py,commands/evaluate.py,commands/orchestrate.py,defaults.py,eval/judge.py. Those are semantic overlaps, not textual ones: the--judge-placeholderand--judge-temperaturewiring, andANSWERING_METHOD_VERSION, sit next to feat(llm): claude-cli provider: run the Q&A track on a logged-in Claude Code CLI #255's backend changes. Re-run the suite after either merge.scripts/growth_baseline.jsonandtests/test_import_architecture.pyagainst this branch. Those are not this PR's lines: feat(llm): claude-cli provider: run the Q&A track on a logged-in Claude Code CLI #255's base predates50250218on main (the import contract test). Rebasing feat(llm): claude-cli provider: run the Q&A track on a logged-in Claude Code CLI #255 ontointel/mainclears them.Both #250 and #255 are ours and the coordinator owns those conflicts; this PR does not resolve them.
Out of scope (listed, not changed here)
benchmark batchhas no--judge-temperature.comparedoes not readmethod_versionyet, so it does not refuse mixed versions.eval panel-score(JudgePanel) still judges the placeholder answer and writes its artifact withoutmethod_version. The panel's trust numbers therefore differ from the single judge's on the same answers.context7_idnow raisesValueErrorfor clients that require a declared id, in question generation and answer generation (the registry-first lookup). Intended, but a hard stop where the client used to be asked.min_with_docs_avg,min_delta,score_cv) run over the both-arm subset, so they are more lenient than on main; (b) a fail-severity retrieval check is not added (retrieval_hit_ratestays warn). Neither is changed here.Refs #228
🤖 Generated with Claude Code