ci(docker): move builds to docker-build nodes; resolve upstream refs up front - #2016
Open
guapisolo wants to merge 12 commits into
Open
ci(docker): move builds to docker-build nodes; resolve upstream refs up front#2016guapisolo wants to merge 12 commits into
guapisolo wants to merge 12 commits into
Conversation
Decision record for the docker build refactor series: measured baseline (72min fully-cold builds on GPU runners, zero layer-cache hits), settled decisions (co-located docker-build nodes, persistent local builder cache, SHA build-arg invalidation, GPU smoke gate, pip discipline buckets), and the implementation/verification plan the follow-up commits execute. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The docker build never touches a GPU: buildx's docker-container driver gives RUN steps no GPU access, so every past build already ran in a GPU-less sandbox while occupying a 2-GPU H200 runner for up to 72min. Point docker-build.yml and pr-test.yml's docker-build job at the dedicated docker-build runner label instead; these are CI machines carrying a second runner instance, per docker/build-refactor-design.md §3.1 (PR-1 cut: no builder/cache change yet — layer cache stays off until SHA-pinned invalidation lands). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ld-args Rework docker-build.yml's check-upstream into resolve-upstream, which now runs on every trigger and emits the full set of inputs the image bakes (sglang/megatron/miles HEAD SHAs + per-release wheels asset fingerprints) as job outputs; scheduling keeps the exact same rebuild gate. The resolver fails loudly on any empty value, and build-and-push now requires it to succeed instead of always() — a failed resolve must block the build, not let it proceed unpinned. build.py gains a repeatable --build-arg KEY=VALUE passthrough (the gap docs/ci/02-docker-build.md recorded), appended after variant args so an explicit value wins. No consumer change yet: the Dockerfile starts checking out these SHAs in the next cut (docker/build-refactor-design.md §3.3). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
First runs on the persistent docker-build nodes exposed the 'Install Python + dependencies' step for what it is — per-run host mutation: apt-get update raced a host apt process (lock failure, novita-host2) and pip3 --break-system-packages tried to replace the debian-owned rich, which has no RECORD to uninstall (novita-host4). Persistent shared nodes must not be apt/pip targets of every build. Remove the step from both docker build jobs and drop what made it necessary: build.py's typer dependency (argparse now, variant choices come straight from VARIANTS) and the prune step's host-jq assumption (stdlib urllib/json/re heredoc, secrets via env instead of inline interpolation). Build nodes now need exactly docker + stock python3. Dry-run emits byte-identical buildx commands to the typer version. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
The Dockerfile now refuses to build without exact pins: SGLANG_COMMIT / MEGATRON_COMMIT / MILES_COMMIT are required (empty ⇒ hard fail, never a branch-HEAD fallback) and each built arch requires a WHEELS_FP_* asset fingerprint, referenced in the download RUN as its cache key. With layer caching, 'git clone -b branch' text never changes while the branch moves — these pins are what make cache hits correct, and the same SHAs rebuild the same image (02 doc's retention gap). docker/resolve_upstream.py is the single resolver: the resolve-upstream job shells out to it (same fingerprint algorithm byte-for-byte) and build.py fills in any pin the caller didn't pass — CI passes the gated source SHAs, while wheels fingerprints are always resolved per variant (a static workflow mapping would hand cu12-x86 the cu13 fingerprint and mis-key its cache). sglang fetches the SHA directly (shallow clone + rebased branch make branch fetches unreliable); Megatron clones fully, checks out the pin, then syncs submodules; the unpinned FlashQLA install is pinned to today's HEAD like every other git install. SGLANG_BRANCH / MEGATRON_BRANCH args are gone — they no longer affect the image. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replace the per-run setup-buildx-action builder (fresh docker-container builder every run: the reason every build measured fully cold, zero CACHED lines in 44min) with an idempotent named builder, miles-builder, that persists on each docker-build node together with its state volume. Cache hits are correct as of the SHA/fingerprint pinning in the previous commit: an upstream that moved changes a build-arg and precisely busts its layer; nothing else re-runs. PR builds share the same builder, so a node warm from a scheduled build serves PR builds too. build.py gains --builder for explicit selection instead of a stateful 'buildx use' default; both workflows pass --builder miles-builder. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… var The resolve step's tail still echoed all six values into GITHUB_OUTPUT from shell variables, but the script rewrite extracts only the five the gate compares — MILES_SHA was gone and set -u killed the job. The six values already reach GITHUB_OUTPUT via resolved.txt; only should_build is produced here. Step body now exercised end-to-end locally (both gated and ungated paths). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e env Restructure the recipe around the now-live layer cache (design doc §3.5): - Layer order is ascending change frequency: apt/nccl-tests, release wheels, pinned third-party installs, the requirements.txt resolve, then the fast-moving source trees last (Megatron → sglang → miles → the tvm-ffi reconcile that must stay the final pip op). A one-line requirements.txt bump no longer rebuilds sglang/miles checkouts' predecessors, and daily source-pin moves invalidate only their own tail. - pip discipline: release wheels install as one --no-deps layer; packages whose deps the base already carries consolidate into docker/requirements-nodeps.txt (all were --no-deps one-liners — pure consolidation); requirements.txt and Megatron's editable install run against an on-the-spot 'pip list --format=freeze' constraints file, so they may add packages but a conflict with anything installed fails loudly instead of silently reinstalling it. flash-linear-attention / Emerging-Optimizers stay independent layers: requirements.txt feeds setup.py install_requires, CPU CI, and user docs, so image-only git pins cannot live there. - Caching: every resolving/downloading pip RUN gets a pip cache mount (the rm -rf /root/.cache/pip layer is gone), apt gets list + archive cache mounts, and the wheels download goes through a /wheels-cache mount via docker/fetch_wheels.py, which reuses an asset only when its release asset id matches — rolling releases replace assets under the same filename, so existence alone would pin stale wheels forever. - Both workflows' docker path filters learn the new files (and the push trigger closes its pre-existing build.py / docker/patch gap). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
First C5 build tripped the new constraints guard on a legitimate case: requirements.txt pins xxhash==3.7.1 while the base image ships 3.8.1 — the old unconstrained install has been silently downgrading it all along, which is miles' declared contract, not transitive drift. Exclude packages requirements.txt itself names from the on-the-spot freeze: explicit pins win, transitive resolution still cannot move anything else. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
torch 2.11+cu130's metadata pins nvidia-cudnn-cu13==9.19.0.56 while the recipe deliberately installs 9.22 for TE. The reorder had moved that bump before the requirements resolve, whose constraints freeze then carried 9.22 and collided with torch's pin (ResolutionImpossible). The bump is the same species as the apache-tvm-ffi reconcile — a forced override that contradicts declared metadata — so it lives with it in a 'post-resolve forced overrides' tail after every resolver in the file: any later constrained resolve would veto it, any later unconstrained one would silently undo it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…pushed docker-build.yml now --loads the amd64 image from the same builder cache the push builds hit and boots it on the build node's GPU running docker/smoke_test.py: CUDA visible + tensor math, real TE/sglang/miles imports, nccl-tests binaries. A broken image never reaches a tag — the gate targets the class where a green build ships a broken runtime (a --no-deps install once dropped onnxscript and every GPU job died on the blessed dev image). Simpler than a push-then-promote design: no timestamped-tag capture, no imagetools promotion, manual and automatic paths gate uniformly; the subsequent push is all cache hits for amd64. arm64 halves ship unprobed (no ARM GPU nodes); rocm / cu13-aarch64 skip. build.py gains --load (single-platform only, loud otherwise). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Commit 1849b55 (#2028) made the session server read args.sglang_speculative_algorithm during sample assembly (miles/rollout/session/samples/merge.py:126) and updated its own test namespace in test_samples.py, but not the three other hand-built SimpleNamespace args that feed SessionServer in tests. On any PR merge ref containing that commit, every agentic-variant test 500s at POST /sessions/<id>/samples: stage-b-cpu fails outright, and stage-a-cpu shards 1/3 hang on the waiting client until the 60-minute job timeout cancels them (run 30629312024). Mirror the driver args value in the two shared fixtures and pin None in test_session_samples_op._ARGS, matching how #2028 patched test_samples.py. Verified on a local merge-ref checkout (9bc4c25 + origin/main): the failing generate_hub test and the hanging inference_rollout integration file pass; the full router/session/generate_utils/inference_rollout sweep is 211 passed, 0 failed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
2 participants
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.
What
First two cuts of the docker build refactor (design doc included as
docker/build-refactor-design.md, measured baseline: 72min fully-cold builds on GPU runners with zero layer-cache hits):18e1c4e70):docker-build.ymland pr-test'sdocker-buildjob now run on the dedicateddocker-buildrunner label instead of["h200","2gpu"]. The build never touches a GPU (buildx sandboxes RUN steps), so image builds stop occupying GPU-suite runner slots. 6 build runner instances (aliyun/novita/scitix) are already online.c0f594a6c):check-upstream→resolve-upstream, which now runs on every trigger and outputs all inputs the image bakes (sglang/megatron/miles HEAD SHAs + wheels release fingerprints); the schedule rebuild gate is unchanged. Resolver failures are loud (empty value ⇒ job fails ⇒ no build), replacing the silentcurl -sflow.build.pygains a repeatable--build-argpassthrough. No consumer change yet — the Dockerfile starts checking out these SHAs in the next cut, making images reproducible builds.Behavior changes
resolve-upstreamto succeed (droppedalways()): a failed resolve blocks the build instead of building unpinned branch HEADs.resolve-upstreamruns on push/dispatch too (was schedule-only); gate semantics unchanged.Verification
docker/build.py --dry-runshows--build-argpassthrough appended after variant args; malformed args rejected.docker-build.yml(cu13-x86, custom tagbuildnode-smoke, run 30598056858) + this PR's own docker-build job must land on*-docker-builder-0runners.🤖 Generated with Claude Code