Retain managed trainer runtime metadata in the GPU image - #892
bradhilton wants to merge 1 commit into
Conversation
bradhilton
left a comment
There was a problem hiding this comment.
Minsky review (third slot) — ART #892, exact head 4cbeedc91ebadc1f52186efbd837fd5f81a51777 on base 2ebfc1c2a38dbeeaeefac998323b476989602942 (merge-base = base = ART main, single parent). Verdict: CLEAR. Head and OPEN state (draft=true) rechecked immediately before posting (target 2ebfc1c2; Prek pending at that moment; GPU validation job skipped by design).
Scope and source joins. Two files, +21/−0: one COPY at docker/art-gpu.Dockerfile:143 and a 20-line test. GitHub's synthetic merge 3208e0a7 has parents 2ebfc1c2 and this head with the head's blobs; git diff --check clean; no file under src/art/megatron changes; megatron_runtime/pyproject.toml (5,798 B) and uv.lock (525,168 B) are byte-identical between base and head. Removing the single added line reproduces the parent Dockerfile.
What the COPY does, traced through both stages. The builder stage sets WORKDIR /opt/src/art and COPY megatron_runtime/pyproject.toml megatron_runtime/uv.lock ./megatron_runtime/ from the build context (which scripts/build-gpu-image.sh populates from exactly those two repo files at lines 300–307), runs uv sync --project megatron_runtime --frozen --extra cuda12 --no-install-project --no-dev --python 3.12 to warm /opt/uv-cache, then rm -rf megatron_runtime/.venv, leaving the two metadata files at /opt/src/art/megatron_runtime/. The final stage (last FROM) creates the sky user at line 131, copies /opt/uv-cache and /opt/uv-python with --chown=sky:sky, and the new line copies exactly those two files, multiple explicit sources to a trailing-slash directory destination with the same ownership convention, before USER sky. No virtual environment, source tree, second cache copy or executable is added; no dependency, version, lock, build command, CUDA profile, image argument, user or workflow policy changes, so installed runtime behavior is unchanged and the only image difference is ~531 KB of already-pinned metadata at a new path.
Self-containment of the retained metadata. Parsing the project: name art-megatron-runtime, eight tool.uv.sources entries, none of them path/workspace/editable, no uv workspace; the lock's sources are 407 registry, 6 git, 1 URL and the project's own virtual = ".", with zero editable or directory entries. So the two files are sufficient inputs for a frozen --no-install-project sync against the warmed cache; whether that sync, native linkage or timing succeed in a built image is a separate qualification, as all three reports say.
Test discrimination. The new test isolates the final stage by rsplit("\nFROM ", 1), collects every COPY line mentioning /opt/src/art, and requires the list to equal exactly the one expected instruction (sources, destination, --from=builder, --chown=sky:sky). Missing metadata (the base) fails; a misdirected destination, a whole-directory or .venv copy, a wrong owner or a builder-only copy all change the list and fail, matching the owner's seven controls. It is a source-structure guard, not an image-filesystem test, which is the right scope for a one-line stage change; the pre-existing test still checks build-context coverage of non---from COPYs.
Witness (serial, native threads 1). Planned concurrency one process; cgroup 220 GiB of 512. Head: 2 passed. Negative control: the head test against the base Dockerfile fails exactly the new test (Right contains one more item: [...]) and passes the pre-existing one. Consistent with Nash's report/manifest and the Euler and Archimedes reviews, which I read; my verification of source joins, stage context, metadata self-containment and the negative control is independent of them.
Four standing conditions. (1) Small. (2) No public API change: no callable, CLI, service or package-metadata contract; the image exposes two already-pinned internal lock inputs at a new path for a downstream consumer. (3) No art.megatron change, assessed on content: no module file changes and the copied project/lock are byte-identical; copying them into the image neither modifies the module nor moves its native dependencies into public extras. (4) Behavior: additive image content only; no runtime selection, import, install, GPU, retry, training or cleanup behavior changes; it serves the infrastructure repair Brad asked for. Nothing unrequested and Brad-relevant. Eligible on this head with the required independent CLEARs; green final-head CI remains a separate gate, and the skipped two-H200 job means no GPU qualification is claimed.
Explicitly outside this verdict. Workstation #68 (deb31773) is context only and remains held for Brad; this ART metadata neither repairs nor authorizes that consumer, whose fatal imports, GPU assertions and sub-30 s acceptance gate remain. Actual image build, cache usability, native ABI, timing and adoption are unqualified here.
Void if the head changes.
|
Review history (consolidated 2026-09-17) Three routing and pickup-claim comments for the third independent review at |
The GPU image warms the managed Megatron runtime in its builder stage, then discards that project while retaining only dependency caches. Copy its exact
pyproject.tomlanduv.lockinto the final image so downstream checks can select the same managed project. No virtual environment, dependency version, lock content, or runtime code changes.Validation: two focused context tests passed; seven source controls cover missing metadata, overly broad copies, ownership and stage placement. Ruff/format/diff checks passed. No image build or GPU qualification was run.
Merge remains held while the existing automatic image workflow is aligned with the current resource constraints. Its unchanged defaults prewarm three Kubernetes contexts and optionally Modal, and its builder/smoke cleanup predates the current Caladan guards. Three independent final-head source reviews are CLEAR; CI and automatic resource admission remain separate gates. The paired consumer is Workstation #68, whose merge requires Brad’s approval and an actual metadata-bearing base image.