diff --git a/xtest/conftest.py b/xtest/conftest.py index 62c62d41..34ece718 100644 --- a/xtest/conftest.py +++ b/xtest/conftest.py @@ -253,15 +253,25 @@ def _add_benchmark_options(parser: pytest.Parser): help="run the performance regression benchmarks (they are long, so they " "are opt-in and collect nothing otherwise)", ) + group.addoption( + "--bench-refs", + help="2 to 4 builds of one sdk to compare, comma- or space-separated, " + "e.g. 'go@v0.29.0,go@main,go@my-branch'. The first is the reference " + "every other build is compared against. Defaults to the newest " + "installed release against main. Supersedes " + "--bench-baseline/--bench-candidate", + ) group.addoption( "--bench-baseline", help="build to compare against, e.g. go@v0.29.0; defaults to the newest " - "installed release of each sdk", + "installed release of each sdk. Two-arm shorthand for the first " + "--bench-refs entry", ) group.addoption( "--bench-candidate", help="build under test, e.g. go@main; defaults to the installed " - "unreleased build of each sdk", + "unreleased build of each sdk. Two-arm shorthand for the second " + "--bench-refs entry", ) group.addoption( "--bench-payloads", @@ -311,8 +321,10 @@ def _add_benchmark_options(parser: pytest.Parser): group.addoption( "--bench-budget-seconds", type=float, - default=1500.0, - help="wall-clock allowance shared by every cell (default: %(default)s)", + default=None, + help="wall-clock allowance shared by every cell (default: 1500 for two " + "arms, scaled by K/2 for K arms, so each interval stays about as narrow " + "as with two)", ) group.addoption( "--bench-seed", @@ -444,7 +456,8 @@ def _parametrize_bench_cells(metafunc: pytest.Metafunc): def _bench_sdk_names(config: pytest.Config) -> list[str]: """SDKs the benchmark fans out over, in --sdks order.""" # --sdks may be version-qualified (go@main); benchmark arms come from - # --bench-baseline/--bench-candidate instead, so only the name matters. + # --bench-refs or --bench-baseline/--bench-candidate instead, so only the + # name matters. specs = config.getoption("--sdks") or " ".join(typing.get_args(tdfs.sdk_type)) return list(dict.fromkeys(s.split("@", 1)[0] for s in str(specs).split())) @@ -469,6 +482,7 @@ def pytest_configure(config: pytest.Config): # runs after the session has already built payloads and installed CLIs -- # minutes of work discarded to report a typo. from fixtures.bench import ( + arm_count, config_from_options, disk_shortfall, payloads_from_options, @@ -476,33 +490,35 @@ def pytest_configure(config: pytest.Config): from perf.cells import payloads_to_generate from perf.config import ( BenchmarkRequest, - normalize_baseline_candidate, parse_arm_spec, + requested_arms, validate_sdk_name, ) bench_config = config_from_options(config) payloads = payloads_from_options(config) + refs = config.getoption("--bench-refs", default=None) baseline = config.getoption("--bench-baseline", default=None) candidate = config.getoption("--bench-candidate", default=None) try: - if baseline is not None and candidate is not None: - # A full pair meets the same rules the local and CI entry points - # apply. - arms = normalize_baseline_candidate(baseline, candidate) + if refs is not None or (baseline is not None and candidate is not None): + # A full request meets the same rules the local and CI entry + # points apply. + arms = requested_arms(refs, baseline, candidate) BenchmarkRequest(arms=arms, config=bench_config) + specs = [str(a) for a in arms] else: # Either side may be given alone -- `select_arms` defaults the # other -- so a lone spec is checked only for what it names. - for spec in (baseline, candidate): - if spec is not None: - validate_sdk_name(parse_arm_spec(spec).name) + specs = [s for s in (baseline, candidate) if s is not None] + for spec in specs: + validate_sdk_name(parse_arm_spec(spec).name) # A spec only applies to the SDK it names, so one naming an SDK # outside --sdks would be dropped and the run would measure the # defaults instead. benchmarked = _bench_sdk_names(config) - for spec in (baseline, candidate): - if spec is not None and parse_arm_spec(spec).name not in benchmarked: + for spec in specs: + if parse_arm_spec(spec).name not in benchmarked: raise ValueError( f"{spec!r} names an SDK that is not being benchmarked; " f"--sdks selects {benchmarked}" @@ -519,6 +535,7 @@ def pytest_configure(config: pytest.Config): payloads_to_generate(payloads), n_sdks=len(_bench_sdk_names(config)), prune=bool(config.getoption("--bench-prune-scratch", default=False)), + n_arms=arm_count(config), ) if shortfall: raise pytest.UsageError(shortfall) diff --git a/xtest/fixtures/bench.py b/xtest/fixtures/bench.py index 80bc090f..09818ced 100644 --- a/xtest/fixtures/bench.py +++ b/xtest/fixtures/bench.py @@ -4,7 +4,7 @@ time budget all live here. The measurement loop itself is in ``perf/runner.py`` and the statistics in ``perf/stats.py``; this module is the glue that turns pytest's world (options, fixtures, SDK discovery) into the runner's world -(two arms and a config). +(2 to 4 arms and a config). """ from __future__ import annotations @@ -24,30 +24,34 @@ import tdfs from perf import report from perf.cells import BenchCell, Payload, parse_payloads, payloads_to_generate +from perf.config import budget_seconds, requested_arms from perf.runner import Arm, BenchConfig, Budget, Invocation class ArmSelectionError(Exception): - """The two builds a comparison needs are not both installed.""" + """The builds a comparison needs are not all installed.""" def select_arms( sdk: str, + specs: Sequence[str] | None = None, *, baseline_spec: str | None = None, candidate_spec: str | None = None, -) -> tuple[tdfs.SDK, tdfs.SDK]: - """Pick (baseline, candidate) builds for one SDK. +) -> tuple[tdfs.SDK, ...]: + """Pick the builds to compare for one SDK, reference first. - By default the candidate is the branch build (``main``) and the baseline - is the newest installed release, which is exactly what the CI setup action - lays down side by side. Explicit specs override either side, for - reproducing a comparison or for pinning a specific release. + ``specs`` names every arm, in order: the reproduction of a recorded + comparison, or a multi-arm run. Without it the run is the nightly two-arm + comparison -- the newest installed final release as the reference and the + branch build (``main``) as the candidate -- and ``baseline_spec`` or + ``candidate_spec`` may override either side of that pair. Raises: - ArmSelectionError: if either side is missing or the two resolve to the - same build (a comparison of a build against itself is only - meaningful as the explicit A/A control). + ArmSelectionError: if any named build is missing, if the default pair + cannot be found, or if two arms resolve to the same build (a + comparison of a build against itself is only meaningful as the + explicit A/A control). """ installed = tdfs.all_versions_of(sdk) # pyright: ignore[reportArgumentType] if not installed: @@ -65,6 +69,16 @@ def resolve(spec: str, role: str) -> tdfs.SDK: ) return matches[0] + if specs: + arms = tuple( + resolve(spec, "reference" if i == 0 else f"arm {i + 1}") + for i, spec in enumerate(specs) + ) + if len(set(arms)) != len(arms): + names = ", ".join(str(a) for a in arms) + raise ArmSelectionError(f"arms resolved to the same build: {names}") + return arms + if candidate_spec: candidate = resolve(candidate_spec, "candidate") else: @@ -103,11 +117,36 @@ def resolve(spec: str, role: str) -> tdfs.SDK: # --- Session-scoped configuration ------------------------------------------- +def arm_specs_from_options(config: pytest.Config) -> tuple[str, ...] | None: + """The run's ordered arm specs, reference first, or None for the defaults.""" + refs = cast(str | None, config.getoption("--bench-refs")) + baseline = cast(str | None, config.getoption("--bench-baseline")) + candidate = cast(str | None, config.getoption("--bench-candidate")) + if refs is None and (baseline is None) != (candidate is None): + # A lone --bench-baseline or --bench-candidate overrides one side of + # the default pair; `select_arms` defaults the other. + return None + try: + arms = requested_arms(refs, baseline, candidate) + except ValueError as e: + raise pytest.UsageError(f"invalid benchmark options: {e}") from e + if arms: + return tuple(str(a) for a in arms) + return None + + +def arm_count(config: pytest.Config) -> int: + """How many arms this run will measure per cell.""" + specs = arm_specs_from_options(config) + return len(specs) if specs else 2 + + def config_from_options(config: pytest.Config) -> BenchConfig: """Build a :class:`BenchConfig` from the ``--bench-*`` options. - Every option has a default, so ``getoption`` never returns None here; the - casts are for the type checker, which cannot see the parser setup. + Every option except the budget has a default, so ``getoption`` never + returns None for them; the casts are for the type checker, which cannot + see the parser setup. The budget defaults to one scaled by the arm count. """ def as_int(name: str) -> int: @@ -116,12 +155,13 @@ def as_int(name: str) -> int: def as_float(name: str) -> float: return float(cast(float, config.getoption(name))) + budget = cast(float | None, config.getoption("--bench-budget-seconds")) try: return BenchConfig( min_rounds=as_int("--bench-min-rounds"), max_rounds=as_int("--bench-max-rounds"), warmup=as_int("--bench-warmup"), - budget_seconds=as_float("--bench-budget-seconds"), + budget_seconds=budget_seconds(budget, arm_count(config)), seed=as_int("--bench-seed"), threshold=as_float("--bench-threshold"), ) @@ -201,15 +241,13 @@ def disk_shortfall( - With ``prune`` (see :class:`Scratch`), a cell's outputs and ciphertext are deleted as it ends, so one cell's worth is live at a time: at most the largest decrypt cell's ciphertext plus one output per arm. - - Without it nothing is deleted. Each SDK leaves a baseline-made + - Without it nothing is deleted. Each SDK leaves a reference-made ciphertext per size and one output per arm from every cell, so the total grows with the number of SDKs. A coarse bound, deliberately. It does not credit files an earlier run left in ``tmp_dir``, which this run would reuse or overwrite but which already count against free space; DSPX-5253 tracks doing better. - ``n_arms`` is a parameter so the bound stays right once a run compares - more than two builds. """ total = sum(p.n_bytes for p in payloads) if prune: @@ -265,7 +303,7 @@ def bench_payloads( bench_config: BenchConfig, bench_payload_set: tuple[Payload, ...], ) -> dict[str, Path]: - """Generate one plaintext file per payload size, shared by both arms. + """Generate one plaintext file per payload size, shared by every arm. Content is pseudo-random but seeded, so a rerun measures byte-identical input. Random rather than repetitive because compressible input would let @@ -385,18 +423,36 @@ def _selected_cells(config: pytest.Config) -> list[BenchCell]: @dataclass(frozen=True, slots=True) class BenchArms: - baseline: tdfs.SDK - candidate: tdfs.SDK + """The builds one SDK's cells compare, reference first.""" + + arms: tuple[tdfs.SDK, ...] + + @property + def reference(self) -> tdfs.SDK: + """The build every candidate is compared against.""" + return self.arms[0] + + @property + def candidates(self) -> tuple[tdfs.SDK, ...]: + """Every other arm -- one in the nightly comparison, up to three.""" + return self.arms[1:] class ArmResolver: - """Resolves and memoizes the two builds to compare, per SDK. + """Resolves and memoizes the builds to compare, per SDK. Resolution is lazy so that a missing build skips one SDK's cells with a readable reason instead of erroring out every cell in the module. """ - def __init__(self, baseline_spec: str | None, candidate_spec: str | None) -> None: + def __init__( + self, + specs: Sequence[str] | None, + *, + baseline_spec: str | None = None, + candidate_spec: str | None = None, + ) -> None: + self._specs = tuple(specs) if specs else None self._baseline_spec = baseline_spec self._candidate_spec = candidate_spec self._cache: dict[str, BenchArms] = {} @@ -404,21 +460,26 @@ def __init__(self, baseline_spec: str | None, candidate_spec: str | None) -> Non def __call__(self, sdk: str) -> BenchArms: cached = self._cache.get(sdk) if cached is None: - baseline, candidate = select_arms( - sdk, - baseline_spec=_spec_for(self._baseline_spec, sdk), - candidate_spec=_spec_for(self._candidate_spec, sdk), + cached = self._cache[sdk] = BenchArms( + select_arms( + sdk, + _specs_for(self._specs, sdk), + baseline_spec=_spec_for(self._baseline_spec, sdk), + candidate_spec=_spec_for(self._candidate_spec, sdk), + ) ) - cached = self._cache[sdk] = BenchArms(baseline, candidate) return cached @pytest.fixture(scope="module") def bench_arms(request: pytest.FixtureRequest) -> ArmResolver: - """Resolver for the (baseline, candidate) pair of any SDK in the run.""" + """Resolver for the arms of any SDK in the run, reference first.""" + config = request.config + specs = arm_specs_from_options(config) return ArmResolver( - cast(str | None, request.config.getoption("--bench-baseline")), - cast(str | None, request.config.getoption("--bench-candidate")), + specs, + baseline_spec=None if specs else config.getoption("--bench-baseline"), + candidate_spec=None if specs else config.getoption("--bench-candidate"), ) @@ -429,8 +490,20 @@ def _spec_for(spec: str | None, sdk: str) -> str | None: return spec if spec.split("@", 1)[0] == sdk else None +def _specs_for(specs: Sequence[str] | None, sdk: str) -> tuple[str, ...] | None: + """Return ``specs`` only if they name this SDK, so one flag covers a run. + + A run that measures several SDKs but names arms for one of them lets the + others fall back to their default pair. Validation has already required + every spec to name the same SDK. + """ + if not specs or specs[0].split("@", 1)[0] != sdk: + return None + return tuple(specs) + + #: Features whose presence changes what an encrypt or decrypt actually *does*. -#: If the two arms disagree on one of these they are not performing the same +#: If two arms disagree on one of these they are not performing the same #: operation, and a timing difference between them is a difference in work, #: not in speed. _COMPARABILITY_FEATURES: tuple[tdfs.feature_type, ...] = ( @@ -441,44 +514,49 @@ def _spec_for(spec: str | None, sdk: str) -> str | None: def comparability_problem(arms: BenchArms) -> str | None: - """Return why these two builds cannot be fairly compared, or None.""" + """Return why these builds cannot be fairly compared, or None. + + Every candidate is checked against the reference, since the reference is + what each candidate's contrast is taken against: a candidate that + disagrees with it invalidates its own comparison whatever the other + candidates do. Agreement is transitive through the reference, so this + also establishes that the candidates agree with each other. + """ for feature in _COMPARABILITY_FEATURES: - if arms.baseline.supports(feature) != arms.candidate.supports(feature): + ref_has = arms.reference.supports(feature) + for arm in arms.candidates: + if arm.supports(feature) == ref_has: + continue supporter, other = ( - (arms.baseline, arms.candidate) - if arms.baseline.supports(feature) - else (arms.candidate, arms.baseline) + (arms.reference, arm) if ref_has else (arm, arms.reference) ) return ( f"{supporter} supports [{feature}] and {other} does not, so the " - "two arms would not be doing the same work" + "arms would not be doing the same work" ) return None def pinned_target_mode(arms: BenchArms) -> tdfs.container_version | None: - """Pick one container version both arms emit, or None for their default. + """Pick one container version every arm emits, or None for their default. - Letting each arm choose its own target would compare two output formats. - ``None`` is only returned when neither arm can be told which to use, in + Letting each arm choose its own target would compare output formats. + ``None`` is only returned when the arms cannot be told which to use, in which case :func:`comparability_problem` has already established that they agree on the relevant features and will pick the same one. """ - if not ( - arms.baseline.supports("hexaflexible") - and arms.candidate.supports("hexaflexible") - ): + if not all(a.supports("hexaflexible") for a in arms.arms): return None - if arms.baseline.supports("hexless") and arms.candidate.supports("hexless"): + if all(a.supports("hexless") for a in arms.arms): return "4.3.0" return "4.2.2" class CiphertextFactory: - """Baseline-produced ciphertexts for the decrypt cells, made on demand. + """Reference-produced ciphertexts for the decrypt cells, made on demand. - Both arms of a decrypt comparison must read the *same* file. If each arm - decrypted its own output, a difference in how the two builds *write* a TDF + Every arm of a decrypt comparison must read the *same* file. If each arm + decrypted its own output, a difference in how the builds *write* a TDF would show up as a difference in how fast they read one. """ @@ -494,13 +572,14 @@ def __init__( self._cache: dict[tuple[str, str], Path] = {} def __call__(self, arms: BenchArms, payload_label: str) -> Path: - key = (str(arms.baseline), payload_label) + reference = arms.reference + key = (str(reference), payload_label) cached = self._cache.get(key) # --bench-prune-scratch deletes a ciphertext once its cell ends. if cached is not None and cached.is_file(): return cached - ct_file = self._tmp_dir / f"bench-ct-{arms.baseline}-{payload_label}.tdf" - arms.baseline.encrypt( + ct_file = self._tmp_dir / f"bench-ct-{reference}-{payload_label}.tdf" + reference.encrypt( self._payloads[payload_label], ct_file, container="ztdf", @@ -520,10 +599,10 @@ def bench_ciphertexts( ) -> CiphertextFactory: """Ciphertext source for decrypt cells. - Pinned to the explicit RSA attribute so both arms wrap with RSA regardless - of what base key the platform happens to have configured -- an arm that - silently switched to EC would look slower for reasons that have nothing to - do with a regression. + Pinned to the explicit RSA attribute so every arm wraps with RSA + regardless of what base key the platform happens to have configured -- an + arm that silently switched to EC would look slower for reasons that have + nothing to do with a regression. """ return CiphertextFactory(bench_payloads, tmp_dir, attribute_default_rsa.value_fqns) @@ -537,20 +616,24 @@ def build_arms( tmp_dir: Path, attr_values: list[str], ) -> tuple[Arm, ...]: - """Turn a cell plus its two builds into one ready-to-run invocation each. + """Turn a cell plus its builds into one ready-to-run invocation each. Everything that is not the build under test is pinned identically across - the arms: same plaintext, same attribute (so both wrap with RSA), same - container, same target mode. A functional difference between the builds - that changed any of these would otherwise show up as a speed difference. + the arms: same plaintext, same attribute (so every arm wraps with RSA), + same container, same target mode. A functional difference between the + builds that changed any of these would otherwise show up as a speed + difference. - For decrypt, both arms read the *same* ``ct_file``, produced once by the - baseline. Letting each arm decrypt its own output would compare the cost - of reading two different files. + For decrypt, every arm reads the *same* ``ct_file``, produced once by the + reference. Letting each arm decrypt its own output would compare the cost + of reading different files. - In a control cell both arms are the baseline build, so they differ only in + In a control cell every arm is the reference build, so they differ only in the output path -- exactly the harness overhead the A/A cell exists to - measure. + measure. It has as many arms as the real cells rather than a cheap pair: + in a K-arm round the last arm runs K-1 invocations after the first and + carries more drift than an adjacent pair does, and a two-arm control would + understate the noise of the contrasts being judged. """ target_mode = pinned_target_mode(arms) @@ -575,11 +658,14 @@ def invocation(sdk: tdfs.SDK, arm_id: str) -> Invocation: return Invocation(argv, env, out) if cell.control: - # The same build twice. The ids have to differ -- they key the sample + # Same build K times. The ids have to differ -- they key the sample # vectors -- so they are numbered rather than named after the version. - builds = [(f"{arms.baseline.version}#{i + 1}", arms.baseline) for i in range(2)] + builds = [ + (f"{arms.reference.version}#{i + 1}", arms.reference) + for i in range(len(arms.arms)) + ] else: - builds = [(sdk.version, sdk) for sdk in (arms.baseline, arms.candidate)] + builds = [(sdk.version, sdk) for sdk in arms.arms] return tuple( Arm(arm_id, str(sdk), invocation(sdk, arm_id)) for arm_id, sdk in builds ) diff --git a/xtest/perf/README.md b/xtest/perf/README.md index f1f17358..04f852f3 100644 --- a/xtest/perf/README.md +++ b/xtest/perf/README.md @@ -3,10 +3,11 @@ A paired A/B benchmark for the OpenTDF SDK CLIs. It answers one question: **did this change make the SDK measurably and meaningfully slower?** -Two builds — normally the newest installed release and the branch build — -are measured on the *same* runner, interleaved round by round, and only their -*ratio* is reported. Nothing is ever compared against a stored historical -number. +Two to four builds of one SDK — normally the newest installed release and the +branch build — are measured on the *same* runner, interleaved round by round, +and only their *ratios* are reported. The first build is the **reference**; +each other build (a **candidate**) is compared against it and can fail the run. +Nothing is ever compared against a stored historical number. It runs nightly (one runner per SDK) and on `workflow_dispatch` with `run-benchmarks` checked. It never runs on pull requests: 30 minutes of serial @@ -137,9 +138,12 @@ lists the reason for each cell. ### The A/A control -Each SDK gets a control cell that compares the baseline build **against itself** +Each SDK gets a control cell that compares the reference build **against itself** through the identical pipeline. Its true ratio is exactly 1.0 by construction, so -whatever it reports is the harness's own error on this runner. It does two jobs: +whatever it reports is the harness's own error on this runner. With K arms the +control runs K copies of the reference, since the last arm in a round drifts +further from the first than an adjacent pair does; of its K-1 A/A contrasts, the +worst one stands as the floor. It does two jobs: - If the control *trips* — its own A/A comparison looks like a real effect — then something is systematically biased and **the whole run stops being able to fail @@ -237,8 +241,9 @@ things; the noise floor will tell you whether you succeeded. | `runner.py` | The paired round loop, the stopping rule, the budget, `analyze()` | | `stats.py` | Pure functions: log-ratios, bootstrap CI, Wilcoxon, BH, the decision rule | | `report.py` | Session recorder, JSON artifact (`SCHEMA_VERSION`), step-summary markdown | +| `config.py` | Option parsing and validation shared by pytest and CI: `BenchConfig`, `--bench-refs`, the arm ceiling, the budget default. No pytest | | `docs/` | The figures in this README, and the Monte Carlo behind them. Documentation only; imported by nothing in the harness | -| `../fixtures/bench.py` | The pytest glue: arm selection, payloads, ciphertexts, budget | +| `../fixtures/bench.py` | The pytest glue: arm selection (options or defaults), payloads, ciphertexts, budget | | `../test_benchmarks.py` | One test per cell. **Records; never asserts** | | `../conftest.py` | `--bench*` options, cell parametrization, the session-finish gate | @@ -458,8 +463,8 @@ does not silently switch to EC), the same container, and the same target mode. disagree on `hexless`, `hexaflexible`, or `autoconfigure` — a timing difference there is a difference in *work*, not in speed. -For decrypt, both arms read one ciphertext produced by the baseline. If each arm -decrypted its own output, a difference in how the two builds *write* a TDF would +For decrypt, every arm reads one ciphertext produced by the reference. If each +arm decrypted its own output, a difference in how the builds *write* a TDF would show up as a difference in how fast they read one. #### Baselines must be final releases @@ -550,7 +555,7 @@ two builds doing different amounts of work) is invisible in the output. 3. Never let a cell assert; the gate is run-level. 4. Never report PASS without a noise floor establishing the run had the power to fail. -5. Never let the two arms differ in anything but the build. +5. Never let the arms differ in anything but the build. 6. Never run the measured command from a process holding memory. 7. Never run the benchmark in parallel with anything, including itself. 8. Never let a run that measured nothing report success. diff --git a/xtest/perf/config.py b/xtest/perf/config.py index 3b80ce9f..e3886f49 100644 --- a/xtest/perf/config.py +++ b/xtest/perf/config.py @@ -16,6 +16,7 @@ from __future__ import annotations import math +import re from collections import Counter from dataclasses import dataclass, field @@ -30,10 +31,11 @@ MIN_USABLE_ROUNDS, ) -#: Benchmark arm-count ceiling. Stage 3 raises this to 4 when multi-arm -#: measurement lands; it is one constant so that stage changes one number -#: rather than adding a second encoding. -MAX_ARM_COUNT = 2 +#: Benchmark arm-count ceiling. A deliberate limit of the benchmark rather +#: than of any installer: every arm runs once per round, so a fifth arm costs +#: another 25% of every round while each candidate's interval stays as wide as +#: the budget allows. Raise it by changing this one constant. +MAX_ARM_COUNT = 4 #: Default two-sided confidence level, derived from alpha rather than written #: out, so the interval and the test can never drift apart. @@ -175,6 +177,46 @@ def parse_arm_spec(spec: str) -> ArmRequest: return ArmRequest(name=name, spec=version or None) +def parse_refs(spec: str) -> tuple[ArmRequest, ...]: + """Parse a `--bench-refs` value into an ordered request, reference first. + + Commas or whitespace separate the builds, so both the shell-friendly + ``go@main,go@my-branch`` and a quoted space-separated list work. The + result is validated by :class:`BenchmarkRequest` as well: 2 to + :data:`MAX_ARM_COUNT` builds of one known SDK, none named twice. + """ + arms = tuple(parse_arm_spec(p) for p in re.split(r"[,\s]+", spec.strip()) if p) + if len(arms) < 2: + raise ValueError( + f"need 2 to {MAX_ARM_COUNT} refs, got {len(arms)}: {spec!r}. The " + "first is the reference every candidate is compared against." + ) + BenchmarkRequest(arms=arms) + return arms + + +def requested_arms( + refs: str | None, baseline_spec: str | None, candidate_spec: str | None +) -> tuple[ArmRequest, ...]: + """The run's ordered arm request from whichever option form was used. + + `--bench-refs` is the general form; `--bench-baseline` and + `--bench-candidate` are the two-arm adapter onto the same request. Mixing + them is an error rather than a merge -- there is no reading of + ``--bench-refs a,b --bench-candidate c`` that is not a mistake. Returns an + empty tuple when no arms were named, which leaves arm selection to its + defaults. + """ + if refs is not None and (baseline_spec is not None or candidate_spec is not None): + raise ValueError( + "--bench-refs cannot be combined with --bench-baseline or " + "--bench-candidate; it supersedes both" + ) + if refs is not None: + return parse_refs(refs) + return normalize_baseline_candidate(baseline_spec, candidate_spec) + + def normalize_baseline_candidate( baseline_spec: str | None, candidate_spec: str | None ) -> tuple[ArmRequest, ...]: @@ -223,3 +265,16 @@ def default_budget_for_arm_count( if arm_count < 2: raise ValueError(f"arm_count must be at least 2; got {arm_count}") return base_budget * arm_count / 2.0 + + +def budget_seconds(explicit: float | None, arm_count: int) -> float: + """The run's budget: an explicit one as given, else the scaled default. + + At a fixed budget the attained round count falls as ``2/K`` and every + interval widens as ``sqrt(K/2)``; the scaled default buys that back, so a + three-arm run is about as precise as the two-arm run the number was chosen + for. Someone who named a number has already decided what to spend. + """ + if explicit is not None: + return explicit + return default_budget_for_arm_count(arm_count) diff --git a/xtest/test_bench_arms.py b/xtest/test_bench_arms.py index e4969e6b..ed1c1557 100644 --- a/xtest/test_bench_arms.py +++ b/xtest/test_bench_arms.py @@ -404,37 +404,50 @@ def touch(path: Path) -> Path: return path +class TestMultiArmSelection: + def test_named_arms_resolve_in_order(self, cwd: Path): + install(cwd, "go", "main", "v0.29.0", "my-branch") + arms = bench.select_arms("go", ["go@v0.29.0", "go@main", "go@my-branch"]) + assert [a.version for a in arms] == ["v0.29.0", "main", "my-branch"] + + def test_a_missing_arm_is_named(self, cwd: Path): + install(cwd, "go", "main", "v0.29.0") + with pytest.raises(bench.ArmSelectionError, match="arm 3"): + bench.select_arms("go", ["go@v0.29.0", "go@main", "go@nope"]) + + class TestBuildArms: - def arms(self, cwd: Path) -> bench.BenchArms: - install(cwd, "go", "v0.29.0", "main") - for v in ("v0.29.0", "main"): + def arms(self, cwd: Path, n: int) -> bench.BenchArms: + # A release first, as in the nightly pair: its dots must survive into + # every output name. + versions = ("v0.29.0", "main", "a--impl", "b--impl")[:n] + install(cwd, "go", *versions) + for v in versions: # `supports` runs the stub, which then answers yes to everything. (cwd / "sdk" / "go" / "dist" / v / "cli.sh").chmod(0o755) - return bench.BenchArms(tdfs.SDK("go", "v0.29.0"), tdfs.SDK("go", "main")) + return bench.BenchArms(tuple(tdfs.SDK("go", v) for v in versions)) - def test_arms_are_named_after_their_builds(self, cwd: Path): + @pytest.mark.parametrize("n", [2, 3, 4]) + def test_a_cell_runs_each_build_once(self, cwd: Path, n: int): + arms = self.arms(cwd, n) cell = next(c for c in cells_for(["go"]) if not c.control) built = bench.build_arms( - cell, - self.arms(cwd), - pt_file=cwd / "pt", - ct_file=None, - tmp_dir=cwd, - attr_values=[], + cell, arms, pt_file=cwd / "pt", ct_file=None, tmp_dir=cwd, attr_values=[] ) - assert [a.name for a in built] == ["v0.29.0", "main"] - assert len({a.invocation.output for a in built}) == 2 - - def test_the_control_runs_the_baseline_twice_under_distinct_ids(self, cwd: Path): + assert [a.name for a in built] == [s.version for s in arms.arms] + assert len({a.invocation.output for a in built}) == n + + @pytest.mark.parametrize("n", [2, 3, 4]) + def test_the_control_is_k_copies_of_the_reference(self, cwd: Path, n: int): + # As many arms as the real cells: the last of K drifts further from + # the first than an adjacent pair does, and a two-arm control would + # understate the noise of the contrasts being judged. + arms = self.arms(cwd, n) control = next(c for c in cells_for(["go"]) if c.control) built = bench.build_arms( - control, - self.arms(cwd), - pt_file=cwd / "pt", - ct_file=None, - tmp_dir=cwd, - attr_values=[], + control, arms, pt_file=cwd / "pt", ct_file=None, tmp_dir=cwd, attr_values=[] ) + assert len(built) == n assert {a.label for a in built} == {"go@v0.29.0"} - assert len({a.name for a in built}) == 2, "ids key the sample vectors" - assert len({a.invocation.output for a in built}) == 2 + assert len({a.name for a in built}) == n, "ids key the sample vectors" + assert len({a.invocation.output for a in built}) == n diff --git a/xtest/test_bench_config.py b/xtest/test_bench_config.py index 56c1ace1..69dc7505 100644 --- a/xtest/test_bench_config.py +++ b/xtest/test_bench_config.py @@ -13,6 +13,7 @@ import pytest import conftest +from fixtures import bench from perf import report, stats from perf.cells import DEFAULT_PAYLOAD_SPEC, BenchCell from perf.config import ( @@ -25,9 +26,12 @@ ArmRequest, BenchConfig, BenchmarkRequest, + budget_seconds, default_budget_for_arm_count, normalize_baseline_candidate, parse_arm_spec, + parse_refs, + requested_arms, validate_sdk_name, ) @@ -221,6 +225,56 @@ def test_arm_count_too_low_raises(self): default_budget_for_arm_count(1) +class TestParseRefs: + @pytest.mark.parametrize( + "spec", + ["go@v0.29.0,go@main", "go@v0.29.0 go@main go@a", "go@1, go@2 ,go@3,go@4"], + ) + def test_two_to_four_refs_parse_in_order(self, spec: str): + arms = parse_refs(spec) + assert 2 <= len(arms) <= MAX_ARM_COUNT == 4 + assert arms[0].name == "go" + assert str(arms[0]) == spec.replace(",", " ").split()[0] + + @pytest.mark.parametrize( + ("spec", "match"), + [ + ("go@main", "need 2 to 4"), + ("go@1,go@2,go@3,go@4,go@5", "at most 4"), + ("go@main,go@main", "duplicate"), + ("go@main,java@main", "same SDK"), + ("gp@main,gp@v1", "unknown SDK"), + ("go@,go@main", "empty version"), + ], + ) + def test_invalid_requests_are_refused(self, spec: str, match: str): + with pytest.raises(ValueError, match=match): + parse_refs(spec) + + +class TestRequestedArms: + def test_baseline_and_candidate_are_an_adapter_onto_refs(self): + assert requested_arms(None, "go@v0.29.0", "go@main") == parse_refs( + "go@v0.29.0,go@main" + ) + + def test_mixing_the_forms_is_refused(self): + with pytest.raises(ValueError, match="cannot be combined"): + requested_arms("go@a,go@b", "go@v0.29.0", None) + + def test_nothing_named_defers_to_arm_selection(self): + assert requested_arms(None, None, None) == () + + +class TestBudgetSeconds: + def test_the_default_scales_with_arm_count(self): + assert budget_seconds(None, 2) == 1500.0 + assert budget_seconds(None, 3) == 2250.0 + + def test_an_explicit_budget_is_taken_as_given(self): + assert budget_seconds(900.0, 4) == 900.0 + + class _FakeConfig: """Just enough of ``pytest.Config`` for the ``--bench`` checks in ``pytest_configure``.""" @@ -228,6 +282,7 @@ def __init__(self, **opts: object): self._options: dict[str, object] = { "--bench": True, "--sdks": "go", + "--bench-refs": None, "--bench-baseline": None, "--bench-candidate": None, "--bench-threshold": stats.DEFAULT_THRESHOLD, @@ -338,6 +393,34 @@ def test_payloads_that_cannot_fit_are_rejected( with pytest.raises(pytest.UsageError, match="scratch space"): _configure() + def test_the_disk_check_counts_every_arm(self, monkeypatch: pytest.MonkeyPatch): + # Each arm leaves its own output per cell, so room for the default + # pair is not room for four. + seen: list[object] = [] + monkeypatch.setattr( + bench, "disk_shortfall", lambda *a, **kw: seen.append(kw["n_arms"]) + ) + _configure(**{"--bench-refs": "go@a,go@b,go@c,go@d"}) + assert seen == [4] + + def test_multi_arm_refs_accepted(self): + _configure(**{"--bench-refs": "go@v0.29.0,go@main,go@my-branch"}) + + @pytest.mark.parametrize( + ("opts", "match"), + [ + ( + {"--bench-refs": "go@a,go@b", "--bench-candidate": "go@main"}, + "cannot be combined", + ), + ({"--bench-refs": "go@1,go@2,go@3,go@4,go@5"}, "at most 4"), + ({"--sdks": "java", "--bench-refs": "go@a,go@b"}, "not being benchmarked"), + ], + ) + def test_bad_refs_rejected(self, opts: dict[str, object], match: str): + with pytest.raises(pytest.UsageError, match=match): + _configure(**opts) + def test_noop_without_bench(self): _configure(**{"--bench": False, "--bench-baseline": "gp@main"}) diff --git a/xtest/test_benchmarks.py b/xtest/test_benchmarks.py index c08be721..be53b8a6 100644 --- a/xtest/test_benchmarks.py +++ b/xtest/test_benchmarks.py @@ -1,8 +1,8 @@ """SDK performance regression cells. -One test per cell: an operation at a payload size, measuring the newest -installed release against the branch build on the same runner, in the same -round, in a randomized order. +One test per cell: an operation at a payload size, measuring 2 to 4 builds -- +by default the newest installed release against the branch build -- on the +same runner, in the same round, in a randomized order. **These tests do not assert.** Each one records its raw samples and passes. The verdict cannot be reached cell by cell: the multiplicity correction is