feat(evaluation) 4/15: fold promptfoo output into retrieval and answer metrics - #815
feat(evaluation) 4/15: fold promptfoo output into retrieval and answer metrics#815Ahmath-Gadji wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughEvaluation now exposes promptfoo result extraction and summarization utilities. Retrieval outputs produce ranking and recall metrics, answer outputs produce pass and component scores, and per-case details include matched retrieved identifiers. Unit tests cover payload shapes, missing data, grading, and filename normalization. ChangesEvaluation metrics
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant EvalTestCase
participant summarize
participant retrieval_payload
participant answer_payload
participant EvaluationMetrics
summarize->>retrieval_payload: extract_results()
summarize->>answer_payload: extract_results()
summarize->>EvalTestCase: match queries and expected sources
summarize->>EvaluationMetrics: compute retrieval and answer aggregates
EvaluationMetrics-->>summarize: metrics and per-case details
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Ahmath-Gadji
changed the title
44c9830 to
7e09c04
Compare
2860e1b to
daff0a7
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
openrag/core/evaluation/metrics.py (1)
228-236: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: merge the hit-rank and recall passes over
documents.
matched(for hit/MRR) andfound(for recall) both iteratedocumentsindependently. They could be computed in a single pass, e.g. by unioning all document identifier sets once and intersecting withexpected. Given the small size of retrieval result lists in this offline evaluation tool, this is a readability nit rather than a real hot path.♻️ Possible simplification
- matched = [rank for rank, (_, identifiers) in enumerate(documents, start=1) if identifiers & expected] - detail.hit = bool(matched) - detail.reciprocal_rank = 1.0 / matched[0] if matched else 0.0 - hits.append(1.0 if matched else 0.0) - reciprocal_ranks.append(detail.reciprocal_rank) - found = {name for name in expected if any(name in ids for _, ids in documents)} - recalls.append(len(found) / len(expected)) + matched = [rank for rank, (_, identifiers) in enumerate(documents, start=1) if identifiers & expected] + detail.hit = bool(matched) + detail.reciprocal_rank = 1.0 / matched[0] if matched else 0.0 + hits.append(1.0 if matched else 0.0) + reciprocal_ranks.append(detail.reciprocal_rank) + all_ids = {i for _, ids in documents for i in ids} + recalls.append(len(expected & all_ids) / len(expected))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openrag/core/evaluation/metrics.py` around lines 228 - 236, Optionally simplify the ground-truth source evaluation in the case-handling block by deriving recall from a single union of document identifiers, while preserving the existing matched-rank logic for hit and reciprocal-rank metrics. Intersect the union with expected and retain the current recalls calculation, including its behavior for empty expected identifiers.tests/unit/core/evaluation/test_metrics.py (1)
259-271: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
pytest.mark.parametrizeover a manual loop.A failing assertion on an early
metadatavariant stops the loop, so later variants in the same test run are silently skipped until the first failure is fixed. Parametrizing surfaces each case as its own test result.♻️ Suggested refactor
-def test_matching_survives_file_id_sanitisation_on_either_side(): - """Whichever form the metadata carries, and whichever the author wrote, - must match: otherwise the ranking metrics silently read as zero.""" - cases = [EvalTestCase(query="q1", expected_answer="a", expected_file_ids=("A B.pdf",))] - for metadata in ( - {"file_id": "A_B.pdf", "source": "A_B.pdf"}, - {"file_id": "A_B.pdf", "source": "/data/A B.pdf"}, - {"file_id": "A_B.pdf"}, - ): - row = {"vars": {"query": "q1"}, "response": {"output": [{"content": "c", "metadata": metadata}]}} - retrieval, _, _ = summarize(cases=cases, retrieval_payload=[row], answer_payload=[]) - assert retrieval.hit_rate == 1.0, metadata +@pytest.mark.parametrize( + "metadata", + [ + {"file_id": "A_B.pdf", "source": "A_B.pdf"}, + {"file_id": "A_B.pdf", "source": "/data/A B.pdf"}, + {"file_id": "A_B.pdf"}, + ], +) +def test_matching_survives_file_id_sanitisation_on_either_side(metadata): + """Whichever form the metadata carries, and whichever the author wrote, + must match: otherwise the ranking metrics silently read as zero.""" + cases = [EvalTestCase(query="q1", expected_answer="a", expected_file_ids=("A B.pdf",))] + row = {"vars": {"query": "q1"}, "response": {"output": [{"content": "c", "metadata": metadata}]}} + retrieval, _, _ = summarize(cases=cases, retrieval_payload=[row], answer_payload=[]) + assert retrieval.hit_rate == 1.0(requires
import pytestat the top of the file)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/core/evaluation/test_metrics.py` around lines 259 - 271, Refactor test_matching_survives_file_id_sanitisation_on_either_side to use pytest.mark.parametrize with each metadata variant as a separate case, adding the required pytest import. Preserve the existing EvalTestCase setup, summarize call, and hit_rate assertion while removing the manual loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@openrag/core/evaluation/metrics.py`:
- Around line 228-236: Optionally simplify the ground-truth source evaluation in
the case-handling block by deriving recall from a single union of document
identifiers, while preserving the existing matched-rank logic for hit and
reciprocal-rank metrics. Intersect the union with expected and retain the
current recalls calculation, including its behavior for empty expected
identifiers.
In `@tests/unit/core/evaluation/test_metrics.py`:
- Around line 259-271: Refactor
test_matching_survives_file_id_sanitisation_on_either_side to use
pytest.mark.parametrize with each metadata variant as a separate case, adding
the required pytest import. Preserve the existing EvalTestCase setup, summarize
call, and hit_rate assertion while removing the manual loop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5ba05720-8432-4bfa-9d60-94f90a65913a
📒 Files selected for processing (3)
openrag/core/evaluation/__init__.pyopenrag/core/evaluation/metrics.pytests/unit/core/evaluation/test_metrics.py
7e09c04 to
8430b48
Compare
daff0a7 to
e595be2
Compare
|
Both nitpicks taken. Recall now folds over a single union. retrieved = {identifier for _, identifiers in documents for identifier in identifiers}
recalls.append(len(expected & retrieved) / len(expected))Equivalent to the old
|
8430b48 to
d08f158
Compare
e595be2 to
19806fd
Compare
d08f158 to
f6f4569
Compare
19806fd to
94ee72b
Compare
…rics `summarize` turns the two promptfoo `results.json` files into hit rate, MRR, recall and context relevance on the retrieval side, pass rate, factuality and rubric score on the answer side, plus the per-question detail the run page tabulates. The ranking definitions follow the write-up already in `tests/load/automatic-evaluation-pipeline/README.md`, so these numbers mean the same thing as the offline pipeline's. Decisions covered by tests: - Rows with no `expected_file_ids` are reported as `skipped_cases`, never as misses. Scoring them as misses would make a sparsely-annotated test set look like a broken retriever. - A document is matched on either identifier it carries, `metadata.source` or `metadata.file_id`, and both sides go through `sanitize_file_id` — a test set naming `A B.pdf` matches the stored `A_B.pdf`. It is displayed by `file_id`, since `source` is a server-side storage path. - promptfoo's output envelope varies by release, so `extract_results` accepts the nested shape or a bare list, and every field read off a row is optional. A dropped row leaves its case unscored rather than raising.
f6f4569 to
e573a8a
Compare
94ee72b to
204206e
Compare
hedhoud
left a comment
hedhoud
left a comment
There was a problem hiding this comment.
The metric calculation looks clear overall, but I found two cases that can make the results misleading.
Provider errors are currently counted as quality failures. For example, if a retrieval request returns an error or promptfoo drops its row, the case is counted as a retrieval miss. An answer error is also counted as a failed answer. This makes an API outage look like poor model quality. Please keep real empty results as misses, but skip or clearly report rows that failed to run. If no valid rows remain, the evaluation should fail instead of completing with misleading scores.
Results are also matched only by the question text. When the test set contains the same question twice with different expected answers, the second promptfoo result is discarded and both cases reuse the first result. Matching by a unique test ID, or rejecting duplicate questions earlier, would prevent incorrect pass rates.
Please add focused tests for provider errors, missing rows, and duplicate questions. Once those cases are covered, the metric logic should be in good shape.
3 participants
Part 4 of 15 of the split of #811. Targets
eval/03-indexing-metrics(#814).What
The second half of
metrics.py.summarizeturns the two promptfooresults.jsonfiles into:plus the per-question
EvalCaseResultdetail the run page tabulates.Ranking definitions follow the write-up already in
tests/load/automatic-evaluation-pipeline/README.md, so these numbers mean the same thing as the offline pipeline's.Notable
expected_file_idsare skipped, not failed. They are reported inskipped_casesand left out of hit rate / MRR / recall. Scoring them as misses would make a sparsely-annotated test set look like a broken retriever, and the count is surfaced so a near-empty ground truth cannot masquerade as a perfect score either.metadata.sourceormetadata.file_id, and both sides run throughsanitize_file_id(part 2) — a test set namingA B.pdfmatches theA_B.pdfthe indexer stored. The document is displayed byfile_id, becausesourceis a server-side storage path and means nothing to the admin reading the table.extract_resultsaccepts{"results": {"results": [...]}}or a bare list, and every field read off a row is optional. promptfoo can drop a row on a provider error; that leaves the case unscored rather than raising mid-run.Review follow-up
Two review nitpicks taken, both in
metrics.py/ its tests:documentsonce per expected id. Rank-sensitive hit/MRR still walks the list in order; recall doesn't care about rank, solen(expected & retrieved) / len(expected)states the definition more directly than the nestedany(...)did.test_matching_survives_file_id_sanitisation_on_either_sideis parametrized. The threemetadatashapes are independent claims, and the manual loop reported only the first failure — hiding whether the others also broke. 34 → 36 tests in the file.Testing
15 further unit tests (34 in the file): first-matching-rank MRR, misses, skipped cases, fractional recall, context-relevance averaging and its
Nonecase, answer component scores, missing rows, and sanitisation matching from either side.ruff, format check and the layer-import guard pass.Summary by CodeRabbit
New Features
Tests