ci: stage TokenSpeed serving for nightly perf gating, with an opt-in docker socket - #452
Open
vivekkhandelwal1 wants to merge 11 commits into
Open
ci: stage TokenSpeed serving for nightly perf gating, with an opt-in docker socket#452vivekkhandelwal1 wants to merge 11 commits into
vivekkhandelwal1 wants to merge 11 commits into
Conversation
Serving metrics are already in eval_lib's gating allowlist, but no serving recipe is in the nightly matrix and no baseline is blessed, so nothing is gated. Turning that on from the single-seed numbers we have would derive thresholds from one observation and fail on the difference between two healthy runs, so this lands the plan, the staged config and the tooling gating needs -- not the gates themselves. docs/tokenspeed-gating-rollout.md collects every variance statement in the docs, decides gate/record-only/never per metric, and derives the record-only run count and threshold statistic from that evidence rather than convention. The claims about the comparator are tests, not prose: tests/ci/test_eval_lib.py feeds the measured numbers through compare_to_baseline and shows the proposed bounds tolerate the observed cross-sweep noise (0.15-2.63%) while catching a 30% regression, and that a startup-time or median-ITL gate would not. Two tooling changes, both needed to roll gating out for one workload: - refresh_baselines --perf-gate-entry. The baseline file is rewritten whole, so an unscoped --perf-gate would arm step-time ceilings for all sixteen existing cells off whatever single run was under way -- the one-observation threshold this exercise exists to avoid, applied to six other workloads. - eval_lib._NO_AUTO_GATE. median_itl_ms is measured at ~0, and a margin is multiplicative, so an observation of 0.0 blesses a ceiling of 0.0 that no later run can satisfy. It stays gateable by hand and charted; a refresh just may not arm it. _METRIC_POLICIES is unchanged: every serving metric worth gating is already there, and the allowlist is what --perf-gate arms from, so adding the noisy ones (server_startup_sec, the peaks, the means) would be gating them. The matrix entry is staged under `pending_entries`, not live, and the reason is not gating policy: nightly_eval.py runs inside the aorta-ci-gpu container, tokenspeed_serve runs the engine in a container of its own, and that image has no docker client and no daemon socket. setup() would raise, the cell would error, and fail-closed reddens the nightly -- record-only defers perf bounds, not failures. Nothing reads pending_entries, but tests/ci validates those entries exactly like live ones, so promoting it is a move of five lines rather than a bet on whether it still parses.
Work in progress, committed before a re-sync onto a main that has moved under
this file. The enablement half of the nightly gating blocker: nightly_eval.py
runs inside aorta-ci-gpu, tokenspeed_serve launches the engine in a container
of its own, and the CI image had neither a docker client nor a socket, so
setup() raised "'docker' not on PATH" and a live matrix entry would have
reddened the nightly every night.
- docker/install_docker_cli.py: fetch the pinned static tarball, verify its
sha256 before extracting, and extract 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. Stdlib-only and downloader-free
because the base guarantees python but promises neither curl nor wget.
- Dockerfile.ci-gpu: COPY + RUN python, matching the rocm_layout_guard pair
rather than an inline heredoc -- this node's daemon still runs the legacy
builder with no buildx, so a heredoc RUN would fail on syntax.
- docker-compose.docker-socket.yaml: an opt-in override, which is the
mechanism the base compose already documents for optional mounts ("Do not
add a volume here"), so the default container keeps no route to the daemon.
Mounts the socket, and mounts the scratch root at an identical path on both
sides so the -v sources the workload builds mean the same directory to the
host daemon as to the process constructing them.
Verified against the real artifact, not just by inspection: the installer runs,
the extracted client reports 29.7.2, negotiates against this node's 29.1.3
daemon, and a deliberately wrong sha256 fails without leaving a binary behind.
Not yet verified: a full image build (the ROCm base needs far more disk than
this node has free) and an actual container launch from inside the CI image.
The matrix entry stays staged under pending_entries. Nothing here demonstrates
a launch, and promoting on a Dockerfile edit alone is the outcome staging it
was meant to avoid.
Co-authored-by: Cursor <cursoragent@cursor.com>
Completes the WIP two commits back, re-derived against #411's rewrite of Dockerfile.ci-gpu. The entry stays in pending_entries; see the last section. Dockerfile.ci-gpu. The install is unchanged in shape but now follows the convention #411 introduced beside it -- prove it at build time, so the build says so instead of the nightly saying so hours later. A smoke-test RUN sits next to the hipcc link check and the proton dlopen check: `docker --version` (answered by the binary alone, so the build depends on no daemon and cannot pass because one happened to be reachable) plus an assertion that dockerd, containerd and runc are absent from PATH. That last one is the whole of "client only" -- the tarball ships all three, so shipping a daemon is a one-word edit away and nothing else in the image would notice. Also corrected a comment that pointed at the layout guard as being "below" when it is above. rocm-ci-setup gains a `docker-socket` input, default false, which adds the override as a second -f. It is opt-in per lane because the action is shared by five workflows and exactly one has a workload that needs the daemon; making it a property of being set up would hand host root to four unrelated jobs. No lane sets it. Turning it on is a security decision for whoever owns this CI, and a test asserts the list of lanes that do is empty so it can never be a quiet diff. Caught while wiring that: the flag could not be exported as DOCKER_SOCKET, which is the name the override substitutes as the socket's host PATH -- compose would have bind-mounted a directory called `true`, and only on the first run that enabled it. Renamed, and a test now rejects any env name in the action that the override reads as a mount source. The rollout doc answers the work_dir question properly rather than flagging it. The short version: the -v sources the workload builds are resolved by the HOST daemon, a missing bind source is created rather than refused, and the default work_dir does NOT save you by being /tmp/ts-work-serve on both sides -- the container's /tmp is not the host's, so the two are different directories with the same name, which is the failure with the confusing property that every log looks right. So work_dir must be an explicitly configured path that the override mounts at an identical string on both sides, and a test asserts source == target. The uid has to agree too: the harness runs as root in the container and writes u0, which the host agrees with only because both are uid 0 -- change `user:`, run as a non-root uid, or enable userns-remap and the ownership check fails with an error that mentions none of that. The security posture is stated in the override's header, in the doc, and in the action's input description rather than in one place: a container that can reach the daemon can ask for a privileged one bind-mounting /, which is root on the host with no exploit. The mitigation (already privileged, dedicated runner) is real but holds only for a single-tenant node. The recommended alternative if that is unacceptable is to run the serving workload outside the CI container entirely, on the runner host, publishing its JSON into the results directory. NOT PROMOTED, and blocked_on now says why. The CI image has never been built -- the ROCm 10 base is ~52 GB and no machine available here had the disk -- so what has actually been observed is the installer running, rejecting a bad sha256 without leaving a binary, and producing a 29.7.2 client that negotiates against a 29.1.3 daemon, plus `docker compose config` resolving all three mounts. None of that is a container launched from inside aorta-ci-gpu. Without one, the failure mode has merely moved from "'docker' not on PATH" to "cannot connect to the Docker daemon", which reddens the nightly exactly as hard, and promoting on the strength of a Dockerfile edit is what staging the entry was meant to prevent. The doc lists the commands a human must run. 563 tests pass (tests/ci, tests/workloads/test_tokenspeed_serve.py); ruff clean. Co-authored-by: Cursor <cursoragent@cursor.com>
The plan proposed four gates on the strength of cross-sweep spreads of 0.15-2.63%. Reading the step_times_ms actually on disk for the cell the staged entry runs -- six sweeps, 13 cell-runs, 39 steps -- says that was too many. Twelve of the thirteen cell-runs are very clean, tighter than the cross-sweep numbers. The thirteenth 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 -- so the doc's claim that warmup covers the compile-cache excursion is wrong, and the 10x TTFT inflation it treats as hypothetical is present in our data at 1 cell-run in 13. Run through compare_to_baseline with the plan's own extremum anchor and margins, three of the four proposed gates fail on that run: step_time_ms.max 2825 vs 1462, median_ttft_ms 465.30 vs 58.59, output_throughput 2612.83 vs 2977.15. All three are duration-derived or wait on the compile. median_tpot_ms and p99_itl_ms are measured between tokens and land *inside* the clean envelope, indistinguishable from a healthy run. So the first bless is those two, and the other three become record-only. Not a threshold tweak: on a bimodal cell the extremum anchor has no good value. A window without an excursion calls the next one a 10x regression; a window containing one blesses a ceiling that a genuine 2x TTFT regression passes. Enlarging the window only makes the second more likely. Gate a metric the second mode does not reach, or remove the second mode -- and here the second mode is removable, because it is positional, so raising warmup_steps to 2 is the follow-up that would let the other three be promoted. Left out of this change deliberately: it alters the measurement, and every number here was taken at warmup_steps: 1. This is a different animal from the concurrency-64 stall, which is ~8% of steps at a fixed ~0.92s with no position to discard. Same symptom in a summary statistic, opposite remedy. Also corrects server_startup_sec to the measured 180-415 s, wider than the 189-379 s quoted, and records that running the workload from inside the CI container does not move any metric more than 5% off the host-side envelope -- which is what lets a nightly baseline be compared against the host-side runs this analysis is built from. Co-authored-by: Cursor <cursoragent@cursor.com>
…lity
A cell was demonstrated end to end on gfx950: the CI image built (51.9
GB, docker client 29.7.2, and dockerd/containerd/runc/shim/proxy/init all
absent from PATH in the built image, so "client only" is a property of
the artifact and not of the Dockerfile's intent); `docker exec
aorta-ci-gpu docker ps` reached the host daemon; and two sweeps of
tokenspeed-serve-bench-smoke ran from inside the container, driving
sibling engine containers. Four cell-runs, all passed, twelve clean
steps at 1108-1149 ms, 12 minutes wall clock, engine containers cleaned
up. That is what promotion was waiting on.
Promoting into `entries` on its own would have been wrong, though, and
the matrix file's own header says why: an entry may only be added once
it can actually pass on the runner. The socket is a per-lane opt-in that
no lane sets, because it grants effective root and that is the CI
owner's decision -- so a promoted entry would fail on "cannot connect to
the Docker daemon" every night until a decision this branch must not
pre-empt.
So the capability is declarable. `needs_docker_daemon: true` makes the
entry SKIP when no daemon is reachable, with the reason in the results,
exactly as min_gpus already does for GPU count. The probe runs `docker
info` rather than `which docker`, because a mounted-but-dead socket is
the case that would otherwise pass a client check and then fail on
connect; it is consulted only by entries that ask for it, so a broken
daemon cannot turn the rest of the nightly into skips; and it is cached,
so it costs one subprocess. When a lane sets `docker-socket: true` the
entry starts running with no further edit.
Two work_dir requirements were found by doing this rather than by
reading, and the doc's guidance was too vague on both:
* ALL bind sources must be resolvable by the daemon, not just
work_dir. An autofs NFS checkout fails at container create with
"mkdir /home/...: permission denied", before the workload's own
checks can say anything more useful.
* The work root must be owned by the uid the container runs as. The
container is root, and a work_dir left by an ordinary host-side run
is owned by that user, so uid 0 refuses it. Which means the ten
record-only runs cannot share a work_dir with the containerised
nightly unless they are taken as root.
pending_entries is now empty but kept, with a note: the convention is
worth more than the one row was.
Co-authored-by: Cursor <cursoragent@cursor.com>
…ightly `rocm-ci-setup` has had a `docker-socket` input since earlier on this branch, but no caller workflow could reach it, so the tokenspeed_serve_smoke entry had no route to a daemon and could only ever report `skip`. The input is declared on `eval-reusable.yml` as `docker_socket`, defaulting to false, and forwarded to the setup step. It is deliberately not set inside the reusable workflow: that workflow is shared by the nightly and by `bump-validate.yml`, so a value chosen centrally would hand the socket to a PR-triggered lane as a side effect of enabling the nightly. A container that can reach the host daemon can start a privileged one bind-mounting /, which on a PR lane is remote root for anyone who can open a PR touching `docker/**`. So `nightly-eval.yml` sets it true -- that lane is workflow_run/dispatch triggered, not PR triggered -- and `bump-validate.yml` pins it false explicitly rather than relying on the default, so a later change to that default cannot arm it silently. `sanitizers-nightly.yml` does not go through this workflow at all; it calls the setup action directly and passes no socket argument, so it keeps the action default. Co-authored-by: Cursor <cursoragent@cursor.com>
`refresh_baselines.py` takes a repeatable `--perf-gate-entry NAME`, validated
against the nightly matrix, but `refresh-baselines.yml` is dispatch-only and had
no matching input -- so the scoped bless the rollout doc describes was not
runnable from the Actions UI, and the only reachable perf refresh was the
unscoped one. That rewrites the whole baseline file and arms step-time ceilings
across all sixteen cells, including workloads belonging to other people, each
from a single observation with no variance data behind it.
`perf_gate_entry` defaults to empty, so a dispatch that omits it behaves exactly
as before. A comma- or space-separated list maps to one flag per name.
Two guards, in a pre-flight step rather than in the script, because the script
only reaches its own checks after pip has installed the wheel inside the
container -- minutes of GPU runner time to learn about a typo in a form. A scope
without `perf_gate` is an error, mirroring the script. An unscoped perf refresh
is only a warning: it is a legitimate end state once every workload has variance
data behind it, and it is what every existing dispatch of this workflow has
done, so refusing it would break a documented invocation.
Dispatch inputs now reach the container through `env:` and `-e` rather than
being interpolated into the shell body. `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 commands on the self-hosted
runner. This also removes the three pre-existing interpolations of the older
margin and perf_gate inputs.
Co-authored-by: Cursor <cursoragent@cursor.com>
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 a 1108-1178 ms range. It lands on the first *measured* step, after the single warmup step has already been discarded, and it breaches three of the four originally-proposed gates on a run that is not a regression. `num_warmups` cannot substitute for this: 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. Set in the recipe rather than the matrix entry because `nightly_eval.py` reads only recipe/min_gpus/timeout_sec/needs_docker_daemon from an entry -- there is no `workload_config` override -- and because the recipe is shared with the baseline refresher, which is what keeps the blessed numbers and the nightly on the same measurement. The cost is that it invalidates every number in the rollout doc as a predictor of the record-only window, since those were all taken at `warmup_steps: 1`. That is accepted rather than deferred: the window has not been taken yet, so changing the setting now costs nothing, whereas changing it afterwards would cost ten nights. The variance table is kept as the rationale for the change. Co-authored-by: Cursor <cursoragent@cursor.com>
…p change The existing socket tripwire only inspected `docker-socket: true` on a direct `rocm-ci-setup` step, so the route the nightly actually uses -- a reusable workflow forwarding its own input -- would have passed it unnoticed. Replaced with a helper that walks both routes and discovers the forwarding wrapper rather than hardcoding it, so a second wrapper is covered the day it is added. The security-critical assertion is derived from each workflow's `on:` block rather than a filename list: no PR-triggered lane may reach the host daemon, which stays true if another PR-triggered caller is added later. Verified by mutation -- enabling the socket on `bump-validate.yml`, flipping the reusable default to true, and hardcoding the value in the shared workflow each fail. Also covers: the input defaults off and is forwarded rather than overridden; the declaration states why it must stay off for PR lanes; `perf_gate_entry` exists with an empty default and maps a list to repeated flags; the flag is passed only when non-empty; inputs cross into the container as environment rather than shell interpolation; the unscoped refresh warns without failing; and the gating recipe discards two steps with the rollout doc's provenance caveat intact. The interpolation guard is scoped to the four workflows this change owns. `gemm-sweep-analysis.yml` and `rccl-warp-speed-analysis.yml` interpolate dispatch inputs into shell bodies today; that is pre-existing and left alone. Co-authored-by: Cursor <cursoragent@cursor.com>
… plumbing The variance data -- 13 cell-runs, 39 steps -- was measured at `warmup_steps: 1`, which the recipe no longer uses. Rather than delete it or restate it at the new setting (there is no measurement at the new setting yet, and inventing one would be worse than having none), it is explicitly reframed as the rationale for the change instead of the baseline to bless against, with the table itself untouched. The ten-night record-only window is now pinned to `warmup_steps: 2` and must be taken afresh; the doc says so at the window, at the variance table, and in the decisions list, since that is the load-bearing consequence. The follow-up that previously read "raise warmup_steps to 2 and re-take the window" now points at that window rather than scheduling a second one, and the "left at 1" decision entry is rewritten as the reversal it is. Also updated for the socket plumbing: enabling it is a `docker_socket: true` on the `eval-reusable.yml` call in `nightly-eval.yml`, not an edit to the setup step, with the per-lane scoping and the two lanes that must stay off spelled out. The bless step now shows the actual dispatch form fields. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Baseline refreshes cannot run the daemon-dependent entry, scoped refreshes can remove prior gates, and the socket is enabled before sign-off.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds TokenSpeed serving to nightly evaluation, including Docker-daemon plumbing, scoped performance gating, rollout documentation, and regression tests.
Changes:
- Adds the serving matrix entry and Docker client/socket support.
- Adds per-entry baseline gating and metric exclusions.
- Documents and tests the staged gating rollout.
File summaries
| File | Description |
|---|---|
tests/ci/test_nightly_eval.py |
Tests scoping, daemon handling, and matrix configuration. |
tests/ci/test_eval_lib.py |
Tests serving metric policies and measured thresholds. |
tests/ci/test_dashboard_and_alert.py |
Tests workflow, socket, and refresh wiring. |
scripts/ci/refresh_baselines.py |
Adds per-entry performance-gate scoping. |
scripts/ci/nightly_eval.py |
Skips daemon-dependent entries when unavailable. |
scripts/ci/eval_lib.py |
Excludes near-zero ITL from automatic gating. |
scripts/ci/dashboard_metadata.py |
Adds serving dashboard metadata and reproduction steps. |
recipes/tokenspeed/tokenspeed-serve-bench-smoke.yaml |
Increases discarded warmup steps. |
docs/tokenspeed-serving.md |
Links serving gating guidance. |
docs/tokenspeed-gating-rollout.md |
Documents rollout evidence and procedure. |
docs/ci-nightly-eval.md |
Documents scoped baseline refreshes. |
docker/install_docker_cli.py |
Installs a pinned Docker client. |
docker/Dockerfile.ci-gpu |
Adds the Docker client to the CI image. |
docker/docker-compose.docker-socket.yaml |
Adds optional host socket and scratch mounts. |
config/ci/nightly_eval_matrix.yaml |
Adds the TokenSpeed serving entry. |
.github/workflows/refresh-baselines.yml |
Exposes and forwards gate scope inputs. |
.github/workflows/nightly-eval.yml |
Enables the Docker socket for nightly evaluation. |
.github/workflows/eval-reusable.yml |
Forwards an opt-in socket setting. |
.github/workflows/bump-validate.yml |
Explicitly disables the socket for PR validation. |
.github/actions/rocm-ci-setup/action.yml |
Applies the socket Compose override conditionally. |
Review details
- Files reviewed: 20/20 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # 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 |
| recipe: recipes/tokenspeed/tokenspeed-serve-bench-smoke.yaml | ||
| min_gpus: 1 | ||
| timeout_sec: 3600 | ||
| needs_docker_daemon: true |
| # 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): |
Comment on lines
+356
to
+365
| { | ||
| "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", | ||
| ], | ||
| }, | ||
| ], |
…de envelope is not an acceptance check Two claims in the rollout plan were disproven, and one measurement is added. The ten-night rationale listed a Dependabot ROCm bump among the events the window is long enough to *contain*, beside a runner reimage and a cold cache. That conflated two opposite categories. An environmental event perturbs a run over an unchanged stack, so the extremum has measured a genuine bad night; a digest bump replaces what is being measured half-way through, so the window stops describing one population. You can sample a stack bump or derive a stable ceiling across the window, but not both. sanitizers-nightly is the live counter-example: #411 moved Dockerfile.ci-gpu to ROCm 10, the GEMM objects the gate extracts from the image changed with it, and the committed expectation now describes objects the image no longer ships (#453). A perf baseline is worse protected, because a shifted number still looks like a number. Hence the operational condition: for the duration of the window, hold any PR moving the ci-gpu FROM digest or the TokenSpeed engine digest. Dependabot runs /docker weekly and has no auto-merge, so a human has to let one through -- preventable by policy. Verified that the only open docker-ecosystem PR, #309, touches the ebpf/nan/pytorch Dockerfiles and not ci-gpu. The residual-breach-rate analysis and the calendar-coverage argument for the three environmental events are unchanged. Separately, the 1108-1149 ms envelope was being carried as if it were a night-one acceptance check. It was taken on an MI355X dev node; the runner smci350-rck-g03-f16-12 is an MI350X, with a lower power budget and air rather than liquid cooling. Same gfx950/CDNA4 ISA, so correctness does not move, but the level may. What has to reproduce across the window is the spread, not the level -- the ceilings are derived from the ten CI nights themselves, so a systematic offset is absorbed by construction. One MI350X cell at warmup_steps: 2 is recorded against that, on Slurm cv350-rck-g03-c16-18 (1000 W cap, VBIOS 113-M350) rather than the reserved CI runner. Every metric landed inside the MI355X clean range and there was no step-0 excursion. Labelled as one cell-run, not a spread: its 0.13% step span is within-cell and is not comparable to the 6.3% across twelve cell-runs, and the warmup_steps: 1 table is retained unchanged as the rationale it always was.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The socket is enabled before its pending security sign-off, and the daemon-dependent entry currently makes baseline refreshes fail.
Review details
Suppressed comments (4)
config/ci/nightly_eval_matrix.yaml:133
- Making this daemon-dependent row live currently breaks the baseline refresh path.
build_baselines()runs every live entry but has no daemon capability handling, whilerefresh-baselines.ymlinvokesrocm-ci-setupwithoutdocker-socket; therefore this cell runs without a daemon, is marked incomplete, and aborts every refresh—including the documented scoped serving bless. Add a trusted, default-off socket input to the refresh workflow and/or carry daemon-dependent entries when unavailable, while failing if the explicitly selected target cannot run.
- name: tokenspeed_serve_smoke
recipe: recipes/tokenspeed/tokenspeed-serve-bench-smoke.yaml
min_gpus: 1
timeout_sec: 3600
needs_docker_daemon: true
.github/workflows/nightly-eval.yml:46
- This enables the root-equivalent host socket even though this PR's rollout document says no lane currently enables it, the sign-off is still pending, and to leave it false until that sign-off (
docs/tokenspeed-gating-rollout.md:493, 607-614, 819). As merged, the first nightly would grant host-daemon access before the stated single-tenant/security decision. Keep this false for this PR and update the workflow-shape expectation; enable it only in the separately signed-off change.
docker_socket: true
scripts/ci/refresh_baselines.py:120
- A scoped refresh rebuilds out-of-scope cells with only correctness metrics, so it silently deletes any performance gates those cells already had. For example, after serving is gated, a later
--perf-gate-entry other_entryrefresh removes serving'sstep_time_msand min/max metric bounds, defeating incremental rollout. Preserve existing performance specs for out-of-scope cells, or require/document the argument as the complete set of entries whose gates should remain enabled.
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_auto_gateable(mname):
scripts/ci/dashboard_metadata.py:363
- This setup step is titled as pre-warming the model cache, but
docker pullonly warms the engine-image cache;HF_HOMEis otherwise unused, so Qwen weights are still downloaded during the first run (and offline reproduction still fails). Addhf download Qwen/Qwen3-0.6Bunder thisHF_HOME, as the other serving recipes document, or rename the step so it does not claim the model cache is populated.
"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",
],
- Files reviewed: 20/20 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It changes CI security posture and workflow wiring (including optional host Docker socket access), which warrants explicit human sign-off and alignment with the stated rollout plan.
Review details
Suppressed comments (1)
.github/workflows/nightly-eval.yml:46
- The workflow currently sets
docker_socket: true, which enables mounting the host Docker daemon socket into the CI container. That conflicts with the stated “safe to merge before socket sign-off / enable afterwards” rollout narrative (and the nearby comment that leaving thisfalseis safe until sign-off). If sign-off is still pending, keep thisfalsesotokenspeed_serve_smokestays in the intentionalskipstate; if sign-off is complete, the PR description/rollout text should be updated to reflect that the socket is being enabled in this PR.
# 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
- Files reviewed: 20/20 changed files
- Comments generated: 0 new
- Review effort level: Lite
amd-vivekag
reviewed
Sep 3, 2026
amd-vivekag
left a comment
amd-vivekag
left a comment
Collaborator
There was a problem hiding this comment.
Review summary
Verdict: fix-before-ship
I reviewed the full current diff across the workflow/Docker boundary, runtime gating, tests, and rollout docs. CI is green, but the baseline rollout path is still broken and the host-root socket is enabled before its stated sign-off; all four prior Copilot findings remain open at this head. I also found three new guard/input/documentation gaps.
Findings: 3 High · 2 Medium · 2 Low
Still open from last review
- High: The nightly mounts the root-equivalent Docker socket while single-tenant/sign-off confirmation is still pending.
- High: The new daemon-dependent matrix entry makes every baseline refresh fail, including the documented serving bless.
- High: A scoped refresh silently deletes performance gates previously blessed for other entries.
No prior concern is resolved yet; no commits landed after those comments.
| # 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 |
Collaborator
There was a problem hiding this comment.
[High] This enables host-root access before the required sign-off.
The rollout still says no lane enables the socket and the PR says single-tenant confirmation is pending.
Fix: Keep docker_socket false or omitted until named approval, then enable it in a separate reviewed change.
| recipe: recipes/tokenspeed/tokenspeed-serve-bench-smoke.yaml | ||
| min_gpus: 1 | ||
| timeout_sec: 3600 | ||
| needs_docker_daemon: true |
Collaborator
There was a problem hiding this comment.
[High] This live entry makes every baseline refresh fail.
build_baselines() runs every GPU-eligible entry, but the refresh workflow neither mounts the socket nor handles needs_docker_daemon, so this produces no matrix and triggers the atomic abort.
Fix: Preserve unavailable daemon-dependent entries during ordinary refreshes, and provide an approved socket-enabled path when this entry is explicitly selected.
| # 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): |
Collaborator
There was a problem hiding this comment.
[High] A scoped refresh silently removes existing performance gates.
Out-of-scope specs are rebuilt without their old step_time_ms or performance metrics, then the whole baseline file is replaced; blessing workload B therefore ungates workload A.
Fix: Merge refreshed correctness fields into each out-of-scope existing spec while preserving its performance bounds.
| 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": |
Collaborator
There was a problem hiding this comment.
[Medium] This security guard recognizes only the literal string true.
A PR caller using ${{ true }} or another truthy expression gets the socket at runtime but is omitted from enabled, so this test still passes.
Fix: For PR-triggered routes, reject every socket value except explicit literal false, and add an expression-valued mutation case.
| # 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 |
Collaborator
There was a problem hiding this comment.
[Medium] A delimiter-only scope silently becomes a global perf refresh.
Inputs such as , or whitespace pass the raw non-empty check, produce no names after splitting, and leave only --perf-gate, which arms every matrix entry without the unscoped warning.
Fix: Parse before validation and reject a non-empty raw value when no names remain; add comma-only and whitespace-only tests.
| "nothing changed)." | ||
| ), | ||
| setup_extra=[ | ||
| { |
Collaborator
There was a problem hiding this comment.
[Low] This step does not pre-warm the model cache.
docker pull only warms the engine-image cache, and this exported HF_HOME is not the cache path the workload resolves.
Fix: Rename the step to pull the engine image, or actually download Qwen/Qwen3-0.6B into the workload's mounted cache as the running UID.
| 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 |
Collaborator
There was a problem hiding this comment.
[Low] This command does not repair the documented ownership failure.
If /tmp/ts-work-serve already exists under a user UID, mkdir -p as root leaves its owner and mode unchanged, so the next command still fails.
Fix: Require a fresh path, or explicitly verify and correct ownership and permissions before starting Compose.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a
tokenspeed_serve_smokeentry to the nightly evaluation matrix, plus theDocker client and opt-in socket plumbing needed to support it, and the rollout
plan for turning perf gating on for it.
Safe to merge before the socket sign-off
The socket decision is still open, and this PR does not depend on it. The entry
declares
needs_docker_daemon: true, andnightly_eval.pyprobes for areachable daemon before running it: with no socket the entry reports
skipwiththe reason recorded in the results (visible on the dashboard) rather than
failing on
Cannot connect to the Docker daemon. Both paths are covered intests/ci/test_nightly_eval.py— the skip path and the run-when-reachable path.Enabling it afterwards is one line in
nightly-eval.yml:The socket is opt-in per lane
rocm-ci-setupalready had adocker-socketinput defaulting tofalse, but nocaller could reach it. This adds a matching
docker_socketworkflow_callinputon
eval-reusable.yml, also defaulting tofalse, forwarded to the setup step.It is deliberately not set inside
eval-reusable.yml. That workflow isshared by the nightly and by
bump-validate.yml, so a value chosen centrallywould hand the socket to a PR-triggered lane as a side effect of enabling the
nightly. A container that can reach the host daemon can start a privileged one
bind-mounting
/, so on a PR-triggered lane that is remote root for anyone whocan open a PR touching
docker/**.docker_socketnightly-eval.ymlworkflow_run(Nightly wheels) + dispatchtruebump-validate.ymlpull_requestfalse, set explicitlysanitizers-nightly.ymleval-reusable.yml; callsrocm-ci-setupdirectly and passes no socket argumentbump-validate.ymlpinsfalserather than relying on the default so that alater change to that default cannot arm it silently. The test that guards this
derives "PR-triggered" from each workflow's
on:block rather than a filenamelist, so a PR-triggered caller added later is covered without editing the test.
Note that the pre-existing tripwire only checked for
docker-socket: trueon adirect
rocm-ci-setupstep, which the reusable-workflow route would have passedunnoticed. It now walks both routes.
Security posture
The socket widens an existing posture rather than opening a new one. The CI
container already runs privileged, with
seccomp=unconfinedand hostnetworking, on a dedicated self-hosted runner. Adding the daemon socket makes
"effective root on the runner" reachable more directly, but does not introduce
it.
docker/docker-compose.docker-socket.yamlstates the grant and records analternative for CI that cannot accept it.
This is contingent on the runner being single-tenant, which is still pending
confirmation. If that runner is shared with anything else, the argument above
does not hold and the nightly lane should stay at
false.Perf-gate scoping
refresh_baselines.pyhas a repeatable--perf-gate-entry NAME, validatedagainst the matrix, but
refresh-baselines.ymlis dispatch-only and had nomatching input — so the scoped bless was not runnable from the Actions UI and
the only reachable perf refresh was the unscoped one, which rewrites the whole
baseline file and arms step-time ceilings across all sixteen cells (including
gpu_smoke,inference_offline,training_ddp,training_fsdp,raceandllm_determinism— other people's workloads, each from a single observationwith no variance data).
perf_gate_entrydefaults to empty, so a dispatch that omits it behaves exactlyas before. A comma- or space-separated list maps to one flag per name. A scope
without
perf_gateis a fast error; an unscoped perf refresh warns but is notblocked, because it is a legitimate end state once every workload has variance
data and it is what every existing dispatch has done.
Dispatch inputs now reach the container through
env:/-erather than beinginterpolated into the shell body —
perf_gate_entryis free text and${{ }}substitution happens before bash sees the script. This also removes three
pre-existing interpolations.
warmup_steps: 1→2A step-0 Triton compile excursion reaches the metrics at
warmup_steps: 1,measured at 1 cell-run in 13: a ~10× first-token spike (465 ms against a
43–47 ms clean range) and a 2825 ms step against a 1108–1178 ms range. It lands
on the first measured step, after the single warmup step has already been
discarded, and breaches three of the four originally-proposed gates on a run
that is not a regression.
num_warmupscannot substitute — it warms requestswithin one bench invocation, not the compile cache across invocations.
Consequence, handled in the docs: the variance data in
docs/tokenspeed-gating-rollout.mdwas all taken atwarmup_steps: 1, so it isno longer a predictor of what the record-only window will show. The table is
kept — there is no measurement at the new setting yet — but explicitly reframed
as the rationale for the change rather than the baseline to bless against, and
the ten-night window is pinned to
warmup_steps: 2.Rollout
See
docs/tokenspeed-gating-rollout.mdfor the record-only window and thetwo-metric first bless:
median_tpot_msandp99_itl_ms. Those two arethe metrics whose definitions exclude the excursion (1.01× and 1.00× on the
excursion run), which is why they are the ones that survive. The three
duration-derived metrics stay record-only until the window shows the excursion
gone.
Tests
tests/ci/: 198 passed (189 before, 9 added).tests/workloads/test_tokenspeed_serve.py:382 passed. The new workflow-shape tests were verified by mutation — enabling
the socket on
bump-validate.yml, flipping the reusable default, hardcoding thevalue in the shared workflow, reverting
warmup_steps, dropping theperf_gate_entryinput, and reintroducing shell interpolation each fail.