Repository navigation
A held-out, repeatable score for the explanation layer (#346) - #385
patelankeet2 wants to merge 9 commits into
Conversation
Five real findings from the real pipeline (rtr-us5-insecure, rtr-us5-messy), regenerated live against the real model after #145's fix, rated by hand on the same accuracy/usefulness scale docs/evaluation.md Part 3 already uses. Held out in the literal sense: none of these findings or their text appear among ai/Modelfile's own worked examples. PC-001 and PC-005 are the two findings #145 was filed against -- both regenerate correctly now, confirming the fix in practice rather than by inspection. AC-002's captured explanation has a real, different defect: it describes the unreachable line's own action backwards (calls a PERMIT line a rule that 'is supposed to block'), even though the final traffic-outcome claim is correct. Checked for reproducibility (0 of 6 immediate re-runs repeated it) and filed as its own issue (#384) rather than quietly swapped for a cleaner sample or fixed inline -- fixing it here would mean rating text that no longer exists.
tools/explanation_quality.py -- pure arithmetic over the frozen held-out set, mean and minimum of each rating dimension separately (not blended into one number, same reasoning docs/evaluation.md already gives for the controls table and F-4 generally). Runs both documented ways; imports neither analysis, ai, nor web, so it needs no sys.path guard, same exemption pfsense_shape.py already has.
EXPLANATION_QUALITY_THRESHOLD (mean_accuracy 4.0, mean_usefulness 3.0), deliberately based on the mean rather than the minimum -- a single low-scoring finding is a correctness bug to fix for that shape (same as #52, #145, #384), not a reason to disable the model for every finding. No runtime auto-check exists: crossing this is detected by a human re-running the held-out measurement, since there is no honest way to score one live generation without another model judging it. tests/test_explanation_quality.py: synthetic cases for the multi-rater/multi-finding averaging the real fixture's single-rater data can't exercise, plus tests pinning the real committed holdout's published numbers against the documented threshold. python -m pytest tests/test_explanation_quality.py -v -> 8 passed.
docs/evaluation.md Part 4 -- the full writeup: why this round regenerated rather than reused Part 3's pre-#145 text, confirmation that both PC-001/PC-005 inversions #145 was filed against are gone on live regeneration, the new AC-002 defect found and filed as #384 (kept in the held-out set rather than re-rolled for a cleaner sample), the published score, and why EXPLANATION_QUALITY_THRESHOLD is based on the mean rather than the minimum and is a documented decision point rather than a runtime check. Explicitly left open for the team: a second independent rater on this set, the same way Part 3 grew from one rater to four.
patelankeet2
added
size: M
ARSH871-bot
left a comment
ARSH871-bot
left a comment
There was a problem hiding this comment.
@patelankeet2, this is careful work, and keeping AC-002's bad capture in the set rather than re-rolling it is exactly right; #384 is the honest consequence. I am requesting changes for one thing about what the score measures, because this PR exists to produce that number.
What I checked that holds
python -m tools.explanation_quality accuracy mean 4.40 min 2 · usefulness mean 3.40 min 2 (matches the PR)
python tools/explanation_quality.py runs (script form)
tests/test_explanation_quality.py 8 passed
empty-set raise replaced with a fake zero KILLED (test_scoring_an_empty_set_raises...)
full suite on the head (contains main) 1794 passed, 42 skipped ruff clean
The four model-written captures pass the live grounding guard (ai/explain.py's _is_unacceptable), so they are the kind of text the product can actually show. RT-050's captured text is byte-for-byte what _fallback_error_explanation() produces today. These tests need no Batfish, so the mutant result is the same either way.
Blocking: the score that decides "model or fallback" counts fallback text
RT-050's capture is captured_explanation_source: "fallback", rated 5/4 with the note "Deterministic fallback, so accuracy cannot be wrong by construction", and it is in the mean. Two consequences, both measured:
published (5 entries, 1 fallback) mean accuracy 4.40 mean usefulness 3.40
model-written only (4 entries) mean accuracy 4.25 mean usefulness 3.25
five fallback entries, no model text mean accuracy 5.00 mean usefulness 4.00 <- passes the threshold
Today's conclusion does not change: 4.25/3.25 still clears 4.0/3.0. But the third row is the problem. EXPLANATION_QUALITY_THRESHOLD exists to say when the model is worse than the fallback, and as written it can be met by a set with no model output in it. That is a weaker claim standing in for a stronger one, and it cuts against the PR's own rule that accuracy and usefulness are "never blended": the same reasoning applies to blending the two sources.
RT-050 has a second issue. It is status="error", and web/main.py attaches explanations only to status="found" findings, so no user ever sees an explanation for it.
Suggested fix: score captured_explanation_source == "model" entries only, and raise if there are none, mirroring your empty-set guard. Then either drop RT-050 or report the fallback entries as their own line beside the model's. Update the published figures to the model-only numbers. A test that would have caught it: a held-out set of only fallback entries must not produce a passing model score.
Not blocking, but I would like it before this is relied on
Nothing makes the score stale when the model changes. I appended a comment line to ai/Modelfile and the suite stayed green, 8 of 8. So "CI green" on test_current_mean_quality_meets_the_documented_threshold will keep reading as "the explanation layer meets its threshold" after a prompt or base-model change that this set never measured. The PR is honest that this is a dated snapshot, but the test name says "current".
A cheap guard: record a sha256 of ai/Modelfile (and the base model tag) in holdout.json when capturing, and have one test fail when it no longer matches, with a message saying to re-capture and re-rate. It goes red exactly when the number has genuinely gone stale, unlike the #267 traceability case, where any new test turned it red.
Happy to be the second rater on the held-out set once the scoring is model-only. Re-request me when it is pushed.
Found by @ARSH871-bot: RT-050's fallback-sourced capture sat in the same aggregate as model-written text, so the published score could be met by a held-out set with no model output at all (five fallback copies alone would score 5.00/4.00, comfortably above the threshold). EXPLANATION_QUALITY_THRESHOLD exists to judge the model, and a fallback explanation cannot be wrong by construction, so it says nothing about that question. RT-050 is also status="error", which web/main.py never attaches an explanation to, so no user sees it regardless of source. score_holdout() now filters to captured_explanation_source == "model" for the headline numbers, raises ValueError if none remain (a fallback-only set included), and reports any fallback-sourced entries under their own "fallback" key rather than dropping them. Published numbers move from 5 findings/4.40/3.40 to 4 model-written findings/4.25/3.25 -- the conclusion (clears the 4.0/3.0 threshold) does not change, but the number it is measured from now actually measures what the threshold exists to judge. python -m pytest tests/test_explanation_quality.py -v -> 13 passed (was 8).
EXPLANATION_QUALITY_THRESHOLD's own comment still cited the old blended 4.4/3.4 measurement. docs/evaluation.md Part 4 now shows the corrected model-only score (4.25/3.25), reports RT-050 on its own fallback line instead of folded into the table, and carries a dated correction note explaining what changed and why, per the project's own convention of recording a correction rather than silently editing a past measurement.
Non-blocking note from the review (@ARSH871-bot): nothing made the published score go stale when the model that produced it changes -- appending one comment line to ai/Modelfile left the whole suite green, because every test replays frozen captured text rather than calling the model again. holdout.json now records what it was captured against (captured_against.modelfile_sha256, base_model). A new test compares that recorded hash to ai/Modelfile's live one and fails loudly, pointing at re-capturing and re-rating rather than at silently updating the recorded hash. python -m pytest tests/test_explanation_quality.py -v -> 14 passed (was 13).
|
Both the blocking and the non-blocking points fixed. Blocking: fallback-sourced captures no longer count toward the model scoreReproduced your numbers exactly before touching anything: all-5 gives 4.40/3.40, model-only gives 4.25/3.25, and five copies of RT-050's rating alone gives 5.00/4.00 -- confirmed that third case really does clear the threshold with zero model output behind it.
Added the test you named directly: a held-out set of only fallback entries now raises rather than scoring. Also added tests for the separation itself (mixed set keeps the fallback entry out of the headline numbers), a set with no fallback entries (no Published figures in Non-blocking: staleness guardImplemented your suggested fix directly. Re-requesting you -- and the offer to be second rater stands, now that the scoring is model-only. |
|
@ARSH871-bot -- your CHANGES_REQUESTED review is on |
Closes #346.
What #346 actually needs
Three acceptance criteria: a held-out set of findings with human-rated reference explanations, a repeatable score run in CI with the number published, and a documented threshold below which the deterministic fallback is preferred.
The core constraint, stated up front because it shapes every decision below: CI runs with no Ollama and no Batfish (
.github/workflows/tests.yml), and LLM output is not bit-for-bit repeatable anyway. So "repeatable" cannot mean calling a live model in CI -- it means freezing a held-out set (finding + captured explanation + human rating) as a committed fixture, and running pure arithmetic over it. That is the same tradeoffdocs/traceability.mdanddocs/evaluation.mdalready make peace with: a measured snapshot, dated, re-run by hand when something that could change it changes (CONTRIBUTING.md5c).Alternatives considered for what goes in the held-out set
docs/evaluation.mdPart 3's exact 5 findings and their existing 4-rater scores as-is. Fast, already multi-rater -- but that text predates Generated explanations can invert which side (policy vs config) requires what #145's fix, so it would pin a known-fixed bug's output forever and call it current behaviour.Went with 2. It measures what the system actually does today rather than a stale snapshot, and it matches how this project already bootstrapped Part 3's own rating exercise.
What changed
tests/fixtures/explanation_quality/holdout.json-- the same 5 findings Part 3 used (RT-050,AC-001,PC-001,AC-002,PC-005), regenerated live against the real pipeline and the real model on 10 October, rated by hand on Part 3's own 1-5 accuracy/usefulness scale. Held out in the literal sense -- none of these appear amongai/Modelfile's own worked examples.tools/explanation_quality.py--load_holdout()/score_holdout(), pure arithmetic, no Ollama or Batfish call. Reports accuracy and usefulness as two separate numbers (never blended -- same reasoning this project already gives for F-4 and the controls table), each as a mean and a minimum, because the mean alone hides exactly the case that matters (see below). Also definesEXPLANATION_QUALITY_THRESHOLD(mean accuracy 4.0, mean usefulness 3.0). Runs both documentedtools/invocation forms; imports neitheranalysis,ai, norweb, so it needs nosys.pathguard (same exemptionpfsense_shape.pyalready has -- confirmed bytests/test_tools_invocation.pyitself, which now parametrizes over this file too).tests/test_explanation_quality.py-- synthetic cases for multi-rater/multi-finding averaging (the real fixture currently has one rater per finding, which alone couldn't exercise that path), plus tests pinning the real committed holdout's published numbers against the documented threshold, and the F-4-style guard that scoring an empty set raises rather than returning a fake zero.docs/evaluation.mdPart 4 -- the full writeup, including a defect this PR does not fix (see below).A real finding, from doing this properly rather than assumed away
Regenerating live (rather than reusing Part 3's stale text) paid off immediately: both
PC-001andPC-005-- the two findings #145 was filed against -- regenerate correctly now. Confirmed, not assumed.It also surfaced a different, new defect.
AC-002's regenerated text describes the dead line's own action backwards (calls aPERMITline one that "is supposed to block traffic"), even though the final traffic-outcome claim is correct -- a different fact than the one #52's existing guard protects. Checked for reproducibility before concluding anything: six more immediate live regenerations of the same finding, zero repeated it. Filed as #384 with the full repro log and a suggested (not applied) fix, and kept in the held-out set rated honestly low rather than re-rolled for a cleaner sample or fixed inline -- the same discipline #145 itself established: fixing the generator mid-measurement means rating text that no longer exists.Verified, not assumed
Mutation-tested both safety properties that matter: temporarily made
score_holdout()return a fake zero score for an empty set instead of raising --test_scoring_an_empty_set_raises_rather_than_returning_a_fake_zerofailed immediately. Temporarily raisedEXPLANATION_QUALITY_THRESHOLD["mean_accuracy"]above the real measured 4.4 --test_current_mean_quality_meets_the_documented_thresholdfailed immediately, for the right reason. Both reverted after.Acceptance criteria (#346)
tests/fixtures/explanation_quality/holdout.json, 5 findings, rated with reasoning recorded per finding.tools/explanation_quality.py+tests/test_explanation_quality.py, needs neither Batfish nor Ollama; published indocs/evaluation.mdPart 4.EXPLANATION_QUALITY_THRESHOLD, deliberately based on the mean rather than the minimum (reasoning in both the code comment and the doc), and deliberately a documented/human-reviewed decision rather than a runtime auto-disable, since there is no honest way to score one live generation's quality without another model judging it.What this deliberately does not do
Does not touch
ai/Modelfile, the three existing deterministic outcome guards, or anything on the live explanation-generation path -- this is a measurement added alongside, not a change to how explanations are generated. Does not fix #384 -- filed separately, on purpose. Does not require team sign-off before merging -- unlike #191, nothing here is a shared-contract change.Left open, explicitly
A second independent rater on this held-out set (@ARSH871-bot @shubhamkataria2005 @SamikaPerera) would turn this from a first round into the same strength of evidence Part 3 reached with four raters. Not required to close this issue -- noted in
docs/evaluation.mdPart 4 as a next step, not a blocker.