Add metric tuning for custom metrics [feature branch] - #2446
Conversation
![peqy[bot]](https://avatars.githubusercontent.com/in/1930020?s=60&v=4){kind=small}
There was a problem hiding this comment.
Two access/visibility gaps to address before merge:
- Metric-owned rows are hidden from lists and
GETdetail routes, butPUT/DELETEroutes can still mutate them by UUID (e.g./tests/{id}and likely/test_sets/{id}), which breaks the “reachable only through the metric” contract. /test_setsnow excludes metric-owned sets in the list query, butX-Total-Countisn’t applying the same filter, so pagination totals can disagree with returned rows.
Found 3 issues (0 critical, 2 improvements, 1 minor).
| @@ -529,7 +530,12 @@ def has_runs_filter(query): | |||
| query_builder = query_builder.with_custom_filter(has_runs_filter) | |||
|
|
|||
| # Exclude explorer test sets (they use the dedicated /explorer API) | |||
There was a problem hiding this comment.
crud.get_test_sets() now filters out metric-owned tuning sets, but /test_sets' X-Total-Count header is still computed with only exclude_explorer_rows=True.
Fix: update the
/test_setsroute to passcustom_filters=[exclude_metric_owned(models.TestSet)]towith_count_header(...)(mirroring the list query), otherwise pagination totals will leak/include hidden rows.
There was a problem hiding this comment.
Still looks like GET /test_sets’s X-Total-Count will include metric-owned tuning sets: crud.get_test_sets() now filters them out, and with_count_header now supports custom_filters, but @with_count_header(model=models.TestSet, exclude_explorer_rows=True) in routers/test_set.py hasn’t been updated to pass custom_filters=[exclude_metric_owned(models.TestSet)].
There was a problem hiding this comment.
This still seems outstanding: crud.get_test_sets() now filters out metric-owned sets, but GET /test_sets is still decorated with @with_count_header(model=models.TestSet, exclude_explorer_rows=True) and doesn’t pass custom_filters=[exclude_metric_owned(models.TestSet)].
So X-Total-Count will likely include metric-owned tuning sets and disagree with the returned rows.
Fix: update the decorator usage in
routers/test_set.pyto pass the samecustom_filtersused by the list query.
| ) | ||
| if db_test is None: | ||
| raise HTTPException(status_code=404, detail="Test not found") | ||
| # A metric's tuning cases are reachable only through their metric, so the |
There was a problem hiding this comment.
This blocks reading metric-owned tuning cases via /tests/{id} 👍
However, PUT /tests/{id} and DELETE /tests/{id} still operate on crud.get_test(...) without the same guard, so a tuning case could still be mutated/deleted through the public tests API if its UUID is known.
Fix: apply the same
metric_id is not None → 404check inupdate_testanddelete_test(and any other by-id test routes) to enforce the “reachable only through the metric” contract consistently.
There was a problem hiding this comment.
The read path is now guarded, but I still don’t see the same metric_id is not None → 404 check on PUT /tests/{id} and DELETE /tests/{id} (they call crud.get_test(...) directly). That still seems to allow mutating/deleting tuning cases via the public /tests API if the UUID is known.
There was a problem hiding this comment.
Still looks like only the read path is guarded: GET /tests/{id} 404s when metric_id != NULL, but PUT /tests/{id} and DELETE /tests/{id} don’t apply the same check.
Fix: add the same
metric_id is not None → 404guard inupdate_test+delete_test(or enforce it insidecrud.update_test/crud.delete_test).
| @@ -0,0 +1,63 @@ | |||
| import { BaseApiClient } from './base-client'; | |||
There was a problem hiding this comment.
Minor: MetricTuningClient imports UUID from crypto but doesn’t use it (the methods type their params as UUID | string).
Fix: drop the unused
import { UUID } from 'crypto';to avoid lint/build warnings.
There was a problem hiding this comment.
Note: UUID is now used in the method signatures. Suggestion: make it a type-only import (import type { UUID } from 'crypto') or just use string to avoid pulling Node crypto into client bundles.
There was a problem hiding this comment.
Update: UUID is now used in the MetricTuningClient method signatures, so the earlier “unused import” note no longer applies.
However, importing UUID from Node’s crypto in a frontend client is risky (can pull in/require Node polyfills). Suggestion: change it to a type-only import (import type { UUID } from 'crypto') or just type these params as string.
| db_test_set = crud.resolve_test_set(identifier, db, organization_id) | ||
| if db_test_set is None: | ||
| raise HTTPException(status_code=404, detail="Test Set not found with provided identifier") | ||
| # A metric's tuning test set is reachable only through its metric. Hiding it |
There was a problem hiding this comment.
Good to 404 metric-owned tuning sets on the detail route.
One gap to double-check: DELETE /test_sets/{id} (and any update endpoints) can still act on a tuning set by UUID if they don’t reuse this guard.
Fix: apply the same
metric_id != None → 404rule to by-id mutate/delete routes (or route them through this resolver) so tuning sets are reachable only via/metrics/{id}/tuning/....
There was a problem hiding this comment.
resolve_test_set_or_raise() now 404s metric-owned sets (and PUT /test_sets/{identifier} goes through it 👍), but DELETE /test_sets/{test_set_id} still calls crud.delete_test_set(...) directly. Unless delete_test_set itself blocks metric_id != NULL, tuning sets could still be deleted via the public route by UUID.
There was a problem hiding this comment.
resolve_test_set_or_raise() now correctly 404s metric-owned sets (and PUT /test_sets/{identifier} goes through it 👍), but DELETE /test_sets/{test_set_id} still calls crud.delete_test_set(...) directly.
Unless crud.delete_test_set blocks metric_id != NULL (it doesn’t today), a tuning set can still be deleted via the public route by UUID.
Fix: either route delete through
resolve_test_set_or_raise()(or add a similar guard in the delete handler / CRUD layer).
There was a problem hiding this comment.
[Critical] Access-control/visibility contract isn’t enforced on mutation endpoints yet: metric-owned tuning cases/sets are 404’d on read, but can still be updated/deleted via PUT/DELETE /tests/{id} and DELETE /test_sets/{id} if the UUID is known. These should also 404 (or otherwise be blocked) so tuning data is reachable only via /metrics/{id}/tuning/....
[Improvement] /test_sets still uses @with_count_header(...exclude_explorer_rows=True) without the same exclude_metric_owned(TestSet) filter the list query applies, so X-Total-Count can disagree with returned rows.
[Nit] apps/frontend/src/utils/api-client/metric-tuning-client.ts imports UUID as a runtime import; should be import type (or just use string).
Found 3 issues (1 critical, 1 improvement, 1 nit).
|
[Improvement]
[Critical] Metric-owned tuning rows can still be mutated/deleted via public APIs
Found 2 issues (1 critical, 1 improvement). |
Marks a test set and its tests as owned by a single metric, so a metric can carry its own set of labelled cases without it showing up in the user's library. Mirrors how explorer_row sits on both tables. Ownership is an access rule, not a display filter. Metric-owned rows are excluded from /test_sets and /tests, and the detail routes 404 them -- hiding a row from a list while its id still works leaves the id as a working handle to data the feature promises is private. Hiding the lists needed an optional custom_filters parameter on get_items_detail, count_items and with_count_header. Pairing the count with the list matters: a hidden row that is counted but not returned shows up as a phantom page in the grid. It sits beside the existing exclude_explorer_rows flag rather than replacing it -- unifying the two would mean touching every Explorer call site, which is worth doing separately. metric_id is response-only on the schemas -- a client-settable value would let anyone hide a test set from the list, or unhide a metric's tuning set. The test_set index is unique and partial (WHERE metric_id IS NOT NULL): a metric owns at most one tuning test set, and without the constraint two concurrent first writes each create one, after which half the cases live in a set nothing reads. test is not unique -- a metric owns many cases. The migration adds no DML, so it skips the FORCE ROW LEVEL SECURITY dance 7dd69fe35db5 needed for its backfill. Foreign keys go on as NOT VALID and are validated separately so the initial ALTER does not hold ACCESS EXCLUSIVE on test through a full-table scan.
Gives every custom metric its own set of labelled cases: what the metric should
have said, for checking and later improving it. Four routes under
/metrics/{id}/tuning/cases, mounted with resource="metric" so the existing
metric:read|create|update|delete capabilities cover them and no capability
catalog migration is needed.
One case maps onto existing columns:
input -> prompt.content
expected -> prompt.expected_response
output -> test.test_metadata["output"]
rationale -> test.test_metadata["rationale"]
expected sits on prompt.expected_response because that column already reaches
metric evaluation as expected_output via get_test_and_prompt, so scoring a
tuning set later needs no new plumbing to see the human's verdict. output uses
the same key Explorer writes, keeping one recorded-output convention.
The verdict is one string for all three score types, so it is validated against
the owning metric on write: a fixed pair for binary, a number in range for
numeric, one of the metric's own categories for categorical. The same check runs
on read, because a metric's score_type can change long after its cases were
written -- a case that no longer fits comes back marked stale rather than being
deleted or migrated. Staleness is derived, never stored: a stored marker would
still say stale after the metric changed back.
Input, output and the verdict are all required. A case without a verdict carries
no judgement and cannot be scored, so there is no draft state to model.
Only custom metrics can be tuned, enforced here rather than only in the UI. The
frontend hides the tab behind a flag, but these routes are live in every
deployment, and a hidden tab is not an access rule.
The test set is created lazily on the first write, so reads have no side effects
and metrics nobody tunes accumulate nothing. It carries no metric of its own:
the agreement check that will compare a metric's score against the human's
verdict does not exist yet, and a placeholder reserving its seat would be a
user-visible metric row that computes nothing.
Cases are written as a Prompt + Test pair directly rather than through
bulk_create_tests: that service requires a behavior, category and topic and
get_or_creates each, which would file rows like "Metric Tuning" into the
organization's real taxonomy. Explorer avoids it the same way.
A Tuning tab on the metric detail page for adding labelled cases by hand: input, the answer it produced, the verdict expected from this metric, and why. Marked with the beta chip. The verdict control is rendered from the metric's score type -- a pass/fail choice for binary, a bounded number field for numeric, the metric's own categories for categorical. The backend validates the verdict anyway; for a field with a handful of valid values, letting someone type "passed" and rejecting it after submit is a bad trade. The list column renders the same way, so a numeric 0.8 no longer displays as a red failure chip. Cases whose verdict no longer fits the metric are marked stale. Gated on NEXT_PUBLIC_METRIC_TUNING, defaulting to off, so it is absent from every deployment until someone sets it. The code branches on the feature, never on the environment name -- an environment check scatters deployment assumptions through feature code and makes the same feature behave differently by accident depending on where it runs. It is deliberately not a FeatureName: that system mirrors a backend enum driven by GET /features and would need a coordinated backend change. The tab is also hidden for anything that is not a custom metric. The flag alone is not enough -- this page serves rhesis metrics too, and the tuning routes refuse them, so the tab would render and every call it made would 400. ScoreType gains 'binary', which the backend has always returned and the type omitted. src/constants/score-types.ts deliberately still omits it: that constant drives the metric creation form, and offering binary there is a separate change. Built to be easy to remove. Everything lives in a new tuning/ folder plus two api-client files, so deleting the feature is: delete the folder, delete the client and its interfaces, revert three lines in MetricDetailPageTabs and one factory getter. The beta chip sits in the card actions rather than its subtitle: SectionCard wraps the subtitle in a Typography (a <p>) and MUI's Chip renders a <div>.
A tuning case puts the metric in the system-under-test role, so prompt.content is what that system is shown -- and what a metric is shown is a whole case to judge, not just the question. Input, output and the case's own expected response now travel together there as the case payload, serialized as JSON. This gives the case's expected response a home for the first time. Before, input went to prompt.content and output to test_metadata, with no third slot, so a metric whose prompt references a reference answer could not be tuned faithfully -- the case had no way to express one. The verdict stays on prompt.expected_response and is deliberately not in the payload. It is the answer key: read by the agreement check after the metric has spoken, never shown to the metric. Scoring must therefore run the metric under test in the endpoint's slot rather than through evaluate_single_turn_metrics, which would pass expected_response straight into it. test_metadata keeps only the rationale, which is shown to nobody at scoring time. Parsing is total, the same contract as the metadata schema: content that will not parse comes back carrying the raw text as the input, so a case written before this shape existed still renders as something a human can repair rather than vanishing or taking the list down with it. See domain.local/adr/0003 for the option rejected -- keeping the three fields decomposed and assembling at scoring time -- and why.
270c92f to
db633e7
Compare
|
[Improvement]
[Critical] Metric-owned tuning rows can still be mutated/deleted via public APIs
Found 2 issues (1 critical, 1 improvement). |
A case saves with just an input and the answer being judged, so it can be captured in the moment and judged later. Requiring the verdict up front means both have to happen in one sitting, which in practice means neither does. A verdict given later is checked against the metric's score type exactly as on create. An unlabelled case is never stale: staleness is about a verdict that no longer fits, not an absent one, and the two are different work. On update, absence and blankness differ -- omitting the verdict leaves the stored one alone, a blank one takes it back. Without that a client submitting every field could not clear a verdict at all.
The verdict control starts on nothing rather than defaulting to a value, so a captured case is never silently labelled with a judgement its author did not make. Input and output stay required. An unlabelled case is marked in the grid in the same visual language as a stale one -- both mean "this cannot be scored yet" -- with the tooltip saying which. The Status column reads two fields now, so nothing single sorts it.
A tuning test set carries no metric permanently, not until an agreement check arrives. By ADR-0004 the comparison is plain code in the tuning service, so nothing is waiting to be attached and the slot never fills.
There was a problem hiding this comment.
[Critical] Metric-owned tuning rows can still be mutated/deleted via public APIs
PUT /tests/{id}andDELETE /tests/{id}still don’t blockmetric_id != NULL(onlyGET /tests/{id}does).DELETE /test_sets/{id}still bypassesresolve_test_set_or_raise()and can delete metric-owned tuning sets by UUID.
Fix: enforce the same
metric_id != NULL → 404contract on all by-id mutate/delete routes (or enforce it inside the relevant CRUD functions).
[Improvement] GET /test_sets pagination totals still wrong
crud.get_test_sets() now excludes metric-owned tuning sets, and with_count_header supports custom_filters, but read_test_sets still uses @with_count_header(model=models.TestSet, exclude_explorer_rows=True) without custom_filters=[exclude_metric_owned(models.TestSet)], so X-Total-Count will disagree with returned rows.
Fix: pass the same
custom_filterstowith_count_headerinrouters/test_set.py.
[Improvement] Frontend client imports Node crypto
MetricTuningClient imports UUID from crypto in a frontend bundle.
Fix: use
import type { UUID } from 'crypto'or just type params asstring.
akwasigroch
changed the title
* feat(backend): run a metric over its tuning cases The metric is invoked as the system under test: it receives the case payload unpacked into the same arguments it gets in a real run, and never the expected verdict. Routing it through normal metric evaluation would hand it the answer key and make every agreement number meaningless without anything failing. A run creates no rows in the execution tables. Per-case results go on the case's test_metadata, the run summary on the tuning test set's attributes, and only the latest run is kept. See ADR-0004. A case whose metric call fails is recorded as errored and the run continues, so a flaky provider never reads as a bad metric. * test(backend): cover tuning runs The load-bearing one is test_the_metric_never_sees_the_expected_verdict: it asserts the three arguments the evaluator received and that neither the verdict nor the reviewer's rationale appears among them. Nothing else fails loudly if someone reconnects that wire. The metric invocation and the Celery dispatch are both stubbed, so the whole path is deterministic and free of LLM calls. * feat(frontend): show what the metric said about each case A Run metric control, polling while a run is in flight, and the metric's own verdict and reasoning beside the verdict the author expected — which is the whole point of a run. A binary metric's verdict renders as pass/fail rather than 1.0, since 1.0 beside an expected pass reads as a disagreement to a human when it is not. A failed call is marked as an error rather than shown as a verdict. Nothing here starts a run except the button: a poll that could start one would turn opening the tab into an LLM bill. * feat(backend): pick the tuning run's judge model explicitly A tuning run was reaching the SDK's built-in default, the hosted Rhesis LLM, and dying on a 401. Seven fallbacks stood between "which model judges this?" and an answer, and none of them announced itself. The chain is now two steps and an error: the model saved on the metric, else the model configured as the default for evaluation, else refuse the run with a 400 before anything is written or queued. get_user_evaluation_model is deliberately not used -- it conflates "the user configured a model" with "the system has a default", which is the silent step being removed here. A run scored by a judge nobody picked measures nothing and says nothing about it: set the metric's model afterwards and every stored verdict silently refers to a different judge. * fix(backend): count a failed metric call as an errored case invoke_metric_on_case only looked for a top-level "error" key, which the shape it actually receives does not have. The SDK reports its own failures as a result rather than by raising, the local strategy wraps that in MetricResultBuilder.success(), and success() carries neither the SDK's details["error"] nor an error key of its own. So the failure was read as a verdict. A 401 against the judging model gave a run of "1 cases, 0 errored": a categorical metric stored "error" as its answer, a binary one stored "pass" -- an unreachable provider recorded as the metric agreeing -- and a numeric one stored 0.0. The score sentinel is now recognised, except where the metric declares "error" as one of its own categories, and the reason the SDK writes is used for the score types whose sentinel is an ordinary number. The tests build their stub with the real builder, since stubbing the connector's shape instead is what let this through. * fix(backend): stop a tuning verdict being invented from a string Two ways the metric's own answer was read as something it did not say. A binary metric fell through to bool(score) for any string that was not already "pass" or "fail", and every non-empty string is truthy -- so "no", "false" and "0" all rendered as "pass". There is no binary judge in the SDK factory, so a binary metric is backed by one that answers in categories and this is reachable, not theoretical: a yes/no judge agreed with every case. The word is now shown as the metric said it rather than guessed at. The evaluation-model setting is parsed as a UUID by the settings accessor, so a malformed stored value raises ValueError. The walk that reads it used getattr, which only swallows AttributeError, and the router catches only MetricModelNotConfigured -- so a broken setting escaped as a 500 from the one function whose purpose is to refuse cleanly. It now refuses with the 400 it was always meant to.
There was a problem hiding this comment.
[Critical] Metric-owned tuning rows are still reachable via public mutate/delete routes
The “reachable only through the metric” contract is still incomplete:
/tests/{id}:PUTandDELETE(and/tests/{id}/test_sets,/tests/{id}/files) don’t 404 whenmetric_id != NULL./test_sets/{id}:DELETEstill deletes by UUID without themetric_id != NULL → 404guard.
Fix: apply the same guard to all by-id routes (or enforce it in the CRUD layer) so knowing a UUID isn’t a write/delete handle.
[Improvement] GET /test_sets X-Total-Count still won’t match returned rows
with_count_header now supports custom_filters, but read_test_sets still uses @with_count_header(model=models.TestSet, exclude_explorer_rows=True) without custom_filters=[exclude_metric_owned(models.TestSet)].
Fix: pass the same filter used by the list query.
[Improvement] Frontend client imports Node crypto
apps/frontend/src/utils/api-client/metric-tuning-client.ts imports UUID from crypto.
Fix: make it a type-only import (
import type { UUID } from 'crypto') or just usestringto avoid Node polyfills in the client bundle.
Found 3 issues (1 critical, 2 improvements).
1 participant
Purpose
Someone writes a custom metric — an evaluation prompt that judges whether an answer is good — and has no way to find out whether it judges the way they would. The metric runs, produces verdicts, and the only way to check it is to eyeball test results and form an impression. There is nowhere to write down "for this input and this answer, the metric should say pass, and here is why". So a metric that quietly disagrees with its author looks exactly like a metric that works, and the disagreement only surfaces downstream when the numbers look wrong and nobody can say which metric caused it.
This adds the place to write that down. Every custom metric gets its own set of tuning cases: an input, the answer being judged, and the verdict a human expects — with the verdict fillable later, so capturing a case costs nothing at the moment you hit one. Collected from a Tuning tab on the metric's own page.
This release collects and displays cases. It does not score the metric against them. Scoring — the agreement between a metric's verdict and the human's — is the next step on this same branch.
What Changed
metric_idontest_setandtestmarks rows as owned by a metric, mirroring howexplorer_rowalready works on the same two tables. Ownership is an access rule, not a display filter: metric-owned rows are excluded from/test_setsand/tests, and the detail routes 404 them. Hiding a row from a list while its id still works leaves the id as a working handle to data the feature promises is private./metrics/{id}/tuning/cases, mounted withresource="metric"so the existingmetric:read|create|update|deletecapabilities cover them and no capability catalog migration is needed.score_typeunderneath it comes back marked stale rather than being deleted or silently wrong.NEXT_PUBLIC_METRIC_TUNING, defaulting to off, with the verdict control rendered from the metric's score type and nothing preselected on a fresh form.Additional Context
prompt+testpair rather than a dedicated table, and the verdict lives onprompt.expected_response. The reason is that a tuning case puts the metric in the system-under-test role:prompt.contentholds what that system is shown (the whole case payload), andprompt.expected_responseholds what it should have answered (the verdict). The columns mean exactly what they always mean, one level up. See ADR-0001, ADR-0002 and ADR-0003.prompt.expected_responseis passed to a metric as itsexpected_output. Route a metric under test through that path and it is handed the answer key — told the expected response to "How are you?" isfail— and the resulting agreement number is meaningless. Nothing fails loudly; the numbers just come out flattering. The metric under test occupies the endpoint's slot; the expected verdict is read by the comparison afterwards and never fed in. An earlier version of this PR description argued the opposite — that the column was the right home because it already reaches metric evaluation — which is a level confusion and is corrected here.FeatureName: that system mirrors a backend enum driven byGET /featuresand would need a coordinated backend change for something still experimental.ScoreTypegains'binary', which the backend has always returned and the frontend type omitted.src/constants/score-types.tsstill omits it on purpose — that constant drives the metric creation form, and offering binary there is a separate change.Testing
cd apps/backend && uv run pytest ../../tests/backend/routes/test_metric_tuning.py ../../tests/backend/schemas/test_metric_tuning_metadata.py ../../tests/backend/services/metric_tuning/— 72 tests, of which 53 cover the four routes: every verdict-validation shape per score type, the required fields, saving and later judging an unlabelled case, the non-custom refusal, stale marking after ascore_typechange, and the visibility contract (absent from both lists, 404 on both detail routes, andX-Total-Countagreeing with the rows returned).The neighbouring suites still pass:
test_test_set_update.py,test_test_test_sets.py,test_test_bulk_delete.py,test_explorer.py— 101 tests. Explorer's own by-id access is deliberately unaffected, since it keys offexplorer_rowrather thanmetric_id.Frontend:
npx jest --testPathPatterns "MetricTuning|metric-tuning"— 21 tests, of which 17 cover the tab: the stale marker, the unlabelled marker, each score-type control variant, and that nothing is preselected on a fresh form.To exercise it by hand, add
NEXT_PUBLIC_METRIC_TUNING=truetoapps/frontend/.env.local, restart the frontend, and open a metric whose backend type iscustom. The tab will not appear on arhesismetric, by design.