From 876925e46f0e9c739bbddac250fdd50802e3733c Mon Sep 17 00:00:00 2001 From: Dave Mihalcik Date: Wed, 23 Sep 2026 12:36:16 -0400 Subject: [PATCH 1/2] fix(otdf-sdk-mgr): flatten slashed branch names into dist/src tags A branch ref like 'feat/DSPX-2604-createtdf-chunked' resolved by name (not by SHA) kept its slash in the tag, nesting dist// and src// one level deeper than every consumer expects: xtest's all_versions_of() lists dist/*/ and the Go Makefile finds src/*/, so the build was silently discovered as a bare 'feat' directory with no cli.sh in it. Flatten the tag the same way _classify_sha_match already flattens a branch reached by SHA. --- otdf-sdk-mgr/src/otdf_sdk_mgr/resolve.py | 9 ++++++++- otdf-sdk-mgr/tests/test_resolve.py | 19 ++++++++++++++++++- 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/otdf-sdk-mgr/src/otdf_sdk_mgr/resolve.py b/otdf-sdk-mgr/src/otdf_sdk_mgr/resolve.py index 9c50d2ce1..7ff3b6811 100644 --- a/otdf-sdk-mgr/src/otdf_sdk_mgr/resolve.py +++ b/otdf-sdk-mgr/src/otdf_sdk_mgr/resolve.py @@ -300,7 +300,14 @@ def _resolve_against( "alias": version, "head": True, "sha": sha, - "tag": version, + # Flattened the same way _classify_sha_match flattens a branch + # it reached by SHA: the tag becomes a single dist// and + # src// path component. A slash here nests those + # directories, and every consumer walks them one level deep -- + # xtest's all_versions_of() lists dist/*/ and the go Makefile + # finds src/*/, so "feat/x" is discovered as a "feat" build + # with no cli.sh in it. + "tag": version.replace("/", "--"), } if infix and version.startswith(f"{infix}/"): diff --git a/otdf-sdk-mgr/tests/test_resolve.py b/otdf-sdk-mgr/tests/test_resolve.py index a7c30b057..29efa6f6f 100644 --- a/otdf-sdk-mgr/tests/test_resolve.py +++ b/otdf-sdk-mgr/tests/test_resolve.py @@ -76,7 +76,24 @@ def test_refs_heads_non_main_branch(self): result = resolve("js", "refs/heads/release/sdk-v0.17", None) assert is_resolve_success(result) assert "head" in result and result["head"] is True - assert result["tag"] == "release/sdk-v0.17" + assert result["tag"] == "release--sdk-v0.17" + assert result["sha"] == SHA40 + + def test_branch_by_name_flattens_slashes(self): + # Same flattening the SHA path applies, and for the same reason: the + # tag is one path component under dist/ and src/. Reached by name + # rather than by SHA, which is the shape a workflow_dispatch input + # arrives in. + ls = make_ls_remote( + (SHA40, "refs/heads/feat/DSPX-2604-createtdf-chunked"), + ("d" * 40, "refs/heads/main"), + ) + with patch_git(ls): + result = resolve("go", "feat/DSPX-2604-createtdf-chunked", None) + assert is_resolve_success(result) + assert result.get("head") is True + assert result["tag"] == "feat--DSPX-2604-createtdf-chunked" + assert result["alias"] == "feat/DSPX-2604-createtdf-chunked" assert result["sha"] == SHA40 From 7bd7506988c7ba912393c90959838c429268f598 Mon Sep 17 00:00:00 2001 From: Dave Mihalcik Date: Wed, 23 Sep 2026 12:39:22 -0400 Subject: [PATCH 2/2] feat(xtest): compare up to four benchmark arms in one run Generalizes the paired A/B benchmark from a fixed baseline/candidate pair to 2-4 named arms, so a bake-off between competing implementations of the same feature is one measurement on one runner instead of two separate two-arm dispatches whose ratios share no denominator. - perf/runner.py: Arm/CellResult keyed by arm_ids and an explicit reference, replacing the baseline/candidate pair. The A/A control now runs K copies of the reference (not a cheap pair), since the last arm in a K-arm round carries more within-round drift than an adjacent pair does. Also folds in trunk's independent refinements: per-arm deadline capping so one slow arm can't run past the shared budget, and a strict configured-minimum-rounds check (previously only the statistical hard floor was enforced). - perf/stats.py: every non-reference arm gets its own gated contrast against the reference; every pair of non-reference arms gets a symmetric FASTER/SLOWER/TIED head-to-head that never gates the build. Three BH-corrected families (gated / symmetric / ungated) so a bake-off costs the regression gate no power. Also folds in trunk's has_candidate_comparisons fix, which catches a control-only run that trunk's simpler nothing_measured check would have missed. - perf/report.py, fixtures/bench.py, conftest.py: --bench-refs (with --bench-baseline/--bench-candidate kept as two-arm shorthand), arm resolution and comparability checks generalized to K arms, and bake-off results rendered in the job summary and terminal output. - xtest.yml: bench-refs/bench-budget-seconds/bench-max-rounds inputs, a validate-and-normalize step, and a per-arm resolution step in the bench job. Budget defaults scale by arm-count/2 since a K-arm round costs K invocations, and the job timeout goes to 240m to cover it. Depends on the otdf-sdk-mgr slash-flattening fix: named-ref arms can be arbitrary branches, and an unflattened tag breaks build discovery. --- .github/workflows/check.yml | 5 +- .github/workflows/xtest.yml | 232 ++++++++- xtest/conftest.py | 67 ++- xtest/fixtures/bench.py | 397 +++++++++++---- xtest/perf/report.py | 955 ++++++++++++++++++++++++++++++++++-- xtest/perf/runner.py | 418 ++++++++++------ xtest/perf/stats.py | 294 +++++++++-- xtest/test_bench_arms.py | 363 ++++++++++++-- xtest/test_bench_report.py | 380 ++++++++++++++ xtest/test_bench_runner.py | 332 ++++++++++--- xtest/test_bench_stats.py | 189 ++++++- xtest/test_benchmarks.py | 10 +- 12 files changed, 3185 insertions(+), 457 deletions(-) create mode 100644 xtest/test_bench_report.py diff --git a/.github/workflows/check.yml b/.github/workflows/check.yml index 31b7d5167..33ced7c18 100644 --- a/.github/workflows/check.yml +++ b/.github/workflows/check.yml @@ -47,8 +47,9 @@ jobs: run: >- uv run --frozen --no-build pytest --no-header -q test_bench_stats.py test_bench_measure.py test_bench_runner.py - test_bench_arms.py test_sdk_commands.py test_tdfs_units.py - test_encryption_units.py test_sizes_units.py test_zip64_units.py + test_bench_arms.py test_bench_report.py test_sdk_commands.py + test_tdfs_units.py test_encryption_units.py test_sizes_units.py + test_zip64_units.py working-directory: xtest - name: Lint and test otdf-local run: | diff --git a/.github/workflows/xtest.yml b/.github/workflows/xtest.yml index c14af20c3..c2e9c710e 100644 --- a/.github/workflows/xtest.yml +++ b/.github/workflows/xtest.yml @@ -53,6 +53,31 @@ on: type: string default: "" description: "Comma-separated platform features to treat as supported. Does not override SDK gates or enable service configuration. Unknown names fail the run." + bench-refs: + required: false + type: string + default: "" + description: "Benchmark these refs against each other instead of the default newest-release-vs-branch-head pair. Comma-separated, 2 to 4 entries, first is the reference every other arm is gated against: 'main,fix/streaming-writer,DSPX-4499-streaming-codec'. Any ref otdf-sdk-mgr resolves works: a branch, a tag, a SHA, 'refs/pull/N/head'. The 4-arm ceiling is setup-cli-tool's -- it installs at most four builds side by side. All arms are measured in the same rounds on this one runner, so every pair is comparable, including two candidates against each other. For a bake-off between two implementations of the same feature, make the reference their shared parent rather than 'main' if they are stacked: 'main' then measures each candidate's whole stack, though the head-to-head that decides the bake-off is unaffected either way. Requires a focus-sdk naming one SDK; ignores the *-ref inputs, which drive the functional matrix rather than this." + bench-baseline-ref: + required: false + type: string + default: "" + description: "Deprecated two-arm spelling of bench-refs; 'X' here and 'Y' in bench-candidate-ref means bench-refs='X,Y'. Setting both forms is an error." + bench-candidate-ref: + required: false + type: string + default: "" + description: "Deprecated: the second arm of the bench-baseline-ref pair. e.g. 'feat/DSPX-2604-createtdf-chunked'." + bench-budget-seconds: + required: false + type: string + default: "" + description: "Wall-clock allowance shared by every benchmark cell (default 1500 for two arms, scaled by K/2 for K arms). A K-arm round costs K invocations rather than 2, so at a fixed budget every interval widens by ~sqrt(K/2); the scaled default buys that back." + bench-max-rounds: + required: false + type: string + default: "" + description: "Hard cap on paired rounds per cell (default 60). Raise it together with the budget: at the default, cells routinely stop on max_rounds with budget left over, and every unspent round is interval width that could have been bought." workflow_call: inputs: platform-ref: @@ -96,6 +121,26 @@ on: type: string default: "" description: "Comma-separated platform features to treat as supported. Does not override SDK gates or enable service configuration. Unknown names fail the run." + bench-refs: + required: false + type: string + default: "" + bench-baseline-ref: + required: false + type: string + default: "" + bench-candidate-ref: + required: false + type: string + default: "" + bench-budget-seconds: + required: false + type: string + default: "" + bench-max-rounds: + required: false + type: string + default: "" schedule: - cron: "30 6 * * *" # 0630 UTC - cron: "0 5 * * 1,3" # 500 UTC (Monday, Wednesday) @@ -125,6 +170,11 @@ jobs: platform-tag-list: ${{ steps.version-info.outputs.platform-tag-list }} heads: ${{ steps.version-info.outputs.platform-heads }} default-tags: ${{ steps.version-info.outputs.default-tags }} + bench-sdks: ${{ steps.bench-inputs.outputs.sdks }} + # Normalized here so the bench job never sees the deprecated pair, and + # so the arm count that scales the budget is counted once. + bench-refs: ${{ steps.bench-inputs.outputs.refs }} + bench-budget-seconds: ${{ steps.bench-inputs.outputs.budget-seconds }} go: ${{ steps.version-info.outputs.go-version-info }} java: ${{ steps.version-info.outputs.java-version-info }} js: ${{ steps.version-info.outputs.js-version-info }} @@ -144,6 +194,78 @@ jobs: echo "Invalid focus-sdk input: ${FOCUS_SDK_INPUT}. Must be one of: all, go, java, js." >> "$GITHUB_STEP_SUMMARY" exit 1 fi + # Decided here rather than in the bench job because a matrix cannot be + # narrowed from inside the job it belongs to: a bad combination would + # already have spun up three runners for 45 minutes each. + - name: Validate benchmark inputs and pick the bench matrix + id: bench-inputs + env: + FOCUS_SDK: ${{ inputs.focus-sdk || 'all' }} + BENCH_REFS: ${{ inputs.bench-refs }} + BASELINE_REF: ${{ inputs.bench-baseline-ref }} + CANDIDATE_REF: ${{ inputs.bench-candidate-ref }} + BUDGET_SECONDS: ${{ inputs.bench-budget-seconds }} + MAX_ROUNDS: ${{ inputs.bench-max-rounds }} + run: |- + for pair in "bench-budget-seconds:$BUDGET_SECONDS" "bench-max-rounds:$MAX_ROUNDS"; do + name=${pair%%:*} + value=${pair#*:} + if [[ -n "$value" && ! "$value" =~ ^[1-9][0-9]*$ ]]; then + echo "::error::${name} must be a positive whole number, got '${value}'." + exit 1 + fi + done + # Fold the deprecated pair into bench-refs, so everything downstream + # of this step deals with one list and one arm count. + refs=$BENCH_REFS + if [[ -n "$BASELINE_REF" || -n "$CANDIDATE_REF" ]]; then + if [[ -n "$refs" ]]; then + echo "::error::bench-refs replaces bench-baseline-ref/bench-candidate-ref -- give one form or the other, not both." + exit 1 + fi + if [[ -z "$BASELINE_REF" || -z "$CANDIDATE_REF" ]]; then + echo "::error::bench-baseline-ref and bench-candidate-ref must be set together; a comparison needs both arms named." + exit 1 + fi + refs="${BASELINE_REF},${CANDIDATE_REF}" + fi + # Two arms unless told otherwise: with no refs at all the bench job + # falls back to newest-release-vs-branch-head, which is a pair. + n_arms=2 + if [[ -n "$refs" ]]; then + read -r -a arms <<<"${refs//,/ }" + n_arms=${#arms[@]} + # 4 is setup-cli-tool's ceiling (slots a/b/c/d); a 5th arm would + # be silently dropped there and then missing from every round. + if ((n_arms < 2 || n_arms > 4)); then + echo "::error::bench-refs needs 2 to 4 refs, got ${n_arms}: '${refs}'. The 4-arm cap is setup-cli-tool's, which installs at most four builds side by side." + exit 1 + fi + if [[ "$(printf '%s\n' "${arms[@]}" | sort -u | wc -l)" -ne "$n_arms" ]]; then + echo "::error::bench-refs names the same ref twice: '${refs}'. Every arm has to be a distinct build." + exit 1 + fi + if [[ "$FOCUS_SDK" == "all" ]]; then + echo "::error::bench-refs names refs of one SDK, so focus-sdk must be go, java, or js -- not 'all'." + exit 1 + fi + printf -v refs '%s,' "${arms[@]}" + refs=${refs%,} + fi + # A K-arm round costs K invocations, so a fixed budget buys K/2 as + # many rounds and every interval widens by ~sqrt(K/2). Scaling the + # default keeps a 3-arm run as precise as the 2-arm one it replaces. + budget=${BUDGET_SECONDS:-$((1500 * n_arms / 2))} + { + echo "refs=${refs}" + echo "budget-seconds=${budget}" + } >> "$GITHUB_OUTPUT" + if [[ "$FOCUS_SDK" == "all" ]]; then + echo 'sdks=["go","java","js"]' >> "$GITHUB_OUTPUT" + else + echo "sdks=[\"${FOCUS_SDK}\"]" >> "$GITHUB_OUTPUT" + fi + - name: Default Versions depend on context id: default-tags run: |- @@ -844,19 +966,33 @@ jobs: ${{ steps.kas-km3.outputs.log-file }} if-no-files-found: ignore - # Paired A/B performance regression benchmark. + # Paired performance regression benchmark: two arms by default, up to four + # with bench-refs. # # Absolute timings from a GitHub-hosted runner are not comparable to # timings from any other runner -- CPU model, tenancy, and steal time all # vary more than any regression worth catching. So nothing is compared to - # history. Instead both builds under comparison run on *this* runner, in - # the same interleaved round, and only their ratio is reported. Runner + # history. Instead every build under comparison runs on *this* runner, in + # the same interleaved round, and only their ratios are reported. Runner # speed divides out. # + # That is also why a bake-off has to be one job rather than two dispatches: + # two runs put the candidates on different runners, so their ratios share + # no denominator and the comparison is invalid by the premise above. + # # Never runs on pull requests: 30 minutes of serial measurement is too slow # for a PR gate, and a PR runner is the noisiest place to measure. bench: - timeout-minutes: 45 + # Has to cover setup plus the whole of bench-budget-seconds, and setup is + # not a small constant: a warm Go module cache builds both arms in ~3 + # minutes, a cold one took 19. At 45 this job could not even finish its + # own default 1500s budget after a cold start -- it would be killed + # mid-measurement, which loses the report entirely rather than reporting + # fewer rounds. The budget is the knob that bounds the run; this is only + # the backstop for a hung one. 240 rather than 90 because a K-arm run + # scales its budget by K/2 and adds a build per arm: three arms at 1 GiB + # want 8100s = 135 minutes of measurement before any setup at all. + timeout-minutes: 240 runs-on: ubuntu-latest needs: resolve-versions # Nightly cron only, not the Mon/Wed or weekly ones: three runs a week of @@ -870,10 +1006,11 @@ jobs: packages: read strategy: # One runner per SDK. Two SDKs on one runner would contend for the very - # CPU being measured. + # CPU being measured. Narrowed by focus-sdk, so investigating one SDK + # does not spend 45 minutes measuring the two nobody asked about. fail-fast: false matrix: - sdk: [go, java, js] + sdk: ${{ fromJSON(needs.resolve-versions.outputs.bench-sdks) }} steps: - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 with: @@ -945,17 +1082,57 @@ jobs: env: PLATFORM_DIR: ${{ steps.run-platform.outputs.platform-working-dir }} - ######## INSTALL BOTH ARMS OF THE COMPARISON ############# - # The whole design rests on this step laying down two builds side by - # side under sdk//dist/: the branch head (candidate) and the - # newest release (baseline). Arm selection picks them up from there. + ######## INSTALL EVERY ARM OF THE COMPARISON ############# + # Named refs instead of the default release-vs-branch pair. Resolved + # here rather than in resolve-versions because the *-ref inputs there + # drive the functional matrix, and a benchmark wants to name its own + # arms without also changing what the rest of the workflow tests. + # + # Reference first, and input order preserved throughout: the tag order + # becomes configure-sdk's `heads` output, and conftest.py takes heads[0] + # as the otdfctl that provisions attributes and the KAS registry. That + # provisioning is not measured, and it should be the same build for + # every arm. + - name: Resolve the benchmark arms + id: bench-arms + if: needs.resolve-versions.outputs.bench-refs != '' + working-directory: otdftests/otdf-sdk-mgr + env: + SDK: ${{ matrix.sdk }} + BENCH_REFS: ${{ needs.resolve-versions.outputs.bench-refs }} + run: |- + read -r -a refs <<<"${BENCH_REFS//,/ }" + info=$(uv run --project . otdf-sdk-mgr versions resolve "$SDK" "${refs[@]}") + jq . <<<"$info" + err=$(jq -r '[.[] | select(.err != null) | .err] | join("; ")' <<<"$info") + if [[ -n "$err" ]]; then + echo "::error::Could not resolve benchmark arms: $err" + exit 1 + fi + # `versions resolve` drops a ref whose SHA it has already seen, so + # two names for one commit come back as one entry. Left alone that + # installs fewer builds than there are arms, fails arm selection in + # every cell, and spends the runner arriving at NOTHING MEASURED. + if [[ "$(jq 'length' <<<"$info")" -ne "${#refs[@]}" ]]; then + echo "::error::${BENCH_REFS} does not name ${#refs[@]} distinct commits -- two of these refs are the same build, so there is nothing to compare between them." + exit 1 + fi + { + echo "version-info=$(jq -c . <<<"$info")" + echo "refs-spec=$(jq -r --arg sdk "$SDK" '[.[] | "\($sdk)@\(.tag)"] | join(",")' <<<"$info")" + } >> "$GITHUB_OUTPUT" + + # The whole design rests on this step laying down every arm side by side + # under sdk//dist/: by default the branch head (candidate) and the + # newest release (baseline), or the refs resolved above. Arm selection + # picks them up from there. - name: Configure ${{ matrix.sdk }} sdk id: configure-sdk uses: ./otdftests/xtest/setup-cli-tool with: path: otdftests/xtest/sdk sdk: ${{ matrix.sdk }} - version-info: "${{ needs.resolve-versions.outputs[matrix.sdk] }}" + version-info: "${{ steps.bench-arms.outputs.version-info || needs.resolve-versions.outputs[matrix.sdk] }}" platform-otdfctl-dir: ${{ steps.platform-otdfctl.outputs.dir }} platform-otdfctl-sha: ${{ steps.platform-otdfctl.outputs.sha }} @@ -1016,7 +1193,7 @@ jobs: fi done env: - java_version_info: ${{ needs.resolve-versions.outputs.java }} + java_version_info: ${{ steps.bench-arms.outputs.version-info || needs.resolve-versions.outputs.java }} platform_ref: ${{ needs.resolve-versions.outputs.platform-main-sha }} - name: Build the ${{ matrix.sdk }} cli @@ -1049,10 +1226,20 @@ jobs: - name: Run performance benchmarks id: bench run: |- + # Empty unless the arms were named explicitly, in which case arm + # selection must not fall back to "newest release vs branch head": + # no named ref need be a release, and with several branch builds + # installed the default would pick the wrong pair or none. + arms=() + if [[ -n "$BENCH_REFS_SPEC" ]]; then + arms=(--bench-refs "$BENCH_REFS_SPEC") + fi uv run --frozen --no-build pytest -ra -v \ --bench \ --sdks "$BENCH_SDK" \ - --bench-budget-seconds 1500 \ + "${arms[@]}" \ + --bench-budget-seconds "$BENCH_BUDGET_SECONDS" \ + --bench-max-rounds "$BENCH_MAX_ROUNDS" \ --bench-out test-results/benchmarks \ --html "test-results/bench-${BENCH_SDK}.html" \ --self-contained-html \ @@ -1060,6 +1247,25 @@ jobs: working-directory: otdftests/xtest env: BENCH_SDK: ${{ matrix.sdk }} + BENCH_REFS_SPEC: ${{ steps.bench-arms.outputs.refs-spec }} + # Already defaulted (and scaled by arm count) in resolve-versions. + BENCH_BUDGET_SECONDS: ${{ needs.resolve-versions.outputs.bench-budget-seconds }} + BENCH_MAX_ROUNDS: ${{ inputs.bench-max-rounds || '60' }} + # Structured resolver output: original alias, immutable SHA, PR, + # release tag, and source/head classification. The report turns it + # into links instead of guessing provenance from a dist directory. + BENCH_VERSION_INFO: >- + ${{ steps.bench-arms.outputs.version-info + || needs.resolve-versions.outputs[matrix.sdk] }} + # Keep the names the caller asked for even when the resolver folds + # aliases at one SHA into a single install. That is how reporting can + # distinguish "main == latest" from a benchmark that broke before it + # found its second arm. + BENCH_REQUESTED_REFS: >- + ${{ needs.resolve-versions.outputs.bench-refs + || (matrix.sdk == 'go' && (inputs.otdfctl-ref || 'main latest')) + || (matrix.sdk == 'java' && (inputs.java-ref || 'main latest')) + || (inputs.js-ref || 'main latest') }} PLATFORM_DIR: "../../${{ steps.run-platform.outputs.platform-working-dir }}" SCHEMA_FILE: "manifest.schema.json" PLATFORM_TAG: main diff --git a/xtest/conftest.py b/xtest/conftest.py index c2b8d5c71..4a82b1dd0 100644 --- a/xtest/conftest.py +++ b/xtest/conftest.py @@ -27,6 +27,7 @@ import sizes import tdfs +from fixtures.bench import MAX_ARMS from otdfctl import OpentdfCommandLineTool from perf import report, stats from perf.cells import cells_for @@ -253,15 +254,24 @@ 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=f"builds to compare, comma- or space-separated, e.g. " + f"'go@main,go@my-branch'. The first is the reference: every gated " + f"contrast is taken against it, and the rest are ranked head-to-head " + f"as a bake-off. 2 to {MAX_ARMS} entries (the ceiling is how " + f"many builds setup-cli-tool can install side by side). Defaults to " + f"the newest installed release against the branch build", + ) group.addoption( "--bench-baseline", - help="build to compare against, e.g. go@v0.29.0; defaults to the newest " - "installed release of each sdk", + help="two-arm shorthand for the reference half of --bench-refs, e.g. " + "go@v0.29.0; must be given with --bench-candidate", ) group.addoption( "--bench-candidate", - help="build under test, e.g. go@main; defaults to the installed " - "unreleased build of each sdk", + help="two-arm shorthand for the candidate half of --bench-refs, e.g. " + "go@main; must be given with --bench-baseline", ) group.addoption( "--bench-threshold", @@ -293,8 +303,11 @@ 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. Defaults to " + "1500s scaled by (arms / 2), because a K-arm round costs K " + "invocations and holding the same precision costs proportionally " + "more time", ) group.addoption( "--bench-seed", @@ -415,7 +428,7 @@ def _parametrize_bench_cells(metafunc: pytest.Metafunc): return # --sdks may be version-qualified (go@main); benchmark arms come from - # --bench-baseline/--bench-candidate instead, so only the name matters. + # --bench-refs instead, so only the name matters here. specs = metafunc.config.getoption("--sdks") or " ".join( typing.get_args(tdfs.sdk_type) ) @@ -439,6 +452,11 @@ def pytest_configure(config: pytest.Config): "--bench cannot run under pytest-xdist: parallel workers compete " "for the CPU being measured. Drop -n / --dist." ) + # Resolve the arm specs now so a malformed --bench-refs is a usage error + # before anything is installed or measured, not an hour into the run. + from fixtures import bench + + bench.arm_specs_from_options(config) def _item_exercises_zip64_window(item: pytest.Item, session_sizes: list[str]) -> bool: @@ -521,12 +539,33 @@ def pytest_sessionfinish(session: pytest.Session, exitstatus: int): out_dir / f"{name}.json", recorder, bench_config, gate ) - summary = report.markdown(recorder, bench_config, gate) - report.append_step_summary(summary) + artifact_url = ( + report.ARTIFACT_URL_PLACEHOLDER + if os.environ.get("BENCH_DEFER_SUMMARY", "").lower() in {"1", "true", "yes"} + else "" + ) + summary = report.markdown(recorder, bench_config, gate, artifact_url=artifact_url) + report.write_markdown(out_dir / f"{name}.summary.md", summary) + if not artifact_url: + report.append_step_summary(summary) reporter = config.pluginmanager.get_plugin("terminalreporter") if reporter is not None: reporter.write_sep("=", "benchmark results") - reporter.write_line(gate.summary) + reporter.write_line( + str(recorder.metadata.get("comparison_note", gate.summary)) + if report.same_commit(recorder.metadata) + else gate.summary + ) + # Repeated on the terminal as well as in the step summary: "the run + # was too short for the number of arms you asked for" is the one + # finding a reader is most likely to mistake for a real result. + underpowered = report.underpowered_warning(recorder, bench_config, gate) + if underpowered: + reporter.write_line(underpowered) + for bake_off in report.bake_offs(recorder, bench_config, gate): + reporter.write_line( + f"{bake_off.cell_id} [{bake_off.metric}]: {bake_off.detail}" + ) reporter.write_line(f"raw samples and statistics: {json_path}") if config.getoption("--bench-no-gate", default=False): @@ -535,8 +574,12 @@ def pytest_sessionfinish(session: pytest.Session, exitstatus: int): # regression. --bench is an explicit request for a measurement; answering # it with a green tick and an empty table is the one outcome nobody # inspects, so a benchmark that has quietly stopped measuring can survive - # indefinitely. Every reason a cell skips is already in the report. - if gate.should_fail or gate.nothing_measured: + # indefinitely. The exception is two requested names resolving to one SHA: + # that is a complete, neutral answer (there is no code difference to test), + # not a harness that failed to measure an existing difference. + if gate.should_fail or ( + gate.nothing_measured and not report.same_commit(recorder.metadata) + ): session.exitstatus = pytest.ExitCode.TESTS_FAILED diff --git a/xtest/fixtures/bench.py b/xtest/fixtures/bench.py index d5194b380..bd2738959 100644 --- a/xtest/fixtures/bench.py +++ b/xtest/fixtures/bench.py @@ -4,14 +4,17 @@ 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). +(K arms and a config). """ from __future__ import annotations +import json import os import platform import random +import re +from collections.abc import Sequence from dataclasses import dataclass from pathlib import Path from typing import cast @@ -24,28 +27,59 @@ from perf.cells import PAYLOADS, BenchCell from perf.runner import Arm, BenchConfig, Budget, Invocation +#: Most builds one run can compare. The ceiling comes from +#: ``xtest/setup-cli-tool/action.yaml``, which installs into four fixed slots +#: (a/b/c/d) and refuses a fifth. Raising it here without raising it there +#: gives a run that resolves five refs and then measures four of them. +MAX_ARMS = 4 + class ArmSelectionError(Exception): - """The two builds a comparison needs are not both installed.""" + """The builds a comparison needs are not all installed.""" + + +def parse_refs(spec: str) -> tuple[str, ...]: + """Split a ``--bench-refs`` value into build specs, first = reference. + + Commas or whitespace, so both the shell-friendly + ``go@main,go@my-branch`` and a quoted space-separated list work. + + Raises: + ValueError: if the result is not between 2 and :data:`MAX_ARMS` + entries, or if a build is named twice. + """ + refs = tuple(p for p in re.split(r"[,\s]+", spec.strip()) if p) + if not 2 <= len(refs) <= MAX_ARMS: + raise ValueError( + f"need 2 to {MAX_ARMS} refs, got {len(refs)}: {spec!r}. The first " + "is the reference every gated contrast is taken against." + ) + if len(set(refs)) != len(refs): + # Two arms running the same build is what the A/A control cell is for, + # and it is added automatically. Asking for it here would spend a slot + # measuring a comparison the run already makes. + raise ValueError(f"duplicate refs in {spec!r}; each arm needs a distinct build") + return refs def select_arms( sdk: str, - *, - baseline_spec: str | None = None, - candidate_spec: str | None = None, -) -> tuple[tdfs.SDK, tdfs.SDK]: - """Pick (baseline, candidate) builds for one SDK. + specs: Sequence[str] | None = None, +) -> 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. + With no specs the run is the nightly two-arm comparison: the reference is + the newest installed final release and the candidate is the branch build + (``main``), which is exactly what the CI setup action lays down side by + side. Explicit specs name the arms instead, for reproducing a comparison, + pinning a specific release, or running a bake-off between several + candidates. 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: @@ -63,8 +97,11 @@ def resolve(spec: str, role: str) -> tdfs.SDK: ) return matches[0] - if candidate_spec: - candidate = resolve(candidate_spec, "candidate") + if specs: + arms = tuple( + resolve(spec, "reference" if i == 0 else f"arm {i + 1}") + for i, spec in enumerate(specs) + ) else: heads = [s for s in installed if not s.is_released()] if not heads: @@ -74,10 +111,6 @@ def resolve(spec: str, role: str) -> tdfs.SDK: ) # Prefer 'main' when several branch builds are present. candidate = next((s for s in heads if s.version == "main"), heads[0]) - - if baseline_spec: - baseline = resolve(baseline_spec, "baseline") - else: # Final releases only. A release candidate parses to the same semver # as its final release, so including them leaves `max` breaking a tie # on whatever order the directory listing happened to produce -- and a @@ -89,44 +122,103 @@ def resolve(spec: str, role: str) -> tdfs.SDK: f"not count); installed: " f"{', '.join(sorted(s.version for s in installed))}" ) - baseline = max(releases, key=lambda s: s.semver() or (0, 0, 0)) + arms = (max(releases, key=lambda s: s.semver() or (0, 0, 0)), candidate) - if baseline == candidate: - raise ArmSelectionError( - f"baseline and candidate are both {baseline}; nothing to compare" - ) - return baseline, candidate + 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 # --- Session-scoped configuration ------------------------------------------- +def arm_specs_from_options(config: pytest.Config) -> tuple[str, ...] | None: + """The run's build specs, reference first, or None for the default pair. + + ``--bench-refs`` is the K-arm form. ``--bench-baseline`` / + ``--bench-candidate`` are the two-arm shorthand it grew out of; they are + still accepted because the shape reads better for the common case, but + mixing the two forms is an error rather than a merge -- there is no + reading of ``--bench-refs a,b --bench-candidate c`` that is not a mistake. + """ + 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 and (baseline or candidate): + raise pytest.UsageError( + "--bench-refs cannot be combined with --bench-baseline or " + "--bench-candidate; --bench-refs supersedes both" + ) + if refs: + try: + return parse_refs(refs) + except ValueError as e: + raise pytest.UsageError(f"invalid --bench-refs: {e}") from e + if baseline and candidate: + return (baseline, candidate) + if baseline or candidate: + # Half a pair cannot be resolved: the unnamed side would fall back to + # a default chosen for a different question, and nothing in the report + # would say the comparison was not the one that was asked for. + raise pytest.UsageError( + "--bench-baseline and --bench-candidate must be given together" + ) + 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 here; the casts are for the type checker, which cannot see + the parser setup. """ def as_int(name: str) -> int: return int(cast(int, config.getoption(name))) - 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=( + float(budget) + if budget is not None + else default_budget_seconds(arm_count(config)) + ), seed=as_int("--bench-seed"), - threshold=as_float("--bench-threshold"), + threshold=float(cast(float, config.getoption("--bench-threshold"))), ) except ValueError as e: raise pytest.UsageError(f"invalid benchmark options: {e}") from e +def default_budget_seconds(n_arms: int) -> float: + """The default time allowance for a K-arm run. + + A round costs one invocation per arm, so at a fixed budget the attained + round count falls as ``2/K`` and every interval widens as ``sqrt(K/2)``. + Scaling the default by ``K/2`` keeps a three-arm run about as precise as + the two-arm run the number was chosen for, instead of quietly trading + precision for arms and reporting the difference as INCONCLUSIVE. + + An explicit ``--bench-budget-seconds`` is taken as given; someone who + named a number has already decided what they are willing to spend. + """ + # `BenchConfig` has slots, so the class attribute is a slot descriptor + # rather than the default; an instance is how you read one back. + return BenchConfig().budget_seconds * n_arms / 2 + + @pytest.fixture(scope="session") def bench_config(request: pytest.FixtureRequest) -> BenchConfig: """Round-loop and analysis settings, from the --bench-* options.""" @@ -135,7 +227,7 @@ def bench_config(request: pytest.FixtureRequest) -> BenchConfig: @pytest.fixture(scope="session") def bench_payloads(tmp_dir: Path, bench_config: BenchConfig) -> 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 @@ -186,52 +278,70 @@ 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 gated contrast is taken against.""" + return self.arms[0] + + @property + def candidates(self) -> tuple[tdfs.SDK, ...]: + """Everything else -- one arm in a regression run, more in a bake-off.""" + 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: - self._baseline_spec = baseline_spec - self._candidate_spec = candidate_spec + def __init__(self, specs: Sequence[str] | None) -> None: + self._specs = tuple(specs) if specs else None self._cache: dict[str, BenchArms] = {} 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)) ) - 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.""" - return ArmResolver( - cast(str | None, request.config.getoption("--bench-baseline")), - cast(str | None, request.config.getoption("--bench-candidate")), - ) + """Resolver for the arms of any SDK in the run, reference first.""" + return ArmResolver(arm_specs_from_options(request.config)) -def _spec_for(spec: str | None, sdk: str) -> str | None: - """Return ``spec`` only if it names this SDK, so one flag can cover a run.""" - if not spec: +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. Specs that name a *mix* of SDKs + are an error: the arms of a cell are all one SDK by construction, so there + is nothing a mixed list could mean. + """ + if not specs: return None - return spec if spec.split("@", 1)[0] == sdk else None + named = tuple(s for s in specs if s.split("@", 1)[0] == sdk) + if not named: + return None + if len(named) != len(specs): + raise ArmSelectionError( + f"benchmark refs name more than one SDK ({', '.join(specs)}); " + "the arms of a comparison must all be builds of the same SDK" + ) + return named #: 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, ...] = ( @@ -242,13 +352,25 @@ 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 arm is checked against the reference rather than only pairwise + neighbours: the reference is what all the gated contrasts are taken + against, so a candidate that disagrees with it invalidates its own gate + whatever the other candidates do. + """ 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) + (arm, arms.reference) + if not ref_has + else ( + arms.reference, + arm, + ) ) return ( f"{supporter} supports [{feature}] and {other} does not, so the " @@ -258,28 +380,25 @@ def comparability_problem(arms: BenchArms) -> str | 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. """ @@ -295,12 +414,13 @@ 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) if cached is not None: 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", @@ -336,28 +456,30 @@ def build_arms( ct_file: Path | None, tmp_dir: Path, attr_values: list[str], -) -> tuple[Arm, Arm]: - """Turn a cell plus its two builds into two ready-to-run invocations. +) -> tuple[Arm, ...]: + """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. - - 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. - - In a control cell both arms are the baseline build, so the pair differs - only in the output path -- exactly the harness overhead the A/A cell - exists to measure. + 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, 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 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. It is built with as many arms as the real cells have rather than + a cheap pair, because in a K-arm round the last arm runs K-1 invocations + after the first and carries more drift than an adjacent pair does; a + two-arm control would understate the noise of the contrasts being judged. """ - baseline_sdk = arms.baseline - candidate_sdk = arms.baseline if cell.control else arms.candidate target_mode = pinned_target_mode(arms) - def invocation(sdk: tdfs.SDK, role: str) -> Invocation: - out = tmp_dir / f"bench-{cell.id}-{role}" + def invocation(sdk: tdfs.SDK, arm_id: str) -> Invocation: + out = tmp_dir / f"bench-{cell.id}-{arm_id}" if cell.operation == "encrypt": out = out.with_suffix(".tdf") argv, env = sdk.encrypt_command( @@ -374,9 +496,17 @@ def invocation(sdk: tdfs.SDK, role: str) -> Invocation: argv, env = sdk.decrypt_command(ct_file, out, container="ztdf") return Invocation(argv, env, out) - return ( - Arm("baseline", str(baseline_sdk), invocation(baseline_sdk, "baseline")), - Arm("candidate", str(candidate_sdk), invocation(candidate_sdk, "candidate")), + if cell.control: + # 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.reference.version}#{i + 1}", arms.reference) + for i in range(len(arms.arms)) + ] + else: + 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 ) @@ -387,7 +517,8 @@ def runner_metadata(config: pytest.Config) -> dict[str, object]: here feeds the decision rule. It is recorded so that a human reading an old artifact can tell what they are looking at. """ - return { + metadata: dict[str, object] = { + "sdk": os.environ.get("BENCH_SDK", ""), "python": platform.python_version(), "platform": platform.platform(), "processor": platform.processor() or "unknown", @@ -395,9 +526,85 @@ def runner_metadata(config: pytest.Config) -> dict[str, object]: "runner_os": os.environ.get("RUNNER_OS", ""), "runner_arch": os.environ.get("RUNNER_ARCH", ""), "github_run_id": os.environ.get("GITHUB_RUN_ID", ""), + "github_run_url": _github_run_url(), "platform_version": _platform_version(), "seed": config.getoption("--bench-seed"), } + sources, warning = _arm_sources() + metadata["arm_sources"] = sources + requested = _requested_refs() + metadata["requested_refs"] = requested + if len(requested) >= 2 and len(sources) == 1 and sources[0].get("sha"): + sha = str(sources[0].get("sha", "")) + names = ", ".join(requested) + metadata["comparison_status"] = "same_commit" + metadata["comparison_note"] = ( + f"{names} resolve to the same commit" + f"{f' {sha[:7]}' if sha else ''}; there is no code difference to benchmark." + ) + if warning: + metadata["arm_sources_warning"] = warning + return metadata + + +def _github_run_url() -> str: + server = os.environ.get("GITHUB_SERVER_URL", "") + repository = os.environ.get("GITHUB_REPOSITORY", "") + run_id = os.environ.get("GITHUB_RUN_ID", "") + if not (server and repository and run_id): + return "" + return f"{server}/{repository}/actions/runs/{run_id}" + + +def _requested_refs() -> list[str]: + """The caller's names, retained even when resolution deduplicates a SHA.""" + value = os.environ.get("BENCH_REQUESTED_REFS", "") + return [part for part in re.split(r"[,\s]+", value.strip()) if part] + + +def _arm_sources() -> tuple[list[dict[str, object]], str]: + """Resolver metadata for the builds, enriched with their GitHub repository. + + CI already paid to resolve every ref to an immutable SHA before installing + it. Carry that result into the benchmark rather than trying to infer a PR, + release, or branch from the flattened dist-directory name afterward. + """ + raw = os.environ.get("BENCH_VERSION_INFO", "").strip() + if not raw: + return [], "" + try: + parsed = json.loads(raw) + except json.JSONDecodeError as e: + return [], f"BENCH_VERSION_INFO was not valid JSON: {e}" + if not isinstance(parsed, list) or not all(isinstance(v, dict) for v in parsed): + return [], "BENCH_VERSION_INFO must be a JSON array of objects" + + sources: list[dict[str, object]] = [] + for value in parsed: + source = {str(k): v for k, v in value.items()} + source["repo_url"] = _repo_url_for(source) + sources.append(source) + return sources, "" + + +def _repo_url_for(source: dict[str, object]) -> str: + sdk = str(source.get("sdk", "")) + if sdk == "java": + return "https://github.com/opentdf/java-sdk" + if sdk == "js": + return "https://github.com/opentdf/web-sdk" + if sdk != "go": + return "" + + # otdfctl moved into the platform monorepo at v0.31.0. Resolver results + # from there use the namespaced release tag; old standalone releases do + # not. Branch/PR/SHA builds resolve against platform first. + release = str(source.get("release", "")) + if release and not release.startswith("otdfctl/"): + match = re.search(r"v?(\d+)\.(\d+)\.(\d+)", release) + if match and tuple(map(int, match.groups())) < (0, 31, 0): + return "https://github.com/opentdf/otdfctl" + return "https://github.com/opentdf/platform" def _platform_version() -> str: diff --git a/xtest/perf/report.py b/xtest/perf/report.py index a100044ae..6021bc414 100644 --- a/xtest/perf/report.py +++ b/xtest/perf/report.py @@ -21,15 +21,18 @@ import json import math import os +import statistics +from collections.abc import Iterable, Mapping from dataclasses import dataclass, field from pathlib import Path +from urllib.parse import quote import pytest from perf import stats from perf.cells import BenchCell from perf.measure import METRIC_LABELS, METRICS, format_metric -from perf.runner import BenchConfig, CellResult, analyze +from perf.runner import BenchConfig, CellResult, analyze, contrast_key #: Cells the session intends to run. Set by the conftest parametrizer, read by #: the budget and arm-resolution fixtures. @@ -38,6 +41,10 @@ #: The session's recorder, reachable from both fixtures and session hooks. RECORDER_KEY: pytest.StashKey[BenchmarkRecorder] = pytest.StashKey() +# Replaced by the workflow after upload-artifact returns its authenticated URL. +# Kept conspicuous so a failed substitution cannot look like a real link. +ARTIFACT_URL_PLACEHOLDER = "@@BENCH_ARTIFACT_URL@@" + def recorder_for(config: pytest.Config) -> BenchmarkRecorder: """Return the session's recorder, creating it on first use.""" @@ -69,12 +76,145 @@ def gate(self, config: BenchConfig) -> stats.GateResult: return analyze(self.results, config) +@dataclass(frozen=True, slots=True) +class BakeOff: + """One cell's head-to-head ranking of the non-reference arms. + + The gate answers "did anything get slower than the reference". A bake-off + answers a different question -- "of these candidate implementations, which + should we merge" -- and it is deliberately kept out of the gate: ranking + two candidates against each other says nothing about whether either is a + regression, and a build must not go red because the runner-up lost. + """ + + cell_id: str + metric: str + #: Candidate arm ids, best first, ordered by ratio against the reference. + order: list[str] + #: The contrast between the top two candidates, as configured order. + head_to_head: str + verdict: stats.Verdict + #: The winning arm id, or None when the top two could not be separated. + winner: str | None + detail: str + + +@dataclass(frozen=True, slots=True) +class ReportRow: + """One reportable contrast/metric with enough context to render it.""" + + result: CellResult + a: str + b: str + metric: str + comparison: stats.PairedComparison + gated: bool + head_to_head: bool + + +def bake_offs( + recorder: BenchmarkRecorder, config: BenchConfig, gate: stats.GateResult +) -> list[BakeOff]: + """Rank the candidates in every cell that ran more than one of them. + + Empty for a two-arm run, which has nothing to rank: there is one candidate + and the gate has already said everything there is to say about it. + + A winner is named only when the top pair's own contrast came back FASTER + or SLOWER. A TIED top pair reports that the two are indistinguishable, + which is a real answer and frequently the correct one -- picking the arm + whose point estimate happened to land lower would be reading noise as a + result. + """ + out: list[BakeOff] = [] + for r in recorder.results: + candidates = [a for a in r.arm_ids if a != r.reference] + if r.control or len(candidates) < 2: + continue + for metric in config.gated_metrics: + ranked = _rank_candidates(r, candidates, metric, gate) + if len(ranked) < 2: + continue + # Head-to-head keys exist in configured order only, so recover + # that order for the top two rather than assuming the ranking's. + top = [a for a in candidates if a in ranked[:2]] + key = contrast_key(r.cell_id, top[0], top[1], metric) + c = gate.comparisons.get(key) + if c is None: + continue + winner = { + stats.Verdict.FASTER: top[1], + stats.Verdict.SLOWER: top[0], + }.get(c.verdict) + out.append( + BakeOff( + cell_id=r.cell_id, + metric=metric, + order=ranked, + head_to_head=f"{top[1]}_vs_{top[0]}", + verdict=c.verdict, + winner=winner, + detail=_bake_off_detail(c, top, winner), + ) + ) + return out + + +def _rank_candidates( + r: CellResult, candidates: list[str], metric: str, gate: stats.GateResult +) -> list[str]: + """Candidate ids ordered by their ratio against the reference, best first. + + Candidates whose reference contrast produced no usable ratio are dropped: + an unmeasurable arm has no place in a ranking, and sorting NaN would put + it wherever the sort happened to leave it. + """ + ratios: dict[str, float] = {} + for arm in candidates: + c = gate.comparisons.get(contrast_key(r.cell_id, r.reference, arm, metric)) + if c is not None and math.isfinite(c.ratio): + ratios[arm] = c.ratio + return sorted(ratios, key=lambda a: ratios[a]) + + +def _bake_off_detail( + c: stats.PairedComparison, top: list[str], winner: str | None +) -> str: + interval = ( + f"{c.ratio:.3f}x [{c.ci_low:.3f}, {c.ci_high:.3f}]" + if math.isfinite(c.ci_low) and math.isfinite(c.ci_high) + else "no usable interval" + ) + pair = f"`{top[1]}` vs `{top[0]}` {interval}" + if winner is not None: + return f"`{winner}` wins: {pair}" + if c.verdict is stats.Verdict.TIED: + return f"no measurable difference between `{top[0]}` and `{top[1]}`: {pair}" + return f"cannot separate `{top[0]}` and `{top[1]}`: {pair}" + + +def _bake_off_dict(b: BakeOff) -> dict[str, object]: + return { + "cell": b.cell_id, + "metric": b.metric, + "order": b.order, + "head_to_head": b.head_to_head, + "verdict": str(b.verdict), + "winner": b.winner, + "detail": b.detail, + } + + def to_dict( recorder: BenchmarkRecorder, config: BenchConfig, gate: stats.GateResult ) -> dict[str, object]: """Serialize a whole run, raw samples included.""" return { - "schema": 1, + # 2: cells hold K arms. `samples` is keyed by arm id rather than by + # `"baseline"`/`"candidate"`, and per-metric statistics moved under + # `contrasts["_vs_"]`. The `baseline`/`candidate` labels stay for + # a two-arm run so existing readers keep working. + "schema": 2, "metadata": recorder.metadata, "config": { "min_rounds": config.min_rounds, @@ -94,14 +234,23 @@ def to_dict( "noise_floor_by_control": { k: _noise_dict(n) for k, n in gate.noise_by_control.items() }, + "nothing_measured": gate.nothing_measured, "trustworthy": gate.trustworthy, "regressions": gate.regressions, "improvements": gate.improvements, + "ranked": gate.ranked, + "bake_off": [_bake_off_dict(b) for b in bake_offs(recorder, config, gate)], "summary": gate.summary, "skipped": recorder.skipped, "cells": [ { "id": r.cell_id, + "arms": list(r.arm_ids), + "arm_labels": r.arm_labels, + "reference": r.reference, + # Kept for two-arm readers that predate the K-arm schema; at + # K > 2 `arms`/`arm_labels` are the complete picture and these + # name only the first candidate. "baseline": r.baseline_label, "candidate": r.candidate_label, "control": r.control, @@ -111,10 +260,13 @@ def to_dict( "stopped_because": r.stopped_because, "rss_floor_bytes": r.rss_floor_bytes, "samples": r.samples, - "metrics": { - m: _comparison_dict(gate.comparisons[f"{r.cell_id}/{m}"]) - for m in METRICS - if f"{r.cell_id}/{m}" in gate.comparisons + "contrasts": { + f"{b}_vs_{a}": { + m: _comparison_dict(gate.comparisons[key]) + for m in METRICS + if (key := contrast_key(r.cell_id, a, b, m)) in gate.comparisons + } + for a, b in r.contrast_pairs() }, } for r in recorder.results @@ -142,8 +294,13 @@ def _comparison_dict(c: stats.PairedComparison) -> dict[str, object]: "ratio": _jsonable(c.ratio), "ci_low": _jsonable(c.ci_low), "ci_high": _jsonable(c.ci_high), + # `p_value`/`p_adjusted` retain their historical meaning: the + # one-sided "b is slower than a" tail. The faster tail is separate so + # consumers never have to reverse an adjusted upper-tail probability. "p_value": _jsonable(c.p_value), "p_adjusted": _jsonable(c.p_adjusted), + "p_value_faster": _jsonable(c.p_value_faster), + "p_adjusted_faster": _jsonable(c.p_adjusted_faster), "verdict": str(c.verdict), "note": c.note, } @@ -167,63 +324,765 @@ def write_json( return path -def markdown( +def write_markdown(path: Path, text: str) -> Path: + """Write a summary template for CI to publish after artifact upload.""" + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text) + return path + + +def underpowered_warning( recorder: BenchmarkRecorder, config: BenchConfig, gate: stats.GateResult +) -> str | None: + """Say so when the run did not buy the precision it was asked for. + + A K-arm round costs K invocations, so at a fixed time budget the round + count falls as arms are added and every interval widens by roughly + ``sqrt(K/2)``. Asking for three arms on a two-arm budget therefore comes + back as a wall of INCONCLUSIVE after burning the whole runner, with + nothing in the output saying that more time was the missing ingredient. + This says it, and says how much more. + + Returns None when every contrast reached the precision target. + """ + widest = 0.0 + rounds = 0 + for r in recorder.results: + if r.control: + continue + for a, b in r.contrast_pairs(): + for metric in config.gated_metrics: + c = gate.comparisons.get(contrast_key(r.cell_id, a, b, metric)) + if c is None or not math.isfinite(c.ci_half_width_log): + continue + if c.ci_half_width_log > widest: + widest, rounds = c.ci_half_width_log, c.n_rounds + + target = config.target_half_width_log + if widest <= target or target <= 0: + return None + + n_arms = max((len(r.arm_ids) for r in recorder.results), default=2) + # Interval width falls as 1/sqrt(n), so closing a factor-f gap costs f^2 + # times the rounds -- and, at a fixed per-round cost, f^2 times the budget. + shortfall = (widest / target) ** 2 + return ( + f"Underpowered: the widest contrast reached " + f"+/-{(math.exp(widest) - 1) * 100:.1f}% after {rounds} rounds, against " + f"a +/-{(math.exp(target) - 1) * 100:.1f}% target. A {n_arms}-arm round " + f"costs {n_arms} invocations; holding precision needs about " + f"{shortfall:.1f}x the rounds, so roughly " + f"{config.budget_seconds * shortfall:.0f}s of budget (currently " + f"{config.budget_seconds:.0f}s) and a max-rounds ceiling above " + f"{math.ceil(rounds * shortfall)}. Contrasts the interval could not " + f"separate are reported INCONCLUSIVE rather than as no difference." + ) + + +def markdown( + recorder: BenchmarkRecorder, + config: BenchConfig, + gate: stats.GateResult, + *, + artifact_url: str = "", ) -> str: - """Render the run as a GitHub step summary.""" - threshold_pct = (config.threshold - 1) * 100 + """Render a decision-first GitHub job summary with progressive disclosure.""" + rows = _report_rows(recorder, config, gate) + gated_rows = [r for r in rows if r.gated] + attention = [ + r + for r in gated_rows + if r.comparison.verdict + in ( + stats.Verdict.REGRESSION, + stats.Verdict.INCONCLUSIVE, + stats.Verdict.IMPROVED, + ) + ] + attention.sort(key=_attention_sort_key) + sdk = next( + (r.sdk for r in recorder.results if r.sdk), + str(recorder.metadata.get("sdk") or "SDK"), + ) + equivalent = same_commit(recorder.metadata) + status, headline = _headline(gate, gated_rows, recorder.metadata) + n_arms = max((len(r.arm_ids) for r in recorder.results), default=2) + summary = ( + str(recorder.metadata.get("comparison_note", gate.summary)) + if equivalent + else gate.summary + ) + lines = [ - "## SDK performance regression benchmark", + f"## {sdk.upper()} SDK performance — {status}", "", - f"Paired A/B on one runner. A cell fails only if the 95% CI lower " - f"bound exceeds **{config.threshold:.2f}x** (+{threshold_pct:.0f}%) " - f"*and* the BH-adjusted p < {stats.DEFAULT_ALPHA}.", + "### TL;DR", "", - f"**{gate.summary}**", + f"> **{headline}**", + ">", + f"> {summary}", "", ] + lines += _quick_links(recorder.metadata, artifact_url) + lines += _provenance(recorder) + warning = underpowered_warning(recorder, config, gate) + if warning: + lines += ["> [!WARNING]", f"> {warning}", ""] noise = gate.noise - if noise is not None and noise.detail: + if noise is not None and noise.detail and not equivalent: lines += [f"> {noise.detail}", ""] - elif noise is not None and math.isfinite(noise.width_ratio): + + lines += ["### What changed", ""] + if equivalent: + lines += [ + "No performance comparison was necessary: the requested refs identify " + "the same source commit.", + "", + ] + elif attention: + lines += [ + "Only gated regressions, unresolved measurements, and confirmed " + "improvements are shown here. Clean rows and diagnostic metrics are below.", + "", + "| measurement | contrast | observed cost | change (95% CI) | gate margin | result |", + "| --- | --- | --- | --- | ---: | --- |", + ] + for row in attention: + c = row.comparison + lines.append( + f"| {_cell_label(row.result)} · {METRIC_LABELS[row.metric][0]} " + f"| `{row.b}` vs `{row.a}` | {_cost_change(row.metric, c)} " + f"| {_percent_change(c)} | {_gate_margin(c, config.threshold)} " + f"| {_verdict_cell(c)} |" + ) + lines += _effect_overview(attention, config.threshold) + elif gate.nothing_measured: lines += [ - f"A/A noise floor: +/-{(noise.width_ratio - 1) * 100:.1f}% " - f"(the smallest effect this run could resolve).", + "No comparison ran. Expand **Not measured** below for the cell-level " + "reasons; this is not a passing performance result.", + "", + ] + else: + lines += [ + "No gated comparison requires attention: every measured wall-clock and " + "RSS contrast passed with enough precision.", "", ] - lines += [ - "| cell | metric | baseline | candidate | ratio (95% CI) | p (BH) | n | verdict |", - "| --- | --- | --- | --- | --- | --- | --- | --- |", - ] + lines += _bake_off_section(recorder, config, gate) + lines += _diagnostics(recorder, attention, config) + lines += _full_measurements(rows, recorder.metadata) + lines += _not_measured(recorder) + if not equivalent: + lines += _method(config, n_arms) + lines += _run_facts(recorder, config, gate, artifact_url) + return "\n".join(lines) + "\n" + + +def _report_rows( + recorder: BenchmarkRecorder, config: BenchConfig, gate: stats.GateResult +) -> list[ReportRow]: + rows: list[ReportRow] = [] for result in recorder.results: - for metric in METRICS: - key = f"{result.cell_id}/{metric}" - c = gate.comparisons.get(key) - if c is None: - continue - gated = metric in config.gated_metrics and not result.control - label = METRIC_LABELS[metric][0] + ("" if gated else " (ungated)") + for a, b in result.contrast_pairs(): + head_to_head = result.reference not in (a, b) + for metric in METRICS: + c = gate.comparisons.get(contrast_key(result.cell_id, a, b, metric)) + if c is None: + continue + rows.append( + ReportRow( + result=result, + a=a, + b=b, + metric=metric, + comparison=c, + gated=( + metric in config.gated_metrics + and not result.control + and not head_to_head + ), + head_to_head=head_to_head, + ) + ) + return rows + + +def same_commit(metadata: Mapping[str, object]) -> bool: + """Whether multiple requested names resolved to one immutable commit.""" + return metadata.get("comparison_status") == "same_commit" + + +def _headline( + gate: stats.GateResult, + gated_rows: list[ReportRow], + metadata: Mapping[str, object], +) -> tuple[str, str]: + inconclusive = sum( + r.comparison.verdict is stats.Verdict.INCONCLUSIVE for r in gated_rows + ) + improvements = sum( + r.comparison.verdict is stats.Verdict.IMPROVED for r in gated_rows + ) + if same_commit(metadata): + return "SAME COMMIT", "No code difference to benchmark." + if gate.nothing_measured: + return "NOTHING MEASURED", "The benchmark produced no comparisons." + if not gate.trustworthy: + return ( + "UNTRUSTWORTHY", + "The A/A control detected bias; results are visible but cannot fail the build.", + ) + if gate.regressions: + return ( + "REGRESSION", + f"{len(gate.regressions)} confirmed regression(s); " + f"{inconclusive} unresolved and {improvements} improved gated comparison(s).", + ) + if inconclusive: + return ( + "INCONCLUSIVE", + f"No confirmed regressions, but {inconclusive} gated comparison(s) " + "could not be resolved.", + ) + return ( + "PASS", + f"No confirmed regressions; {improvements} gated comparison(s) improved.", + ) + + +def _quick_links(metadata: Mapping[str, object], artifact_url: str) -> list[str]: + links: list[str] = [] + if artifact_url: + links.append(f"[Download raw samples and HTML report]({artifact_url})") + run_url = str(metadata.get("github_run_url", "")) + if run_url: + links.append(f"[Workflow run]({run_url})") + return [" · ".join(links), ""] if links else [] + + +def _provenance(recorder: BenchmarkRecorder) -> list[str]: + result = next((r for r in recorder.results if not r.control), None) + if result is None: + result = next(iter(recorder.results), None) + sources = _sources_by_tag(recorder.metadata) + lines = [ + "### Compared builds", + "", + "| arm | role | source | commit | compare to reference |", + "| --- | --- | --- | --- | --- |", + ] + if result is None: + if not (same_commit(recorder.metadata) and sources): + return [] + source = next(iter(sources.values())) + requested = recorder.metadata.get("requested_refs", []) + arms = [str(arm) for arm in requested] if isinstance(requested, list) else [] + for index, arm in enumerate(arms): + resolution = ( + _source_link(source, arm) if index == 0 else "same resolved source" + ) lines.append( - f"| {result.cell_id} | {label} " - f"| {format_metric(metric, c.baseline_median)} " - f"| {format_metric(metric, c.candidate_median)} " - f"| {_ratio_cell(c)} | {_p_cell(c)} | {c.n_rounds} " - f"| {_verdict_cell(c)} |" + f"| {_linked_arm(arm, sources, fallback_source=source)} " + f"| **same commit** | {resolution} | {_commit_link(source)} | — |" ) + return lines + [""] + + reference_source = sources.get(result.reference) + for arm in result.arm_ids: + source = sources.get(arm) + role_name = _arm_role(result, arm) + role = f"**{role_name}**" if arm == result.reference else role_name + label = result.arm_labels.get(arm, arm) + source_link = _source_link(source, label) + commit_link = _commit_link(source) + compare_link = ( + "—" if arm == result.reference else _compare_link(reference_source, source) + ) + lines.append( + f"| {_linked_arm(arm, sources)} | {role} | {source_link} " + f"| {commit_link} | {compare_link} |" + ) + lines.append("") + warning = recorder.metadata.get("arm_sources_warning") + if warning: + lines += [f"> Provenance unavailable: {_md(str(warning))}", ""] + return lines + + +def _sources_by_tag(metadata: Mapping[str, object]) -> dict[str, dict[str, object]]: + raw_sources = metadata.get("arm_sources", []) + if not isinstance(raw_sources, list): + return {} + sources: dict[str, dict[str, object]] = {} + for source in raw_sources: + if not isinstance(source, dict) or not source.get("tag"): + continue + normalized = {str(key): value for key, value in source.items()} + sources[str(normalized["tag"])] = normalized + return sources + + +def _source_url(source: object) -> str: + if not isinstance(source, dict): + return "" + repo = str(source.get("repo_url", "")) + pr = str(source.get("pr", "")) + release = str(source.get("release", "")) + sha = str(source.get("sha", "")) + if repo and pr: + return f"{repo}/pull/{quote(pr, safe='')}" + if repo and release: + return f"{repo}/releases/tag/{quote(release, safe='')}" + if repo and sha: + return f"{repo}/tree/{quote(sha, safe='')}" + return "" + + +def _linked_arm( + arm: str, + sources: Mapping[str, object], + *, + fallback_source: object = None, +) -> str: + base, separator, copy = arm.partition("#") + label = f"{base} copy {copy}" if separator else arm + source = sources.get(base, fallback_source) + url = _source_url(source) + code = f"`{_md(label)}`" + return f"[{code}]({url})" if url else code + - if recorder.skipped: - lines += ["", "### Not measured", ""] - lines += [f"- `{cid}`: {why}" for cid, why in sorted(recorder.skipped.items())] +def _source_link(source: object, fallback: str) -> str: + if not isinstance(source, dict): + return _md(fallback) + repo = str(source.get("repo_url", "")) + tag = str(source.get("tag", fallback)) + alias = str(source.get("alias", tag)) + pr = str(source.get("pr", "")) + release = str(source.get("release", "")) + sha = str(source.get("sha", "")) + if repo and pr: + return f"[PR #{_md(pr)}]({repo}/pull/{quote(pr, safe='')}) · `{_md(tag)}`" + if repo and release: + return f"[{_md(tag)}]({repo}/releases/tag/{quote(release, safe='')}) · release" + if repo and sha: + kind = "branch" if source.get("head") else "commit" + return f"[{_md(alias)}]({repo}/tree/{quote(sha, safe='')}) · {kind}" + return _md(fallback) + +def _commit_link(source: object) -> str: + if not isinstance(source, dict): + return "—" + repo = str(source.get("repo_url", "")) + sha = str(source.get("sha", "")) + if not (repo and sha): + return "—" + return f"[`{_md(sha[:7])}`]({repo}/commit/{quote(sha, safe='')})" + + +def _compare_link(reference: object, candidate: object) -> str: + if not (isinstance(reference, dict) and isinstance(candidate, dict)): + return "—" + repo = str(reference.get("repo_url", "")) + if not repo or repo != str(candidate.get("repo_url", "")): + return "—" + a, b = str(reference.get("sha", "")), str(candidate.get("sha", "")) + if not (a and b): + return "—" + return f"[diff]({repo}/compare/{quote(a, safe='')}...{quote(b, safe='')})" + + +def _md(value: str) -> str: + return value.replace("|", "\\|").replace("\n", " ") + + +def _attention_sort_key(row: ReportRow) -> tuple[int, float, str, int]: + order = { + stats.Verdict.REGRESSION: 0, + stats.Verdict.INCONCLUSIVE: 1, + stats.Verdict.IMPROVED: 2, + } + ratio = row.comparison.ratio + magnitude = abs(math.log(ratio)) if math.isfinite(ratio) and ratio > 0 else 0 + return ( + order.get(row.comparison.verdict, 3), + -round(magnitude, 3), + row.result.cell_id, + METRICS.index(row.metric), + ) + + +def _cell_label(result: CellResult) -> str: + return result.cell_id.removeprefix(f"{result.sdk}-").replace("-", " / ") + + +def _arm_role(result: CellResult, arm: str) -> str: + """A short, stable role that maps plot rows back to the build table.""" + if arm == result.reference: + return "reference" + candidates = [ + candidate for candidate in result.arm_ids if candidate != result.reference + ] + try: + return f"candidate {chr(ord('A') + candidates.index(arm))}" + except ValueError: + return arm + + +def _cost_change(metric: str, c: stats.PairedComparison) -> str: + if not (math.isfinite(c.baseline_median) and math.isfinite(c.candidate_median)): + return "—" + delta = c.candidate_median - c.baseline_median + sign = "+" if delta >= 0 else "−" + return ( + f"{format_metric(metric, c.baseline_median)} → " + f"{format_metric(metric, c.candidate_median)} " + f"({sign}{format_metric(metric, abs(delta))})" + ) + + +def _pct(ratio: float) -> str: + if not math.isfinite(ratio): + return "—" + return f"{(ratio - 1) * 100:+.1f}%" + + +def _percent_change(c: stats.PairedComparison) -> str: + point = _pct(c.ratio) + if not (math.isfinite(c.ci_low) and math.isfinite(c.ci_high)): + return point + return f"{point} [{_pct(c.ci_low)}, {_pct(c.ci_high)}]" + + +def _gate_margin(c: stats.PairedComparison, threshold: float) -> str: + if c.verdict is stats.Verdict.REGRESSION and math.isfinite(c.ci_low): + return f"+{(c.ci_low - threshold) * 100:.1f} pp" + if c.verdict is stats.Verdict.IMPROVED and math.isfinite(c.ci_high): + return f"+{(1 / threshold - c.ci_high) * 100:.1f} pp" + return "—" + + +def _effect_overview(rows: list[ReportRow], threshold: float) -> list[str]: + labels = [f"{_cell_label(r.result)} {METRIC_LABELS[r.metric][0]}" for r in rows] + roles = [_arm_role(row.result, row.b) for row in rows] + label_width = min(30, max(map(len, labels), default=0)) + role_width = max(map(len, roles), default=0) + width = 57 + threshold_log = math.log(threshold) + observed_logs = [ + abs(math.log(value)) + for row in rows + for value in ( + row.comparison.ratio, + row.comparison.ci_low, + row.comparison.ci_high, + ) + if math.isfinite(value) and value > 0 + ] + # Preserve room around the practical band, but expand far enough that the + # largest interval gets brackets rather than an off-scale arrow. The tail + # transform in `_effect_strip` prevents an epic result from squeezing the + # gate markers and every merely-large result into the center character. + span = max(3 * threshold_log, max(observed_logs, default=0) * 1.05) + axis = [" "] * width + for text, start in ( + ("← faster", 0), + ("no change", (width - len("no change")) // 2), + ("slower →", width - len("slower →")), + ): + axis[start : start + len(text)] = text + lines = [ + "", + "#### Effect at a glance", + "", + "Shared tail-compressed log-ratio scale; `┆` marks ±the practical " + "threshold and `│` no change. Candidate letters match the build table; " + "exact changes are printed at right, and `◆` means the CI is narrower " + "than one character.", + "", + "```text", + f"{'arm':<{role_width}} {'measurement':<{label_width}} {''.join(axis)}", + ] + for role, label, row in zip(roles, labels, rows, strict=True): + lines.append( + f"{role:<{role_width}} {label[:label_width]:<{label_width}} " + f"{_effect_strip(row.comparison, threshold, width=width, span=span)} " + f"{_pct(row.comparison.ratio):>7} {row.comparison.verdict}" + ) + lines += ["```", ""] + return lines + + +def _effect_strip( + c: stats.PairedComparison, + threshold: float, + *, + width: int = 57, + span: float | None = None, +) -> str: + """A fixed-width CI forest strip on a symmetric compressed-log scale.""" + chars = [" "] * width + threshold_log = math.log(threshold) + span = span or 3 * threshold_log + + def unit(value: float) -> float: + """Keep the gate at 1/3 width and compress the dynamic tails.""" + magnitude = abs(value) + if magnitude <= threshold_log: + scaled = magnitude / threshold_log / 3 + else: + tail = max(span - threshold_log, 1e-12) + # Keep ten percent of either edge as breathing room: the largest + # observed effect should look extreme without masquerading as a + # clipped value. + scaled = 1 / 3 + (0.9 - 1 / 3) * ( + math.log1p((magnitude - threshold_log) / threshold_log) + / math.log1p(tail / threshold_log) + ) + return math.copysign(max(-1.0, min(1.0, scaled)), value) + + def pos(ratio: float) -> int: + value = math.log(ratio) if math.isfinite(ratio) and ratio > 0 else 0.0 + return round((unit(value) + 1) * (width - 1) / 2) + + for ratio, marker in ((1 / threshold, "┆"), (1.0, "│"), (threshold, "┆")): + chars[pos(ratio)] = marker + collapsed_ci_at: int | None = None + if math.isfinite(c.ci_low) and math.isfinite(c.ci_high): + lo, hi = sorted((pos(c.ci_low), pos(c.ci_high))) + if lo == hi: + collapsed_ci_at = lo + chars[lo] = "◆" + else: + for i in range(lo, hi + 1): + if chars[i] == " ": + chars[i] = "━" + chars[lo], chars[hi] = "[", "]" + if math.isfinite(c.ratio) and c.ratio > 0: + point = pos(c.ratio) + chars[point] = "◆" if point == collapsed_ci_at else "●" + if math.log(c.ratio) < -span: + chars[0] = "◀" + elif math.log(c.ratio) > span: + chars[-1] = "▶" + return "".join(chars) + + +def _bake_off_section( + recorder: BenchmarkRecorder, config: BenchConfig, gate: stats.GateResult +) -> list[str]: + rankings = bake_offs(recorder, config, gate) + if not rankings: + return [] + lines = [ + "### Bake-off", + "", + "Candidate-to-candidate ranking only; these contrasts never fail the build.", + "", + ] + for bo in rankings: + order = " < ".join(f"`{a}`" for a in bo.order) + lines.append( + f"- **{_cell_label(next(r for r in recorder.results if r.cell_id == bo.cell_id))}** " + f"({METRIC_LABELS[bo.metric][0]}): {order} — {bo.detail}" + ) + return lines + [""] + + +def _diagnostics( + recorder: BenchmarkRecorder, + attention: list[ReportRow], + config: BenchConfig, +) -> list[str]: + if not attention: + return [] + lines = [ + "
", + "Round stability for attention rows", + "", + "Each Braille glyph carries two rounds at four vertical levels. The scale " + "is centered on 1.0 and is never tighter than the practical threshold.", + "", + "```text", + ] + for row in attention: + values = _paired_ratios(row) + if not values: + continue + slower = sum(v > 1 for v in values) + label = f"{_cell_label(row.result)} {METRIC_LABELS[row.metric][0]}" + lines.append( + f"{label[:28]:<28} {_braille_sparkline(values, config.threshold):<24} " + f"{slower}/{len(values)} rounds slower; median {_pct(statistics.median(values))}" + ) + lines += ["```", "", "
", ""] + return lines + + +def _paired_ratios(row: ReportRow) -> list[float]: + baseline = row.result.samples.get(row.a, {}).get(row.metric, []) + candidate = row.result.samples.get(row.b, {}).get(row.metric, []) + return [c / b for b, c in zip(baseline, candidate, strict=True) if b > 0 and c > 0] + + +def _braille_sparkline( + values: Iterable[float], threshold: float, *, max_chars: int = 24 +) -> str: + """Render positive ratios as a compact two-samples-per-glyph trace.""" + logs = [math.log(v) for v in values if math.isfinite(v) and v > 0] + if not logs: + return "—" + limit = max_chars * 2 + if len(logs) > limit: + # Median bins retain the robust character of the reported estimator. + logs = [ + statistics.median( + logs[round(i * len(logs) / limit) : round((i + 1) * len(logs) / limit)] + ) + for i in range(limit) + ] + span = max(math.log(threshold), max(abs(v) for v in logs), 1e-12) + dot_bits = ((0, 1, 2, 6), (3, 4, 5, 7)) + glyphs: list[str] = [] + for i in range(0, len(logs), 2): + bits = 0 + for column, value in enumerate(logs[i : i + 2]): + # Braille rows run top to bottom; positive/slower belongs at top. + row = round((span - value) / (2 * span) * 3) + row = max(0, min(3, row)) + bits |= 1 << dot_bits[column][row] + glyphs.append(chr(0x2800 + bits)) + return "".join(glyphs) + + +def _full_measurements( + rows: list[ReportRow], metadata: Mapping[str, object] +) -> list[str]: + if not rows: + return [] + sources = _sources_by_tag(metadata) + lines = [ + "
", + "All measurements, controls, and statistical details", + "", + ] + if any(row.result.control for row in rows): + lines += [ + "**A/A control** rows intentionally run multiple copies of the reference " + "build against itself on a fixed 1 MiB encrypt operation. They measure " + "runner noise; they are not candidate comparisons.", + "", + ] lines += [ + "| cell | contrast | metric | a median | b median | ratio b/a (95% CI) | p (BH) | n | verdict |", + "| --- | --- | --- | --- | --- | --- | --- | ---: | --- |", + ] + for row in rows: + c = row.comparison + qualifier = ( + "A/A control" + if row.result.control + else "head-to-head" + if row.head_to_head + else "" + if row.gated + else "ungated" + ) + label = METRIC_LABELS[row.metric][0] + (f" ({qualifier})" if qualifier else "") + lines.append( + f"| {_detail_cell_label(row.result)} " + f"| {_linked_arm(row.b, sources)} vs {_linked_arm(row.a, sources)} " + f"| {label} " + f"| {format_metric(row.metric, c.baseline_median)} " + f"| {format_metric(row.metric, c.candidate_median)} " + f"| {_ratio_cell(c)} | {_p_cell(c)} | {c.n_rounds} " + f"| {_verdict_cell(c)} |" + ) + return lines + ["", "
", ""] + + +def _detail_cell_label(result: CellResult) -> str: + label = _cell_label(result) + if result.control: + operation = label.removesuffix(" / control").replace("1MiB", "1 MiB") + return f"**A/A control** · {operation}" + return label + + +def _not_measured(recorder: BenchmarkRecorder) -> list[str]: + if not recorder.skipped: + return [] + lines = [ + "
", + "" + + ( + "Cells not run (same commit)" + if same_commit(recorder.metadata) + else "Not measured" + ) + + "", "", - f"seed {config.seed}; warm-up {config.warmup} rounds; " - f"{config.min_rounds}-{config.max_rounds} measured rounds per cell; " - "stopping on attained CI width, never on significance.", ] - return "\n".join(lines) + "\n" + lines += [f"- `{cid}`: {why}" for cid, why in sorted(recorder.skipped.items())] + return lines + ["", "
", ""] + + +def _method(config: BenchConfig, n_arms: int) -> list[str]: + threshold_pct = (config.threshold - 1) * 100 + return [ + "
", + "How to read this gate", + "", + f"This is a paired {n_arms}-arm comparison: every arm ran in the same " + "randomized rounds on one runner. A reference contrast regresses only " + f"when its 95% CI is wholly beyond +{threshold_pct:.0f}% " + f"and its directional BH-adjusted p-value is below {stats.DEFAULT_ALPHA}. " + "The loop stops on attained interval width, never significance.", + "", + "PASS means the run was precise enough to have found an effect at the " + "threshold; INCONCLUSIVE does not mean no change.", + "", + "
", + "", + ] + + +def _run_facts( + recorder: BenchmarkRecorder, + config: BenchConfig, + gate: stats.GateResult, + artifact_url: str, +) -> list[str]: + rounds = [r.n_rounds for r in recorder.results if not r.control] + elapsed = sum(r.elapsed_s for r in recorder.results) + noise = gate.noise + noise_text = ( + f"±{(noise.width_ratio - 1) * 100:.1f}%" + if noise is not None and math.isfinite(noise.width_ratio) + else "unavailable" + ) + metadata = recorder.metadata + artifact = f"[JSON + HTML]({artifact_url})" if artifact_url else "local output" + round_text = f"{min(rounds)}–{max(rounds)}" if rounds else "0" + return [ + "### Run facts", + "", + "| result cells | rounds/cell | elapsed | A/A noise | platform | runner | evidence |", + "| ---: | ---: | ---: | ---: | --- | --- | --- |", + f"| {sum(not r.control for r in recorder.results)} " + f"| {round_text} | {elapsed:.0f}s | {noise_text} " + f"| {_md(str(metadata.get('platform_version', 'unknown')))} " + f"| {_md(str(metadata.get('runner_os') or metadata.get('platform', 'unknown')))} " + f"| {artifact} |", + "", + f"seed {config.seed}; {config.warmup} warm-up rounds; " + f"{config.min_rounds}–{config.max_rounds} measured rounds allowed; " + f"{len(recorder.skipped)} " + f"{'cell' if len(recorder.skipped) == 1 else 'cells'} skipped.", + ] def _ratio_cell(c: stats.PairedComparison) -> str: @@ -235,7 +1094,13 @@ def _ratio_cell(c: stats.PairedComparison) -> str: def _p_cell(c: stats.PairedComparison) -> str: - p = c.p_adjusted if c.p_adjusted is not None else c.p_value + # Show the one-sided tail matching the observed effect. For a ratio below + # one that is the explicit faster-tail test; adjusted upper-tail p-values + # cannot be interpreted backwards after BH correction. + if c.ratio < 1: + p = c.p_adjusted_faster if c.p_adjusted_faster is not None else c.p_value_faster + else: + p = c.p_adjusted if c.p_adjusted is not None else c.p_value if p is None or not math.isfinite(p): return "-" return f"{p:.3f}" if p >= 0.001 else "<0.001" @@ -246,6 +1111,12 @@ def _p_cell(c: stats.PairedComparison) -> str: stats.Verdict.REGRESSION: "**REGRESSION**", stats.Verdict.IMPROVED: "IMPROVED", stats.Verdict.INCONCLUSIVE: "inconclusive", + # Head-to-head vocabulary. Not bolded: none of these can fail the build, + # and a SLOWER that looked like a REGRESSION would invite someone to treat + # it as one. + stats.Verdict.FASTER: "faster", + stats.Verdict.SLOWER: "slower", + stats.Verdict.TIED: "tied", } diff --git a/xtest/perf/runner.py b/xtest/perf/runner.py index e4a80b576..5748d4cbe 100644 --- a/xtest/perf/runner.py +++ b/xtest/perf/runner.py @@ -1,20 +1,29 @@ """The paired round loop that produces comparable samples for one cell. -A *cell* is one operation at one payload size, measured for two SDK builds. -The loop runs both arms once per round, in a randomized order, until it has -either enough precision or no more time. +A *cell* is one operation at one payload size, measured for K SDK builds +(2 to 4). The loop runs every arm once per round, in a randomized order, until +it has either enough precision or no more time. Why rounds rather than "run A 30 times, then B 30 times" -------------------------------------------------------- A shared runner drifts: a noisy neighbour arrives, the CPU thermally throttles, the page cache warms. Run all of A and then all of B and every one of those effects lands entirely on one arm and shows up as a difference between builds. -Interleaving means both arms see the same conditions within a round, and the +Interleaving means every arm sees the same conditions within a round, and the per-round ratio differences it out. The order *within* a round is randomized because a fixed order is itself a confounder -- whichever arm runs second inherits the first one's cache state. +Why every arm shares a round, and why that is the whole point at K > 2 +---------------------------------------------------------------------- +Because all K arms are measured in the same round on the same runner, *every* +pairwise contrast is a within-run ratio -- not just each candidate against the +reference, but candidate-against-candidate too. That is what makes a bake-off +between two competing implementations answerable at all. Running them as two +separate 2-arm jobs puts them on different runners, and this harness's founding +premise is that timings from different runners are not comparable. + Why stopping on precision and not on significance ------------------------------------------------- The loop stops when the confidence interval is narrow enough, never when the @@ -124,11 +133,17 @@ def child_env(self) -> dict[str, str]: @dataclass(frozen=True, slots=True) class Arm: - """One side of a comparison.""" + """One build participating in a cell. + + At K > 2 there is no such thing as "the candidate", so arms are keyed by + identity rather than by role: ``name`` is the arm id, and which arm is the + reference is a property of the cell, not of the arm. + """ - #: ``"baseline"`` or ``"candidate"``; identifies the role, not the build. + #: Arm id, unique within a cell -- the flattened dist tag, e.g. ``"main"`` + #: or ``"fix--otdfctl-streaming-encrypt-writer"``. name: str - #: The build under this role, e.g. ``"go@v0.29.0"``. + #: The build this arm runs, e.g. ``"go@v0.29.0"``. label: str invocation: Invocation @@ -143,14 +158,18 @@ class CellResult: """ cell_id: str - baseline_label: str - candidate_label: str - #: ``samples[arm_name][metric]`` is the per-round vector, warm-up excluded. + #: Arm ids in the order they were configured. ``arm_ids[0]`` is the + #: reference by convention, and ``reference`` names it explicitly. + arm_ids: tuple[str, ...] + #: Arm id -> the build it ran, e.g. ``"go@v0.29.0"``. + arm_labels: dict[str, str] + reference: str + #: ``samples[arm_id][metric]`` is the per-round vector, warm-up excluded. samples: dict[str, dict[str, list[float]]] n_warmup: int elapsed_s: float stopped_because: str - #: True for the A/A control, where both arms are the same build. + #: True for the A/A control, where every arm is the same build. control: bool = False #: Which SDK's control cell assesses this cell's noise floor. A run may #: measure several SDKs, and each has its own harness path and its own @@ -160,12 +179,32 @@ class CellResult: #: that forked each invocation. See :mod:`perf._launcher`. rss_floor_bytes: int = 0 + @property + def baseline_label(self) -> str: + """The reference build's label. + + Kept alongside :attr:`candidate_label` so that the two-arm shape of the + JSON artifact -- which predates K arms and which people have scripts + pointed at -- still reads correctly for a two-arm run. + """ + return self.arm_labels[self.reference] + + @property + def candidate_label(self) -> str: + """The second arm's build label; see :attr:`baseline_label`. + + At K > 2 this is one candidate among several and says nothing about the + rest, which is why the artifact also carries the full ``arm_labels``. + """ + others = [a for a in self.arm_ids if a != self.reference] + return self.arm_labels[others[0]] if others else self.baseline_label + @property def rss_censored_reason(self) -> str | None: """Why this cell's peak RSS cannot be compared, or None if it can. A command whose peak sits at the floor was not measured, it was - clipped, and both arms clip to the same value. The resulting ratio is + clipped, and every arm clips to the same value. The resulting ratio is 1.000 with a tight interval, which is the most convincing-looking PASS the harness can emit and means nothing at all. """ @@ -176,7 +215,7 @@ def rss_censored_reason(self) -> str | None: return None return ( f"peak rss reaches the {self.rss_floor_bytes / 2**20:.0f} MiB " - "measurement floor, so the two arms are not distinguishable" + "measurement floor, so the arms are not distinguishable" ) @property @@ -184,31 +223,54 @@ def n_rounds(self) -> int: first = next(iter(self.samples.values()), {}) return len(next(iter(first.values()), [])) - def metric_pair(self, metric: str) -> tuple[list[float], list[float]]: - """Return ``(baseline, candidate)`` vectors for one metric.""" - return self.samples["baseline"][metric], self.samples["candidate"][metric] + def contrast( + self, a: str, b: str, metric: str, config: BenchConfig + ) -> stats.PairedComparison: + """Compare arm ``b`` against arm ``a`` -- a ratio of b over a. - def compare(self, metric: str, config: BenchConfig) -> stats.PairedComparison: - baseline, candidate = self.metric_pair(metric) + The argument order matches the reading of the result: + ``contrast(reference, candidate, ...)`` answers "how much slower is the + candidate than the reference", which is the direction every ratio in + the report is quoted in. + """ return stats.compare( - baseline, - candidate, + self.samples[a][metric], + self.samples[b][metric], confidence=config.confidence, seed=config.seed, n_resamples=config.n_resamples, ) + def contrast_pairs(self) -> list[tuple[str, str]]: + """Every ordered ``(a, b)`` pair to report, reference contrasts first. -def _empty_samples() -> dict[str, dict[str, list[float]]]: - return {arm: {m: [] for m in METRICS} for arm in ("baseline", "candidate")} + Reference contrasts are quoted as ``(reference, candidate)`` so they + read as "candidate vs reference". Head-to-head pairs are emitted in + configured order, once each -- a pair and its inverse are the same + measurement read two ways, and reporting both would double-count it in + the multiplicity correction. + """ + others = [a for a in self.arm_ids if a != self.reference] + pairs = [(self.reference, b) for b in others] + pairs += [(a, b) for i, a in enumerate(others) for b in others[i + 1 :]] + return pairs + + +def contrast_key(cell_id: str, a: str, b: str, metric: str) -> str: + """The stable key for one contrast, as used throughout the analysis.""" + return f"{cell_id}/{b}_vs_{a}/{metric}" + + +def _empty_samples(arm_ids: Sequence[str]) -> dict[str, dict[str, list[float]]]: + return {arm: {m: [] for m in METRICS} for arm in arm_ids} def run_cell( cell_id: str, - baseline: Arm, - candidate: Arm, + arms: Sequence[Arm], config: BenchConfig, *, + reference: str | None = None, deadline: float | None = None, control: bool = False, sdk: str = "", @@ -220,10 +282,15 @@ def run_cell( Args: cell_id: stable identifier, also the per-cell RNG seed material so that two cells do not share an interleaving order. - baseline: the arm the candidate is compared against. - candidate: the arm under test. For an A/A control this is the same - build as ``baseline``, running through the identical path. + arms: the 2 to 4 builds to measure, all in every round. For an A/A + control these are the same build running through identical paths. + config: round-loop knobs. + reference: id of the arm every gated contrast is taken against; + defaults to ``arms[0]``. deadline: absolute ``clock()`` value past which no new round starts. + Also caps each invocation's timeout to whatever remains, so one + slow arm cannot run past the deadline before the round-level + checks get a chance to notice. control: records that this is the A/A cell; does not change the loop. sdk: which SDK this cell belongs to, so that the analysis can pair it with the right control. Only matters in a multi-SDK run. @@ -231,24 +298,37 @@ def run_cell( run: injectable measurement function, for testing the loop itself. Raises: + ValueError: if fewer than two arms were given, or their ids collide. MeasurementError: if any invocation fails. A benchmark over an operation that errors out is measuring the error path. BudgetExhausted: if the deadline passed before ``min_rounds``, or during warm-up. """ - arms = (baseline, candidate) + arms = tuple(arms) + if len(arms) < 2: + raise ValueError(f"{cell_id}: a cell needs at least two arms to compare") + arm_ids = tuple(a.name for a in arms) + if len(set(arm_ids)) != len(arm_ids): + # Ids key the sample vectors, so a collision would silently interleave + # two builds' measurements into one arm and compare it with itself. + raise ValueError(f"{cell_id}: arm ids must be unique, got {list(arm_ids)}") + reference = arm_ids[0] if reference is None else reference + if reference not in arm_ids: + raise ValueError(f"{cell_id}: reference {reference!r} is not one of the arms") + # Seeded per cell so a rerun reproduces the interleaving exactly, but the # cells do not all share one order (which would correlate their noise). rng = random.Random(f"{config.seed}:{cell_id}") - samples = _empty_samples() + samples = _empty_samples(arm_ids) round_durations: list[float] = [] rss_floor = 0 started = clock() def one_round(into: dict[str, dict[str, list[float]]]) -> None: - # Build the round off to the side. If the deadline expires after one - # arm, none of that partial round may enter the paired sample vectors. - completed = _empty_samples() + # Build the round off to the side. If the deadline expires partway + # through, none of this partial round may enter the paired sample + # vectors. + completed = _empty_samples(arm_ids) round_rss_floor = 0 order = list(arms) rng.shuffle(order) @@ -286,99 +366,124 @@ def one_round(into: dict[str, dict[str, list[float]]]) -> None: for metric in METRICS: into[arm_name][metric].extend(completed[arm_name][metric]) - for i in range(config.warmup): - # Warm-up rounds pay the one-time costs -- page cache, `go build` - # cache, npx package resolution, JIT warm-up -- that would otherwise - # land unevenly and show up as a difference between builds. Their - # samples are collected into a throwaway dict and dropped. - # - # The deadline is checked here too, and not only in the measured loop - # below. The budget's end is absolute, so warm-ups that overrun it - # spend the *following* cells' time and then reach the measured loop - # with nothing left -- paying the full cost of the cell and producing - # no data. Better to give up here and say why. - if deadline is not None and clock() >= deadline: - raise BudgetExhausted( - f"{cell_id}: budget ran out after {i} of {config.warmup} " - f"warm-up rounds ({clock() - started:.0f}s), " - "before any measurement began" - ) - try: - one_round(_empty_samples()) - except BudgetExhausted as e: - raise BudgetExhausted( - f"{cell_id}: budget ran out during warm-up after {i} of " - f"{config.warmup} rounds ({clock() - started:.0f}s)" - ) from e - - stopped_because = "max_rounds" - for _ in range(config.max_rounds): - round_start = clock() - if deadline is not None and round_start >= deadline: - stopped_because = "budget" - break - if deadline is not None and round_durations: - # Do not start a round we cannot finish: a half-measured round is - # unpaired data, and unpaired data is exactly what this design - # exists to avoid. - expected = float(np.median(round_durations)) - if round_start + expected > deadline: + try: + for i in range(config.warmup): + # Warm-up rounds pay the one-time costs -- page cache, `go build` + # cache, npx package resolution, JIT warm-up -- that would + # otherwise land unevenly and show up as a difference between + # builds. Their samples are collected into a throwaway dict and + # dropped. + # + # The deadline is checked here too, and not only in the measured + # loop below. The budget's end is absolute, so warm-ups that + # overrun it spend the *following* cells' time and then reach the + # measured loop with nothing left -- paying the full cost of the + # cell and producing no data. Better to give up here and say why. + if deadline is not None and clock() >= deadline: + raise BudgetExhausted( + f"{cell_id}: budget ran out after {i} of {config.warmup} " + f"warm-up rounds ({clock() - started:.0f}s), " + "before any measurement began" + ) + try: + one_round(_empty_samples(arm_ids)) + except BudgetExhausted as e: + raise BudgetExhausted( + f"{cell_id}: budget ran out during warm-up after {i} of " + f"{config.warmup} rounds ({clock() - started:.0f}s)" + ) from e + + stopped_because = "max_rounds" + for _ in range(config.max_rounds): + round_start = clock() + if deadline is not None and round_start >= deadline: stopped_because = "budget" break - try: - one_round(samples) - except BudgetExhausted: - stopped_because = "budget" - break - round_durations.append(clock() - round_start) - - n = len(samples["baseline"]["wall"]) - if n >= config.min_rounds and _precise_enough(samples, config): - stopped_because = "precision" - break - - elapsed = clock() - started - n = len(samples["baseline"]["wall"]) - if n < config.min_rounds: - raise BudgetExhausted( - f"{cell_id}: only {n} rounds completed in {elapsed:.0f}s, " - f"below the configured minimum of {config.min_rounds}" + if deadline is not None and round_durations: + # Do not start a round we cannot finish: a half-measured round + # is unpaired data, and unpaired data is exactly what this + # design exists to avoid. + expected = float(np.median(round_durations)) + if round_start + expected > deadline: + stopped_because = "budget" + break + try: + one_round(samples) + except BudgetExhausted: + stopped_because = "budget" + break + round_durations.append(clock() - round_start) + + n = len(samples[reference]["wall"]) + if n >= config.min_rounds and _precise_enough( + samples, config, reference=reference + ): + stopped_because = "precision" + break + + elapsed = clock() - started + n = len(samples[reference]["wall"]) + if n < config.min_rounds: + raise BudgetExhausted( + f"{cell_id}: only {n} rounds completed in {elapsed:.0f}s, " + f"below the configured minimum of {config.min_rounds}" + ) + return CellResult( + cell_id=cell_id, + arm_ids=arm_ids, + arm_labels={a.name: a.label for a in arms}, + reference=reference, + samples=samples, + n_warmup=config.warmup, + elapsed_s=elapsed, + stopped_because=stopped_because, + control=control, + sdk=sdk, + rss_floor_bytes=rss_floor, ) - return CellResult( - cell_id=cell_id, - baseline_label=baseline.label, - candidate_label=candidate.label, - samples=samples, - n_warmup=config.warmup, - elapsed_s=elapsed, - stopped_because=stopped_because, - control=control, - sdk=sdk, - rss_floor_bytes=rss_floor, - ) + finally: + # Each arm leaves behind an output the size of the payload, and + # nothing reads it once the cell is done. Keeping them costs K GiB per + # 1 GiB cell, which every later cell then has to fit around -- so the + # cell that fails on disk is not the one that filled it. + for arm in arms: + if arm.invocation.output is not None: + arm.invocation.output.unlink(missing_ok=True) def _precise_enough( - samples: dict[str, dict[str, list[float]]], config: BenchConfig + samples: dict[str, dict[str, list[float]]], + config: BenchConfig, + *, + reference: str, ) -> bool: - """True once every gated metric's CI is narrow enough to decide on. + """True once every gated contrast's CI is narrow enough to decide on. + + Every non-reference arm must be resolved on every gated metric, not just + the first one: stopping as soon as *some* contrast is precise would leave + the rest INCONCLUSIVE while the budget was still there to spend on them, + and at K arms the slowest contrast to converge is the one that matters. Deliberately looks only at interval *width*, never at where the interval sits or at any p-value -- see the module docstring. """ for metric in config.gated_metrics: - c = stats.compare( - samples["baseline"][metric], - samples["candidate"][metric], - confidence=config.confidence, - seed=config.seed, - n_resamples=_INTERIM_RESAMPLES, - ) - # `not (a <= b)` rather than `a > b`, which is not the same thing when - # a is NaN: an unusable interval must read as "keep going", and - # `NaN > b` is False, which would end the loop and call it precise. - if not c.ci_half_width_log <= config.target_half_width_log: # NOSONAR - return False + for arm, vectors in samples.items(): + if arm == reference: + continue + c = stats.compare( + samples[reference][metric], + vectors[metric], + confidence=config.confidence, + seed=config.seed, + n_resamples=_INTERIM_RESAMPLES, + ) + # `not (a <= b)` rather than `a > b`, which is not the same thing + # when a is NaN: an unusable interval must read as "keep going", + # and `NaN > b` is False, which would end the loop and call it + # precise. + if not c.ci_half_width_log <= config.target_half_width_log: # NOSONAR + return False return True @@ -418,12 +523,24 @@ def remaining_s(self) -> float: def analyze(results: Sequence[CellResult], config: BenchConfig) -> stats.GateResult: """Turn every cell's raw samples into one gate decision. - ``GateResult.comparisons`` is keyed by ``"/"`` and holds - the *finalized* comparisons -- the ones carrying adjusted p-values and - verdicts. Ungated metrics are included so they appear in the report, but - they cannot fail the build, and they are corrected separately from the - gated ones: adjusting across tests nobody gates on only makes a real - regression harder to confirm. + ``GateResult.comparisons`` is keyed by ``"/_vs_/"`` + and holds the *finalized* comparisons -- the ones carrying adjusted + p-values and verdicts. Every pairwise contrast in every cell is here, but + they fall into three groups that are judged and corrected separately: + + - **Gated**: a non-reference arm against the reference, on a gated metric. + One-sided; only these can fail the build. At K arms there are K-1 of + them per cell and metric rather than one. + - **Symmetric**: a head-to-head between two non-reference arms -- the + bake-off question. Judged two-sided against an equivalence band, ranked + and reported, never gated. See invariant #9 in ``README.md``. + - **Ungated**: everything else, reported for context only. + + They get separate BH families for the reason documented in + :func:`stats.apply_multiplicity_control`: adjusting the gate against tests + nobody gates on only makes a real regression harder to confirm. A bake-off + is a question of interest, not a build gate, so it must not dilute the + gate either. Each SDK's cells are paired with *that SDK's* control. A run measuring go and java has two harness paths and two noise floors, and judging java's @@ -432,35 +549,60 @@ def analyze(results: Sequence[CellResult], config: BenchConfig) -> stats.GateRes """ comparisons: dict[str, stats.PairedComparison] = {} gated: set[str] = set() + symmetric: set[str] = set() censored: dict[str, str] = {} control_keys: set[str] = set() controls: dict[str, str] = {} - control_for_sdk = { - r.sdk: f"{r.cell_id}/{_CONTROL_METRIC}" for r in results if r.control - } for result in results: floored = result.rss_censored_reason - for metric in METRICS: - key = f"{result.cell_id}/{metric}" - comparisons[key] = result.compare(metric, config) - control_key = control_for_sdk.get(result.sdk) - if control_key is not None: - controls[key] = control_key - # Identify every metric of a control before applying metric-specific - # censoring. Otherwise a floored control RSS key looks like an A/B - # comparison to the run-level "nothing measured" safeguard. - if result.control: - control_keys.add(key) - if metric == "rss" and floored is not None: - censored[key] = floored - continue - if not result.control and metric in config.gated_metrics: - gated.add(key) + for a, b in result.contrast_pairs(): + head_to_head = result.reference not in (a, b) + for metric in METRICS: + key = contrast_key(result.cell_id, a, b, metric) + comparisons[key] = result.contrast(a, b, metric, config) + if metric == "rss" and floored is not None: + censored[key] = floored + continue + if result.control: + control_keys.add(key) + elif head_to_head: + # Every head-to-head is judged symmetrically, on gated and + # ungated metrics alike: "which arm is faster" has no + # privileged direction, and the one-sided PASS/REGRESSION + # vocabulary would misdescribe it. None of them gate, so + # sharing one BH family costs the gate nothing -- the + # separation that matters is keeping them *out* of it. + symmetric.add(key) + elif metric in config.gated_metrics: + gated.add(key) + + # Pick each SDK's noise floor only once every control contrast exists: a + # K-arm control produces C(K,2) of them and the worst one is the floor. + assessor_for_sdk = { + r.sdk: stats.worst_control_key( + comparisons, + [ + contrast_key(r.cell_id, a, b, _CONTROL_METRIC) + for a, b in r.contrast_pairs() + ], + threshold=config.threshold, + ) + for r in results + if r.control + } + for result in results: + assessor = assessor_for_sdk.get(result.sdk) + if assessor is None: + continue + for a, b in result.contrast_pairs(): + for metric in METRICS: + controls[contrast_key(result.cell_id, a, b, metric)] = assessor return stats.apply_multiplicity_control( comparisons, gated=gated, + symmetric=symmetric, controls=controls, control_keys=control_keys, censored=censored, diff --git a/xtest/perf/stats.py b/xtest/perf/stats.py index f7aea9660..eaaaa9611 100644 --- a/xtest/perf/stats.py +++ b/xtest/perf/stats.py @@ -1,4 +1,4 @@ -"""Paired statistical comparison of two SDK builds. +"""Paired statistical comparison of SDK builds. Why this shape -------------- @@ -28,16 +28,33 @@ Requiring both is deliberate, and neither clause is redundant: -- Clause 1 alone would fire on a real-but-trivial effect measured precisely - enough -- a reproducible 0.5% slowdown is not worth a red build. - It cannot fire on pure noise, since that would require the interval to - exclude an effect that is not there. -- Clause 2 alone would fire on noise roughly ``alpha`` of the time per cell, - and a run has enough cells that "roughly alpha" becomes "most nights". - BH adjustment across cells controls the false discovery rate. +- Clause 1 establishes that the effect is larger than the practical threshold, + but an unadjusted 95% interval on every cell does not control false + discoveries across the run. +- Clause 2 supplies that multiplicity control, but alone would flag both + real-but-trivial effects and pure-noise false positives. A reproducible 0.5% + slowdown is statistically real and still not worth a red build. Together they answer the only question worth gating on: is the slowdown both real and large enough to care about? + +Head-to-head contrasts +---------------------- +A run may measure more than two arms. Every arm runs once per round, so any +*pair* of them is a valid within-round comparison -- which is what makes a +bake-off between two competing implementations possible at all, and what two +separate two-arm runs on two different runners could never give you. + +Contrasts against the run's designated reference keep the rule above and can +fail the build. Contrasts between two non-reference arms are judged by +:func:`_symmetric_verdict_for` instead, which reads the same threshold as a +two-sided equivalence band and returns FASTER, SLOWER, or TIED. They never +gate: a bake-off ranks candidates, it does not decide whether the build is +broken, and there is no incumbent for "regression" to be relative to. + +The three families -- gated, head-to-head, and ungated -- are BH-corrected +separately, so adding candidates to a bake-off does not cost the regression +gate any power. """ from __future__ import annotations @@ -67,13 +84,28 @@ class Verdict(StrEnum): - """Outcome for a single comparison cell.""" + """Outcome for a single comparison. + + The first four are the *gated* vocabulary, used for a contrast against the + run's reference build: the question is one-sided ("did the candidate get + slower?") and only REGRESSION can turn the build red. + + The last three are the *symmetric* vocabulary, used for a head-to-head + between two non-reference arms in a bake-off. There the question has no + privileged direction -- neither arm is the incumbent -- and "no measurable + difference" is a real answer rather than the absence of one, so TIED exists + instead of reusing PASS. + """ PASS = "PASS" REGRESSION = "REGRESSION" IMPROVED = "IMPROVED" INCONCLUSIVE = "INCONCLUSIVE" + FASTER = "FASTER" + SLOWER = "SLOWER" + TIED = "TIED" + @dataclass(frozen=True, slots=True) class PairedComparison: @@ -89,9 +121,15 @@ class PairedComparison: ratio: float ci_low: float ci_high: float + #: One-sided p-value for "candidate is slower". Retains the original field + #: name for artifact/API compatibility; the opposite direction is explicit. p_value: float + #: One-sided p-value for "candidate is faster". + p_value_faster: float #: Set by :func:`apply_multiplicity_control` once every cell is known. p_adjusted: float | None = None + #: BH-adjusted form of :attr:`p_value_faster`. + p_adjusted_faster: float | None = None verdict: Verdict = Verdict.INCONCLUSIVE note: str = "" @@ -174,18 +212,22 @@ def _bootstrap_ci( return float(res.confidence_interval.low), float(res.confidence_interval.high) -def _one_sided_p(d: np.ndarray) -> float: - """One-sided Wilcoxon signed-rank p-value for "candidate is slower". +def _one_sided_ps(d: np.ndarray) -> tuple[float, float]: + """Wilcoxon signed-rank p-values for slower and faster, respectively. - Signed-rank rather than a t-test because latency distributions are - skewed and occasionally have a stray outlier round; we do not want a - single stalled invocation to drive the verdict. + Signed-rank does not require normally distributed raw latencies and a + single stalled invocation cannot drive it by magnitude alone. Interpreting + it as a location test does assume the *paired log differences* are roughly + symmetric; the log transform and multiplicative-jitter measurement model + are intended to make that a reasonable assumption. """ if np.all(d == 0): # No difference whatsoever. Wilcoxon rejects an all-zero input. - return 1.0 + return 1.0, 1.0 # scipy's stubs type the result as an opaque tuple-like; index and cast. - return cast(float, _scipy_stats.wilcoxon(d, alternative="greater")[1]) + slower = cast(float, _scipy_stats.wilcoxon(d, alternative="greater")[1]) + faster = cast(float, _scipy_stats.wilcoxon(d, alternative="less")[1]) + return slower, faster def compare( @@ -216,12 +258,14 @@ def compare( ci_low=math.nan, ci_high=math.nan, p_value=math.nan, + p_value_faster=math.nan, note=f"only {n} usable rounds; need at least {MIN_USABLE_ROUNDS}", ) lo_log, hi_log = _bootstrap_ci( d, confidence=confidence, seed=seed, n_resamples=n_resamples ) + p_slower, p_faster = _one_sided_ps(d) return PairedComparison( n_rounds=n, baseline_median=b_med, @@ -229,7 +273,8 @@ def compare( ratio=math.exp(float(np.median(d))), ci_low=math.exp(lo_log), ci_high=math.exp(hi_log), - p_value=_one_sided_p(d), + p_value=p_slower, + p_value_faster=p_faster, ) @@ -312,6 +357,13 @@ def assess_noise_floor( ) +def _noise_rank(n: NoiseFloor) -> tuple[bool, bool, float]: + """Order noise floors from most to least reassuring.""" + # A NaN width is an unusable interval, which is worse than any real one. + width = n.width_ratio if math.isfinite(n.width_ratio) else math.inf + return (n.tripped, n.underpowered, width) + + def _worst_noise(noises: Iterable[NoiseFloor]) -> NoiseFloor | None: """The least reassuring control in the run, or None if there were none. @@ -319,13 +371,31 @@ def _worst_noise(noises: Iterable[NoiseFloor]) -> NoiseFloor | None: harness may be biased on this runner, and averaging that away with two quiet ones is exactly the reassurance the control exists to withhold. """ + return max(noises, key=_noise_rank, default=None) - def rank(n: NoiseFloor) -> tuple[bool, bool, float]: - # A NaN width is an unusable interval, which is worse than any real one. - width = n.width_ratio if math.isfinite(n.width_ratio) else math.inf - return (n.tripped, n.underpowered, width) - return max(noises, key=rank, default=None) +def worst_control_key( + comparisons: Mapping[str, PairedComparison], + keys: Sequence[str], + *, + threshold: float = DEFAULT_THRESHOLD, +) -> str | None: + """Which of several A/A contrasts should stand as the noise floor. + + A K-arm control cell yields C(K,2) A/A contrasts rather than one, and they + are not interchangeable: arm 3 runs two invocations after arm 1, so it + carries more within-round drift than an adjacent pair does. Taking the + worst of them keeps the floor honest for the widest-spaced contrast the + run actually judges. Taking whichever came first would let dict ordering + decide how noisy the run is allowed to look. + + Ties break on input order, so the choice is reproducible across runs. + """ + ranked = [ + (_noise_rank(assess_noise_floor(comparisons.get(k), threshold=threshold)), i, k) + for i, k in enumerate(keys) + ] + return max(ranked, key=lambda r: (r[0], -r[1]))[2] if ranked else None def benjamini_hochberg(p_values: Sequence[float]) -> list[float]: @@ -359,13 +429,19 @@ class GateResult: #: Keys of cells that are confirmed regressions on a gated metric. regressions: list[str] = field(default_factory=list) improvements: list[str] = field(default_factory=list) + #: Head-to-head keys that came back FASTER or SLOWER -- a bake-off contrast + #: between two non-reference arms that the run was able to decide. Never + #: gates anything; this is the material a caller ranks arms from. Naming a + #: winner needs to know which arm is which, and a key is opaque here, so + #: that lives in the reporting layer. + ranked: list[str] = field(default_factory=list) #: True if the run may fail the build. False when the A/A control tripped: #: we still report, but a gate we cannot trust must not turn the build red. trustworthy: bool = True - summary: str = "" - #: At least one comparison between distinct baseline and candidate roles - #: was measured. A clean A/A control alone says nothing about the candidate. + #: At least one comparison against something other than a control was + #: measured. A clean A/A control alone says nothing about the candidate. has_candidate_comparisons: bool = False + summary: str = "" @property def should_fail(self) -> bool: @@ -373,12 +449,16 @@ def should_fail(self) -> bool: @property def nothing_measured(self) -> bool: - """True if the run produced no baseline/candidate comparison. - - An A/A control measures the harness, not the candidate, so a control-only - run still measured nothing that can answer the benchmark's question. - This is not the same thing as "no regressions", though the two are - identical from the outside: both have an empty ``regressions`` list. + """True if the run produced no comparison beyond its own controls. + + Not the same thing as "no regressions", though the two are identical + from the outside: both have an empty ``regressions`` list. Two distinct + ways to get here look the same from outside but need the same + treatment: a run where every cell was skipped -- no baseline installed, + an SDK that would not build -- and a run that measured only its A/A + controls, which assess the harness rather than the candidate. Either + way the benchmark's question was never answered, which is how one that + quietly stopped measuring anything could survive for months unnoticed. Callers gate on this separately. """ return not self.has_candidate_comparisons @@ -388,6 +468,7 @@ def apply_multiplicity_control( comparisons: dict[str, PairedComparison], *, gated: set[str] | None = None, + symmetric: set[str] | None = None, controls: Mapping[str, str] | None = None, control_keys: set[str] | None = None, censored: dict[str, str] | None = None, @@ -401,6 +482,11 @@ def apply_multiplicity_control( gated: keys allowed to fail the build. Keys outside this set are still given a verdict and reported, but never counted as a regression. ``None`` means every key is gated. + symmetric: keys judged by the two-sided FASTER/SLOWER/TIED rule instead + of the one-sided regression rule -- head-to-head contrasts between + two arms neither of which is the run's reference. They are reported + and ranked but never gate, so a bake-off cannot turn the build red + on the strength of a comparison that has no incumbent. controls: comparison key -> the A/A control key that assesses *its* noise floor. A run measuring several SDKs has one control each, and a cell judged against another SDK's control is judged against @@ -423,6 +509,7 @@ def apply_multiplicity_control( """ keys = list(comparisons) controls = controls or {} + symmetric = symmetric or set() # The keys actually doing the assessing: one metric of one control cell # per SDK. A control cell's other metrics are still control keys -- kept # out of the gate -- but they are not anybody's noise floor. @@ -446,11 +533,23 @@ def apply_multiplicity_control( # reason: adjusting them against metrics nobody gates on only makes a real # regression harder to confirm. Ungated metrics still get a family of # their own so that they carry a reportable verdict. + # + # Head-to-head contrasts get a third family on the same argument. A + # bake-off between two candidates is a question of interest, not a build + # gate, and correcting the gate against it would cost the gate power for + # tests that cannot fail the build -- which is the exact trade the + # gated/ungated split already refuses to make. adjustable = [k for k in keys if k not in control_keys and k not in censored] gated_family = [k for k in adjustable if gated is None or k in gated] - rest = [k for k in adjustable if k not in set(gated_family)] + gated_set = set(gated_family) + # Gated wins a tie. A key that is somehow both is a vs-reference contrast, + # and the one-sided rule is the one that can fail the build. + symmetric_family = [k for k in adjustable if k not in gated_set and k in symmetric] + symmetric_set = set(symmetric_family) + rest = [k for k in adjustable if k not in gated_set and k not in symmetric_set] p_adj: dict[str, float] = {} - for family in (gated_family, rest): + p_adj_faster: dict[str, float] = {} + for family in (gated_family, symmetric_family, rest): p_adj.update( zip( family, @@ -458,6 +557,17 @@ def apply_multiplicity_control( strict=True, ) ) + # The opposite direction needs its own lower-tail p-value and its own + # adjustment. Reading an adjusted upper-tail value as `p > 1-alpha` + # is invalid: BH controls small p-values and generally pushes large + # ones toward 1, making that backwards test easier rather than safer. + p_adj_faster.update( + zip( + family, + benjamini_hochberg([comparisons[k].p_value_faster for k in family]), + strict=True, + ) + ) result = GateResult( noise=noise, @@ -467,16 +577,41 @@ def apply_multiplicity_control( for key in keys: c = comparisons[key] pa = p_adj.get(key) + pa_faster = p_adj_faster.get(key) + key_noise = noise_by_control.get(controls.get(key, ""), uncontrolled) if key in censored: verdict, note = Verdict.INCONCLUSIVE, censored[key] + elif key in control_keys: + # A control's own contrast is reported in the gated vocabulary + # whatever kind of pair it is: its job is to say what the harness's + # error looks like in the same terms the gate uses. + verdict, note = _verdict_for( + c, + pa, + pa_faster, + threshold=threshold, + alpha=alpha, + noise=key_noise, + is_control=True, + ) + elif key in symmetric_set: + verdict, note = _symmetric_verdict_for( + c, + pa, + pa_faster, + threshold=threshold, + alpha=alpha, + noise=key_noise, + ) else: verdict, note = _verdict_for( c, pa, + pa_faster, threshold=threshold, alpha=alpha, - noise=noise_by_control.get(controls.get(key, ""), uncontrolled), - is_control=key in control_keys, + noise=key_noise, + is_control=False, ) result.comparisons[key] = PairedComparison( n_rounds=c.n_rounds, @@ -486,7 +621,9 @@ def apply_multiplicity_control( ci_low=c.ci_low, ci_high=c.ci_high, p_value=c.p_value, + p_value_faster=c.p_value_faster, p_adjusted=pa, + p_adjusted_faster=pa_faster, verdict=verdict, note=note or c.note, ) @@ -496,6 +633,8 @@ def apply_multiplicity_control( result.regressions.append(key) elif verdict is Verdict.IMPROVED: result.improvements.append(key) + elif verdict in (Verdict.FASTER, Verdict.SLOWER): + result.ranked.append(key) result.trustworthy = not noise.tripped result.summary = _summarize(result, noise, threshold) @@ -505,6 +644,7 @@ def apply_multiplicity_control( def _verdict_for( c: PairedComparison, p_adjusted: float | None, + p_adjusted_faster: float | None, *, threshold: float, alpha: float, @@ -514,13 +654,16 @@ def _verdict_for( if c.n_rounds < MIN_USABLE_ROUNDS or not math.isfinite(c.ci_low): return Verdict.INCONCLUSIVE, c.note or "no usable interval" - p = c.p_value if is_control else p_adjusted - if p is None or not math.isfinite(p): + p_slower = c.p_value if is_control else p_adjusted + p_faster = c.p_value_faster if is_control else p_adjusted_faster + if p_slower is None or p_faster is None: + return Verdict.INCONCLUSIVE, "no p-value" + if not (math.isfinite(p_slower) and math.isfinite(p_faster)): return Verdict.INCONCLUSIVE, "no p-value" - if c.ci_low > threshold and p < alpha: + if c.ci_low > threshold and p_slower < alpha: return Verdict.REGRESSION, "" - if c.ci_high < 1 / threshold and p > 1 - alpha: + if c.ci_high < 1 / threshold and p_faster < alpha: return Verdict.IMPROVED, "" # Not a regression. But "we looked and found nothing" only counts as PASS @@ -537,6 +680,69 @@ def _verdict_for( return Verdict.PASS, "" +def _symmetric_verdict_for( + c: PairedComparison, + p_adjusted: float | None, + p_adjusted_faster: float | None, + *, + threshold: float, + alpha: float, + noise: NoiseFloor, +) -> tuple[Verdict, str]: + """Rank two arms against each other, with no privileged direction. + + Used for a head-to-head between two candidates in a bake-off, where the + one-sided regression rule does not apply: neither arm is the incumbent, so + there is no "did it get worse" to ask. + + The band is ``[1/threshold, threshold]`` -- the same effect size the gate + cares about, read in both directions: + + - interval entirely above the band -> SLOWER + - interval entirely below the band -> FASTER + - interval entirely *inside* the band -> TIED, meaning any real difference + is smaller than the effect anybody has claimed to care about. This is a + positive finding and the most likely honest answer for two + implementations of the same idea, which is why it is not folded into + PASS: PASS is the one-sided claim "did not regress", and reporting a + bake-off that way would let a slower arm read as a clean result. + - anything straddling a band edge -> INCONCLUSIVE, the run could not rank + them. + + A CI-inside-band test at 95% is TOST at 2.5% rather than the nominal 5%, + so TIED is the conservative call: harder to earn than the equivalence test + it stands in for, never easier. + """ + if c.n_rounds < MIN_USABLE_ROUNDS or not math.isfinite(c.ci_low): + return Verdict.INCONCLUSIVE, c.note or "no usable interval" + + # Same precondition as the gated rule: without a noise floor establishing + # that an effect of this size was resolvable, neither a ranking nor a tie + # is a statement about the arms. Invariant 4 covers TIED too -- a tie + # nobody had the power to distinguish from a difference is not a tie. + if noise.underpowered: + return Verdict.INCONCLUSIVE, noise.detail + + if p_adjusted is None or p_adjusted_faster is None: + return Verdict.INCONCLUSIVE, "no p-value" + if not (math.isfinite(p_adjusted) and math.isfinite(p_adjusted_faster)): + return Verdict.INCONCLUSIVE, "no p-value" + + # Direction clauses mirror REGRESSION and IMPROVED exactly. Each direction + # uses its own BH-adjusted one-sided p-value. + if c.ci_low > threshold and p_adjusted < alpha: + return Verdict.SLOWER, "" + if c.ci_high < 1 / threshold and p_adjusted_faster < alpha: + return Verdict.FASTER, "" + if c.ci_low > 1 / threshold and c.ci_high < threshold: + return Verdict.TIED, "" + return ( + Verdict.INCONCLUSIVE, + f"interval [{c.ci_low:.3f}, {c.ci_high:.3f}] straddles the " + f"+/-{(threshold - 1) * 100:.0f}% band; the two arms cannot be ranked", + ) + + def _summarize(result: GateResult, noise: NoiseFloor, threshold: float) -> str: if result.nothing_measured: # Before the noise check: with nothing measured there is no control @@ -556,6 +762,14 @@ def _summarize(result: GateResult, noise: NoiseFloor, threshold: float) -> str: f"{len(result.regressions)} confirmed regression(s) past the " f"{(threshold - 1) * 100:.0f}% threshold: {', '.join(result.regressions)}" ) + # Appended rather than returned on its own: a bake-off still has a gate + # running against the reference, and "which candidate won" must not + # displace "did either of them regress". + head_to_head = ( + f" {len(result.ranked)} head-to-head contrast(s) decided." + if result.ranked + else "" + ) inconclusive = [ k for k, c in result.comparisons.items() if c.verdict is Verdict.INCONCLUSIVE ] @@ -563,9 +777,11 @@ def _summarize(result: GateResult, noise: NoiseFloor, threshold: float) -> str: return ( f"No confirmed regressions. {len(inconclusive)} cell(s) INCONCLUSIVE " f"(runner noise floor +/-{(noise.width_ratio - 1) * 100:.1f}%)." + f"{head_to_head}" ) return ( f"No regressions. All cells resolved within the " f"{(threshold - 1) * 100:.0f}% threshold " f"(runner noise floor +/-{(noise.width_ratio - 1) * 100:.1f}%)." + f"{head_to_head}" ) diff --git a/xtest/test_bench_arms.py b/xtest/test_bench_arms.py index bf7a3194e..1862710c8 100644 --- a/xtest/test_bench_arms.py +++ b/xtest/test_bench_arms.py @@ -10,13 +10,15 @@ ``tmp_path``. """ +import json from pathlib import Path +from typing import cast import pytest import tdfs from fixtures import bench -from perf.cells import PAYLOADS +from perf.cells import cells_for from perf.runner import BenchConfig @@ -35,6 +37,20 @@ def cwd(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: return tmp_path +class FakeConfig: + """A ``pytest.Config`` stub answering the ``--bench-*`` options named.""" + + def __init__(self, **opts: str | None) -> None: + self._opts = {name.replace("_", "-"): value for name, value in opts.items()} + + def getoption(self, name: str) -> str | None: + return self._opts.get(name.removeprefix("--")) + + +def options(**opts: str | None) -> pytest.Config: + return cast(pytest.Config, FakeConfig(**opts)) + + class TestFinalRelease: @pytest.mark.parametrize("version", ["v0.29.0", "0.29.0"]) def test_accepts_a_plain_tag(self, cwd: Path, version: str): @@ -76,60 +92,323 @@ def test_no_branch_build_is_a_clear_refusal(self, cwd: Path): def test_explicit_specs_win(self, cwd: Path): install(cwd, "go", "main", "v0.28.0", "v0.29.0") - baseline, candidate = bench.select_arms( - "go", baseline_spec="go@v0.28.0", candidate_spec="go@v0.29.0" - ) + baseline, candidate = bench.select_arms("go", ["go@v0.28.0", "go@v0.29.0"]) assert (baseline.version, candidate.version) == ("v0.28.0", "v0.29.0") def test_refuses_to_compare_a_build_against_itself(self, cwd: Path): install(cwd, "go", "main", "v0.29.0") - with pytest.raises(bench.ArmSelectionError, match="nothing to compare"): - bench.select_arms("go", baseline_spec="go@main", candidate_spec="go@main") + with pytest.raises(bench.ArmSelectionError, match="same build"): + bench.select_arms("go", ["go@main", "go@main"]) + + def test_two_branch_builds_need_explicit_specs(self, cwd: Path): + # What a branch-vs-branch dispatch installs: two heads and no release + # at all. Named explicitly it is a fine comparison; left to the default + # there is no baseline, and "newest final release" cannot invent one. + install(cwd, "go", "main", "feat--DSPX-2604-createtdf-chunked") + baseline, candidate = bench.select_arms( + "go", ["go@main", "go@feat--DSPX-2604-createtdf-chunked"] + ) + assert baseline.version == "main" + assert candidate.version == "feat--DSPX-2604-createtdf-chunked" + with pytest.raises(bench.ArmSelectionError, match="no final go release"): + bench.select_arms("go") + + def test_the_first_spec_is_the_reference(self, cwd: Path): + # Order is the whole interface: every gated contrast is taken against + # arms[0], so reversing the list reverses which build is on trial. + install(cwd, "go", "main", "a--impl", "b--impl") + arms = bench.select_arms("go", ["go@main", "go@a--impl", "go@b--impl"]) + assert [a.version for a in arms] == ["main", "a--impl", "b--impl"] + assert bench.BenchArms(arms).reference.version == "main" + assert [a.version for a in bench.BenchArms(arms).candidates] == [ + "a--impl", + "b--impl", + ] + + def test_a_missing_arm_names_which_one(self, cwd: Path): + install(cwd, "go", "main", "a--impl") + with pytest.raises(bench.ArmSelectionError, match="arm 3"): + bench.select_arms("go", ["go@main", "go@a--impl", "go@nope"]) + + +class TestRefSpecParsing: + def test_commas_or_whitespace_both_work(self): + assert bench.parse_refs("go@main,go@a") == ("go@main", "go@a") + assert bench.parse_refs("go@main go@a") == ("go@main", "go@a") + + def test_a_single_ref_is_refused(self): + # One arm is not a comparison, and the harness reports only ratios. + with pytest.raises(ValueError, match="need 2 to 4 refs"): + bench.parse_refs("go@main") + + def test_more_than_four_is_refused(self): + # setup-cli-tool installs four builds side by side; a fifth would be + # silently absent at measurement time. + with pytest.raises(ValueError, match="need 2 to 4 refs"): + bench.parse_refs("go@a,go@b,go@c,go@d,go@e") + + def test_a_repeated_ref_is_refused(self): + with pytest.raises(ValueError, match="duplicate"): + bench.parse_refs("go@main,go@main") + + +class TestSpecsForSdk: + def test_specs_naming_another_sdk_fall_back_to_the_default(self): + # A run measuring go and java with refs for go only: java still gets + # its own default pair rather than an error or go's builds. + assert bench._specs_for(("go@main", "go@a"), "java") is None + + def test_a_mixed_sdk_list_is_an_error(self): + with pytest.raises(bench.ArmSelectionError, match="more than one SDK"): + bench._specs_for(("go@main", "java@main"), "go") + + +class TestArmOptions: + def test_no_options_means_the_default_pair(self): + # The nightly passes nothing at all, and must keep getting the + # (newest release, branch head) comparison it has always run. + assert bench.arm_specs_from_options(options()) is None + assert bench.arm_count(options()) == 2 + + def test_the_two_arm_shorthand_still_works(self): + cfg = options(bench_baseline="go@v0.29.0", bench_candidate="go@main") + assert bench.arm_specs_from_options(cfg) == ("go@v0.29.0", "go@main") + + def test_half_a_pair_is_a_usage_error(self): + with pytest.raises(pytest.UsageError, match="together"): + bench.arm_specs_from_options(options(bench_baseline="go@main")) + + def test_mixing_the_two_forms_is_a_usage_error(self): + # There is no reading of "--bench-refs a,b --bench-candidate c" that + # is not a mistake, and silently picking one would measure something + # nobody asked for. + cfg = options(bench_refs="go@a,go@b", bench_candidate="go@c") + with pytest.raises(pytest.UsageError, match="cannot be combined"): + bench.arm_specs_from_options(cfg) + + def test_a_malformed_refs_list_is_a_usage_error(self): + with pytest.raises(pytest.UsageError, match="invalid --bench-refs"): + bench.arm_specs_from_options(options(bench_refs="go@main")) + + def test_the_arm_count_follows_the_refs(self): + assert bench.arm_count(options(bench_refs="go@a,go@b,go@c")) == 3 + + +class TestRunnerMetadata: + def test_resolver_provenance_is_preserved_and_enriched( + self, monkeypatch: pytest.MonkeyPatch + ): + monkeypatch.setenv( + "BENCH_VERSION_INFO", + json.dumps( + [ + { + "sdk": "go", + "tag": "v0.30.0", + "release": "v0.30.0", + "sha": "a" * 40, + }, + { + "sdk": "go", + "tag": "feature", + "pr": 42, + "head": True, + "sha": "b" * 40, + }, + ] + ), + ) + + sources, warning = bench._arm_sources() + + assert warning == "" + assert sources[0]["repo_url"] == "https://github.com/opentdf/otdfctl" + assert sources[1]["repo_url"] == "https://github.com/opentdf/platform" + + def test_bad_resolver_metadata_degrades_to_an_explanation( + self, monkeypatch: pytest.MonkeyPatch + ): + monkeypatch.setenv("BENCH_VERSION_INFO", "not-json") + + sources, warning = bench._arm_sources() + + assert sources == [] + assert "not valid JSON" in warning + + def test_workflow_url_needs_all_three_parts(self, monkeypatch: pytest.MonkeyPatch): + monkeypatch.setenv("GITHUB_SERVER_URL", "https://github.com") + monkeypatch.setenv("GITHUB_REPOSITORY", "opentdf/tests") + monkeypatch.setenv("GITHUB_RUN_ID", "123") + assert bench._github_run_url() == ( + "https://github.com/opentdf/tests/actions/runs/123" + ) + + monkeypatch.delenv("GITHUB_RUN_ID") + assert bench._github_run_url() == "" + + def test_requested_names_survive_resolver_deduplication( + self, monkeypatch: pytest.MonkeyPatch + ): + monkeypatch.setenv("BENCH_REQUESTED_REFS", "main latest") + + assert bench._requested_refs() == ["main", "latest"] + + def test_one_resolved_sha_from_two_names_is_marked_same_commit( + self, monkeypatch: pytest.MonkeyPatch + ): + monkeypatch.setenv("BENCH_REQUESTED_REFS", "main latest") + monkeypatch.setattr( + bench, + "_arm_sources", + lambda: ([{"tag": "main", "sha": "a" * 40}], ""), + ) + monkeypatch.setattr(bench, "_platform_version", lambda: "test") + + metadata = bench.runner_metadata(options(bench_seed="0")) + + assert metadata["comparison_status"] == "same_commit" + assert "main, latest" in str(metadata["comparison_note"]) + +class TestDefaultBudget: + def test_two_arms_keep_the_number_the_default_was_chosen_for(self): + assert bench.default_budget_seconds(2) == BenchConfig().budget_seconds -#: The fixture body, called directly: these tests are about the bytes it -#: writes, not about pytest's fixture wiring. -make_payloads = bench.bench_payloads.__wrapped__ # pyright: ignore[reportAttributeAccessIssue] + def test_the_budget_scales_with_the_arm_count(self): + # A round costs one invocation per arm, so at a fixed budget the round + # count falls as 2/K and every interval widens as sqrt(K/2). Scaling + # the default by K/2 buys back the precision instead of quietly + # trading it for arms and reporting the loss as INCONCLUSIVE. + base = BenchConfig().budget_seconds + assert bench.default_budget_seconds(3) == base * 1.5 + assert bench.default_budget_seconds(4) == base * 2 + + def test_an_explicit_budget_is_taken_as_given(self): + cfg = FakeConfig( + bench_refs="go@a,go@b,go@c", + bench_budget_seconds="900", + bench_min_rounds="20", + bench_max_rounds="60", + bench_warmup="5", + bench_seed="1", + bench_threshold="1.15", + ) + built = bench.config_from_options(cast(pytest.Config, cfg)) + assert built.budget_seconds == 900.0, "a named number is not scaled" + + +class TestDistTagShape: + def test_a_slashed_tag_breaks_discovery(self, cwd: Path): + # Why otdf-sdk-mgr flattens '/' to '--' in a resolved ref. A branch + # installed as dist/feat/x/ is listed as a build named "feat", which + # has no cli.sh -- and this raises during collection, before any cell + # has a chance to report why. + install(cwd, "go", "feat/DSPX-2604-createtdf-chunked") + with pytest.raises(FileNotFoundError): + tdfs.all_versions_of("go") + + def test_a_flattened_tag_is_discovered(self, cwd: Path): + install(cwd, "go", "feat--DSPX-2604-createtdf-chunked") + assert [s.version for s in tdfs.all_versions_of("go")] == [ + "feat--DSPX-2604-createtdf-chunked" + ] + + +def stub_sdk(cwd: Path, version: str, **features: bool) -> tdfs.SDK: + """An installed stub build whose feature support is stated, not inferred. + + Real support is derived from the version string, which would make these + tests assertions about the version table rather than about arm selection. + """ + install(cwd, "go", version) + sdk = tdfs.SDK("go", version) + sdk._supports.update(cast(dict[tdfs.feature_type, bool], features)) + return sdk -class TestPayloads: - def test_every_size_is_generated(self, tmp_path: Path): - out = make_payloads(tmp_path, BenchConfig(seed=1)) - for payload in PAYLOADS: - assert out[payload.label].stat().st_size == payload.n_bytes +#: Every comparability feature present, which is the uninteresting case. +COMPARABLE = {"hexless": True, "hexaflexible": True, "autoconfigure": True} - def test_a_seed_reproduces_the_bytes(self, tmp_path: Path): - a = read_all(make_payloads(subdir(tmp_path, "a"), BenchConfig(seed=1))) - b = read_all(make_payloads(subdir(tmp_path, "b"), BenchConfig(seed=1))) - assert a == b - def test_a_different_seed_changes_them(self, tmp_path: Path): - a = read_all(make_payloads(subdir(tmp_path, "a"), BenchConfig(seed=1))) - b = read_all(make_payloads(subdir(tmp_path, "b"), BenchConfig(seed=2))) - assert a != b +class TestComparability: + def test_matching_arms_are_comparable(self, cwd: Path): + arms = bench.BenchArms( + ( + stub_sdk(cwd, "main", **COMPARABLE), + stub_sdk(cwd, "a--impl", **COMPARABLE), + stub_sdk(cwd, "b--impl", **COMPARABLE), + ) + ) + assert bench.comparability_problem(arms) is None + + def test_every_candidate_is_checked_against_the_reference(self, cwd: Path): + # Checking only adjacent pairs would clear a third arm that disagrees + # with the reference, and its gated contrast is taken against exactly + # that reference -- so it would be timing different work. + odd_one_out = bench.BenchArms( + ( + stub_sdk(cwd, "main", **COMPARABLE), + stub_sdk(cwd, "a--impl", **COMPARABLE), + stub_sdk(cwd, "b--impl", **(COMPARABLE | {"autoconfigure": False})), + ) + ) + problem = bench.comparability_problem(odd_one_out) + assert problem is not None + assert "b--impl" in problem and "autoconfigure" in problem - def test_a_partial_cache_still_reproduces_the_bytes(self, tmp_path: Path): - # tmp_dir persists between runs. With one RNG stream shared across the - # payloads, skipping a cached file shifts every payload after it, so a - # rerun measures different input than the run it is compared against. - first = read_all(make_payloads(tmp_path, BenchConfig(seed=1))) - (tmp_path / f"bench-plain-{PAYLOADS[0].label}.bin").unlink() - second = read_all(make_payloads(tmp_path, BenchConfig(seed=1))) - assert first == second + def test_the_target_mode_is_pinned_only_when_every_arm_can_be_told(self, cwd: Path): + # Letting one arm choose its own container version would compare + # output formats rather than speed. + all_new = ( + stub_sdk(cwd, "main", **COMPARABLE), + stub_sdk(cwd, "a--impl", **COMPARABLE), + ) + assert bench.pinned_target_mode(bench.BenchArms(all_new)) == "4.3.0" + one_old = all_new + ( + stub_sdk(cwd, "b--impl", **(COMPARABLE | {"hexaflexible": False})), + ) + assert bench.pinned_target_mode(bench.BenchArms(one_old)) is None - def test_a_truncated_cache_entry_is_regenerated(self, tmp_path: Path): - first = read_all(make_payloads(tmp_path, BenchConfig(seed=1))) - path = tmp_path / f"bench-plain-{PAYLOADS[1].label}.bin" - path.write_bytes(b"truncated") - second = read_all(make_payloads(tmp_path, BenchConfig(seed=1))) - assert first == second +class TestBuildArms: + def cell_arms(self, cwd: Path, control: bool, n: int): + arms = bench.BenchArms( + tuple( + stub_sdk(cwd, v, **COMPARABLE) + for v in ("main", "a--impl", "b--impl", "c--impl")[:n] + ) + ) + cells = cells_for(["go"]) + cell = next( + c for c in cells if c.control is control and c.operation == "encrypt" + ) + pt = cwd / "plain.bin" + pt.write_bytes(b"x" * 1024) + return bench.build_arms( + cell, + arms, + pt_file=pt, + ct_file=None, + tmp_dir=cwd, + attr_values=[], + ) -def read_all(paths: dict[str, Path]) -> dict[str, bytes]: - return {label: p.read_bytes() for label, p in paths.items()} + def test_one_arm_per_build(self, cwd: Path): + built = self.cell_arms(cwd, control=False, n=3) + assert [a.name for a in built] == ["main", "a--impl", "b--impl"] + def test_every_arm_writes_its_own_output(self, cwd: Path): + # Sharing an output path would have the arms overwrite each other + # mid-round, and the second one would be measured deleting the first. + built = self.cell_arms(cwd, control=False, n=3) + assert len({a.invocation.output for a in built}) == 3 -def subdir(root: Path, name: str) -> Path: - out = root / name - out.mkdir() - return out + def test_the_control_is_k_copies_of_the_reference(self, cwd: Path): + # Not a cheap pair: in a K-arm round the last arm runs K-1 invocations + # after the first, so a two-arm control would measure less drift than + # the contrasts it is the noise floor for. + built = self.cell_arms(cwd, control=True, n=3) + assert len(built) == 3 + assert {a.label for a in built} == {"go@main"} + assert len({a.name for a in built}) == 3, "ids key the sample vectors" + assert len({a.invocation.output for a in built}) == 3 diff --git a/xtest/test_bench_report.py b/xtest/test_bench_report.py new file mode 100644 index 000000000..662024981 --- /dev/null +++ b/xtest/test_bench_report.py @@ -0,0 +1,380 @@ +"""Tests for what a benchmark run publishes: the JSON artifact and the summary. + +The report is where a K-arm run stops being a pile of ratios and starts being +an answer, so the things worth pinning down are the ones a reader would act on +without checking: which arm won a bake-off, whether a tie is reported as a tie, +and whether a run that could not resolve anything says so instead of returning +a quiet page of INCONCLUSIVE. + +Measurement is simulated exactly as in ``test_bench_runner``; nothing here +touches a platform or a subprocess. +""" + +from __future__ import annotations + +import json +from collections.abc import Mapping +from pathlib import Path + +from perf import report, stats +from perf.runner import BenchConfig +from test_bench_runner import REF, config, run + + +def recorder( + costs: Mapping[str, float], + *, + cfg: BenchConfig | None = None, + noise: float = 0.05, + control: bool = True, +) -> report.BenchmarkRecorder: + """A recorder holding one measured cell and, by default, its A/A control.""" + cfg = cfg or config(max_rounds=40) + rec = report.BenchmarkRecorder() + if control: + aa, _ = run( + dict.fromkeys(costs, 1.0), + cfg=cfg, + noise=noise, + seed=11, + cell_id="aa", + control=True, + sdk="go", + ) + rec.record(aa) + measured, _ = run(costs, cfg=cfg, noise=noise, seed=12, cell_id="encrypt", sdk="go") + rec.record(measured) + return rec + + +def bake_offs( + costs: Mapping[str, float], *, cfg: BenchConfig | None = None, noise: float = 0.05 +) -> tuple[list[report.BakeOff], BenchConfig]: + cfg = cfg or config(max_rounds=40) + rec = recorder(costs, cfg=cfg, noise=noise) + return report.bake_offs(rec, cfg, rec.gate(cfg)), cfg + + +class TestBakeOff: + def test_a_two_arm_run_has_nothing_to_rank(self): + # One candidate is not a bake-off, and the gate has already said + # everything there is to say about it. + offs, _ = bake_offs({REF: 1.0, "cand": 1.3}) + assert offs == [] + + def test_the_control_is_never_ranked(self): + offs, _ = bake_offs({REF: 1.0, "a": 1.0, "b": 1.3}) + assert offs and all(b.cell_id == "encrypt" for b in offs) + + def test_the_faster_candidate_wins(self): + offs, _ = bake_offs({REF: 1.0, "slow": 1.4, "quick": 1.0}, noise=0.02) + wall = next(b for b in offs if b.metric == "wall") + assert wall.winner == "quick" + assert wall.order == ["quick", "slow"] + assert "`quick` wins" in wall.detail + + def test_a_tie_refuses_to_name_a_winner(self): + # Picking whichever point estimate landed lower would be reporting + # noise as a decision, and a merge would be made on it. + offs, _ = bake_offs({REF: 1.0, "a": 1.0, "b": 1.0}, noise=0.01) + wall = next(b for b in offs if b.metric == "wall") + assert wall.verdict is stats.Verdict.TIED + assert wall.winner is None + assert "no measurable difference" in wall.detail + + def test_an_unresolvable_pair_says_so_rather_than_guessing(self): + offs, _ = bake_offs( + {REF: 1.0, "a": 1.0, "b": 1.15}, + cfg=config(max_rounds=20), + noise=0.35, + ) + wall = next(b for b in offs if b.metric == "wall") + assert wall.winner is None + assert "cannot separate" in wall.detail + + def test_the_ranking_covers_every_gated_metric(self): + offs, cfg = bake_offs({REF: 1.0, "a": 1.0, "b": 1.4}, noise=0.02) + assert {b.metric for b in offs} == set(cfg.gated_metrics) + + +class TestUnderpoweredWarning: + def test_a_precise_run_says_nothing(self): + cfg = config(max_rounds=60) + rec = recorder({REF: 1.0, "a": 1.0, "b": 1.0}, cfg=cfg, noise=0.005) + assert report.underpowered_warning(rec, cfg, rec.gate(cfg)) is None + + def test_a_run_that_could_not_resolve_anything_asks_for_more_budget(self): + # Otherwise three arms on a two-arm budget comes back as a wall of + # INCONCLUSIVE after burning the whole runner, with nothing in the + # output saying that time was the missing ingredient. + cfg = config(max_rounds=20) + rec = recorder({REF: 1.0, "a": 1.0, "b": 1.0}, cfg=cfg, noise=0.4) + warning = report.underpowered_warning(rec, cfg, rec.gate(cfg)) + assert warning is not None + assert "3-arm round costs 3 invocations" in warning + assert "INCONCLUSIVE" in warning + + def test_the_warning_reaches_the_summary(self): + cfg = config(max_rounds=20) + rec = recorder({REF: 1.0, "a": 1.0, "b": 1.0}, cfg=cfg, noise=0.4) + md = report.markdown(rec, cfg, rec.gate(cfg)) + assert "[!WARNING]" in md and "Underpowered" in md + + +class TestJsonArtifact: + def artifact(self, tmp_path: Path, costs: Mapping[str, float]) -> dict: + cfg = config(max_rounds=40) + rec = recorder(costs, cfg=cfg, noise=0.02) + path = report.write_json(tmp_path / "bench.json", rec, cfg, rec.gate(cfg)) + return json.loads(path.read_text()) + + def test_arms_and_the_reference_are_recorded(self, tmp_path: Path): + # Which arm the ratios are taken against is not recoverable from the + # numbers, and every contrast in the file is meaningless without it. + doc = self.artifact(tmp_path, {REF: 1.0, "a": 1.0, "b": 1.3}) + cell = next(c for c in doc["cells"] if c["id"] == "encrypt") + assert cell["arms"] == [REF, "a", "b"] + assert cell["reference"] == REF + + def test_every_pair_appears_once(self, tmp_path: Path): + doc = self.artifact(tmp_path, {REF: 1.0, "a": 1.0, "b": 1.3}) + cell = next(c for c in doc["cells"] if c["id"] == "encrypt") + assert set(cell["contrasts"]) == {f"a_vs_{REF}", f"b_vs_{REF}", "b_vs_a"} + + def test_two_arm_readers_still_find_a_baseline_and_candidate(self, tmp_path: Path): + doc = self.artifact(tmp_path, {REF: 1.0, "cand": 1.3}) + cell = next(c for c in doc["cells"] if c["id"] == "encrypt") + assert doc["schema"] == 2 + assert cell["baseline"] == f"sdk@{REF}" + assert cell["candidate"] == "sdk@cand" + + def test_raw_samples_survive_for_every_arm(self, tmp_path: Path): + # Re-analysing a surprising result offline is the difference between + # understanding a red build and re-running the whole job to see the + # same numbers again. + doc = self.artifact(tmp_path, {REF: 1.0, "a": 1.0, "b": 1.3}) + cell = next(c for c in doc["cells"] if c["id"] == "encrypt") + assert set(cell["samples"]) == {REF, "a", "b"} + for arm in cell["samples"].values(): + assert len(arm["wall"]) == cell["n_rounds"] + + def test_the_bake_off_is_in_the_artifact(self, tmp_path: Path): + doc = self.artifact(tmp_path, {REF: 1.0, "slow": 1.4, "quick": 1.0}) + wall = next(b for b in doc["bake_off"] if b["metric"] == "wall") + assert wall["winner"] == "quick" + + def test_both_directional_p_values_are_recorded(self, tmp_path: Path): + doc = self.artifact(tmp_path, {REF: 1.0, "cand": 0.7}) + cell = next(c for c in doc["cells"] if c["id"] == "encrypt") + wall = cell["contrasts"][f"cand_vs_{REF}"]["wall"] + assert wall["p_value"] is not None + assert wall["p_adjusted"] is not None + assert wall["p_value_faster"] is not None + assert wall["p_adjusted_faster"] is not None + + def test_the_file_is_valid_json_despite_nan(self, tmp_path: Path): + # A cell with no usable interval produces NaN, which `json.dumps` + # would happily write as a bare `NaN` that no strict parser accepts. + cfg = config(min_rounds=stats.MIN_USABLE_ROUNDS, max_rounds=40) + rec = recorder({REF: 1.0, "a": 1.0, "b": 1.0}, cfg=cfg, control=False) + path = report.write_json(tmp_path / "b.json", rec, cfg, rec.gate(cfg)) + json.loads(path.read_text()) # strict by default: no NaN accepted + + +class TestMarkdown: + def test_the_header_names_the_arm_count(self): + cfg = config(max_rounds=40) + rec = recorder({REF: 1.0, "a": 1.0, "b": 1.0}, cfg=cfg, noise=0.02) + md = report.markdown(rec, cfg, rec.gate(cfg)) + assert "3-arm" in md + + def test_the_table_names_both_sides_of_each_contrast(self): + cfg = config(max_rounds=40) + rec = recorder({REF: 1.0, "a": 1.0, "b": 1.3}, cfg=cfg, noise=0.02) + md = report.markdown(rec, cfg, rec.gate(cfg)) + assert "| contrast |" in md + assert "| `b` vs `a` |" in md, "the head-to-head is the point of a bake-off" + assert "| `a` vs `base` |" in md + + def test_a_bake_off_section_appears_only_with_candidates_to_rank(self): + cfg = config(max_rounds=40) + two = recorder({REF: 1.0, "cand": 1.3}, cfg=cfg, noise=0.02) + assert "### Bake-off" not in report.markdown(two, cfg, two.gate(cfg)) + three = recorder({REF: 1.0, "a": 1.0, "b": 1.3}, cfg=cfg, noise=0.02) + assert "### Bake-off" in report.markdown(three, cfg, three.gate(cfg)) + + def test_the_bottom_line_precedes_supporting_detail(self): + cfg = config(max_rounds=40) + rec = recorder({REF: 1.0, "cand": 1.35}, cfg=cfg, noise=0.01) + md = report.markdown(rec, cfg, rec.gate(cfg)) + + assert md.index("### TL;DR") < md.index("### Compared builds") + assert md.index("### Compared builds") < md.index("### What changed") + assert md.index("### What changed") < md.index("All measurements") + assert md.index("All measurements") < md.index("### Run facts") + + def test_provenance_links_releases_prs_commits_and_the_diff(self): + cfg = config(max_rounds=40) + rec = recorder({REF: 1.0, "cand": 1.35}, cfg=cfg, noise=0.01) + repo = "https://github.com/opentdf/platform" + rec.metadata = { + "github_run_url": "https://github.com/opentdf/tests/actions/runs/42", + "arm_sources": [ + { + "tag": REF, + "alias": "latest", + "release": "otdfctl/v0.40.0", + "sha": "a" * 40, + "repo_url": repo, + }, + { + "tag": "cand", + "alias": "feature", + "pr": 123, + "head": True, + "sha": "b" * 40, + "repo_url": repo, + }, + ], + } + md = report.markdown( + rec, + cfg, + rec.gate(cfg), + artifact_url="https://github.com/opentdf/tests/actions/runs/42/artifacts/7", + ) + + assert f"{repo}/releases/tag/otdfctl%2Fv0.40.0" in md + assert f"{repo}/pull/123" in md + assert f"{repo}/commit/{'b' * 40}" in md + assert f"{repo}/compare/{'a' * 40}...{'b' * 40}" in md + assert "actions/runs/42/artifacts/7" in md + assert "actions/runs/42" in md + assert "candidate A" in md + + def test_short_arm_names_are_links_and_controls_explain_the_same_build(self): + cfg = config(max_rounds=40) + rec = recorder({REF: 1.0, "cand": 1.0}, cfg=cfg, noise=0.01) + repo = "https://github.com/opentdf/platform" + rec.metadata = { + "arm_sources": [ + { + "tag": REF, + "release": "otdfctl/v0.37.0", + "sha": "a" * 40, + "repo_url": repo, + }, + { + "tag": "cand", + "pr": 321, + "sha": "b" * 40, + "repo_url": repo, + }, + ] + } + md = report.markdown(rec, cfg, rec.gate(cfg)) + + assert f"[`{REF}`]({repo}/releases/tag/otdfctl%2Fv0.37.0)" in md + assert f"[`cand`]({repo}/pull/321)" in md + assert "**A/A control** rows intentionally run multiple copies" in md + sources = report._sources_by_tag(rec.metadata) + assert report._linked_arm(f"{REF}#2", sources) == ( + f"[`{REF} copy 2`]({repo}/releases/tag/otdfctl%2Fv0.37.0)" + ) + assert "ratio b/a (95% CI)" in md + + def test_same_commit_is_neutral_and_explains_why_nothing_ran(self): + cfg = config(max_rounds=25) + rec = report.BenchmarkRecorder( + skipped={"java-encrypt-1KiB": "only one build installed"}, + metadata={ + "sdk": "java", + "comparison_status": "same_commit", + "comparison_note": ( + "main, latest resolve to the same commit 57d070b; " + "there is no code difference to benchmark." + ), + "requested_refs": ["main", "latest"], + "arm_sources": [ + { + "tag": "main", + "head": True, + "sha": "57d070b075a9134a11f8926b8898ac19b8cb6718", + "repo_url": "https://github.com/opentdf/java-sdk", + } + ], + }, + ) + gate = rec.gate(cfg) + md = report.markdown(rec, cfg, gate) + + assert gate.nothing_measured + assert report.same_commit(rec.metadata) + assert md.startswith("## JAVA SDK performance — SAME COMMIT") + assert "No performance comparison was necessary" in md + assert "every measured wall-clock" not in md + assert "### Compared builds" in md + assert "[`latest`](https://github.com/opentdf/java-sdk/tree/57d070" in md + + def test_the_primary_table_omits_clean_rows_but_the_full_table_keeps_them(self): + cfg = config(min_rounds=10, max_rounds=60) + rec = recorder({REF: 1.0, "clean": 1.0, "slow": 1.4}, cfg=cfg, noise=0.005) + md = report.markdown(rec, cfg, rec.gate(cfg)) + primary = md.split("### What changed", 1)[1].split("### Bake-off", 1)[0] + + assert "`slow` vs `base`" in primary + assert "`clean` vs `base`" not in primary + assert "`clean` vs `base`" in md + + def test_attention_rows_get_shared_scale_unicode_views(self): + cfg = config(max_rounds=40) + rec = recorder({REF: 1.0, "cand": 1.35}, cfg=cfg, noise=0.01) + md = report.markdown(rec, cfg, rec.gate(cfg)) + + assert "#### Effect at a glance" in md + assert "┆" in md and "│" in md and "●" in md + assert "Round stability for attention rows" in md + assert any("\u2800" <= char <= "\u28ff" for char in md) + + def test_extreme_effects_fit_and_name_their_candidate(self): + cfg = config(min_rounds=12, max_rounds=40) + rec = recorder({REF: 1.0, "epic": 0.02, "fast": 0.5}, cfg=cfg, noise=0.005) + md = report.markdown(rec, cfg, rec.gate(cfg)) + effect = md.split("#### Effect at a glance", 1)[1].split("### Bake-off", 1)[0] + + assert "candidate A" in effect and "candidate B" in effect + assert "-98.0%" in effect and "-49.7%" in effect + assert "◀" not in effect + assert "◆" in effect + + def test_run_facts_end_the_summary_with_reproducibility_context(self): + cfg = config(max_rounds=40, seed=91) + rec = recorder({REF: 1.0, "cand": 1.0}, cfg=cfg, noise=0.01) + rec.metadata = { + "platform_version": "v0.4.50", + "runner_os": "Linux", + } + md = report.markdown(rec, cfg, rec.gate(cfg)) + + assert "### Run facts" in md + assert "v0.4.50" in md and "Linux" in md + assert "seed 91" in md + assert md.rstrip().endswith("cells skipped.") + + def test_braille_trace_is_compact_and_deterministic(self): + values = [0.9, 1.0, 1.1, 1.2] * 20 + trace = report._braille_sparkline(values, 1.15) + + assert trace == report._braille_sparkline(values, 1.15) + assert len(trace) == 24 + assert all("\u2800" <= char <= "\u28ff" for char in trace) + + +class TestMarkdownTemplate: + def test_it_can_be_published_after_the_artifact_url_is_known(self, tmp_path: Path): + path = report.write_markdown( + tmp_path / "go.summary.md", + f"[evidence]({report.ARTIFACT_URL_PLACEHOLDER})\n", + ) + + assert report.ARTIFACT_URL_PLACEHOLDER in path.read_text() diff --git a/xtest/test_bench_runner.py b/xtest/test_bench_runner.py index ee0e7dd9b..2a53cf9ad 100644 --- a/xtest/test_bench_runner.py +++ b/xtest/test_bench_runner.py @@ -13,7 +13,7 @@ import math import random -from collections.abc import Callable +from collections.abc import Callable, Mapping from pathlib import Path import pytest @@ -27,6 +27,7 @@ BudgetExhausted, Invocation, analyze, + contrast_key, run_cell, ) @@ -34,14 +35,18 @@ BASELINE_RSS = 100_000_000 BASELINE_CPU = 0.8 +#: Arm id of the reference in every cell built here. +REF = "base" -def arm(role: str, key: str, output: Path | None = None) -> Arm: - """An arm whose argv is a single token, so ``FakeRuns`` can recognize it. - ``role`` is what the runner keys samples by ("baseline"/"candidate"); - ``key`` is the stand-in for the build. - """ - return Arm(role, f"sdk@{key}", Invocation([key], {}, output)) +def arm(name: str, output: Path | None = None) -> Arm: + """An arm whose argv is its own id, so ``FakeRuns`` can recognize it.""" + return Arm(name, f"sdk@{name}", Invocation([name], {}, output)) + + +def key(cell_id: str, metric: str, *, a: str = REF, b: str = "cand") -> str: + """The contrast key for ``b`` against ``a`` -- the default vs-reference.""" + return contrast_key(cell_id, a, b, metric) def config(**overrides: object) -> BenchConfig: @@ -108,7 +113,7 @@ def clock_from(runs: FakeRuns) -> Callable[[], float]: def run( - ratio: float, + ratios: float | Mapping[str, float], *, cfg: BenchConfig | None = None, noise: float = 0.05, @@ -118,14 +123,21 @@ def run( sdk: str = "", rss_floor: int = 0, ): - """Run one cell where the candidate costs ``ratio`` times the baseline.""" - runs = FakeRuns( - {"base": 1.0, "cand": ratio}, noise=noise, seed=seed, rss_floor=rss_floor + """Run one cell whose arms cost ``ratios`` times the reference. + + A bare float is the two-arm shorthand: a reference at 1.0 and one + candidate at that ratio. A mapping names the arms, and the first key is + the reference -- which is how a bake-off is set up here. + """ + costs = ( + {REF: 1.0, "cand": float(ratios)} + if isinstance(ratios, (int, float)) + else dict(ratios) ) + runs = FakeRuns(costs, noise=noise, seed=seed, rss_floor=rss_floor) result = run_cell( cell_id, - arm("baseline", "base"), - arm("candidate", "cand"), + [arm(name) for name in costs], cfg or config(), control=control, sdk=sdk, @@ -138,19 +150,47 @@ def run( class TestRoundLoop: def test_arms_are_paired_every_round(self): _, runs = run(1.0) - assert runs.calls.count("base") == runs.calls.count("cand") + assert runs.calls.count(REF) == runs.calls.count("cand") # Every consecutive pair holds one of each: that is what pairing means. pairs = [set(runs.calls[i : i + 2]) for i in range(0, len(runs.calls), 2)] - assert all(p == {"base", "cand"} for p in pairs) + assert all(p == {REF, "cand"} for p in pairs) + + def test_every_arm_runs_once_per_round_at_three_arms(self): + # The whole reason a bake-off is answerable: all three arms share a + # round, so the a-vs-b contrast is a within-round ratio like any other. + _, runs = run({REF: 1.0, "a": 1.1, "b": 1.2}) + rounds = [set(runs.calls[i : i + 3]) for i in range(0, len(runs.calls), 3)] + assert all(r == {REF, "a", "b"} for r in rounds) def test_order_within_rounds_is_shuffled(self): _, runs = run(1.0) firsts = runs.calls[::2] - assert "base" in firsts and "cand" in firsts, ( + assert REF in firsts and "cand" in firsts, ( "a fixed within-round order lets the second arm inherit the first " "one's cache state" ) + def test_all_three_arms_take_turns_going_first(self): + _, runs = run({REF: 1.0, "a": 1.0, "b": 1.0}) + assert set(runs.calls[::3]) == {REF, "a", "b"}, ( + "an arm pinned to one slot in the round inherits the same cache " + "state every time, which is a confounder and not a measurement" + ) + + def test_rejects_a_single_arm(self): + with pytest.raises(ValueError, match="at least two arms"): + run_cell("cell", [arm(REF)], config()) + + def test_rejects_colliding_arm_ids(self): + # Ids key the sample vectors, so a collision would interleave two + # builds' measurements into one arm and compare it with itself. + with pytest.raises(ValueError, match="unique"): + run_cell("cell", [arm(REF), arm(REF)], config()) + + def test_rejects_a_reference_that_is_not_an_arm(self): + with pytest.raises(ValueError, match="reference"): + run_cell("cell", [arm(REF), arm("cand")], config(), reference="other") + def test_warmup_rounds_are_discarded(self): cfg = config(warmup=3, max_rounds=stats.MIN_USABLE_ROUNDS) result, runs = run(1.0, cfg=cfg) @@ -173,10 +213,10 @@ def test_output_is_removed_before_each_run(self, tmp_path: Path): out = tmp_path / "out.tdf" out.write_bytes(b"stale") seen: list[bool] = [] - runs = FakeRuns({"base": 1.0, "cand": 1.0}) + runs = FakeRuns({REF: 1.0, "cand": 1.0}) def observe(argv: list[str], env: dict[str, str], **kwargs: object) -> Sample: - if argv[0] == "base": + if argv[0] == REF: # What the arm that owns this output sees when it starts. seen.append(out.exists()) out.write_bytes(b"produced") @@ -184,8 +224,7 @@ def observe(argv: list[str], env: dict[str, str], **kwargs: object) -> Sample: run_cell( "cell", - arm("baseline", "base", out), - arm("candidate", "cand"), + [arm(REF, out), arm("cand")], config(max_rounds=stats.MIN_USABLE_ROUNDS), clock=clock_from(runs), run=observe, @@ -194,10 +233,32 @@ def observe(argv: list[str], env: dict[str, str], **kwargs: object) -> Sample: def test_samples_are_collected_for_every_metric(self): result, _ = run(1.0) - for name in ("baseline", "candidate"): + for name in (REF, "cand"): for metric in ("wall", "cpu", "rss"): assert len(result.samples[name][metric]) == result.n_rounds + def test_records_its_arms_and_which_one_is_the_reference(self): + result, _ = run({REF: 1.0, "a": 1.0, "b": 1.0}) + assert result.arm_ids == (REF, "a", "b") + assert result.reference == REF + assert result.arm_labels == {REF: "sdk@base", "a": "sdk@a", "b": "sdk@b"} + + def test_contrast_pairs_cover_every_pair_once(self): + result, _ = run({REF: 1.0, "a": 1.0, "b": 1.0}) + # Reference contrasts first, then the head-to-head. A pair and its + # inverse are the same measurement, so only one of each appears. + assert result.contrast_pairs() == [(REF, "a"), (REF, "b"), ("a", "b")] + + def test_contrast_direction_is_b_over_a(self): + result, _ = run({REF: 1.0, "slow": 1.5}, noise=0.01) + cfg = config() + assert result.contrast(REF, "slow", "wall", cfg).ratio == pytest.approx( + 1.5, rel=0.1 + ) + assert result.contrast("slow", REF, "wall", cfg).ratio == pytest.approx( + 1 / 1.5, rel=0.1 + ) + class TestStopping: def test_stops_early_on_precision_when_quiet(self): @@ -215,13 +276,47 @@ def test_never_stops_before_min_rounds(self): result, _ = run(1.0, cfg=cfg, noise=0.0001) assert result.n_rounds >= 25 + def test_precision_waits_for_the_slowest_contrast_to_converge(self): + # One quiet candidate and one noisy one. Stopping as soon as *some* + # contrast is precise would leave the noisy arm unresolved with budget + # still on the table -- and at K arms the slowest contrast to converge + # is exactly the one someone is waiting on. + cfg = config(max_rounds=40) + quiet, _ = run({REF: 1.0, "cand": 1.0}, cfg=cfg, noise=0.005) + assert quiet.stopped_because == "precision" + + runs = FakeRuns({REF: 1.0, "quiet": 1.0, "noisy": 1.0}, noise=0.005) + real_call = runs.__call__ + + def jittery(argv: list[str], env: dict[str, str], **kwargs: object) -> Sample: + sample = real_call(argv, env, **kwargs) + if argv[0] != "noisy": + return sample + spike = math.exp(runs.rng.gauss(0.0, 0.5)) + return Sample( + wall_ns=int(sample.wall_ns * spike), + cpu_s=sample.cpu_s * spike, + max_rss_bytes=int(sample.max_rss_bytes * spike), + exit_code=0, + ) + + mixed = run_cell( + "cell", + [arm(REF), arm("quiet"), arm("noisy")], + cfg, + clock=clock_from(runs), + run=jittery, + ) + assert mixed.stopped_because == "max_rounds", ( + "the loop stopped on the quiet contrast and left the noisy one unresolved" + ) + def test_deadline_stops_the_loop(self): - runs = FakeRuns({"base": 1.0, "cand": 1.0}, noise=0.3) + runs = FakeRuns({REF: 1.0, "cand": 1.0}, noise=0.3) clock = clock_from(runs) result = run_cell( "cell", - arm("baseline", "base"), - arm("candidate", "cand"), + [arm(REF), arm("cand")], config(warmup=0, max_rounds=200), deadline=clock() + 60.0, # each round costs ~2 simulated seconds clock=clock, @@ -230,18 +325,34 @@ def test_deadline_stops_the_loop(self): assert result.stopped_because == "budget" assert result.elapsed_s <= 60.0, "a round we could not finish was started" + def test_a_three_arm_round_costs_three_invocations_of_budget(self): + # Rounds get more expensive as arms are added, which is the whole + # reason the default budget scales with K. + runs = FakeRuns({REF: 1.0, "a": 1.0, "b": 1.0}, noise=0.3) + clock = clock_from(runs) + result = run_cell( + "cell", + [arm(REF), arm("a"), arm("b")], + config(warmup=0, max_rounds=200), + deadline=clock() + 60.0, # each round now costs ~3 simulated seconds + clock=clock, + run=runs, + ) + assert result.stopped_because == "budget" + assert len(runs.calls) == 3 * result.n_rounds + assert result.n_rounds < 30, "three-arm rounds cost more than two-arm ones" + def test_warmup_gives_up_when_the_budget_runs_out(self): # The budget's end is absolute, so warm-ups that run past it are # spending the *following* cells' time -- and then reaching the # measured loop with nothing left, paying the whole cost of the cell # for no data at all. Stop at the deadline and say where it went. - runs = FakeRuns({"base": 1.0, "cand": 1.0}) + runs = FakeRuns({REF: 1.0, "cand": 1.0}) clock = clock_from(runs) with pytest.raises(BudgetExhausted, match="warm-up"): run_cell( "cell", - arm("baseline", "base"), - arm("candidate", "cand"), + [arm(REF), arm("cand")], config(warmup=10), deadline=clock() + 4.0, # each round costs ~2 simulated seconds clock=clock, @@ -250,13 +361,12 @@ def test_warmup_gives_up_when_the_budget_runs_out(self): assert len(runs.calls) < 2 * 10, "warm-up ran past its own deadline" def test_budget_below_min_usable_rounds_refuses_a_verdict(self): - runs = FakeRuns({"base": 1.0, "cand": 1.0}) + runs = FakeRuns({REF: 1.0, "cand": 1.0}) clock = clock_from(runs) with pytest.raises(BudgetExhausted, match="below the"): run_cell( "cell", - arm("baseline", "base"), - arm("candidate", "cand"), + [arm(REF), arm("cand")], config(warmup=0), deadline=clock() + 4.0, clock=clock, @@ -264,13 +374,12 @@ def test_budget_below_min_usable_rounds_refuses_a_verdict(self): ) def test_budget_below_configured_minimum_refuses_a_verdict(self): - runs = FakeRuns({"base": 1.0, "cand": 1.0}, noise=0.0) + runs = FakeRuns({REF: 1.0, "cand": 1.0}, noise=0.0) clock = clock_from(runs) with pytest.raises(BudgetExhausted, match="configured minimum of 10"): run_cell( "cell", - arm("baseline", "base"), - arm("candidate", "cand"), + [arm(REF), arm("cand")], config(min_rounds=10, warmup=0), # Five complete rounds fit. That clears the statistical hard # floor but not the configured minimum for this experiment. @@ -296,7 +405,7 @@ def scripted_run( # Five ordinary two-second rounds, then the first arm of round six # overruns what remains. That incomplete round must be discarded. elapsed[0] += 1.0 if len(calls) <= 10 else 3.0 - wall = 1.0 if argv[0] == "base" else next(candidate_walls) + wall = 1.0 if argv[0] == REF else next(candidate_walls) return Sample( wall_ns=int(wall * 1e9), cpu_s=wall, @@ -306,8 +415,7 @@ def scripted_run( result = run_cell( "cell", - arm("baseline", "base"), - arm("candidate", "cand"), + [arm(REF), arm("cand")], config(warmup=0, max_rounds=6), deadline=12.5, clock=lambda: elapsed[0], @@ -390,8 +498,8 @@ def gate(self, candidate_ratio: float, *, noise: float = 0.05, seed: int = 11): def test_planted_25_percent_slowdown_is_caught(self): gate = self.gate(1.25) assert gate.should_fail - assert "encrypt/wall" in gate.regressions - c = gate.comparisons["encrypt/wall"] + assert key("encrypt", "wall") in gate.regressions + c = gate.comparisons[key("encrypt", "wall")] assert c.verdict is stats.Verdict.REGRESSION assert c.ci_low > 1.15, "the interval must exclude the threshold, not just 1.0" assert c.ratio == pytest.approx(1.25, rel=0.1) @@ -399,7 +507,10 @@ def test_planted_25_percent_slowdown_is_caught(self): def test_planted_3_percent_slowdown_is_ignored(self): gate = self.gate(1.03) assert not gate.should_fail - assert gate.comparisons["encrypt/wall"].verdict is not stats.Verdict.REGRESSION + assert ( + gate.comparisons[key("encrypt", "wall")].verdict + is not stats.Verdict.REGRESSION + ) def test_no_effect_does_not_fire(self): gate = self.gate(1.0) @@ -409,8 +520,10 @@ def test_no_effect_does_not_fire(self): def test_planted_speedup_is_reported_not_failed(self): gate = self.gate(0.7) assert not gate.should_fail - assert gate.comparisons["encrypt/wall"].verdict is stats.Verdict.IMPROVED - assert "encrypt/wall" in gate.improvements + assert ( + gate.comparisons[key("encrypt", "wall")].verdict is stats.Verdict.IMPROVED + ) + assert key("encrypt", "wall") in gate.improvements def test_the_control_cell_never_fails_the_build(self): # Both arms of the control are the same build, so any verdict it @@ -418,22 +531,6 @@ def test_the_control_cell_never_fails_the_build(self): gate = self.gate(1.25) assert not any(k.startswith("aa/") for k in gate.regressions) - def test_control_only_run_measured_nothing_about_the_candidate(self): - cfg = config(max_rounds=40) - control, _ = run(1.0, cfg=cfg, cell_id="aa", control=True, sdk="go") - gate = analyze([control], cfg) - - assert gate.nothing_measured - assert "NOTHING MEASURED" in gate.summary - - def test_control_plus_candidate_counts_as_measured(self): - cfg = config(max_rounds=40) - control, _ = run(1.0, cfg=cfg, cell_id="aa", control=True, sdk="go") - measured, _ = run(1.0, cfg=cfg, cell_id="encrypt", sdk="go") - gate = analyze([control, measured], cfg) - - assert not gate.nothing_measured - def test_a_regression_in_an_ungated_metric_does_not_fail(self): cfg = config(max_rounds=40, gated_metrics=("wall",)) control, _ = run(1.0, cfg=cfg, cell_id="aa", control=True) @@ -441,9 +538,11 @@ def test_a_regression_in_an_ungated_metric_does_not_fail(self): gate = analyze([control, measured], cfg) # CPU time moved with everything else and is reported as such; it # simply is not allowed to turn the build red. - assert gate.comparisons["encrypt/cpu"].verdict is stats.Verdict.REGRESSION - assert "encrypt/cpu" not in gate.regressions - assert "encrypt/wall" in gate.regressions + assert ( + gate.comparisons[key("encrypt", "cpu")].verdict is stats.Verdict.REGRESSION + ) + assert key("encrypt", "cpu") not in gate.regressions + assert key("encrypt", "wall") in gate.regressions def test_rss_pinned_to_the_measurement_floor_cannot_report_pass(self): # A command whose peak sits at the floor is not measured, it is @@ -456,14 +555,14 @@ def test_rss_pinned_to_the_measurement_floor_cannot_report_pass(self): measured, _ = run(1.25, cfg=cfg, seed=12, cell_id="encrypt", rss_floor=floor) gate = analyze([control, measured], cfg) - rss = gate.comparisons["encrypt/rss"] + rss = gate.comparisons[key("encrypt", "rss")] assert rss.ratio == pytest.approx(1.0), "the floor clipped both arms" assert rss.verdict is stats.Verdict.INCONCLUSIVE assert "floor" in rss.note - assert "encrypt/rss" not in gate.regressions - assert "encrypt/rss" not in gate.improvements + assert key("encrypt", "rss") not in gate.regressions + assert key("encrypt", "rss") not in gate.improvements # Wall clock is untouched by a memory floor and still does its job. - assert "encrypt/wall" in gate.regressions + assert key("encrypt", "wall") in gate.regressions def test_rss_above_the_floor_is_still_gated(self): cfg = config(max_rounds=40) @@ -474,7 +573,7 @@ def test_rss_above_the_floor_is_still_gated(self): 1.25, cfg=cfg, seed=12, cell_id="encrypt", rss_floor=BASELINE_RSS // 10 ) gate = analyze([control, measured], cfg) - assert "encrypt/rss" in gate.regressions + assert key("encrypt", "rss") in gate.regressions def test_each_sdk_is_judged_against_its_own_control(self): # One control per SDK: they are different harness paths with different @@ -505,9 +604,10 @@ def test_each_sdk_is_judged_against_its_own_control(self): gate = analyze([go_aa, go_cell, java_aa, java_cell], cfg) assert len(gate.noise_by_control) == 2, "one noise floor per SDK" - assert gate.comparisons["go-encrypt/wall"].verdict is stats.Verdict.PASS + assert gate.comparisons[key("go-encrypt", "wall")].verdict is stats.Verdict.PASS assert ( - gate.comparisons["java-encrypt/wall"].verdict is stats.Verdict.INCONCLUSIVE + gate.comparisons[key("java-encrypt", "wall")].verdict + is stats.Verdict.INCONCLUSIVE ), "java's own control had no power, whatever go's control managed" def test_a_run_with_no_control_cannot_report_pass(self): @@ -515,5 +615,101 @@ def test_a_run_with_no_control_cannot_report_pass(self): measured, _ = run(1.0, cfg=cfg, cell_id="encrypt") gate = analyze([measured], cfg) assert gate.noise is not None and gate.noise.underpowered - assert gate.comparisons["encrypt/wall"].verdict is stats.Verdict.INCONCLUSIVE + assert ( + gate.comparisons[key("encrypt", "wall")].verdict + is stats.Verdict.INCONCLUSIVE + ) assert not gate.should_fail, "an unassessed run warns; it does not fail" + + +class TestGateAtThreeArms: + """What the extra arms buy, and what they must not be allowed to do. + + Every contrast here is a within-round ratio measured on one runner, which + is the only reason a candidate-versus-candidate question is answerable at + all. But only the vs-reference contrasts may fail a build: a bake-off + ranks implementations, it does not decide whether the branch is shippable. + """ + + def gate(self, costs: Mapping[str, float], *, noise: float = 0.05, seed: int = 11): + cfg = config(max_rounds=40) + control_costs = dict.fromkeys(costs, 1.0) + control, _ = run( + control_costs, cfg=cfg, noise=noise, seed=seed, cell_id="aa", control=True + ) + measured, _ = run(costs, cfg=cfg, noise=noise, seed=seed + 1, cell_id="encrypt") + return analyze([control, measured], cfg) + + def test_both_candidates_are_gated_against_the_reference(self): + gate = self.gate({REF: 1.0, "slow": 1.3, "quick": 1.0}) + assert gate.should_fail + assert key("encrypt", "wall", b="slow") in gate.regressions + assert key("encrypt", "wall", b="quick") not in gate.regressions + + def test_a_head_to_head_gap_never_fails_the_build(self): + # Neither candidate regressed against the reference; one is simply + # slower than the other. That is a ranking, and rankings do not turn + # the build red -- invariant #9. + gate = self.gate({REF: 1.3, "slow": 1.3, "quick": 1.0}) + h2h = key("encrypt", "wall", a="slow", b="quick") + assert gate.comparisons[h2h].verdict is stats.Verdict.FASTER + assert not gate.should_fail + assert h2h not in gate.regressions and h2h not in gate.improvements + + def test_a_head_to_head_is_judged_symmetrically(self): + # The one-sided vocabulary would call this PASS or REGRESSION, both of + # which presume an incumbent. Between two candidates there is none. + gate = self.gate({REF: 1.0, "a": 1.0, "b": 1.3}) + h2h = gate.comparisons[key("encrypt", "wall", a="a", b="b")] + assert h2h.verdict is stats.Verdict.SLOWER + assert h2h.verdict not in {stats.Verdict.PASS, stats.Verdict.REGRESSION} + + def test_indistinguishable_candidates_are_tied_not_passed(self): + # PASS is a one-sided claim -- "not slower". For a bake-off the answer + # worth reporting is that the two are the same, and TIED says so. + gate = self.gate({REF: 1.0, "a": 1.0, "b": 1.0}, noise=0.01) + assert ( + gate.comparisons[key("encrypt", "wall", a="a", b="b")].verdict + is stats.Verdict.TIED + ) + + def test_head_to_head_covers_ungated_metrics_too(self): + # "Which arm is faster" has no privileged direction on cpu either, and + # none of these gate, so they all share the symmetric vocabulary. + gate = self.gate({REF: 1.0, "a": 1.0, "b": 1.3}) + assert gate.comparisons[key("encrypt", "cpu", a="a", b="b")].verdict in { + stats.Verdict.FASTER, + stats.Verdict.SLOWER, + stats.Verdict.TIED, + stats.Verdict.INCONCLUSIVE, + } + + def test_a_decided_head_to_head_is_recorded_for_ranking(self): + gate = self.gate({REF: 1.0, "slow": 1.4, "quick": 1.0}) + # ``ranked`` carries the material a report turns into a winner; it is + # keys only, because at this layer an arm id is opaque. + assert key("encrypt", "wall", a="slow", b="quick") in gate.ranked + assert not any( + k in gate.regressions or k in gate.improvements for k in gate.ranked + ) + + def test_the_control_yields_one_contrast_per_pair(self): + gate = self.gate({REF: 1.0, "a": 1.0, "b": 1.0}) + aa = [ + k for k in gate.comparisons if k.startswith("aa/") and k.endswith("/wall") + ] + assert len(aa) == 3, "C(3,2) pairs, not one -- arm 3 drifts further than arm 2" + + def test_the_noise_floor_is_the_worst_control_pair(self): + # A 2-arm control measures adjacent invocations only, and so understates + # the drift carried by the widest contrast the run actually judges. + gate = self.gate({REF: 1.0, "a": 1.0, "b": 1.0}) + aa = [ + k for k in gate.comparisons if k.startswith("aa/") and k.endswith("/wall") + ] + assert len(gate.noise_by_control) == 1, "exactly one of the pairs is the floor" + assert gate.noise is not None + widest = max( + stats.assess_noise_floor(gate.comparisons[k]).width_ratio for k in aa + ) + assert gate.noise.width_ratio == pytest.approx(widest) diff --git a/xtest/test_bench_stats.py b/xtest/test_bench_stats.py index 71a94d9ae..b76e9caef 100644 --- a/xtest/test_bench_stats.py +++ b/xtest/test_bench_stats.py @@ -130,6 +130,7 @@ def test_identical_inputs_give_unit_ratio_and_no_significance(self): r = stats.compare(v, v, seed=0, n_resamples=RESAMPLES) assert r.ratio == pytest.approx(1.0) assert r.p_value == 1.0 + assert r.p_value_faster == 1.0 def test_constant_offset_has_degenerate_interval(self): # Every round shows exactly a 2x slowdown: there is no sampling @@ -185,7 +186,10 @@ def test_planted_speedup_is_reported_but_never_fails(self): g = gate_one( stats.compare(b, c, seed=12, n_resamples=RESAMPLES), control=quiet_control() ) - assert g.comparisons["cell"].verdict is Verdict.IMPROVED + result = g.comparisons["cell"] + assert result.verdict is Verdict.IMPROVED + assert result.p_adjusted_faster is not None + assert result.p_adjusted_faster < stats.DEFAULT_ALPHA assert not g.should_fail def test_borderline_effect_without_power_is_inconclusive_not_pass(self): @@ -301,6 +305,26 @@ def test_bh_passes_nan_through(self): assert math.isnan(adj[1]) assert all(math.isfinite(a) for a in (adj[0], adj[2])) + def test_faster_tail_is_adjusted_directly(self): + cells = {} + for i, ratio in enumerate((0.70, 0.75, 0.80)): + rng = np.random.default_rng(2900 + i) + b, c = synth(rng, ratio, n=60) + cells[f"cell{i}"] = stats.compare(b, c, seed=i, n_resamples=RESAMPLES) + cells["control"] = quiet_control() + + g = stats.apply_multiplicity_control( + cells, controls=all_under_one_control(cells) + ) + expected = stats.benjamini_hochberg( + [cells[f"cell{i}"].p_value_faster for i in range(3)] + ) + actual = [g.comparisons[f"cell{i}"].p_adjusted_faster for i in range(3)] + assert actual == pytest.approx(expected) + assert all( + g.comparisons[f"cell{i}"].verdict is Verdict.IMPROVED for i in range(3) + ) + def test_correction_suppresses_lone_lucky_cell(self): # 20 pure-noise cells: without BH one of them firing is expected. cells = {} @@ -338,6 +362,51 @@ def test_ungated_metric_is_reported_but_cannot_fail(self): assert g.comparisons["cpu"].verdict is Verdict.REGRESSION assert g.regressions == ["wall"], "cpu is reported but never gates" + def test_the_three_families_are_corrected_separately(self): + # A bake-off adds head-to-head contrasts that cannot fail the build. + # Folding them into the gate's family would raise every gated p-value + # for the sake of tests nobody gates on, which is precisely the trade + # the gated/ungated split already refuses to make. + rng = np.random.default_rng(41) + b, c = synth(rng, 1.4, n=60) + regressed = stats.compare(b, c, seed=41, n_resamples=RESAMPLES) + cells = {"wall": regressed, "control": quiet_control()} + alone = stats.apply_multiplicity_control( + cells, gated={"wall"}, controls=all_under_one_control(cells) + ) + + crowded = dict(cells) + for i in range(10): + r = np.random.default_rng(4100 + i) + x, y = synth(r, 1.0, n=60) + crowded[f"h2h{i}"] = stats.compare(x, y, seed=i, n_resamples=RESAMPLES) + with_h2h = stats.apply_multiplicity_control( + crowded, + gated={"wall"}, + symmetric={f"h2h{i}" for i in range(10)}, + controls=all_under_one_control(crowded), + ) + assert with_h2h.comparisons["wall"].p_adjusted == pytest.approx( + alone.comparisons["wall"].p_adjusted + ), "ten head-to-heads must not dilute the one contrast that can gate" + assert with_h2h.regressions == ["wall"] + + def test_a_symmetric_key_that_is_also_gated_stays_gated(self): + # Only reachable through a caller bug, and the safe resolution is the + # rule that can still fail the build rather than the one that cannot. + rng = np.random.default_rng(42) + b, c = synth(rng, 1.4, n=60) + cells = {"wall": stats.compare(b, c, seed=42, n_resamples=RESAMPLES)} + cells["control"] = quiet_control() + g = stats.apply_multiplicity_control( + cells, + gated={"wall"}, + symmetric={"wall"}, + controls=all_under_one_control(cells), + ) + assert g.comparisons["wall"].verdict is Verdict.REGRESSION + assert g.regressions == ["wall"] + def test_empty_run_reports_no_regressions(self): g = stats.apply_multiplicity_control({}) assert not g.should_fail, "nothing measured is not a regression" @@ -359,3 +428,121 @@ def test_a_run_with_comparisons_measured_something(self): stats.compare(b, c, seed=33, n_resamples=RESAMPLES), control=quiet_control() ) assert not g.nothing_measured + + +class TestSymmetricVerdicts: + """The bake-off rule: which of two candidates is faster, if either. + + Neither arm is an incumbent, so the one-sided vocabulary does not apply. + PASS would let a slower arm read as a clean result, and REGRESSION would + imply the other arm was the thing that changed. + """ + + def h2h( + self, + true_ratio: float, + *, + n: int = 60, + seed: int = 51, + sigma: float = NOISE_SIGMA, + ): + rng = np.random.default_rng(seed) + a, b = synth(rng, true_ratio, n=n, sigma=sigma) + cells = { + "h2h": stats.compare(a, b, seed=seed, n_resamples=RESAMPLES), + "control": quiet_control(), + } + g = stats.apply_multiplicity_control( + cells, + gated=set(), + symmetric={"h2h"}, + controls=all_under_one_control(cells), + ) + return g, g.comparisons["h2h"] + + def test_a_clearly_slower_arm_is_slower(self): + _, c = self.h2h(1.5) + assert c.verdict is Verdict.SLOWER + + def test_a_clearly_faster_arm_is_faster(self): + _, c = self.h2h(1 / 1.5) + assert c.verdict is Verdict.FASTER + assert c.p_adjusted_faster is not None + assert c.p_adjusted_faster < stats.DEFAULT_ALPHA + + def test_indistinguishable_arms_are_tied(self): + # A positive finding, and the most likely honest answer for two + # implementations of the same idea. Not PASS: that is a one-sided + # claim about an incumbent that does not exist here. + _, c = self.h2h(1.0, n=120, sigma=0.02) + assert c.verdict is Verdict.TIED + assert 1 / 1.15 < c.ci_low and c.ci_high < 1.15 + + def test_an_interval_straddling_the_band_edge_is_inconclusive(self): + # A real but unresolved difference. Calling it TIED would claim an + # equivalence the interval does not support. + _, c = self.h2h(1.15, n=20, sigma=0.15) + assert c.verdict is Verdict.INCONCLUSIVE + assert "cannot be ranked" in c.note + + def test_a_head_to_head_never_gates(self): + g, c = self.h2h(1.5) + assert c.verdict is Verdict.SLOWER + assert not g.should_fail, "a bake-off ranks; it does not fail the build" + assert g.regressions == [] and g.improvements == [] + + def test_a_decided_head_to_head_is_ranked(self): + g, _ = self.h2h(1.5) + assert g.ranked == ["h2h"] + + def test_a_tie_is_not_ranked(self): + # Nothing to rank: naming a winner from a tie is the failure mode this + # verdict exists to prevent. + g, c = self.h2h(1.0, n=120, sigma=0.02) + assert c.verdict is Verdict.TIED + assert g.ranked == [] + + def test_without_a_noise_floor_nothing_is_ranked(self): + # Invariant 4 covers TIED as much as PASS: a tie nobody had the power + # to tell from a difference is not a tie. + rng = np.random.default_rng(52) + a, b = synth(rng, 1.0, n=120, sigma=0.02) + cells = {"h2h": stats.compare(a, b, seed=52, n_resamples=RESAMPLES)} + g = stats.apply_multiplicity_control(cells, gated=set(), symmetric={"h2h"}) + assert g.comparisons["h2h"].verdict is Verdict.INCONCLUSIVE + assert g.ranked == [] + + +class TestWorstControlKey: + """A K-arm control yields C(K,2) A/A contrasts, and they are not equal. + + Arm 3 runs two invocations after arm 1, so it carries more within-round + drift. Taking whichever came first would let dict ordering decide how + noisy the run is allowed to look. + """ + + def controls(self) -> dict[str, stats.PairedComparison]: + rng = np.random.default_rng(61) + tight_b, tight_c = synth(rng, 1.0, n=120, sigma=0.02) + wide_b, wide_c = synth(rng, 1.0, n=25, sigma=0.25) + return { + "tight": stats.compare(tight_b, tight_c, seed=61, n_resamples=RESAMPLES), + "wide": stats.compare(wide_b, wide_c, seed=62, n_resamples=RESAMPLES), + } + + def test_the_widest_contrast_is_the_floor(self): + c = self.controls() + assert stats.worst_control_key(c, ["tight", "wide"]) == "wide" + + def test_the_answer_does_not_depend_on_input_order(self): + c = self.controls() + assert stats.worst_control_key(c, ["wide", "tight"]) == "wide" + + def test_a_missing_contrast_is_worse_than_any_real_one(self): + # An absent control is not a quiet one; `assess_noise_floor(None)` + # calls it underpowered, and that must win over a real interval. + c = self.controls() + assert stats.worst_control_key(c, ["tight", "absent"]) == "absent" + + def test_no_keys_means_no_floor(self): + assert stats.worst_control_key({}, []) is None diff --git a/xtest/test_benchmarks.py b/xtest/test_benchmarks.py index 0e4197455..e7b603389 100644 --- a/xtest/test_benchmarks.py +++ b/xtest/test_benchmarks.py @@ -1,7 +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 +One test per cell: an operation at a payload size, measuring every arm of the +run -- by default the newest installed release against the branch build, and +in a bake-off several candidates at once -- 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. @@ -66,7 +67,7 @@ def bail(reason: str) -> NoReturn: if bench_cell.operation == "decrypt" else None ) - baseline, candidate = bench.build_arms( + cell_arms = bench.build_arms( bench_cell, arms, pt_file=bench_payloads[bench_cell.payload.label], @@ -78,8 +79,7 @@ def bail(reason: str) -> NoReturn: try: result = runner.run_cell( bench_cell.id, - baseline, - candidate, + cell_arms, bench_config, deadline=bench_budget.next_deadline(), control=bench_cell.control,