From 0f5e7db11df61cb45bed88f9d4322f838602c2b0 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Sun, 20 Sep 2026 04:40:56 +0000 Subject: [PATCH 1/2] 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)) From c4f6a96ed992455859b8984e442182e7a06d5265 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Sun, 20 Sep 2026 04:45:39 +0000 Subject: [PATCH 2/2] Adversarial review: the harness had the hole it was written to close `precondition` was only worth the discrimination in it, and nothing checked for any. `lambda: True` reads like a guard, passes every sample and proves nothing -- the same vacuous shape this repository has found three times elsewhere today, sitting in the file written to stop it. After each sample, with that arm's configuration still active, the other arms' preconditions are evaluated. If one of them also holds, the two cannot tell the arms apart and the comparison is refused with a distinct message. That is the rule the rest of the work arrived at -- an assertion that something is absent proves nothing until the same check has shown it can be present -- applied to the guard rather than only to the thing guarded. The new check immediately caught the test helper in this file, whose preconditions returned a constant. The helper now reads a shared marker the arms actually set, and a separate always-true arm exists solely to be caught. Two smaller ones. Re-using arms across runs silently merged two distributions into what looks like one noisy measurement, and is refused. And ordering reversed rather than rotated, so with three or more arms the middle one stayed in the middle and carried a systematic warming bias -- exactly the effect the alternation exists to remove, left in place for every case except two arms. All three verified by sabotage, and the first attempt at that is worth recording: removing the discrimination check appeared to fail nothing, because the edit had silently not applied. Re-done with the sabotage itself asserted, the test fails as it should. The precondition needed a precondition. Co-Authored-By: Claude Opus 5 --- src/evaluation/ab.py | 42 ++++++++++++++++--- tests/evaluation/test_ab.py | 80 ++++++++++++++++++++++++++++++++++++- 2 files changed, 115 insertions(+), 7 deletions(-) diff --git a/src/evaluation/ab.py b/src/evaluation/ab.py index 663c0f9..4836384 100644 --- a/src/evaluation/ab.py +++ b/src/evaluation/ab.py @@ -17,7 +17,16 @@ `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 +**3. A precondition that cannot fail.** The check above is only worth the +discrimination in it: `lambda: True` passes every time and reads like a guard. +So after each sample the *other* arms' preconditions are evaluated against the +configuration that is actually active, and if one of them also holds, the two +preconditions do not tell the arms apart and the comparison is refused. This +is the same rule the rest of the repository learned the hard way -- an +assertion that something is absent proves nothing until the same check has +shown it can be present. + +**4. 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. """ @@ -52,10 +61,17 @@ class Arm(Generic[T]): precondition: Callable[[], bool] samples: list[float] = field(default_factory=list) precondition_failures: int = 0 + #: Times another arm's precondition also held under this arm's + #: configuration, meaning the two do not discriminate. + indiscriminate: int = 0 @property def held(self) -> bool: - return self.precondition_failures == 0 and bool(self.samples) + return ( + self.precondition_failures == 0 + and self.indiscriminate == 0 + and bool(self.samples) + ) @dataclass @@ -79,6 +95,12 @@ def report(self) -> str: f" PRECONDITION FAILED on {arm.precondition_failures} " f"of {len(values)} samples -- this comparison means nothing" ) + elif arm.indiscriminate: + note = ( + f" PRECONDITION DOES NOT DISCRIMINATE: another arm's held " + f"under this one's configuration on {arm.indiscriminate} " + f"samples, so it cannot tell the arms apart" + ) elif len(values) < ADVISORY_MIN_SAMPLES: note = f" (n={len(values)}, small; treat the median loosely)" lines.append( @@ -107,15 +129,25 @@ async def run( """ if len(arms) < 2: raise ValueError("an A/B comparison needs at least two arms") + if any(arm.samples for arm in arms): + # Re-using arms silently mixes two runs into one distribution, and the + # result looks like an ordinary noisy measurement. + raise ValueError("these arms already hold samples; build fresh ones") cases = list(cases) for repeat in range(repeats): for index, case in enumerate(cases): - ordered = list(arms) - if (repeat + index) % 2: - ordered.reverse() + # Rotate rather than reverse: with more than two arms, reversing + # leaves the middle one always in the middle. + offset = (repeat + index) % len(arms) + ordered = arms[offset:] + arms[:offset] for arm in ordered: arm.apply() arm.samples.append(await measure(case, arm.name)) if not arm.precondition(): arm.precondition_failures += 1 + continue + # The configuration for `arm` is still active, so any other + # arm whose precondition also holds is not distinguishing. + if any(other is not arm and other.precondition() for other in arms): + arm.indiscriminate += 1 return Summary(arms) diff --git a/tests/evaluation/test_ab.py b/tests/evaluation/test_ab.py index 85073cf..9f304b2 100644 --- a/tests/evaluation/test_ab.py +++ b/tests/evaluation/test_ab.py @@ -10,19 +10,36 @@ from evaluation.ab import ADVISORY_MIN_SAMPLES, Arm, run +#: Which arm's configuration is currently applied. Shared, because a +#: precondition that does not read something the arms actually change cannot +#: tell them apart -- which the harness now refuses, and which this helper +#: used to do by returning a constant. +ACTIVE: list[str] = [] + def _arm(name: str, holds: bool = True) -> tuple[Arm[float], list[str]]: applied: list[str] = [] + + def apply() -> None: + applied.append(name) + ACTIVE.clear() + ACTIVE.append(name) + return ( Arm( name=name, - apply=lambda: applied.append(name), - precondition=lambda: holds, + apply=apply, + precondition=(lambda: [name] == ACTIVE) if holds else (lambda: False), ), applied, ) +def _always_true_arm(name: str) -> Arm[float]: + """A guard that cannot fail, which is the thing the harness must catch.""" + return Arm(name=name, apply=lambda: None, precondition=lambda: True) + + def _measure_order(order: list[str]): # type: ignore[no-untyped-def] async def measure(_case: object, arm: str) -> float: order.append(arm) @@ -90,3 +107,62 @@ 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)) + + +def test_a_precondition_that_cannot_fail_is_caught() -> None: + # The hole this harness had: `lambda: True` reads like a guard, passes + # every sample, and proves nothing. Same shape as an absence test that + # never showed the thing can be present -- which is the mistake this file + # was written to stop, present in the file itself. + summary = asyncio.run( + run( + [_always_true_arm("always-a"), _always_true_arm("always-b")], + ["q"], + _measure_order([]), + repeats=3, + ) + ) + assert not summary.trustworthy + assert "DOES NOT DISCRIMINATE" in summary.report() + assert "cannot tell the arms apart" in summary.report() + + +def test_discriminating_preconditions_are_accepted() -> None: + # The honest case: a shared flag the arms actually set, so each arm's + # precondition is false under the other's configuration. + active: list[str] = [] + + def arm(name: str) -> Arm[float]: + return Arm( + name=name, + apply=lambda: active.clear() or active.append(name), # type: ignore[func-returns-value] + precondition=lambda: active == [name], + ) + + summary = asyncio.run( + run([arm("a"), arm("b")], ["q"], _measure_order([]), repeats=6) + ) + assert summary.trustworthy + assert "DISCRIMINATE" not in summary.report() + + +def test_reusing_arms_across_runs_is_refused() -> None: + # Two runs into one distribution looks exactly like one noisy run. + a, _ = _arm("a") + b, _ = _arm("b") + asyncio.run(run([a, b], ["q"], _measure_order([]), repeats=2)) + with pytest.raises(ValueError, match="already hold samples"): + asyncio.run(run([a, b], ["q"], _measure_order([]), repeats=2)) + + +def test_three_arms_each_take_every_position() -> None: + # Reversing gives two orderings, so with three arms the middle one is + # always in the middle and carries a systematic warming bias. + order: list[str] = [] + arms = [_arm(name)[0] for name in ("a", "b", "c")] + asyncio.run(run(arms, ["q"], _measure_order(order), repeats=3)) + positions: dict[str, set[int]] = {name: set() for name in ("a", "b", "c")} + for start in range(0, len(order), 3): + for position, name in enumerate(order[start : start + 3]): + positions[name].add(position) + assert all(len(seen) == 3 for seen in positions.values()), positions