fix(rag): normalise model-returned JSON types - #1625
Conversation
Normalise span IDs and labels from model-generated RAG JSON in both citation support and necessity parsers. Add regression coverage for quoted IDs and unexpected field types. Assisted-by: Codex Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
Add regression coverage for nested non-string support levels and invalid span identifiers. Assisted-by: Codex Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
Treat malformed necessity labels as requiring citation, aggregate duplicate judgments pessimistically, and reject unparseable span IDs without discarding the batch. Assisted-by: Codex Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
Keep the new parser regression docstring compatible with the repository documentation validator. Assisted-by: Codex Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
psschwei
left a comment
There was a problem hiding this comment.
Reviewed this by running the main and branch versions of _parse_batch_support_output / _parse_necessity_output side by side, so the cases below are observed rather than inferred. Three of them are behavior regressions relative to main, and one is a pre-existing bug in code this PR touches.
Inline comments cover the substantive items. Two more that do not land on changed lines:
test/stdlib/requirements/test_groundedness_requirement.py cannot be collected without the hf extra. The module-scope from mellea.backends.huggingface import LocalHFBackend at line 11 aborts collection with ImportError: The Hugging Face backend requires extra dependencies in an environment without mellea[hf]. None of the new tests touch a backend, they call the parsers with literal strings. CI runs uv sync --frozen --all-extras so CI is unaffected, but a contributor following the AGENTS.md uv sync --extra backends --all-groups path loses the entire regression file, and it will not appear in the skip summary either. Moving that import into the fixtures that need it, or a pytest.importorskip, keeps the pure-unit tests runnable.
Consider constraining the generation rather than the parser. _identify_citation_necessity (line 330) and _assess_citation_support (line 430) both call backend.generate_from_context(...) with no format=, though Backend.generate_from_context accepts format: type[BaseModelSubclass] | None for constrained decoding. All three new coercers exist because the output shape is unconstrained, and per the #1316 note at line 614 this is at least the third patch to this parser. A two-field pydantic model per call would remove the class of bug instead of the current instances. Fine as a follow-up if it is out of scope here.
|
|
||
| def _normalise_needs_citation(raw: object) -> bool: | ||
| """Normalise a model-returned citation-necessity label.""" | ||
| if not isinstance(raw, str): |
There was a problem hiding this comment.
_normalise_needs_citation returns True for every non-string input, so a JSON boolean false or an integer 0 is flipped from "does not need a citation" to "needs a citation". That is a regression from main, which handled both correctly.
Verified by running both versions:
needs_citation value |
main | this branch |
|---|---|---|
false |
False |
True |
0 |
False |
True |
A JSON boolean is the most likely non-string form for a field named needs_citation. Those spans then reach _assess_citation_support, find no overlapping citations, get NOT_SUPPORTED, and _build_groundedness_result fails a fully grounded response.
Suggested fix, handling bool before the string path and treating integral 0/1 like their string forms:
if isinstance(raw, bool):
return raw
if isinstance(raw, int):
return bool(raw)Two related notes:
test_parse_necessity_output_normalises_model_typesasserts1 -> Trueand nothing coversfalseor0, so the new tests lock this direction in.test_malformed_necessity_label_keeps_span_subject_to_groundednessalso passes with the bug present.- The debug log at line 843 now prints the coerced boolean rather than the raw label, so this misconversion would be undiagnosable from logs. Worth logging both.
There was a problem hiding this comment.
Fixed in 8ceb250. JSON booleans and integers now preserve their meaning, the debug log includes both the raw label and normalised result, and false/0 regression coverage is included.
| raw = (raw or "").upper().strip() | ||
| def _norm(raw: object) -> str: | ||
| raw = _normalise_label(raw).upper() | ||
| if "FULLY" in raw and "SUPPORTED" in raw: |
There was a problem hiding this comment.
This ladder tests "FULLY" in raw and "SUPPORTED" in raw before any negation check, so negated labels are classified as their positive counterpart:
_parse_batch_support_output('[{"span_id": 0, "support_level": "NOT FULLY SUPPORTED"}]', 1)returns{0: "FULLY_SUPPORTED"}- the nested form
'[{"span_id": 0, "evidence": [{"support_level": "not fully supported"}]}]'returns{0: "FULLY_SUPPORTED"} "FULLY UNSUPPORTED"hits it too, since"UNSUPPORTED"contains"SUPPORTED"
An ungrounded span is declared fully supported and the response passes groundedness validation, which is the worse failure direction for a safety check. The comment directly above says the purpose of _norm is to catch near-miss labels, so the near-miss handling is inverted for the negated variants. A negation guard checked first fixes it:
if "NOT" in raw or "UN" in raw:
return "NOT_SUPPORTED"This is pre-existing rather than introduced here, but the PR touches _norm and the same ladder is duplicated verbatim at lines 776-784 on the flat path, so the fix is needed in both places. Alternatively _norm can absorb the outer block: support_level = _norm(support_level_raw) or "NOT_SUPPORTED".
There was a problem hiding this comment.
Fixed in 8ceb250. Normalisation now detects explicit NOT and UNSUPPORTED labels before positive matching for both flat and nested output. Added negated-label coverage.
| span_id = int(raw.strip()) | ||
| except ValueError: | ||
| return None | ||
| else: |
There was a problem hiding this comment.
Integral floats now fall through to return None, which regresses the batch-support path.
For [{"span_id": 0.0, "support_level": "FULLY_SUPPORTED"}]:
- main: stored as
{0.0: "FULLY_SUPPORTED"}, andbatch_results.get(0)resolves it becausehash(0.0) == hash(0), so the correct verdict reached the caller - this branch: the id is rejected, the judgment is dropped, and the fill loop writes
{0: "NOT_SUPPORTED"}, failing a fully supported span
The 1.5 case in test_parse_batch_support_output_ignores_invalid_span_id is right to reject. Suggest accepting float when raw.is_integer() and rejecting only non-integral floats.
There was a problem hiding this comment.
Fixed in 8ceb250. Integral floats such as 0.0 now normalise to the integer span ID; non-integral floats remain rejected.
| span_id = raw | ||
| elif isinstance(raw, str): | ||
| try: | ||
| span_id = int(raw.strip()) |
There was a problem hiding this comment.
Replacing raw.strip().isdigit() with a bare int(raw.strip()) over-accepts several forms that then resolve to a valid-looking index:
| input | result |
|---|---|
"1_0" |
10 (PEP 515 underscores) |
"\u0663" |
3 (Arabic-Indic) |
"\uff11" |
1 (fullwidth) |
"+1" |
1 |
A model emitting "1_0" for span 1 has its verdict applied to span 10, and with the new pessimistic dedup below that verdict can overwrite span 10's real judgment. re.fullmatch(r"[0-9]+", raw.strip()) keeps the intended "\u00b2" rejection without opening these up.
There was a problem hiding this comment.
Fixed in 8ceb250. String IDs now require ASCII digits via [0-9]+, rejecting signed, underscore, Arabic-Indic, and fullwidth forms. Added regression coverage.
| support_level = "NOT_SUPPORTED" | ||
|
|
||
| result[span_id] = support_level | ||
| existing_support = result.get(span_id) |
There was a problem hiding this comment.
This dedup collapses duplicate flat judgments to the worst label, which is the wrong direction when the model emits one row per (span, evidence) pair rather than one row per span.
The batch prompt gives each span an evidence array, so [{"span_id": 0, "support_level": "FULLY_SUPPORTED"}, {"span_id": 0, "support_level": "NOT_SUPPORTED"}] plausibly means "evidence 0 fully supports, evidence 1 does not". This branch yields {0: "NOT_SUPPORTED"}, failing a span that one piece of evidence fully supports.
A span is supported if any evidence supports it, so aggregation across per-evidence rows should be optimistic (max support), not minimum. Worth being deliberate about, since the PR extends the existing nested-path pessimism at lines 756-765 to the flat path, where main used last-wins.
There was a problem hiding this comment.
The duplicate rows are ambiguous: they could represent separate evidence items, but they can also be contradictory span-level judgments. Optimistic aggregation cannot safely be the default for groundedness validation because it turns an explicit NOT_SUPPORTED verdict into a passing result. The prompt requests one flat object per span, not one per evidence item, so duplicates are malformed or contradictory output rather than an established evidence-level contract. Conservative aggregation therefore fails closed and is independent of output order. 8ceb250 adds coverage for flat, nested, and mixed duplicate output.
Preserve boolean and numeric necessity labels, strictly normalise span identifiers, and fail closed on contradictory support judgments. Add regression coverage for malformed model output and optional backend collection. Assisted-by: Codex Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
|
Two review-level follow-ups:
|
Pull Request
Issue
Fixes #1614
Description
GroundednessRequirementshould use a valid model judgment as returned. Forexample, the model may return:
[{"span_id": "0", "support_level": "FULLY_SUPPORTED"}]Before this fix, the parser preserved
"0"as a string key:The parser now converts the ID to integer
0before storing it:The parser now normalises model-returned IDs and labels for both citation
support and citation necessity. Invalid values retain conservative behaviour,
and duplicate judgments are aggregated pessimistically.
Testing
Focused verification:
uv run --extra hf pytest test/stdlib/requirements/test_groundedness_requirement.py -q— 39 passed, 5 skippeduv run ruff check mellea/stdlib/requirements/rag.py test/stdlib/requirements/test_groundedness_requirement.pyuv run mypy mellea/stdlib/requirements/rag.pyAttribution
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.