diff --git a/.github/workflows/bench.yml b/.github/workflows/bench.yml new file mode 100644 index 00000000..2360b18f --- /dev/null +++ b/.github/workflows/bench.yml @@ -0,0 +1,194 @@ +name: Launch benchmark + +# The launch-latency trend (map #193). Every merge to main benches the two +# reproducible launch shapes, publishes one median point per stage, and alerts +# on a large relative regression. +# +# ALERT, DO NOT GATE. This workflow must never become a required check and must +# never fail a build on a slow point. One quiet host's five identical +# cold-recreate runs spread 15.2-20.0s; a shared runner is worse, so a hard +# wall-clock gate would be either too loose to fire or flaky. Deterministic +# structural guards (test_devpod_spawn_counts.py and its kin) keep the gating +# role, and they live in ci.yml where a gate belongs. +# +# It is a separate file for exactly that reason: house convention is that a new +# job in ci.yml joins `gate`'s `needs` in the same pull request, and `needs` +# cannot reach across files. A job in there would have become a merge gate by +# convention, which is the thing the charter grilled out. `test_bench_workflow.py` +# holds this and the other charter properties down. +on: + push: + branches: [main] + workflow_dispatch: + inputs: + alert-threshold: + description: >- + Relative regression that alerts, as a percentage of the previous + point. Lower it on a dispatch to watch the alert actually fire -- + an alert nobody has ever seen fire is worth nothing -- without + committing a temporary threshold to main. + default: '150%' + required: false + +# Deliberately no `paths:` filter, and this is not an omission: a launch can be +# slowed by any file in the repo, and a trend with holes in it cannot compare a +# point against its own previous point. + +concurrency: + # Serialise. The publishing action commits to gh-pages, so two runs race on + # that push -- it retries, but not racing is cheaper -- and two benches + # sharing a runner class would also be measuring each other. + group: launch-benchmark + cancel-in-progress: false + +permissions: + # One permission covers both halves of publishing: the push to gh-pages and + # the alert's commit comment. The repository default is read-only. + contents: write + +jobs: + bench: + runs-on: ubuntu-latest + # Generous, because the priming step below builds a devcontainer image from + # scratch on every run. Bounded, because an unbounded job on a registry + # serving at 640 B/s hangs for six hours and reports a timeout nobody reads. + timeout-minutes: 45 + env: + # The benched repo. This is an interface, not a detail: the trend's whole + # meaning is "the same launch, over time", so changing this repo makes + # every existing point incomparable with every later one. Change it the + # way a stage name is changed -- deliberately, knowing the line breaks. + # + # Its own devcontainer builds an image and runs `pixi install` in + # postCreate, both of which the cold-recreate shape pays for. That is the + # first thing to look at if the cold trend proves noisier than the alert + # threshold can live with. + TARGET: blooop/mcp-devtasks + # Every byte this job writes, scoped away from anything else on the + # runner: DEVPOD_HOME (plus DEVPOD_SSH_CONFIG, which `devpod up` writes + # outside that home) is devpod's own state, XDG_CACHE_HOME is + # devlaunch's clone cache and bookkeeping. The runner is ephemeral, so + # /tmp is the same scratch recipe the bench script's epilog documents. + DEVPOD_HOME: /tmp/dl-bench/devpod + DEVPOD_SSH_CONFIG: /tmp/dl-bench/ssh_config + XDG_CACHE_HOME: /tmp/dl-bench/cache + # dl forwards a GitHub token into every workspace it opens, via + # `gh auth token` -- which answers from GH_TOKEN when one is set. Without + # this the host-prep stage times a token fetch that fails. + GH_TOKEN: ${{ github.token }} + steps: + - name: Checkout + uses: actions/checkout@v6 + + # No devpod install step: devpod is a pixi dependency, already in the + # lockfile setup-pixi restores, and it is the version this tree is pinned + # against rather than whatever a runner image happens to carry. + - uses: prefix-dev/setup-pixi@v0.9.6 + with: + cache: true + frozen: true + environments: default + + - name: Let dl's git reach GitHub without an SSH key + # dl clones and probes over `git@github.com:`, and a runner has no key + # for that. The rewrite is the standard CI substitution and touches + # only what git resolves -- devpod never talks to GitHub here, because + # the worktree backend hands it a local path. + run: | + git config --global \ + url."https://x-access-token:${{ github.token }}@github.com/".insteadOf \ + "git@github.com:" + + - name: Register the docker provider in this job's devpod home + # A fresh DEVPOD_HOME has no provider at all, so this is not optional + # setup -- it is what makes the home usable. Docker itself is already + # on the runner. + run: pixi run dev-add-docker + + - name: Prime the image and the clone cache + # THE FIRST-EVER-COLD EXCLUSION, and it is deliberate rather than + # redundant with the benched runs below. Charter decision 4 keeps the + # first-ever cold launch out of the per-merge trend: its dominant cost + # is an image transfer or build this repo does not control, and the + # image build's own duration is already visible in the target's own + # workflows. An ephemeral runner starts every job in that state, so + # without this step the first timed cold run would be a first-ever cold + # one and the trend would carry a number about a registry's day. + # + # It also leaves the workspace up, which is what makes the next step + # warm. + run: pixi run dl "$TARGET" -- true + + # Five runs per shape, both shapes. The bench script's own recipe says + # five, and the point published is their MEDIAN -- the trend's baseline is + # the immediately previous point and nothing older, so de-noising has to + # happen here or the alert is a coin flip. Three would save about a minute + # on a job that spends longer than that priming, and would halve the + # median's resistance to one slow run: the exact noise this whole map is + # about. + - name: Bench the warm shape + run: pixi run bench -n 5 --record warm.json --shape warm -- dl "$TARGET" -- true + + - name: Bench the cold-recreate shape + # `--before` re-establishes the cold state before EVERY run: delete once + # and bench five times and runs 2..5 are warm, i.e. a warm median under + # a cold label. What it removes is the workspace and its container; the + # image layers and the bare clone the priming step left behind survive + # on purpose, which is what makes this "recreate" rather than "first + # ever". + run: | + pixi run bench -n 5 --record cold-recreate.json --shape cold-recreate \ + --before "dl $TARGET rm --force" -- dl "$TARGET" -- true + + - name: Turn the records into this commit's trend points + # `--require-stages-on cold-recreate` is the load-bearing flag: a cold + # recreate that reported no `tools` stage means the launch changed shape + # or stopped saying so, and this step then fails the JOB and publishes + # nothing -- rather than letting a stage go quietly absent from a chart + # nobody reads that week. The warm shape is not asserted: it legitimately + # lends nothing, and requiring `tools` of it would fail every green run. + run: | + pixi run python scripts/bench_points.py warm.json cold-recreate.json \ + --out bench.json --require-stages-on cold-recreate \ + --note "$(pixi run -q devpod version | tr -d '\r\n'), ${{ runner.os }}-${{ runner.arch }}" + + - name: Publish the point and alert on a regression + uses: benchmark-action/github-action-benchmark@v1.22.1 + with: + name: devlaunch launch stages + tool: customSmallerIsBetter + output-file-path: bench.json + github-token: ${{ secrets.GITHUB_TOKEN }} + auto-push: true + gh-pages-branch: gh-pages + benchmark-data-dir-path: dev/bench + # One global relative threshold to start with; per-stage thresholds are + # parked until there is false-alarm experience to tune them against. + alert-threshold: ${{ inputs.alert-threshold || '150%' }} + comment-on-alert: true + # The point of the entire map. `fail-threshold` only applies when this + # is true, so leaving it unset is safe. + fail-on-alert: false + # On a push there is no pull request, so the alert is a COMMIT comment + # -- and commit comments notify almost nobody. This @-mention is what + # actually produces a notification; without it the alert is invisible. + alert-comment-cc-users: '@blooop' + # The comparison table lands in every run's job summary, so "did it + # move" is answerable without opening the chart or waiting for a breach. + summary-always: true + # Unset means unbounded growth of the published data file. + max-items-in-chart: 500 + + - name: Keep the runs behind the medians + # The trend carries medians; this carries the spread that produced them, + # for digging after an alert. `always()` because a job that failed its + # stage assertion is exactly the one whose records somebody wants. + if: always() + uses: actions/upload-artifact@v7 + with: + name: bench-records + path: | + warm.json + cold-recreate.json + bench.json + if-no-files-found: warn diff --git a/README.md b/README.md index 3e8e6504..698b7468 100644 --- a/README.md +++ b/README.md @@ -942,6 +942,50 @@ things about it are load-bearing: caller's to say: the same command benches either shape depending on `--before`, and a wrong label is worse in a trend than a missing one. +### The trend on main + +Every push to `main` runs `.github/workflows/bench.yml`, which benches both +shapes on the runner and publishes one point per stage to +. It can also be dispatched by +hand. Reading it needs nothing but the chart; what follows is for changing it. + +`scripts/bench_points.py` is the step between the two formats — bench records +in, one flat array of trend cases out: + +```bash +python3 scripts/bench_points.py warm.json cold-recreate.json --out bench.json \ + --require-stages-on cold-recreate +``` + +(`pixi run bench-points ...` in the devcontainer.) One case per stage the shape +reported, plus that shape's own total: `warm / host-prep`, +`cold-recreate / devpod-up`, `warm / total`. Five properties of it are +load-bearing: + +- **The published value is the median, and the case name is a key.** The trend + compares a point against the immediately previous point and nothing older, so + the de-noising has to have happened before publishing. Renaming a stage + starts a new, empty series beside a frozen old one. +- **The spread rides along as the point's error bar**, and the outside + stopwatch (`wall=`) as part of its `extra`. Evidence beside the number, + rather than a second trend line for the same launch that disagrees with the + first by a constant. +- **An absent stage is absent.** A warm launch lends nothing, so there is no + `warm / tools` case at all — never a zero, which would claim an instantaneous + lend and would drag the line down exactly where a regression should show. +- **`--require-stages-on` fails the job rather than the trend.** The way this + decomposition is expected to break is an absence, not a wrong number: a stage + stops being emitted and the total keeps working. So the cold-recreate shape, + where every stage is known to be present, asserts them all, and a run that + lost one publishes nothing and goes red. Naming a shape that was not benched + fails too — an assertion that covers nothing reads exactly like one that + passed. +- **A regression alerts; it never gates.** The workflow is deliberately not a + job in `ci.yml`: a job there would join the CI gate's `needs` by house + convention and turn a noisy wall-clock measurement into a merge gate. A point + above the threshold leaves a commit comment and a red mark on the chart, and + the build stays green. + ## Worktree Backend For git repositories, devlaunch uses an efficient worktree backend by default: diff --git a/pyproject.toml b/pyproject.toml index be052ad4..93de7179 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -92,6 +92,10 @@ commit-format = "git commit -a -m'autoformat code' || true" test = "pytest" # Median wall time of a command over N runs; see "Measuring launch time" in README.md. bench = "python scripts/bench_launch.py" +# The step between a bench record and the published trend: see "The trend on +# main" in README.md. The bench workflow runs it; it is a task so that the same +# command can be run by hand against records a developer benched themselves. +bench-points = "python scripts/bench_points.py" # Needs a real Docker daemon and a real devpod. Inside this repo's devcontainer # that daemon is the container's own, via the docker-in-docker feature; on an # ephemeral CI runner or a machine you do not mind writing to, it is the host's. diff --git a/scripts/bench_points.py b/scripts/bench_points.py new file mode 100755 index 00000000..43c681c3 --- /dev/null +++ b/scripts/bench_points.py @@ -0,0 +1,269 @@ +#!/usr/bin/env python3 +"""Turn bench records into the trend's points (ticket #198). + +`bench_launch.py --record` writes one JSON object per bench invocation; the +trend (`benchmark-action/github-action-benchmark`, ticket #197) reads a flat +array of `{name, unit, value}`. This is the one step between them, and it is +deliberately the only place that knows both shapes. + +Stdlib only, like its sibling: the instrument must not cost the project a +dependency. See --help for what it refuses to publish. +""" + +import argparse +import json +import statistics +import sys +from typing import Any, Dict, List, Optional + +EPILOG = """\ +bench_points.py warm.json cold-recreate.json --out bench.json + +CI: bench_points.py warm.json cold-recreate.json --out bench.json \\ + --require-stages-on cold-recreate --note 'devpod 0.26.1, ubuntu-latest' + +One case per stage each record's shape reported, plus that shape's `total`: +`warm / host-prep`, `cold-recreate / devpod-up`, `warm / total`. The case name +is the trend's key, so it is an interface -- renaming a stage starts a new, +empty series next to a frozen old one. + +The value is the record's MEDIAN, never a run: the trend compares a point +against the immediately previous point and nothing older, so N runs published +as N points would read ordinary spread as a regression. The spread those runs +had rides along as the point's `range`, and the outside stopwatch as part of +`extra` -- evidence beside the number rather than a second trend line. + +Two things it refuses to publish, both of them absences that would otherwise +reach a chart looking like measurements: + + - A stage no run reported gets no point, never a zero. A warm launch lends + no tools, and a zero there claims an instantaneous lend. + - With --require-stages-on, a shape that lost a stage fails the JOB and + writes no file at all. Point it at the shape where every stage is known + to be present: a cold recreate that reports no `tools` means the launch + changed shape, and a red job says so where a flattening trend line does + not. Naming a shape that was not benched is itself a failure, for the + same reason: an assertion that covers nothing reads like one that passed. +""" + +UNIT = "s" + +# The clock the stages add up to. The record carries both this and the outside +# stopwatch; publishing both as cases would be two trend lines for one launch +# that disagree by a constant, so the other one rides in `extra` instead. +TOTAL = "total" + +# What a shape under `--require-stages-on` must have reported, from every run. +# +# This is `devlaunch.timing.STAGES` minus `handoff`, written out rather than +# imported for the same reason its sibling writes out the timing variables: +# the script stays stdlib-only and runnable from a checkout that was never +# installed. `test_bench_points.py` pins the two together, so a rename in the +# vocabulary fails there rather than quietly asserting a stage nothing emits. +# +# `handoff` is excluded because nothing hands off to dl on a runner: it is the +# gap before dl started, stamped by whoever launched it, and requiring it would +# fail every green CI run. It is also the one stage that lies outside `total`. +REQUIRED_STAGES = ("host-prep", "devpod-up", "tools", "attach") + + +def points_of(record: Dict[str, Any], note: Optional[str] = None) -> List[Dict[str, Any]]: + """The trend points *record* makes, in the record's own stage order. + + A stage the record does not carry produces no point at all. That is #195's + rule and it is the property the whole trend rests on: a warm launch lends + no tools, and a zero there would claim the lend happened instantly rather + than not at all -- indistinguishable, in a chart, from a lend that got + faster. + """ + shape = shape_of(record) + median = record["median"] + runs = record["runs"] + total_runs = len(runs) + points = [ + point( + name=f"{shape} / {stage}", + value=summary["seconds"], + samples=[run["stages"][stage] for run in runs if stage in run["stages"]], + extra=f"runs={summary['runs']}/{total_runs}", + note=note, + ) + for stage, summary in median["stages"].items() + ] + points.append( + point( + name=f"{shape} / {TOTAL}", + value=median["total_seconds"], + samples=[run["total_seconds"] for run in runs], + extra=f"runs={total_runs}/{total_runs} wall={median['wall_seconds']}s", + note=note, + ) + ) + return points + + +class Refused(Exception): + """Why nothing is being published. + + Raised rather than returned so that every refusal takes the same exit -- + a message and no output file, the discipline `bench_launch.py` already + holds to for a failed run. Half a trend point written before the reason to + refuse was found would be worse than none. + """ + + +def shape_of(record: Dict[str, Any]) -> str: + """The trend line this record belongs to, or a refusal. + + #196 refuses to guess a shape from the runs -- the same command benches + warm or cold depending on a reset it cannot interpret -- so an unlabelled + record has no case name to publish under, and inventing one here would put + the guess back in at the last possible moment. + """ + shape = record.get("shape") + if not shape: + raise Refused( + "this record carries no shape, so its points have no trend line to " + "belong to; re-run the bench with --shape" + ) + return str(shape) + + +def check_one_record_per_shape(records: List[Dict[str, Any]]) -> None: + """Refuse two records of the same shape. + + The case name is the trend's key, so two `warm` records publish two points + per case for one commit and the chart has no way to say which one the + commit's point is. + """ + seen = set() + for record in records: + shape = shape_of(record) + if shape in seen: + raise Refused( + f"two records both labelled {shape!r}: the shape names the trend line, " + "so publishing both would put two points per case on one commit" + ) + seen.add(shape) + + +def check_required(records: List[Dict[str, Any]], shapes: List[str], required: List[str]) -> None: + """Refuse unless every named shape reported every required stage. + + The expected way this decomposition breaks is an absence, not a wrong + number: dl stops entering an arm, or devpod renames what it prints, and a + stage quietly stops appearing while the total keeps working. So the shape + where every stage is known to be present asserts them, and a run that lost + one fails the *job* -- publishing nothing at all, rather than a trend line + that flattens where nobody is looking. + + A named shape that was not benched is itself a refusal, for the reason + this repo's CI gate already gives about covering nothing: an assertion + that matched no shape reads exactly like one that passed. + """ + benched = {shape_of(record): record for record in records} + for shape in shapes: + record = benched.get(shape) + if record is None: + raise Refused( + f"--require-stages-on names {shape!r}, which no record here benched " + f"(benched: {', '.join(sorted(benched)) or 'nothing'}); an assertion " + "that covers nothing reads exactly like one that passed" + ) + runs = len(record["runs"]) + stages = record["median"]["stages"] + for stage in required: + reported = stages.get(stage, {}).get("runs", 0) + if reported < runs: + raise Refused( + f"the {shape!r} shape reported {stage!r} in {reported} of its {runs} " + "runs, and it is required in all of them -- so the launch changed " + "shape or the stage stopped being emitted. Publishing nothing: a " + "trend line that goes flat is harder to notice than a red job" + ) + + +def point( + name: str, value: float, samples: List[float], extra: str, note: Optional[str] +) -> Dict[str, Any]: + """One case in the trend's own shape. + + *samples* are the runs the median is of, and they are here for the error + bar alone. Two of them are the fewest there is a spread of, so a single + run publishes no range rather than a `± 0` claiming a repeatability + nothing measured. + """ + published: Dict[str, Any] = {"name": name, "unit": UNIT, "value": value} + if len(samples) > 1: + published["range"] = f"± {round(statistics.stdev(samples), 6)}" + published["extra"] = f"{extra} {note}" if note else extra + return published + + +def write_points(path: str, points: List[Dict[str, Any]]) -> None: + with open(path, "w", encoding="utf-8") as handle: + # The `±` in a range stays a `±` rather than an escape: this file is + # uploaded as an artifact and read by a human after an alert. + json.dump(points, handle, indent=2, ensure_ascii=False) + handle.write("\n") + + +def build_parser() -> argparse.ArgumentParser: + """The command line, built where a test can also ask it what it accepts.""" + parser = argparse.ArgumentParser( + description="Turn bench records into the trend's points.", + epilog=EPILOG, + formatter_class=argparse.RawDescriptionHelpFormatter, + allow_abbrev=False, + ) + parser.add_argument("records", nargs="+", metavar="RECORD", help="bench records to publish") + parser.add_argument("--out", required=True, metavar="PATH", help="where to write the points") + parser.add_argument( + "--note", + metavar="TEXT", + help="appended to every point's `extra` (e.g. the devpod version and the runner)", + ) + parser.add_argument( + "--require-stages-on", + metavar="SHAPE", + action="append", + default=[], + dest="require_stages_on", + help=( + "publish nothing unless this shape reported every required stage in every " + "run; name the shape where they are all known to be present (cold-recreate)" + ), + ) + parser.add_argument( + "--require-stages", + metavar="A,B", + default=",".join(REQUIRED_STAGES), + help=f"what --require-stages-on requires (default: {','.join(REQUIRED_STAGES)})", + ) + return parser + + +def main(argv=None) -> int: + parser = build_parser() + args = parser.parse_args(argv) + records = [] + for path in args.records: + with open(path, encoding="utf-8") as handle: + records.append(json.load(handle)) + try: + check_one_record_per_shape(records) + check_required( + records, args.require_stages_on, [s for s in args.require_stages.split(",") if s] + ) + published: List[Dict[str, Any]] = [] + for record in records: + published.extend(points_of(record, note=args.note)) + except Refused as refusal: + print(f"bench_points: {refusal}", file=sys.stderr) + return 1 + write_points(args.out, published) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/test/test_bench_points.py b/test/test_bench_points.py new file mode 100644 index 00000000..07a991f0 --- /dev/null +++ b/test/test_bench_points.py @@ -0,0 +1,376 @@ +"""The trend points a bench record makes (#198). + +`bench_launch.py --record` (#196) writes one JSON object per bench invocation; +`benchmark-action/github-action-benchmark` (#197) reads a flat array of +`{name, unit, value, range, extra}`. This is the seam between them, and it is +where every rule the two blockers settled actually has teeth: the median is the +point, an absent stage is absent rather than zero, and a stage that has gone +missing on the shape it is known to appear on fails the *job* rather than +publishing a hole in the trend. + +Records here are written as literals, medians included, rather than computed by +running a bench: what is under test is the mapping, and a test that recomputed +the median the way the converter does would assert nothing about it. +""" + +import importlib.util +import json +import re +import shlex +import subprocess +import sys +from pathlib import Path +from typing import Any, Dict + +POINTS = Path(__file__).parent.parent / "scripts" / "bench_points.py" + + +def run_points(*argv: str) -> subprocess.CompletedProcess: + return subprocess.run( + [sys.executable, str(POINTS), *argv], capture_output=True, text=True, check=False + ) + + +def write_record(path: Path, document: Dict[str, Any]) -> str: + path.write_text(json.dumps(document, indent=2), encoding="utf-8") + return str(path) + + +# A warm launch as #196 records one: no `tools` stage, because a warm launch +# lends nothing, and no `handoff`, because nothing handed off to it. The +# medians are the published numbers and are written here as literals. +WARM: Dict[str, Any] = { + "command": ["dl", "owner/repo", "--", "true"], + "n": 3, + "shape": "warm", + "runs": [ + { + "wall_seconds": 2.0, + "total_seconds": 1.9, + "stages": {"host-prep": 0.02, "devpod-up": 0.45, "attach": 1.43}, + }, + { + "wall_seconds": 2.2, + "total_seconds": 2.1, + "stages": {"host-prep": 0.03, "devpod-up": 0.46, "attach": 1.61}, + }, + { + "wall_seconds": 2.4, + "total_seconds": 2.3, + "stages": {"host-prep": 0.04, "devpod-up": 0.47, "attach": 1.79}, + }, + ], + "median": { + "wall_seconds": 2.2, + "total_seconds": 2.1, + "stages": { + "host-prep": {"seconds": 0.03, "runs": 3}, + "devpod-up": {"seconds": 0.46, "runs": 3}, + "attach": {"seconds": 1.61, "runs": 3}, + }, + }, +} + +# The other shape the charter benches, with every non-handoff stage present. +COLD: Dict[str, Any] = { + "command": ["dl", "owner/repo", "--", "true"], + "n": 3, + "shape": "cold-recreate", + "runs": [ + { + "wall_seconds": 16.0, + "total_seconds": 15.9, + "stages": {"host-prep": 2.2, "devpod-up": 5.8, "tools": 10.2, "attach": 2.5}, + }, + { + "wall_seconds": 16.0, + "total_seconds": 15.9, + "stages": {"host-prep": 2.2, "devpod-up": 5.8, "tools": 10.2, "attach": 2.5}, + }, + { + "wall_seconds": 16.0, + "total_seconds": 15.9, + "stages": {"host-prep": 2.2, "devpod-up": 5.8, "tools": 10.2, "attach": 2.5}, + }, + ], + "median": { + "wall_seconds": 16.0, + "total_seconds": 15.9, + "stages": { + "host-prep": {"seconds": 2.2, "runs": 3}, + "devpod-up": {"seconds": 5.8, "runs": 3}, + "tools": {"seconds": 10.2, "runs": 3}, + "attach": {"seconds": 2.5, "runs": 3}, + }, + }, +} + + +class TestARecordBecomesTheShapesTrendPoints: + """One case per stage the shape reported, plus the shape's own total.""" + + def test_every_stage_and_the_total_are_named_by_their_shape(self, tmp_path): + record = write_record(tmp_path / "warm.json", WARM) + out = tmp_path / "bench.json" + result = run_points(record, "--out", str(out)) + assert result.returncode == 0, result.stderr + points = json.loads(out.read_text()) + assert [point["name"] for point in points] == [ + "warm / host-prep", + "warm / devpod-up", + "warm / attach", + "warm / total", + ] + + def test_the_published_value_is_the_median_in_seconds(self, tmp_path): + record = write_record(tmp_path / "warm.json", WARM) + out = tmp_path / "bench.json" + assert run_points(record, "--out", str(out)).returncode == 0 + points = json.loads(out.read_text()) + assert [point["value"] for point in points] == [0.03, 0.46, 1.61, 2.1] + assert {point["unit"] for point in points} == {"s"} + + def test_the_total_is_the_in_process_clock_with_the_wall_clock_beside_it(self, tmp_path): + """Two clocks measure a launch and they are not the same quantity, so + one of them is the trend line and the other is evidence next to it. + The stages sum to `total_seconds`, so that is the one the chart plots; + the outside stopwatch rides along in `extra` where it cannot be read + as a second, disagreeing trend.""" + record = write_record(tmp_path / "warm.json", WARM) + out = tmp_path / "bench.json" + assert run_points(record, "--out", str(out)).returncode == 0 + total = json.loads(out.read_text())[-1] + assert total["value"] == 2.1 + assert "wall=2.2s" in total["extra"] + + def test_several_records_publish_side_by_side_in_the_order_given(self, tmp_path): + warm = write_record(tmp_path / "warm.json", WARM) + cold = write_record(tmp_path / "cold.json", COLD) + out = tmp_path / "bench.json" + assert run_points(warm, cold, "--out", str(out)).returncode == 0 + names = [point["name"] for point in json.loads(out.read_text())] + assert names[:4] == [ + "warm / host-prep", + "warm / devpod-up", + "warm / attach", + "warm / total", + ] + assert names[4:] == [ + "cold-recreate / host-prep", + "cold-recreate / devpod-up", + "cold-recreate / tools", + "cold-recreate / attach", + "cold-recreate / total", + ] + + +class TestAnAbsentStageIsAbsent: + """#195's rule, and the reason the whole trend is trustworthy: a stage + that did not run has no point, never a zero. A warm launch lends no tools; + a zero there would claim the lend happened instantly and would drag the + line down exactly where a regression should show.""" + + def test_a_stage_no_run_reported_gets_no_point_rather_than_a_zero(self, tmp_path): + record = write_record(tmp_path / "warm.json", WARM) + out = tmp_path / "bench.json" + assert run_points(record, "--out", str(out)).returncode == 0 + points = json.loads(out.read_text()) + assert "warm / tools" not in [point["name"] for point in points] + assert 0 not in [point["value"] for point in points] + + def test_a_stage_only_some_runs_reported_says_how_many_it_is_a_median_of(self, tmp_path): + """A median of two and a median of five are not the same claim, and + the difference is invisible in a chart unless the point carries it.""" + partial: Dict[str, Any] = dict(WARM) + partial["runs"] = [ + dict(run, stages=dict(run["stages"], **({"tools": 9.0} if index else {}))) + for index, run in enumerate(WARM["runs"]) + ] + partial["median"] = dict( + WARM["median"], + stages=dict(WARM["median"]["stages"], tools={"seconds": 9.0, "runs": 2}), + ) + record = write_record(tmp_path / "warm.json", partial) + out = tmp_path / "bench.json" + assert run_points(record, "--out", str(out)).returncode == 0 + tools = next(p for p in json.loads(out.read_text()) if p["name"] == "warm / tools") + assert tools["value"] == 9.0 + assert "runs=2/3" in tools["extra"] + + +class TestThePointCarriesTheSpreadBehindIt: + """The median is the point; the runs are its evidence. A chart with no + error bar on a quantity that spreads 15-20s on a quiet host invites + reading noise as movement.""" + + def test_the_range_is_the_spread_of_the_runs_the_median_is_of(self, tmp_path): + record = write_record(tmp_path / "warm.json", WARM) + out = tmp_path / "bench.json" + assert run_points(record, "--out", str(out)).returncode == 0 + ranges = {p["name"]: p["range"] for p in json.loads(out.read_text())} + assert ranges["warm / host-prep"] == "± 0.01" + assert ranges["warm / total"] == "± 0.2" + + def test_one_run_has_no_spread_and_so_publishes_no_range(self, tmp_path): + single: Dict[str, Any] = dict(WARM, n=1, runs=WARM["runs"][:1]) + single["median"] = { + "wall_seconds": 2.0, + "total_seconds": 1.9, + "stages": {"host-prep": {"seconds": 0.02, "runs": 1}}, + } + record = write_record(tmp_path / "single.json", single) + out = tmp_path / "bench.json" + assert run_points(record, "--out", str(out)).returncode == 0 + assert all("range" not in point for point in json.loads(out.read_text())) + + +def without(record: Dict[str, Any], stage: str) -> Dict[str, Any]: + """*record* as a launch that never reported *stage* — the shape a devpod + upgrade that renamed a log line would leave behind.""" + return dict( + record, + runs=[ + dict(run, stages={k: v for k, v in run["stages"].items() if k != stage}) + for run in record["runs"] + ], + median=dict( + record["median"], + stages={k: v for k, v in record["median"]["stages"].items() if k != stage}, + ), + ) + + +class TestAShapeThatLostAStageFailsTheJob: + """#195's discipline, applied to the vocabulary that exists. + + The expected failure of measuring somebody else's process is not a wrong + number, it is an absence: a devpod upgrade renames what it prints, or dl + stops entering an arm, and the stage silently goes missing while the total + keeps working. That is the good failure only if it is loud — so the shape + where every stage is known to be present asserts them, and a run that lost + one fails *the job*, publishing nothing, rather than quietly flattening a + trend line nobody is watching that week. + """ + + def test_a_stage_missing_from_the_shape_it_is_required_on_is_refused(self, tmp_path): + record = write_record(tmp_path / "cold.json", without(COLD, "tools")) + out = tmp_path / "bench.json" + result = run_points(record, "--out", str(out), "--require-stages-on", "cold-recreate") + assert result.returncode != 0 + assert "tools" in result.stderr + assert "Traceback" not in result.stderr + assert not out.exists(), "a run that refused to publish must publish nothing" + + def test_a_stage_only_some_runs_reported_is_refused_on_that_shape(self, tmp_path): + """Half a stage is the same story mid-way through: it is going, and + the point that would be published is a median of a different N than + the one beside it.""" + partial: Dict[str, Any] = dict( + COLD, + median=dict( + COLD["median"], + stages=dict(COLD["median"]["stages"], tools={"seconds": 10.2, "runs": 2}), + ), + ) + record = write_record(tmp_path / "cold.json", partial) + out = tmp_path / "bench.json" + result = run_points(record, "--out", str(out), "--require-stages-on", "cold-recreate") + assert result.returncode != 0 + assert "tools" in result.stderr + assert "Traceback" not in result.stderr + assert not out.exists() + + def test_another_shape_may_legitimately_lack_what_that_one_requires(self, tmp_path): + """A warm launch lends nothing, so requiring `tools` of it would fail + every green run. The assertion is scoped to the shape, not the trend.""" + warm = write_record(tmp_path / "warm.json", WARM) + cold = write_record(tmp_path / "cold.json", COLD) + out = tmp_path / "bench.json" + result = run_points(warm, cold, "--out", str(out), "--require-stages-on", "cold-recreate") + assert result.returncode == 0, result.stderr + assert len(json.loads(out.read_text())) == 9 + + def test_a_requirement_that_matches_no_benched_shape_is_itself_a_failure(self, tmp_path): + """Same argument this repo's CI gate already makes: a check that + covers nothing reads exactly like a check that passed. A misspelt + shape here would leave the assertion asserting nothing, silently.""" + record = write_record(tmp_path / "cold.json", COLD) + out = tmp_path / "bench.json" + result = run_points(record, "--out", str(out), "--require-stages-on", "cold-recreat") + assert result.returncode != 0 + assert "cold-recreat" in result.stderr + assert "Traceback" not in result.stderr + assert not out.exists() + + def test_the_handoff_stage_is_not_required_of_anything(self, tmp_path): + """Nothing hands off to dl in CI, so `handoff` is legitimately absent + from every shape a runner benches — and it is also the one stage that + lies outside the total.""" + record = write_record(tmp_path / "cold.json", COLD) + out = tmp_path / "bench.json" + result = run_points(record, "--out", str(out), "--require-stages-on", "cold-recreate") + assert result.returncode == 0, result.stderr + assert "cold-recreate / handoff" not in [p["name"] for p in json.loads(out.read_text())] + + +class TestWhatItRefusesToPublish: + def test_a_record_with_no_shape_cannot_name_a_trend_line(self, tmp_path): + """`--shape` is the caller's to say and #196 refuses to guess it, so + an unlabelled record has no case name to publish under.""" + unlabelled = {k: v for k, v in WARM.items() if k != "shape"} + record = write_record(tmp_path / "warm.json", unlabelled) + out = tmp_path / "bench.json" + result = run_points(record, "--out", str(out)) + assert result.returncode != 0 + assert "shape" in result.stderr + assert "Traceback" not in result.stderr + assert not out.exists() + + def test_two_records_of_the_same_shape_would_collide(self, tmp_path): + """The case name is the trend's key. Two records both labelled `warm` + publish two points per case in one commit, and the chart has no way to + say which is the point.""" + first = write_record(tmp_path / "one.json", WARM) + second = write_record(tmp_path / "two.json", WARM) + out = tmp_path / "bench.json" + result = run_points(first, second, "--out", str(out)) + assert result.returncode != 0 + assert "warm" in result.stderr + assert "Traceback" not in result.stderr + assert not out.exists() + + +def points_module(): + """The converter, imported by path — it is a script, not a package.""" + spec = importlib.util.spec_from_file_location("bench_points", POINTS) + assert spec is not None and spec.loader is not None, POINTS + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +class TestTheVocabularyItAssertsIsTheOneDlEmits: + def test_every_stage_but_the_handoff_is_required_by_default(self): + """Written out in the script rather than imported, like its sibling, + so it stays runnable from a checkout that was never installed — which + makes this the test that catches the two drifting apart.""" + from devlaunch import timing # pylint: disable=import-outside-toplevel + + module = points_module() + assert module.REQUIRED_STAGES == tuple( + stage for stage in timing.STAGES if stage != timing.HANDOFF_STAGE + ) + + +class TestEveryDocumentedInvocationParses: + """Sibling of the same guard on the bench script (#192): every documented + invocation is handed to the real parser, so a flag renamed or documented + before it existed fails here.""" + + def test_the_epilog_shows_invocations_the_parser_accepts(self): + module = points_module() + joined = re.sub(r"\\\n\s+", " ", module.EPILOG) + documented = re.findall(r"bench_points\.py (.+)$", joined, re.M) + assert documented, module.EPILOG + for invocation in documented: + module.build_parser().parse_args(shlex.split(invocation)) diff --git a/test/test_bench_workflow.py b/test/test_bench_workflow.py new file mode 100644 index 00000000..b83f9c2d --- /dev/null +++ b/test/test_bench_workflow.py @@ -0,0 +1,115 @@ +"""The trend workflow's charter, guarded where it can be edited (#198). + +Map #193's first decision is *alert-don't-gate*: this workflow notices a +regression and must never be able to block a merge on one, because a wall-clock +measurement on a shared runner is either too loose to fire or flaky. That is a +property of a YAML file, so it is one edit away from being lost — a +`pull_request:` trigger added for convenience, a `fail-on-alert: true` copied +from another project's example, the job wired into the CI gate's `needs`. + +These are text assertions on config, deliberately: the invariants live in the +file's own words, there is no YAML parser in this project's dependencies, and +adding one to assert four strings would cost more than it guards. Each check +below is a thing somebody could plausibly write, with the reason it must not be +written. +""" + +from pathlib import Path + +WORKFLOWS = Path(__file__).parent.parent / ".github" / "workflows" +BENCH = WORKFLOWS / "bench.yml" +CI = WORKFLOWS / "ci.yml" +BENCH_JOB = "bench" + + +def bench_workflow() -> str: + assert BENCH.is_file(), f"{BENCH.name} is what publishes the trend" + return BENCH.read_text(encoding="utf-8") + + +def bench_settings() -> str: + """The workflow with its commentary removed. + + Half of what this file says about itself is a comment explaining a key it + deliberately does *not* set, so a check for "this key is absent" has to + read the settings rather than the prose about them. + """ + return "\n".join( + line for line in bench_workflow().splitlines() if not line.lstrip().startswith("#") + ) + + +class TestItCannotBecomeAMergeGate: + """Three independent ways this could turn into a required check, and the + reason each stays shut.""" + + def test_it_is_its_own_workflow_rather_than_a_job_in_the_gated_one(self): + """House convention is that a new job in the CI workflow joins the + gate's `needs` in the same pull request. A slow, noisy benchmark that + did that would be exactly the merge gate the charter grilled out.""" + needs = [line for line in CI.read_text(encoding="utf-8").splitlines() if "needs:" in line] + assert needs, "the CI gate's needs list is what this must stay out of" + for line in needs: + assert BENCH_JOB not in line, line + + def test_it_does_not_run_on_pull_requests_at_all(self): + """Per-PR benching is out of scope under alert-don't-gate — the value + is the trend line, not a pre-merge verdict — and a job that reports on + a pull request is a job somebody can require.""" + assert "pull_request" not in bench_settings() + + def test_a_slow_point_alerts_and_does_not_fail_the_build(self): + """`fail-on-alert` is the whole map in one setting.""" + text = bench_settings() + assert "fail-on-alert: false" in text + assert "fail-on-alert: true" not in text + + +class TestItMeasuresEveryCommitTheSameWay: + def test_nothing_is_path_filtered(self): + """A performance regression can arrive in any file, and a trend with + holes in it cannot be compared against its own previous point.""" + text = bench_settings() + assert "paths:" not in text + assert "paths-ignore:" not in text + + def test_it_runs_on_main_and_on_demand(self): + text = bench_settings() + assert "branches: [main]" in text + assert "workflow_dispatch:" in text + + def test_two_merges_cannot_bench_at_once(self): + """The publishing action commits to a branch; concurrent runs race on + that push, and two benches sharing one runner-class measure each + other.""" + assert "concurrency:" in bench_settings() + + +class TestTheAlertReachesSomebody: + def test_the_alert_comment_names_a_user(self): + """On a push there is no pull request, so the action leaves a commit + comment — and commit comments notify almost nobody. The @-mention is + what actually generates a notification, so without it the alert is + functionally invisible (#197).""" + assert "alert-comment-cc-users" in bench_settings() + + +class TestTheRunnerStateIsScoped: + def test_the_bench_writes_to_the_runners_own_scratch_state(self): + """The job runs real launches, which create real containers and real + devpod state. Both variables, because they scope different halves: + `DEVPOD_HOME` is devpod's own, `XDG_CACHE_HOME` is devlaunch's clone + cache and bookkeeping.""" + text = bench_settings() + assert "DEVPOD_HOME:" in text + assert "XDG_CACHE_HOME:" in text + + +class TestTheExclusionsSayWhyInTheFile: + def test_the_first_ever_cold_exclusion_is_written_down_where_it_is_made(self): + """Charter decision 4 excludes the first-ever cold launch — its + dominant cost is an image transfer this repo does not control. On an + ephemeral runner that exclusion is a step that primes the image before + anything is timed, which looks exactly like a step somebody could + delete as redundant. The reason has to be next to it.""" + assert "first-ever" in bench_workflow()