docs: rewrite README as concise landing page and add architecture diagrams by coketaste · Pull Request #161 · ROCm/madengine

coketaste · 2026-07-16T16:12:22Z

Summary

Rewrite README.md as a concise landing page, replacing the long-form content with a tighter overview (734 → much shorter, ~460 net removed)
Add Mermaid architecture diagrams to docs/README.md (CLI → orchestration → execution/deployment → launcher flow) and docs/deployment.md (build/deploy workflow and target-inference
decision tree), replacing ASCII-art diagrams
Add capability reference tables to docs/launchers.md: per-launcher parallelism support (TP/PP/DP/CP/FSDP/EP) and per-target infrastructure features (local/K8s/SLURM)
Remove now-unused docs/img/architecture_overview.png and docs/img/distributed_workflow.png (superseded by inline Mermaid diagrams)

Test plan

Render README.md, docs/README.md, docs/deployment.md, and docs/launchers.md on GitHub (or a Markdown/Mermaid previewer) to confirm diagrams and tables render correctly
Verify no remaining links reference the deleted PNG files

…ures 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>

Copilot

Pull request overview

This PR refreshes madengine’s documentation set to better reflect the current CLI/config surface area and to provide clearer architecture/workflow guidance (including Mermaid diagrams and updated examples across local/K8s/SLURM usage).

Changes:

Rewrites and streamlines the top-level README.md, adding Mermaid architecture/workflow diagrams and tightening links to the docs.
Updates multiple docs to match current CLI flags, config keys, and JSON manifest/config shapes (database/reporting/profiling/deployment/launchers).
Renames/standardizes a set of example config fields (notably distributed launcher names and port fields) across K8s/SLURM examples.

Reviewed changes

Copilot reviewed 33 out of 35 changed files in this pull request and generated 13 comments.

Show a summary per file

File	Description
`README.md`	Replaces long-form README content with a tighter landing page + Mermaid diagrams and refreshed examples/links.
`src/madengine/reporting/README.md`	Updates reporting module usage/examples and clarifies which helpers are used by the modern CLI.
`src/madengine/execution/README.md`	Updates execution-layer examples/notes to reflect current APIs and helper modules.
`src/madengine/database/README.md`	Re-documents MongoDB ingestion as an active module, including CLI mapping and API usage.
`docs/README.md`	Adds inline Mermaid architecture diagram matching the main README.
`docs/deployment.md`	Replaces ASCII workflow with Mermaid and clarifies target inference + precedence.
`docs/usage.md`	Refreshes end-to-end usage examples and aligns CLI flags and manifest example shape.
`docs/cli-reference.md`	Updates CLI reference defaults/flags and expands `database` command documentation.
`docs/configuration.md`	Updates configuration guidance (ROCm path semantics, timeouts, credentials examples, etc.).
`docs/launchers.md`	Updates launcher docs, adds capability matrices, and refreshes configuration examples.
`docs/profiling.md`	Updates profiling preset references and example env var naming/documentation.
`docs/installation.md`	Minor cleanup (duplicate command line removal).
`docs/contributing.md`	Fixes test path in contributor instructions.
`docs/batch-build.md`	Updates manifest key documentation and adds clarifying note about `deployment_config`.
`examples/slurm-configs/README.md`	Updates example references/paths and documents node health preflight flags.
`examples/slurm-configs/minimal/megatron-lm-minimal.json`	Updates launcher name to `megatron-lm`.
`examples/slurm-configs/basic/09-megatron-lm-multi-node.json`	Updates launcher name and uses `distributed.port`.
`examples/k8s-configs/README.md`	Updates example file layout/paths and clarifies K8s-specific config semantics.
`examples/k8s-configs/minimal/megatron-lm-minimal.json`	Updates launcher name to `megatron-lm`.
`examples/k8s-configs/minimal/megatron-lm-exclude-node.json`	Updates launcher name to `megatron-lm`.
`examples/k8s-configs/minimal/megatron-lm-optimized.json`	Updates launcher name and uses `distributed.port`.
`examples/k8s-configs/basic/02-torchrun-single-node-multi-gpu.json`	Uses `distributed.port` in example config.
`examples/k8s-configs/basic/02-torchrun-single-node-multi-gpu-tools.json`	Uses `distributed.port` in example config.
`examples/k8s-configs/basic/03-torchrun-multi-node-basic.json`	Uses `distributed.port` in example config.
`examples/k8s-configs/basic/04-torchrun-multi-node-advanced.json`	Uses `distributed.port` in example config.
`examples/k8s-configs/basic/05-torchrun-nvidia-gpu-example.json`	Uses `distributed.port` in example config.
`examples/k8s-configs/basic/06-data-provider-with-pvc.json`	Uses `distributed.port` in example config.
`examples/k8s-configs/basic/megatron-lm-multi-node-basic.json`	Uses `distributed.port` in example config.
`examples/k8s-configs/basic/vllm-multi-node-basic.json`	Uses `distributed.port` in example config.
`examples/k8s-configs/basic/torchtitan-multi-node-basic.json`	Uses `distributed.port` in example config.
`examples/k8s-configs/basic/sglang-multi-node-basic.json`	Uses `distributed.port` in example config.
`examples/k8s-configs/basic/sglang-disagg-multi-node-basic.json`	Uses `distributed.port` in example config.
`examples/k8s-configs/basic/sglang-disagg-custom-split.json`	Uses `distributed.port` in example config.

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

  "distributed": {
    "enabled": true,
    "backend": "nccl",
    "launcher": "torchrun",
    "nnodes": 2,
    "nproc_per_node": 2,
-    "master_port": 29500
+    "port": 29500
  },


    "launcher": "torchrun",
    "nnodes": 4,
    "nproc_per_node": 2,
-    "master_port": 29500
+    "port": 29500
  },


    "launcher": "torchrun",
    "nnodes": 1,
    "nproc_per_node": 2,
-    "master_port": 29500
+    "port": 29500
  },


    "launcher": "torchrun",
    "nnodes": 1,
    "nproc_per_node": 2,
-    "master_port": 29500
+    "port": 29500
  },


    "launcher": "torchrun",
    "nnodes": 1,
    "nproc_per_node": 4,
-    "master_port": 29500
+    "port": 29500
  },


    "launcher": "sglang",
    "nnodes": 2,
    "nproc_per_node": 4,
-    "master_port": 29500
+    "port": 29500
  },


    "launcher": "sglang-disagg",
    "nnodes": 5,
    "nproc_per_node": 8,
-    "master_port": 29500
+    "port": 29500
  },


    "launcher": "sglang-disagg",
    "nnodes": 7,
    "nproc_per_node": 8,
-    "master_port": 29500,
+    "port": 29500,
    "sglang_disagg": {


  "distributed": {
-    "launcher": "megatron",
+    "launcher": "megatron-lm",
    "nnodes": 4,
    "nproc_per_node": 8,
-    "master_port": 29500
+    "port": 29500
  },


  "distributed": {
    "enabled": true,
    "backend": "nccl",
-    "launcher": "megatron",
+    "launcher": "megatron-lm",
    "nnodes": 1,
    "nproc_per_node": 2,
-    "master_port": 29500
+    "port": 29500
  },


Copilot

Pull request overview

Copilot reviewed 47 out of 49 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/madengine/cli/validators.py:291

In the ConfigurationError path you only print the raw invalid value plus suggestions, but you drop the actual exception message (which includes the source/context like "... in additional_context"). This makes it harder to diagnose which field triggered the failure. Consider printing str(exc) (or including the source in your own message), and pass a more specific source string (e.g. additional_context.distributed.launcher vs additional_context.launcher.type) when calling validate_launcher().
src/madengine/deployment/common.py:55
PR title/description focus on documentation changes, but this diff also introduces significant runtime behavior changes (new launcher validation + canonicalization, deployment template logic changes, and new/updated unit tests). Please update the PR description (and test plan) to reflect these non-doc changes so reviewers know to focus on both documentation and behavior.

…sage 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.

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).

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).

…ples 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.

…ample 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).

… 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).

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.

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.

…eferences - 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.

Copilot

Pull request overview

Copilot reviewed 16 out of 18 changed files in this pull request and generated no new comments.

@sha256

* fix(run_orchestrator): preserve multiple_results in local-image manifest (#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> * feat(auth): reuse existing docker login (OAT) instead of requiring credential.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> * fix(slurm): make the generated job script portable across clusters (#163) * 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> * docs: rewrite README as concise landing page and add architecture diagrams (#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> * docs: add design spec for pinned image digest enforcement 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. * docs: add implementation plan for pinned image digest enforcement Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(image-digest): add digest parsing, pinned reference, and enforcement 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> * feat(build): capture pushed image digest during docker push 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> * feat(build): record image_digest in build manifest entries 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> * feat(run): add --require-pinned-image flag and context propagation 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> * feat(run): pin local docker pulls to manifest digest when required 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> * feat(k8s): pin pod image to manifest digest when required 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> * feat(slurm): pin slurm_multi image to manifest digest when required 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> * test(execution): cover log naming for digest-pinned image references 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> * docs: document --require-pinned-image and image digest capture Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(run): enforce pinned digest on build-on-compute-node manifests 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> * fix(execution): derive Docker-legal container name from pinned image refs Container names were built straight from the run image reference, mapping only "/" and ":" to "_". Under --require-pinned-image the run image becomes repo@sha256:..., so the "@" survived into --name and the daemon rejected it: docker: Error response from daemon: Invalid container name (container_***_mad-private@sha256_af99a16c...), only [a-zA-Z0-9][a-zA-Z0-9_.-] are allowed The pull succeeded because a digest reference is a valid image ref; only the name derived from it was invalid. Add container_name_from_image_ref(), which strips the digest, applies the same "/" and ":" mapping as before, and sanitizes any remaining out-of-charset byte so the invariant holds instead of failing at `docker run`. _docker_image_ref_for_log_naming() is deliberately not reused: it collapses CI-style refs to the bare tag, which would rename containers on every existing non-pinned path. Keeping the tag also keeps different tags of one repo distinct. Verified byte-identical output against the previous expression for 60 non-digest refs (bare tags, registry hosts, localhost:5000 ports, CI tags, dotted tags), including the name the e2e suite asserts. Also drops the dead re.sub(".*:", "", ...) at the call site: ":" had already been replaced with "_" on the preceding call, so it never matched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(run): fail pinned pulls instead of falling back to a local tag Under --require-pinned-image a failed registry pull fell back to run_image = image_name. Local tags are mutable too, so that fallback broke the pinned-image guarantee in the failure mode it matters most in: a digest/tag mismatch or an auth error would silently run whatever the local tag happened to point at. The pull failure is now fatal for that model. The surrounding handler already records it as a failed run and continues with the rest of the manifest, so one unpinnable model does not abort the whole run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(build): run docker push without the 60s command timeout push_image() called Console.sh() with the default 60s timeout, so pushing a multi-GB image failed an otherwise successful build with "Console script timeout". build_image() already passes timeout=None for docker build; do the same for the push. Console.sh's timeout was annotated int while five call sites already pass None (which subprocess accepts as "wait indefinitely"), so widen the annotation to Optional[int] to match the behaviour. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: mkuznet1 <Mikhail.Kuznetsov@amd.com> Co-authored-by: Mikhail Kuznetsov <mkuznets@ruby-slurmlogin01.rckg.g03.cpe.ice.amd.com> Co-authored-by: Cursor <cursoragent@cursor.com>

This reverts commit 6c966a8. PR #178 targeted main by mistake; it should have gone to develop. Because the head branch was based on develop while main was five PRs behind, the squash merge pulled unreleased develop work into main along with the feature: #166, #168, #163, #161 and #175. Reverting restores main to ec4de0b exactly. The feature is being re-opened against develop; no revert-of-this-revert is needed, since the squash commit shares no SHAs with develop's history and a later develop -> main merge applies cleanly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

coketaste self-assigned this Jul 16, 2026

coketaste changed the title ~~docs: rewrite README as concise landing page and add architecture diagrams [WIP]~~ docs: rewrite README as concise landing page and add architecture diagrams Aug 19, 2026

coketaste marked this pull request as ready for review August 19, 2026 15:48

coketaste requested review from Rohan138 and gargrahul as code owners August 19, 2026 15:48

Copilot AI lite review requested due to automatic review settings August 19, 2026 15:48

coketaste requested review from Cemberk and leconcio as code owners August 19, 2026 15:48

Copilot started reviewing on behalf of coketaste August 19, 2026 15:48 View session

Copilot AI reviewed Aug 19, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 20, 2026 00:41

Copilot started reviewing on behalf of coketaste August 20, 2026 00:42 View session

Copilot AI reviewed Aug 20, 2026

View reviewed changes

coketaste added 10 commits August 19, 2026 20:02

Merge branch 'develop' into coketaste/docs-rewrite

69d5f2f

Copilot AI review requested due to automatic review settings August 20, 2026 01:04

coketaste force-pushed the coketaste/docs-rewrite branch from 7567594 to cb17652 Compare August 20, 2026 01:04

Copilot started reviewing on behalf of coketaste August 20, 2026 01:05 View session

Copilot AI reviewed Aug 20, 2026

View reviewed changes

coketaste merged commit 4f281ef into develop Aug 20, 2026
1 check passed

coketaste added a commit that referenced this pull request Aug 20, 2026

docs(changelog): add entry for README rewrite (#161)

6bd9387

Copilot AI mentioned this pull request Sep 1, 2026

v2.2.0 release #169

Open

Navigation Menu

Uh oh!

docs: rewrite README as concise landing page and add architecture diagrams - #161

docs: rewrite README as concise landing page and add architecture diagrams#161
coketaste merged 11 commits into
developfrom
coketaste/docs-rewrite

coketaste commented Jul 16, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

coketaste commented Jul 16, 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!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants