Skip to content

Commit 92c9dcf

Browse files
feat(xtest): compare benchmark candidates with each other
Stage 4 of the DSPX-4372-t1 plan. With more than one candidate, every pair of candidates is compared directly. These comparisons never gate. - perf/stats.py: FASTER/SLOWER/TIED verdicts in their own BH family. Each tail is tested at alpha/2, since neither candidate is privileged; TIED requires the interval inside the +/-threshold band. The rule is antisymmetric, so listing order does not change a conclusion. - perf/runner.py: candidate pairs follow the reference pairs; the control yields every copy-to-copy pair so its floor covers the widest spacing. - perf/conclusions.py (new): pairwise conclusions per cell and metric, and winner eligibility, decided once. A winner must be FASTER than every other candidate, and a tripped A/A control suppresses it however narrow the interval. Ties and unresolved competitors say so. - report JSON 'conclusions', a candidate-comparison section in the markdown, and the same conclusion lines on the terminal. Fixes the three defects review found in #621: the control-registration order (kept from stage 3), a winner named from the top two point estimates, and a tripped control that did not suppress the winner. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
1 parent 402772a commit 92c9dcf

10 files changed

Lines changed: 771 additions & 34 deletions

File tree

‎.github/workflows/check.yml‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,8 @@ jobs:
5252
uv run --frozen --no-build pytest --no-header -q
5353
test_bench_stats.py test_bench_measure.py test_bench_runner.py
5454
test_bench_arms.py test_bench_config.py test_bench_report.py
55-
test_bench_installation.py test_sdk_commands.py test_tdfs_units.py test_encryption_units.py
55+
test_bench_installation.py test_bench_conclusions.py
56+
test_sdk_commands.py test_tdfs_units.py test_encryption_units.py
5657
test_sizes_units.py test_zip64_units.py
5758
working-directory: xtest
5859
- name: Lint and test otdf-local

‎xtest/conftest.py‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -630,6 +630,8 @@ def pytest_sessionfinish(session: pytest.Session, exitstatus: int):
630630
if reporter is not None:
631631
reporter.write_sep("=", "benchmark results")
632632
reporter.write_line(report.headline(recorder, gate))
633+
for conclusion in recorder.conclusions(bench_config, gate):
634+
reporter.write_line(report.conclusion_line(conclusion))
633635
reporter.write_line(f"raw samples and statistics: {json_path}")
634636

635637
if config.getoption("--bench-no-gate", default=False):

‎xtest/perf/README.md‎

Lines changed: 37 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -140,14 +140,46 @@ lists the reason for each cell.
140140
> Check with `otdf-sdk-mgr versions resolve <sdk> main latest` — one entry back
141141
> instead of two means there is nothing to measure until `main` moves.
142142
143+
### Comparing candidates with each other
144+
145+
With more than one candidate, each pair of candidates is also compared
146+
directly — the same within-round ratio, so the comparison is as fair as the
147+
one against the reference. These comparisons **never fail the build**: there
148+
is no incumbent, so "regression" means nothing between them. They are listed
149+
under **Candidate comparisons** in the summary and in the JSON `conclusions`.
150+
151+
**FASTER** / **SLOWER** — the interval lies entirely outside the
152+
`[1/threshold, threshold]` band and the matching one-sided adjusted p-value is
153+
below `alpha / 2`. Half alpha because the question is two-sided: neither
154+
candidate is privileged, and testing both tails at alpha would double the
155+
chance of ranking two equal builds.
156+
157+
**TIED** — the interval lies entirely *inside* the band: any real difference
158+
is smaller than the threshold. A positive finding, and deliberately not PASS,
159+
which only claims "not slower".
160+
161+
**inconclusive** — anything else, including a missing or underpowered A/A
162+
control.
163+
164+
Reversing a pair turns FASTER into SLOWER and leaves TIED alone, so the order
165+
the candidates were listed in does not change any conclusion. They form their
166+
own correction family: adding candidates costs the regression gate no power.
167+
168+
A **winner** is named for a cell and metric only when one candidate is FASTER
169+
than *every* other candidate in their own contrasts, and the A/A control did
170+
not trip. A tie, an inconclusive pair involving a contender, or a tripped
171+
control — however narrow the interval — means no winner, and the summary says
172+
which of those it was. Ranking by point estimates is not evidence that two
173+
candidates differ.
174+
143175
### The A/A control
144176

145177
Each SDK gets a control cell that compares the reference build **against itself**
146178
through the identical pipeline. Its true ratio is exactly 1.0 by construction, so
147179
whatever it reports is the harness's own error on this runner. With K arms the
148180
control runs K copies of the reference, since the last arm in a round drifts
149-
further from the first than an adjacent pair does; of its K-1 A/A contrasts, the
150-
worst one stands as the floor. It does two jobs:
181+
further from the first than an adjacent pair does; of its C(K,2) copy-to-copy
182+
contrasts, the worst one stands as the floor. It does two jobs:
151183

152184
- If the control *trips* — its own A/A comparison looks like a real effect — then
153185
something is systematically biased and **the whole run stops being able to fail
@@ -245,6 +277,7 @@ things; the noise floor will tell you whether you succeeded.
245277
| `runner.py` | The paired round loop, the stopping rule, the budget, `analyze()` |
246278
| `stats.py` | Pure functions: log-ratios, bootstrap CI, Wilcoxon, BH, the decision rule |
247279
| `report.py` | Session recorder, JSON artifact (`SCHEMA_VERSION`), step-summary markdown |
280+
| `conclusions.py` | Candidate-to-candidate conclusions and winner eligibility, read by every renderer |
248281
| `config.py` | Option parsing and validation shared by pytest and CI: `BenchConfig`, `--bench-refs`, the arm ceiling, the budget default. No pytest |
249282
| `installation.py` | Reads the installation manifest `otdf-sdk-mgr install benchmark` writes. No pytest |
250283
| `docs/` | The figures in this README, and the Monte Carlo behind them. Documentation only; imported by nothing in the harness |
@@ -569,6 +602,8 @@ two builds doing different amounts of work) is invisible in the output.
569602
10. Never hand-edit the figures. Change `docs/make_figures.py` and regenerate;
570603
if a number moves, `--verify` is what tells you whether the figure or the
571604
world changed.
605+
11. Never let a comparison between two candidates fail the build, share the
606+
gate's correction family, or name a winner the conclusions model did not.
572607

573608
Every one of these fails *silently* and *plausibly* when broken: the numbers
574609
still look like numbers. That is why they are written down.

‎xtest/perf/conclusions.py‎

Lines changed: 167 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,167 @@
1+
"""What a multi-candidate run concluded about its candidates.
2+
3+
The gate answers "did any candidate get slower than the reference". This
4+
module answers the other question a run with several candidates is asked:
5+
"how do the candidates compare with each other, and is one of them the best".
6+
It never changes the gate. A build must not go red because one candidate lost
7+
to another.
8+
9+
Pairwise conclusions come first. Point-estimate order is not evidence: two
10+
candidates whose ratios against the reference are 1.02 and 1.05 have not been
11+
shown to differ unless their own contrast says so. A unique winner is named
12+
only when it is supported against *every* other candidate, and never when the
13+
run's control tripped.
14+
15+
Winner eligibility is decided here, once, from the analysed result. The JSON
16+
artifact, the terminal line, and the markdown summary all read these objects,
17+
so they cannot disagree about the conclusion.
18+
"""
19+
20+
from __future__ import annotations
21+
22+
from collections.abc import Sequence
23+
from dataclasses import dataclass
24+
25+
from perf import stats
26+
from perf.runner import BenchConfig, CellResult, contrast_key
27+
28+
29+
@dataclass(frozen=True, slots=True)
30+
class Pairwise:
31+
"""One candidate-to-candidate contrast, read as "``b`` vs ``a``"."""
32+
33+
a: str
34+
b: str
35+
verdict: stats.Verdict
36+
ratio: float
37+
ci_low: float
38+
ci_high: float
39+
note: str
40+
41+
def faster(self) -> str | None:
42+
"""The arm shown to be faster, or None if neither was."""
43+
if self.verdict is stats.Verdict.FASTER:
44+
return self.b
45+
if self.verdict is stats.Verdict.SLOWER:
46+
return self.a
47+
return None
48+
49+
50+
@dataclass(frozen=True, slots=True)
51+
class Conclusion:
52+
"""The candidates of one cell compared on one metric."""
53+
54+
cell_id: str
55+
metric: str
56+
#: Arm id -> build label, for rendering.
57+
labels: dict[str, str]
58+
pairwise: tuple[Pairwise, ...]
59+
#: The candidate supported against every other, or None.
60+
winner: str | None
61+
#: Why there is or is not a winner, in one sentence.
62+
reason: str
63+
64+
def to_dict(self) -> dict[str, object]:
65+
return {
66+
"cell": self.cell_id,
67+
"metric": self.metric,
68+
"pairwise": [
69+
{
70+
"a": p.a,
71+
"b": p.b,
72+
"verdict": str(p.verdict),
73+
"note": p.note,
74+
}
75+
for p in self.pairwise
76+
],
77+
"winner": self.winner,
78+
"reason": self.reason,
79+
}
80+
81+
82+
def conclude(
83+
results: Sequence[CellResult], config: BenchConfig, gate: stats.GateResult
84+
) -> list[Conclusion]:
85+
"""Compare the candidates of every cell that measured more than one.
86+
87+
Empty for a two-arm run: one candidate has nothing to be compared with,
88+
and the gate has already said everything there is to say about it.
89+
"""
90+
out: list[Conclusion] = []
91+
for r in results:
92+
if r.control or len(r.candidates) < 2:
93+
continue
94+
for metric in config.gated_metrics:
95+
pairwise = tuple(
96+
_pairwise(a, b, gate.comparisons[key])
97+
for a, b in r.contrast_pairs()
98+
if not r.is_reference_pair(a, b)
99+
and (key := contrast_key(r.cell_id, a, b, metric)) in gate.comparisons
100+
)
101+
winner, reason = _winner(r.candidates, pairwise, gate)
102+
out.append(
103+
Conclusion(
104+
cell_id=r.cell_id,
105+
metric=metric,
106+
labels=dict(r.arm_labels),
107+
pairwise=pairwise,
108+
winner=winner,
109+
reason=reason,
110+
)
111+
)
112+
return out
113+
114+
115+
def _pairwise(a: str, b: str, c: stats.PairedComparison) -> Pairwise:
116+
return Pairwise(
117+
a=a,
118+
b=b,
119+
verdict=c.verdict,
120+
ratio=c.ratio,
121+
ci_low=c.ci_low,
122+
ci_high=c.ci_high,
123+
note=c.note,
124+
)
125+
126+
127+
def _winner(
128+
candidates: Sequence[str], pairwise: Sequence[Pairwise], gate: stats.GateResult
129+
) -> tuple[str | None, str]:
130+
"""Name the candidate faster than every other, or say why there is none.
131+
132+
Every requirement is checked, in an order that gives the most useful
133+
reason first:
134+
135+
1. The control must not have tripped. A biased harness can produce a
136+
narrow, decisive-looking interval, so a tripped control suppresses the
137+
claim however clean the contrast looks.
138+
2. Some candidate must be shown FASTER than each of the others in its own
139+
contrast. Beating the runner-up is not enough when a third candidate's
140+
contrast is unresolved: that competitor may be the faster one.
141+
"""
142+
if not gate.trustworthy:
143+
return None, (
144+
"no winner: the A/A control failed its own comparison, so no "
145+
"ranking from this run can be trusted"
146+
)
147+
beats: dict[str, set[str]] = {c: set() for c in candidates}
148+
for p in pairwise:
149+
faster = p.faster()
150+
if faster is not None:
151+
beats[faster].add(p.b if faster == p.a else p.a)
152+
for c in candidates:
153+
if beats[c] == set(candidates) - {c}:
154+
return c, f"faster than every other candidate ({len(candidates) - 1})"
155+
156+
ties = [p for p in pairwise if p.verdict is stats.Verdict.TIED]
157+
unresolved = [p for p in pairwise if p.verdict is stats.Verdict.INCONCLUSIVE]
158+
if unresolved:
159+
p = unresolved[0]
160+
return None, (
161+
f"no winner: `{p.b}` vs `{p.a}` is unresolved"
162+
+ (f" ({p.note})" if p.note else "")
163+
)
164+
if ties:
165+
p = ties[0]
166+
return None, f"no winner: `{p.a}` and `{p.b}` are tied"
167+
return None, "no winner: no candidate is faster than every other"

‎xtest/perf/report.py‎

Lines changed: 57 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,13 +29,15 @@
2929

3030
from perf import stats
3131
from perf.cells import BenchCell
32+
from perf.conclusions import Conclusion, conclude
3233
from perf.measure import METRIC_LABELS, METRICS, format_metric
3334
from perf.runner import BenchConfig, CellResult, analyze, contrast_key
3435

3536
#: Version of the JSON artifact's shape. 2 added multi-arm cells: samples are
3637
#: keyed by arm id, each cell names its ``arms`` and ``reference``, and the
37-
#: statistics live under ``contrasts`` -- one per candidate -- instead of a
38-
#: single ``metrics`` block. Consumers that read version 1 must check this.
38+
#: statistics live under ``contrasts`` -- one per pair of arms -- instead of a
39+
#: single ``metrics`` block, with candidate comparisons summarized under the
40+
#: top-level ``conclusions``. Consumers that read version 1 must check this.
3941
SCHEMA_VERSION = 2
4042

4143
#: Cells the session intends to run. Set by the conftest parametrizer, read by
@@ -75,6 +77,11 @@ def skip(self, cell_id: str, reason: str) -> None:
7577
def gate(self, config: BenchConfig) -> stats.GateResult:
7678
return analyze(self.results, config)
7779

80+
def conclusions(
81+
self, config: BenchConfig, gate: stats.GateResult
82+
) -> list[Conclusion]:
83+
return conclude(self.results, config, gate)
84+
7885

7986
def to_dict(
8087
recorder: BenchmarkRecorder, config: BenchConfig, gate: stats.GateResult
@@ -106,6 +113,7 @@ def to_dict(
106113
"regressions": gate.regressions,
107114
"improvements": gate.improvements,
108115
"summary": gate.summary,
116+
"conclusions": [c.to_dict() for c in recorder.conclusions(config, gate)],
109117
"skipped": recorder.skipped,
110118
"cells": [_cell_dict(r, gate) for r in recorder.results],
111119
}
@@ -259,6 +267,8 @@ def markdown(
259267
]
260268
for result in recorder.results:
261269
for a, b in result.contrast_pairs():
270+
if not result.is_reference_pair(a, b):
271+
continue
262272
build = f"A/A {b}" if result.control else f"`{result.arm_labels[b]}`"
263273
for metric in METRICS:
264274
c = gate.comparisons.get(contrast_key(result.cell_id, a, b, metric))
@@ -274,6 +284,8 @@ def markdown(
274284
f"| {_verdict_cell(c)} |"
275285
)
276286

287+
lines += _candidate_comparisons(recorder.conclusions(config, gate))
288+
277289
references = sorted(
278290
{r.arm_labels[r.reference] for r in recorder.results if not r.control}
279291
)
@@ -294,6 +306,46 @@ def markdown(
294306
return "\n".join(lines) + "\n"
295307

296308

309+
def _candidate_comparisons(conclusions: list[Conclusion]) -> list[str]:
310+
"""The candidate-to-candidate section: pairwise verdicts, then any winner."""
311+
if not conclusions:
312+
return []
313+
lines = [
314+
"",
315+
"### Candidate comparisons",
316+
"",
317+
"Each pair of candidates, compared directly. These never fail the build. "
318+
"A winner is named only when it is faster than every other candidate "
319+
"and the A/A control held.",
320+
"",
321+
"| cell | metric | comparison | ratio (95% CI) | verdict |",
322+
"| --- | --- | --- | --- | --- |",
323+
]
324+
for c in conclusions:
325+
for p in c.pairwise:
326+
interval = (
327+
f"{p.ratio:.3f}x [{p.ci_low:.3f}, {p.ci_high:.3f}]"
328+
if math.isfinite(p.ci_low) and math.isfinite(p.ci_high)
329+
else "-"
330+
)
331+
verdict = f"{p.verdict} ({p.note})" if p.note else str(p.verdict)
332+
lines.append(
333+
f"| {c.cell_id} | {METRIC_LABELS[c.metric][0]} "
334+
f"| `{c.labels[p.b]}` vs `{c.labels[p.a]}` | {interval} | {verdict} |"
335+
)
336+
lines.append("")
337+
lines += [conclusion_line(c) for c in conclusions]
338+
return lines
339+
340+
341+
def conclusion_line(c: Conclusion) -> str:
342+
"""One line per cell and metric: the winner, or why there is none."""
343+
head = f"- **{c.cell_id}** ({METRIC_LABELS[c.metric][0]}): "
344+
if c.winner is not None:
345+
return head + f"`{c.labels[c.winner]}` wins, {c.reason}."
346+
return head + f"{c.reason}."
347+
348+
297349
def _ratio_cell(c: stats.PairedComparison) -> str:
298350
if not math.isfinite(c.ratio):
299351
return "-"
@@ -321,6 +373,9 @@ def _p_cell(c: stats.PairedComparison) -> str:
321373
stats.Verdict.REGRESSION: "**REGRESSION**",
322374
stats.Verdict.IMPROVED: "IMPROVED",
323375
stats.Verdict.INCONCLUSIVE: "inconclusive",
376+
stats.Verdict.FASTER: "FASTER",
377+
stats.Verdict.SLOWER: "SLOWER",
378+
stats.Verdict.TIED: "TIED",
324379
}
325380

326381

0 commit comments

Comments
 (0)