feat(madengine): give the manifest a contract and multi-node runs an honest verdict - #165
Draft
mkuznet1 wants to merge 5 commits into
Draft
Conversation
… 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.
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, and the 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 in the card looked exactly like a card that never declared the field. The declaration stays the contract. What changes is what a run says when it cannot read a metric: 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, and that predicate is used only to explain itself. The message now names the CSVs lying beside the run that look like results files and the field to declare them in, so a typo and an omission read differently while both still fail. madengine's own perf.csv and the perf_super/perf_entry family are never named, since they match by construction. Nothing is substituted. A file nobody declared is not read for a verdict, so no run silently reports a number from a file its card never mentioned. The same module also replaces the ad-hoc CSV parsing that validated a declared file inline, so 'no performance column' and 'every row is empty' are worded in one place instead of two. Deliberately not here: searching for and reporting from an undeclared file. Two MAD cards write a conforming CSV without declaring it, and one line in each card fixes that where the omission is. The other 32 such cards write to /run_logs on shared storage, which no depth-1 search would have reached anyway.
There was a problem hiding this comment.
Pull request overview
This PR strengthens madengine’s SLURM/K8s run behavior by introducing explicit contracts (JSON Schemas) for the build manifest and perf.csv, adding a manifest-driven env_file mechanism, improving multi-node failure verdict reporting, and improving diagnostics when no metric is produced (especially around multiple_results CSVs).
Changes:
- Add JSON Schemas + runtime validation/migration for build manifests, and a schema-backed contract for
perf.csvcolumns. - Add
deployment_config.env_filesupport (sourced via bash) and propagate the “no metric” outcome up to CLI exit codes. - Improve SLURM multi-node outcome reporting (per-node exit markers +
sacctstate) and add results-CSV “shape” recognition for better “no metric” explanations.
Reviewed changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_result_csv.py | New tests for results-CSV header shape recognition + suggestion messaging. |
| tests/unit/test_perf_csv_schema.py | New tests enforcing perf.csv header comes from schema and schema pointers remain valid. |
| tests/unit/test_orchestration.py | Update orchestration unit tests for new built_models manifest key usage. |
| tests/unit/test_multinode_failure.py | New tests pinning SLURM multi-node failure/NO_METRIC behavior and job template markers. |
| tests/unit/test_manifest_schema.py | New tests for manifest schema validation, pointer-based errors, and deployment block migration. |
| tests/unit/test_env_file.py | New tests for deployment_config.env_file sourcing, precedence, and failure reporting. |
| src/madengine/schemas/perf_csv.schema.json | New perf.csv row schema; property order defines column order; links back to manifest fields. |
| src/madengine/schemas/build_manifest.schema.json | New build manifest schema with nullable optional fields and deployment config structure. |
| src/madengine/schemas/init.py | New schema loader + manifest validator + perf schema helpers. |
| src/madengine/reporting/update_perf_csv.py | Switch PERF_CSV_HEADER to schema-derived header. |
| src/madengine/reporting/result_csv.py | New module to recognize “results CSV” by header shape and generate candidate suggestions. |
| src/madengine/orchestration/run_orchestrator.py | Validate manifests on load; apply env_file; improve distributed summary reporting. |
| src/madengine/execution/container_runner.py | Better diagnostics for missing/invalid multiple_results; suggest nearby result-like CSVs. |
| src/madengine/deployment/templates/slurm/job.sh.j2 | Record per-node exit markers; avoid set -e tearing down evidence; optional kill-on-bad-exit. |
| src/madengine/deployment/slurm.py | Add node marker parsing, sacct state checks, NO_METRIC handling, and honest incomplete verdicts. |
| src/madengine/deployment/k8s_results.py | Switch perf header creation to schema-derived header. |
| src/madengine/deployment/base.py | Validate manifests and source env_file on deployment load; switch perf header to schema-derived. |
| src/madengine/core/env_file.py | New bash-sourced env-file loader/applicator with timeout and “names-only” reporting. |
| src/madengine/cli/constants.py | Add ExitCode.NO_METRIC = 5. |
| src/madengine/cli/commands/run.py | Surface warnings/incomplete/no-metric outcomes distinctly; return NO_METRIC exit code. |
| pyproject.toml | Add jsonschema runtime dependency. |
| docs/deployment.md | Document env_file, MAD_DOCKER_BUILDS contract, and multi-node verdict logic. |
| docs/configuration.md | Cross-link host-side env-file mechanism in configuration docs. |
| docs/cli-reference.md | Document NO_METRIC and updated multi-node outcome semantics. |
| .gitignore | Unignore packaged schema JSON files so they ship in wheels. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+195
to
+197
| validator = jsonschema.Draft202012Validator(load_schema()) | ||
| first = next(iter(sorted(validator.iter_errors(manifest), key=lambda e: list(e.path))), None) | ||
| if first is not None: |
Comment on lines
232
to
+236
| deployment_config = manifest.get("deployment_config", {}) | ||
|
|
||
|
|
||
| if deployment_config.get("env_file"): | ||
| applied = apply_env_file( | ||
| deployment_config["env_file"], |
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.
Depends on #163
The base of this PR is
mkuznet1/slurm-portability, the head of #163, so the diff here isonly the five commits listed below. #163 has to land first.
This one is opened from a fork (
mkuznet1/madengine) rather than from a branch in thisrepository, because the
develop-mergingruleset matches~ALLrefs and rejects everypush to a branch after the first one — which froze the head of #164 and made review
feedback impossible to address there. From a fork the head can still be updated, so
comments on this PR can be answered in place.
What is in it
feat(manifest): validate the manifest schema and settle one source of truthA JSON Schema for the build manifest, validated on load, with errors that name the
offending field by JSON pointer instead of failing later somewhere else. It also settles a
long-standing ambiguity: a deployment block written at the top level of the manifest is
migrated under
deployment_configwith a warning, and when both are present the one underdeployment_configwins and the other is reported as ignored. Cross-field rules that usedto be folklore are checked, e.g.
slurm.nodesagainstdistributed.nnodes.feat(config): give the environment and build-dir contract a manifest fielddeployment_config.env_filenames a shell env file (themad.envconvention) andmadengine sources it itself, on the submit node while rendering the job and on every worker
during the run, instead of requiring an operator to remember
source mad.envin the rightshell. Relative paths resolve against the manifest, so a run directory stays movable. The
field is optional and nothing requires
MAD_DOCKER_BUILDSor any other variable to be set;what is fatal is naming a file that is not there. Only the names of the applied variables
are logged, never the values, because such a file legitimately carries tokens.
refactor(reporting): make the result schema a contract, not a column listperf.csvhad its columns spelled out in code. They now come from a schema that alsorecords, per column, which manifest field it is derived from (
x-manifest-source, a JSONpointer), so the row a run produces can be traced back to the manifest that produced it.
fix(slurm): report multinode failures honestlyA multi-node job used to be judged by the master rank. Each node now leaves an exit marker,
the job state is read back from
sacct, and the verdict distinguishes "some ranks failed"from "nothing was measured" — while rows that did carry a number still count as
measurements, since a model that reports four precisions and crashes on two still measured
the other two.
fix(reporting): name the reason a run produced no metricWhen a model card declares
multiple_resultsand the path disagrees with what the scriptin MAD actually wrote, madengine fell back to scraping 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. A typo in the card looked exactly like a card that declares nothing. Thedeclaration stays the contract; what changes is the message. A results CSV is recognised by
a header carrying
model,performanceandmetric, in any order and with any number ofextra columns, and that predicate is used only to explain: the run now names the CSVs beside
it that look like results files and the field to declare them in. Nothing is substituted —
a file nobody declared is never read for a verdict.
Deliberately not here
value they carried is already expressible in the manifest, so they added configuration in
the source tree without adding expressiveness.
addressed is not reachable from the real submit path, which already skips the probes when
the values are declared.
conforming CSV without declaring one, and a single line in each card fixes that where the
omission is; the other 32 such cards write to
/run_logson shared storage, which nodepth-1 search would have reached anyway.
Testing
A unit run on this branch is in progress and I will post the result here; nothing above is
claimed as verified until then. The comparison that matters is against
developon the samenode, since a number of unit failures there are pre-existing and unrelated to this branch.