Repository navigation
test(rag): labelled set and harness for groundedness prompts vs hallucination_detection - #1762
Charansripadi wants to merge 1 commit into
Conversation
…cination_detection Adds the measurement set and harness for generative-computing#1721, ahead of the arms themselves (which wait on generative-computing#1718 and generative-computing#1719): - labelled_set.jsonl: 128 hand-written items with fictional facts, so neither arm can pass a claim from world knowledge. Covers grounded, cross-document synthesis, genuinely partial, invented, non-factual, mixed, and long (20-80 sentence) responses. Every response carries per-sentence gold labels in the adapter's own label set, split exactly as the adapter input splits it (NLTK Punkt), checked by a unit test. - harness.py: runs arm 1 (GroundednessRequirement as shipped) or arm 2 (find_citations for the output, verdict from flag_hallucinated_content with the rule in the issue) in either allow_partial_support mode, with repeats, per-validation latency and token counts, and resumable JSONL output. compare() reports false fails and misses separately, exact McNemar on disagreements, a paired bootstrap CI, hard failures, label use and stability, and applies a pre-registered decision rule. - README.md: set design, how to run, and a proposed decision rule to be agreed on the issue before any run. Refs generative-computing#1721 Signed-off-by: Sri Charan Sripadi <charansripadi@gmail.com>
jakelorocco
left a comment
jakelorocco
left a comment
There was a problem hiding this comment.
Thanks for working on this issue!
I see Nigel opened this issue and has been working with you on it; so I will defer to him for now. Whatever the outcome, I don't think the code for determining that outcome should live in the main mellea codebase.
I do see the positives for saving harnesses like this; we should figure out where to save things like this so that we can reuse / repurpose them for future experiments.
planetf1
left a comment
planetf1
left a comment
There was a problem hiding this comment.
Thanks for this. The set design is careful: invented facts so neither arm can lean on the model's own knowledge, gold labels that line up with the verdicts, and the Punkt split pinned by a test. The unit tests are thorough, and the pure logic (McNemar, the verdict rule, resume) checks out.
Nothing here blocks merge. The inline comments are all about making the measurement trustworthy before the runs on #1721, plus a few nits.
| m1, m2 = arm_metrics(items, first), arm_metrics(items, second) | ||
| lenient = next(iter({o.allow_partial_support for o in first})) | ||
| if {o.allow_partial_support for o in second} != {lenient}: | ||
| raise ValueError("compare outcomes from the same allow_partial_support mode") |
There was a problem hiding this comment.
Nothing here blocks merge, but I'd fix this before the runs: compare doesn't check its input files.
It checks that both files use the same mode (allow_partial_support), and nothing else. I tried three bad inputs:
- Missing items. An arm-2 file with only 10 of the 128 items still gets
go: true. Nothing says the other 118 are missing. - Files in the wrong order. Passing the prompts file as arm 2 prints a normal report.
- Two bindings in one file.
runappends to--out, so alocal-filerun and aswitchrun can end up in the same file. Only the first run's verdicts are used, but errors from both are counted. I built aswitchrun of 128 rows: 5 errored, 6 invented items wrongly passing, the rest correct. On its own it fails (go: false, 5 of 128 = 3.9% hard failures). Appended behind a cleanlocal-filerun, the same rows givego: true, because 5 of 256 is 1.95%. The README's rule is "under 2% of validations, on each binding", so mixing runs hides the failure.
The fix is to check the inputs the same way the mode is already checked:
for outcomes in (first, second):
if len({o.arm for o in outcomes}) != 1:
raise ValueError(f"outcomes mix arms: {sorted({o.arm for o in outcomes})}")
if len({o.binding for o in outcomes}) != 1:
raise ValueError(f"outcomes mix bindings: {sorted({o.binding for o in outcomes})}")And two small additions: error if an item is missing from either file, and check the first file really is the prompts arm.
| | Primary verdict | repeat 0 of 3 | | ||
| | Margin | 5 percentage points, for false fails and for misses, in each mode | | ||
| | Paired test | exact McNemar on the items where the arms disagree, α = 0.05 | | ||
| | "No worse" | rule `mcnemar`: arm 2's rate is at most arm 1's + 5 points, and McNemar does not show arm 2 making more of that error | |
There was a problem hiding this comment.
Worth agreeing on #1721 before any run: on this set, the McNemar half of the rule can never fire. Whenever the 5-point margin passes, the p-value is above 0.05 (I checked both one-sided and two-sided). So the rule is just the margin: arm 2 may miss 1 more of the 34 invented claims, or have 3 more false fails of 74 in strict mode, 4 of 94 in lenient.
That's probably a fine rule. But the table reads as if the test adds protection, and the "Why mcnemar, not ci" note leans on it too. I'd say plainly that it's a margin rule, here and in the #1721 proposal. Growing the set until the test can fire takes roughly 120 invented-claim items, which doesn't seem worth it for this decision.
| @hook("generation_post_call", mode=PluginMode.AUDIT) | ||
| async def _count(payload: Any, context: Any) -> None: | ||
| usage = getattr(payload.model_output.generation, "usage", None) | ||
| if usage: |
There was a problem hiding this comment.
One problem for the cost numbers: on LocalHFBackend, the token counts will be empty for arm 2 and too low for arm 1.
The counter reads generation.usage. On HF, adapter calls never fill it in. The intrinsic path passes a decoded string to processing (huggingface.py:1639), and usage only gets set from a GenerateDecoderOnlyOutput (huggingface.py:2460). The judge prompts use the normal chat path, so they do report.
So arm 2 shows None, and arm 1's total misses its find_citations call. The Switch path does report usage for adapter calls, so the two bindings won't be comparable either. The README says tokens are "left empty if a backend does not report usage", but this is about the call path, not the backend.
Easy fix: count the generation calls the plugin sees, and how many reported usage. If the numbers differ, store None. That way a partial total never looks like a real one.
| t0 = time.perf_counter() | ||
| try: | ||
| raw = await fn(item, backend, allow_partial_support) | ||
| except Exception as exc: |
There was a problem hiding this comment.
One more thing before the runs: an errored result is written to the file like any other, and resume treats it as done. I checked: if a run errors on an item, rerunning with a fixed setup skips it. The only way to retry is to delete the file.
Why it matters: this except Exception catches everything. An OOM on the 80-sentence items, a failed adapter download, or a bad --backend-factory all become "hard failures" counted against the 2% gate. Those say nothing about the adapter. The unit test even uses RuntimeError("adapter not loaded") as its hard-failure example, and that's an environment problem, not an adapter one.
Two small fixes, either or both:
- Retry errored rows on resume, so they're not in the done set.
- Record the exception type, and count only the adapter's own errors (the
ValueError/AdapterSchemaMismatchErrorthe wrappers raise) as hard failures. Anything else should stop the run.
| completion_tokens: Summed over generation calls, if reported. | ||
| """ | ||
|
|
||
| item_id: str |
There was a problem hiding this comment.
Last of the main points: the result rows don't say what ran. Outcome records no model and no adapter revision. The resume key skips the model too, so rerunning with a different --model into the same file silently skips everything.
The fix is close to free. mellea already fires ADAPTER_FUNCTION_INVOCATION_COMPLETE on every adapter call, with the adapter name, revision, and binding type. The same scoped plugin that counts tokens could write those onto each row.
Why it's worth it: the README plans to re-run the set after an adapter bump and compare. Without the revision in the results, there's nothing to compare against. And knowing the binding type on each row would confirm the adapter arm actually ran the adapter on both bindings.
| scope = plugin_scope(counter.as_plugin()) | ||
| await scope.__aenter__() | ||
| return scope | ||
| except Exception: |
There was a problem hiding this comment.
Nit: this except Exception hides real breakage. If the hooks extra is missing, token counting silently turns off, and it looks exactly like a backend that reports no usage. Catching just ImportError would do.
| def test_gold_sentences_match_the_adapter_split(item): | ||
| """Gold labels line up one-to-one with the sentences the adapter sees.""" | ||
| spans = sentence_spans(item.response) | ||
| assert [item.response[b:e] for b, e in spans] == [s["text"] for s in item.sentences] |
There was a problem hiding this comment.
Nit: worth pinning the join too. The adapter's character offsets assume the response is the sentences joined with single spaces. All 128 items satisfy " ".join(s["text"] for s in item.sentences) == item.response today, but a future item with a newline or a double space would still pass this test while the offsets drift. One more assert covers it.
| ## Running (GPU) | ||
|
|
||
| ```bash | ||
| H="python -m test.stdlib.requirements.groundedness_eval.harness" |
There was a problem hiding this comment.
Nit: this fails under zsh, which doesn't word-split $H. uv run python -m test.stdlib.requirements.groundedness_eval.harness run ... works everywhere.
| from typing import Any | ||
|
|
||
| SET_PATH = Path(__file__).with_name("labelled_set.jsonl") | ||
| LABELS = ("faithful", "partial", "unfaithful", "NA") |
There was a problem hiding this comment.
Nit: this label set matches the adapter's enum, which is the right reference. One related thing, not for this PR: the flag_hallucinated_content docstring gives "hallucinated" as an example faithfulness value, and that isn't in the enum. That docstring is already in #1719's scope, so I've left it alone here.
Pull Request
Issue
Part of #1721. This adds the labelled set and the harness; the per-arm results follow once #1718 and #1719 have merged.
Description
Adds
test/stdlib/requirements/groundedness_eval/, the measurement set and harness for deciding #1669.labelled_set.jsonl: 128 hand-written items. Every fact is invented, so neither arm can pass a claim from the model's own knowledge, and the set has no third-party licence terms. It covers grounded (30), cross-document synthesis (20), genuinely partial (20), invented (30), needs-no-citation (10), mixed (10) and long responses of 20 to 80 sentences (8). Each sentence has a gold label in the adapter's own set (faithful,partial,unfaithful,NA). The responses are split exactly as the adapter input splits them (NLTK Punkt), and a unit test checks that for every item.harness.py:runvalidates the set with arm 1 (GroundednessRequirementas shipped) or arm 2 (find_citationsfor the citation output, verdict fromflag_hallucinated_contentwith the rule in the issue), in eitherallow_partial_supportmode, with repeats. Output is resumable JSONL, with latency per validation and tokens summed fromgeneration.usagethrough a scoped plugin.comparereports, per arm:It then runs exact McNemar on the disagreements, computes a paired bootstrap CI, and applies the decision rule.
Backends other than local HF (e.g. Granite Switch) plug in with
--backend-factory module:function.README.md: set design, how to run, and the proposed decision rule (also posted on design(rag): measure hallucination_detection against the groundedness judgment prompts before swapping #1721 for agreement before any run).Arm 1 is meant to run after #1718, so that an errored result counts as a hard failure rather than a "not grounded" verdict.
Testing
test_groundedness_eval.pyhas 280 unit tests, no model needed. They cover set integrity, the arm-2 verdict rule (including no-record and malformed records), McNemar and the CI, metrics, the decision rule, and run/resume. Therunpaths that call a real backend are not covered by unit tests; they need a GPU and the adapters.Attribution
Adding a new component, requirement, sampling strategy, or tool?
If your PR adds or modifies one of the types below, check the matching box. A checklist of type-specific review items will be posted as a comment.
NOTE: Please ensure you have an issue that has been acknowledged by a core contributor and routed you to open a pull request against this repository. Otherwise, please open an issue before continuing with this pull request.