Repository navigation
Add grader-aware experiment analysis - #54936
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot ensure graders and evals are supported in the analysis |
There was a problem hiding this comment.
Pull request overview
Adds grader-backed experiment analysis by joining assignment history with grader artifacts and computing per-variant statistics.
Changes:
- Resolves and downloads grader observations with exclusion diagnostics.
- Adds statistical comparisons and observation-based readiness.
- Documents grader-backed analysis and adds tests.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/experiments_state.go |
Exposes complete run history internally. |
pkg/cli/experiments_grader_statistics.go |
Implements grader statistical comparisons. |
pkg/cli/experiments_grader_observations.go |
Loads and extracts grader observations. |
pkg/cli/experiments_grader_observations_test.go |
Tests observation and statistics behavior. |
pkg/cli/experiments_fetch.go |
Parses grader frontmatter configuration. |
pkg/cli/experiments_command.go |
Integrates observations into analysis. |
pkg/cli/experiments_analyze_statistics.go |
Adds grader diagnostics and rendering. |
go.mod |
Adds Gonum for distributions. |
docs/src/content/docs/experimental/experiments.md |
Documents grader-backed metrics. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 9/9 changed files
- Comments generated: 3
- Review effort level: Balanced
| case "bayesian_ab": | ||
| probability, err := betaBinomialProbability(controlValues, variantValues) | ||
| if err != nil { | ||
| comparison.Error = err.Error() | ||
| } else { | ||
| comparison.ProbabilitySuperiority = &probability |
There was a problem hiding this comment.
Fixed in f4ab966: graderMetricObservationSet now carries a Direction field populated from the grader's config, and computeGraderMetricComparisons inverts ProbabilitySuperiority (1 - probability) when Direction == "lower_is_better" before assigning it, so the reported result matches the grader's intended direction.
| graderID, isGrader := workflow.ParseExperimentMetricGraderReference(cfg.Metric) | ||
| if !isGrader { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Fixed in f4ab966: added hasValidGraderMetricSuffix to resolveGraderMetricReferences, which rejects graders.<id>.<suffix> references where the suffix isn't exactly value (e.g. .passed or a typo like .vaule) with the existing "expected grader reference format" error.
| // ObservationCount is the number of usable grader measurements for this variant. | ||
| ObservationCount int `json:"observation_count,omitempty"` | ||
|
|
||
| // Mean is the arithmetic mean of usable grader measurements. | ||
| Mean *float64 `json:"mean,omitempty"` | ||
|
|
||
| // Excluded summarizes assigned runs that did not yield a usable grader measurement. | ||
| Excluded []ExcludedObservationSummary `json:"excluded,omitempty"` |
There was a problem hiding this comment.
Fixed in f4ab966: added an Observations []GraderMetricObservation field to VariantAnalysis, populated in buildVariantAnalyses with the usable observations for that variant, so RunID/GraderID/status/value provenance is preserved in the JSON output alongside the count/mean.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added eval-backed metric support in 4f9e3a6 (and follow-up commit): eval-backed metrics ( |
|
/matt |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — no blocking issues, leaving as COMMENT with a few targeted observations.
📋 Key Themes & Highlights
Key Themes
- Type semantic overload (
graderMetricObservationSet.GraderIDholds an eval question ID for eval-backed experiments) — inline comment on line 55 ofexperiments_grader_observations.go. - Four-return-value initialiser (
initializeGraderObservationSets) — inline comment on line 295. - Misleading exclusion label for oversized artifacts (
"artifact malformed"vs"artifact too large") — inline comment on line 188. - Asymmetric merge of grader and eval observation sets in
RunExperimentsAnalyze— inline comment on line 222 ofexperiments_command.go.
Positive Highlights
- ✅ The decomposition of the old monolithic
computeExperimentAnalysisinto well-named helpers is a clear improvement — easier to test and extend. - ✅ Bounded concurrency in
loadGraderRunDataviaconc/poolis correct and safe. - ✅ Both eval and grader observation pipelines are well covered by table-driven tests with explicit exclusion-reason assertions.
- ✅ The re-use of the grader statistical pipeline for eval YES/NO observations is elegant and avoids code duplication.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 55.6 AIC · ⌖ 11.4 AIC · ⊞ 7.6K
Comment /matt to run again
| GraderID string | ||
| ByVariant map[string][]GraderMetricObservation | ||
| Exclusions map[string][]ExcludedObservationSummary | ||
| } |
There was a problem hiding this comment.
[/codebase-design] graderMetricObservationSet and its GraderID field are reused for eval observations, but for eval-backed metrics GraderID will hold an eval question ID — a semantic mismatch that will confuse future readers.
💡 Suggested fix
Consider renaming the field to MetricID or introduce a discriminated type so the caller doesn't need to know whether the ID refers to a grader or an eval question:
type metricObservationSet struct {
MetricID string // grader ID for grader-backed, eval question ID for eval-backed
MetricKind string // "grader" | "eval"
ByVariant map[string][]GraderMetricObservation
Exclusions map[string][]ExcludedObservationSummary
}This keeps the type honest without requiring a full separation of the two pipelines.
@copilot please address this.
There was a problem hiding this comment.
Fixed in f4ab966: renamed graderMetricObservationSet.GraderID to MetricID to reflect that it holds either a grader ID or an eval question ID.
| if !assigned { | ||
| return | ||
| } | ||
| recordedCounts[experimentName][variant]++ |
There was a problem hiding this comment.
[/codebase-design] initializeGraderObservationSets returns four values. Four return values is a sign this function is doing too much and makes call sites hard to read.
💡 Suggested fix
Introduce a small private struct to hold the initialisation state and return it as a single value:
type graderObservationState struct {
assignedCounts map[string]map[string]int
sets map[string]*graderMetricObservationSet
recordedCounts map[string]map[string]int
seen map[string]map[string]struct{}
}Callers become state := initializeGraderObservationSets(...) and field access is self-documenting.
@copilot please address this.
There was a problem hiding this comment.
Fixed in f4ab966: introduced graderObservationState (assignedCounts, sets, recordedCounts, seen) and initializeGraderObservationSets now returns a single *graderObservationState; addMissingAssignmentHistory and appendGraderObservation/appendEvalObservation were updated to take this struct instead of separate map parameters.
| refs := make(map[string]string) | ||
| for experimentName, cfg := range configs { | ||
| if cfg == nil { | ||
| continue |
There was a problem hiding this comment.
[/codebase-design] An oversized grader_results.json (> 10 MB) is excluded with reason "artifact malformed", but the real cause is size, not malformation. This conflation will make diagnostics misleading when an operator sees "artifact malformed" and goes looking for a JSON parse error.
💡 Suggested fix
Introduce a dedicated exclusion reason constant and use it at the size check:
const exclusionArtifactTooLarge = "artifact too large"
// in readGraderResultsArtifact:
if info.Size() > maxGraderResultsBytes {
return graderRunData{ExclusionReason: exclusionArtifactTooLarge}
}@copilot please address this.
There was a problem hiding this comment.
Fixed in f4ab966: added a dedicated exclusionArtifactTooLarge = "artifact too large" constant and readGraderResultsArtifact now uses it for the oversized-artifact case instead of exclusionMalformedArtifact.
| metricEvalResults, | ||
| graderObservationSets, | ||
| ) | ||
|
|
There was a problem hiding this comment.
[/codebase-design] loadGraderObservationSetsForAnalysis and loadEvalObservationSetsForAnalysis use different error-handling conventions. The grader path returns (map, func(), error) while the eval path returns (map, error) — the caller then merges eval sets into the grader map. Unifying these two pipelines into one function would eliminate the merge step and the asymmetry.
💡 Suggested approach
A single loadMetricObservationSets function could dispatch internally based on whether the experiment config references a grader or eval metric, returning one merged map and one cleanup closure. This also removes the need for the 15-line merge loop in RunExperimentsAnalyze.
@copilot please address this.
There was a problem hiding this comment.
Partially addressed in f4ab966: added loadMetricObservationSetsForAnalysis, which wraps both loadGraderObservationSetsForAnalysis and loadEvalObservationSetsForAnalysis and performs the merge internally, so RunExperimentsAnalyze calls a single function and the 15-line merge loop is gone from the caller. Kept the two underlying loaders separate rather than a full dispatch-by-metric-type rewrite, since they load from different artifacts (grader_results.json vs evals.jsonl) and a bigger merge risks destabilizing both pipelines in this change.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in 9c3... (latest push): replaced the manual map-copy loop with |
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot this PR looks close, but it still needs a maintainer-ready finish.
Run: https://github.com/github/gh-aw/actions/runs/32612677145
|
…der-aware-analysis Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
All previously requested review feedback (copilot-pull-request-reviewer and Matt Pocock Skills Reviewer threads) was already addressed in f4ab966 — re-verified each fix is present at HEAD (grader direction inversion, Refreshed the branch by merging Note: |
|
🎉 This pull request is included in a new release. Release: |
Experiment analysis could not use deterministic grader outputs from assigned workflow runs. This change joins historical assignments with completed-run artifacts and analyzes valid grader measurements by variant.
Observation resolution
grader:<id>andgraders.<id>.valuereferences.Analysis
grader_results.json.min_samplesreadiness on usable observations rather than assignments.Diagnostics
Compatibility