From 89eaafe6d45de6e88e8b19180ca0deaa11ccea75 Mon Sep 17 00:00:00 2001 From: hugocasa Date: Tue, 11 Aug 2026 19:41:59 +0200 Subject: [PATCH] fix: address round-6 review findings on experiments - hold the dataset lock across an experiment launch, so a delete landing between reading the cases and writing the experiment cannot recreate the deleted dataset's inputs - match scorers between experiments on kind and path, not on label: labels default to a path's last segment, so f/a/quality and f/b/quality compared against each other - average mean deltas over the cases both runs scored; comparing each run's own average reported a regression from a case the baseline never ran, with no regressed row to point at - openapi: the row status is the job's, which is also canceled/skipped; runEval takes scorers; the update-case body no longer advertises source, which the handler deliberately ignores - record why the experiment prefix cannot reach a sibling dataset Co-Authored-By: Claude Opus 5 (1M context) --- backend/windmill-api/openapi.yaml | 38 ++++++++++++----- backend/windmill-api/src/ai_evals.rs | 20 +++++---- .../aiEvals/ExperimentResults.svelte | 41 ++++++++++++------- 3 files changed, 68 insertions(+), 31 deletions(-) diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index 6a4a1d7bfa..8542169d8b 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -11745,19 +11745,32 @@ paths: - $ref: "#/components/parameters/WorkspaceId" - $ref: "#/components/parameters/Path" requestBody: - description: updated eval case + description: > + The editable fields of a case. `source` is deliberately absent: provenance is recorded at + capture and never rewritten. required: true content: application/json: schema: - allOf: - - type: object - required: [id] - properties: - id: - type: string - format: uuid - - $ref: "#/components/schemas/NewEvalCase" + type: object + required: [id] + properties: + id: + type: string + format: uuid + name: + type: string + input: + $ref: "#/components/schemas/EvalCaseInput" + host_flow_path: + type: string + tool_inputs: + type: object + expected: {} + tags: + type: array + items: + type: string responses: "200": description: eval case updated @@ -11816,6 +11829,10 @@ paths: $ref: "#/components/schemas/EvalSubject" case: $ref: "#/components/schemas/NewEvalCase" + scorers: + type: array + items: + $ref: "#/components/schemas/ScorerRef" dataset: type: string description: dataset holding the case to run, instead of an inline case @@ -26365,7 +26382,8 @@ components: format: uuid status: type: string - enum: [running, success, failure] + description: The case job's status; `running` until it completes. + enum: [running, success, failure, canceled, skipped] output: type: string description: The agent's answer. The full trajectory stays reachable through job_id. diff --git a/backend/windmill-api/src/ai_evals.rs b/backend/windmill-api/src/ai_evals.rs index 6434615457..94dc5e539b 100644 --- a/backend/windmill-api/src/ai_evals.rs +++ b/backend/windmill-api/src/ai_evals.rs @@ -610,13 +610,14 @@ mod with_storage { let client = object_store(&authed, &db, user_db, &w_id).await?; let mut tx = db.begin().await?; lock_dataset(&mut tx, &w_id, &path).await?; - // Cases first. The metadata is what `create_dataset` checks for, so deleting it first - // would let a failure between the two deletes leave the cases behind *and* unblock - // recreating the path — the new dataset would open holding the old one's cases. This way - // a failure leaves an empty dataset, which is visible and retryable. - // Experiments first, for the same reason cases precede metadata: they hold copies of the - // cases, so a delete that stopped halfway must not leave them readable under a recreated - // dataset of the same path. + // Experiments, then cases, then metadata. The metadata is what `create_dataset` checks + // for, so deleting it first would let a failure partway through leave case copies behind + // *and* unblock recreating the path — the new dataset would open holding the old one's + // data. This order leaves an empty dataset instead, which is visible and retryable. + // + // `list` matches on whole path segments, so this prefix cannot reach a sibling dataset + // whose path merely starts with the same characters (`f/t/foo` vs `f/t/foobar`). Anything + // that replaces it with string-prefix filtering would delete the sibling's experiments. use futures::TryStreamExt; let experiment_keys: Vec = client .list(Some(&ObjectPath::from( @@ -1222,6 +1223,10 @@ mod with_storage { let version = current_resource_version(&db, &w_id, &agent_path).await?; let client = object_store(&authed, &db, user_db.clone(), &w_id).await?; + // Held across the whole launch: without it a delete can land between reading the cases and + // writing the experiment, recreating the deleted dataset's inputs under its own path. + let mut tx = db.begin().await?; + lock_dataset(&mut tx, &w_id, &payload.dataset).await?; let cases = read_cases(&client, &payload.dataset).await?; if cases.is_empty() { return Err(Error::BadRequest(format!( @@ -1292,6 +1297,7 @@ mod with_storage { serde_json::to_vec(&experiment)?, ) .await?; + tx.commit().await?; Ok(experiment_id.to_string()) } diff --git a/frontend/src/lib/components/aiEvals/ExperimentResults.svelte b/frontend/src/lib/components/aiEvals/ExperimentResults.svelte index d01a79e64e..101bcaba4f 100644 --- a/frontend/src/lib/components/aiEvals/ExperimentResults.svelte +++ b/frontend/src/lib/components/aiEvals/ExperimentResults.svelte @@ -36,7 +36,8 @@ // against the run before the change says whether the change helped. let baselineId = $state(undefined) let baselineRows = $state([]) - let baselineLabels = $state([]) + let baselineScorers = $state<{ kind: string; path: string }[]>([]) + let scorers = $state<{ kind: string; path: string }[]>([]) let onlyRegressions = $state(false) let listGeneration = 0 @@ -77,6 +78,7 @@ if (generation !== resultsGeneration) return rows = res.rows ?? [] scorerLabels = res.scorer_labels ?? [] + scorers = (res.experiment?.scorers ?? []).map((sc) => ({ kind: sc.kind, path: sc.path })) } finally { if (generation === resultsGeneration) loading = false } @@ -95,18 +97,21 @@ const id = baselineId const generation = ++baselineGeneration baselineRows = [] - baselineLabels = [] + baselineScorers = [] if (!ws || !path || !id) return AiEvalsService.experimentResults({ workspace: ws, requestBody: { dataset: path, id } }) .then((res) => { if (generation !== baselineGeneration) return baselineRows = res.rows ?? [] - baselineLabels = res.scorer_labels ?? [] + baselineScorers = (res.experiment?.scorers ?? []).map((sc) => ({ + kind: sc.kind, + path: sc.path + })) }) .catch(() => { if (generation === baselineGeneration) { baselineRows = [] - baselineLabels = [] + baselineScorers = [] } }) }) @@ -116,9 +121,12 @@ ) // The two experiments' scorer lists can differ, so a delta is only meaningful between the same - // scorer — matched by label, not by its position in the array. + // scorer. Matched on kind and path rather than label: labels default to a path's last segment, + // so `f/a/quality` and `f/b/quality` both read "quality" and would compare against each other. function baselineIndex(index: number): number { - return baselineLabels.indexOf(scorerLabels[index]) + const mine = scorers[index] + if (!mine) return -1 + return baselineScorers.findIndex((b) => b.kind === mine.kind && b.path === mine.path) } function delta(row: ExperimentRow, index: number): number | undefined { @@ -158,11 +166,16 @@ scorerLabels.map((_, index) => { const other = baselineIndex(index) if (other < 0) return undefined - const before = baselineRows - .map((r) => r.scores?.[other]) - .filter((v): v is number => typeof v === 'number') - if (before.length === 0 || means[index] == undefined) return undefined - return means[index]! - before.reduce((a, b) => a + b, 0) / before.length + // Averaged over the cases both runs scored. Comparing each run's own average would + // report a regression from a case the baseline never ran, with no regressed row to + // point at. + const pairs = rows + .map((r) => [r.scores?.[index], baselineByCase[r.case_id]?.scores?.[other]]) + .filter((p): p is [number, number] => typeof p[0] === 'number' && typeof p[1] === 'number') + if (pairs.length === 0) return undefined + const now = pairs.reduce((a, [n]) => a + n, 0) / pairs.length + const before = pairs.reduce((a, [, b]) => a + b, 0) / pairs.length + return now - before }) ) @@ -220,7 +233,7 @@ {#if stillRunning > 0} {stillRunning} running {/if} - {#each scorerLabels as label, index (label)} + {#each scorerLabels as label, index (index)} {label}: @@ -256,7 +269,7 @@ Case Output - {#each scorerLabels as label (label)} + {#each scorerLabels as label, index (index)} {label} {/each} @@ -284,7 +297,7 @@ {row.output ?? ''} - {#each scorerLabels as label, index (label)} + {#each scorerLabels as _label, index (index)} {@const d = delta(row, index)} {row.scores?.[index] != undefined ? row.scores[index]?.toFixed(2) : '—'}