You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
A slurm_multi model card declares what it needs — distributed.launcher, distributed.nnodes, multiple_results, an image. madengine was dropping five of
those declarations, starting with launcher itself, which meant the card never
reached 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.
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>
The reason will be displayed to describe this comment to others. Learn more.
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.launcher is a self-managed SLURM launcher (e.g., slurm_multi), even without an explicit slurm block.
Add shared helpers to resolve launcher and reconcile slurm.nodes vs distributed.nnodes, and wire them into SlurmDeployment (plus result CSV discovery for slurm_multi via multiple_results).
Reject placeholder DOCKER_IMAGE_NAME values (e.g., "<supply-your-image>") early with a ConfigurationError, 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.
Both branches appended new test classes to the same two files; the
conflicts were purely positional and both sides are kept.
In slurm.py's prepare() the two intents compose: this branch's
_resolve_nodes()/_resolve_launcher() peek runs inside develop's
ConfigurationError re-raise, so --require-pinned-image still aborts
instead of silently generating an unpinned script.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…al test
Two review findings from Copilot on #176:
resolve_node_count() accepted nnodes values of 0 or -1 (and their string
forms) and propagated them into slurm.nodes, so a typo in a model card
became '#SBATCH --nodes=0' and failed at submit time with a scheduler
error that named nothing useful. Non-positive values now fall back to
the configured slurm.nodes with a note, matching the non-numeric path.
test_dispatch_and_env_block_cannot_disagree compared the function's
result to itself, so it could never fail. Replaced with an explicit
expectation matrix, including a falsy deployment launcher to pin down
that it does not override the model card.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…cit-key provenance
Addresses remaining PR #176 review comments:
- Extract the model-card distributed/slurm merge (previously only run by
_execute_with_prebuilt_image) into BuildOrchestrator._merge_model_config_into_manifest
and call it from the normal Docker-build path too, so a slurm_multi model card's
distributed.launcher reaches deployment_config regardless of build path. Without
this, run --manifest-file inferred "local" for models built without --use-image.
- Record which slurm.* keys were actually explicit (--additional-context or model
card) as deployment_config._explicit_slurm_keys in the manifest, and have
SlurmDeployment prefer that provenance over inferring explicitness from which
keys are present. A manifest's slurm dict is serialized after ConfigLoader
applies preset defaults, so a persisted nodes=1 default was indistinguishable
from a real user/model-card setting and could silently override
distributed.nnodes.
- Expand the slurm.* merge whitelist to include account, qos, modules,
skip_gpus_directive, and results_dir, matching what SlurmDeployment actually
reads and docs/configuration.md documents. Update docs/launchers.md's
model-card contract table to match.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Preserve multiple_results in build-on-compute manifests
src/madengine/deployment/slurm.py:1923
model_info is now the only source for the declared results filename here, but the --build-on-compute manifest builder does not copy multiple_results into each built_models entry. Consequently a slurm_multi card built through that supported path always passes None to _slurm_multi_declared_result_csv and falls back to conventional perf paths, so its declared CSV is ignored. Preserve multiple_results when constructing that manifest.
--build-on-compute wrote its own manifest and returned before
_merge_model_config_into_manifest, so it skipped the shared model-field
and provenance handling: the card's multiple_results was dropped, no
_explicit_slurm_keys was recorded, and because the merge read
self.additional_context (post-ConfigLoader) the preset defaults outranked
the card. Route it through the same helper and apply the same
explicit-key precedence rule when deriving its slurm config.
_prepare_slurm_multi_script promoted account/qos/modules into
deployment_config and then dropped them: unlike job.sh.j2 the wrapper
emitted no #SBATCH --account/--qos and ran no module load, so a card
declaring them was silently ignored on the self-managed path. Emit them,
keeping the directives inside the header block sbatch actually parses.
_load_and_merge_manifest replaced the manifest's slurm block with the
runtime one but left _explicit_slurm_keys describing the old block, so a
build-time explicit nodes marked the runtime default nodes=1 as
deliberate and suppressed distributed.nnodes. Recompute the provenance
from the runtime keys whenever runtime slurm is supplied.
Also stop _merge_model_config_into_manifest from turning a missing
manifest into a BuildError. It runs after _save_build_summary and
_save_deployment_config, which both degrade to a warning when
export_build_manifest() produced nothing; the new step did not, which
failed test_build_then_run_workflow.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This lookup accepts any existing file with the declared name in the persistent model-script directory, but the wrapper always cds there and the filename is not job-scoped. If a run fails before rewriting the CSV, or two jobs for the same model overlap, collection can read stale/another job's metrics and report them as this deployment's result. Remove or namespace the artifact before launch, or require a fresh/job-scoped file before accepting this candidate.
This issue also appears on line 2338 of the same file.
… runs
When a persisted deployment config has a slurm block but no explicit
user/model-card slurm keys, provenance was omitted entirely. On a
later `run --manifest-file`, SlurmDeployment then fell back to
inferring explicitness from the defaulted slurm dict and treated a
preset nodes=1 as deliberate, silently ignoring distributed.nnodes.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
int(nnodes) silently truncated fractional values like 1.9, letting a
malformed model card schedule the wrong node count with no warning.
Reject booleans, non-integral floats, and non-finite values so they
fall back to slurm.nodes with a logged note instead.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
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.