Marcos
2e1ad817e7
The filter bar matches on metadata only — not on input and not on the item id — so a dataset seeded with repo/pr in `input` alone could not be sliced by repo at all. Every facet worth filtering on is now a flat primitive in `metadata`: repo, owner, repo_name, pr, head_sha, finding_count, has_findings, max_severity, reviews_run and the review timestamp both ways. `owner` is split out because a filter on the joined repo matches one repo, never a whole org, and `max_severity` is "none" rather than absent because an absent key matches no filter. `eval_experiment.py` links reviews that already ran into a dataset run, one run per model, so the Experiments tab is populated without re-running the reviewer. One trace per (run, item), the most recent: a PR re-reviewed on every push has many traces and a run is one output per input. It posts to the deprecated /api/public/dataset-run-items — the notice exempts self-hosted v3 from the cutoff date and the pilot is stdlib-only by design. Revisit at v4. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
180 lines
8.9 KiB
Markdown
180 lines
8.9 KiB
Markdown
# Evaluation — scorers, ground truth, and the dataset
|
||
|
||
Langfuse already receives one trace per review (`README-langfuse.md`). This is
|
||
the layer on top: numbers attached to those traces that say how the reviewer
|
||
*behaved*, and the beginnings of a ground-truth signal that says whether it was
|
||
*right*.
|
||
|
||
Those two things are deliberately kept apart, because only one of them exists
|
||
yet.
|
||
|
||
## What could and could not be built
|
||
|
||
`feedback.db` has recorded 113 reviews across 4 repos. It has recorded **zero**
|
||
reactions, zero thread resolutions and zero replies. The harvester, the schema
|
||
and the daily analyzer are all working; nobody has ever reacted to a bot
|
||
comment.
|
||
|
||
That rules out an accuracy metric today. Correctness needs labels, and a
|
||
judge scored against no labels is theatre. So the scorers here measure
|
||
behaviour, which is computable from data already in hand, and a separate
|
||
bridge exists to turn human reactions into scores the moment any arrive.
|
||
|
||
## The five behavioural scores
|
||
|
||
Emitted with every review by `eval_scores.py`, folded into the same ingestion
|
||
batch as the trace so they cost no extra request.
|
||
|
||
| score | type | what a change in it means |
|
||
|---|---|---|
|
||
| `finding_rate` | NUMERIC | Findings posted. 0 is the restraint case — good on clean code, a failure when the run degraded. Only the rate over time separates those. |
|
||
| `severity_info_ratio` | NUMERIC 0–1 | Share of findings the model rated `info`/`trivial`. Rising = the model is hedging rather than committing. `None` when the review was silent: a ratio over an empty set is undefined, and charting it as 0 would read as perfect calibration. |
|
||
| `severity_max` | CATEGORICAL | Highest severity surfaced, `none` when silent. Categorical because "did this ever surface something serious" is the real question, and a mean of severity ranks answers nothing. |
|
||
| `dropped_findings` | NUMERIC | Findings the model emitted that the parser rejected for an unusable `path`/`line`. This is the only score here that measures the model's raw output. |
|
||
| `cost_per_finding` | NUMERIC | Equivalent USD per finding. A cheaper model that finds nothing is not cheaper. |
|
||
|
||
### Why `dropped_findings` needed a change to the parser
|
||
|
||
`parse_findings` and `parse_review_output` discard any finding with a missing or
|
||
unusable location. That happens silently, so a model emitting ten findings at
|
||
invalid locations was indistinguishable from a model that found nothing — both
|
||
produce an empty list. `ai_review.last_parse_dropped()` exposes the delta,
|
||
recorded at parse time.
|
||
|
||
It must be read at parse time specifically: by the time findings reach
|
||
`_emit_langfuse`, `apply_repo_config` has already filtered them by
|
||
`severity_threshold` and `max_findings`, and those drops are the config working
|
||
as intended, not the model misbehaving.
|
||
|
||
## Ground truth: `feedback_scores.py`
|
||
|
||
Turns `feedback.db` into two session-level scores, keyed on `"{repo}#{pr}"`
|
||
(which is what `langfuse_trace` already sets as `sessionId`).
|
||
|
||
| score | meaning |
|
||
|---|---|
|
||
| `review_engagement` | Share of a PR's findings that drew any human reaction, resolution or reply. **Watch this first** — every quality number is vapour until it moves off 0. |
|
||
| `review_acceptance` | Net verdict over engaged findings, −1 to +1. Absent, not 0, when nothing was engaged: zero would claim humans judged the review neutral, when the truth is nobody looked. |
|
||
|
||
Session-level rather than trace-level because feedback arrives days later
|
||
against a PR, and nothing in `feedback.db` records which re-run of the reviewer
|
||
produced which comment. The session is both the available join and the honest
|
||
granularity.
|
||
|
||
Score ids are `uuid5(namespace, repo#pr#name)`, so the daily backfill updates
|
||
rather than duplicates.
|
||
|
||
## The dataset
|
||
|
||
`pragent-reviews`, one item per PR the reviewer has run on, seeded by
|
||
`eval_bootstrap.py` from `feedback.db`.
|
||
|
||
`expectedOutput` is **the reviewer's own prior output**, not human-verified
|
||
truth — every item carries `metadata.labelled_by_human: false`. Read it as a
|
||
regression baseline: re-run a candidate model over these PRs and the diff
|
||
against this column is the behaviour change. Promoting an item to real ground
|
||
truth means a human editing it in the dataset view after re-reading the PR.
|
||
|
||
### Item ids
|
||
|
||
`{owner}__{repo}__pr{n}`. The obvious `{repo}#{pr}` cannot be used: items are
|
||
routed as `/datasets/{id}/items/{item_id}`, so the `/` in `owner/repo` splits
|
||
into extra path segments and everything after `#` is a fragment the browser
|
||
never sends — the item is created fine by the API and then 404s when opened.
|
||
Session ids elsewhere keep `{repo}#{pr}`; those are never path segments.
|
||
|
||
### Filterable metadata
|
||
|
||
The filter bar matches on `metadata` only — not on `input`, and not on the item
|
||
id — so every facet worth slicing on is a flat, primitive key in `metadata`
|
||
even where it duplicates `input`:
|
||
|
||
| key | why it is there |
|
||
| --- | --- |
|
||
| `repo`, `owner`, `repo_name` | `owner` exists because a filter on the joined `repo` matches one repo, never a whole org |
|
||
| `pr`, `head_sha` | jump from a filtered row back to the actual PR |
|
||
| `finding_count`, `has_findings` | isolate the silent reviews, which are the interesting negatives |
|
||
| `max_severity` | `"none"` rather than absent — an absent key matches no filter |
|
||
| `reviews_run` | how churny the PR was; high values skew per-item averages |
|
||
| `last_reviewed_at` / `_iso` | epoch sorts, ISO reads |
|
||
| `labelled_by_human` | `false` everywhere today; the flag to filter on before trusting any of it |
|
||
|
||
Nested objects and lists are deliberately absent: the filter bar cannot reach
|
||
into them.
|
||
|
||
`max_severity` is derived from `feedback.db`, whose `severity` column is
|
||
re-parsed out of the rendered comment by `feedback_harvest._parse_severity` and
|
||
defaults to `INFO` when its regex misses the badge. Trust the `severity_max`
|
||
**score** (read from the model's structured output) over this facet.
|
||
|
||
## Experiments
|
||
|
||
`eval_experiment.py` links reviews that already ran into a dataset run, so the
|
||
Experiments tab is populated without re-running anything. Runs are grouped by
|
||
model — the comparison the pilot actually needs is the same PRs under a
|
||
candidate model with `finding_rate` and `cost_per_finding` side by side. A new
|
||
model produces a new run automatically on the next invocation.
|
||
|
||
One trace per (run, item), the most recent: a PR re-reviewed on every push has
|
||
many traces, and a run is one output per input.
|
||
|
||
It uses `POST /api/public/dataset-run-items`, which is deprecated in favour of
|
||
the SDK experiment runner and disappears in Langfuse v4. The deprecation notice
|
||
exempts self-hosted v3 from the cutoff date, and this pilot is stdlib-only by
|
||
design. Revisit when this deployment moves to v4.
|
||
|
||
Coverage is bounded by the dataset, not by the traces: items only exist for PRs
|
||
with a row in `feedback.db`, and a review that posted no comment leaves a trace
|
||
but no row. That is why a run links fewer items than there are traces.
|
||
|
||
## Running it
|
||
|
||
```bash
|
||
# once per project: score configs + dataset (+ score historical traces)
|
||
python3 pilot/eval_bootstrap.py --db /data/feedback.db --backfill-traces
|
||
|
||
# ship feedback verdicts (runs daily from the feedback CronJob)
|
||
python3 pilot/feedback_scores.py --db /data/feedback.db
|
||
|
||
# link already-traced reviews into a dataset run per model
|
||
python3 pilot/eval_experiment.py --dry-run
|
||
python3 pilot/eval_experiment.py
|
||
```
|
||
|
||
Both need `LANGFUSE_HOST`, `LANGFUSE_PUBLIC_KEY`, `LANGFUSE_SECRET_KEY`. In
|
||
cluster they come from the `pragent-langfuse` Secret and point at the ClusterIP
|
||
— never the NodePort, whose oauth2-proxy 302s ingestion to Logto and drops it.
|
||
|
||
## Gotcha: HTTP 207 is not success
|
||
|
||
The ingestion endpoint answers `207 Multi-Status` when *some* events failed, so
|
||
a batch where **every** event was rejected still returns 207. An early version
|
||
of these scorers omitted the required per-event `timestamp` and silently
|
||
ingested nothing while reporting success. `langfuse_trace._warn_on_rejected_events`
|
||
now logs the per-event errors under `LANGFUSE_DEBUG=1`. If scores are missing,
|
||
check that before anything else.
|
||
|
||
## What the first run showed
|
||
|
||
Backfilled over 42 existing traces and 13 PRs:
|
||
|
||
```
|
||
cost_per_finding n=42 mean=0.3133 min=0.0880 max=0.9042
|
||
finding_rate n=42 mean=0.4762 min=0.0000 max=4.0000
|
||
severity_info_ratio n=14 mean=0.0000
|
||
review_engagement n=14 mean=0.0000
|
||
severity_max {none: 28, medium: 11, high: 1, critical: 2}
|
||
```
|
||
|
||
Two things worth keeping:
|
||
|
||
- **The reviewer is not info-heavy.** `feedback.db` shows 61 of 62 findings at
|
||
`INFO`, which looked like a badly calibrated model. It is not: `severity_max`
|
||
reads `medium`/`high`/`critical` on every trace that found anything, and
|
||
`severity_info_ratio` is flat 0. The `INFO` in the DB comes from
|
||
`feedback_harvest._parse_severity`, which defaults to `INFO` when its regex
|
||
misses the severity badge in the rendered comment. The DB severity is a
|
||
re-parse artifact; the score reads the model's structured output directly.
|
||
- **28 of 42 reviews found nothing** (67%), and **engagement is flat zero**. The
|
||
first is not yet interpretable without the second.
|