Add tuning runs for custom metrics - #2470
Conversation
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.
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.
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.
![peqy[bot]](https://avatars.githubusercontent.com/in/1930020?s=60&v=4){kind=small}
There was a problem hiding this comment.
[Improvement] Tuning run metric invocation:
-
expected_output=payload.expected_output or ""collapsesNoneinto"". If downstream logic usesNoneto mean “missing ground truth”, passing""can make ground-truth-required metrics run with an empty reference.Fix: pass
payload.expected_outputthrough as-is (and adjust evaluator typing/handling if needed). -
verdict_from_score()for binary metrics falls back tobool(score), which treats any non-empty string as truthy. If connectors ever return stringified scores like'0', this would incorrectly render aspass.Fix: for string scores (non pass/fail), try numeric coercion before
bool(score).
[Improvement] Frontend polling UX:
- When a run is already
running, polling waits for the first interval tick before fetching, so UI can be stale for up to 3s.Fix: call
poll()once immediately when starting the interval.
Overall the shape (latest-only JSONB summary + per-case result, no execution rows) is clear and the tests around “metric never sees expected verdict” are doing the right kind of guardrail.
| # The case's own expected output -- what the *system under test* | ||
| # should have answered. Never prompt.expected_response, which is the | ||
| # expected verdict. See the module docstring. | ||
| expected_output=payload.expected_output or "", |
There was a problem hiding this comment.
[Improvement] expected_output=payload.expected_output or "" changes the “missing expected output” sentinel from None → "".
Fix: consider passing through
payload.expected_outputas-is (allowingNone) and updatingMetricEvaluator.evaluate()typing/handling accordingly. Some downstream logic distinguishesNone(missing) from a present-but-empty string, so""can make ground-truth-required metrics run with an empty reference.
There was a problem hiding this comment.
Still think this is worth fixing. MetricEvaluator.evaluate() is typed as expected_output: str, but downstream LocalStrategy does distinguish None (missing reference → skip ground-truth-required metrics) from "" (treated as provided). With payload.expected_output or "", a missing expected_output becomes "" and can make ground-truth-required metrics run with an empty reference. Suggestion: pass payload.expected_output through as-is and relax MetricEvaluator.evaluate() typing to Optional[str] (connector strategy already normalizes falsy to "" anyway).
| if lowered in BINARY_VERDICTS: | ||
| return lowered | ||
| passing, failing = BINARY_VERDICTS | ||
| return passing if bool(score) else failing |
There was a problem hiding this comment.
[Improvement] For binary metrics, bool(score) will treat any non-empty string as True (e.g. '0', 'false') if connectors return stringified numbers.
Fix: if
scoreis a string and not one ofpass/fail, try coercing numeric-ish strings to float/bool (e.g.'0'→ fail,'1'→ pass) before falling back tobool(score). This avoids misclassifying'0'as pass.
There was a problem hiding this comment.
Addressed now: verdict_from_score() treats non-pass/fail strings as the judge’s own category (returns the string) instead of running them through bool(score), so values like '0'/'false' won’t be misclassified as pass. This thread can be resolved.
| } | ||
| }; | ||
|
|
||
| const timer = setInterval(poll, RUN_POLL_INTERVAL_MS); |
There was a problem hiding this comment.
[Improvement] Polling starts setInterval(poll, …) but doesn’t do an initial poll() call. If the user opens the tab while a run is already running, the UI may sit stale for up to 3s.
Fix: call
poll()once immediately after setting up (or usesetTimeout(poll, 0)), then keep the interval.
There was a problem hiding this comment.
Still applicable: the polling effect sets up setInterval(poll, …) but never does an initial poll(). If the tab opens while a run is already running, the UI can be stale for up to the interval. Suggest calling poll() once immediately after defining it (or setTimeout(poll, 0)) before starting the interval.
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.
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.
There was a problem hiding this comment.
Nice work keeping the metric-under-test wiring explicit (and adding the model-resolution refusal + SDK-failure detection) — this looks consistent with the ADR constraints and the new test coverage is solid.
I still see the 3 prior Improvement threads as the only remaining nits before merge (expected_output None→"" coercion, binary bool(score) on string scores, and doing an immediate poll when opening the tab mid-run).
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.
1f7d939
into
feat/metric-tuning-test-sets
There was a problem hiding this comment.
[Improvement] Preserve expected_output=None vs "" in tuning invocation.
invoke_metric_on_case() currently does expected_output=payload.expected_output or "", which collapses “missing reference” into “empty reference”. In this codebase, LocalStrategy distinguishes None (skip ground-truth-required metrics) from "" (treated as provided), so this can make reference-required metrics run with an empty ground truth.
Fix: pass
payload.expected_outputthrough as-is and relaxMetricEvaluator.evaluate()to acceptOptional[str](connector path already normalizes falsy →"").
[Improvement] Poll once immediately when run is already running.
The polling effect sets an interval but doesn’t call poll() initially, so opening the tab mid-run can show stale state for up to the interval.
Found 2 issues (0 critical, 2 improvements).
|
Two follow-ups I’d still recommend (even though the binary verdict string issue is fixed now): [Improvement] Tuning invoke turns missing In
[Improvement] Polling effect doesn’t do an initial In
Found 2 issues (0 critical, 2 improvements). |
* 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.
* 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.
1 participant
Purpose
A metric author can now write down what their metric should say — a tuning case per situation, each with an expected verdict — but has no way to find out whether it actually says it. So the labelled cases sit there being correct and inert, and checking a metric still means running something, reading the output and forming an impression. Editing an evaluation prompt stays guesswork with no feedback.
This adds the run: press a button and the metric is run over every case it has. Each case then shows what the metric said and why, beside the verdict the author expected. That side-by-side is what you edit an evaluation prompt against.
No agreement number yet — that is ticket 03. Seeing the divergence case by case is already the substance of a run, and the ratio needs the agreed/disagreed/skipped split that ticket brings.
What Changed
services/metric_tuning/invoke.pyunpacks the case payload into the same three arguments the metric receives in a real run — input, output, and the case's own expected output — and nothing else. This is the part of the feature that is easy to get backwards and the reason the whole thing is shaped this way; see the caveat below.test.test_metadata["result"], the run summary onto the tuning test set'sattributes["tuning_run"]. ADR-0004 records why, and the two schema constraints that rule out reusing the execution path.POSTandGET /metrics/{id}/tuning/run. 400 when the metric has no cases (an empty scorecard says nothing; the refusal says what to do), 409 while a run is in flight, 400 on a non-custom metric. ThePOSTis markedmetric:updateexplicitly rather than left to the POST-means-create convention — running writes results onto the metric's own cases, and an explicitmetric:executewould need a capability catalog migration for something still behind a flag.LocalStrategywraps that inMetricResultBuilder.success(), which carries noerrorkey at all. Only the key was being checked, so a 401 against the judging model produced a run of "1 cases, 0 errored" with the failure stored as the metric's answer —erroron a categorical metric,passon a binary one,0.0on a numeric one. The score sentinel and the reason the SDK writes are now both read.get_user_evaluation_modelis deliberately not used: it conflates "the user configured a model" with "the system has a default", which is the silent step being removed. A run scored by a judge nobody chose measures nothing and says nothing about it — set the metric's model afterwards and every stored verdict silently refers to a different judge.failedrather than leftrunning, which would both look like progress and block the next attempt.1.0. The SDK returns a float that it treats as a flag;1.0displayed beside an expectedpassreads as a disagreement to a human even when it is not.Additional Context
prompt.expected_responseis passed to a metric as itsexpected_output. On a tuning case that column holds the expected verdict. Route the 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.test_the_metric_never_sees_the_expected_verdictis the only thing that fails if someone reconnects that wire, which is why it asserts the arguments the evaluator received and that neither the verdict nor the reviewer's rationale appears anywhere among them.MetricTuningRunandMetricTuningCaseResultextend plainBaseModel, not the sharedBase.Basecarriesid/nano_id/project_id, and a nullidon a run response would advertise a handle to something ADR-0004 says is deliberately not a row.NEXT_PUBLIC_METRIC_TUNING, which defaults to off and nothing sets it. The routes are live, which is why the custom-metric refusal is enforced server-side rather than only by hiding the button.Testing
cd apps/backend && uv run pytest ../../tests/backend/routes/test_metric_tuning_runs.py ../../tests/backend/services/metric_tuning/test_invoke.py— 50 tests. The 28 route tests cover starting a run, the in-progress and finished summaries, the no-cases and non-custom refusals, the concurrent-run refusal, per-case verdict and reasoning, every flavour of errored case (the evaluator raising, the evaluator setting anerrorkey, and the shape that actually occurs — a failure the SDK reports as a scored result with no error key, on both a categorical and a numeric metric, plus a metric that offerserroras a real category keeping its verdict), a failed run surfacing as failed and not blocking the next one, only-the-latest-run-kept, a case added after a run having no result, and that a run leaves the cases themselves byte-for-byte unmodified. The 22 unit tests cover verdict rendering per score type, including the words a binary metric's judge can answer with that are notpass/fail—bool()on any of them is true, so they would otherwise all render aspass.The metric invocation and the Celery dispatch are both stubbed, so the suite is deterministic and makes no LLM calls.
Neighbouring suites still pass:
test_metric_tuning.py,test_metric_tuning_metadata.py,services/metric_tuning/— 122 together with the new ones;test_test_set_update.py,test_test_test_sets.py,test_test_bulk_delete.py,test_explorer.py— 101;tasks/test_base_task.py,tasks/test_pipeline_wiring.py— 25.Frontend:
npx jest --testPathPatterns "MetricTuning|metric-tuning"— 31 tests, 10 of them new: the run control appearing only once there are cases, that loading the tab starts nothing, the in-flight state and its progress, the last-run line, the metric's verdict and reasoning on the row, the error marker, and a failed run being reported.npx tsc --noEmitandnpm run lintare clean.To exercise it by hand, add
NEXT_PUBLIC_METRIC_TUNING=truetoapps/frontend/.env.local, restart the frontend, open a custom metric's Tuning tab, add a case or two and press Run metric. A Celery worker must be running for the run to progress pastrunning.