mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-07 16:03:21 +00:00
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) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
ec070eab1d
commit
89eaafe6d4
@@ -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.
|
||||
|
||||
@@ -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<String> = 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())
|
||||
}
|
||||
|
||||
|
||||
@@ -36,7 +36,8 @@
|
||||
// against the run before the change says whether the change helped.
|
||||
let baselineId = $state<string | undefined>(undefined)
|
||||
let baselineRows = $state<ExperimentRow[]>([])
|
||||
let baselineLabels = $state<string[]>([])
|
||||
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
|
||||
})
|
||||
)
|
||||
</script>
|
||||
@@ -220,7 +233,7 @@
|
||||
{#if stillRunning > 0}
|
||||
<span>{stillRunning} running</span>
|
||||
{/if}
|
||||
{#each scorerLabels as label, index (label)}
|
||||
{#each scorerLabels as label, index (index)}
|
||||
<span>
|
||||
{label}:
|
||||
<span class="text-primary font-medium">
|
||||
@@ -256,7 +269,7 @@
|
||||
<tr>
|
||||
<Cell head first>Case</Cell>
|
||||
<Cell head>Output</Cell>
|
||||
{#each scorerLabels as label (label)}
|
||||
{#each scorerLabels as label, index (index)}
|
||||
<Cell head>{label}</Cell>
|
||||
{/each}
|
||||
<Cell head last></Cell>
|
||||
@@ -284,7 +297,7 @@
|
||||
{row.output ?? ''}
|
||||
</span>
|
||||
</Cell>
|
||||
{#each scorerLabels as label, index (label)}
|
||||
{#each scorerLabels as _label, index (index)}
|
||||
{@const d = delta(row, index)}
|
||||
<Cell numeric>
|
||||
{row.scores?.[index] != undefined ? row.scores[index]?.toFixed(2) : '—'}
|
||||
|
||||
Reference in New Issue
Block a user