diff --git a/.github/actions/rocm-ci-setup/action.yml b/.github/actions/rocm-ci-setup/action.yml index 51d05f94b..6322d2a76 100644 --- a/.github/actions/rocm-ci-setup/action.yml +++ b/.github/actions/rocm-ci-setup/action.yml @@ -11,6 +11,16 @@ inputs: base image (public) is pulled anonymously. required: false default: "" + docker-socket: + description: >- + Mount the HOST docker daemon socket into the CI container, for workloads + that launch a container of their own (tokenspeed_serve). Off by default, + and it should stay off for every lane that does not need it: a container + that can reach the daemon can start a privileged one bind-mounting /, so + this is effectively root on the runner. Read the security note in + docker/docker-compose.docker-socket.yaml before setting it. + required: false + default: "false" runs: using: composite @@ -37,9 +47,25 @@ runs: echo "::warning::Docker Hub login failed (ROCM_SHARED_KEY may be expired). Continuing with anonymous pulls." fi + # The socket override is a second `-f`, not an edit to the base compose, so + # a lane that does not ask for it gets a container with no route to the + # daemon -- the same posture as before this input existed. `up` needs the + # override; `build` does not, since it adds no build stage. - name: Build and start ROCm CI container shell: bash working-directory: docker + env: + # NOT `DOCKER_SOCKET`: the override reads that name as the socket's host + # PATH, so a true/false flag under it would be substituted as the bind + # source and compose would mount a directory called `true`. + MOUNT_DOCKER_SOCKET: ${{ inputs.docker-socket }} run: | + files="-f docker-compose.build.yaml" + if [ "${MOUNT_DOCKER_SOCKET}" = "true" ]; then + echo "::warning::Mounting the host docker socket into ${CONTAINER_NAME:-the CI container}: this grants effective root on the runner." + files="${files} -f docker-compose.docker-socket.yaml" + fi + # shellcheck disable=SC2086 bash ../scripts/ci/docker_compose.sh --env-file .env.ci -f docker-compose.build.yaml build - bash ../scripts/ci/docker_compose.sh --env-file .env.ci -f docker-compose.build.yaml up -d + # shellcheck disable=SC2086 + bash ../scripts/ci/docker_compose.sh --env-file .env.ci ${files} up -d diff --git a/.github/workflows/bump-validate.yml b/.github/workflows/bump-validate.yml index 3c3725e24..6e0fdc787 100644 --- a/.github/workflows/bump-validate.yml +++ b/.github/workflows/bump-validate.yml @@ -29,4 +29,11 @@ jobs: with: ref: ${{ github.event.pull_request.head.sha }} do_alert: false # PRs must not file/close the nightly regression issue + # Explicitly off, not merely defaulted off. This lane runs PR head code on + # the self-hosted runner, so it must have no route to the host docker + # daemon -- a container that can reach the daemon can start a privileged + # one bind-mounting /. Stating it here means a future change to the + # input's default cannot silently arm the socket on a PR-triggered lane. + # The tokenspeed_serve_smoke entry reports `skip` without it, by design. + docker_socket: false secrets: inherit diff --git a/.github/workflows/eval-reusable.yml b/.github/workflows/eval-reusable.yml index 428877106..030ba3969 100644 --- a/.github/workflows/eval-reusable.yml +++ b/.github/workflows/eval-reusable.yml @@ -24,6 +24,21 @@ on: type: boolean required: false default: false + # MUST stay false for every PR-triggered lane (bump-validate.yml), and for + # any lane that does not run a workload needing its own container. + # Mounting the host daemon socket into the CI container is effectively root + # on the runner: a container that can reach the daemon can start a + # privileged one bind-mounting /. On a PR-triggered lane that is reachable + # by anyone who can open a PR that touches the trigger paths, so the socket + # is opt-in per caller and off by default -- deliberately NOT set here in + # the shared reusable workflow, where it would apply to every caller at + # once. Only nightly-eval.yml sets it true. See the security note in + # docker/docker-compose.docker-socket.yaml. + docker_socket: + description: "mount the host docker socket (nightly eval lane only)" + type: boolean + required: false + default: false env: CONTAINER_NAME: aorta-ci-gpu @@ -72,6 +87,7 @@ jobs: uses: ./.github/actions/rocm-ci-setup with: rocm-shared-key: ${{ secrets.ROCM_SHARED_KEY }} + docker-socket: ${{ inputs.docker_socket }} - name: Install wheel and run evaluation env: diff --git a/.github/workflows/nightly-eval.yml b/.github/workflows/nightly-eval.yml index 18eeb9d62..de4e85bd3 100644 --- a/.github/workflows/nightly-eval.yml +++ b/.github/workflows/nightly-eval.yml @@ -35,6 +35,15 @@ jobs: ref: ${{ github.event.workflow_run.head_sha || github.sha }} wheel_run_id: ${{ github.event.workflow_run.id || '' }} do_alert: true + # The nightly is the ONLY lane that gets the host docker socket: the + # tokenspeed_serve_smoke entry launches the TokenSpeed container itself, so + # it needs a route to the daemon. This lane is not PR-triggered -- it runs + # on a published nightly wheel or an explicit dispatch -- which is what + # makes the grant acceptable. Do not copy this line into bump-validate.yml. + # Until the socket is signed off, leaving this false is also safe: the + # entry declares `needs_docker_daemon: true` and reports `skip` with the + # reason recorded rather than failing the nightly. + docker_socket: true secrets: inherit # Append today's results to the ci-results data branch. The dashboard itself is diff --git a/.github/workflows/refresh-baselines.yml b/.github/workflows/refresh-baselines.yml index 0170532fd..8117150da 100644 --- a/.github/workflows/refresh-baselines.yml +++ b/.github/workflows/refresh-baselines.yml @@ -21,6 +21,20 @@ on: type: boolean required: false default: false + # The scoping guard for a perf refresh. An unscoped `--perf-gate` rewrites + # the whole baseline file and arms step-time ceilings on every matrix + # entry -- gpu_smoke, inference_offline, training_ddp, training_fsdp, race + # and llm_determinism included -- each from a single observation with no + # variance data behind it. Naming the entry under test keeps the refresh + # correctness-only everywhere else, so perf gating can be rolled out one + # workload at a time. Unknown names are rejected by refresh_baselines.py + # rather than silently scoping the gate to nothing. + perf_gate_entry: + description: >- + Restrict perf_gate to these matrix entries (comma- or space-separated, + e.g. "tokenspeed_serve_smoke"). Empty gates EVERY entry. + required: false + default: "" permissions: contents: write @@ -58,14 +72,51 @@ jobs: - name: Check out code uses: actions/checkout@v5 + # refresh_baselines.py rejects both of these too, but only after pip has + # installed the wheel inside the container -- minutes of GPU runner time to + # learn about a typo in a dispatch form. Same rules, checked in seconds. + - name: Validate perf-gate inputs + env: + PERF_GATE: ${{ inputs.perf_gate }} + PERF_GATE_ENTRY: ${{ inputs.perf_gate_entry }} + run: | + set -euo pipefail + if [ -n "${PERF_GATE_ENTRY}" ] && [ "${PERF_GATE}" != "true" ]; then + echo "::error::perf_gate_entry has no effect without perf_gate: true" + exit 1 + fi + # Deliberately a warning, not an error: an unscoped perf refresh is a + # legitimate end-state operation (bless everything once every workload + # has variance data), and it is the behaviour every existing dispatch + # of this workflow has had. Loud, not blocked. + if [ "${PERF_GATE}" = "true" ] && [ -z "${PERF_GATE_ENTRY}" ]; then + echo "::warning::Unscoped perf refresh: this arms step-time ceilings on EVERY matrix entry from a single observation each. Set perf_gate_entry to scope it to the workload under test." + fi + - name: Set up ROCm CI container uses: ./.github/actions/rocm-ci-setup with: rocm-shared-key: ${{ secrets.ROCM_SHARED_KEY }} + # Dispatch inputs reach the container as environment variables rather than + # being interpolated into this shell string. `perf_gate_entry` is free + # text, and `${{ }}` substitution happens before bash sees the script, so + # interpolating it would let a dispatch value close the quote and run + # arbitrary commands on the runner. - name: Install nightly wheel and regenerate baselines + env: + STEP_TIME_MARGIN: ${{ inputs.step_time_margin }} + THROUGHPUT_MARGIN: ${{ inputs.throughput_margin }} + PERF_GATE: ${{ inputs.perf_gate }} + PERF_GATE_ENTRY: ${{ inputs.perf_gate_entry }} run: | - bash scripts/ci/docker_cmd.sh exec "${{ env.CONTAINER_NAME }}" bash -lc ' + bash scripts/ci/docker_cmd.sh exec \ + -e STEP_TIME_MARGIN="${STEP_TIME_MARGIN}" \ + -e THROUGHPUT_MARGIN="${THROUGHPUT_MARGIN}" \ + -e PERF_GATE="${PERF_GATE}" \ + -e PERF_GATE_ENTRY="${PERF_GATE_ENTRY}" \ + -e DEV_WHEELS_INDEX="${{ env.DEV_WHEELS_INDEX }}" \ + "${{ env.CONTAINER_NAME }}" bash -lc ' set -euo pipefail cd /workspace/aorta python -m pip install --upgrade pip @@ -73,11 +124,29 @@ jobs: # so baselines are generated on the exact dependency set they gate. constraint="" [ -f config/ci/ci-constraints.txt ] && constraint="-c config/ci/ci-constraints.txt" - pip install --pre "amd-aorta[hw-queue]" -f "${{ env.DEV_WHEELS_INDEX }}" $constraint + pip install --pre "amd-aorta[hw-queue]" -f "$DEV_WHEELS_INDEX" $constraint + # NOTE: this whole script is inside a single-quoted bash -lc, so it + # must contain no apostrophes. + args=() + if [ "${PERF_GATE}" = "true" ]; then + args+=(--perf-gate) + fi + # Comma- or space-separated list -> one --perf-gate-entry per name, + # which is what the repeatable flag expects. Empty fields (from a + # trailing comma) are dropped rather than passed as an empty string, + # which the known-entry check in refresh_baselines.py would reject. + if [ -n "${PERF_GATE_ENTRY}" ]; then + IFS=", " read -r -a _entries <<< "${PERF_GATE_ENTRY}" + for _e in "${_entries[@]}"; do + if [ -n "${_e}" ]; then + args+=(--perf-gate-entry "${_e}") + fi + done + fi python scripts/ci/refresh_baselines.py \ - --step-time-margin "${{ github.event.inputs.step_time_margin || 0.25 }}" \ - --throughput-margin "${{ github.event.inputs.throughput_margin || 0.15 }}" \ - ${{ github.event.inputs.perf_gate == 'true' && '--perf-gate' || '' }} + --step-time-margin "${STEP_TIME_MARGIN:-0.25}" \ + --throughput-margin "${THROUGHPUT_MARGIN:-0.15}" \ + ${args[@]+"${args[@]}"} ' # The container wrote regression_baselines.yaml into the mounted workspace; diff --git a/config/ci/nightly_eval_matrix.yaml b/config/ci/nightly_eval_matrix.yaml index 87a047319..de7f959e3 100644 --- a/config/ci/nightly_eval_matrix.yaml +++ b/config/ci/nightly_eval_matrix.yaml @@ -10,6 +10,21 @@ # nproc - optional; launch under `torchrun --standalone --nproc_per_node=` # min_gpus - optional; skip when torch.cuda.device_count() is lower (default 1; # defaults to nproc when nproc is set) +# timeout_sec - optional; per-entry wall-clock budget (default 1800). Exceeding +# it kills the whole process group and FAILS the entry. +# needs_docker_daemon - optional; skip when no Docker daemon is reachable from +# wherever nightly_eval.py is running. For workloads that start +# their engine in a sibling container. Like min_gpus this states a +# runner CAPABILITY, so its absence is a skip, not a failure -- +# and only entries that set it consult the probe, so a broken +# daemon cannot turn the rest of the nightly into skips. +# +# Record-only is NOT a field here: a cell is record-only exactly while +# config/ci/regression_baselines.yaml has no key for it. Adding an entry to +# `entries` therefore starts it record-only by construction. Note the limit of +# that -- record-only defers PERFORMANCE gating only. A cell that errors or +# fails is a `fail` with or without a baseline (nightly_eval.py is fail-closed), +# so an entry may only be added here once it can actually pass on the runner. # # Phase 1 wires one config per registered workload (the smoke recipes). Per-workload # matrices (dtypes, larger GPU counts) are Phase 3 -- add rows here, no code change. @@ -79,6 +94,61 @@ entries: nproc: 8 min_gpus: 8 + # TokenSpeed online serving. Chosen over the other four serving recipes as the + # cheapest cell that still answers something: Qwen3-0.6B is ~1.2 GB of weights + # against ~40 GB for the gpt-oss recipes, and two cells against four (models) + # or six (load). It is also the only serving recipe whose cells differ ONLY by + # mitigation, so the pair is a same-night control: a move in both cells is the + # stack, a move in one is the mitigation or noise. That is the triage lever the + # multi-model and load recipes cannot give, since their cells legitimately + # differ in speed. + # + # 3600s, not the 1800s default: two bring-ups at the observed startup spread + # (180-415 s measured over 13 cell-runs) plus the teardown VRAM drain is ~15 + # min before the image pull and any cold-cache weight download, and a timeout + # is an unconditional entry FAILURE rather than a slow record. + # + # `needs_docker_daemon` is what makes this row safe to have here today. The + # nightly runs nightly_eval.py INSIDE aorta-ci-gpu, and `tokenspeed_serve` + # starts the TokenSpeed engine in its own container, so it needs a docker + # client in the image (present, and proven: the built image carries 29.7.2 and + # no daemon) plus a route to a daemon (a per-lane opt-in that no lane sets, + # because it grants effective root on the runner and that is the CI owner's + # call). Until a lane sets `docker-socket: true` this entry SKIPS, with the + # reason in the results, rather than failing on "cannot connect to the Docker + # daemon" -- the same contract min_gpus gives the 8-GPU rows. + # + # Proven on gfx950 on 2026-09-02: two sweeps driven from inside the CI + # container over a mounted socket, four cell-runs, all passed, twelve clean + # steps (1108-1149 ms), metrics within 5% of the host-side envelope. + # + # The recipe's `work_dir: /tmp/ts-work-serve` must be mounted at the SAME + # string on both sides and be root-owned -- see the work_dir section of + # docs/tokenspeed-gating-rollout.md, which the demonstration made a good deal + # more specific. + - name: tokenspeed_serve_smoke + recipe: recipes/tokenspeed/tokenspeed-serve-bench-smoke.yaml + min_gpus: 1 + timeout_sec: 3600 + needs_docker_daemon: true + +# Entries that are ready to run but blocked on a runner prerequisite. This key +# is not read by anything: nightly_eval.py and refresh_baselines.py both iterate +# `entries` only, so nothing below executes. It exists because the alternative +# for a staged-but-blocked entry is a commented-out block (the convention the +# two NOTEs above use), and a comment cannot be checked -- tests/ci/ +# test_nightly_eval.py validates these the same way it validates `entries`, so +# the recipe path, the GPU requirement and the metric names are known-good on +# the day the prerequisite lands rather than discovered by a red nightly. +# +# Promote by moving the entry (minus `blocked_on`) into `entries` above. +# Empty as of 2026-09-02: tokenspeed_serve_smoke, the only entry that was ever +# staged here, was promoted into `entries` once a cell was demonstrated running +# from inside the CI container. Keep the key and this comment -- the convention +# is worth more than the one row was, and the next blocked entry should land +# here rather than in a comment block. +pending_entries: [] + # dtype axis (fp32/fp16/bf16) is a follow-up: the smoke recipes pin one dtype # each, so per-dtype coverage needs dtype recipe variants (or workload_config # overrides). Add them as new rows here once those recipes exist -- no code change. diff --git a/docker/Dockerfile.ci-gpu b/docker/Dockerfile.ci-gpu index fbf9b6c62..632411c21 100644 --- a/docker/Dockerfile.ci-gpu +++ b/docker/Dockerfile.ci-gpu @@ -271,6 +271,76 @@ RUN python -m pip install --no-cache-dir \ "openpyxl==3.1.5" \ "seaborn==0.13.2" +# Docker CLI -- the CLIENT only, no daemon. +# +# The nightly eval harness runs INSIDE this image (`docker exec aorta-ci-gpu +# python scripts/ci/nightly_eval.py`), and one of the workloads it drives, +# `tokenspeed_serve`, runs the TokenSpeed engine in a published container of its +# own. Its `setup()` starts with `shutil.which("docker")` and raises +# "'docker' not on PATH" without it, which fails the cell -- so a serving recipe +# in the nightly matrix reddens the nightly every night until this exists. +# +# What the workload needs is a client that can reach the HOST daemon over a +# bind-mounted socket, i.e. the SIBLING-container pattern -- the engine comes up +# next to this container, not nested inside it. So this installs no dockerd, no +# containerd and no runc: the static tarball ships all of them and only the +# `docker` client is extracted. Nothing here grants the container anything on +# its own. The socket is what would, and it is deliberately NOT in the base +# compose: it lives in the opt-in docker-compose.docker-socket.yaml override, +# whose header carries the security argument and the alternatives. Read that +# before enabling it -- a container that can reach the daemon has effective root +# on the host. +# +# Pinned by version AND sha256, the same way the base above is pinned by +# manifest digest and the pins below are exact, and for the same reason: the +# static channel keeps old versions, but a bare URL still names a moving target +# the day the version is bumped without review. Both live in the script. +# +# Not apt. This image has no apt layer at all, and adding one for a single +# binary would mean Docker's own repo (Ubuntu's `docker.io` pulls in the daemon +# this deliberately excludes), which means a signing key to rotate and package +# versions that float between builds -- three new maintenance surfaces against +# one pinned file. A pinned prebuilt artifact is also what #350 moved the +# sanitizer toolchain TO, so it is the direction this image already went. +# +# Fetched with python rather than curl or wget because this base guarantees +# neither downloader but does guarantee python -- the layout guard above, the +# two fixup blocks and the pip pins below all run it. Stdlib only, so a Python +# bump moves it for free; verified against 3.14 with the rest of #411. +# +# A COPY + RUN pair rather than an inline heredoc, matching the layout guard +# above: heredoc RUNs need BuildKit, and a runner with DOCKER_BUILDKIT=0 (this +# repo has hosts still on the legacy builder, with no buildx installed) would +# fail on syntax rather than on anything real. Where this file needs inline +# python it uses the `printf > /tmp/x.py` form for the same reason. A COPY'd +# script is also testable on its own, which neither form is. +COPY install_docker_cli.py /usr/local/share/aorta/install_docker_cli.py +RUN python /usr/local/share/aorta/install_docker_cli.py + +# And prove the client is usable, at build time -- same reasoning as the hipcc +# link check and the proton dlopen check above. A docker client that is present +# but cannot execute is indistinguishable from a working one until a nightly +# cell fails hours later, which is the loop those checks exist to break. +# +# `docker --version` is answered by the binary alone, with no daemon contacted, +# which is exactly the property wanted here: the build must not depend on a +# daemon (there is none inside a build step) and must not appear to pass because +# one happened to be reachable. +# +# The daemon-absence assertions are the other half. They are what keeps "client +# only" true: extracting one member from a tarball that also ships dockerd, +# containerd and runc is a one-word edit away from shipping a daemon, and +# nothing else in this image would notice. +RUN set -eu; \ + docker --version; \ + for daemon in dockerd containerd runc containerd-shim-runc-v2; do \ + if command -v "${daemon}" >/dev/null 2>&1; then \ + echo "${daemon} is on PATH: this image is meant to carry the docker CLIENT only" >&2; \ + exit 1; \ + fi; \ + done; \ + echo "docker client check: OK -- $(command -v docker), no daemon on PATH" + # Deliberately NO `ENV ROCM_HOME=/opt/rocm` here (issue #381). # # ROCM_HOME is an explicit operator override, so it ranks ABOVE autodetection in diff --git a/docker/docker-compose.docker-socket.yaml b/docker/docker-compose.docker-socket.yaml new file mode 100644 index 000000000..3b9121c76 --- /dev/null +++ b/docker/docker-compose.docker-socket.yaml @@ -0,0 +1,80 @@ +# Optional override: let the container drive the HOST docker daemon. +# +# Needed by exactly one thing today: the `tokenspeed_serve` workload, which runs +# the TokenSpeed engine in its own published container. The nightly eval harness +# runs *inside* aorta-ci-gpu, so without this the workload's `setup()` fails on +# "'docker' not on PATH" (the client comes from docker/Dockerfile.ci-gpu) or, with +# the client but no socket, on "Cannot connect to the Docker daemon". +# +# Usage: +# docker compose -f docker-compose.build.yaml -f docker-compose.docker-socket.yaml up -d +# +# Do not use this file if you do not need it; omit -f docker-compose.docker-socket.yaml. +# It is a separate file rather than a volume in the base compose precisely so the +# default container has no route to the daemon -- see the security note below. +# +# --------------------------------------------------------------------------- +# SECURITY: this grants effective root on the HOST +# --------------------------------------------------------------------------- +# A process that can talk to the docker daemon can start a container that is +# privileged and bind-mounts `/`, which is root on the host with no further +# exploit -- so this mount hands anything running in the container, including +# any code the nightly installs from an index, the whole node. The base compose +# is already `privileged: true` with `seccomp=unconfined`, so on a dedicated, +# trusted, single-tenant CI runner this widens an already-wide posture rather +# than opening a new one. On a shared or multi-tenant runner it is NOT +# acceptable, and the alternatives, in order of preference, are: +# +# 1. Run the serving workload outside the CI container -- as a step on the +# runner host, which already has a docker client and the socket, publishing +# its results JSON into the harness's results directory. The nightly keeps +# the socket out of the container entirely. +# 2. A socket proxy (e.g. tecnativa/docker-socket-proxy) mounted instead of +# the raw socket, allowlisting only the container create/start/logs/remove +# calls this workload makes. Still a privilege boundary worth reviewing, +# but not a blanket one. +# 3. Rootless docker or a per-runner daemon in a user namespace, so "root on +# the host" is root on an unprivileged uid. +# +# Nested docker-in-docker (a dockerd inside this container) is deliberately NOT +# on that list: it needs `privileged` too, so it trades no privilege away, and +# it gives the engine container a different view of the filesystem, which breaks +# the bind mounts described below. +# +# --------------------------------------------------------------------------- +# Why the second mount, and why source == target +# --------------------------------------------------------------------------- +# With the socket mounted, the engine container is a SIBLING of this one: this +# container asks the host daemon to start it. The `-v` sources in that request +# are strings the HOST daemon resolves against the HOST filesystem, not paths +# this container can see. `tokenspeed_serve` builds them from `work_dir` +# (`/u/{scripts,out,hf}`), so if `work_dir` names a path that +# exists only inside this container, the daemon does not error -- it CREATES the +# missing source on the host and mounts that. The engine container then gets an +# empty /ts-scripts and dies on a missing script, or worse comes up against an +# empty /ts-out and the harvest reports no results for a run that happened. +# +# Mounting the same host path at the same path inside the container makes the +# string mean the same directory on both sides, which is what the workload needs +# and cannot check for itself. `work_dir` must then be set to TS_SERVE_WORK_DIR +# explicitly in the recipe -- it happens to equal the workload default, but a +# default that must match a mount is a coincidence, not a configuration. +# +# The directory must be node-local (the workload rejects NFS under root-squash) +# and is created on the host by the compose run if absent. Ownership matters: +# the base compose runs this container as root, so the workload sees uid 0, +# writes `/u0`, and re-checks that it is owned by uid 0 and not +# group/world-writable. That holds because a root-owned directory created +# through this mount is root-owned on the host too. Change `user:` in the base +# compose, or introduce userns-remap on the daemon, and that check starts +# failing with an ownership error that does not mention either. + +services: + torchenv: + volumes: + # The daemon socket. Read-write is required: the workload creates, starts, + # inspects and removes a container. `user: root` in the base compose means + # no group_add is needed to open it. + - ${DOCKER_SOCKET:-/var/run/docker.sock}:/var/run/docker.sock + # Shared scratch, at an identical path on both sides. See above. + - ${TS_SERVE_WORK_DIR:-/tmp/ts-work-serve}:${TS_SERVE_WORK_DIR:-/tmp/ts-work-serve} diff --git a/docker/install_docker_cli.py b/docker/install_docker_cli.py new file mode 100644 index 000000000..f42764bf6 --- /dev/null +++ b/docker/install_docker_cli.py @@ -0,0 +1,78 @@ +#!/usr/bin/env python3 +"""Install the Docker CLI -- the client only -- into a CI image. + +Run from docker/Dockerfile.ci-gpu. See the block that invokes it there for why +the image needs a docker client at all; this file is only concerned with getting +one in place reproducibly. + +Kept in the image after use, like ``rocm_layout_guard.py`` next to it: re-running +it is the fastest way to repair or re-verify a client on a runner, and it records +in the image itself which artifact the binary came from. + +Deliberately stdlib-only and downloader-free. The base image guarantees python +(the layout guard, the ROCm fixup blocks and the pip pins all run it) but +promises neither curl nor wget, so shelling out to one would add a dependency +this image does not have. Stdlib-only also means the Ubuntu 26.04 / py3.14 base +this now sits on moved it for free, which a pinned third-party dependency would +not have. +""" + +from __future__ import annotations + +import hashlib +import io +import os +import sys +import tarfile +import urllib.request + +# Pinned by version AND by the tarball's sha256, the same way the base image is +# pinned by manifest digest: the static channel keeps old versions around, but a +# bare URL still names a moving target the moment the version is bumped without +# review. Re-resolve both together when bumping: +# +# curl -fsSL -O https://download.docker.com/linux/static/stable/x86_64/docker-.tgz +# sha256sum docker-.tgz +VERSION = "29.7.2" +SHA256 = "803d433f226db4776e1768fd319fc6c6e4935a456acf84fcc0080818b854bc8f" + +URL = f"https://download.docker.com/linux/static/stable/x86_64/docker-{VERSION}.tgz" + +# The one member extracted. The same tarball also ships dockerd, containerd, +# containerd-shim, runc, docker-init and docker-proxy; this image wants a client +# that talks to the *host* daemon over a bind-mounted socket, not a daemon of its +# own, so naming the member explicitly is what keeps a daemon out of the image. +MEMBER = "docker/docker" +DEST = "/usr/local/bin/docker" + + +def main() -> int: + with urllib.request.urlopen(URL, timeout=180) as response: + blob = response.read() + + # Verify before extracting, not after installing: a truncated or substituted + # download should fail the build, not leave a partial binary on PATH for the + # next layer to find. + digest = hashlib.sha256(blob).hexdigest() + if digest != SHA256: + print( + f"{URL}\n sha256 {digest}\n expected {SHA256}", + file=sys.stderr, + ) + return 1 + + with tarfile.open(fileobj=io.BytesIO(blob), mode="r:gz") as tar: + extracted = tar.extractfile(tar.getmember(MEMBER)) + if extracted is None: + print(f"{MEMBER} is not a regular file in {URL}", file=sys.stderr) + return 1 + with open(DEST, "wb") as target: + target.write(extracted.read()) + + os.chmod(DEST, 0o755) + print(f"installed {MEMBER} from docker-{VERSION}.tgz to {DEST}") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/docs/ci-nightly-eval.md b/docs/ci-nightly-eval.md index c6a6c322d..80a973588 100644 --- a/docs/ci-nightly-eval.md +++ b/docs/ci-nightly-eval.md @@ -188,7 +188,16 @@ controls stay hidden unless it runs. `refresh_baselines.py --perf-gate` (adds `step_time_ms.max` plus per-metric `policy`/`value` bounds -- min for throughput, max for latency/step-time, equal for checksums -- that the comparator then enforces; a required metric that is - absent is a failure). + absent is a failure). `--perf-gate-entry ` (repeatable) restricts that to + named entries: the baseline file is rewritten whole, so an unscoped refresh + arms *every* entry's perf gates from whichever single run it just did. + Both are dispatch inputs on `refresh-baselines.yml` (`perf_gate` and + `perf_gate_entry`, the latter taking a comma- or space-separated list), so a + scoped refresh is runnable from the Actions UI without editing the workflow. + A worked example of rolling gating out for one workload -- which metrics to + bound, how many record-only runs to take first, and what to derive the + threshold from -- is in + [tokenspeed-gating-rollout.md](tokenspeed-gating-rollout.md). ## Baselines diff --git a/docs/tokenspeed-gating-rollout.md b/docs/tokenspeed-gating-rollout.md new file mode 100644 index 000000000..c2a6df7e7 --- /dev/null +++ b/docs/tokenspeed-gating-rollout.md @@ -0,0 +1,1067 @@ +# Turning on nightly perf gating for TokenSpeed serving + +[`tokenspeed_serve`](tokenspeed-serving.md) reports TTFT, TPOT, ITL and +throughput, and the serving metric names are already in the nightly's gating +allowlist. A serving recipe is now in `config/ci/nightly_eval_matrix.yaml`, but +nothing is gated, because no serving baseline has been blessed. This document is +the sequence for changing that, and the reasoning behind the numbers it picks. + +The reason it is a document rather than a commit is that we do not yet have a +window to derive thresholds from. A threshold derived from a single observation +encodes whichever night it was taken on, and the nightly then fails on the +difference between two healthy runs. That failure is worse than no gate: it +trains everyone to ignore the alert, and the first real regression arrives into a +channel nobody reads. + +Measuring the cell rather than reasoning about it has since made that case +stronger and the first bless smaller. One cell-run in thirteen carries a +five-second compile excursion in its first measured step, which three of the four +originally-proposed gates cannot survive — see [the smoke cell +section](#the-smoke-cell-is-not-unconditionally-clean-measured-13-cell-runs). + +See [ci-nightly-eval.md](ci-nightly-eval.md) for how the nightly works in +general; this only covers what is specific to serving. + +## Record-only is the absence of a baseline, not a matrix field + +Worth stating plainly, because the phrase suggests a setting somewhere and there +isn't one. `nightly_eval.py` looks each `(entry, cell)` up in +`config/ci/regression_baselines.yaml` by the key `entry::cell`; a cell with no +key gets `compare_to_baseline(harvested, None)`, which returns `record`. So an +entry added to the matrix is record-only by construction, and stops being +record-only the moment a `refresh-baselines` PR merges a key for it. + +Two consequences that decide the shape of this rollout: + +- **Record-only defers performance gating only.** A cell that errors or fails is + a `fail` with or without a baseline — the harness is fail-closed by design, and + `docs/ci-nightly-eval.md` says so. An entry may therefore only be added to the + matrix once it can actually *pass* on the runner. Adding one that cannot is not + a soft landing; it is a nightly that is red every night. That constraint is + what shaped this entry: it declares `needs_docker_daemon: true` so that a + runner without the socket **skips** it rather than failing — see [What blocks + this today](#what-blocks-this-today). +- **Blessing is all-or-nothing per refresh, unless it is scoped.** + `refresh_baselines.py` rebuilds the whole baseline file and `--perf-gate` armed + every entry in it. Running it to bless serving would, in the same PR, derive + step-time ceilings for `gpu_smoke`, `inference_offline`, `training_ddp`, + `training_fsdp`, `race` and `llm_determinism` — sixteen cells, each from a + single observation, none of which anyone has variance data for. That is the + failure this document is about, inflicted on six unrelated workloads as a side + effect. `--perf-gate-entry` (added with this document) scopes it. + +## What we actually know about the variance + +Every number below is measured, on one gfx950 (MI355X) node against the image +digest the recipes pin. There is no synthetic data here, and no more of it than +this — that is the whole problem. + +Two caveats that apply to every number in this section, both established later +in the document rather than assumed here. They were taken at `warmup_steps: 1` +and the recipe now sets `2`, so they are rationale and not a baseline; and they +were taken on an **MI355X**, whereas the CI runner `smci350-rck-g03-f16-12` is an +**MI350X**, so their absolute *level* does not transfer to the runner even though +their *spread* is the thing being argued about. See +[Verification status](#verification-status). + +**Bring-up is the noisy one, and it is very noisy.** From +[tokenspeed.md](tokenspeed.md): + +> Startup to `/health` is the dominant cost and it is **noisy**: 189, 276, 285, +> 291, 316 and 319 seconds across six runs of the same recipe on the same node — +> a 1.7× spread with nothing changed between them. So do not treat a slow +> bring-up as a signal without repeats, and do not tune `timeout_per_trial` close +> to an observed number; the recipe's 1800 s leaves deliberate headroom. + +The multi-model sweep adds a seventh observation for the same 0.6B model at +379 s, and [tokenspeed-serving.md](tokenspeed-serving.md) declines to treat the +column as a measurement at all: + +> Read the startup column as a floor, not a measurement: it is dominated by +> weight loading and Triton compilation against whatever the node's caches +> already hold, which is why the smallest model here posts the largest number. It +> is reported because `ready_timeout_sec` has to cover it, not because it scales +> with anything. + +The four-model table is the evidence for that last clause: 379, 283, 289 and 328 +seconds for Qwen3 at 0.6B, 1.7B, 4B and 8B. Bring-up does not even order by +model size. + +**The compile cache is a second, larger excursion, and one mitigation stands +between it and the metrics:** + +> On Qwen3-0.6B the first bench invocation against a fresh server took 6.2s +> against 1.1s for every later one. Rolled into the metrics that one outlier +> dominates the mean step time and inflates TTFT tenfold (465ms vs 47ms), so a +> cell looks like a regression purely because it went first. + +`warmup_steps: 1` is supposed to discard that step. **It does not always**, and +that is the single most important number in this document — see the next +section, which measures it on the cell the staged entry actually runs. That +measurement is why the recipe now sets `warmup_steps: 2`. + +**Steady-state serving metrics, by contrast, reproduce well.** The docs do not +say this in one sentence, but they contain two independent repeats: + +- `tokenspeed-serve-models.yaml::qwen3-0.6b` and + `tokenspeed-serve-load.yaml::conc-8` run a byte-identical measurement + configuration — Qwen3-0.6B, ISL 512 / OSL 128, concurrency 8, 32 prompts, 3 + measured steps, 1 warmup step, `ignore_eos`, seed 0 — in two separate sweeps. +- `tokenspeed-serve-gptoss.yaml::baseline` and + `tokenspeed-serve-gptoss-tp.yaml::tp1`, of which the doc says "TP=1 reproduces + the single-GPU numbers above to within a percent, which is the control this + axis needs". + +| Metric | Qwen3-0.6B, two sweeps | spread | gpt-oss-20b, two sweeps | spread | +|---|---|---|---|---| +| `median_ttft_ms` | 46.3 / 45.9 | 0.87% | 67.1 / 67.2 | 0.15% | +| `median_tpot_ms` | 1.94 / 1.91 | 1.57% | 7.61 / 7.63 | 0.26% | +| `output_throughput` | 3538 / 3631 | 2.63% | 994 / 991 | 0.30% | +| `server_startup_sec` | 379 / — | — | 316 / 199 | 59% | + +The same node reports serving rates to within 3% across sweeps and bring-up time +to within 59%. Those two facts are what the per-metric table below is derived +from. + +There is also a useful contrast from the kernel side of the integration, where +the repo already distinguishes a tight metric from a loose one: the GEMM probe's +"spread across trials was under 1.5 µs in every cell". Not everything TokenSpeed +measures is noisy; bring-up is. + +### The smoke cell is not unconditionally clean (measured, 13 cell-runs) + +Everything above is a claim quoted from another document. This section is a +measurement of the exact cell the staged nightly entry runs, taken from the +`step_times_ms` and `metrics_summary` recorded in every `matrix.json` on disk for +`TOKENSPEED-SERVE-SMOKE`: six sweeps, 13 cell-runs with step times, 39 steps. + +> **These numbers were all taken at `warmup_steps: 1`, which the recipe no +> longer uses.** The recipe now sets `warmup_steps: 2`, precisely because of what +> this section measures. So read everything below as **the rationale for that +> change, not as the baseline to bless against**: the table is the evidence that +> a step-0 excursion reaches the metrics at `warmup_steps: 1`, and it is retained +> for that purpose. It is *not* a prediction of what the record-only window will +> show, because the window is taken at a different setting — one whose whole +> purpose is to remove the excursion these thirteen cell-runs contain. Nothing +> here is deleted or restated at the new setting; there is no measurement at +> `warmup_steps: 2` yet, and inventing one would be worse than having none. The +> ten-night window is what produces it. See step 3 of +> [the rollout sequence](#the-rollout-sequence). + +**Twelve of the thirteen are very clean.** Tighter than the cross-sweep numbers +above, because these are the same recipe on the same node: + +| Metric | Clean range over 12 cell-runs | Spread | +|---|---|---| +| `median_ttft_ms` | 43.65 – 46.87 | 7.37% | +| `median_tpot_ms` | 1.89 – 1.95 | 3.08% | +| `output_throughput` | 3502.53 – 3646.20 | 4.10% | +| `p99_itl_ms` | 34.00 – 35.84 | 5.43% | +| step time | 1108 – 1178 ms | 6.3% | + +**The thirteenth is the compile-cache excursion, and `warmup_steps: 1` did not +catch it.** `serve-smoke3::no-scratch-reclaim` recorded its three measured steps, +in order, as: + +``` +6193.4 ms, 1140.2 ms, 1142.6 ms +``` + +The excursion is the **first measured step** — after the warmup step was already +discarded. Its cell then reported `median_ttft_ms` 465.30 against a clean 43.65– +46.87, which is 10.26× the clean mean and the same 465 vs 47 the serving doc +quotes. So the 10× TTFT excursion is not a hypothetical a recipe edit could +introduce; it is present, once, in the data we already have, at **1 cell-run in +13 (7.7%)**. + +What it does to the four gates the table below proposes, anchoring each on the +worst of the twelve clean runs and applying the plan's own margins: + +| Gate | Anchor | Threshold | Excursion | Verdict | +|---|---|---|---|---| +| `step_time_ms.max` | 1169.50 | 1461.88 | 2825.40 | **fail** | +| `median_ttft_ms` | 46.87 | 58.59 | 465.30 | **fail** | +| `output_throughput` | 3502.53 | 2977.15 | 2612.83 | **fail** | +| `median_tpot_ms` | 1.95 | 2.44 | 1.93 | pass | +| `p99_itl_ms` | 35.84 | 44.80 | 34.96 | pass | + +Three of the four proposed gates fire on a run that is not a regression. Those +verdicts are `compare_to_baseline`'s, not arithmetic done here — +`tests/ci/test_eval_lib.py` runs the measured numbers through the real comparator. + +**Why the latency-per-token metrics survive it, and why that is the useful +signal.** The excursion is a fixed ~5 s of compilation added to one step. It +therefore lands on anything derived from step *duration* — the step-time mean, +and throughput, which is tokens over that duration — and on TTFT, because the +first request of that step waits for the compile. It does not touch +`median_tpot_ms` (1.01× clean) or `p99_itl_ms` (1.00× clean), because those are +measured *between tokens*, after the compile has happened. A metric whose +definition excludes the excursion is immune to it by construction, not by luck. + +**This is a different failure from the concurrency-64 one, and the difference +decides the fix.** The matrix work found ~8% of bench steps at concurrency 64 +stalling by a fixed ~0.92 s with no positional pattern — the `serve-load::conc-64` +cell on disk reads `1621, 2286, 2595` ms, bimodal in a way no warmup setting can +remove. The smoke cell's excursion is at position 0 every time it appears, which +makes it **deterministically avoidable**: it is cache warming, and one more +discarded step removes it. The two look alike in a summary statistic (both make a +three-step mean untrustworthy) and are opposite in what to do about them. + +Finally, the mitigation A/B is consistent with all of the above. On gpt-oss the +`hsa_no_scratch_reclaim` cell landed at TTFT 66.7 vs 67.1 baseline, TPOT 7.49 vs +7.61, throughput 1008 vs 994 — the doc calls it "within noise of baseline, as it +was at every smaller size", and the size of that "noise" is the 0.3–2.6% band +above, not the 59% one. + +## Per-metric: gate, record-only, or never + +Direction is fixed by `_METRIC_POLICIES` in `scripts/ci/eval_lib.py` (`max` for +latencies, `min` for throughputs). What this table decides is which of them get a +*bound* on the first bless. + +Only that. The verdicts below are about the nightly's blessed baselines, which +are derived by applying a margin to an observed value, and they say nothing +about the workload's own `gates:` block (`_GATE_SPECS` in +`src/aorta/workloads/tokenspeed_serve.py`), which enforces absolute numbers a +recipe writes out per trial. The two differ exactly where the margin does the +damage: `max_p99_itl_ms: 50` is a stated ceiling on tail stalls and is a +perfectly good gate, whereas a *baseline* ITL ceiling is `observed × 1.25`, +which for an observation near zero is near zero. So "Never" in this table means +"never armed automatically from a baseline", not "not gateable" — and a metric +listed here as record-only can still carry a hand-written per-trial bound today. + +> **Revised by measurement.** The four-gate set below was derived before the +> step-0 excursion was measured on the smoke cell. Three of those four fire on +> it. The `First bless` column now reflects that; the `Why` column keeps the +> original reasoning, because it was not wrong about the noise — it was wrong +> about `warmup_steps: 1` making the excursion unreachable. + +| Metric | Policy | First bless | Why | +|---|---|---|---| +| `median_tpot_ms` | max | **Gate** | Reproduced to 1.57% and 0.26% across sweeps, 3.08% over 12 same-node cell-runs, and the docs already name it "the better-behaved per-token metric". It is steady-state decode cost with no queueing term, which is why it is the tightest number we have — and it is measured between tokens, so the step-0 compile excursion does not enter it (1.01× on the excursion run). The one gate the measurement leaves standing. | +| `p99_itl_ms` | max | **Gate** | Promoted from record-only for the same reason: 1.00× on the excursion run, 5.43% clean spread. It is the useful half of the ITL pair, it catches tail stalls that a median cannot, and it is the only other metric whose definition excludes the excursion. Gating it and `median_tpot_ms` together covers per-token latency at both the centre and the tail without touching anything duration-derived. | +| `mean_step_time_ms` (`step_time_ms.max`) | max | **Record-only** (was: gate) | The bench step `duration`, and therefore the metric the excursion hits hardest: 2825 ms against a 1462 ms ceiling, 2.4× over. It is still the bound `--perf-gate` always writes, so arming it is the *default* — which is exactly why the rollout sequence below has to prune it by hand until `warmup_steps` is proven to cover the excursion. | +| `output_throughput` | min | **Record-only** (was: gate) | Reproduced to 2.63% and 0.30%, and it is the headline number — but it is tokens over the step duration, so the excursion drags it to 0.73× clean and through a 0.85 floor. Nothing is wrong with the metric; it simply cannot be gated while a five-second compile can land inside the window it divides by. | +| `median_ttft_ms` | max | **Record-only** (was: gate) | The original entry said "if one gate flaps, expect it to be this one", and that was right for the wrong reason — not a flap but a 10.26× excursion, 465.30 against a 58.59 ceiling. It carries queueing delay the others do not, and its first request is the one that waits for the compile. | +| `p99_ttft_ms` | max | **Record-only** | A p99 over 32 requests is the 32nd of 32 order statistics — effectively the maximum, and we have no repeat measurement of it. The load sweep shows it moving 194 → 2139 ms across shapes and 315 → 426 ms for a 2× concurrency change, so it is responsive to things a gate should not fire on. Promote on evidence from the record-only window. | +| `p99_tpot_ms`, `p99_e2el_ms` | max | **Record-only** | Same order-statistic argument, same absence of repeat data. (`p99_itl_ms` was in this row and has been promoted to the gate set above — it now has 13 same-node cell-runs behind it, not zero repeats, and it is one of the two metrics the step-0 excursion leaves alone.) | +| `median_e2el_ms` | max | **Record-only** | End-to-end latency is TTFT plus OSL × TPOT, so gating it adds a third alarm for an event two gates already catch, and its reason line is the least specific of the three. | +| `request_throughput` | min | **Record-only** | Under `ignore_eos` at fixed OSL it is `output_throughput / output_len` — fully determined by a metric already gated. | +| `total_token_throughput`, `tokens_per_sec` | min | **Record-only** | Restatements of `output_throughput` at fixed ISL/OSL (`tokens_per_sec` is documented as an alias of it). Gating them costs nothing but produces three reason lines for one event, which makes triage slower rather than safer. | +| `median_itl_ms` | max | **Never** | Measured at ~0, because the gateway delivers several tokens per SSE chunk. A margin is multiplicative, so an observation of 0.0 blesses a ceiling of 0.0 and every later run with any inter-token gap at all fails. `eval_lib` already warned about this in a comment; it is now enforced by `_NO_AUTO_GATE`. | +| `server_startup_sec` | — | **Never** | 189–379 s in the docs; **180–415 s measured** over the 13 smoke cell-runs, so the spread is wider than the quoted one, not narrower. It does not order by model size. Not in the allowlist and must stay out: the allowlist is what `--perf-gate` arms from, so adding it *is* gating it. | +| `container_elapsed_sec` | — | **Never** | Dominated by bring-up; same argument. | +| `duration`, `total_input_tokens`, `total_output_tokens` | — | **Never** | Work-done counters. The token totals are pinned by the recipe, so a bound on them restates the configuration rather than measuring the stack. | +| `completed_total`, `failed_total` | — | **Never** | Already enforced, harder, elsewhere: the bench script and the workload independently require `failed == 0` and `completed == num_prompts`, so a shortfall fails the *cell* and reddens the nightly with no baseline involved. A metric bound here would be a third and weaker copy of a check that already fails closed. | +| `max_output_tokens_per_s`, `max_concurrent_requests` | — | **Never** | Single-sample maxima — the noisiest available summary of a distribution. | +| `mean_*_ms`, `std_*_ms`, `p50_*`, `p90_*` | — | **Never** | Deliberately absent from the allowlist already ("means are the noisiest summary of a latency distribution and the least useful thing to gate on"). Keep them absent. | + +Two gates, then — `median_tpot_ms` and `p99_itl_ms`, the per-token pair — and +everything else charted, including three metrics that would have been gated +before the excursion was measured. That is a deliberately smaller first bless +than the plan originally proposed: the two that remain are the two whose +definitions exclude the failure mode we can actually demonstrate, and the three +that were dropped can be promoted from the record-only window as soon as it shows +the excursion is gone. The gap between "gated" and +"invisible" is covered by the dashboard's *What changed* view, which reports any +metric that moved more than 10% between the two most recent runs without failing +the job. A 10% throughput drift is real, is not a gate breach under these +margins, and is exactly what that view exists to surface. + +## How many record-only runs, and what the threshold comes from + +**Take ten nightlies before blessing. Derive each bound from the window's +extremum — the maximum for a `max` metric, the minimum for a `min` metric — not +from its mean, and not from one night's observation.** + +The statistic matters more than the count, so take that first. Deriving from the +mean is the intuitive choice and it is wrong here, because the variation we have +measured is not jitter around a centre. The startup series is 189 against a +cluster of 276–379: a single environmental excursion, not a spread. A mean is +precisely the statistic that hides one, and a bound built on it is breached by +the next occurrence. The extremum is the statistic that asks the question we +actually care about — *how bad has a healthy night ever been?* + +That also fixes the current tooling's real defect, which is not the margin size +but the anchor. `refresh_baselines.py --perf-gate` derives `value × 1.25` from +whatever single run was under way, so the bound depends on which night the +operator pressed the button. Feeding the seven known startup observations through +`compare_to_baseline` shows what that costs: + +| Blessed on | Ceiling (×1.25) | Later runs breaching | +|---|---|---| +| 189 s | 236.2 | 6 of 6 | +| 276 s | 345.0 | 1 of 6 | +| 285 s | 356.2 | 1 of 6 | +| 291 s | 363.8 | 1 of 6 | +| 316 s / 319 s / 379 s | 395.0 / 398.8 / 473.8 | 0 of 6 | + +Four of the seven possible blessing nights produce a gate that breaches, and one +of them reddens every subsequent run. The window maximum (379 → 473.8) breaches +none — but only because the window contained the excursion, which is the argument +for the count. + +Ten is chosen from that, not from convention. Scoring every *n*-run subset of the +series against the runs it did not contain gives the residual breach rate on an +unseen night: + +| Window size | Mean breach rate on unseen runs | Worst window | +|---|---|---| +| 1 | 21.4% | 6 breaches | +| 2 | 5.7% | 1 breach | +| 3 | 2.9% | 1 breach | +| 4 | 1.0% | 1 breach | +| 5 and up | 0% | none | + +The nightly runs roughly 250 times a year, so one false alarm per quarter needs a +per-run breach rate under about 1.1% — which this series reaches at n=4, on the +noisiest metric the workload produces. The metrics we are actually gating are +10–16× tighter than that one. Zero at n=5 is an artefact of a seven-point sample +rather than a real floor, so the honest reading is that the risk is small by 5 +and the remaining reason to go further is calendar coverage: ten nightlies is two +full weeks, long enough to contain a runner reimage, a Docker Hub re-pull or a +cold HF cache — the once-a-week-ish events that produce excursions in the first +place. **Five is the floor; below it the breach rate on known-noisy data is 2.9% +per run, about nine false alarms a quarter.** + +### A stack bump does not get absorbed by the window — it invalidates it + +An earlier version of the paragraph above listed "a Dependabot ROCm bump" beside +the runner reimage and the cold cache, as one more thing ten nights is long +enough to contain. That was wrong, and it is worth correcting rather than +quietly dropping, because the two categories look alike and are opposites. + +A reimage, a re-pull and a cold cache are *environmental* events: they perturb a +run and the stack underneath is the same before and after, so a window that +contains one has measured a genuine bad night and the extremum is doing exactly +its job. A digest bump is not that. It replaces the thing being measured +half-way through measuring it, so the window no longer describes one population. +You can **sample a stack bump or derive a stable ceiling across the window, but +not both** — the extremum then answers "how bad has a healthy night been on +either of two stacks", which is a number about neither. + +The counter-example is not hypothetical, and it happened while this branch was +open. `sanitizers-nightly.yml` went red on 2026-09-02 and stayed red, because +[#411](https://github.com/ROCm/aorta/pull/411) moved `Dockerfile.ci-gpu` from +ROCm 7.2.4 to ROCm 10 and the f32 GEMM code objects the gate scans are extracted +from the image's own Tensile bundle — so the committed expectation was describing +objects the image no longer ships +([#453](https://github.com/ROCm/aorta/issues/453)). That is a *correctness* +baseline invalidated by a mid-flight digest change. A perf baseline is not +better protected; it is worse, because a shifted number still looks like a +number and nothing about it announces that the stack moved. A mid-window bump +would do to the perf baseline precisely what that one did to the sanitizer +baseline. + +**Operational condition, for the duration of the window: hold any PR that +changes either pinned digest.** Concretely, the `FROM … @sha256` in +`docker/Dockerfile.ci-gpu` (currently +`rocm/pytorch:rocm10.0_ubuntu26.04_py3.14_pytorch_release_2.13.0@sha256:3174cb70…`) +and the engine digest in +`recipes/tokenspeed/tokenspeed-serve-bench-smoke.yaml` +(`lightseekorg/tokenspeed-amd@sha256:60c12e37…`). If one has to land, restart +the count; do not average across it. + +This is preventable by policy rather than by luck, which is the useful part. +`.github/dependabot.yml` runs the `docker` ecosystem against `/docker` on a +**weekly** schedule and its own comment says the job "bumps the `FROM ... @sha256` +digests", so an in-window bump is not merely possible but scheduled. There is +**no auto-merge** — the same file says so explicitly, and a human therefore has +to approve and merge one for it to reach the window. Ten nights is long enough +for roughly two of these to be opened, so the hold is a real decision someone +will be asked to make twice, not a theoretical one. + +Checked at the time of writing: the only open PR in the `docker` ecosystem is +[#309](https://github.com/ROCm/aorta/pull/309) (`bump ubuntu from 22.04 to +26.04 in /docker`), and it is **clear** — it touches +`Dockerfile.rocm-ubuntu-ebpf`, `Dockerfile.rocm70_2-ubuntu-nan` and +`Dockerfile.rocm70_2-ubuntu-pytorch`, none of which is `Dockerfile.ci-gpu`, and +it does not move the engine digest. The other open `dependabot/*` PRs (#310–#314) +are `github-actions` bumps. So nothing currently open needs holding; re-check +before the window starts, since the ecosystem is on a weekly timer. + +Everything else in the ten-night justification stands. The residual-breach-rate +analysis is unaffected — it is a statement about sampling a single population, +which is what this condition exists to preserve — and so is the calendar-coverage +argument for the three environmental events it still lists. + +### The extremum anchor has no good answer on a bimodal cell + +That whole derivation assumes the window's extremum is a *healthy worst case*. +The step-0 excursion breaks the assumption, and it is worth being explicit about +why, because it is the reason the gate set shrank rather than the window growing. + +With a bimodal cell the ten-night window either contains an excursion or it does +not, and both outcomes are bad: + +| Window | `median_ttft_ms` anchor | Ceiling | Consequence | +|---|---|---|---| +| No excursion (12 of 13 nights) | 46.87 | 58.59 | The excursion, when it lands, reads as a 10× regression. False alarm. | +| Contains one (1 in 13) | 465.30 | 581.63 | A genuine 2× TTFT regression — 47 → 94 ms — passes comfortably. No detection power at all. | + +Enlarging the window does not resolve this; it only makes the second row more +likely. The extremum is the right statistic for a unimodal metric with occasional +environmental excursions, which is what the startup series is. It is the wrong +statistic for a metric with two modes, because "the worst a healthy night has +been" is not a single number any more. + +So the correct response to a bimodal cell is not a cleverer threshold. It is +either to gate a metric the second mode does not reach — which is what +`median_tpot_ms` and `p99_itl_ms` are — or to remove the second mode. For the +step-0 excursion the second option is genuinely available, because the excursion +is positional: raising `warmup_steps` from 1 to 2 discards it by construction. + +**That has now been done** — the recipe sets `warmup_steps: 2` — which is what +should let the three duration-derived metrics be promoted later. The cost is that +it changes the measurement: every number in this document was taken at +`warmup_steps: 1`, so none of them is a baseline any more, and the ten-night +record-only window has to be taken afresh at the new setting before anything is +blessed. That is a deliberate trade. Carrying the excursion into the window +instead would have meant either blessing a bimodal cell or spending ten nights +establishing a distribution we already intended to change. + +For the concurrency-64 stall no such fix exists — it has no position to discard — +which is why that cell stays out of the nightly entirely rather than being gated +on a narrower metric. + +The margins themselves need no change. `--step-time-margin 0.25` and +`--throughput-margin 0.15` are already right for these metrics, and the +simulation says so: against the measured cross-sweep spread, every one of the +twelve bless-one-run-check-the-other combinations passes. Against the window +extremum the separation is wide — worst observed TPOT noise 1.57% versus a 25% +detection threshold, a factor of 16 — while a regression the size of a genuine +stack change is caught: + +| Scenario | `median_tpot_ms` vs 2.425 | `output_throughput` vs 3007.3 | +|---|---|---| +| the other measured night | 1.910 → pass | 3631 → pass | +| +10% / −10% | 2.134 → pass | 3184 → pass | +| +30% / −25% | 2.522 → **fail** | 2654 → **fail** | +| +94% / −46% (the 0.6B → 4B step) | 3.770 → **fail** | 1905 → **fail** | + +Those rows are in `tests/ci/test_eval_lib.py`, run against the real comparator, +so the claim is checked rather than asserted. + +## What blocks this today + +The matrix entry is live, and what remains is a decision rather than an +engineering task. + +`nightly_eval.py` runs *inside* the `aorta-ci-gpu` container +(`eval-reusable.yml` → `docker_cmd.sh exec`), and `tokenspeed_serve` runs the +TokenSpeed engine in a container of its own. So the entry needs a Docker client +and a route to a daemon in there. It had neither, and the workload's `setup()` +raised `'docker' not on PATH` before anything else happened. Record-only does not +help — it defers perf bounds, not failures. + +Both pieces now exist, the image has been built, and a cell has been run to +completion from inside the container (see +[Verification](#verification-status)). The client half is therefore done and +proven. The daemon half is a per-lane opt-in that no lane sets, because it grants +effective root on the runner and that is the CI owner's call. + +The entry lives in `entries` with `needs_docker_daemon: true`, which is what makes +that safe: on a runner without a socket it skips with the reason recorded, rather +than failing. `tests/ci/test_nightly_eval.py` covers both halves of that — the +skip when no daemon is reachable, and the run when one is — so the day a lane +flips the flag is a configuration change, not a discovery. + +### The enabling change + +**1. A Docker client in the CI image.** `docker/install_docker_cli.py`, invoked +from `Dockerfile.ci-gpu`, fetches the pinned static tarball, checks its sha256 +before extracting, and extracts exactly one member — `docker/docker`. The same +tarball ships `dockerd`, `containerd` and `runc`; naming the member is what keeps +a daemon out of the image, and a build-time check fails the build if any of them +reaches `PATH`. The engine container is a **sibling**, started by the host daemon +next to `aorta-ci-gpu`, not nested inside it. + +**2. A route to the daemon, opt-in per lane.** `docker-compose.docker-socket.yaml` +is an override, which is the mechanism the base compose already documents for +optional mounts ("Do not add a volume here"), so the default container still has +no route to the daemon. `rocm-ci-setup` takes a `docker-socket` input, default +`false`, and adds the override's `-f` only when it is `true`. + +That input is reachable from a caller workflow through a matching `docker_socket` +`workflow_call` input on `eval-reusable.yml`, also defaulting to `false` and +forwarded to the setup step. The default is what matters here: `eval-reusable.yml` +is shared by the nightly and by `bump-validate.yml`, so the choice has to be the +caller's rather than the reusable workflow's — setting it centrally would grant +the socket to a PR-triggered lane as a side effect of enabling the nightly. +`bump-validate.yml` therefore pins it `false` explicitly, and +`sanitizers-nightly.yml` never sees it, using `rocm-ci-setup` directly with no +`docker-socket` argument. **No lane currently sets it `true`** — see the security +note below. + +**3. `work_dir` must be an explicitly configured shared path.** This is the detail +most likely to be missed, because nothing reports it as a path problem. + +With the socket mounted, the `-v` sources in the `docker run` the workload builds +are strings the **host daemon** resolves against the **host** filesystem. The +workload builds them from `work_dir` — `/u/{scripts,out,hf}` — using +paths as seen from *inside* `aorta-ci-gpu`. Those are different namespaces. A +missing bind source is not an error to the daemon: it **creates** the directory on +the host and mounts that. So the engine container comes up with an empty +`/ts-scripts` and dies on a missing script, or — worse — with an empty writable +`/ts-out`, and the harvest reports no results for a run that really happened. + +The default `work_dir` does **not** fix this by being `/tmp/ts-work-serve` on both +sides. `/tmp` inside the CI container is the container's own `/tmp`, not the +host's, so the two are different directories that share a name — which is the +failure above with the confusing property that the path looks right in every log. + +What the nightly needs, concretely: + +- The socket override bind-mounts `${TS_SERVE_WORK_DIR:-/tmp/ts-work-serve}` at + **the same path** inside the container. Source and target are identical on + purpose, and a test asserts they stay identical; that is what makes one string + name one directory on both sides of the boundary. +- The recipe sets `work_dir` to that same path **explicitly**. It happens to equal + the workload default, but a default that must agree with a mount is a + coincidence, not a configuration: change either one alone and the run breaks + quietly. +- The path must be node-local. The workload already rejects an NFS home, for the + root-squash reason documented in + [tokenspeed-serving.md](tokenspeed-serving.md), and that reason is stronger + here, not weaker. + +**Measured, and more specific than the above was.** Demonstrating this on a +gfx950 node produced two failures worth writing down, because neither is what the +guidance as written would have led you to expect. + +*Every* bind source must be resolvable by the daemon, not only `work_dir`. The +first attempt put `AORTA_WORKSPACE` on an autofs-mounted NFS home and the +container never started: + +``` +Error response from daemon: error while creating mount source path +'/home/.../wt-gating': mkdir /home/...: permission denied +``` + +The daemon resolves *all* `-v` sources in the host namespace, so a checkout on an +automounted or root-squashed filesystem fails the same way a `work_dir` there +would — and it fails at container create, before any of the workload's own +checks can produce a better message. On a GitHub runner the workspace is local +disk and this does not arise; on any node where `/home` is networked, the repo +has to be staged locally first. Worth knowing before debugging it as a socket +problem, which is what it looks like. + +**The work root must be owned by the uid the container runs as, and the obvious +way to create it gets that wrong.** `aorta-ci-gpu` runs as root, so the workload +checks `u0` and requires the root itself to be root-owned. A `work_dir` created +by an ordinary host-side run is owned by *that* user, and the containerised run +then refuses it — the node used here already had a `/tmp/ts-work-serve` at +`uid=100550 mode=1777` left by earlier host-side sweeps, which uid 0 must reject. +The demonstration used a fresh path created *through the daemon* so it landed +root-owned, which is exactly what the workload's own error message advises an +administrator to do. + +The operational trap in that is worth stating on its own: **the ten record-only +runs cannot share a `work_dir` with the containerised nightly** if they are taken +by hand as an ordinary user. Either take them as root, or give the two a separate +`work_dir` and accept the cold HF cache on the first nightly. + +**The uid has to agree too**, and this is a second way the same boundary bites. +The workload uses a per-uid scratch root and then *verifies* it: `/u` +must be a real directory (not a symlink), owned by the running uid, and not group- +or world-writable. Inside `aorta-ci-gpu` the process is **root** (`user: root` in +the base compose), so it writes and checks `u0`. That works — a root-owned +directory created through the shared mount is root-owned on the host too, so the +host daemon and the checking process agree. But it holds only because both sides +are uid 0. Change `user:` in the base compose, run the harness as a non-root uid, +or turn on userns-remap on the daemon, and `/u` is created by one +uid and inspected by another: the check fails with an ownership error that does +not mention containers, uids-across-a-boundary, or the mount. Anyone changing +either should expect that error to be the symptom. + +One consequence worth stating: with `run_as_current_user` defaulting true, the +engine container also runs as uid 0, so exports land on the host owned by root, +outside the workspace that `eval-reusable.yml`'s "Reclaim results ownership" step +chowns. They are cleaned per trial unless `keep_work_dir` is set, but a runner +that fills `/tmp` with root-owned scratch is a plausible future complaint. + +### Security: this grants effective root on the runner + +Stated plainly, because it is the part most easily lost in a diff: a process that +can talk to the Docker daemon can ask it for a privileged container that +bind-mounts `/`. That is root on the host, with no exploit involved. Mounting the +socket into `aorta-ci-gpu` therefore hands the host to anything running in that +container — including every package the nightly installs from an index. + +The mitigating argument is real but partial. The container is already +`privileged: true` with `seccomp=unconfined` on a self-hosted runner, so on a +dedicated, trusted, single-tenant node this widens an already-wide posture rather +than opening a new one. The argument fails on a shared or multi-tenant runner. + +**If the CI owner is not willing to accept it, the recommended alternative is to +run the serving workload outside the CI container entirely** — as a step on the +runner host, which already has a client and the socket, publishing its results +JSON into the harness's results directory. The nightly keeps the socket out of +the container, and the only cost is that this one cell is invoked differently +from the other sixteen. A socket proxy allowlisting just the container +create/start/logs/remove calls is a middle option; rootless Docker is a third. +Nested docker-in-docker is not on the list: it needs `privileged` too, so it +trades no privilege away, and it gives the engine a different filesystem view, +which breaks exactly the bind mounts described above. + +This branch deliberately leaves the decision open: the mechanism is in place and +switched off, and turning it on is one flag plus a named sign-off. + +### What is still missing + +**One thing, and it is a decision rather than a piece of work: no lane enables +the socket.** That is the sign-off described above, and it is deliberately not +this branch's to give. + +Everything else on this list has been done. The entry is therefore in `entries` +rather than `pending_entries`, carrying `needs_docker_daemon: true` — so on a +runner without the socket it **skips**, with the reason in the results, instead +of failing on `Cannot connect to the Docker daemon`. When the nightly lane sets +`docker_socket: true` the entry starts running with no further edit. Promoting it +without that flag would have been the mistake the matrix file's own header warns +about: an entry may only be added once it can actually pass on the runner. + +### Verification status + +Built and run on a gfx950 (MI355X) dev node on 2026-09-02 — **not** on the CI +runner, which is an MI350X. Read the hardware note further down this section +before treating any absolute number here as something the nightly should +reproduce. + +**The image builds and is client-only.** `docker compose --env-file .env.ci -f +docker-compose.build.yaml build` succeeds, producing a 51.9 GB `aorta:ci-gpu`. +The build's own `RUN` checks pass — `docker client check: OK -- +/usr/local/bin/docker, no daemon on PATH` — and the assertion holds when +re-checked against the built image rather than during it: `dockerd`, +`containerd`, `runc`, `containerd-shim-runc-v2`, `docker-proxy` and +`docker-init` are all absent from `PATH`, and `docker --version` reports +**29.7.2**. That is what makes "client only" a property of the artifact and not +of the Dockerfile's intent. + +**The client reaches the host daemon from inside the container.** With the +override mounted, `docker exec aorta-ci-gpu docker ps` returns the running +container list, exit 0. + +**A cell completes.** Two sweeps of `tokenspeed-serve-bench-smoke.yaml` were +driven from inside `aorta-ci-gpu`, each starting the TokenSpeed engine as a +sibling container: four cell-runs, all passed, `failed_total` 0 and +`completed_total` 96 throughout, engine containers cleaned up afterwards. The +twelve measured steps were 1108–1149 ms with no step-0 excursion, and every +metric landed within 5% of the host-side clean envelope — so the container +boundary does not move the measurement, which is what lets a nightly baseline be +compared against the host-side runs the variance analysis is built from. + +> [!IMPORTANT] +> **The 1108–1149 ms envelope is not a night-one acceptance check.** It was +> measured on an **MI355X** dev node. The CI runner +> `smci350-rck-g03-f16-12` is an **MI350X** — same gfx950/CDNA4 ISA, but a lower +> power budget and air rather than liquid cooling. Correctness is unaffected by +> that difference: the ISA is identical, so the kernels, the compiler output and +> every verdict the harness produces are the same. Absolute step times are a +> different matter and will plausibly differ — a priori, at least; one MI350X +> cell has since been measured and did not show an offset, which narrows the +> expectation without licensing the envelope as a check. See the subsection +> below. +> +> So do **not** use this envelope to accept or reject night one, in either +> direction. A first nightly landing outside 1108–1149 ms is not evidence of a +> problem, and one landing inside it is not evidence that the lane is healthy. +> +> **What has to reproduce across the window is the spread, not the level.** The +> ceilings are derived from the ten CI nights themselves — `max × 1.25` and +> `min × 0.85` over that window, on that runner — so a systematic offset between +> the dev node and the runner is absorbed by construction and is expected, +> harmless and not worth investigating. What would matter is the *shape*: a +> window spread materially wider than the measured few percent, or a step-0 +> excursion, and step 4 already checks both. Judge night one against the two +> checks in step 4 and against the previous nights on the same runner, never +> against a number taken on other hardware. + +#### One MI350X cell at `warmup_steps: 2` (2026-09-03) — a data point, not a spread + +Both caveats above — MI355X hardware, and `warmup_steps: 1` — are now partly +addressed by one measurement, and it is recorded here with its limits stated +first because it is easy to over-read. + +**This is a single cell-run. It is not a variance estimate and must not be used +as one, and it does not replace anything above.** The +[13-cell-run table](#the-smoke-cell-is-not-unconditionally-clean-measured-13-cell-runs) +stays exactly as it is — it remains the rationale for `warmup_steps: 2`, and one +cell cannot restate it. The ten-night window is still the only thing that +produces a spread at the new setting. + +Run on **`cv350-rck-g03-c16-18`** (Slurm partition `meta64`), which is an +**MI350X**: 1000 W package power cap, VBIOS `113-M350-01-1K5-000C`, gfx950, +288 GiB HBM. The 1000 W cap and the air-cooled VBIOS are what identify it as an +MI350X rather than an MI355X, and it is the *same hardware class as the CI +runner* — which is the point. Deliberately **not** on +`smci350-rck-g03-f16-12`, which is reserved for the baseline window. One +`baseline` cell of `tokenspeed-serve-bench-smoke.yaml` with every +measurement-relevant setting as committed (image digest, Qwen3-0.6B, ISL 512 / +OSL 128, concurrency 8, 32 prompts, 3 measured steps, `num_warmups: 1`, +`warmup_steps: 2`, `ignore_eos`, seed 0), on a private `work_dir` so it cannot +touch the CI lane's. Passed, `failed_total` 0, `completed_total` 96. + +| Metric | MI350X, 1 cell, `warmup_steps: 2` | MI355X clean range, `warmup_steps: 1` | +|---|---|---| +| step times, in order | 1135.03 / 1136.39 / 1136.51 ms | 1108 – 1178 ms | +| `median_ttft_ms` | 45.00 | 43.65 – 46.87 | +| `median_tpot_ms` | 1.910 | 1.89 – 1.95 | +| `p99_itl_ms` | 34.64 | 34.00 – 35.84 | +| `output_throughput` | 3605.7 | 3502.53 – 3646.20 | +| `server_startup_sec` | 282 | 180 – 415 | + +Two things worth taking from it, and one worth not taking. + +**The MI350X level is not detectably different at this workload size.** Every +metric lands inside the MI355X clean range, step time included. That is a +plausible result rather than a surprising one: Qwen3-0.6B at concurrency 8 is a +small, latency-bound load that does not approach either card's power ceiling, so +the MI355X's higher budget and liquid cooling have nothing to bite on. It does +**not** license using the MI355X envelope as an acceptance check — one cell +cannot establish agreement, and the rule above stands unchanged — but it does +mean a large night-one offset would itself be worth a look, rather than being +shrugged off as expected hardware difference. + +**No step-0 excursion, with the excursion's own signature absent.** The first +measured step is the *fastest* of the three, so there is no positional outlier at +all. This is the first observation at `warmup_steps: 2` and n=1, so it is +consistent with the change working and nowhere near sufficient to conclude it; +step 4's bimodality check over twenty cell-runs is what settles that. + +**What not to take from it:** the three step times span 0.13%, and that number is +**not** comparable to the 6.3% step-time or 3.08%/5.43% metric spreads in the +table above. Those are across *twelve cell-runs* — separate server bring-ups on +separate invocations, which is where the variance lives. 0.13% is three +consecutive steps against one already-warm server, and it is the *within*-cell +figure that the earlier table never reported. Reading it as a 48× variance +improvement would be a category error. Re-taking the spread needs the window. + +Two configuration requirements were discovered by doing this rather than by +reading, and are written up under [work_dir](#the-enabling-change) above: all +bind sources must be resolvable by the daemon (an autofs NFS checkout is not), +and the work root must be root-owned because the container runs as uid 0. + +Also verified earlier, and still true: the installer rejects a wrong sha256 +without leaving a binary behind; the client negotiates against an older (29.1.3) +daemon; `docker compose config` resolves all three mounts with the scratch mount +identical on both sides. + +Not verified, because it is the thing awaiting sign-off: the entry has never run +under `nightly_eval.py` on a CI runner, only under `aorta sweep run` in the +container by hand. The skip path is covered by tests rather than by a runner. + +To reproduce: + +```bash +cd docker +bash ../scripts/ci/docker_compose.sh --env-file .env.ci -f docker-compose.build.yaml build +docker run --rm aorta:ci-gpu docker --version + +# work root must be root-owned; creating it through the daemon is the easy way +docker run --rm -v /tmp:/mnt busybox:1.37 mkdir -p /mnt/ts-work-serve + +export TS_SERVE_WORK_DIR=/tmp/ts-work-serve +bash ../scripts/ci/docker_compose.sh --env-file .env.ci \ + -f docker-compose.build.yaml -f docker-compose.docker-socket.yaml up -d +docker exec aorta-ci-gpu docker ps +docker exec aorta-ci-gpu aorta sweep run \ + --recipe recipes/tokenspeed/tokenspeed-serve-bench-smoke.yaml \ + --output-dir "$TS_SERVE_WORK_DIR/out" --strict +``` + +## The rollout sequence + +Steps 1–2 are **done**. Step 3 cannot start until a lane enables the socket, +because until then the entry skips rather than records. Everything from step 3 is +the part this document is really specifying. + +**1. ~~Promote the entry.~~ Done.** It is in `entries` with `min_gpus: 1`, +`timeout_sec: 3600` and `needs_docker_daemon: true`. + +`timeout_sec: 3600` rather than the 1800 default: two bring-ups at the observed +spread plus the teardown VRAM drain is around 15 minutes before the image pull +and any cold-cache weight download, and a timeout is an unconditional entry +failure rather than a slow record. Check the job's own `timeout-minutes: 150` in +`eval-reusable.yml` still has room. Measured end to end from inside the +container, a full two-cell sweep took **12 minutes** on a warm cache, so the +budget is right but most of it is bring-up. + +**2. ~~Verify it before merging.~~ Done**, and rerun these after any change: + +```bash +python -m pytest tests/ci -q --timeout=180 +python -m pytest tests/workloads/test_tokenspeed_serve.py -q +aorta sweep run --recipe recipes/tokenspeed/tokenspeed-serve-bench-smoke.yaml --dry-run +``` + +**2a. Enable the socket on the nightly lane.** The plumbing is in place; the +decision is not this branch's to make — see +[Security](#security-this-grants-effective-root-on-the-runner). It is one line in +`.github/workflows/nightly-eval.yml`, plus a named sign-off: + +```yaml + uses: ./.github/workflows/eval-reusable.yml + with: + docker_socket: true +``` + +The `docker_socket` input on `eval-reusable.yml` defaults to `false` and is set +per caller, so this arms the nightly lane and nothing else. `bump-validate.yml` +sets it `false` explicitly: that lane runs PR head code on the same self-hosted +runner and must keep no route to the host daemon. `sanitizers-nightly.yml` does +not go through `eval-reusable.yml` at all — it uses the `rocm-ci-setup` action +directly and passes no `docker-socket`, so it inherits the action default of +`false`. Setting the input inside `eval-reusable.yml` instead of per caller would +hand the socket to every lane at once and is the mistake this shape exists to +prevent. + +Until the sign-off lands, leave it `false`: the entry reports `skip` with +`needs a docker daemon: ...` in its reasons, which is visible on the dashboard +and costs the nightly nothing. + +**3. Let it record for ten nightlies, at `warmup_steps: 2`.** It will report +`recording` on the dashboard. + +> The window is only valid at the setting the gate will run at, and that setting +> is now `warmup_steps: 2` in +> `recipes/tokenspeed/tokenspeed-serve-bench-smoke.yaml`. Confirm that before +> counting nights, and restart the count if it changes underneath the window. +> None of the numbers earlier in this document can substitute for any of these +> ten: they were measured at `warmup_steps: 1`, and the change was made +> specifically to alter the behaviour they describe. Expect the window to be +> *cleaner* than the 13-cell-run table — that is the change working — but do not +> assume it; the point of the window is to measure it rather than predict it. + +> **And do not accept or reject night one against any number in this document.** +> All of them were taken on an MI355X dev node; the runner is an MI350X. A +> systematic offset in the absolute level is expected and harmless, because the +> ceilings come from these ten nights on this runner. It is the spread that has +> to reproduce — see the hardware note under +> [Verification status](#verification-status), and the single MI350X data point +> recorded there. + +Also confirm before counting that neither pinned digest is about to move: an +in-window bump invalidates the window rather than being absorbed by it, which is +[its own section](#a-stack-bump-does-not-get-absorbed-by-the-window--it-invalidates-it) +and the reason for the digest hold. + +Watch the *Workloads* view and write the numbers down; the ten values +of `median_tpot_ms` and `p99_itl_ms` per cell are the input to step 4, and the +ten of `median_ttft_ms`, `output_throughput` and `mean_step_time_ms` are what +decides whether the three record-only metrics can be promoted later. Two cells, +so twenty observations of each. A `fail` during this window is a real failure — +the entry is unbaselined but the harness is fail-closed — and must be fixed +rather than waited out. + +**4. Check the window before blessing, and check it for bimodality first.** +Compute, per cell and per metric, the extremum and the ratio of extremum to +median. Two separate checks: + +- *Spread.* If either gated metric shows a window spread above about 10%, do not + bless it — the margin was sized for a 3% spread and a 10% one means something + is varying that we have not identified. Demote it in step 5 and investigate. +- *Bimodality.* Look at the per-step times, not only the per-cell summary. If any + night's first measured step stands well clear of its other two, the compile + excursion is still reachable and the three record-only metrics stay + record-only regardless of how good their spread looks — a clean ten-night + window over a bimodal cell is the exact situation where the extremum anchor + gives a threshold with no detection power. See + [the bimodal-cell section](#the-extremum-anchor-has-no-good-answer-on-a-bimodal-cell). + +If the window shows no excursion in twenty cell-runs, that is reasonable evidence +it has stopped happening, and the three can be promoted with the same margins. + +This window is the first measurement at `warmup_steps: 2`, so it is also the +test of whether that change did what it was supposed to. Two outcomes worth +telling apart: no excursion in twenty cell-runs is the expected result and clears +the three duration-derived metrics for promotion in step 7; an excursion that +still appears at position 0 means two discarded steps are not enough to cover +the compile, which is new information and should be investigated rather than +absorbed by raising `warmup_steps` again. + +**5. Bless, scoped to this entry only.** + +``` +Actions -> Refresh baselines -> Run workflow +``` + +with the dispatch form filled in as + +| Input | Value | +|---|---| +| `perf_gate` | `true` | +| `perf_gate_entry` | `tokenspeed_serve_smoke` | +| `step_time_margin` | `0.25` (default) | +| `throughput_margin` | `0.15` (default) | + +which the workflow turns into + +```bash +python scripts/ci/refresh_baselines.py --perf-gate \ + --perf-gate-entry tokenspeed_serve_smoke +``` + +The scope is not optional. Without it the same PR arms step-time ceilings for +every other entry in the matrix from that one run, so leaving `perf_gate_entry` +empty with `perf_gate: true` logs a warning on the job. It is a warning rather +than a hard failure because an unscoped refresh is the legitimate end state once +every workload has variance data behind it — but during rollout it is not what +you want. A misspelled entry name is rejected rather than silently scoping to +nothing, and setting `perf_gate_entry` without `perf_gate` fails immediately +instead of after the wheel install. + +`perf_gate_entry` accepts several names, comma- or space-separated, mapping to +one `--perf-gate-entry` each. For this rollout it should be exactly one. + +**6. Correct the PR diff by hand, then merge.** The refresher derives bounds from +the *single* run it just did, and from every auto-gateable metric it observed. +Two edits are needed, both mechanical: + +- Replace each bound with one derived from the ten-run window: `max × 1.25` for a + `max` metric, `min × 0.85` for a `min` metric. +- Delete the keys this document lists as record-only — `p99_ttft_ms`, + `p99_tpot_ms`, `median_e2el_ms`, `p99_e2el_ms`, `request_throughput`, + `total_token_throughput`, `tokens_per_sec`, and now also `median_ttft_ms`, + `output_throughput` **and the `step_time_ms.max` entry** — leaving + `median_tpot_ms` and `p99_itl_ms`. `median_itl_ms` will not be there; + `_NO_AUTO_GATE` keeps the refresher from writing it. + +Deleting `step_time_ms.max` is the one that needs attention, because it is the +bound `--perf-gate` always writes and the only one on this list that is not a +metric key. Leaving it in by inattention arms the gate the measured excursion +breaches hardest — 2825 ms against a 1462 ms ceiling. It is the most likely +mistake in this whole sequence. + +This hand-editing is the honest cost of the current tooling, and it is bounded: +ten keys across two cells, on a PR a human reviews anyway, and it is meant to +shrink as the record-only metrics are promoted on evidence. It got larger rather +than smaller when the excursion was measured, which strengthens the case below +for a per-metric scope. + +**7. Promote the record-only metrics on evidence, not on schedule.** After +another ten nightlies under the live gate there are twenty more observations. +Take them in two groups, because they are blocked on different things: + +- `median_ttft_ms`, `output_throughput` and `step_time_ms.max` are blocked on the + step-0 excursion, not on their spread — which is already good enough. The + `warmup_steps: 2` change is intended to remove the blocker, so promote them + once the step-3 window shows twenty cell-runs with no excursion. That window + *is* the re-taken one; there is no separate re-take to schedule. +- The remaining `p99_*` metrics are blocked on having no repeat data. Promote any + whose window spread is comparable to its median's. `p99_ttft_ms` gated is worth + more than `median_ttft_ms` gated, because tail latency is what a serving + regression damages first — we just have no basis for a bound on it yet. + +If the hand-editing recurs every refresh rather than converging, the fix is a +per-metric scope alongside `--perf-gate-entry`. That is the point to build it — +not before, when the right metric set is still a guess. + +## When a gate fires + +The alert names the cell, the metric, the observed value and the bound. Three +things it can be, and they are distinguishable without a GPU: + +**A stack change.** Check the dashboard's *What changed* line first: it reports +whether PyTorch, ROCm or HIP moved that night. If one did, and the direction of +the metric move is plausible for it, this is a stack effect and the answer is +either an upstream bug report or a re-bless. Do not re-bless silently — a bump +that costs 20% of decode throughput is exactly the result the nightly exists to +produce, and `docs/ci-nightly-eval.md` is explicit that the baseline diff *is* +the bump's impact. + +**A noise event.** The two cells of this recipe differ only by mitigation and run +minutes apart on the same node, which is the reason this recipe was chosen. Use +it: if `baseline` and `no-scratch-reclaim` both moved by a similar amount, the +node or the stack changed. If exactly one moved, and the mitigation has never +shown an effect before at any model size, that is a single-cell excursion — a +co-tenant, a thermal event, a cold cache. Confirm by looking at the previous +nights on the dashboard's run history; a noise event is one red cell in a column +of green, a regression is a column that turns red and stays red. Also check +`server_startup_sec` for that cell: it is not gated, but a bring-up at the top of +its range is a good indicator that the node was busy. + +**A real regression.** Everything else — both cells moved, no toolchain change, +and the next night reproduces it. Reproduce locally with the recipe as committed +and bisect against the pinned image digest. Note that the image is pinned by +digest precisely so this case cannot be caused by TokenSpeed changing underneath +the baseline. + +If you cannot tell which of the three within one working day, revert to +record-only and keep collecting. An unexplained gate is worth less than the +`recording` state it came from. + +## Reverting to record-only + +Delete the `metrics` and `step_time_ms` keys for the affected cells from +`config/ci/regression_baselines.yaml`, leaving `passed: true`. That returns those +cells to record-only immediately — the comparator treats an absent bound as +nothing to check — while keeping correctness gating and the fail-closed behaviour +intact. It is a small, reviewable, obviously-correct diff, which is what you want +at the point where a gate is misbehaving. + +Do **not** revert by deleting the matrix entry. That stops the measurement as +well as the gate, and the record-only data is the thing needed to size a better +bound. + +Do **not** revert by widening the margin as a first move. A margin wide enough to +absorb an unexplained excursion is usually too wide to catch a regression, and +the widening tends to be permanent. Go back to record-only, find out what moved, +then re-bless from a longer window. + +One whole-file caveat: any later `refresh-baselines` run rewrites +`regression_baselines.yaml` completely, so a hand-reverted cell will be re-armed +by the next unscoped `--perf-gate` refresh. Scope those runs with +`--perf-gate-entry`, or check the diff for cells you had deliberately demoted. + +## Assumptions + +The user referenced `Aorta planning & Updates.docx`, which is not reachable from +this machine. Everything above was derived from the repository, and where that +was not enough a decision was made and is recorded here. + +- **Nightly cadence and volume.** ~250 nightly runs a year, and one false alarm + per quarter is the tolerable rate. This sets the window length; a stricter + target needs a longer window, not a wider margin. +- **The recipe.** `tokenspeed-serve-bench-smoke.yaml` was chosen over the other + four serving recipes as the cheapest that still answers something — Qwen3-0.6B + is ~1.2 GB of weights against ~40 GB for the gpt-oss pair, and two cells rather + than four or six. The tie-breaker was triage rather than cost: it is the only + serving recipe whose cells differ *only* by mitigation, so the pair is a + same-night control. `tokenspeed-serve-gptoss.yaml` is the recipe whose numbers + matter most (it is TokenSpeed's canonical AMD benchmark) and is the right + second entry once this one has been gated for a month. +- **Cost.** Measured at **12 minutes** of runner time per nightly for the two + cells on a warm HF cache, driven from inside the CI container — close to the 15 + minutes originally estimated. Not separately budgeted with anyone. +- **`--perf-gate-entry` over hand-pruning.** Scoping was added as code because + the alternative recurs on every refresh, for sixteen cells belonging to other + people's workloads, in a diff where the omission looks identical to the + inclusion. The per-metric pruning in step 6 was left manual for the mirror-image + reason: it is ten keys in one entry, on a PR a human reviews anyway, and + building a flag for it now would fix a metric set we are explicitly planning to + change. +- **`needs_docker_daemon` over leaving the entry staged.** A launch was + demonstrated, so the entry has earned promotion; but the socket is still off by + design, and an entry in `entries` that cannot reach a daemon fails every night. + Rather than choose between a stale `pending_entries` row and a red nightly, the + capability was made declarable, exactly as `min_gpus` already is for GPU count. + The cost is one field and one probe; the alternative was for the promotion to + wait on a security decision it does not actually depend on. +- **`warmup_steps` raised to 2.** Reversed from "left at 1" earlier in this + branch. The step-0 excursion is positional, so one more discarded step removes + it by construction, and it is measured at 1 cell-run in 13 — frequent enough + that a ten-night window taken at 1 would probably contain one and produce + either a false alarm or a threshold with no detection power. The objection to + bundling it was that it changes the measurement and invalidates every number + here; that is true and is now the accepted cost, because those numbers were + never going to be the bless baseline — the window is, and the window has not + been taken yet. Changing the setting *before* the window costs nothing; + changing it after would have cost ten nights. The variance table is kept as + the rationale. +- **`median_itl_ms`.** Kept in the allowlist and excluded from auto-blessing, + rather than removed. Removing it would make a legitimate hand-written bound + impossible; the problem is only ever with a bound derived from a margin. +- **TP and multi-GPU serving.** Out of scope. TP=4 does not come up on this + image, and TP=2 buys 4.5% throughput for a second GPU, so neither is worth a + gate before the single-GPU one has proven itself. diff --git a/docs/tokenspeed-serving.md b/docs/tokenspeed-serving.md index 8b647027e..8b92075b1 100644 --- a/docs/tokenspeed-serving.md +++ b/docs/tokenspeed-serving.md @@ -921,6 +921,13 @@ To gate a serving recipe in the nightly: add it to `config/ci/nightly_eval_matrix.yaml`, let it run record-only, then bless it via the `refresh-baselines` workflow. See [ci-nightly-eval.md](ci-nightly-eval.md). +Which of those metrics should actually get a bound is a separate question from +whether they can, and the answer is not "all of them": bring-up ranges 189-379 s +on one node with nothing changed, while the steady-state rates reproduce to +within 3% across sweeps. [tokenspeed-gating-rollout.md](tokenspeed-gating-rollout.md) +works through the per-metric decision, how many record-only runs to take first, +and what the thresholds should be derived from. + ## Tests ```bash diff --git a/recipes/tokenspeed/tokenspeed-serve-bench-smoke.yaml b/recipes/tokenspeed/tokenspeed-serve-bench-smoke.yaml index f0e81f447..a412e279f 100644 --- a/recipes/tokenspeed/tokenspeed-serve-bench-smoke.yaml +++ b/recipes/tokenspeed/tokenspeed-serve-bench-smoke.yaml @@ -47,6 +47,18 @@ workload_config: output_len: 128 max_concurrency: 8 num_warmups: 1 + # Two discarded bench steps, not the default one. A step-0 Triton compile + # excursion shows up in roughly 1 cell-run in 13 -- a ~10x first-token spike + # (465 ms against a 43-47 ms clean range) and a 2825 ms step against a + # 1108-1178 ms range -- and it survives `warmup_steps: 1`, because the compile + # lands in the first *measured* step after the single warmup step has already + # run. `num_warmups` cannot cover it: that warms requests within one bench + # invocation, not the compile cache across invocations. The excursion is + # positional, so discarding one more whole step removes it by construction. + # See the variance section of docs/tokenspeed-gating-rollout.md -- and note + # that every number in that table was taken at `warmup_steps: 1`, so the + # record-only window has to be re-taken at this setting before any bless. + warmup_steps: 2 # Hold OSL fixed. Without it a model that emits EOS early produces short # outputs, so TPOT and throughput would describe a different amount of work # per cell and the two cells would no longer be comparable. diff --git a/scripts/ci/dashboard_metadata.py b/scripts/ci/dashboard_metadata.py index 54b09c2ad..bc3bff0a9 100644 --- a/scripts/ci/dashboard_metadata.py +++ b/scripts/ci/dashboard_metadata.py @@ -199,6 +199,11 @@ def _workload_repro( "summary": "Offline LLM prefill/decode latency and throughput.", "workloads": ["inference_offline"], }, + "serving": { + "label": "Serving", + "summary": "Online LLM serving latency (TTFT/TPOT) and token throughput.", + "workloads": ["tokenspeed_serve_smoke"], + }, "training": { "label": "Training", "summary": "PyTorch DDP and FSDP training step times.", @@ -293,6 +298,73 @@ def _workload_repro( ), ), }, + "tokenspeed_serve_smoke": { + "title": "TokenSpeed online serving", + "summary": ( + "Time to first token, time per output token, and token throughput " + "from a containerised TokenSpeed server." + ), + # Deliberately not the p99s or median_itl_ms: see + # docs/tokenspeed-gating-rollout.md for which serving metrics are + # stable enough to read as headline numbers. + "headline_metrics": [ + "median_ttft_ms", + "median_tpot_ms", + "output_throughput", + ], + "recipe": "recipes/tokenspeed/tokenspeed-serve-bench-smoke.yaml", + "min_gpus": 1, + "run_command": ( + "aorta sweep run --recipe " + "recipes/tokenspeed/tokenspeed-serve-bench-smoke.yaml" + ), + "repro": _workload_repro( + entry_name="tokenspeed_serve_smoke", + prerequisites=[ + "One gfx950 (MI355X) or gfx1250 GPU — the TokenSpeed image targets these", + "A working docker client and daemon: the engine runs in its own container", + "A node-local work_dir — an NFS home under root-squash cannot be bind-mounted", + "Egress to the Hugging Face Hub, or a pre-populated cache plus hf_offline", + ], + recipe="recipes/tokenspeed/tokenspeed-serve-bench-smoke.yaml", + run_command=( + "aorta sweep run --recipe " + "recipes/tokenspeed/tokenspeed-serve-bench-smoke.yaml" + ), + min_gpus=1, + distributed=False, + dry_run=( + "aorta sweep run --recipe " + "recipes/tokenspeed/tokenspeed-serve-bench-smoke.yaml --dry-run" + ), + verify_title="Inspect serving latency and throughput artifacts", + verify_extra=[ + _METRICS_IN_PERF.format( + pattern="ttft|tpot|output_throughput|completed_total" + ), + ], + success=( + "Both cells pass with completed_total == num_prompts * steps and " + "failed_total == 0. median_ttft_ms / median_tpot_ms / " + "output_throughput appear in perf.md. Serving cells are " + "record-only until a baseline is blessed — see " + "docs/tokenspeed-gating-rollout.md for which metrics get bounds " + "and why bring-up time never does (189-379s on one node with " + "nothing changed)." + ), + setup_extra=[ + { + "title": "Serving-specific: pre-warm the model cache as the running uid", + "commands": [ + "export HF_HOME=/tmp/ts-work-serve/u$(id -u)/hf", + "# run_as_current_user defaults to true: a cache populated by a", + "# root container leaves the trial failing with PermissionError", + "docker pull lightseekorg/tokenspeed-amd@sha256:60c12e37c01496891053b9c30c4204e5d1cf9b4b641859d3aadcbd95bccc7c78", + ], + }, + ], + ), + }, "training_ddp": { "title": "PyTorch DDP training (2 GPU)", "summary": "Distributed data parallel training step time on two GPUs.", diff --git a/scripts/ci/eval_lib.py b/scripts/ci/eval_lib.py index ae8e6f94f..ea65e5ef8 100644 --- a/scripts/ci/eval_lib.py +++ b/scripts/ci/eval_lib.py @@ -66,6 +66,22 @@ def cell_key(entry_name: str, cell_name: str) -> str: "checksum": "equal", } +# Allowlisted metrics that must never be blessed AUTOMATICALLY -- gateable when a +# baseline names them by hand, but skipped by `refresh_baselines.py --perf-gate`. +# +# The allowlist above answers "may this metric be gated at all", and until now +# that was also the whole of the answer to "should --perf-gate emit a bound for +# it", because the refresher gates every allowlisted metric it observes. Those +# are different questions for a metric whose observed value sits at or near +# zero: --perf-gate derives `value * (1 + margin)`, and a relative margin around +# zero is not a bound. `median_itl_ms` is measured at ~0 (the gateway delivers +# several tokens per SSE chunk, so most recorded inter-token gaps are ~0 and the +# real gaps land in `p99_itl_ms`), and an observation of exactly 0.0 blesses a +# ceiling of 0.0 -- after which ANY positive inter-token gap fails the cell. That +# is not a noisy gate, it is one that cannot pass. Excluding it here keeps the +# metric charted and hand-gateable while stopping a refresh from arming it. +_NO_AUTO_GATE: frozenset[str] = frozenset({"median_itl_ms"}) + def metric_policy(name: str) -> str | None: """Return the comparison policy (min/max/equal) for a metric, or None if the @@ -87,6 +103,17 @@ def is_performance_metric(name: str) -> bool: return _METRIC_POLICIES.get(name) in ("min", "max") +def is_auto_gateable(name: str) -> bool: + """True for performance metrics `refresh_baselines.py --perf-gate` may bless. + + Narrower than ``is_performance_metric``: see ``_NO_AUTO_GATE`` for why a + metric can be gateable by hand and still be wrong to arm from a margin. + ``compare_to_baseline`` is unaffected -- a baseline that names such a metric + explicitly is still enforced. + """ + return is_performance_metric(name) and name not in _NO_AUTO_GATE + + def cell_passed(cell: dict[str, Any]) -> bool: """A cell 'passed' iff it ran cleanly: no whole-cell error, no failing or erroring trials, and at least one trial actually passed. diff --git a/scripts/ci/nightly_eval.py b/scripts/ci/nightly_eval.py index b24ed3343..d47d7aeac 100755 --- a/scripts/ci/nightly_eval.py +++ b/scripts/ci/nightly_eval.py @@ -18,6 +18,7 @@ import argparse import datetime as _dt +import functools import json import shutil import subprocess @@ -61,6 +62,36 @@ def gpu_count() -> int: return torch.cuda.device_count() if torch.cuda.is_available() else 0 +@functools.lru_cache(maxsize=1) +def docker_daemon() -> tuple[bool, str]: + """Can this runner reach a Docker daemon? Probed once, reported either way. + + Same contract as `gpu_count`: this answers "is the capability present on + this runner", not "did the workload work". A workload that runs its engine + in a sibling container needs a client in the CI image AND a daemon socket + mounted into it, and the socket is a per-lane opt-in that grants effective + root on the runner -- so "absent" is the normal, deliberate state, not a + fault, and an entry that needs it must skip rather than fail. + + Deliberately narrow: only entries that set `needs_docker_daemon` consult + this, so a broken daemon cannot turn the rest of the nightly into skips. + """ + docker = shutil.which("docker") + if docker is None: + return False, "no docker client on PATH" + try: + proc = subprocess.run( + [docker, "info", "--format", "{{.ServerVersion}}"], + capture_output=True, text=True, timeout=60, check=False, + ) + except (OSError, subprocess.TimeoutExpired) as exc: + return False, f"docker info did not complete ({type(exc).__name__})" + if proc.returncode != 0: + last = [ln for ln in (proc.stderr or "").splitlines() if ln.strip()] + return False, f"docker info failed: {last[-1].strip()[:160] if last else 'no stderr'}" + return True, f"daemon {proc.stdout.strip() or 'present'}" + + def build_metadata() -> dict[str, Any]: import os @@ -199,6 +230,16 @@ def evaluate(matrix_doc: dict[str, Any], baselines: dict[str, Any], out_dir: Pat ) continue + if entry.get("needs_docker_daemon"): + reachable, detail = docker_daemon() + if not reachable: + results.append( + {"entry": name, "recipe": entry["recipe"], "cell": None, + "verdict": "skip", "reasons": [f"needs a docker daemon: {detail}"], + "metrics": {}, "deltas": {}, "duration_sec": 0.0, "error": None} + ) + continue + start = _dt.datetime.now() rc, matrix_path, timed_out = run_entry(entry, out_dir) dur = (_dt.datetime.now() - start).total_seconds() diff --git a/scripts/ci/refresh_baselines.py b/scripts/ci/refresh_baselines.py index 9cb1036ed..c38598742 100755 --- a/scripts/ci/refresh_baselines.py +++ b/scripts/ci/refresh_baselines.py @@ -31,6 +31,7 @@ def build_baselines( throughput_margin: float, perf_gate: bool, existing_baselines: dict[str, Any] | None = None, + perf_gate_entries: set[str] | None = None, ) -> dict[str, Any]: existing_baselines = existing_baselines or {} ngpu = nightly_eval.gpu_count() @@ -103,12 +104,20 @@ def build_baselines( # Performance thresholds (min/max) are opt-in via --perf-gate. Only # allowlisted metrics are gated; unknown metrics (step_time_p99, # final_loss, ...) are NEVER auto-gated as min. - if perf_gate: + # + # --perf-gate is also scopable to named entries, because this file is + # rewritten WHOLE on every refresh: without a scope, arming one + # workload's perf gates arms every other entry's at the same time, + # off whatever single run happened to be under way. Those entries + # have no variance evidence behind them, and a bound derived from one + # observation is the flaky-gate failure this whole exercise exists to + # avoid -- so rolling gating out per workload has to be expressible. + if perf_gate and (not perf_gate_entries or name in perf_gate_entries): st = metrics.get("mean_step_time_ms") if st is not None: spec["step_time_ms"] = {"max": round(st * (1.0 + step_time_margin), 4)} for mname, value in summary.items(): - if value is None or not eval_lib.is_performance_metric(mname): + if value is None or not eval_lib.is_auto_gateable(mname): continue policy = eval_lib.metric_policy(mname) # "min" or "max" if policy == "min": @@ -154,9 +163,29 @@ def main() -> int: ap.add_argument("--perf-gate", action="store_true", help="also emit step-time/throughput bounds (Phase 5 perf gating); " "default is correctness-only baselines") + ap.add_argument("--perf-gate-entry", action="append", default=[], metavar="NAME", + help="restrict --perf-gate to this matrix entry (repeatable). " + "Every other entry is refreshed correctness-only, so perf " + "gating can be rolled out one workload at a time. Default " + "(no flag) gates every entry, as before.") args = ap.parse_args() matrix_doc = nightly_eval._load_yaml(nightly_eval.MATRIX) + + perf_gate_entries = set(args.perf_gate_entry) + if perf_gate_entries: + if not args.perf_gate: + raise SystemExit("--perf-gate-entry has no effect without --perf-gate") + # A misspelled entry would otherwise scope perf gating to nothing and + # produce a correctness-only refresh that reads as a successful bless. + known = {e["name"] for e in matrix_doc.get("entries") or []} + unknown = sorted(perf_gate_entries - known) + if unknown: + raise SystemExit( + f"--perf-gate-entry names no such matrix entry: {', '.join(unknown)}\n" + f"known entries: {', '.join(sorted(known))}" + ) + args.work_dir.mkdir(parents=True, exist_ok=True) existing_baselines: dict[str, Any] = {} @@ -166,7 +195,7 @@ def main() -> int: doc = build_baselines( matrix_doc, args.work_dir, args.step_time_margin, args.throughput_margin, - args.perf_gate, existing_baselines, + args.perf_gate, existing_baselines, perf_gate_entries or None, ) header = ( diff --git a/tests/ci/test_dashboard_and_alert.py b/tests/ci/test_dashboard_and_alert.py index c4be8587b..1087cc152 100644 --- a/tests/ci/test_dashboard_and_alert.py +++ b/tests/ci/test_dashboard_and_alert.py @@ -5,6 +5,7 @@ import importlib.util import json import os +import re import subprocess from pathlib import Path @@ -1851,3 +1852,418 @@ def test_hidden_path_uploads_declare_include_hidden_files(): if globs_hidden_dir and not with_.get("include-hidden-files"): offenders.append(f"{path.name}: {step.get('name')}") assert offenders == [] + + +# --------------------------------------------------------------------------- +# The CI image's docker client, and the opt-in route to the daemon. +# +# `tokenspeed_serve` runs the TokenSpeed engine in a container of its own, while +# nightly_eval.py runs INSIDE aorta-ci-gpu. Its setup() begins with +# `shutil.which("docker")`, so without a client in the image every serving cell +# errors and fail-closed reddens the whole nightly. These are file-shape +# assertions, not a build: they cannot prove the image works, only that nobody +# removed the pieces that make it possible. See docs/tokenspeed-gating-rollout.md. +# --------------------------------------------------------------------------- + + +def _ci_dockerfile() -> str: + return (_REPO_ROOT / "docker" / "Dockerfile.ci-gpu").read_text("utf-8") + + +def _split_mount(mount: str) -> list[str]: + """Split a compose short-form mount into its colon-separated fields. + + Brace-aware, because `${VAR:-/default}` carries colons of its own and a + plain `split(":")` tears the default value off the variable. + """ + fields, current, depth = [], [], 0 + for char in mount: + if char == "{": + depth += 1 + elif char == "}": + depth -= 1 + if char == ":" and depth == 0: + fields.append("".join(current)) + current = [] + continue + current.append(char) + fields.append("".join(current)) + return fields + + +def _docker_cli_installer(): + """Import the installer. Nothing runs at import: the download is behind + ``main()``, which only ``__main__`` calls.""" + path = _REPO_ROOT / "docker" / "install_docker_cli.py" + spec = importlib.util.spec_from_file_location("install_docker_cli", path) + assert spec and spec.loader + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +def test_ci_image_installs_a_docker_client(): + """The nightly cannot launch a serving container without one.""" + installer = _REPO_ROOT / "docker" / "install_docker_cli.py" + assert installer.is_file() + + dockerfile = _ci_dockerfile() + assert "install_docker_cli.py" in dockerfile, ( + "docker/Dockerfile.ci-gpu no longer installs a docker client; " + "tokenspeed_serve's setup() fails on \"'docker' not on PATH\"" + ) + # Copied AND run: a COPY on its own leaves the script in the image without + # ever putting a binary on PATH, which reads as installed and is not. + assert "COPY install_docker_cli.py" in dockerfile + assert "RUN python /usr/local/share/aorta/install_docker_cli.py" in dockerfile + + +def test_the_docker_client_is_pinned_by_version_and_digest(): + """Same rule as the base image and the pip pins: no moving targets.""" + installer = _docker_cli_installer() + + assert re.fullmatch(r"\d+\.\d+\.\d+", installer.VERSION), installer.VERSION + assert re.fullmatch(r"[0-9a-f]{64}", installer.SHA256), installer.SHA256 + assert installer.VERSION in installer.URL + assert installer.URL.startswith("https://") + + +def test_the_ci_image_carries_no_docker_daemon(): + """Client only. The static tarball ships dockerd, containerd and runc + alongside the client, so shipping a daemon is a one-word edit away -- and a + daemon in the image would be a second, nested container runtime rather than + the sibling pattern the workload and the socket mount are built around.""" + assert _docker_cli_installer().MEMBER == "docker/docker" + + # And the build says so itself, so this cannot rot into a comment. + assert "dockerd" in _ci_dockerfile() + + +def test_the_daemon_socket_is_opt_in_rather_than_always_mounted(): + """Mounting the socket grants effective root on the host, so it must not be + something a run gets by default. The base compose documents override files + as the mechanism for optional mounts ("Do not add a volume here").""" + import yaml + + base = yaml.safe_load( + (_REPO_ROOT / "docker" / "docker-compose.build.yaml").read_text("utf-8") + ) + volumes = base["services"]["torchenv"].get("volumes") or [] + assert not any("docker.sock" in str(v) for v in volumes), ( + "the daemon socket is mounted by the DEFAULT compose file; it belongs " + "in docker-compose.docker-socket.yaml so enabling it is a deliberate act" + ) + + override_path = _REPO_ROOT / "docker" / "docker-compose.docker-socket.yaml" + assert override_path.is_file() + override = yaml.safe_load(override_path.read_text("utf-8")) + mounts = override["services"]["torchenv"]["volumes"] + assert any("docker.sock" in m for m in mounts) + + # The scratch mount must be the SAME path on both sides. The -v sources the + # workload builds from work_dir are resolved by the HOST daemon, so a path + # that exists only inside the container makes the daemon create an empty one + # and mount that -- an engine container with no scripts and no exports. + same_path = [m for m in mounts if "docker.sock" not in m] + assert same_path, "no scratch mount in the socket override" + for mount in same_path: + source, target = _split_mount(mount)[:2] + assert source == target, ( + f"{mount}: work_dir must resolve to the same path for the host " + "daemon as for the process inside the container" + ) + + +def _socket_enabling_lanes(): + """Every route by which a job can end up with the host docker socket. + + There are two, and a tripwire that knows only the first is blind to the one + the nightly actually uses: + + 1. a job that calls `rocm-ci-setup` directly and passes `docker-socket: true`; + 2. a job that calls a reusable workflow which forwards its own input to that + action -- `eval-reusable.yml`'s `docker_socket`. + + Returns `(enabled, forwarding)`, where `forwarding` maps a reusable + workflow's `uses:` string to the input name it forwards. The forwarding map + is discovered rather than hardcoded, so a second reusable wrapper is covered + the day it is added. + """ + workflows = sorted((_REPO_ROOT / ".github" / "workflows").glob("*.yml")) + + forwarding: dict[str, str] = {} + for path in workflows: + for job in (_load_workflow(path.name).get("jobs") or {}).values(): + for step in job.get("steps") or []: + if "rocm-ci-setup" not in (step.get("uses") or ""): + continue + value = str((step.get("with") or {}).get("docker-socket", "")) + match = re.search(r"\binputs\.([A-Za-z_][A-Za-z0-9_-]*)", value) + if match: + forwarding[f"./.github/workflows/{path.name}"] = match.group(1) + + enabled = [] + for path in workflows: + for name, job in (_load_workflow(path.name).get("jobs") or {}).items(): + for step in job.get("steps") or []: + if "rocm-ci-setup" not in (step.get("uses") or ""): + continue + if str((step.get("with") or {}).get("docker-socket", "")).lower() == "true": + enabled.append(f"{path.name}:{name}") + forwarded = forwarding.get(job.get("uses") or "") + if forwarded and str((job.get("with") or {}).get(forwarded, "")).lower() == "true": + enabled.append(f"{path.name}:{name}") + return sorted(enabled), forwarding + + +def test_only_the_nightly_lane_gets_the_daemon_socket(): + """The setup action is shared by the GPU gate, the sanitizer nightly, the + lock-requirements job, refresh-baselines and the eval lane. Exactly one of + them has a workload that needs the daemon, so the socket must be opt-in per + lane rather than a property of being set up -- otherwise one workload's + requirement hands host root to four unrelated jobs.""" + import yaml + + action = yaml.safe_load( + (_REPO_ROOT / ".github/actions/rocm-ci-setup/action.yml").read_text("utf-8") + ) + assert action["inputs"]["docker-socket"]["default"] == "false" + + enabled, forwarding = _socket_enabling_lanes() + assert forwarding, ( + "no reusable workflow forwards a socket input to rocm-ci-setup; either " + "the plumbing was removed or this guard has stopped finding it, and in " + "both cases the check below is asserting nothing" + ) + + # A review tripwire, not a ban: the nightly lane runs tokenspeed_serve_smoke, + # which launches a container of its own. Turning it on for any OTHER lane is + # a security decision for whoever owns this CI, so it should never be a quiet + # diff. Update this list, and say who signed off, in the same PR. + assert enabled == ["nightly-eval.yml:nightly-eval"], ( + f"lanes mounting the host docker socket: {enabled}. This grants " + "effective root on the runner; see docker/docker-compose.docker-socket.yaml" + ) + + +def test_no_pr_triggered_lane_can_reach_the_host_docker_daemon(): + """The security argument for the socket rests entirely on this. + + A PR-triggered lane runs code from the pull request head on the self-hosted + runner. Give that lane the daemon socket and anyone who can open a PR + touching the trigger paths can start a privileged container bind-mounting / + -- i.e. take the runner. `bump-validate.yml` is the lane in question and it + pins `docker_socket: false` explicitly rather than relying on the default, + so a change to the default cannot arm it silently. + + Derived from each workflow's `on:` block rather than a filename list, so a + PR-triggered lane added later is covered without editing this test. + """ + enabled, _ = _socket_enabling_lanes() + + pr_triggered = set() + for path in sorted((_REPO_ROOT / ".github" / "workflows").glob("*.yml")): + # PyYAML parses the `on:` key as the boolean True. + triggers = _load_workflow(path.name).get(True) or {} + if isinstance(triggers, str): + triggers = {triggers: None} + if isinstance(triggers, list): + triggers = dict.fromkeys(triggers) + if {"pull_request", "pull_request_target"} & set(triggers): + pr_triggered.add(path.name) + + assert "bump-validate.yml" in pr_triggered, ( + "bump-validate.yml is no longer PR-triggered; this guard was written " + "around it and needs rechecking" + ) + offenders = sorted(e for e in enabled if e.split(":")[0] in pr_triggered) + assert offenders == [], ( + f"PR-triggered lanes with the host docker socket: {offenders}. These run " + "pull-request code on the self-hosted runner, so this is remote root for " + "anyone who can open a PR." + ) + + +def test_the_socket_toggle_does_not_shadow_a_compose_substitution(): + """The override's mount sources are `${NAME:-default}` substitutions, so any + env var the setup action exports under one of those names is read as a PATH. + A boolean toggle sharing a name with the socket's own variable would make + compose bind-mount a directory called `true` -- and only on the first run + that turned the socket on, which is the worst time to find out.""" + import yaml + + override = yaml.safe_load( + (_REPO_ROOT / "docker" / "docker-compose.docker-socket.yaml").read_text("utf-8") + ) + substituted = { + match + for mount in override["services"]["torchenv"]["volumes"] + for match in re.findall(r"\$\{([A-Za-z_][A-Za-z0-9_]*)", mount) + } + assert substituted, "no substitutions in the override; this guard is asserting nothing" + + action = yaml.safe_load( + (_REPO_ROOT / ".github/actions/rocm-ci-setup/action.yml").read_text("utf-8") + ) + for step in action["runs"]["steps"]: + clash = substituted & set(step.get("env") or {}) + assert not clash, ( + f"step {step.get('name')!r} exports {sorted(clash)}, which " + "docker-compose.docker-socket.yaml substitutes as a mount path" + ) + + +def test_the_socket_override_states_the_privilege_it_grants(): + """It is a root-equivalent grant. Whoever enables it should not have to + infer that, and a future edit should not be able to quietly drop it.""" + text = ( + _REPO_ROOT / "docker" / "docker-compose.docker-socket.yaml" + ).read_text("utf-8") + assert "root on the host" in text + # And an alternative for CI that cannot accept it. + assert "outside the CI container" in text + + +def test_the_reusable_eval_workflow_leaves_the_socket_to_its_caller(): + """`eval-reusable.yml` is shared by the nightly and by the PR-triggered bump + validation, so the socket cannot be decided inside it: a `true` there would + apply to every caller at once, which is exactly the scoping the security + argument depends on. It must be a `workflow_call` input, defaulting off, and + forwarded rather than overridden.""" + doc = _load_workflow("eval-reusable.yml") + spec = doc[True]["workflow_call"]["inputs"]["docker_socket"] + assert spec["type"] == "boolean" + assert spec["default"] is False, "the socket must be off unless a caller asks" + assert spec.get("required") is not True + + forwarded = [ + (step.get("with") or {}).get("docker-socket") + for job in (doc.get("jobs") or {}).values() + for step in job.get("steps") or [] + if "rocm-ci-setup" in (step.get("uses") or "") + ] + assert forwarded, "eval-reusable.yml no longer sets up the ROCm container" + for value in forwarded: + assert value is not None, ( + "the rocm-ci-setup step does not pass docker-socket at all, so the " + "workflow_call input is declared but dead" + ) + # Forwarded from the caller, never a literal decided here. + assert "inputs.docker_socket" in str(value), ( + f"docker-socket is {value!r}; it must forward inputs.docker_socket " + "so the choice stays with the calling workflow" + ) + + +def test_the_socket_input_records_why_it_must_stay_off_for_pr_lanes(): + """The default is the whole control, and a default is easy to flip without + understanding it. The reason has to live next to the declaration, not only + in a rollout doc nobody reads while editing YAML.""" + text = (_REPO_ROOT / ".github/workflows/eval-reusable.yml").read_text("utf-8") + assert "docker_socket" in text + assert "PR-triggered" in text, ( + "the docker_socket input does not say it must stay off for PR-triggered " + "lanes; that constraint is the reason it is an input at all" + ) + + +# --------------------------------------------------------------------------- +# refresh-baselines.yml: the perf-gate scope (docs/tokenspeed-gating-rollout.md) +# --------------------------------------------------------------------------- + + +def _refresh_baselines_script_step(): + for job in (_load_workflow("refresh-baselines.yml").get("jobs") or {}).values(): + for step in job.get("steps") or []: + if "refresh_baselines.py" in (step.get("run") or ""): + return step + raise AssertionError("no step in refresh-baselines.yml runs refresh_baselines.py") + + +def test_refresh_baselines_exposes_the_perf_gate_scope_as_an_input(): + """`refresh_baselines.py` has had `--perf-gate-entry` since the gating branch, + but the workflow is dispatch-only -- so without a matching input the scoped + bless described in docs/tokenspeed-gating-rollout.md is not runnable from the + Actions UI at all, and the only reachable perf refresh is the unscoped one + that arms ceilings on every entry in the matrix.""" + inputs = _load_workflow("refresh-baselines.yml")[True]["workflow_dispatch"]["inputs"] + assert "perf_gate_entry" in inputs + spec = inputs["perf_gate_entry"] + # Empty default: absent means "as before", i.e. gate everything. Anything + # else would change the behaviour of a dispatch that omits the field. + assert spec["default"] == "" + assert spec.get("required") is False + + +def test_the_perf_gate_entry_flag_is_passed_only_when_a_name_was_given(): + """An empty `--perf-gate-entry ""` is not a no-op: it is a name that matches + no matrix entry, which refresh_baselines.py rejects. So the flag has to be + built conditionally rather than always interpolated.""" + run = _refresh_baselines_script_step()["run"] + assert "--perf-gate-entry" in run + assert re.search(r'if\s+\[\s+-n\s+"\$\{PERF_GATE_ENTRY\}"\s+\]', run), ( + "the --perf-gate-entry flag is not guarded by a non-empty test on " + "PERF_GATE_ENTRY, so an omitted input would pass an empty name" + ) + # One flag per name, so a list maps to the repeatable flag. + assert "IFS" in run and "read -r -a" in run, ( + "perf_gate_entry does not split into repeated flags; a comma- or " + "space-separated list would be passed as one bogus entry name" + ) + + +def test_the_scoped_refresh_reaches_the_container_as_environment(): + """`perf_gate_entry` is free text from a dispatch form, and `${{ }}` + substitution happens before bash sees the script -- so interpolating it into + the `run` body would let a dispatch value close the quote and run arbitrary + commands on the self-hosted GPU runner. Inputs must arrive as env vars. + + Scoped to the four workflows this gating change owns. `gemm-sweep-analysis` + and `rccl-warp-speed-analysis` interpolate dispatch inputs into shell bodies + today; that is pre-existing and out of scope here, not an endorsement. + """ + owned = ( + "refresh-baselines.yml", + "eval-reusable.yml", + "nightly-eval.yml", + "bump-validate.yml", + ) + for name in owned: + for job_name, job in (_load_workflow(name).get("jobs") or {}).items(): + for step in job.get("steps") or []: + run = step.get("run") or "" + leaked = re.findall(r"\$\{\{[^}]*\binputs\.[A-Za-z0-9_]+[^}]*\}\}", run) + assert not leaked, ( + f"{name}:{job_name} step {step.get('name')!r} interpolates " + f"{leaked} into a shell body; pass it through `env:` instead" + ) + + step = _refresh_baselines_script_step() + env = step.get("env") or {} + assert "inputs.perf_gate_entry" in str(env.get("PERF_GATE_ENTRY", "")), ( + "PERF_GATE_ENTRY is not wired from the dispatch input" + ) + # And forwarded across the container boundary, or the inner bash sees nothing. + assert "-e PERF_GATE_ENTRY=" in step["run"] + + +def test_an_unscoped_perf_refresh_is_flagged_but_not_blocked(): + """Refusing `perf_gate: true` with an empty scope would break a documented + invocation: an unscoped refresh is the legitimate end state once every + workload has variance data, and it is what every existing dispatch of this + workflow has done. So it warns. The reverse -- a scope with no perf_gate -- + has never been meaningful and fails fast, before the wheel install, rather + than after minutes of GPU runner time.""" + runs = [ + step.get("run") or "" + for job in (_load_workflow("refresh-baselines.yml").get("jobs") or {}).values() + for step in job.get("steps") or [] + ] + warns = [r for r in runs if "::warning::" in r and "PERF_GATE_ENTRY" in r] + assert warns, "an unscoped perf refresh produces no warning" + errors = [r for r in runs if "::error::" in r and "PERF_GATE_ENTRY" in r] + assert errors, "a scope without perf_gate is not rejected" + # The warning path must not exit non-zero. + assert not re.search( + r'::warning::[^\n]*\n\s*fi\s*\n\s*exit 1', "\n".join(warns) + ), "the unscoped-refresh warning appears to fail the job" diff --git a/tests/ci/test_eval_lib.py b/tests/ci/test_eval_lib.py index 1fd19e9f9..bc4c138fd 100644 --- a/tests/ci/test_eval_lib.py +++ b/tests/ci/test_eval_lib.py @@ -106,6 +106,265 @@ def test_serving_metrics_are_gateable(): assert eval_lib.metric_policy(name) == "min", name +def test_median_itl_is_gateable_by_hand_but_never_auto_gated(): + """Gateable and auto-gateable are different questions for a metric at ~0. + + `median_itl_ms` stays in the allowlist so a baseline that names it is still + enforced; what it must not do is get a bound derived from a margin. See + `_NO_AUTO_GATE`. + """ + assert eval_lib.metric_policy("median_itl_ms") == "max" + assert eval_lib.is_performance_metric("median_itl_ms") is True + assert eval_lib.is_auto_gateable("median_itl_ms") is False + + +def test_the_no_auto_gate_set_is_narrow(): + """Every other allowlisted perf metric is still auto-gateable, and the set + never claims a metric that is not gateable in the first place.""" + for name in ("gflops", "tokens_per_sec", "median_ttft_ms", "median_tpot_ms", + "p99_itl_ms", "output_throughput", "total_token_throughput"): + assert eval_lib.is_auto_gateable(name) is True, name + for name in ("logits_checksum", "server_startup_sec", "unknown_metric"): + assert eval_lib.is_auto_gateable(name) is False, name + assert all(eval_lib.is_performance_metric(n) for n in eval_lib._NO_AUTO_GATE) + + +# --------------------------------------------------------------------------- +# Offline gating simulation for the TokenSpeed serving rollout. +# +# The numbers below are the ones docs/tokenspeed-serving.md and docs/tokenspeed.md +# actually recorded on gfx950, not invented ones. They exist as tests because the +# rollout plan (docs/tokenspeed-gating-rollout.md) makes claims about which +# metrics tolerate the measured noise and which do not, and a claim about a +# comparator is checkable against the comparator. +# +# `_MAX_MARGIN` / `_MIN_MARGIN` are refresh_baselines.py's defaults, restated +# here so a change to them fails these tests rather than silently invalidating +# the plan. +# --------------------------------------------------------------------------- + +_MAX_MARGIN = 0.25 # --step-time-margin: latency ceiling = value * 1.25 +_MIN_MARGIN = 0.15 # --throughput-margin: throughput floor = value * 0.85 + + +def _bound(value: float, policy: str) -> float: + return round(value * (1 + _MAX_MARGIN), 4) if policy == "max" \ + else round(value * (1 - _MIN_MARGIN), 4) + + +def _gate(observed: float, name: str, threshold: float) -> str: + """Run one metric through the real comparator and return its verdict.""" + harvested = {"passed": True, "error": None, "metrics": {"summary": {name: observed}}} + baseline = {"passed": True, + "metrics": {name: {"policy": eval_lib.metric_policy(name), "value": threshold}}} + return eval_lib.compare_to_baseline(harvested, baseline)["verdict"] + + +# serve-models::qwen3-0.6b vs serve-load::conc-8 -- two separate sweeps that ran +# a byte-identical measurement configuration (Qwen3-0.6B, ISL 512 / OSL 128, +# concurrency 8, 32 prompts, 3 measured steps, 1 warmup step, ignore_eos, seed 0). +_QWEN_RUN_A = {"median_ttft_ms": 46.3, "median_tpot_ms": 1.94, "output_throughput": 3538.0} +_QWEN_RUN_B = {"median_ttft_ms": 45.9, "median_tpot_ms": 1.91, "output_throughput": 3631.0} + +# serve-gptoss::baseline vs serve-gptoss-tp::tp1, which the doc calls the control +# for the TP axis ("reproduces the single-GPU numbers to within a percent"). +_GPTOSS_RUN_A = {"median_ttft_ms": 67.1, "median_tpot_ms": 7.61, "output_throughput": 994.0} +_GPTOSS_RUN_B = {"median_ttft_ms": 67.2, "median_tpot_ms": 7.63, "output_throughput": 991.0} + +# Startup to /health, same recipe, same node, nothing changed between them +# (docs/tokenspeed.md: "189, 276, 285, 291, 316 and 319 seconds across six runs +# ... a 1.7x spread"), plus the 379 s the multi-model sweep recorded for the +# same 0.6B model. +_STARTUP_SEC = [189, 276, 285, 291, 316, 319, 379] + + +@pytest.mark.parametrize("run_a,run_b", [(_QWEN_RUN_A, _QWEN_RUN_B), + (_GPTOSS_RUN_A, _GPTOSS_RUN_B)]) +def test_measured_run_to_run_noise_passes_the_proposed_gates(run_a, run_b): + """The metrics the plan gates tolerate the noise we have actually measured. + + Both directions: whichever of the two runs is blessed, the other must pass. + A gate that depends on which night it was armed is a coin flip, not a gate. + """ + for blessed, observed in ((run_a, run_b), (run_b, run_a)): + for name, value in blessed.items(): + policy = eval_lib.metric_policy(name) + verdict = _gate(observed[name], name, _bound(value, policy)) + assert verdict == "pass", f"{name}: bless {value} -> {observed[name]} {verdict}" + + +def test_a_real_serving_regression_fails_the_proposed_gates(): + """A regression the size of a genuine stack change is caught. + + Scale reference from the same table: moving Qwen3-0.6B to Qwen3-4B took TPOT + 1.94 -> 3.77 ms and output throughput 3538 -> 1905 tok/s. A gate that cannot + see that is not worth arming. + """ + tpot_ceiling = _bound(max(_QWEN_RUN_A["median_tpot_ms"], _QWEN_RUN_B["median_tpot_ms"]), "max") + thru_floor = _bound(min(_QWEN_RUN_A["output_throughput"], + _QWEN_RUN_B["output_throughput"]), "min") + + assert _gate(1.94 * 1.30, "median_tpot_ms", tpot_ceiling) == "fail" + assert _gate(3538 * 0.75, "output_throughput", thru_floor) == "fail" + assert _gate(3.77, "median_tpot_ms", tpot_ceiling) == "fail" + assert _gate(1905.0, "output_throughput", thru_floor) == "fail" + + # And the band between measured noise (<=2.7%) and the gate is deliberately + # left to the dashboard's 10% move detector, not to the gate: a 10% drift + # passes here and is reported as "what changed" instead of failing the job. + assert _gate(1.94 * 1.10, "median_tpot_ms", tpot_ceiling) == "pass" + assert _gate(3538 * 0.90, "output_throughput", thru_floor) == "pass" + + +def test_a_startup_time_gate_would_fire_on_measured_noise(): + """Why `server_startup_sec` is absent from the allowlist and must stay absent. + + Arming it from a single observation -- which is what --perf-gate does -- makes + the verdict depend on which night was blessed: from the fastest of the seven + known runs, every other run breaches. + """ + assert eval_lib.metric_policy("server_startup_sec") is None + + def breaches(threshold): + return [v for v in _STARTUP_SEC + if v > threshold] # policy would be `max` + + assert breaches(_bound(min(_STARTUP_SEC), "max")) == [276, 285, 291, 316, 319, 379] + # Four of the seven possible blessing nights produce a gate that breaches at all. + breaking = [v for v in _STARTUP_SEC + if [o for o in _STARTUP_SEC if o != v and o > _bound(v, "max")]] + assert len(breaking) == 4 + + +def test_a_near_zero_itl_bound_cannot_be_satisfied(): + """The concrete reason `median_itl_ms` is in `_NO_AUTO_GATE`. + + The docs record it as sitting near zero ("most recorded gaps are ~0"). A + margin is multiplicative, so an observation of 0.0 blesses a ceiling of 0.0 + and every later run with any inter-token gap at all fails. + """ + assert _bound(0.0, "max") == 0.0 + assert _gate(0.001, "median_itl_ms", _bound(0.0, "max")) == "fail" + # Even a plainly non-zero-but-small observation gates on 0.01 ms of movement. + assert _gate(0.03, "median_itl_ms", _bound(0.02, "max")) == "fail" + + +# --------------------------------------------------------------------------- +# The step-0 compile excursion, measured on the cell the staged entry runs. +# +# Read from the step_times_ms / metrics_summary of every matrix.json on disk for +# TOKENSPEED-SERVE-SMOKE: 6 sweeps, 13 cell-runs, 39 steps. Twelve are clean; one +# recorded its measured steps as 6193.4, 1140.2, 1142.6 ms -- the excursion is +# the FIRST measured step, after warmup_steps: 1 had already discarded one. +# docs/tokenspeed-gating-rollout.md derives the revised first-bless set from +# these; the point of putting them here is that the derivation is checkable. +# --------------------------------------------------------------------------- + +# Full clean envelope (min, max) over the 12 clean cell-runs. +_SMOKE_CLEAN_RANGE = { + "median_ttft_ms": (43.65, 46.87), + "median_tpot_ms": (1.89, 1.95), + "output_throughput": (3502.53, 3646.20), + "p99_itl_ms": (34.00, 35.84), +} +# Worst clean observation per metric -- the "ten-night extremum" anchor the plan +# proposes, so a gate built from it is the most permissive the plan would ever +# bless. For a `max` metric that is the top of the range, for a `min` metric the +# bottom. +_SMOKE_CLEAN_ANCHOR = { + name: (hi if eval_lib.metric_policy(name) == "max" else lo) + for name, (lo, hi) in _SMOKE_CLEAN_RANGE.items() +} +# The one excursion cell-run, same metric names. +_SMOKE_EXCURSION = { + "median_ttft_ms": 465.30, + "median_tpot_ms": 1.93, + "output_throughput": 2612.83, + "p99_itl_ms": 34.96, +} +_SMOKE_CLEAN_MEAN_STEP_MS = 1169.5 +_SMOKE_EXCURSION_MEAN_STEP_MS = 2825.4 + + +def test_the_step_zero_excursion_breaks_the_duration_derived_gates(): + """Three of the four proposed gates fire on a run that is not a regression. + + All three are derived from step DURATION -- the step-time mean directly, and + throughput as tokens over that duration -- or from TTFT, whose first request + waits for the compile. This is why the rollout plan does not bless them from + a ten-night extremum while the excursion is still reachable. + """ + for name in ("median_ttft_ms", "output_throughput"): + policy = eval_lib.metric_policy(name) + threshold = _bound(_SMOKE_CLEAN_ANCHOR[name], policy) + assert _gate(_SMOKE_EXCURSION[name], name, threshold) == "fail", name + + # step_time_ms.max is the bound --perf-gate always writes, and it is the + # worst of the three: 2.4x the ceiling, not a marginal breach. + assert _SMOKE_EXCURSION_MEAN_STEP_MS > _bound(_SMOKE_CLEAN_MEAN_STEP_MS, "max") + + +def test_the_per_token_metrics_are_immune_to_the_excursion_by_construction(): + """`median_tpot_ms` and `p99_itl_ms` are measured BETWEEN tokens, after the + compile has happened, so a fixed ~5 s of compilation added to one step does + not enter them. That is a property of their definition, not luck, and it is + the reason the revised plan gates the per-token pair first.""" + for name in ("median_tpot_ms", "p99_itl_ms"): + policy = eval_lib.metric_policy(name) + threshold = _bound(_SMOKE_CLEAN_ANCHOR[name], policy) + assert _gate(_SMOKE_EXCURSION[name], name, threshold) == "pass", name + # Stronger than "passes the gate": the excursion run's value lands + # *inside* the clean envelope, so on these two metrics the excursion run + # is not distinguishable from a healthy one at all. + low, high = _SMOKE_CLEAN_RANGE[name] + assert low <= _SMOKE_EXCURSION[name] <= high, name + + +# Measured from INSIDE the aorta-ci-gpu container, driving sibling engine +# containers over the bind-mounted daemon socket, on 2026-09-02. Two sweeps of +# the two-cell recipe; all four cell-runs passed and all twelve steps were clean +# (1108-1149 ms, no step-0 excursion). This is the evidence the matrix entry was +# promoted on. +_IN_CONTAINER = { + "sweep1/baseline": {"median_ttft_ms": 47.1303, "median_tpot_ms": 1.8964, + "output_throughput": 3621.1781, "p99_itl_ms": 34.6303}, + "sweep1/no-scratch-reclaim": {"median_ttft_ms": 46.6870, "median_tpot_ms": 1.8548, + "output_throughput": 3593.2672, "p99_itl_ms": 34.6720}, + "sweep2/baseline": {"median_ttft_ms": 45.1460, "median_tpot_ms": 1.9007, + "output_throughput": 3601.5000, "p99_itl_ms": 34.6700}, + "sweep2/no-scratch-reclaim": {"median_ttft_ms": 46.7940, "median_tpot_ms": 1.9135, + "output_throughput": 3577.2000, "p99_itl_ms": 35.0300}, +} + + +def test_running_inside_the_ci_container_does_not_move_the_measurement(): + """The sibling-container arrangement is a measurement question, not only a + plumbing one: if driving the engine from inside aorta-ci-gpu shifted the + numbers, a baseline blessed by the nightly could not be compared against the + host-side runs this plan's variance analysis is built from, and the ten-night + window would have to start over the first time CI changed how it launches. + + It does not shift them. Every one of the sixteen in-container observations + lands within 5% of the host-side clean envelope -- and mostly inside it. The + boundary therefore contributes less than the run-to-run noise the plan has + already accounted for. + """ + for cell, metrics in _IN_CONTAINER.items(): + for name, observed in metrics.items(): + low, high = _SMOKE_CLEAN_RANGE[name] + assert low * 0.95 <= observed <= high * 1.05, f"{cell}:{name}={observed}" + + +def test_a_gate_blessed_in_the_container_still_passes_host_side_runs(): + """The other direction, which is the one that would redden a nightly: bless + from the in-container run and the host-side clean envelope must still pass.""" + for metrics in _IN_CONTAINER.values(): + for name, blessed in metrics.items(): + policy = eval_lib.metric_policy(name) + threshold = _bound(blessed, policy) + assert _gate(_SMOKE_CLEAN_ANCHOR[name], name, threshold) == "pass", name + + def test_compare_record_only_when_no_baseline_and_passed(): harvested = {"cell": "c", "passed": True, "error": None, "metrics": {}} out = eval_lib.compare_to_baseline(harvested, None) diff --git a/tests/ci/test_nightly_eval.py b/tests/ci/test_nightly_eval.py index fdce71885..f89149ada 100644 --- a/tests/ci/test_nightly_eval.py +++ b/tests/ci/test_nightly_eval.py @@ -9,6 +9,7 @@ import importlib.util import json +import re from pathlib import Path from types import SimpleNamespace @@ -248,6 +249,390 @@ def fake_run_entry(entry, out_dir): refresh_baselines.build_baselines(matrix_doc, tmp_path, 0.25, 0.15, False) +# --------------------------------------------------------------------------- +# Scoping --perf-gate to one entry, and not auto-arming a near-zero metric. +# Both exist so serving perf gating can be rolled out on its own; see +# docs/tokenspeed-gating-rollout.md. +# --------------------------------------------------------------------------- + + +_SERVING_SUMMARY = { + "median_ttft_ms": {"mean": 46.3}, + "median_tpot_ms": {"mean": 1.94}, + "median_itl_ms": {"mean": 0.0}, + "p99_itl_ms": {"mean": 21.4}, + "output_throughput": {"mean": 3538.0}, +} + + +def _serving_matrix_runner(summary=None): + summary = _SERVING_SUMMARY if summary is None else summary + + def fake_run_entry(entry, out_dir): + mpath = _write_matrix(out_dir / entry["name"] / "matrix.json", + [{"name": "baseline", "error": None, "passed_count": 1, + "failed_count": 0, "error_count": 0, + "mean_step_time_ms": 1100.0, + "metrics_summary": summary}]) + return 0, mpath, False + + return fake_run_entry + + +def test_perf_gate_can_be_scoped_to_a_single_entry(tmp_path, monkeypatch): + """Rolling gating out per workload must not arm every other entry too. + + refresh_baselines rewrites the whole baseline file, so an unscoped + --perf-gate would derive step-time ceilings for every unrelated entry from + whatever single run happened to be under way -- the one-observation + threshold this rollout exists to avoid, applied to workloads nobody looked at. + """ + matrix_doc = {"entries": [ + {"name": "tokenspeed_serve_smoke", "recipe": "ts.yaml"}, + {"name": "gpu_smoke", "recipe": "r1.yaml"}, + ]} + monkeypatch.setattr(refresh_baselines.nightly_eval, "gpu_count", lambda: 1) + monkeypatch.setattr(refresh_baselines.nightly_eval, "run_entry", _serving_matrix_runner()) + + doc = refresh_baselines.build_baselines( + matrix_doc, tmp_path, 0.25, 0.15, True, + perf_gate_entries={"tokenspeed_serve_smoke"}) + + gated = doc["baselines"]["tokenspeed_serve_smoke::baseline"] + ungated = doc["baselines"]["gpu_smoke::baseline"] + assert gated["step_time_ms"] == {"max": 1375.0} + assert gated["metrics"]["median_tpot_ms"] == {"policy": "max", "value": 2.425} + assert gated["metrics"]["output_throughput"] == {"policy": "min", "value": 3007.3} + # The unscoped entry stays record-only for performance: no step-time ceiling + # and no metric bounds at all. + assert "step_time_ms" not in ungated + assert "metrics" not in ungated + + +def test_perf_gate_without_a_scope_still_gates_every_entry(tmp_path, monkeypatch): + """The default is unchanged, so existing callers keep their behaviour.""" + matrix_doc = {"entries": [ + {"name": "tokenspeed_serve_smoke", "recipe": "ts.yaml"}, + {"name": "gpu_smoke", "recipe": "r1.yaml"}, + ]} + monkeypatch.setattr(refresh_baselines.nightly_eval, "gpu_count", lambda: 1) + monkeypatch.setattr(refresh_baselines.nightly_eval, "run_entry", _serving_matrix_runner()) + + doc = refresh_baselines.build_baselines(matrix_doc, tmp_path, 0.25, 0.15, True) + for key in ("tokenspeed_serve_smoke::baseline", "gpu_smoke::baseline"): + assert doc["baselines"][key]["step_time_ms"] == {"max": 1375.0} + + +def test_perf_gate_does_not_arm_a_near_zero_median_itl(tmp_path, monkeypatch): + """`median_itl_ms` is measured at ~0, so `value * 1.25` is a ceiling of 0.0 + that no later run with any inter-token gap can satisfy. It stays charted and + hand-gateable; the refresher must not bless a bound for it.""" + matrix_doc = {"entries": [{"name": "tokenspeed_serve_smoke", "recipe": "ts.yaml"}]} + monkeypatch.setattr(refresh_baselines.nightly_eval, "gpu_count", lambda: 1) + monkeypatch.setattr(refresh_baselines.nightly_eval, "run_entry", _serving_matrix_runner()) + + doc = refresh_baselines.build_baselines(matrix_doc, tmp_path, 0.25, 0.15, True) + metrics = doc["baselines"]["tokenspeed_serve_smoke::baseline"]["metrics"] + assert "median_itl_ms" not in metrics + # Its tail counterpart is a real number and stays gateable. + assert metrics["p99_itl_ms"] == {"policy": "max", "value": 26.75} + + +def _refresh_cli(*argv): + import subprocess + import sys + return subprocess.run( + [sys.executable, str(_REPO_ROOT / "scripts" / "ci" / "refresh_baselines.py"), *argv], + capture_output=True, text=True, timeout=120) + + +def test_perf_gate_entry_rejects_a_name_the_matrix_does_not_have(): + """A typo would scope perf gating to nothing and produce a correctness-only + refresh that reads, in the PR diff, exactly like a successful bless.""" + out = _refresh_cli("--perf-gate", "--perf-gate-entry", "tokenspeed_serve_smoek") + assert out.returncode != 0 + assert "no such matrix entry" in out.stderr + # The message has to be actionable from the terminal it appeared in. + assert "gpu_smoke" in out.stderr + + +def test_perf_gate_entry_without_perf_gate_is_rejected(): + """Silently ignoring it would leave the operator believing gates were armed.""" + out = _refresh_cli("--perf-gate-entry", "gpu_smoke") + assert out.returncode != 0 + assert "no effect without --perf-gate" in out.stderr + + +def test_an_unknown_entry_is_not_a_valid_perf_gate_scope(): + """Scoping to a name that is not in `entries` -- a staged entry, or a typo -- + must fail rather than quietly refresh everything correctness-only.""" + out = _refresh_cli("--perf-gate", "--perf-gate-entry", "not_a_real_entry") + assert out.returncode != 0 + assert "no such matrix entry" in out.stderr + + +# --------------------------------------------------------------------------- +# The committed matrix file itself, through the loader the nightly uses. +# --------------------------------------------------------------------------- + + +def _real_matrix(): + return nightly_eval._load_yaml(nightly_eval.MATRIX) + + +def test_the_committed_matrix_loads_and_every_recipe_exists(): + doc = _real_matrix() + assert doc["version"] == 1 + entries = doc["entries"] + assert entries and len({e["name"] for e in entries}) == len(entries) + for entry in entries: + assert (nightly_eval.REPO_ROOT / entry["recipe"]).is_file(), entry["recipe"] + + +def test_pending_entries_are_inert(tmp_path, monkeypatch): + """`pending_entries` documents staged work; nothing may execute it. + + Both consumers iterate `entries` only, and this pins that -- a future reader + who wires the key up would otherwise start a blocked entry by accident. + """ + doc = _real_matrix() + # Synthesised rather than read from the file: the key is legitimately empty + # between staged entries, and the property being pinned -- that a future + # reader who wires the key up starts a blocked entry -- must hold then too. + doc = dict(doc) + doc["pending_entries"] = [ + {"name": "staged_thing", "recipe": "recipes/ci/gpu-smoke.yaml", "blocked_on": "x"}, + *(doc.get("pending_entries") or []), + ] + pending = {e["name"] for e in doc["pending_entries"]} + + monkeypatch.setattr(nightly_eval, "gpu_count", lambda: 8) + monkeypatch.setattr(nightly_eval, "build_metadata", lambda: {}) + ran: list[str] = [] + + def fake(entry, out_dir): + ran.append(entry["name"]) + return 0, _write_matrix(out_dir / entry["name"] / "matrix.json", + [{"name": "c", "error": None, "passed_count": 1, + "failed_count": 0, "error_count": 0, + "metrics_summary": {}}]), False + + monkeypatch.setattr(nightly_eval, "run_entry", fake) + result = nightly_eval.evaluate(doc, {"baselines": {}}, tmp_path) + + assert not (pending & set(ran)) + assert not (pending & {e["entry"] for e in result["entries"]}) + + +def _docker_matrix(): + return {"entries": [ + {"name": "needs_daemon", "recipe": "r.yaml", "needs_docker_daemon": True}, + {"name": "plain", "recipe": "r2.yaml"}, + ]} + + +def _fake_run(ran): + def run(entry, out_dir): + ran.append(entry["name"]) + return 0, _write_matrix(out_dir / entry["name"] / "matrix.json", + [{"name": "c", "error": None, "passed_count": 1, + "failed_count": 0, "error_count": 0, + "metrics_summary": {}}]), False + return run + + +def test_an_entry_needing_a_daemon_skips_rather_than_fails_when_there_is_none( + tmp_path, monkeypatch): + """The socket is a per-lane opt-in that grants effective root, so no lane + sets it and "no daemon" is the normal state. An entry that needs one must + therefore skip -- a fail would redden every nightly until a security + decision that is not the pipeline's to make.""" + monkeypatch.setattr(nightly_eval, "gpu_count", lambda: 8) + monkeypatch.setattr(nightly_eval, "build_metadata", lambda: {}) + monkeypatch.setattr(nightly_eval, "docker_daemon", + lambda: (False, "no docker client on PATH")) + ran: list[str] = [] + monkeypatch.setattr(nightly_eval, "run_entry", _fake_run(ran)) + + result = nightly_eval.evaluate(_docker_matrix(), {"baselines": {}}, tmp_path) + by_name = {e["entry"]: e for e in result["entries"]} + + assert by_name["needs_daemon"]["verdict"] == "skip" + assert "no docker client on PATH" in by_name["needs_daemon"]["reasons"][0] + assert "needs_daemon" not in ran + # And the probe must not quarantine anything that did not ask for it. + assert by_name["plain"]["verdict"] != "skip" + assert ran == ["plain"] + + +def test_an_entry_needing_a_daemon_runs_when_one_is_reachable(tmp_path, monkeypatch): + """The other half: the skip must disappear the moment a lane mounts the + socket, with no further edit to the matrix.""" + monkeypatch.setattr(nightly_eval, "gpu_count", lambda: 8) + monkeypatch.setattr(nightly_eval, "build_metadata", lambda: {}) + monkeypatch.setattr(nightly_eval, "docker_daemon", lambda: (True, "daemon 29.4.2")) + ran: list[str] = [] + monkeypatch.setattr(nightly_eval, "run_entry", _fake_run(ran)) + + nightly_eval.evaluate(_docker_matrix(), {"baselines": {}}, tmp_path) + assert ran == ["needs_daemon", "plain"] + + +def test_the_daemon_probe_reports_a_client_that_cannot_reach_a_daemon(monkeypatch): + """A mounted-but-dead socket is the interesting case: the client is on PATH, + so a `which docker` check would call the capability present and the entry + would fail on connect. The probe must actually talk to the daemon.""" + import subprocess as sp + + nightly_eval.docker_daemon.cache_clear() + monkeypatch.setattr(nightly_eval.shutil, "which", lambda _: "/usr/local/bin/docker") + monkeypatch.setattr( + nightly_eval.subprocess, "run", + lambda *a, **k: sp.CompletedProcess( + a[0], 1, "", "Cannot connect to the Docker daemon at unix:///var/run/docker.sock."), + ) + try: + reachable, detail = nightly_eval.docker_daemon() + assert reachable is False + assert "Cannot connect to the Docker daemon" in detail + finally: + nightly_eval.docker_daemon.cache_clear() + + +def test_the_live_serving_entry_declares_the_capability_it_needs(): + """Promoted on a demonstrated launch, but only safe in `entries` because it + declares the capability. Without the flag it is a guaranteed nightly failure + on every runner that has not opted into the socket.""" + live = {e["name"]: e for e in _real_matrix()["entries"]} + entry = live["tokenspeed_serve_smoke"] + assert entry["needs_docker_daemon"] is True + assert "blocked_on" not in entry + assert int(entry["timeout_sec"]) >= 3600 + + +def test_the_gating_recipe_discards_two_steps_before_measuring(): + """A step-0 Triton compile excursion reaches the metrics at `warmup_steps: 1`. + + Measured at 1 cell-run in 13: a ~10x first-token spike (465 ms against a + 43-47 ms clean range) and a 2825 ms step against 1108-1178 ms. The excursion + lands on the first *measured* step, after the single warmup step has already + been discarded, so it breaches three of the four originally-proposed gates on + a run that is not a regression. + + `num_warmups` is a different knob and cannot substitute: it warms requests + within one bench invocation, not the compile cache across invocations. The + excursion is positional, so one more discarded step removes it by + construction -- which is the only reason the duration-derived metrics can be + promoted later at all. + + The matrix carries no `workload_config` override (nightly_eval.py reads only + recipe/min_gpus/timeout_sec/needs_docker_daemon), so the recipe is the single + place this can be set, and it is shared with the baseline refresher -- which + is what keeps the blessed numbers and the nightly on the same measurement. + """ + import yaml + + entry = {e["name"]: e for e in _real_matrix()["entries"]}["tokenspeed_serve_smoke"] + recipe_path = nightly_eval.REPO_ROOT / entry["recipe"] + config = yaml.safe_load(recipe_path.read_text("utf-8"))["workload_config"] + + assert config["warmup_steps"] == 2, ( + "the gating recipe must discard two bench steps; at 1 the measured " + "step-0 compile excursion enters the metrics the nightly gates on" + ) + + # The workload derives its internal budget from + # ready_timeout + (steps + warmup_steps) * bench_timeout, so an extra warmup + # step lengthens it. The binding cap is the entry's own timeout_sec, and a + # bench step here is ~1.1s against a 3600s budget dominated by bring-up -- + # but assert the relationship rather than trusting the arithmetic to hold. + assert int(entry["timeout_sec"]) >= 3600 + + +def test_the_rollout_doc_marks_its_variance_data_as_the_old_configuration(): + """The 13-cell-run variance table was measured at `warmup_steps: 1`, which the + recipe no longer uses, so it is the rationale for the change and not the + baseline to bless against. It is deliberately retained -- there is no + measurement at the new setting yet, and the ten-night record-only window is + what produces one. This guards the caveat, not the table.""" + doc = (nightly_eval.REPO_ROOT / "docs/tokenspeed-gating-rollout.md").read_text("utf-8") + assert "warmup_steps: 1" in doc, "the provenance of the variance data is gone" + assert "warmup_steps: 2" in doc, "the doc does not state the new setting" + # The table itself must survive. + assert "Clean range over 12 cell-runs" in doc + # And the window has to be pinned to the setting the gate will run at. + assert re.search( + r"ten nightlies, at `warmup_steps: 2`", doc + ), "the record-only window is not pinned to warmup_steps: 2" + + +def test_pending_entries_are_valid_and_loadable(): + """Validated exactly like a live entry, so promoting one is a move, not a bet. + + A staged entry that names a deleted recipe or a misspelled field would + otherwise only be discovered by the first red nightly after promotion. + """ + from aorta.triage.recipe import load_recipe + + known_fields = {"name", "recipe", "nproc", "min_gpus", "timeout_sec", + "needs_docker_daemon", "blocked_on"} + pending = _real_matrix().get("pending_entries") or [] + live = {e["name"] for e in _real_matrix()["entries"]} + + for entry in pending: + assert set(entry) <= known_fields, f"{entry['name']}: {set(entry) - known_fields}" + assert entry["name"] not in live, f"{entry['name']} is both staged and live" + # A staged entry exists because it cannot run yet; say why, in the file. + assert entry.get("blocked_on"), f"{entry['name']} has no blocked_on" + path = nightly_eval.REPO_ROOT / entry["recipe"] + assert path.is_file(), entry["recipe"] + recipe = load_recipe(path) + assert recipe.cells, f"{entry['name']}: recipe has no cells" + # Every entry's budget must cover the recipe it names; the default is + # 1800s and a serving bring-up alone has been measured at 379s a cell. + assert int(entry.get("timeout_sec", 1800)) >= 1800 + + +def test_serving_entry_metric_names_resolve_against_the_policy_table(): + """The names `tokenspeed bench serve` exports, checked against the allowlist. + + The workload passes its export through verbatim, so a metric the plan intends + to gate must resolve to the right direction -- and the ones it intends never + to gate must resolve to nothing, since an allowlist entry is what --perf-gate + arms from. + """ + eval_lib = _load("eval_lib") + live = {e["name"]: e for e in _real_matrix()["entries"]} + assert "tokenspeed_serve_smoke" in live + + # Gated from the first bless (docs/tokenspeed-gating-rollout.md). Both are + # per-token metrics: they are measured between tokens, so the step-0 compile + # excursion measured on this cell does not enter them. + for name in ("median_tpot_ms", "p99_itl_ms"): + assert eval_lib.metric_policy(name) == "max", name + assert eval_lib.is_auto_gateable(name) is True, name + + # Gateable and correctly directed, but held record-only for the first bless + # because all three are duration-derived or wait on the compile, and the + # excursion carries each of them through its threshold. + assert eval_lib.metric_policy("median_ttft_ms") == "max" + assert eval_lib.metric_policy("output_throughput") == "min" + for name in ("median_ttft_ms", "output_throughput"): + assert eval_lib.is_auto_gateable(name) is True, name + + # Recorded but never gated: bring-up time is the noisiest thing the workload + # reports (189-379s on one node, nothing changed), and the counters/totals + # restate the recipe rather than measuring the stack. + for name in ("server_startup_sec", "container_elapsed_sec", "duration", + "completed_total", "failed_total", "total_output_tokens", + "max_output_tokens_per_s", "max_concurrent_requests", + "mean_ttft_ms", "std_ttft_ms", "p50_ttft_ms", "p90_ttft_ms"): + assert eval_lib.metric_policy(name) is None, name + + # Gateable, but not armed by a refresh. + assert eval_lib.is_auto_gateable("median_itl_ms") is False + + # --------------------------------------------------------------------------- # Dashboard metadata: the `rocm` column on both install layouts (issue #381) # ---------------------------------------------------------------------------