Expire a tuning run claim that stops advancing - #2537
Merged
akwasigroch merged 3 commits intoAug 20, 2026
Merged
Conversation
added 3 commits
August 20, 2026 17:01
Only the run itself ever cleared its `running` claim, so a worker that died left the claim standing: every later run refused, and the Run button disabled on that same status. Recovery meant editing the database. The claim after a dispatch that never reaches a worker is the second way in -- the slot is committed before `task_launcher` is called. Both get one escape hatch. The summary carries `progressed_at`, touched when the run claims its slot and again after every case, and a claim that has not advanced for fifteen minutes stops refusing anything and reads as failed rather than as forever in progress. The window covers a single case's evaluation, not a whole run, because a long run keeps renewing it. Staleness is derived on read and never written -- a GET does not repair the database, and the next run overwrites the summary anyway. No reset route: the claim expiring on its own is the hatch, and a button would be a second way to start a run that skips the refusal. The dispatch failure is released on the spot instead of waiting the window out, since that side knows no run is coming, and answers 503. `progressed_at` stays internal to the service; the API and the interface are unchanged. The tab already renders `failed` and enables Run on any status but `running`, so an abandoned run recovers by itself. Refs playground/tracker/tuning-runs/issues/04
Routes are the seam, one class per entrance: a stale claim taken over rather than refused, a claim from before the heartbeat existed falling back to `started_at`, a run still advancing left alone so the hatch does not become a second way to double-run a metric, and a refused second press costing the first run nothing. The stale rule gets its own unit tests -- either side of the window and exactly on it, the heartbeat winning over the start, no timestamps at all, an unparseable one, a naive one, and every status that cannot go stale. `_claim` writes the claim a dead worker leaves behind, which is what makes these deterministic without a clock to freeze.
The existing test was named for it but only asserted the error text appeared, never that the previous run's counts were gone -- the behaviour was already right, just untested. Also covers the run control staying usable after a failure, which is the interface half of a claim that no longer advances: the button is disabled on `running`, which is what an abandoned run used to stay forever.
![peqy[bot]](https://avatars.githubusercontent.com/in/1930020?s=60&v=4){kind=small}
akwasigroch
merged commit Aug 20, 2026
36ca70c
into
feat/metric-tuning-test-sets
7 of 8 checks passed
akwasigroch
added a commit
that referenced
this pull request
Aug 26, 2026
* feat(backend): expire a tuning run claim that stops advancing Only the run itself ever cleared its `running` claim, so a worker that died left the claim standing: every later run refused, and the Run button disabled on that same status. Recovery meant editing the database. The claim after a dispatch that never reaches a worker is the second way in -- the slot is committed before `task_launcher` is called. Both get one escape hatch. The summary carries `progressed_at`, touched when the run claims its slot and again after every case, and a claim that has not advanced for fifteen minutes stops refusing anything and reads as failed rather than as forever in progress. The window covers a single case's evaluation, not a whole run, because a long run keeps renewing it. Staleness is derived on read and never written -- a GET does not repair the database, and the next run overwrites the summary anyway. No reset route: the claim expiring on its own is the hatch, and a button would be a second way to start a run that skips the refusal. The dispatch failure is released on the spot instead of waiting the window out, since that side knows no run is coming, and answers 503. `progressed_at` stays internal to the service; the API and the interface are unchanged. The tab already renders `failed` and enables Run on any status but `running`, so an abandoned run recovers by itself. Refs playground/tracker/tuning-runs/issues/04 * test(backend): cover both ways a run claim wedges Routes are the seam, one class per entrance: a stale claim taken over rather than refused, a claim from before the heartbeat existed falling back to `started_at`, a run still advancing left alone so the hatch does not become a second way to double-run a metric, and a refused second press costing the first run nothing. The stale rule gets its own unit tests -- either side of the window and exactly on it, the heartbeat winning over the start, no timestamps at all, an unparseable one, a naive one, and every status that cannot go stale. `_claim` writes the claim a dead worker leaves behind, which is what makes these deterministic without a clock to freeze. * test(frontend): check a failed run leaves no stale figures The existing test was named for it but only asserted the error text appeared, never that the previous run's counts were gone -- the behaviour was already right, just untested. Also covers the run control staying usable after a failure, which is the interface half of a claim that no longer advances: the button is disabled on `running`, which is what an abandoned run used to stay forever.
akwasigroch
added a commit
that referenced
this pull request
Aug 26, 2026
* feat(backend): add metric_id to test_set and test
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 reuses get_items_detail's extra_filter and adds the same
parameter to 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.
* feat(backend): add metric tuning cases
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.
* feat(frontend): add experimental metric tuning tab
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>.
* style(backend): format metric tuning router
* feat(backend): store the whole case in the prompt content
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.
* feat(backend): make the tuning case verdict optional
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.
* feat(frontend): mark tuning cases with no verdict yet
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.
* docs(backend): drop the reserved metric seat claim
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.
* Add tuning runs for custom metrics (#2470)
* 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.
* fix(backend): rebase metric_id migration onto current head
The migration forked the chain at 82881df987af, leaving two alembic
heads. Point it at 3f5954f6c374 instead.
* fix(backend): resolve get_user from its crud module
get_user moved to crud/user.py in #2458, and reaching it through the crud
package raises AttributeError unless something else imported the submodule
first. Starting a tuning run for a metric with no model of its own hit that
every time.
* refactor(backend): import test crud helpers directly
create_test_set and delete_test are still in the crud monolith, so the
parent-package call works today and breaks silently the day they are
extracted. A direct import fails at startup instead.
* fix(backend): follow main's requirement rename
Rebasing onto main picked up a rename the tuning code still referred to by
the old name: Behavior became Requirement (#2487), so the taxonomy test
asserted a column that no longer exists and failed with AttributeError.
Also notes on get_items_detail that a filter applied there needs the same
filter on count_items, or X-Total-Count disagrees with the rows returned.
* Review what the metric said instead of expecting a verdict (#2533)
* feat(backend): review verdicts instead of expecting them
A tuning case no longer records what the metric should say. Nobody can
honestly author the number a numeric metric ought to return, while saying
whether 0.2 is wrong is easy, so the judgement moves after the run: a
reviewer accepts what the metric said or rejects it with a comment, and
those comments are what someone reads when rewriting an evaluation prompt.
Reviews live beside the run's result in test.test_metadata, accumulate
across runs, and are capped at ten per case with eviction taking an accept
and never a comment. Re-judging a verdict that has not moved replaces the
reviewer's last review, so a corrected mis-click does not spend two slots.
A review stands while the metric's decision has not changed: material_change
buckets both verdicts by the metric's current threshold or passing
categories, derived on read so moving a threshold re-evaluates the reviews
that exist rather than freezing yesterday's arithmetic. A score_type change
invalidates every review and keeps the comments.
Gone with the old model: verdict.py and its per-score-type validation, the
expected verdict, the rationale, the stale and unlabelled markers, and any
write to prompt.expected_response. expected_output becomes reference_answer,
still read under the old key so existing cases keep their text.
Refs domain.local/adr/0005
* test(backend): cover tuning reviews and material change
Routes are the seam: the four outcomes, a rejection needing a comment,
accept-the-rest skipping what has no verdict to judge, replace-not-append,
the cap evicting an accept but never a comment, and a review surviving
0.79 to 0.81 while crossing the threshold sends the case back to unreviewed.
The material-change rule gets its own unit tests -- every threshold
operator, either side and exactly on it, a categorical move across
passing_categories, and each fallback where no bucket can be derived.
Two harness notes worth keeping. Reviews are written by the request session,
so a run driven from the test session expires it first or rewrites metadata
it cached before that commit. And every request in a test shares one
transaction, which makes Postgres now() identical for every case, so results
are keyed on the case's input and assertions on its id rather than on a
list position.
* feat(frontend): review tuning results by exception
The grid reads left to right as the case, then what the metric output, then
the review. A thumb up accepts in one click; a thumb down opens the comment
box the rejection requires. The thumbs are the state as well as the control
-- the pressed one is filled and coloured -- because a chip saying
"Accepted" next to a green thumb says the same thing twice.
An amber warning marks a review a material change took away, which is not
the same as never having had one, and says so on hover. A case whose metric
call failed offers no buttons: there is no verdict there to judge.
"Accept the rest" sits beside Run metric and covers every case still
unreviewed, so forty cases are not forty decisions.
The case drawer loses the verdict control and the rationale, and asks for
the reference answer only when the metric needs one. BaseDataGrid gains an
optional columnGroupingModel, which is what the bands are built from.
* Expire a tuning run claim that stops advancing (#2537)
* feat(backend): expire a tuning run claim that stops advancing
Only the run itself ever cleared its `running` claim, so a worker that died
left the claim standing: every later run refused, and the Run button disabled
on that same status. Recovery meant editing the database. The claim after a
dispatch that never reaches a worker is the second way in -- the slot is
committed before `task_launcher` is called.
Both get one escape hatch. The summary carries `progressed_at`, touched when
the run claims its slot and again after every case, and a claim that has not
advanced for fifteen minutes stops refusing anything and reads as failed
rather than as forever in progress. The window covers a single case's
evaluation, not a whole run, because a long run keeps renewing it.
Staleness is derived on read and never written -- a GET does not repair the
database, and the next run overwrites the summary anyway. No reset route: the
claim expiring on its own is the hatch, and a button would be a second way to
start a run that skips the refusal.
The dispatch failure is released on the spot instead of waiting the window
out, since that side knows no run is coming, and answers 503.
`progressed_at` stays internal to the service; the API and the interface are
unchanged. The tab already renders `failed` and enables Run on any status but
`running`, so an abandoned run recovers by itself.
Refs playground/tracker/tuning-runs/issues/04
* test(backend): cover both ways a run claim wedges
Routes are the seam, one class per entrance: a stale claim taken over rather
than refused, a claim from before the heartbeat existed falling back to
`started_at`, a run still advancing left alone so the hatch does not become a
second way to double-run a metric, and a refused second press costing the
first run nothing.
The stale rule gets its own unit tests -- either side of the window and
exactly on it, the heartbeat winning over the start, no timestamps at all, an
unparseable one, a naive one, and every status that cannot go stale.
`_claim` writes the claim a dead worker leaves behind, which is what makes
these deterministic without a clock to freeze.
* test(frontend): check a failed run leaves no stale figures
The existing test was named for it but only asserted the error text appeared,
never that the previous run's counts were gone -- the behaviour was already
right, just untested.
Also covers the run control staying usable after a failure, which is the
interface half of a claim that no longer advances: the button is disabled on
`running`, which is what an abandoned run used to stay forever.
* Show the metric's agreement over its tuning cases (#2587)
* feat(backend): report agreement over a metric's cases
Agreement is the share of judged cases the reviewer accepted: accepted over
accepted plus rejected. Unreviewed and errored cases are left out of the ratio
and reported beside it, since counting either one in produces a plausible figure
meaning something other than what its reader thinks.
Nothing is stored. The outcomes it folds are themselves derived from the metric's
current threshold on every read, so a review a run has just invalidated stops
counting immediately.
* feat(frontend): show the metric's agreement on the tuning tab
One number above the grid, with the count it was computed over beside it so
three out of three does not read like a solved problem, and the unreviewed and
errored counts that are deliberately not in it.
Two things it will not show: a hundred percent when nothing has been judged, and
any number while a run is in flight -- until the worker has cleared the last
run's results that would be the previous run's figure sitting above a progress
line. The run is re-read after every review, since judging a case moves the
number without a run.
* Narrow the agreement read to the metadata it folds over (#2589)
* perf(backend): read only metadata for the agreement fold
The agreement endpoint is polled by the tuning tab on a timer, and the
fold behind it reads nothing but each case's outcome. Going through
get_tuning_cases eager-loaded the prompt beside it -- the whole case
payload, one blob per case -- to count four outcomes.
get_tuning_case_metadata selects the test_metadata column alone, and
skips the ordering get_tuning_cases needs to keep the grid stable, since
a tally has no order to preserve.
* test(backend): cover agreement after a run dies mid-set
A run clears every case up front and refills them one at a time, so a
worker that dies halfway leaves the cases it never reached carrying no
verdict at all.
Pins that those cases read as unreviewed and stay in the denominator --
the ratio is only ever over the cases this run actually scored, never two
runs' numbers averaged together -- and that the review sitting on a case
the run missed is kept, waiting for a verdict to be about again.
* Match the metric tuning tab to the rest of the app (#2598)
* refactor(frontend): extract the test run stat card
SummaryCard was a private function inside TestRunHeader, so the metric
tuning tab could not show its agreement the way a test run shows its pass
rate. Moved to components/common unchanged — same props, same markup —
and imported back where it was.
* feat(frontend): let a detail tab carry a badge
A tab marking an experimental feature had nowhere to say so, and the badge
cannot go inside the label: Chip renders a div and the label is a span.
The badge is a sibling of the label instead, in a row above the active
underline.
* feat(frontend): match the tuning tab to the rest of the app
The tab was built its own way and read as an experiment bolted onto the metric detail page. Four things now follow what the test run screens already do.
The beta chip moved from the panel header onto the Tuning tab itself. Beside the action buttons it read as a label on the buttons rather than on the feature.
Verdicts are tinted GridBadge pills using the same expression as a test run's Result column, and long cells clamp to two lines with the value in a tooltip instead of a single truncated line. A non-binary verdict stays untinted: a numeric 0.8 in a red pill says the opposite of what the number means.
Agreement is three SummaryCard tiles rather than one dense line — the ratio, how many were rejected, and how many cases are still outstanding. The denominator still travels with the number, and unreviewed and errored cases are still counted out of the ratio and reported apart.
The grid also gains page-size options, hover-revealed row actions and no column menus, matching the other grids. The Case and Metric output header bands stay: the test run grids have nothing to band, and the bands are a decision recorded in the spec.
* feat(frontend): drop the metric tuning flag
NEXT_PUBLIC_METRIC_TUNING was set in no deployment, so the tab was invisible everywhere. The custom-metric check stays and is now the only gate — it always was the real one, since the tuning routes refuse anything else and a hidden tab is not an access rule.
useIsCustomMetric loses its `enabled` parameter along with the flag. One consequence worth knowing: every metric detail page view now costs an extra getMetric call to decide whether to show the tab, which MetricDetailView already fetches. Threading it down instead is a follow-up.
* Improve a metric from the rejections its reviewers wrote (#2611)
* feat(backend): improve a metric from its reviews
POST /metrics/{id}/tuning/improve reads the rejections a reviewer wrote,
asks the generation model to rewrite the metric from them, and returns the
proposed fields without saving. Applying is an ordinary metric update the
caller sends afterwards.
It never writes, because the evaluation prompt is the artifact the whole
tuning feature exists to produce and an in-place LLM rewrite of it has no
diff and no undo. domain.local/adr/0006 records what was rejected,
including applying by calling improve a second time with a save flag --
which returns a different rewrite than the one the reviewer approved.
MetricSynthesizer is deliberately not reused: this prompt is about
reviews, which the SDK has no reason to know about, so the tuning service
owns the template and the ImprovedMetricFields schema end to end. Only the
model client is borrowed, because it is where provider auth, retries and
metering live. The consequence is that the naming, field-depth and
score-type rules now exist in two templates and nothing keeps them in
step; that is accepted, not overlooked.
Two rules keep what is shown and what is saved the same thing. score_type
and the categories list are overwritten with the metric's current values,
since an improvement that moved either would invalidate every review for
the metric. And nothing may be proposed empty over a field the metric has,
because a metric update drops a null rather than writing it -- so a blank
would show as a change, apply successfully, and leave the old value there.
Applying also makes the run on screen stale, so the run summary records a
fingerprint of the verdict-affecting fields at run start. A run whose
fingerprint no longer matches reads as predating the metric. A fingerprint
rather than updated_at, because renaming a metric must not stale a run and
a manual prompt edit must; a missing one reads as unknown, not as stale.
* feat(frontend): improve a metric from its reviews
Improve joins the tuner toolbar beside Run metric, Accept the rest and Add
case. It is enabled only with at least one standing rejection and no run in
flight, and the disabled state carries a tooltip naming which of those is
the reason -- a tooltip that explains what Improve does while the button is
off explains the wrong thing.
While the call is out the button carries a spinner and reads "Improving...",
and a line above the grid says how many rejections are being read. The same
shape as the run's own progress line, because both are a slow thing the tab
started and a second visual language for the second one would be two ways
of saying "wait". The dialog opens when the rewrite arrives, not on click.
The dialog is two columns, current on the left and proposed on the right,
showing only the fields that changed and naming the ones that did not.
The evaluation prompt leads and gets the height.
The proposed side is editable. The model's rewrite is a draft, not a
verdict: someone who can see what is wrong with one clause should be able
to fix that clause rather than discard the whole rewrite. What is in the
boxes stays exactly what Apply saves, which is the invariant ADR-0006 is
about -- asking the model again would return a different rewrite. Editing a
value is not choosing which values apply, so Apply stays all-or-nothing
across fields: the score bands, the steps and the reasoning are written to
agree with each other in one pass.
A blank box is refused rather than treated as "leave this one alone". The
API drops a null on update instead of clearing the field, so an empty box
would apply successfully and change nothing -- and the update body carries
only the fields that have a value, for the same reason.
* fix(frontend): refresh the metric after an apply
MetricDetailView fetches the metric once per id, which was right while it
was the only thing on the page writing one. It is not any more: the Tuning
tab, rendered through tabBody, applies an improvement that rewrites the
evaluation prompt, and Basic Information went on showing the copy read on
mount until the page was reloaded.
The view now takes a refreshKey; bumping it clears the fetch guard and
re-reads. It deliberately does not blank the metric first the way a new id
does -- this is the same metric read again, so emptying the view would flash
a loading state over content that is only slightly out of date. The tabs
component owns the counter and the Tuning tab bumps it, but only after an
apply actually succeeded.
* fix(frontend): raise the proxy budget for improve
The BFF proxy gave /metrics/{id}/tuning/improve the 10s budget sized for
ordinary CRUD, so a whole-metric rewrite came back as a 504 from the proxy
while the backend was still working -- which reads as a broken feature
rather than a slow one.
It belongs on the long-running list, which exists for exactly this: calls
that hold the response until an LLM answers. /tuning/run stays on the short
budget, since starting a run returns 202 straight away.
Adds the first tests for resolveTimeoutMs. They need the node environment,
because the module imports next/server and jsdom has no Request global,
which is why the file had none.
* fix(backend): rebase metric_id migration onto current head
Main added seven migrations on this lineage since the last rebase, so the old
down_revision left the branch as a second alembic head.
* test(backend): patch launch_job after the jobs rename
* style(backend): reformat test set listing after rebase
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
A tuning run claims its slot by writing
runningto the summary, and the next run is refused while that claim stands. Only the run itself ever cleared the claim, which left two ways for one to stay behind forever: a worker that dies mid-run never reachesfail_tuning_run, and a dispatch that raises after the claim is committed means no worker ever picks the run up. Either way every later run is refused, the interface disables its own Run button on that samerunningstatus, and there was no timeout, no reset route and no way out from the interface — recovery meant editing the database.Both entrances want the same escape hatch, which is why they are fixed together rather than by patching the dispatch ordering on its own.
What Changed
progressed_atis touched when the run claims its slot and again after every case. Arunningclaim that has not advanced forSTALE_RUN_AFTER(15 minutes) is stale: it stops refusing new runs, and reads asfailedrather than as forever in progress. The window only has to cover a single case's evaluation plus whatever retries the evaluator already does — a forty-case run keeps renewing it — which is why it can be far shorter than any whole run.services/metric_tuning/staleness.py(new) holds the rule:run_is_stale, andabandonedfor presenting a stale claim as the failure it turned out to be. Derived on read and never written — aGETdoes not repair the database, and the next run overwrites the summary anyway. Same reasoning as the outcome and the threshold bucket in ADR-0005.POST .../tuning/runnow wrapstask_launcher: if the dispatch raises, the claim is released immediately and the caller gets a503, rather than being made to wait out a window for a run that was never queued. That side knows for certain no run is coming.progressed_atstays internal —MetricTuningRunnever exposes it, because the frontend has no use for it. The tab already rendersfailedwith its message and enables Run on any status butrunning, so an abandoned run recovers by itself: the poll flips tofailedwithin one tick and stops.Additional Context
playground/tracker/tuning-runs/issues/04-make-runs-safe-to-press-twice.md, including the note from the review of Add tuning runs for custom metrics #2470 that identified the second entrance.feat/metric-tuning-test-sets, notmain— every ticket in this effort is cut from and merges back into that integration branch, per the Branching section of the tuning-runs spec.mainsees one merge when the feature is finished.progressed_atexisted falls back tostarted_at.Testing
All green: 128 backend tuning tests, 36 frontend Tuning tab tests, ruff clean on the changed files.
New coverage, one class per entrance to the wedge:
TestARunThatNeverFinishes— a stale claim taken over rather than refused, a stale claim reading as failed, a pre-heartbeat claim falling back tostarted_at, a run still advancing left alone (so the hatch does not become a second way to double-run a metric), a long run renewing its own claim, and a refused second press costing the first run nothing.TestADispatchThatNeverReachesAWorker— the503, the claim released, and the next run going through without waiting.test_staleness.py— the rule per branch: either side of the window and exactly on it, the heartbeat winning over the start, no timestamps at all, an unparseable one, a naive one, and every status that cannot go stale.To see the wedge by hand: start a run, kill the worker, and confirm the tab shows the run as failed and lets you press Run again once the window passes.