experiment(eval): statistical significance layer for non-deterministi…#39
Open
AshwinUgale wants to merge 1 commit into
Open
Conversation
…c evals Signed-off-by: AshwinUgale <ugaleashwin@gmail.com>
PR Summary by Qodoexperiment(eval): add statistical significance gating for non-deterministic evals
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo
1. eval-statistical-significance missing 4-digit prefix
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+1
to
+4
| # Experiment: Statistical significance for non-deterministic agent evals | ||
|
|
||
| **Status:** Complete (v1) | ||
| **Relates to:** [fullsend-ai/fullsend#2460](https://github.com/fullsend-ai/fullsend/issues/2460) · `testing-agents.md` open question #1 · `experiments/promptfoo-eval` · `experiments/code-agent-evaluation` |
There was a problem hiding this comment.
1. eval-statistical-significance missing 4-digit prefix 📘 Rule violation ⚙ Maintainability
The new experiment directory name does not follow the required NNNN-short-description/ convention used by this repo. This breaks experiment artifact naming consistency and related tooling/lint expectations.
Agent Prompt
## Issue description
The experiment was added under `eval-statistical-significance/`, which does not match the required experiment artifact naming convention `NNNN-short-description/`.
## Issue Context
The repository’s experiment conventions (and lint tooling) expect a zero-padded 4-digit numeric prefix for each experiment directory.
## Fix Focus Areas
- eval-statistical-significance/EXPERIMENT.md[1-81]
- README.md[33-41]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1
to
+5
| # Experiment: Statistical significance for non-deterministic agent evals | ||
|
|
||
| **Status:** Complete (v1) | ||
| **Relates to:** [fullsend-ai/fullsend#2460](https://github.com/fullsend-ai/fullsend/issues/2460) · `testing-agents.md` open question #1 · `experiments/promptfoo-eval` · `experiments/code-agent-evaluation` | ||
|
|
There was a problem hiding this comment.
2. experiment.md missing yaml frontmatter 📘 Rule violation § Compliance
The experiment’s primary markdown document does not start with YAML frontmatter containing at least title and status. This violates the experiment documentation metadata requirement.
Agent Prompt
## Issue description
`eval-statistical-significance/EXPERIMENT.md` begins directly with an H1 and does not include the required YAML frontmatter block with non-empty `title:` and `status:` fields.
## Issue Context
Existing experiments in this repo use YAML frontmatter at the top of the main experiment markdown (see README conventions).
## Fix Focus Areas
- eval-statistical-significance/EXPERIMENT.md[1-10]
- README.md[37-39]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1
to
+4
| # Experiment: Statistical significance for non-deterministic agent evals | ||
|
|
||
| **Status:** Complete (v1) | ||
| **Relates to:** [fullsend-ai/fullsend#2460](https://github.com/fullsend-ai/fullsend/issues/2460) · `testing-agents.md` open question #1 · `experiments/promptfoo-eval` · `experiments/code-agent-evaluation` |
There was a problem hiding this comment.
3. Readme missing experiment row 📘 Rule violation ≡ Correctness
A new experiment artifact was added but the root README experiment index table was not updated to include it. This leaves the index out of sync with experiments on disk.
Agent Prompt
## Issue description
The root `README.md` experiment index table does not include an entry for the newly added experiment.
## Issue Context
The README is the canonical index of experiments and is expected to have exactly one row per experiment artifact present on disk.
## Fix Focus Areas
- README.md[5-32]
- eval-statistical-significance/EXPERIMENT.md[1-5]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+81
to
+96
| def build_parser() -> argparse.ArgumentParser: | ||
| parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) | ||
| parser.add_argument("results", help="path to a promptfoo results JSON file") | ||
| parser.add_argument( | ||
| "--target", type=float, required=True, | ||
| help="minimum acceptable pass rate, e.g. 0.90", | ||
| ) | ||
| parser.add_argument( | ||
| "--confidence", type=float, default=0.95, | ||
| help="confidence level for the interval (default 0.95)", | ||
| ) | ||
| return parser | ||
|
|
||
|
|
||
| def main(argv: list[str] | None = None) -> int: | ||
| args = build_parser().parse_args(argv) |
There was a problem hiding this comment.
4. build_parser single-use helper 📘 Rule violation ⚙ Maintainability
The new build_parser() abstraction is introduced but is only used once by main() and does not replace duplicated logic. This adds an extra abstraction layer without clear reuse benefit.
Agent Prompt
## Issue description
`build_parser()` is a single-use helper that currently only serves one call site and doesn’t reduce duplication.
## Issue Context
The compliance rule aims to avoid introducing new abstractions that aren’t reused or simplifying existing repetition.
## Fix Focus Areas
- eval-statistical-significance/threshold_check.py[81-96]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+130
to
+136
| def __str__(self) -> str: # pragma: no cover - display only | ||
| verdict = "PASS" if self.passed else "FAIL" | ||
| return ( | ||
| f"{verdict}: {self.observed_rate:.1%} observed over {self.n} trials " | ||
| f"(95% CI [{self.lower_bound:.1%}, {self.upper_bound:.1%}], " | ||
| f"target {self.target_rate:.1%})" | ||
| ) |
There was a problem hiding this comment.
5. Hardcoded 95% ci label 🐞 Bug ◔ Observability
ThresholdResult.__str__ always prints “95% CI” even when threshold_test() is called with a different confidence (e.g. via threshold_check.py --confidence), making CLI/log output misleading.
Agent Prompt
### Issue description
`ThresholdResult.__str__` hardcodes the string `95% CI` even though the interval is computed using the `confidence` parameter. This makes `threshold_check.py` output misleading whenever `--confidence` is not 0.95.
### Issue Context
The CLI supports `--confidence`, and `threshold_test(..., confidence=...)` uses that value to compute the Wilson interval; only the display string is wrong.
### Fix Focus Areas
- eval-statistical-significance/significance.py[119-136]
- eval-statistical-significance/threshold_check.py[88-108]
### Suggested fix
- Add a `confidence: float` field to `ThresholdResult` and populate it in `threshold_test`.
- Update `__str__` to print `{confidence:.0%} CI` (or `{confidence*100:.1f}%`) instead of hardcoding 95%.
- (Optional consistency) Apply the same approach to `ComparisonResult.__str__` as well.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+171
to
+201
| def bootstrap_ci( | ||
| samples: Sequence[float], | ||
| confidence: float = 0.95, | ||
| iterations: int = 10_000, | ||
| statistic: Callable[[Sequence[float]], float] = _mean, | ||
| seed: int | None = None, | ||
| ) -> tuple[float, float]: | ||
| """Percentile bootstrap confidence interval for an arbitrary statistic. | ||
|
|
||
| Makes no normality assumption, which matters for bounded judge scales | ||
| (1-5) where the sampling distribution is skewed near the ceiling. | ||
|
|
||
| `seed` is required for reproducible CI runs; leave it None for analysis. | ||
| """ | ||
| if len(samples) < 2: | ||
| raise ValueError("need at least 2 samples to bootstrap") | ||
| if iterations < 1: | ||
| raise ValueError(f"iterations must be positive, got {iterations!r}") | ||
|
|
||
| rng = random.Random(seed) | ||
| n = len(samples) | ||
| resampled = [ | ||
| statistic([samples[rng.randrange(n)] for _ in range(n)]) | ||
| for _ in range(iterations) | ||
| ] | ||
| resampled.sort() | ||
|
|
||
| alpha = 1.0 - confidence | ||
| lo_idx = int(math.floor((alpha / 2.0) * iterations)) | ||
| hi_idx = min(int(math.ceil((1.0 - alpha / 2.0) * iterations)) - 1, iterations - 1) | ||
| return resampled[lo_idx], resampled[hi_idx] |
There was a problem hiding this comment.
6. Bootstrap params not validated 🐞 Bug ☼ Reliability
bootstrap_ci() and compare_means() don’t validate confidence, and compare_means() also doesn’t validate iterations, so invalid inputs can silently produce nonsensical/inverted intervals (e.g., confidence>1 yields negative indices) or crash (iterations<=0 yields IndexError from empty diffs).
Agent Prompt
### Issue description
`bootstrap_ci()` and `compare_means()` lack input validation for `confidence` (and `compare_means()` also lacks validation for `iterations`). Because percentile indices are derived directly from these parameters, out-of-range values can:
- silently compute incorrect bounds (e.g., negative indices when `confidence > 1.0`), or
- crash at runtime (e.g., `iterations <= 0` makes `diffs` empty in `compare_means`, then indexing raises `IndexError`).
### Issue Context
Other helpers already validate similar parameters (e.g., `_z_two_sided` validates confidence), so these APIs should be consistent and fail fast with `ValueError`.
### Fix Focus Areas
- eval-statistical-significance/significance.py[171-202]
- eval-statistical-significance/significance.py[224-271]
### Suggested fix
- In both `bootstrap_ci` and `compare_means`, add `if not 0.0 < confidence < 1.0: raise ValueError(...)`.
- In `compare_means`, add `if iterations < 1: raise ValueError(...)`.
- (Optional) Add unit tests asserting `ValueError` for confidence outside (0,1) and iterations<=0 in `compare_means`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+47
to
+76
| if stats is not None and "successes" in stats: | ||
| successes = int(stats["successes"]) | ||
| failures = int(stats.get("failures", 0)) | ||
| total = successes + failures | ||
| if total <= 0: | ||
| raise ValueError("stats block reports zero trials") | ||
| return successes, total | ||
|
|
||
| # Shape 2: a per-result array with `success` booleans. | ||
| rows = None | ||
| if isinstance(root, dict) and isinstance(root.get("results"), list): | ||
| rows = root["results"] | ||
| elif isinstance(root, list): | ||
| rows = root | ||
| if rows is not None: | ||
| total = len(rows) | ||
| if total == 0: | ||
| raise ValueError("results array is empty") | ||
| successes = 0 | ||
| for row in rows: | ||
| if isinstance(row, dict): | ||
| if "success" in row: | ||
| successes += 1 if row["success"] else 0 | ||
| elif "pass" in row: | ||
| successes += 1 if row["pass"] else 0 | ||
| else: | ||
| raise ValueError("result row has no 'success'/'pass' field") | ||
| else: | ||
| raise ValueError("result row is not an object") | ||
| return successes, total |
There was a problem hiding this comment.
7. Truthiness miscounts successes 🐞 Bug ≡ Correctness
extract_counts() increments successes based on Python truthiness for row['success']/row['pass'] and coerces stats counts via int(), so malformed JSON types (e.g., "false" as a string, or non-integer counts) can be misinterpreted and produce an incorrect CI gate verdict.
Agent Prompt
### Issue description
`extract_counts()` currently treats any truthy value as a success (e.g., the string `'false'` is truthy and would be counted as success) and silently truncates non-integer stats values via `int(...)`. This can lead to incorrect success/total counts and therefore incorrect threshold gating.
### Issue Context
This tool is intended to be a CI gate. When the input JSON is malformed or has unexpected types, it’s safer to fail closed (raise `ValueError`) than to guess.
### Fix Focus Areas
- eval-statistical-significance/threshold_check.py[41-76]
### Suggested fix
- For stats shape:
- Require `type(stats['successes']) is int` and `type(stats.get('failures',0)) is int` (reject floats/strings/bools).
- Validate non-negative counts and `total > 0`.
- For row shape:
- Require `type(row['success']) is bool` / `type(row['pass']) is bool`.
- Raise `ValueError` if types are not booleans.
- Add a small unit test showing that `'false'` (string) is rejected rather than counted as success.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the experiment proposed in fullsend-ai/fullsend#2460.
Why
Agent evals are non-deterministic — the same agent on the same input can produce different outputs, and therefore different scores, from run to run. A single run is a noisy sample, not a fact, which breaks the usual pass/fail assertion model.
testing-agents.mdflags this as open question #1: "What statistical threshold, and how many runs, make a non-deterministic test reliable?"Two prior experiments here hit the same wall:
promptfoo-evalneeded "a custom threshold wrapper" it didn't build, andcode-agent-evaluationdrew statistical conclusions ("within noise," "statistically equivalent") without a power analysis. This adds the missing machinery — gate noisy runs pass/fail with a stated confidence, and compute how many runs a claim actually needs.What's here
Everything under
eval-statistical-significance/, standard library only (no install step):significance.pythreshold_testgate,compare_means(baseline vs. changed), andmin_trials_*sample-size calculators.threshold_check.pypromptfoo-evalflagged as missing.test_significance.pyEXPERIMENT.md/RECOMMENDATION.mdVerify in ~30s:
Findings
Small-n point estimates don't support the claims made on them. 19/20 looks like 95%, but its 95% CI runs [76%, 99%] — it can't certify a 90% floor. 190/200 (same rate, 10× the trials) tightens to [91%, 97%] and does. Gate on the lower bound, not the point estimate.
Reliability costs runs, and now there's a number for it. Reliably catching a 10-point drop — say a pass rate slipping from 95% to 85% — takes about 140 runs per variant. Catching sub-0.1-point differences in a 1–5 judge score takes hundreds to thousands. The doc lists "cost budget for agent testing" as an open worry; this replaces the worry with an actual figure.
A quick check against an existing experiment.
code-agent-evaluationconcluded that two agent variants (V8 vs. V5/V7) were "statistically equivalent" — but it based that on a ~0.04-point difference on a 1–5 scale, measured from only 3 runs each. With that few runs, random run-to-run noise is far bigger than a 0.04 gap, so the data can't actually tell whether the variants differ. "We found no difference" isn't the same as "there is no difference" — and this is exactly the kind of overclaim the significance layer is built to catch. (I'm keeping this loose rather than exact: their raw scores aren't published, and their test design is a bit more efficient than my calculator assumes, so my figures are an upper bound.)Scope and limits
compare_meansisn't finished — it's an early version of the "did the eval catch the mutation?" decision. Two things still need fixing: it currently flags a score change in either direction, but a mutation "kill" should only count a score drop; and when it reports "no difference" from few runs, that usually means we didn't run enough, not that the eval truly missed the mutation. Getting this right is part of the mutation follow-on.compare_means) that general-purpose stats libraries don't include.Cross-references fullsend-ai/fullsend#2460.