honor model-card declarations on the slurm_multi path - #176
Open
Cemberk wants to merge 3 commits into
Open
Conversation
slurm_multi is an escape hatch for topologies the templated launchers cannot express, but it had also become an escape hatch from the model-card contract: fields the templated path honors were silently dropped, so a card that fully described its job still ran wrong. Node count. `distributed.nnodes` never reached `#SBATCH --nodes`, which came only from `slurm.nodes` (default 1). A card declaring a 4-node topology was submitted as a 1-node job. This was invisible in the common workflow, where `salloc -N 4` comes first and the wrapper — run with bash, not sbatch — inherits SLURM_NNODES; it only bites on the sbatch path. Cards worked around it with `"args": "-N 4 -n 4"`, but args go to `bash <model>.slurm`, not to sbatch, so a script that never reads $@ discarded them. Both paths now reconcile the two fields: nnodes sizes the allocation when slurm.nodes was not set explicitly, an explicit slurm.nodes wins a conflict and warns, and resolution recomputes from a saved baseline so the prepare() that deploy() re-runs after preflight is idempotent. Launcher resolution. prepare() picked the path from the model card while _prepare_template_context() read the deployment config, so a card-declared launcher could take one path and emit another path's env block. Both now call one resolver, deployment-config-first — matching how BuildOrchestrator builds the manifest. multiple_results. The slurm_multi collector never read model_info, so the declared filename was dead config and collection worked only when a workload happened to write to a hardcoded path. It is now searched next to the model script (where the wrapper cd's, so `$(pwd)` writes land) before the conventional locations. The CSV is still read directly rather than routed through handle_multiple_results(): a self-managed script has no common_info to merge against and writes the full schema itself, and re-ingesting it would recompute status from performance, flipping a legitimate zero-score FAILURE to SUCCESS. Placeholder images. Cards ship DOCKER_IMAGE_NAME as "<supply-your-image>" to mean "fill this in". The implicit --use-image path accepted any single distinct value, so the marker became the image name and every node failed on `docker pull <supply-your-image>`. Angle-bracketed values are now rejected at submit time with an actionable error. Also guards the per-job perf aggregation against appending cwd/perf.csv to itself, which duplicated every row whenever the source resolved to it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Target inference is convention-over-configuration: it keys purely on the presence of a `slurm` or `k8s` block. But model cards routinely declare only `distributed.launcher: slurm_multi` and no `slurm` block — all 37 slurm_multi entries in ROCm/MAD are shaped that way. Those inferred "local" and were handed to the container runner, so the slurm_multi path was never reached and the model's .slurm script would be run as an ordinary local container workload. slurm_multi drives sbatch/srun directly, so it is a SLURM deployment by construction. Infer it as one when no explicit block says otherwise; an explicit k8s or slurm block still wins, since that is the user's stated intent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Cemberk
requested review from
Rohan138,
coketaste,
gargrahul and
leconcio
as code owners
August 21, 2026 12:06
There was a problem hiding this comment.
Pull request overview
This PR tightens “path parity” between the templated SLURM launcher path and the self-managed slurm_multi escape-hatch by making model-card contracts (launcher selection, node sizing, and results CSV naming) consistently honored across orchestration and deployment.
Changes:
- Infer SLURM deployment when
distributed.launcheris a self-managed SLURM launcher (e.g.,slurm_multi), even without an explicitslurmblock. - Add shared helpers to resolve launcher and reconcile
slurm.nodesvsdistributed.nnodes, and wire them intoSlurmDeployment(plus result CSV discovery forslurm_multiviamultiple_results). - Reject placeholder
DOCKER_IMAGE_NAMEvalues (e.g.,"<supply-your-image>") early with aConfigurationError, and document the contract.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_slurm_multi.py | Adds contract tests for launcher/node resolution and slurm_multi results handling. |
| tests/unit/test_orchestration.py | Adds tests for placeholder image rejection and slurm_multi implying SLURM deployment target. |
| src/madengine/orchestration/run_orchestrator.py | Treats self-managed SLURM launchers as implying slurm target in inference. |
| src/madengine/orchestration/build_orchestrator.py | Rejects placeholder DOCKER_IMAGE_NAME values during implicit image resolution. |
| src/madengine/deployment/slurm.py | Centralizes launcher/node resolution and supports declared multiple_results CSV for slurm_multi. |
| src/madengine/deployment/common.py | Introduces shared helpers: resolve_launcher_from_sources() and resolve_node_count(). |
| docs/launchers.md | Documents the shared model-card contract and placeholder-image rejection behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+660
to
+668
| def test_dispatch_and_env_block_cannot_disagree(self): | ||
| """ | ||
| prepare() used to read the card while _prepare_template_context() read the | ||
| deployment config, so a card-declared launcher could pick one path and emit | ||
| another path's env block. Both now call this, so they agree by construction. | ||
| """ | ||
| for deployment, card in [(None, "vllm"), ("sglang", "vllm"), (None, None)]: | ||
| assert resolve_launcher_from_sources(deployment, card) == \ | ||
| resolve_launcher_from_sources(deployment, card) |
Comment on lines
+170
to
+179
| try: | ||
| requested = int(nnodes) if nnodes is not None else None | ||
| except (TypeError, ValueError): | ||
| return configured_nodes, ( | ||
| f"Ignoring non-numeric distributed.nnodes={nnodes!r}; " | ||
| f"using slurm.nodes={configured_nodes}." | ||
| ) | ||
|
|
||
| if requested is None or requested == configured_nodes: | ||
| return configured_nodes, None |
Cemberk
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A
slurm_multimodel card declares what it needs —distributed.launcher,distributed.nnodes,multiple_results, an image. madengine was dropping five ofthose declarations, starting with
launcheritself, which meant the card neverreached the SLURM deployment at all.
This PR makes madengine honor what the card declares. It fixes five dropped
declarations plus one unrelated perf-aggregation bug found on the way.