From 0f5e7db11df61cb45bed88f9d4322f838602c2b0 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Sun, 20 Sep 2026 04:40:56 +0000 Subject: [PATCH] Put the measurement checks in the code rather than in my memory Three mistakes have recurred in this repository's measurements, each more than once, and knowing the lesson has not prevented repeating it within hours -- the cross-window comparison was corrected in one PR and repeated in the next. `src/evaluation/ab.py` makes each one structurally hard. Arms alternate. There is no mode that runs them in sequence, because the second arm otherwise inherits a warm process and whatever the API is doing that minute. That has twice produced a result in the direction the author was hoping for, and once showed a real improvement as a regression. Every arm declares a `precondition`, checked after each sample, and the summary refuses to draw a conclusion when one fails. A prompt that asked for "exactly 1" alternate returned four; a ContextVar set around a call was overwritten by the node inside it. Both produced ordinary-looking numbers that were measuring the baseline twice. And the summary reports p50 with min and max, never a percentile the sample cannot support. A p90 was quoted from fifteen samples, where it is about the second-highest value. The configuration is applied before every sample rather than once per arm, because a setting applied once and mutated in between fails the same way as never applying it. Each test recreates a mistake actually made here, and both guards were verified by sabotage: removing the alternation fails the alternation test, ignoring preconditions fails the refusal test. Exercised on the real graph too, not only on stubs -- expansion on 6.22s against off 2.86s, precondition held. Co-Authored-By: Claude Opus 5 --- src/evaluation/ab.py | 121 ++++++++++++++++++++++++++++++++++++ tests/evaluation/test_ab.py | 92 +++++++++++++++++++++++++++ 2 files changed, 213 insertions(+) create mode 100644 src/evaluation/ab.py create mode 100644 tests/evaluation/test_ab.py diff --git a/src/evaluation/ab.py b/src/evaluation/ab.py new file mode 100644 index 0000000..663c0f9 --- /dev/null +++ b/src/evaluation/ab.py @@ -0,0 +1,121 @@ +"""Compare two configurations without the three mistakes that keep recurring. + +Every latency and retrieval comparison in this repository has gone wrong in +one of three ways, more than once each, and knowing the lesson has not +prevented repeating it within hours. So the checks live here rather than in +whoever is writing the script. + +**1. Arms measured in sequence.** The second arm benefits from a warm process, +a warm connection and whatever the API is doing that minute. Twice this has +produced a result in the direction the author was hoping for -- and once, in +an earlier measurement, showed a real improvement as a 2.5s regression. `run` +alternates the arms and never offers a mode that does not. + +**2. The manipulation never verified.** A prompt asking for "exactly 1" +alternate produced four; a ContextVar set around a call was overwritten by the +node inside it. Both looked like ordinary results. Each arm must declare a +`precondition`, and `run` refuses to report a comparison whose precondition +did not hold rather than printing numbers that mean nothing. + +**3. A percentile the sample cannot support.** p90 from fifteen samples is +about the second-highest value. `Summary` reports p50 with min and max, and +says so, instead of implying a tail estimate that is not there. +""" + +import statistics +from collections.abc import Awaitable, Callable, Iterable +from dataclasses import dataclass, field +from typing import Generic, TypeVar + +T = TypeVar("T") + +#: Below this, a median is a gesture. Not enforced -- a small run is often the +#: right thing -- but reported, so nobody quotes six samples as a p50. +ADVISORY_MIN_SAMPLES = 10 + + +@dataclass +class Arm(Generic[T]): + """One configuration under test. + + `apply` makes the configuration active and is called immediately before + each sample, never once per arm -- a setting applied once and mutated by + something else in between is exactly the failure this guards. + + `precondition` is checked after each sample and must return True for the + manipulation to be believed. Returning True unconditionally defeats the + purpose, so write it against something the configuration actually changes. + """ + + name: str + apply: Callable[[], None] + precondition: Callable[[], bool] + samples: list[float] = field(default_factory=list) + precondition_failures: int = 0 + + @property + def held(self) -> bool: + return self.precondition_failures == 0 and bool(self.samples) + + +@dataclass +class Summary: + arms: list[Arm[float]] + + @property + def trustworthy(self) -> bool: + return all(arm.held for arm in self.arms) + + def report(self) -> str: + lines = [] + for arm in self.arms: + if not arm.samples: + lines.append(f" {arm.name:24s} no samples") + continue + values = sorted(arm.samples) + note = "" + if arm.precondition_failures: + note = ( + f" PRECONDITION FAILED on {arm.precondition_failures} " + f"of {len(values)} samples -- this comparison means nothing" + ) + elif len(values) < ADVISORY_MIN_SAMPLES: + note = f" (n={len(values)}, small; treat the median loosely)" + lines.append( + f" {arm.name:24s} p50 {statistics.median(values):6.2f} " + f"min {values[0]:6.2f} max {values[-1]:6.2f} n={len(values)}{note}" + ) + if not self.trustworthy: + lines.append( + " Refusing to draw a conclusion: an arm's precondition did not hold." + ) + return "\n".join(lines) + + +async def run( + arms: list[Arm[float]], + cases: Iterable[object], + measure: Callable[[object, str], Awaitable[float]], + *, + repeats: int = 5, +) -> Summary: + """Measure every case under every arm, alternating which arm goes first. + + The alternation is not optional and the order is derived from the case and + repeat indices, so a run is deterministic in structure while still + splitting any warming effect evenly between the arms. + """ + if len(arms) < 2: + raise ValueError("an A/B comparison needs at least two arms") + cases = list(cases) + for repeat in range(repeats): + for index, case in enumerate(cases): + ordered = list(arms) + if (repeat + index) % 2: + ordered.reverse() + for arm in ordered: + arm.apply() + arm.samples.append(await measure(case, arm.name)) + if not arm.precondition(): + arm.precondition_failures += 1 + return Summary(arms) diff --git a/tests/evaluation/test_ab.py b/tests/evaluation/test_ab.py new file mode 100644 index 0000000..85073cf --- /dev/null +++ b/tests/evaluation/test_ab.py @@ -0,0 +1,92 @@ +"""The harness exists to catch three mistakes, so the tests are that it does. + +Each test recreates a mistake actually made in this repository rather than a +hypothetical one. +""" + +import asyncio + +import pytest + +from evaluation.ab import ADVISORY_MIN_SAMPLES, Arm, run + + +def _arm(name: str, holds: bool = True) -> tuple[Arm[float], list[str]]: + applied: list[str] = [] + return ( + Arm( + name=name, + apply=lambda: applied.append(name), + precondition=lambda: holds, + ), + applied, + ) + + +def _measure_order(order: list[str]): # type: ignore[no-untyped-def] + async def measure(_case: object, arm: str) -> float: + order.append(arm) + return 1.0 + + return measure + + +def test_the_arms_alternate_rather_than_running_in_sequence() -> None: + # The mistake: measuring arm A fully, then arm B, so B inherits a warm + # process and whatever the API is doing. It has twice produced a result + # in the direction the author wanted. + a, _ = _arm("a") + b, _ = _arm("b") + order: list[str] = [] + asyncio.run(run([a, b], ["q1", "q2"], _measure_order(order), repeats=2)) + first_of_each_pair = order[::2] + assert set(first_of_each_pair) == {"a", "b"}, "one arm always went first" + + +def test_a_failed_precondition_refuses_the_comparison() -> None: + # The mistake: a prompt asked for "exactly 1" alternate and got four; a + # ContextVar was overwritten by the node it was set around. Both produced + # ordinary-looking numbers that measured the baseline twice. + good, _ = _arm("good") + broken, _ = _arm("broken", holds=False) + summary = asyncio.run(run([good, broken], ["q"], _measure_order([]), repeats=3)) + assert not summary.trustworthy + assert "PRECONDITION FAILED" in summary.report() + assert "means nothing" in summary.report() + assert "Refusing to draw a conclusion" in summary.report() + + +def test_a_held_precondition_reports_normally() -> None: + a, _ = _arm("a") + b, _ = _arm("b") + summary = asyncio.run(run([a, b], ["q"], _measure_order([]), repeats=6)) + assert summary.trustworthy + assert "PRECONDITION FAILED" not in summary.report() + assert "Refusing" not in summary.report() + + +def test_the_configuration_is_applied_per_sample_not_once_per_arm() -> None: + # A setting applied once and mutated in between is the same class of + # failure as never applying it: the numbers look fine either way. + a, applied_a = _arm("a") + b, _ = _arm("b") + asyncio.run(run([a, b], ["q1", "q2"], _measure_order([]), repeats=3)) + assert len(applied_a) == len(a.samples) == 6 + + +def test_a_small_sample_is_flagged_rather_than_quoted_confidently() -> None: + # p90 from fifteen samples is about the second-highest value, and one was + # quoted from fifteen. The harness reports min and max instead, and says + # when the median is thin. + a, _ = _arm("a") + b, _ = _arm("b") + summary = asyncio.run(run([a, b], ["q"], _measure_order([]), repeats=2)) + assert all(len(arm.samples) < ADVISORY_MIN_SAMPLES for arm in summary.arms) + assert "small; treat the median loosely" in summary.report() + assert "p90" not in summary.report() + + +def test_one_arm_is_not_a_comparison() -> None: + a, _ = _arm("a") + with pytest.raises(ValueError, match="at least two arms"): + asyncio.run(run([a], ["q"], _measure_order([]), repeats=1))