diff --git a/skills/skill-creator/scripts/aggregate_benchmark.py b/skills/skill-creator/scripts/aggregate_benchmark.py index 3e66e8c1..40f0c4da 100755 --- a/skills/skill-creator/scripts/aggregate_benchmark.py +++ b/skills/skill-creator/scripts/aggregate_benchmark.py @@ -177,7 +177,8 @@ def aggregate_results(results: dict) -> dict: """ Aggregate run results into summary statistics. - Returns run_summary with stats for each configuration and delta. + Returns run_summary with stats for each configuration and, when two or + more configurations exist, a delta between the first two. """ run_summary = {} configs = list(results.keys()) @@ -203,23 +204,22 @@ def aggregate_results(results: dict) -> dict: "tokens": calculate_stats(tokens) } - # Calculate delta between the first two configs (if two exist) + # A delta only exists between two configurations; with a single config + # there is no baseline to compare against, and emitting one against an + # invented zero baseline would fabricate a "+X" improvement. if len(configs) >= 2: primary = run_summary.get(configs[0], {}) baseline = run_summary.get(configs[1], {}) - else: - primary = run_summary.get(configs[0], {}) if configs else {} - baseline = {} - delta_pass_rate = primary.get("pass_rate", {}).get("mean", 0) - baseline.get("pass_rate", {}).get("mean", 0) - delta_time = primary.get("time_seconds", {}).get("mean", 0) - baseline.get("time_seconds", {}).get("mean", 0) - delta_tokens = primary.get("tokens", {}).get("mean", 0) - baseline.get("tokens", {}).get("mean", 0) + delta_pass_rate = primary.get("pass_rate", {}).get("mean", 0) - baseline.get("pass_rate", {}).get("mean", 0) + delta_time = primary.get("time_seconds", {}).get("mean", 0) - baseline.get("time_seconds", {}).get("mean", 0) + delta_tokens = primary.get("tokens", {}).get("mean", 0) - baseline.get("tokens", {}).get("mean", 0) - run_summary["delta"] = { - "pass_rate": f"{delta_pass_rate:+.2f}", - "time_seconds": f"{delta_time:+.1f}", - "tokens": f"{delta_tokens:+.0f}" - } + run_summary["delta"] = { + "pass_rate": f"{delta_pass_rate:+.2f}", + "time_seconds": f"{delta_time:+.1f}", + "tokens": f"{delta_tokens:+.0f}" + } return run_summary @@ -285,10 +285,7 @@ def generate_markdown(benchmark: dict) -> str: # Determine config names (excluding "delta") configs = [k for k in run_summary if k != "delta"] - config_a = configs[0] if len(configs) >= 1 else "config_a" - config_b = configs[1] if len(configs) >= 2 else "config_b" - label_a = config_a.replace("_", " ").title() - label_b = config_b.replace("_", " ").title() + label_a = configs[0].replace("_", " ").title() if configs else "config_a" lines = [ f"# Skill Benchmark: {metadata['skill_name']}", @@ -299,28 +296,42 @@ def generate_markdown(benchmark: dict) -> str: "", "## Summary", "", - f"| Metric | {label_a} | {label_b} | Delta |", - "|--------|------------|---------------|-------|", ] - a_summary = run_summary.get(config_a, {}) - b_summary = run_summary.get(config_b, {}) - delta = run_summary.get("delta", {}) + a_summary = run_summary.get(configs[0], {}) if configs else {} # Format pass rate a_pr = a_summary.get("pass_rate", {}) - b_pr = b_summary.get("pass_rate", {}) - lines.append(f"| Pass Rate | {a_pr.get('mean', 0)*100:.0f}% ± {a_pr.get('stddev', 0)*100:.0f}% | {b_pr.get('mean', 0)*100:.0f}% ± {b_pr.get('stddev', 0)*100:.0f}% | {delta.get('pass_rate', '—')} |") + a_pass_rate = f"{a_pr.get('mean', 0)*100:.0f}% ± {a_pr.get('stddev', 0)*100:.0f}%" # Format time a_time = a_summary.get("time_seconds", {}) - b_time = b_summary.get("time_seconds", {}) - lines.append(f"| Time | {a_time.get('mean', 0):.1f}s ± {a_time.get('stddev', 0):.1f}s | {b_time.get('mean', 0):.1f}s ± {b_time.get('stddev', 0):.1f}s | {delta.get('time_seconds', '—')}s |") + a_time_s = f"{a_time.get('mean', 0):.1f}s ± {a_time.get('stddev', 0):.1f}s" # Format tokens a_tokens = a_summary.get("tokens", {}) - b_tokens = b_summary.get("tokens", {}) - lines.append(f"| Tokens | {a_tokens.get('mean', 0):.0f} ± {a_tokens.get('stddev', 0):.0f} | {b_tokens.get('mean', 0):.0f} ± {b_tokens.get('stddev', 0):.0f} | {delta.get('tokens', '—')} |") + a_tokens_s = f"{a_tokens.get('mean', 0):.0f} ± {a_tokens.get('stddev', 0):.0f}" + + if len(configs) >= 2: + label_b = configs[1].replace("_", " ").title() + b_summary = run_summary.get(configs[1], {}) + delta = run_summary.get("delta", {}) + + b_pr = b_summary.get("pass_rate", {}) + b_time = b_summary.get("time_seconds", {}) + b_tokens = b_summary.get("tokens", {}) + + lines.append(f"| Metric | {label_a} | {label_b} | Delta |") + lines.append("|--------|------------|---------------|-------|") + lines.append(f"| Pass Rate | {a_pass_rate} | {b_pr.get('mean', 0)*100:.0f}% ± {b_pr.get('stddev', 0)*100:.0f}% | {delta.get('pass_rate', '—')} |") + lines.append(f"| Time | {a_time_s} | {b_time.get('mean', 0):.1f}s ± {b_time.get('stddev', 0):.1f}s | {delta.get('time_seconds', '—')}s |") + lines.append(f"| Tokens | {a_tokens_s} | {b_tokens.get('mean', 0):.0f} ± {b_tokens.get('stddev', 0):.0f} | {delta.get('tokens', '—')} |") + else: + lines.append(f"| Metric | {label_a} |") + lines.append("|--------|------------|") + lines.append(f"| Pass Rate | {a_pass_rate} |") + lines.append(f"| Time | {a_time_s} |") + lines.append(f"| Tokens | {a_tokens_s} |") # Notes section if benchmark.get("notes"): diff --git a/skills/skill-creator/tests/test_aggregate_benchmark.py b/skills/skill-creator/tests/test_aggregate_benchmark.py new file mode 100644 index 00000000..a7b30322 --- /dev/null +++ b/skills/skill-creator/tests/test_aggregate_benchmark.py @@ -0,0 +1,81 @@ +"""Regression tests for aggregate_benchmark.py. + +Covers the fabricated-delta bug: with only a single configuration of runs +(e.g. only with_skill, which the loader supports), aggregate_results() +previously emitted a "delta" against an invented zero baseline, so +benchmark reports claimed a pass-rate/time/token improvement that no +comparison supported. +""" + +import sys +from pathlib import Path + +import pytest + +SKILL_CREATOR = Path(__file__).resolve().parents[1] +sys.path.insert(0, str(SKILL_CREATOR)) + +from scripts.aggregate_benchmark import aggregate_results, generate_markdown # noqa: E402 + + +def _run(pass_rate: float, time_seconds: float = 45.0, tokens: int = 3800) -> dict: + return { + "eval_id": 1, + "run_number": 1, + "pass_rate": pass_rate, + "passed": 8, + "failed": 2, + "total": 10, + "time_seconds": time_seconds, + "tokens": tokens, + "tool_calls": 18, + "errors": 0, + "expectations": [], + "notes": [], + } + + +def _benchmark(run_summary: dict) -> dict: + return { + "metadata": { + "skill_name": "docx", + "executor_model": "m", + "timestamp": "t", + "evals_run": [1], + "runs_per_configuration": 1, + }, + "run_summary": run_summary, + "notes": [], + } + + +def test_single_config_has_no_delta() -> None: + summary = aggregate_results({"with_skill": [_run(0.80)]}) + assert "delta" not in summary + assert summary["with_skill"]["pass_rate"]["mean"] == pytest.approx(0.80) + + +def test_single_config_markdown_omits_delta_column() -> None: + summary = aggregate_results({"with_skill": [_run(0.80)]}) + markdown = generate_markdown(_benchmark(summary)) + assert "Delta" not in markdown + assert "Config B" not in markdown + assert "+0.80" not in markdown + assert "80% ± 0%" in markdown + + +def test_two_configs_still_report_delta() -> None: + summary = aggregate_results( + { + "with_skill": [_run(0.85), _run(0.71)], + "without_skill": [_run(0.35, time_seconds=32.0, tokens=2100)], + } + ) + assert summary["delta"] == { + "pass_rate": "+0.43", + "time_seconds": "+13.0", + "tokens": "+1700", + } + markdown = generate_markdown(_benchmark(summary)) + assert "| Metric | With Skill | Without Skill | Delta |" in markdown + assert "+0.43" in markdown