mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-08 00:03:07 +00:00
fix: address round-5 review findings on experiments
- match scorers by label when diffing two experiments; joining by array position subtracted one scorer from another whenever the scorer sets differed - report a row's status from the case job, not the agent step, so a case whose scorer failed no longer reads as a success - delete a dataset's experiments with it: they hold copies of its cases, and a recreated dataset of the same path would have exposed them - select the experiment that Run dataset just started instead of leaving the table on the previous one - expected is scored now, so stop describing it as having no consumer Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
dc3c500178
commit
ec070eab1d
+29
@@ -0,0 +1,29 @@
|
||||
{
|
||||
"db_name": "PostgreSQL",
|
||||
"query": "SELECT id, status::text AS \"status!\" FROM v2_job_completed\n WHERE id = ANY($1) AND workspace_id = $2",
|
||||
"describe": {
|
||||
"columns": [
|
||||
{
|
||||
"ordinal": 0,
|
||||
"name": "id",
|
||||
"type_info": "Uuid"
|
||||
},
|
||||
{
|
||||
"ordinal": 1,
|
||||
"name": "status!",
|
||||
"type_info": "Text"
|
||||
}
|
||||
],
|
||||
"parameters": {
|
||||
"Left": [
|
||||
"UuidArray",
|
||||
"Text"
|
||||
]
|
||||
},
|
||||
"nullable": [
|
||||
false,
|
||||
null
|
||||
]
|
||||
},
|
||||
"hash": "1ecabdaa9c64eb0ac5bcb62b872158a9e9b158626fdd3afa6dc8b4303fe0945c"
|
||||
}
|
||||
@@ -26246,7 +26246,7 @@ components:
|
||||
tool_inputs:
|
||||
type: object
|
||||
expected:
|
||||
description: Reference output. Recorded at capture time; nothing scores it yet.
|
||||
description: Reference output a scorer compares a rerun against.
|
||||
tags:
|
||||
type: array
|
||||
items:
|
||||
@@ -26283,7 +26283,7 @@ components:
|
||||
tool_inputs:
|
||||
type: object
|
||||
expected:
|
||||
description: What the captured run answered.
|
||||
description: What the captured run answered, kept as the reference to score against.
|
||||
source:
|
||||
$ref: "#/components/schemas/EvalCaseSource"
|
||||
agent_path:
|
||||
|
||||
@@ -614,7 +614,23 @@ mod with_storage {
|
||||
// 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.
|
||||
for key in [cases_key(&path), meta_key(&path)] {
|
||||
// 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.
|
||||
use futures::TryStreamExt;
|
||||
let experiment_keys: Vec<String> = client
|
||||
.list(Some(&ObjectPath::from(
|
||||
format!("{}/{}", EXPERIMENTS_PREFIX, path).as_str(),
|
||||
)))
|
||||
.map_ok(|meta| meta.location.to_string())
|
||||
.try_collect()
|
||||
.await
|
||||
.map_err(object_store_error_to_error)?;
|
||||
|
||||
for key in experiment_keys
|
||||
.into_iter()
|
||||
.chain([cases_key(&path), meta_key(&path)])
|
||||
{
|
||||
match client.delete(&ObjectPath::from(key.as_str())).await {
|
||||
Ok(()) => {}
|
||||
Err(ObjectStoreError::NotFound { .. }) => {}
|
||||
@@ -1397,11 +1413,26 @@ mod with_storage {
|
||||
.ok_or_else(|| Error::NotFound(format!("Experiment {} not found", payload.id)))?;
|
||||
let experiment: EvalExperiment = serde_json::from_slice(&bytes)?;
|
||||
|
||||
// One query for every case job's own status: reading the agent step's success reported a
|
||||
// case as successful even when a scorer step had failed.
|
||||
let job_ids: Vec<Uuid> = experiment.cases.iter().map(|c| c.job_id).collect();
|
||||
let statuses = sqlx::query!(
|
||||
"SELECT id, status::text AS \"status!\" FROM v2_job_completed
|
||||
WHERE id = ANY($1) AND workspace_id = $2",
|
||||
&job_ids,
|
||||
&w_id
|
||||
)
|
||||
.fetch_all(&db)
|
||||
.await?
|
||||
.into_iter()
|
||||
.map(|r| (r.id, r.status))
|
||||
.collect::<std::collections::HashMap<_, _>>();
|
||||
|
||||
let mut rows = Vec::with_capacity(experiment.cases.len());
|
||||
for case in &experiment.cases {
|
||||
// Each case is its own flow, so its steps are read back by node id rather than by
|
||||
// walking a nested loop's status.
|
||||
let (output, success) = windmill_queue::get_result_and_success_by_id_from_flow(
|
||||
let output = windmill_queue::get_result_and_success_by_id_from_flow(
|
||||
&db,
|
||||
&w_id,
|
||||
&case.job_id,
|
||||
@@ -1409,8 +1440,8 @@ mod with_storage {
|
||||
None,
|
||||
)
|
||||
.await
|
||||
.map(|(r, s)| (agent_answer(&r), Some(s)))
|
||||
.unwrap_or((None, None));
|
||||
.ok()
|
||||
.and_then(|(r, _)| agent_answer(&r));
|
||||
|
||||
let mut scores = Vec::with_capacity(experiment.scorers.len());
|
||||
for index in 0..experiment.scorers.len() {
|
||||
@@ -1433,11 +1464,10 @@ mod with_storage {
|
||||
input: case.input.clone(),
|
||||
expected: case.expected.clone(),
|
||||
job_id: case.job_id,
|
||||
status: match success {
|
||||
Some(true) => "success".to_string(),
|
||||
Some(false) => "failure".to_string(),
|
||||
None => "running".to_string(),
|
||||
},
|
||||
status: statuses
|
||||
.get(&case.job_id)
|
||||
.cloned()
|
||||
.unwrap_or_else(|| "running".to_string()),
|
||||
output,
|
||||
scores,
|
||||
});
|
||||
|
||||
@@ -78,8 +78,8 @@ draft for review rather than writing anything:
|
||||
everything before it replayed as the agent's memory and whatever the agent answered after it
|
||||
kept as `expected`. Splitting there rather than at the end is what makes a finished
|
||||
conversation — which ends on the assistant — yield a runnable case. Tool messages are left out:
|
||||
their content is keyed to call ids this replay will not reissue. `expected` has no consumer
|
||||
yet; it is recorded now because it is only available at capture time.
|
||||
their content is keyed to call ids this replay will not reissue. `expected` is what a scorer
|
||||
compares a rerun against, and capture time is the only moment it exists.
|
||||
|
||||
## Experiments
|
||||
|
||||
|
||||
@@ -82,6 +82,7 @@
|
||||
let paneTab = $state<'cases' | 'results'>('cases')
|
||||
let scorers = $state<{ kind: 'script' | 'flow' | 'agent'; path: string; name?: string }[]>([])
|
||||
let experimentToken = $state(0)
|
||||
let startedExperimentId = $state<string | undefined>(undefined)
|
||||
let startingExperiment = $state(false)
|
||||
|
||||
async function loadDatasets() {
|
||||
@@ -281,7 +282,7 @@
|
||||
}
|
||||
startingExperiment = true
|
||||
try {
|
||||
await AiEvalsService.runExperiment({
|
||||
startedExperimentId = await AiEvalsService.runExperiment({
|
||||
workspace: ws,
|
||||
requestBody: {
|
||||
dataset: selectedDataset,
|
||||
@@ -602,6 +603,7 @@
|
||||
dataset={selectedDataset}
|
||||
workspace={ws}
|
||||
refreshToken={experimentToken}
|
||||
selectExperimentId={startedExperimentId}
|
||||
/>
|
||||
</div>
|
||||
{/if}
|
||||
|
||||
@@ -14,12 +14,15 @@
|
||||
let {
|
||||
dataset,
|
||||
workspace = undefined,
|
||||
refreshToken = 0
|
||||
refreshToken = 0,
|
||||
selectExperimentId = undefined
|
||||
}: {
|
||||
dataset: string | undefined
|
||||
workspace?: string
|
||||
/** Bumped by the parent when it starts an experiment, to pick the new one up. */
|
||||
refreshToken?: number
|
||||
/** The experiment the parent just started, which becomes the selected one. */
|
||||
selectExperimentId?: string
|
||||
} = $props()
|
||||
|
||||
let ws = $derived(workspace ?? $workspaceStore)
|
||||
@@ -33,6 +36,7 @@
|
||||
// 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 onlyRegressions = $state(false)
|
||||
|
||||
let listGeneration = 0
|
||||
@@ -46,8 +50,11 @@
|
||||
const found = await AiEvalsService.listExperiments({ workspace: ws, path }).catch(() => [])
|
||||
if (generation !== listGeneration) return
|
||||
experiments = found
|
||||
// Newest first from the API, so an unset or vanished selection lands on the latest run.
|
||||
if (!selectedId || !found.some((e) => e.id === selectedId)) {
|
||||
// A just-started experiment wins; otherwise, newest first from the API means an unset or
|
||||
// vanished selection lands on the latest run.
|
||||
if (selectExperimentId && found.some((e) => e.id === selectExperimentId)) {
|
||||
selectedId = selectExperimentId
|
||||
} else if (!selectedId || !found.some((e) => e.id === selectedId)) {
|
||||
selectedId = found[0]?.id
|
||||
}
|
||||
}
|
||||
@@ -88,13 +95,19 @@
|
||||
const id = baselineId
|
||||
const generation = ++baselineGeneration
|
||||
baselineRows = []
|
||||
baselineLabels = []
|
||||
if (!ws || !path || !id) return
|
||||
AiEvalsService.experimentResults({ workspace: ws, requestBody: { dataset: path, id } })
|
||||
.then((res) => {
|
||||
if (generation === baselineGeneration) baselineRows = res.rows ?? []
|
||||
if (generation !== baselineGeneration) return
|
||||
baselineRows = res.rows ?? []
|
||||
baselineLabels = res.scorer_labels ?? []
|
||||
})
|
||||
.catch(() => {
|
||||
if (generation === baselineGeneration) baselineRows = []
|
||||
if (generation === baselineGeneration) {
|
||||
baselineRows = []
|
||||
baselineLabels = []
|
||||
}
|
||||
})
|
||||
})
|
||||
|
||||
@@ -102,9 +115,17 @@
|
||||
Object.fromEntries(baselineRows.map((r) => [r.case_id, r]))
|
||||
)
|
||||
|
||||
// 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.
|
||||
function baselineIndex(index: number): number {
|
||||
return baselineLabels.indexOf(scorerLabels[index])
|
||||
}
|
||||
|
||||
function delta(row: ExperimentRow, index: number): number | undefined {
|
||||
const other = baselineIndex(index)
|
||||
if (other < 0) return undefined
|
||||
const now = row.scores?.[index]
|
||||
const before = baselineByCase[row.case_id]?.scores?.[index]
|
||||
const before = baselineByCase[row.case_id]?.scores?.[other]
|
||||
if (typeof now !== 'number' || typeof before !== 'number') return undefined
|
||||
return now - before
|
||||
}
|
||||
@@ -135,8 +156,10 @@
|
||||
let regressionCount = $derived(baselineId ? rows.filter(isRegression).length : 0)
|
||||
let meanDeltas = $derived(
|
||||
scorerLabels.map((_, index) => {
|
||||
const other = baselineIndex(index)
|
||||
if (other < 0) return undefined
|
||||
const before = baselineRows
|
||||
.map((r) => r.scores?.[index])
|
||||
.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
|
||||
|
||||
Reference in New Issue
Block a user