starred/windmill
1
0
Fork 0
mirror of https://github.com/windmill-labs/windmill.git synced 2026-08-23 16:00:38 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: address round-11 review findings on experiments and scorers

Browse Source 
- give scorers the whole case input, not just the message: an answer that came
  from attachments or a replayed conversation could not be judged on it
- accept a judge's boolean and structured {score} answers, including stringified
  ones, and pin every documented scorer shape with a test
- record an experiment for the cases that did launch when a later push fails,
  instead of leaving those jobs running with nothing to attribute them to
- do not capture a preview parent's synthetic runnable_path as a host flow; the
  saved case could not be rerun
- clear the case table before loading a dataset and surface a failed load, so a
  failure cannot leave the previous dataset's cases under the new name
- keep the results table through a refresh of the same experiment
- exclude flow-step jobs from the per-case last-run lookup
- drop case sets from the experiment list, which is only used to pick a run
- report a database failure at the recording lock as itself, not as contention

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
hugocasa
2026-08-11 23:33:16 +02:00
parent 3c96fc3f37
commit 39e2f07f3f
4 changed files with 117 additions and 22 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/windmill-api/openapi.yaml
+3 -1
View File
@@ -11942,7 +11942,9 @@ paths:
- $ref: "#/components/parameters/Path"
responses:
"200":
description: experiments, newest first
description: >
Experiments, newest first. `cases` is empty here — the list is for picking a run; read
an experiment's results to get its case set.
content:
application/json:
schema:
backend/windmill-api/src/ai_evals.rs
+93 -15
View File
@@ -840,7 +840,7 @@ mod with_storage {
"user_message": {
"type": "javascript",
"expr": format!(
"JSON.stringify({{ input: flow_input.user_message, output: {}, expected: flow_input.expected }})",
"JSON.stringify({{ input: flow_input._eval_input, output: {}, expected: flow_input.expected }})",
output_expr
),
}
@@ -850,7 +850,7 @@ mod with_storage {
"type": if scorer.kind == ScorerKind::Script { "script" } else { "flow" },
"path": scorer.path,
"input_transforms": {
"input": { "type": "javascript", "expr": "flow_input.user_message" },
"input": { "type": "javascript", "expr": "flow_input._eval_input" },
"output": { "type": "javascript", "expr": output_expr },
"expected": { "type": "javascript", "expr": "flow_input.expected" },
}
@@ -1000,6 +1000,12 @@ mod with_storage {
if let Some(expected) = &case.expected {
args.insert("expected".to_string(), expected.clone());
}
// The whole case input, for scorers: the message alone cannot explain an answer that came
// from attachments or a replayed conversation.
args.insert(
"_eval_input".to_string(),
serde_json::value::to_raw_value(&case.input)?,
);
// Self-describing run: opened cold from the runs page, the job says what it was evaluating.
// Extra flow inputs are inert — the agent step reads only user_message/user_attachments.
args.insert(
@@ -1254,8 +1260,13 @@ mod with_storage {
None => None,
};
let case_count = cases.len();
let experiment_id = Uuid::new_v4();
let mut experiment_cases = Vec::with_capacity(cases.len());
// A push that fails partway leaves the earlier jobs running. They are recorded anyway
// below, so they stay attributable and a retry is a visibly separate experiment rather
// than a silent second run of the same cases.
let mut push_error: Option<Error> = None;
for case in cases {
let new_case = NewEvalCase {
name: case.name.clone(),
@@ -1266,7 +1277,7 @@ mod with_storage {
tags: case.tags.clone(),
source: case.source.clone(),
};
let job_id = push_case_run(
let job_id = match push_case_run(
&authed,
&db,
&user_db,
@@ -1280,7 +1291,14 @@ mod with_storage {
Some(case.id),
Some(experiment_id),
)
.await?;
.await
{
Ok(job_id) => job_id,
Err(e) => {
push_error = Some(e);
break;
}
};
experiment_cases.push(ExperimentCase {
case_id: case.id,
name: case.name,
@@ -1310,14 +1328,17 @@ mod with_storage {
let mut tx = db.begin().await?;
lock_dataset(&mut tx, &w_id, &payload.dataset, LOCK_ATTEMPTS_LAUNCH)
.await
.map_err(|_| {
Error::internal_err(format!(
.map_err(|e| match e {
// Only a lock timeout means another writer held it; anything else is a database
// failure and must not be reported as contention.
Error::Generic(axum::http::StatusCode::CONFLICT, _) => Error::internal_err(format!(
"Eval dataset {} stayed locked by another writer; this experiment's {} jobs are \
running but no experiment was recorded for them. Do not retry — that would run \
the dataset again on top of them.",
payload.dataset,
experiment.cases.len()
))
)),
other => other,
})?;
let still_there = match read_dataset(&client, &payload.dataset).await {
Ok(dataset) => Some(dataset.created_at),
@@ -1341,6 +1362,17 @@ mod with_storage {
)
.await?;
tx.commit().await?;
if let Some(e) = push_error {
return Err(Error::internal_err(format!(
"Experiment {} launched {} of {} cases before failing: {}. The launched cases are \
recorded under that experiment; rerunning starts a new one.",
experiment_id,
experiment.cases.len(),
case_count,
e
)));
}
Ok(experiment_id.to_string())
}
@@ -1374,6 +1406,11 @@ mod with_storage {
.collect::<Vec<_>>()
.await;
experiments.sort_by(|a, b| b.created_at.cmp(&a.created_at));
// The list is for the picker: shipping every experiment's whole case set would send the
// dataset back once per run.
for experiment in &mut experiments {
experiment.cases.clear();
}
Ok(Json(experiments))
}
@@ -1432,15 +1469,21 @@ mod with_storage {
Some(serde_json::Value::Number(n)) => n.as_f64(),
Some(serde_json::Value::Bool(b)) => Some(if *b { 1.0 } else { 0.0 }),
// An agent scorer answers in `output`, which is itself often a JSON string.
// An agent scorer wraps its answer in `output`, which is itself a number, a
// boolean, a structured {score}, or a string holding any of those.
_ => match map.get("output") {
Some(serde_json::Value::Number(n)) => n.as_f64(),
Some(serde_json::Value::String(s)) => s
.trim()
.parse::<f64>()
.ok()
.or_else(|| serde_json::from_str::<serde_json::Value>(s).ok().and_then(
|v| v.get("score").and_then(|s| s.as_f64()),
)),
Some(serde_json::Value::Bool(b)) => Some(if *b { 1.0 } else { 0.0 }),
Some(serde_json::Value::Object(inner)) => match inner.get("score") {
Some(serde_json::Value::Number(n)) => n.as_f64(),
Some(serde_json::Value::Bool(b)) => Some(if *b { 1.0 } else { 0.0 }),
_ => None,
},
Some(serde_json::Value::String(s)) => s.trim().parse::<f64>().ok().or_else(|| {
serde_json::from_str::<serde_json::Value>(s)
.ok()
.and_then(|v| extract_score(&serde_json::value::to_raw_value(&v).ok()?))
}),
_ => None,
},
},
@@ -1657,7 +1700,13 @@ mod with_storage {
.fetch_optional(&mut *tx)
.await?;
if let Some(parent) = parent {
host_flow_path = parent.runnable_path.clone();
// Only a real flow can be resolved again. A preview parent — which is what every
// eval run and module test is — has a synthetic `runnable_path`, and recording it
// would make the captured case fail to rerun.
host_flow_path = match parent.kind.as_str() {
"flow" | "flownode" => parent.runnable_path.clone(),
_ => None,
};
let value = match parent.raw_flow {
Some(raw_flow) => Some(raw_flow.0),
None => match (parent.kind.as_str(), parent.runnable_id) {
@@ -1818,6 +1867,35 @@ mod with_storage {
mod tests {
use super::*;
fn raw(json: &str) -> Box<RawValue> {
serde_json::from_str(json).unwrap()
}
/// A scorer is any runnable, so its answer arrives in whatever shape that runnable returns:
/// a script's bare value, or an agent's answer wrapped in `output` and often stringified.
/// A shape that goes unrecognised is a silently empty column, not an error.
#[test]
fn extract_score_reads_every_documented_scorer_shape() {
assert_eq!(extract_score(&raw("0.75")), Some(0.75));
assert_eq!(extract_score(&raw("true")), Some(1.0));
assert_eq!(extract_score(&raw(r#"{"score": 0.5}"#)), Some(0.5));
assert_eq!(extract_score(&raw(r#"{"score": false}"#)), Some(0.0));
// agent scorers: the answer is under `output`
assert_eq!(extract_score(&raw(r#"{"output": 0.25}"#)), Some(0.25));
assert_eq!(extract_score(&raw(r#"{"output": true}"#)), Some(1.0));
assert_eq!(extract_score(&raw(r#"{"output": "0.9"}"#)), Some(0.9));
assert_eq!(extract_score(&raw(r#"{"output": {"score": 0.8}}"#)), Some(0.8));
assert_eq!(
extract_score(&raw(r#"{"output": "{\"score\": 0.4}"}"#)),
Some(0.4)
);
// nothing numeric to plot: left empty rather than guessed at
assert_eq!(extract_score(&raw(r#"{"output": "not a score"}"#)), None);
assert_eq!(extract_score(&raw(r#"{"verdict": "good"}"#)), None);
}
/// The stamp is what makes a run findable again, and overflowing `runnable_path`'s
/// varchar(255) would truncate it into a path that matches nothing.
#[test]
frontend/src/lib/components/aiEvals/AgentEvalDrawer.svelte
+11
View File
@@ -104,6 +104,12 @@
loadingCases = false
return
}
if (page === 1) {
// Cleared before the request, not after it: a failed load would otherwise leave the
// previous dataset's cases rendered under the newly selected one.
cases = []
totalCases = 0
}
loadingCases = true
try {
const res = await AiEvalsService.listEvalCases({
@@ -115,6 +121,10 @@
if (generation !== caseLoadGeneration) return
cases = page === 1 ? (res.cases ?? []) : [...cases, ...(res.cases ?? [])]
totalCases = res.total ?? cases.length
} catch (err) {
if (generation === caseLoadGeneration) {
sendUserToast((err as any)?.body ?? String(err), true)
}
} finally {
if (generation === caseLoadGeneration) {
loadingCases = false
@@ -369,6 +379,7 @@
const jobs = await JobService.listJobs({
workspace: ws,
scriptPathStart: prefix,
isFlowStep: false,
perPage: 200
})
if (generation !== runsGeneration) return
frontend/src/lib/components/aiEvals/ExperimentResults.svelte
+10 -6
View File
@@ -33,6 +33,7 @@
let rows = $state<ExperimentRow[]>([])
let scorerLabels = $state<string[]>([])
let loading = $state(false)
let shownExperimentId: string | undefined = undefined
// The comparison is the point of the table: one experiment's numbers say little, the delta
// against the run before the change says whether the change helped.
let baselineId = $state<string | undefined>(undefined)
@@ -73,12 +74,15 @@
let resultsGeneration = 0
async function loadResults(path: string | undefined, id: string | undefined) {
const generation = ++resultsGeneration
// Cleared up front, not on arrival: the header names the new selection as soon as it
// changes, so leaving the old rows up labels one experiment's numbers as another's — and a
// request that fails would leave them there for good.
rows = []
scorerLabels = []
scorers = []
// Cleared up front when the selection changes, so the header never names one experiment
// over another's numbers and a failed request cannot leave them there. A refresh of the
// same experiment keeps its rows rather than flashing a skeleton.
if (id !== shownExperimentId) {
rows = []
scorerLabels = []
scorers = []
}
shownExperimentId = id
if (!ws || !path || !id) {
loading = false
return