feat(build): honor per-model docker_build_arg from models.json

[MIR-AMD]

Problem

Docker build args can only reach a build through --additional-context / --additional-context-file. That dictionary is global to the invocation, so:

• a model cannot declare its own build-time pins in-repo; every caller has to repeat them on the command line, and nothing in the repo records what the model was validated against
• two models that need different values for the same arg cannot be built in one invocation, because the single context dict applies to all of them

DockerBuilder.get_build_arg() reads only context.ctx["docker_build_arg"], and build_image() never consults model_info, so a docker_build_arg block in a models.json entry is silently ignored today.

Change

build_image() now merges a model card's docker_build_arg into that model's own build args.

Precedence is context-wins, matching what _pick_context_over_model() already does for run-time keys (log_error_patterns, node counts, and friends): a key present in --additional-context overrides the card, so an operator can still redirect a build without editing models.json. Credential args and the multi-arch additional_build_args also win, so a card entry cannot shadow MAD_SYSTEM_GPU_ARCHITECTURE during a --target-archs build.

Also stops indexing ctx["docker_build_arg"] directly in get_build_arg(). It survives today only because Context.__init__ always seeds that key; a non-empty run_build_arg with a context lacking it raised KeyError.

No schema change is needed — unknown keys already pass through DiscoverModels untouched.

{
  "name": "my_model",
  "dockerfile": "docker/my_model",
  "tags": ["my_model"],
  "docker_build_arg": {
    "VLLM_REPO": "https://github.com/myorg/vllm.git",
    "VLLM_REF": "d723eb305eb78d1bda0ed357b2b54cc29487221f"
  }
}

Motivation

A MAD recipe pins its vLLM fork and commit in a per-model Dockerfile. Reproducing that image today means either editing the Dockerfile or remembering two --build-arg values on every rebuild. Neither survives an invocation the recipe author does not control (CI, another team). With this change the pins live next to the model that needs them.

Test plan

New tests in tests/integration/test_docker_integration.py:

• a card's docker_build_arg reaches the docker build command
• --additional-context overrides the card for the same key
• a card entry cannot shadow the multi-arch MAD_SYSTEM_GPU_ARCHITECTURE

Verified end to end with real docker build runs, two models in one invocation with different pins and no CLI flags:

ci-probe_one -> --build-arg VLLM_REF=d723eb305eb78d1bda0ed357b2b54cc29487221f
ci-probe_two -> --build-arg VLLM_REF=vllm_2p2d_wide-ep_write_shikpate_test_06_29_customer

Both values confirmed baked into the resulting images, and re-running with -f ctx.json collapsed both to the context value.

Full suite: 741 passed, 1 skipped (the skip requires a non-AMD GPU).

[Copilot]

Pull request overview

This PR updates the Docker build path so each model can define its own build-time docker_build_arg block in models.json, while still allowing invocation-wide --additional-context and multi-arch/credential build args to take precedence. This makes per-model build pins reproducible and avoids forcing callers to repeat model-specific build arguments on every run.

Changes:

• Merge per-model docker_build_arg from model_info into the build args used by DockerBuilder.build_image(), with explicit precedence rules (context + caller-supplied args win).
• Make DockerBuilder.get_build_arg() resilient to contexts that don’t include a docker_build_arg key.
• Add integration tests and documentation describing per-model docker_build_arg support.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/madengine/execution/docker_builder.py	Merges model-card build args into per-build arguments and avoids KeyError when context lacks docker_build_arg.
tests/integration/test_docker_integration.py	Adds integration tests verifying model-card build args are applied and that context/multi-arch args override them.
docs/configuration.md	Documents docker_build_arg support within models.json entries and precedence with --additional-context.

---

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

[MIR-AMD]

Thanks for taking a look! Both points are addressed.

Full test suite — no regressions. Ran unit + integration on this branch and on unmodified develop (4f281ef) with the same interpreter:

Branch	Result
develop (baseline)	738 passed, 1 skipped
this PR	742 passed, 1 skipped

The delta is exactly the 4 tests this PR adds; every pre-existing test still passes. (The one skip is the pre-existing test_gpu_management.py case that requires a non-AMD GPU.) The e2e suite was also run, but its failures are Docker Hub 429 rate limits on public base images and are identical on both branches, so they are environmental rather than related to this change.

Copilot comment. Fair catch — a docker_build_arg that is not an object (e.g. a bare string) would previously have surfaced as an AttributeError from inside the build path. build_image() now validates the type up front and raises a message naming the model and the offending type, and there is a test covering it (e85cade).

The behavior itself is unchanged: context and multi-arch/credential args still win over the model card, matching _pick_context_over_model() for run-time keys.

[Copilot]

Pull request overview

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

Suppressed comments (1)

src/madengine/execution/docker_builder.py:255

• card_build_arg = model_info.get("docker_build_arg") or {} treats falsy non-dict values (e.g., [], 0, false) as if the key were absent, so malformed docker_build_arg can slip through without raising the intended RuntimeError. Consider explicitly checking for key presence / None and raising for any non-dict type.

        # Per-model build args declared in the model card (models.json). Lets a model
        # pin build-time sources (e.g. VLLM_REPO/VLLM_REF) in-repo instead of requiring
        # every caller to pass --additional-context. Context and multi-arch/cred args
        # win on conflict, matching _pick_context_over_model() for run-time keys.
        card_build_arg = model_info.get("docker_build_arg") or {}
        if not isinstance(card_build_arg, dict):
            raise RuntimeError(
                f"docker_build_arg for model {model_info['name']} must be a JSON object "
                f"mapping build-arg names to values, got {type(card_build_arg).__name__}"
            )

[MIR-AMD]

Testing summary

Every check below was run on this branch and on unmodified develop (4f281ef), so any change to existing behavior would show up as a difference between the two. The diff is 3 files, +183/-2.

1. Unit + integration suites — no regressions

Environment	develop (4f281ef)	this PR (e85cade)
Dev box	738 passed, 1 skipped	742 passed, 1 skipped
MI300X compute node	725 passed, 13 failed, 1 skipped	729 passed, 13 failed, 1 skipped

• The passed-count delta is exactly the 4 tests this PR adds. No pre-existing test changed result on either machine.
• The 13 failures on the compute node occur on both branches, and the failure sets are identical (verified with comm): 11 are TestGetGpuRenderDNodesIntegration (GPU / renderD discovery inside a Slurm job step), 2 are file-not-found expectations. None involve docker_builder or build args, and all 13 pass on the dev box.
• The 1 skip is the pre-existing test that requires a non-AMD GPU.

Tests added in tests/integration/test_docker_integration.py:

• test_build_image_uses_model_card_docker_build_arg
• test_context_docker_build_arg_overrides_model_card
• test_model_card_docker_build_arg_does_not_shadow_multi_arch
• test_malformed_model_card_docker_build_arg_raises

2. Real docker build verification on hardware

The test suite mocks Console.sh, so no docker build ever executes there. The following ran real builds on an MI300X node (Ubuntu 22.04, docker 29.0.2), each paired with the develop control:

Scenario	Result
Card pins, zero CLI flags	both args land on the docker build command and are baked into the image environment
Same, read from inside a running container	values match the card, as does a file written at build time from the ARG
Control: identical card on develop	image still carries the Dockerfile defaults — the card is ignored without this change
--additional-context override	the overridden key wins, the other card key is retained (per-key merge)
--additional-context-file override	same result via file
Two models, one invocation, different pins	each image gets its own values, no cross-contamination
Card declares an arg no ARG consumes	build still succeeds; the extra arg is passed and harmlessly ignored
Malformed docker_build_arg (a string)	targeted RuntimeError; no AttributeError in any log
Pin actually consumed	a Dockerfile cloning from the card's repo at the card's ref fetched and checked out the exact pinned 40-character SHA
Real multi-arch build (--target-archs gfx942)	a card deliberately setting MAD_SYSTEM_GPU_ARCHITECTURE=CARD_TRIES_TO_WIN did not shadow it: the image reports gfx942, and the card's other arg still applied
An existing model recipe with its real, unmodified Dockerfile	both pins on the build command with no build-arg flags on the CLI; the develop control passed neither

Existing behavior explicitly confirmed unchanged: --additional-context and --additional-context-file precedence, multi-arch architecture injection, credential build args, and models that declare no docker_build_arg at all.

3. Copilot review comment — addressed

Copilot's one comment, on docker_builder.py:255, was a fair catch: a non-object docker_build_arg would have surfaced as a bare AttributeError from inside the build path, hard to trace back to the model card.

Fixed in e85cade — build_image() validates before iterating:

card_build_arg = model_info.get("docker_build_arg") or {}
if not isinstance(card_build_arg, dict):
    raise RuntimeError(
        f"docker_build_arg for model {model_info['name']} must be a JSON object "
        f"mapping build-arg names to values, got {type(card_build_arg).__name__}"
    )

Covered by test_malformed_model_card_docker_build_arg_raises, and confirmed on hardware: a card carrying "docker_build_arg": "VLLM_REF=..." now fails with docker_build_arg for model <name> must be a JSON object mapping build-arg names to values, got str, with no AttributeError anywhere in the logs.

[coketaste]

LGTM