feat: enforce pinned image digest for madengine run by coketaste · Pull Request #178 · ROCm/madengine

coketaste · 2026-08-28T02:56:53Z

Summary

Every build now records the pushed image's sha256 digest as image_digest on each built_images manifest entry (always on, no behavior change by default).
New --require-pinned-image flag (and equivalent require_pinned_image additional-context key) makes madengine run pull registry images by repo@sha256:... instead of by tag, across local Docker, Kubernetes, and SLURM (including slurm_multi). If the manifest has no recorded digest, the run fails immediately with no tag fallback — a moved/mutable tag can no longer silently resolve to the wrong image.
SLURM propagates the setting through the manifest's context block so nested madengine run invocations on compute nodes inherit it; build-on-compute-node manifests are also covered.

Test plan

pytest tests/unit -q — 638 passed
pytest tests/integration -q — 151 passed, 1 skipped (needs non-AMD GPU)
mypy src/madengine/core/image_digest.py — clean
Manual trace of both SLURM compute-node manifest rewrites in job.sh.j2 to confirm context survives re-dump
Design doc and implementation plan under docs/superpowers/

🤖 Generated with Claude Code

…est (#166) MAD_CONTAINER_IMAGE (local image) mode built a synthetic manifest that omitted the models.json `multiple_results` field. Without it, ContainerRunner never sets MAD_OUTPUT_CSV, never copies the perf CSV out of the container, and falls back to scraping the run log for a "performance: NUMBER METRIC" line -- reporting FAILURE even when the model produced valid perf-CSV results. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>

…edential.json (#168) * feat(auth): reuse existing docker login instead of requiring credential.json Reads ${DOCKER_CONFIG:-~/.docker}/config.json the same way the Docker CLI does, so a machine already authenticated via `docker login` (including an org access token) is not forced to duplicate credentials into credential.json, and blank placeholder credentials never override or break a working login. Also distinguishes insufficient_scope (authorization) from unauthorized (authentication) in base-image pull failures so the error message points at the right fix, and logs in before `docker build --pull` only when there's no existing login to reuse. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(gpu-tools): support non-default ROCm install paths and detect GPUs via PATH Honor $ROCM_PATH (falling back to /opt/rocm) instead of hardcoding /opt/rocm in amd_smi_utils.py, rocm_smi_utils.py, and gpu_info_profiler.py, and detect nvidia-smi/rocm-smi/amd-smi via `command -v` in gpu_info_pre.sh instead of a fixed binary path, so detection works when ROCm is installed elsewhere or GPU tools are only on PATH. Also make the rpd tracer's LD_LIBRARY_PATH ROCm-path-aware, and fall back to saving the raw trace.rpd when rpd2tracing.py fails instead of losing the trace. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(gpu-tools): guard rocminfo call and detect nvidia-smi/rocm-smi via PATH rocminfo may be absent even when rocm-smi/amd-smi are present; call it only when available instead of failing the pre-script. Also prefer PATH lookups over hardcoded /usr/bin and $ROCM_PATH/bin paths when detecting GPU vendor. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(auth): name the actual registry in pull-denied hints explain_registry_denial() always suggested Docker Hub credentials even when the failing image referenced another registry (e.g. ghcr.io). Extract the registry host from the image reference and tailor the docker login / credential.json suggestions to it, falling back to the existing Docker Hub guidance when the image has no registry host. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>

) * feat(slurm): allow opting out of the --gpus-per-node sbatch directive Clusters that do not advertise GPU GRES reject any job script carrying --gpus-per-node, so the generated sbatch fails before launch. Add slurm.skip_gpus_directive (default false) to omit the directive and rely on exclusive/nproc_per_node instead. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(slurm): raise the madengine availability probe timeout The pre-submission check ran `madengine --version` with a 5s timeout, which a cold interpreter start off shared/NFS storage exceeds, aborting submission on a perfectly healthy environment. Raise it so the probe only catches a hang. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(run): cap the informational rocm-libs package query The node-info step shelled out to the host package manager with no time limit. On a node where yum wants to import a repo GPG key the command waits on a prompt that never arrives, so the whole multi-node run hangs before the workload starts. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(slurm): inherit the submitter's PATH in the sbatch job A batch job is not guaranteed to inherit the submitter's PATH: a site can default sbatch to --export=NONE, and the module loads in the job body can rewrite it. The pre-submission check then passes on the login node while the compute node aborts with "madengine not found in PATH". Render the per-user bin directory and the directory the madengine console script was resolved from at submission time into the generated script, so the job puts the same interpreter back on PATH instead of relying on inheritance. * fix(slurm): match nfs4 in the shared-filesystem probe The single-node workspace probe matched \bnfs\b only, but df -T reports nfs4 on most modern NFS mounts. A shared submission directory was therefore classified as node-local and the job copied the whole project into /tmp instead of using the shared path. Match \bnfs[0-9]*\b so nfs, nfs3 and nfs4 are all recognized. The rendered job script now also has coverage for the --gpus-per-node opt-out it grew earlier in this batch: skip_gpus_directive shipped without tests, so nothing failed if the directive crept back into the template. Both states of the flag are asserted against the rendered script. * fix(slurm): read the filesystem type, not the whole df line The shared-filesystem probe grepped the entire `df -T` output line, which carries the mount point as well as the type. A local disk mounted at a path such as /mnt/nfs-scratch therefore matched, the submission directory was classified as shared, and the single-node job worked out of storage the other side of the run could not see. Read the type column alone via `df --output=fstype` and anchor the pattern to it. The option is GNU coreutils 8.21 and up, so an awk fallback over `df -T` covers older systems. beegfs and panfs join the list of shared types while the pattern is being rewritten; both are common enough on HPC sites to be worth recognizing. --------- Co-authored-by: Mikhail Kuznetsov <mkuznets@ruby-slurmlogin01.rckg.g03.cpe.ice.amd.com> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Stephen Shao <yu.shao@amd.com>

…grams (#161) * docs(readme): rewrite README as concise landing page with Mermaid figures Trim root README from 707 to 258 lines by moving deep reference material (profiling tables, extended config/usage recipes, tips) into docs/ and linking out. Replace the ASCII architecture block and the stale, unreferenced docs/img PNGs with accurate inline Mermaid figures for the layered architecture, build->run->report pipeline, and deployment-target inference. Move the parallelism/infrastructure capability matrices into docs/launchers.md and add Mermaid workflow diagrams to docs/deployment.md and docs/README.md. Co-Authored-By: Claude <noreply@anthropic.com> * docs: fix CLI flag names and defaults across README, cli-reference, usage Corrects --csv-file to the real --csv-file-path/--file flags, fills in missing `database` command flags (--unique-key/-k, --batch-size, --no-upsert, --no-index, --dry-run, MONGO_AUTH_SOURCE/MONGO_TIMEOUT_MS), fixes wrong `run --output`/`--tools-config` defaults, fixes broken space-separated --tags syntax, and updates the stale version badge. * docs(configuration): remove fabricated keys, fix defaults and schemas Removes fabricated timeout_multiplier/service_account keys and the vllm.tensor_parallel_size example (never read; real value derives from distributed.nproc_per_node). Fixes SLURM gpus_per_node default (1 -> 8), docker_env_vars.MAD_ROCM_PATH -> ROCM_PATH, the Data Provider schema (fabricated data_sources wrapper -> real flat shape), and credential key names (AMD_GITHUB -> PUBLIC_GITHUB_ROCM_KEY, uppercase USERNAME/PASSWORD). * docs(deployment): fix config priority order and stale example references Corrects the backwards/incomplete K8s "Configuration Priority" list to match config_loader.py's real 5-layer merge order (including the previously-missing AMD multi-GPU vendor layer), and removes nonexistent SLURM fields (mem, mail_user, mail_type). In examples/k8s-configs and examples/slurm-configs READMEs: fixes ~35 stale/renamed file paths, removes dead Primus example references, marks gpu_vendor/guest_os/deploy as non-required with real defaults, removes the invalid "RHEL" guest_os value, documents results_pvc and host_ipc as non-configurable (host_ipc previously told users to set a key that is always silently overridden), adds missing results_storage_size / data_storage_size / allow_privileged_profiling fields, and documents the SLURM node-health preflight keys (enable_node_check, auto_cleanup_nodes, allow_submit_without_clean_nodes, verbose_node_check). * docs(launchers): fix launcher value, dead config keys, and wrong examples Fixes "megatron" -> "megatron-lm" launcher value, removes the nonexistent --config flag in favor of --additional-context-file, clarifies the dead distributed.master_port key (SLURM reads distributed.port; K8s reads a separate top-level launcher.master_port object), corrects the 5-node auto-split table row (1/3, not 2/2), fixes the vLLM SLURM multi-node description (data-parallel, not TP+PP with Ray), and removes broken Primus example links. * docs(profiling): fix tool flags, env var names, and default config example Removes the nonexistent --tools therock_check flag, fixes the "Default Tool Configuration" example (rocprof command, gpu_info_power_profiler env vars using bare names instead of the real POWER_/VRAM_-prefixed names) across all Multi-GPU and sampling-rate examples, and adds the undocumented tool names (rocprof_hip_only, rocprof_sys, rocprofv3, rocprofv3_agent, rocprofv3_agent_counter, hipblaslt_trace, instruction_mix.txt). * docs: fix batch-build manifest example and stale install/contributing refs Notes that deployment_config in build_manifest.json is only written for non-local deployments (per _save_deployment_config in build_orchestrator.py), and adds the always-present context/ credentials_required keys to the example. Removes a duplicated `madengine --version` line in installation.md and fixes a stale test path in contributing.md (tests/test_cli.py -> tests/unit/test_cli.py). * docs(database): rewrite README to describe the shipped mongodb module The README described the module as "Not yet implemented" and documented a fictional future API (mongodb_client.py/MongoDBClient, local_storage.py/LocalStorage, api.py/ingest_results()), even though mongodb.py is fully implemented and wired into the `database` CLI command. Rewrites the README around the real classes (MongoDBConfig, UploadOptions, UploadResult, DocumentLoader/JSONLoader/CSVLoader, DocumentTransformer, MongoDBUploader, upload_file_to_mongodb) and adds a CLI-flag-to-API-param mapping table. * docs: fix execution and reporting README signature and API mismatches execution/README.md: fixes build_all_models's models_list -> models param, run_container's fabricated model_docker/gpu_ids params -> real docker_image string param, wrong status value casing/set (successful/failed/timeout -> SUCCESS/FAILURE/SKIPPED), wrong result key (duration -> test_duration), and documents the previously-missing dockerfile_utils.py and container_runner_helpers.py files. reporting/README.md: removes the fabricated "Legacy Reporting Tools" section claiming csv_to_html.py/csv_to_email.py live in a nonexistent tools/ directory and are unused by the modern CLI (they live in reporting/ and back `report to-html`/`report to-email`), fixes the update_perf_csv()/flatten_tags() example signatures, documents the perf_entry.csv/.json side effect, and adds a missing entry for update_perf_super.py. * docs: correct launcher names, docker-login env vars, and config key references - Fix remaining megatron -> megatron-lm launcher references in README, usage, and configuration docs. - Document DOCKER_CONFIG and MAD_SKIP_DOCKER_LOGIN, and clarify that MAD_CONTAINER_IMAGE is an --additional-context key, not an env var (cli-reference, configuration, usage). - Add missing Kubernetes and SLURM additional_context keys (cluster/scheduling, storage, node health/results) with pointers to the example READMEs for full reference. - Correct SGLang Disaggregated minimum node counts and split formula to reflect the SLURM co-located-proxy layout vs. Kubernetes' dedicated proxy requirement. - Replace stale hardcoded version/date footer with a pointer to `madengine --version` and CHANGELOG.md. --------- Co-authored-by: Claude <noreply@anthropic.com>

Proposes capturing the pushed image digest at build time (always on) and gating enforcement of digest-pinned pulls behind an opt-in --require-pinned-image flag, addressing a run that pulled a different image than the one the build pushed due to a mutable-tag race.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…ment helpers Add madengine/core/image_digest.py with four pure helpers: - parse_push_digest: extract sha256 digest from `docker push` output - parse_repo_digest: extract digest from a repo@sha256:... reference - build_pinned_reference: build repo@sha256:... , stripping any tag/digest - resolve_pinned_image: pass through, pin, or raise ConfigurationError Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

push_image() now captures the push output and records the resulting sha256 digest in self.pushed_digests, falling back to `docker image inspect --format '{{index .RepoDigests 0}}'` when the registry does not print a digest line. Best-effort: a missing digest is noted at dim level and never fails the build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Both push call sites (single-arch and per-GPU-arch) copy the digest recorded by push_image into build_info["image_digest"]. build_info is serialized wholesale into build_manifest.json, so the key is purely additive for existing manifest consumers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

The CLI flag and the require_pinned_image additional-context key are equivalent entry points. The key is also persisted into manifest["context"] so the nested `madengine run` that SLURM job scripts execute on each compute node inherits the setting. Two pre-existing tests built args as a bare MagicMock and asserted exact-equality on additional_context; auto-vivified attributes are truthy, so the new flag leaked in. Pin the attribute in those mocks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

The resolve_pinned_image call sits outside the pull try/except so a missing digest aborts rather than falling back to the local image tag. The container runs the same pinned reference that was pulled. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

The pod spec image field resolves through resolve_pinned_image, so a moved tag surfaces as an ImagePullBackOff rather than a silent wrong-image run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

slurm_multi runs the model's own script on the head node with no nested `madengine run` on the compute nodes, so enforcement happens here. Pinning DOCKER_IMAGE_NAME covers both the parallel `srun docker pull` (which interpolates it) and the `docker run` inside the model script. prepare()'s launcher peek wrapped the whole slurm_multi dispatch in a bare `except Exception: pass`, which would have swallowed the enforcement error and generated an unpinned script instead. Re-raise ConfigurationError so deliberate aborts propagate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@sha256

Characterization tests: _docker_image_ref_for_log_naming already strips @sha256:..., so pinned references produce the same log/tar filenames as tags. Locks that in against future refactors. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

The build-on-compute-node path writes built_images entries carrying both a truthy local_image and a registry reference in docker_image. That branch runs before the registry branch in run_models_from_manifest, so --require-pinned-image was silently a no-op for those manifests -- the exact bypass the flag exists to prevent, and contrary to the documented fail-fast behaviour. Resolve the pin in the local_image branch too. resolve_pinned_image now passes through references that are already digest-pinned, so an explicitly pinned MAD_CONTAINER_IMAGE is accepted rather than rejected for lacking a manifest digest. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Copilot

Pull request overview

This PR adds end-to-end support for recording pushed registry image digests at build time and (optionally) enforcing digest-pinned pulls at run time across local Docker, Kubernetes, and SLURM. It also includes related SLURM portability hardening, improved registry-auth ergonomics, and extensive tests/docs to lock in the new behavior.

Changes:

Record image_digest for pushed images during build and introduce shared helpers to resolve repo@sha256:... references.
Add --require-pinned-image / require_pinned_image enforcement to prevent tag-move wrong-image runs (local, K8s, SLURM, including nested SLURM runs via manifest context).
Improve SLURM job script portability and registry-auth handling; add/extend unit+integration coverage and update documentation.

Reviewed changes

Copilot reviewed 45 out of 47 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
tests/unit/test_slurm_multi.py	Validates slurm_multi script pinning behavior and failure when digest is missing under enforcement.
tests/unit/test_slurm_job_template.py	Adds unit tests locking SLURM job template PATH/fs-probe/GPU-directive behavior.
tests/unit/test_orchestration.py	Tests require-pinned flag/context propagation and manifest context persistence; ensures local-image manifest keeps needed fields.
tests/unit/test_k8s.py	Tests K8s template context uses pinned image when enforcement is enabled.
tests/unit/test_image_digest.py	Adds unit tests for digest parsing and pinned reference construction/enforcement.
tests/unit/test_execution.py	Ensures pinned refs don’t destabilize log naming behavior.
tests/unit/test_docker_builder.py	Tests digest capture from push/inspect and manifest propagation.
tests/unit/test_container_runner.py	Tests local runner enforcement (pull by digest, fail fast without digest, inherit via manifest context).
tests/unit/test_auth.py	Adds coverage for ambient docker auth detection and improved login behavior.
tests/integration/test_orchestrator_workflows.py	Pins MagicMock flag behavior for new CLI arg in integration tests.
src/madengine/scripts/common/tools/rocm_smi_utils.py	Makes ROCm path configurable via `ROCM_PATH`.
src/madengine/scripts/common/tools/gpu_info_profiler.py	Improves GPU tool detection and ROCm path handling; uses PATH checks.
src/madengine/scripts/common/tools/amd_smi_utils.py	Makes AMD SMI path configurable via `ROCM_PATH`.
src/madengine/scripts/common/tools.json	Updates RPD tool invocation to use ROCm path in LD_LIBRARY_PATH inline.
src/madengine/scripts/common/pre_scripts/gpu_info_pre.sh	Switches GPU detection to PATH-based checks and tolerates missing rocminfo.
src/madengine/scripts/common/post_scripts/trace.sh	Makes trace conversion best-effort; preserves raw output on conversion failure.
src/madengine/reporting/README.md	Updates API docs/examples to match current reporting helpers/arguments.
src/madengine/orchestration/run_orchestrator.py	Wires CLI flag into additional_context; persists require_pinned_image into manifest context; preserves `multiple_results`.
src/madengine/orchestration/build_orchestrator.py	Allows relying on ambient docker auth for public registries during build-on-compute flows.
src/madengine/execution/README.md	Updates execution docs/API snippets to current signatures and components.
src/madengine/execution/docker_builder.py	Adds digest capture on push, base-registry auth reuse, and actionable registry denial hints.
src/madengine/execution/container_runner.py	Enforces pinned references for registry pulls and local-image entries (including build-on-compute-node manifests).
src/madengine/deployment/templates/slurm/job.sh.j2	Adds PATH restoration, fixes shared-fs probe, and makes GPU directive optional.
src/madengine/deployment/slurm.py	Propagates pinned enforcement into slurm_multi script; adds submission bin dir and skip GPU directive support.
src/madengine/deployment/k8s_template_context.py	Pins pod image reference when enforcement is enabled.
src/madengine/database/README.md	Updates database layer docs to reflect current MongoDB ingestion implementation.
src/madengine/core/image_digest.py	New digest parsing + pinned reference/enforcement helpers.
src/madengine/core/console.py	Ensures captured output is emitted on failures for better diagnostics.
src/madengine/core/auth.py	Adds ambient docker auth detection, better login precedence, and denial explanation helper.
src/madengine/cli/commands/run.py	Adds `--require-pinned-image` CLI flag and plumbs it into orchestrator args.
README.md	Refreshes top-level README structure, examples, and updated references.
examples/slurm-configs/README.md	Updates example paths/names and documents additional SLURM knobs.
examples/k8s-configs/README.md	Updates example paths/names and clarifies inference/behavior around keys.
docs/usage.md	Updates CLI examples and option names to current interface.
docs/superpowers/specs/2026-08-27-pinned-image-digest-design.md	Adds design doc describing digest pinning rationale and implementation plan.
docs/README.md	Adds/updates architecture diagram and launcher naming references.
docs/profiling.md	Updates tool preset docs and environment variable names.
docs/launchers.md	Updates launcher naming/details and adds capability matrices.
docs/installation.md	Minor cleanup.
docs/deployment.md	Updates diagrams and clarifies precedence/target inference.
docs/contributing.md	Fixes test path in examples.
docs/configuration.md	Documents pinned-image behavior and registry-auth precedence; updates various config details.
docs/cli-reference.md	Documents new flag and updated option names/defaults.
docs/batch-build.md	Updates manifest structure documentation.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

                elif build_info.get("registry_image"):
-                    # Registry image: Pull from registry
+                    # Registry image: Pull from registry. Under
+                    # require_pinned_image this resolves to repo@sha256:... and
+                    # raises (outside the pull try/except, so there is no tag
+                    # fallback) when the manifest recorded no digest.


+        # Under require_pinned_image the pod pulls repo@sha256:... so a moved tag
+        # surfaces as an ImagePullBackOff rather than a silent wrong-image run.
+        resolved_image = resolve_pinned_image(
+            image_info["registry_image"],
+            image_info.get("image_digest"),


coketaste and others added 16 commits August 11, 2026 21:36

docs: add implementation plan for pinned image digest enforcement

d9c4ad3

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

feat(k8s): pin pod image to manifest digest when required

3e8f1a5

The pod spec image field resolves through resolve_pinned_image, so a moved tag surfaces as an ImagePullBackOff rather than a silent wrong-image run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

docs: document --require-pinned-image and image digest capture

102c216

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

coketaste requested a review from gargrahul as a code owner August 28, 2026 02:56

Copilot AI lite review requested due to automatic review settings August 28, 2026 02:56

coketaste requested review from Cemberk, Rohan138 and leconcio as code owners August 28, 2026 02:56

Copilot started reviewing on behalf of coketaste August 28, 2026 02:57 View session

coketaste self-assigned this Aug 28, 2026

Copilot AI reviewed Aug 28, 2026

View reviewed changes

Navigation Menu

Uh oh!

feat: enforce pinned image digest for madengine run - #178

feat: enforce pinned image digest for madengine run#178
coketaste wants to merge 16 commits into
mainfrom
coketaste/pulled-image-sha

coketaste commented Aug 28, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

coketaste commented Aug 28, 2026

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants