Skip to content

fix(run_orchestrator): preserve multiple_results in MAD_CONTAINER_IMAGE synthetic manifest - #166

Merged
coketaste merged 1 commit into
developfrom
coketaste/fix-local-image-multiple-results
Aug 12, 2026
Merged

fix(run_orchestrator): preserve multiple_results in MAD_CONTAINER_IMAGE synthetic manifest#166
coketaste merged 1 commit into
developfrom
coketaste/fix-local-image-multiple-results

Conversation

@coketaste

@coketaste coketaste commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • _create_manifest_from_local_image() builds a synthetic manifest for MAD_CONTAINER_IMAGE (local-image) runs, but the built_models entry it constructs omitted the multiple_results field
    from models.json.
  • Without it, ContainerRunner never sets MAD_OUTPUT_CSV, never copies the perf CSV out of the container, and falls back to regex-scraping the run log for a performance: NUMBER METRIC
    line — reporting FAILURE (no performance metrics) even when the model produced a fully valid perf CSV.
  • Discovered while debugging a MAD Uber-storefront run that completed a full correctness + perf sweep successfully but was marked FAILURE in perf.csv/perf_super.json solely because it
    ran via local-image mode.

Fix

  • run_orchestrator.py: add "multiple_results": model.get("multiple_results", "") to the synthetic built_models entry, matching how every other models.json field is already carried
    through.

Test plan

  • Added TestCreateManifestFromLocalImage in tests/unit/test_orchestration.py:
    • test_multiple_results_field_is_preserved — a model with multiple_results set retains it in the generated manifest.
    • test_multiple_results_defaults_to_empty_string — a model without it still gets the key defaulted to "", matching existing field-default conventions.
  • pytest tests/unit tests/integration/test_orchestrator_workflows.py — 562 passed.

@coketaste @claude
fix(run_orchestrator): preserve multiple_results in local-image manifest
3c96bde 
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>
@coketaste coketaste self-assigned this Aug 10, 2026
Copilot AI lite review requested due to automatic review settings August 10, 2026 22:37
Copilot AI reviewed Aug 10, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes local-image (MAD_CONTAINER_IMAGE) execution by ensuring the synthetic build_manifest.json produced by RunOrchestrator._create_manifest_from_local_image() preserves the multiple_results field from models.json, which ContainerRunner relies on to set MAD_OUTPUT_CSV and correctly export perf CSV results.

Changes:

  • Preserve multiple_results when constructing synthetic built_models entries for local-image runs.
  • Add unit tests verifying multiple_results is preserved when provided and defaults to "" when absent.

Reviewed changes

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

File Description
src/madengine/orchestration/run_orchestrator.py Adds multiple_results to the synthetic manifest’s built_models entries so local-image runs follow the same manifest schema as built-image runs.
tests/unit/test_orchestration.py Adds unit coverage to prevent regressions for multiple_results propagation and defaulting behavior.

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

@coketaste
coketaste merged commit 4b739b6 into develop Aug 12, 2026
1 check passed
coketaste added a commit that referenced this pull request Sep 1, 2026
@coketaste @claude
@mkuznet1 Mikhail Kuznetsov @cursoragent
feat: enforce pinned image digest for madengine run (#178)
6c966a8 
* 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>
coketaste added a commit that referenced this pull request Sep 1, 2026
@coketaste @claude
revert: "feat: enforce pinned image digest for madengine run (#178)"
50bef03 
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort, defaults to Lite
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@gargrahul gargrahul Awaiting requested review from gargrahul gargrahul is a code owner
@Rohan138 Rohan138 Awaiting requested review from Rohan138 Rohan138 is a code owner
@leconcio leconcio Awaiting requested review from leconcio leconcio is a code owner
@Cemberk Cemberk Awaiting requested review from Cemberk Cemberk is a code owner

Assignees

@coketaste coketaste

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@coketaste