feat(madengine): give the manifest a contract and multi-node runs an honest verdict - #164
Closed
mkuznet1 wants to merge 7 commits into
Closed
feat(madengine): give the manifest a contract and multi-node runs an honest verdict#164mkuznet1 wants to merge 7 commits into
mkuznet1 wants to merge 7 commits into
Conversation
added 7 commits
August 7, 2026 14:02
… truth The manifest was checked for three top-level keys and nothing else, so a typo in a transport variable, a nested env value, or a node count that disagreed with itself surfaced as a failed multi-node job minutes later rather than as a startup error. Separately, the deployment target could be read from two places: a top-level slurm block selected the target while the values that took effect were the ones under deployment_config, so a manifest could be configured and ignored at the same time. Declare the shape in src/madengine/schemas/build_manifest.schema.json and validate it where the manifest is loaded, reporting the first violation with its JSON pointer. Unknown keys stay allowed so a manifest can carry consumer metadata. Two cross-field checks a schema cannot express are included: built_models entries must have a matching built_images entry, and slurm.nodes must equal distributed.nnodes. A top-level deployment block is folded into deployment_config with a warning, which leaves one place to read the target from. The schema accepts the nulls madengine itself writes: `madengine build` copies optional model fields straight from models.json, so a model declaring `"timeout": null` reaches the manifest as null rather than as the default the code appears to supply. Optional scalars, the tag list and the env/slurm/distributed blocks therefore allow null, while required fields and value types are unchanged -- a null block is a model that declared nothing, a nested null inside an env map is still an error. The blanket *.json ignore, there to keep model JSONs out of a dev checkout, would have swallowed the schema: runs from a checkout would work while the wheel shipped without it, since hatchling selects wheel contents by VCS status. src/madengine/ schemas/*.json is package data, so it is exempt.
…field A cluster run depends on host variables that say where things live: MODEL_DIR, the cache roots, MAD_DOCKER_BUILDS. The only way to supply them was to source mad.env in the shell that launched madengine, and forgetting produced failures far from the cause -- an empty MODEL_DIR resolves the run script path to nothing, and a MAD_DOCKER_BUILDS off shared storage makes every worker fail to find the image. deployment_config.env_file now names that file and madengine sources it itself, on the submit node while rendering the job and on every worker during the run. Relative paths resolve against the manifest, so a run directory stays movable. A missing file is fatal at startup instead of an empty string mid-run. Only the names of the applied variables are logged; an env file may carry tokens. MAD_DOCKER_BUILDS, until now only visible in the code, is documented alongside it.
…list Three writers create perf.csv -- the container runner, the deployment aggregation path and the Kubernetes results mixin -- and each carried its own copy of the 29-column header under a comment asking the reader to keep them in sync. Nothing enforced that, and a consumer reading the CSV had no declaration to read at all. perf_csv.schema.json now declares the row: names, order, types and a description per column, with the writers reading the header from it. Columns that restate a manifest field carry a JSON pointer to it, so the result row and the build manifest are two views of one shape; a test walks those pointers and fails if a manifest field is renamed out from under the result contract.
The SLURM presets describe how big a job is and for how long. Nothing described the cluster it lands on, so cluster facts were baked into the shape: the multi-node preset sets NCCL_IB_DISABLE=1 and NCCL_SOCKET_IFNAME=eth0, putting every multi-node run on TCP over an interface many clusters do not have, on a fabric that is often RoCE. Fixing that per cluster meant editing a shipped preset or repeating transport variables in every manifest. slurm.cluster_profile now names one or more fact profiles, merged after the shape presets and before the user, so a profile can correct a bad default and the user can still correct the profile. Facts are orthogonal, so several profiles may be named -- a fabric and "this scheduler advertises no GPU GRES" are separate statements -- and a null value removes a variable an earlier layer set, because an interface name that does not exist here is worse than none. Bundled profiles name hardware archetypes (Broadcom Thor2 and ConnectX-7 RoCE, Mellanox InfiniBand, plain TCP, no-GRES schedulers). Anything site-specific belongs in a site profile, found by path or via MADENGINE_CLUSTER_PROFILES. An unknown profile stops the run: a silent fallback to the wrong fabric costs more than a startup error. The profile shape is declared in cluster_profile.schema.json and validated on load, and the .gitignore exemption is extended so preset JSON reaches the wheel.
Runtime context initialisation probes the local node for its GPU vendor, architecture, count and render devices. On a SLURM login node there are none of those, so it raised "Unable to determine gpu vendor" and the run stopped before it reached the scheduler -- on a node that was never going to run the workload anyway. A cluster profile facts block now answers those questions when the local node cannot. Facts are a fallback, not an override: a node that can answer for itself still does, so on a heterogeneous partition the architecture recorded in results is the one that ran the work. A probe that reports nothing rather than failing -- "0 GPUs" from a machine with none -- counts as no answer too.
A two-node run finished with the scheduler recording exit code 3 and madengine printing "All model executions completed successfully!". Three defects lined up to make that possible: per-node exit codes were collected nowhere, a metric parsed from a log was treated as proof the whole run succeeded, and a deployment the scheduler called failed produced an empty failure list when its results could not be parsed. Each node now records its exit code, host and rank in a marker the submit node reads back, and result collection consults those markers and the scheduler's own verdict before it believes any number. The task script runs under `set -e`, so a non-zero madengine used to end it on that very line, leaving the failing node with no marker and no artifacts -- exactly the evidence a failed node has to leave behind; errexit is now off across that one call and restored once the exit code is captured. What a non-zero node means is decided the way the single-node path already states it in resolve_run_status: a metric is the strongest evidence a run did the work. Primus/Megatron reports throughput from the last global rank only, so every other node finds no metric locally and exits non-zero even when the training was perfect -- both accepted baseline runs on the reference cluster ended with node 0 at exit 3 and node 1 at 0. With a metric, the node outcomes and the scheduler's verdict are warnings and the numbers stand: a row with a number in it is a measurement whatever happened around it, so the rows stay in successful_runs and the verdict travels separately, naming what did not finish. With no metric anywhere the node evidence is all there is and it decides: a node that exited non-zero or never reported gives a failure naming it, a clean set of exit codes gives NO_METRIC and exit 5, because a broken result contract is not the same event as a crash, and neither is success. The summary is built in _summarise_deployment so the rule can be tested without standing up a deployment. For the same reason srun --kill-on-bad-exit is opt-in through slurm.kill_on_bad_exit rather than the default: the rank exiting non-zero may be the peer of the one holding the results, and killing the step there would throw them away. The multiple_results diagnostics now name the paths searched and the variable the container was given, so a model script that does not hold up its end of that contract can be fixed without reading madengine's source.
A model card declares where its results CSV lives in multiple_results, but the
field is optional and its value has to agree with a path a script in another
repository builds by hand. When the two disagreed madengine fell back to
scraping 'performance: <n> <metric>' from the log, found nothing there because a
CSV-reporting model never prints that line, and wrote a row with an empty
performance and status=FAILURE. One yellow warning was the only trace, and a
typo looked exactly like an omission.
The shape of the file is the sturdier contract. madengine.reporting.result_csv
recognises a results CSV by a header carrying model, performance and metric, in
any order and with any number of extra columns -- which is what MAD's scripts
write, from three columns in scripts/dummy/run_multi.sh to twenty-nine in
scripts/large-ep-benchmark/parse_ep_to_csv.py, with one script spacing them out
as 'model, performance, metric'. When nothing was declared, or what was declared
is not there, the run directory and the workspace root are searched at depth 1
and the best match stands in. A declared file that exists always wins.
Two things keep the search honest. madengine's own perf.csv and the
perf_super/perf_entry family are excluded: they match by construction, and
reading one back would feed a run its own previous verdict. And the workspace
root is shared by every model in a run, so a file not written during this
model's run is ignored -- fourteen of the fifteen MAD scripts that write a
conforming CSV put it in exactly that shared directory.
The job templates blocked all of this: the per-node copy sat inside
{% if multiple_results %}, so a card without the field left nothing on the node
for the collector to find. All four copy blocks now also sweep for
shape-matching CSVs, with the same exclusions, from one macro per template.
What a run says when it finds nothing changes too. Instead of 'Performance
metric not found in expected format' it names both halves of the contract:
where it looked, how many CSVs it saw, why each was rejected, and whether
MAD_OUTPUT_CSV was exported at all. The ranking that chooses between several
candidates moves out of the SLURM collector into the shared module, so the
Docker and SLURM paths judge the same file the same way and ties break
deterministically.
Known limit: 33 multi-node cards write their CSV to /run_logs on shared storage,
outside any depth-1 search. They were undiscoverable before this change too.
Tests: 675 -> 732 unit tests, all passing.
There was a problem hiding this comment.
Pull request overview
This PR formalizes several previously “tribal knowledge” contracts in madengine (manifest shape, perf.csv schema, cluster facts) and makes multi-node SLURM runs report outcomes more truthfully (including a distinct “no metric” outcome).
Changes:
- Add JSON Schemas (and validation) for the build manifest and perf.csv rows, plus migration to make
deployment_configthe single source of truth. - Introduce
deployment_config.env_filesourcing and SLURM cluster fact profiles (slurm.cluster_profile) to support headless submit nodes and portable multi-node defaults. - Improve multi-node verdict logic (per-node exit markers +
sacctfallback) and add results-CSV discovery by header shape.
Reviewed changes
Copilot reviewed 38 out of 39 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_result_csv_templates.py | Verifies SLURM/K8s templates copy result CSVs even when not declared. |
| tests/unit/test_result_csv_discovery.py | Tests result CSV discovery/ranking by header shape + staleness. |
| tests/unit/test_perf_csv_schema.py | Locks writers to perf.csv schema + manifest pointer linkage. |
| tests/unit/test_orchestration.py | Updates orchestration tests to include built_models. |
| tests/unit/test_multinode_failure.py | Pins new multi-node failure/NO_METRIC verdict behaviors. |
| tests/unit/test_manifest_schema.py | Tests manifest schema validation + cross-field checks + migration. |
| tests/unit/test_headless_context.py | Tests cluster-fact fallback for GPU-less submit nodes. |
| tests/unit/test_env_file.py | Tests env file sourcing semantics and failure modes. |
| tests/unit/test_cluster_profiles.py | Tests cluster profile discovery, validation, and merge semantics. |
| src/madengine/schemas/perf_csv.schema.json | Declares perf.csv row contract and manifest link pointers. |
| src/madengine/schemas/cluster_profile.schema.json | Declares schema for cluster fact profiles. |
| src/madengine/schemas/build_manifest.schema.json | Declares build manifest contract consumed by run. |
| src/madengine/schemas/init.py | Adds schema loading, perf header helpers, and manifest validation/migration. |
| src/madengine/reporting/update_perf_csv.py | Sources perf.csv header from schema (removes duplicated header). |
| src/madengine/reporting/result_csv.py | Adds discovery/ranking utilities for result CSVs by header shape. |
| src/madengine/orchestration/run_orchestrator.py | Validates manifest + applies env_file + summarizes deployment outcomes. |
| src/madengine/execution/container_runner.py | Uses shared result CSV discovery and preserves discovered CSV for reporting. |
| src/madengine/deployment/templates/slurm/job.sh.j2 | Copies result CSVs by shape; writes per-node node.status; optional kill-on-bad-exit. |
| src/madengine/deployment/templates/kubernetes/job.yaml.j2 | Copies result CSVs by shape into results volume. |
| src/madengine/deployment/slurm.py | Uses shared result CSV ranking; adds per-node status + sacct-based job-state verdicts. |
| src/madengine/deployment/presets/slurm/clusters/roce-mellanox-cx7.json | Bundled RoCE/CX7 cluster fact archetype. |
| src/madengine/deployment/presets/slurm/clusters/roce-broadcom-thor2.json | Bundled RoCE/Thor2 cluster fact archetype. |
| src/madengine/deployment/presets/slurm/clusters/no-gpu-gres.json | Bundled “no GPU GRES” scheduler profile. |
| src/madengine/deployment/presets/slurm/clusters/infiniband-mellanox.json | Bundled InfiniBand Mellanox archetype. |
| src/madengine/deployment/presets/slurm/clusters/ethernet-tcp.json | Bundled TCP-only archetype (no RDMA). |
| src/madengine/deployment/presets/slurm/init.py | Documents new layering order including cluster profiles. |
| src/madengine/deployment/presets/cluster_profiles.py | Implements cluster profile selection, lookup, validation, and merging. |
| src/madengine/deployment/k8s_results.py | Sources perf.csv header from schema. |
| src/madengine/deployment/config_loader.py | Applies cluster profiles between shape presets and user config. |
| src/madengine/deployment/base.py | Validates manifest + applies env_file on submit side; schema-based perf header. |
| src/madengine/core/env_file.py | Adds bash-sourced env file loader/applicator with safety/diagnostics. |
| src/madengine/core/context.py | Adds cluster-fact fallback probes for headless submit nodes. |
| src/madengine/cli/constants.py | Adds ExitCode.NO_METRIC = 5. |
| src/madengine/cli/commands/run.py | Exits distinctly for incomplete, failed, and NO_METRIC runs; prints warnings. |
| pyproject.toml | Adds jsonschema dependency. |
| docs/deployment.md | Documents env_file, cluster profiles, shared image store, and multi-node verdict rules. |
| docs/configuration.md | Links env_file docs for host-side variables. |
| docs/cli-reference.md | Documents exit code 5 and multi-node verdict semantics. |
| .gitignore | Un-ignores shipped schemas and preset JSON so wheels include them. |
Suppressed comments (1)
src/madengine/core/context.py:372
MAD_SYSTEM_NGPUSusesfacts.get('gpus_per_node', 0)as the headless fallback. When the submit node probe returns 0 (common on GPU-less login nodes), this makes the context record a concrete GPU count of 0, which then propagates into container env/build args. In headless mode, missing facts should not become a guessed 0; prefer an explicit empty/unanswered value unless the profile provides a realgpus_per_node.
if "MAD_SYSTEM_NGPUS" not in self.ctx["docker_env_vars"]:
self.ctx["docker_env_vars"]["MAD_SYSTEM_NGPUS"] = self._probe(
"the GPU count", self.get_system_ngpus, facts.get("gpus_per_node", 0)
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+311
to
+316
| if self._headless and not detected and fallback: | ||
| print( | ||
| f"Warning: this node reports no {description}; " | ||
| f"continuing with {fallback!r} from the cluster profile" | ||
| ) | ||
| return fallback |
Comment on lines
+123
to
+133
| def rank_candidates( | ||
| candidates: typing.Sequence[typing.Union[str, Path]] | ||
| ) -> typing.List[Path]: | ||
| """Candidates best first: most measured rows, then most rows, then newest, then name. | ||
|
|
||
| In a multi-node run every node copies its own CSV into the collection directory and | ||
| only some of them observed the final throughput, so the file with the most non-empty | ||
| ``performance`` rows is the one carrying the measurement. The remaining keys exist so | ||
| that two runs over the same inputs pick the same file: newest first, and failing that | ||
| the order the caller offered them in, which is the order the directories were searched. | ||
| """ |
| def _staleness_reason( | ||
| path: typing.Union[str, Path], min_mtime: typing.Optional[float] | ||
| ) -> typing.Optional[str]: | ||
| """"Written before this run started", when that is what happened.""" |
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.
Stacked on #PR163 (
mkuznet1/slurm-portability) — review that one first; this branch isbased on it, and the diff shown here is only correct once the base PR is merged or the
base is set to
mkuznet1/slurm-portability.PR 163 removed the cluster-specific assumptions from the generated job script. What was
left is the layer above it: a manifest nobody validates, cluster facts that only exist as
tribal knowledge in a wiki page, a submit node that insists on having GPUs, and a
multi-node run that reports success no matter how it ended. Bringing up a multi-node
training workload on a new cluster hit all four in one afternoon.
Ticket: AICOMNET-366.
What is in here
feat(manifest): validate the manifest schema and settle one source of truthslurm/distributedblocks were read from the top level in one code path and fromdeployment_configin another, so the two could disagree silently.slurm.nodesdisagreeing withdistributed.nnodes. Top-level deployment blocks are migrated underdeployment_configwith a warning, sodeployment_configis the one place that answers "where does this run".feat(config): give the environment and build-dir contract a manifest fieldmad.envwith its storage paths and transport variables, and every operator had to remember to source it before each command; forgetting it produced a run that looked fine and wrote its images to the wrong filesystem.deployment_config.env_filenames a shell env file (relative paths resolve against the manifest). madengine sources it before the run and logs which variables it applied — names only, never values. TheMAD_DOCKER_BUILDSshared image-store contract is now written down indocs/deployment.mdinstead of being folklore.refactor(reporting): make the result schema a contract, not a column listschemas/perf_csv.schema.jsondeclares the row shape, and every writer takes its header from it. Each column that comes from the manifest carries anx-manifest-sourceJSON pointer into the manifest schema, and a test resolves every one of those pointers — renaming a manifest field now breaks a test instead of quietly breaking a consumer.feat(slurm): make the per-cluster presets record the cluster's factsdeployment/presets/slurm/clusters/holds fact profiles, selected withslurm.cluster_profile(a bundled name, a path to a site profile, or a list merged in order). Five hardware archetypes ship with madengine: Broadcom Thor2 RoCE, Mellanox CX7 RoCE, Mellanox InfiniBand, plain Ethernet/TCP, and no-GPU-GRES. Site facts — cluster name, partition, account — stay in a site-owned file. Profiles override shape presets and lose to the user's own config.feat(context): support a headless submit nodefactsblock, a probe that cannot answer on the submit node falls back to the fact instead of guessing. On a compute node the probe still wins: the local answer describes the machine that will actually run the work. Render devices stay unanswered rather than borrowed — they are per-machine.fix(slurm): report multinode failures honestlysacctis consulted for the failures that leave no marker at all. A run that produced no metric while every node exited cleanly now exits with the newNO_METRIC(5) instead of masquerading as either success or crash, and themultiple_resultscontract prints what it looked for and where when the file is missing.feat(reporting): find a model's results CSV by its shapeNO_METRICverdict above is only as good as the search that precedes it. A card that writes a results CSV but never declaresmultiple_results, or declares a name it does not write, was reported as producing no metric even though the numbers were sitting on disk.model,performanceandmetriccolumns — inreporting/result_csv.py, shared by the container runner, the SLURM collector and the job templates. A declared file still wins; discovery only fills the gap, and says so when it does. Candidates are ranked by measured rows and mtime, with a staleness floor so a previous model's leftovers cannot be picked up.The schema and profile commits also exempt their JSON from the repository's blanket
*.jsonignore, which is there for model definitions in a dev checkout and wouldotherwise have kept the schemas out of the wheel — hatchling picks wheel contents by VCS
status, so a run from a checkout would work while an installed madengine did not.
Order
The commits are in dependency order and each one builds and tests on its own: the
manifest schema comes first because the others extend it, the perf.csv contract derives
from it, the cluster profiles introduce the facts the headless submit node reads, and the
multi-node verdict sits on top.
What the run taught us
Four assumptions in the commits above were contradicted by a real two-node run. Each
correction lives in the commit it belongs to; the reasoning is the interesting part:
build_orchestrator.pycopies optional model fields straight frommodels.json, so a model declaring"timeout": nullreaches the manifest as null — the default the code appears to supply only applies when the key is absent. The e2e suite caught it. Optional scalars and the env/slurm/distributed blocks now accept null; value types are unchanged.set -e, so a non-zero madengine ended it on that very line: no exit-code marker, no artifact copy — the failure reporting lost exactly the evidence it was written to collect. errexit is now off across that one call. The same commit stops relabelling measurements as failures, which had produced "24 successful, 0 failed" followed by "Execution failed for 8 models".resolve_run_statusalready states for a single node: a metric is the strongest evidence a run did the work.--kill-on-bad-exitis opt-inslurm.kill_on_bad_exit. Tearing the step down on the first bad exit sounds right for a collective job, but for the reason above the failing rank may be the peer of the one holding the numbers, so it is opt-in.The resulting multi-node verdict, documented in
docs/deployment.md:RUN_FAILURE, naming the nodeNO_METRICTesting
tests/unit: 738 passed (565 after PR 1, plus 173 new acrosstest_manifest_schema.py,test_env_file.py,test_perf_csv_schema.py,test_cluster_profiles.py,test_headless_context.py,test_multinode_failure.py,test_result_csv_discovery.py,test_result_csv_templates.py).tests/integrationon a GPU node: 149 passed, 1 failed, 2 skipped. The failure(
test_renderD_count_matches_gpu_count,assert 8 == 17) is a property of that node'sKFD topology and reproduces without these changes.
The full suite was rerun on a GPU node once the branch was complete: 57 failed, 885
passed, 2 skipped, against 57 failed, 697 passed on a
developcontrol run on the samenode — the same 57 tests both times, all of them blocked by that site's NFS root_squash
rather than by anything in this branch.
Manifest validation against the manifest of the real run:
Both before submission, where forty minutes ago the second one surfaced as a failed job.
env_filein an empty environment (env -i): the sitemad.envapplied 23 variables,among them
MAD_DOCKER_BUILDSpointing at the shared image store, so a run no longerdepends on the operator having sourced the file first.
Headless submit node, on a real login node with no
rocm-smiand no/dev/kfd:The render devices stay unanswered rather than borrowed from the profile, since those
are per-machine device numbers.
Five real 2-node SLURM runs on a
GresTypes=(null)cluster (Broadcom Thor2 / RoCE), aPrimus/Megatron training workload, with the manifest driving
env_fileandcluster_profileend to end. Each run completed and reported its metrics, with nobehaviour change against a pre-change run on the same node pair.
Each of these runs also exercises the interesting verdict case: the master node exits 3
while the numbers arrive from the other node. The final code reports it as
node 0 exited 3, while the run produced 8 metric row(s)followed byAll model executions completed successfully!and exit 0.