fix(benchmarking): report how many queries the RAG benchmark actually scored - #6414
Conversation
The benchmark runners skip a conversation when a query or an ingest raises, and retrieval_metrics then averages over the qrels/results intersection. So the dropped queries leave the denominator and the output looks the same as a clean run. On the retrieval side there was no count in the returned dict at all. Adds num_scored_queries and num_missing_queries to retrieval_metrics, and num_missing_queries to generation metrics. No existing number changes. The names avoid num_queries because beir_bench sets that key itself after the call, with a different meaning (queries attempted).
arthi-arumugam-git
changed the title
There was a problem hiding this comment.
Pull request overview
This PR improves the RAG benchmarking harness metrics output by explicitly reporting how many queries were actually scored (and how many were missing) so that partial/errored runs are visible in published results.
Changes:
- Extend
retrieval_metricsto returnnum_scored_queriesandnum_missing_queries, including the no-overlap case. - Extend
answer_metricsto returnnum_missing_queriesalongside existing generation metrics. - Add a pytest file covering complete, partial, and empty-results retrieval runs, ensuring equal scores can be distinguished by counts.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| benchmarking/rag/lib/metrics.py | Adds scored/missing query counts to retrieval metrics and missing-query counts to answer metrics. |
| benchmarking/rag/lib/test_metrics_coverage.py | Adds retrieval metrics coverage tests for complete/partial/no-results scenarios. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| Returns: | ||
| Aggregated metrics dict, e.g. {"ndcg_cut_10": 0.45, "recall_10": 0.78, ...} | ||
| Aggregated metrics dict, e.g. {"ndcg_cut_10": 0.45, "recall_10": 0.78, ...}, | ||
| plus "num_scored_queries" (queries the averages were taken over) and | ||
| "num_missing_queries" (queries in qrels that got no result, usually | ||
| because the run errored past them). Callers already set their own | ||
| "num_queries", so these use distinct names. |
| import pytest | ||
|
|
||
| pytest.importorskip("pytrec_eval") | ||
| pytest.importorskip("evaluate") | ||
|
|
||
| from benchmarking.rag.lib.metrics import retrieval_metrics # noqa: E402 |
…urn types Both from review. scripts/unit-tests.sh runs pytest against tests/unit only, so a test under benchmarking/ was never collected: moved it there and added the sys.path insert it needs, since benchmarking/ is not a package. The return annotations still said dict[str, float] while the functions now also return integer counts, so both are dict[str, float | int] (answer_metrics already returned an int num_queries before this PR, so that one was wrong already).
|
Both fixed, thanks: Test placement. This was the important one. One caveat I should flag rather than hide: the test still needs Return types. Right, and it was already slightly wrong before this PR: |
What happens now
The RAG benchmark runners skip a whole conversation when something raises:
(
doc2dial_bench.pyL152, and the same inqrecc_bench.pyfor both ingest and query.)Those query ids never make it into
results, andretrieval_metricsscores the intersection:So the averages are over whatever survived, and on the retrieval side the returned dict had no count in it at all. A run that lost 40 of 100 queries and a run that lost none look identical in the output. I think that matters here because these numbers get published: PR #5559 put this harness's results in the repo as a comparison against another provider.
Quick check with the current code, five queries in qrels, three results:
What this changes
retrieval_metricsnow also returnsnum_scored_queriesandnum_missing_queries. Generation metrics getsnum_missing_queries(it already reportednum_queries). Nothing else moves, no existing metric value changes.I did not reuse the name
num_queriesfor the scored count becausebeir_bench.pysetsmetrics["num_queries"] = len(queries)right after callingretrieval_metrics, meaning queries attempted. Returning a differently-meaningnum_querieswould have been silently overwritten there, so the new keys have their own names.Tests
benchmarking/rag/lib/test_metrics_coverage.pycovers the complete run, the partial run, and the no-results case, including the case where two runs score the same and only the counts tell them apart.One honest note on how I ran them:
evaluatewill not import on my machine (a native library error), andmetrics.pyimports it at module load, so the test file skips there viaimportorskip. I verified the behaviour by loadingmetrics.pywithevaluateandrouge_scorestubbed, sinceretrieval_metricsuses neither. If someone with the benchmark requirements installed runs the file, that would be a useful second check.Not included
The
continuepaths themselves are untouched, and so iszip(..., strict=False)in the turn handling. Skipping a failed conversation may well be the behaviour you want; this PR is only about making the skip visible in the output. Happy to look at either separately.