Skip to content

feat(config): add --config YAML configuration system with Hydra compose API - #121

Open
coketaste wants to merge 3 commits into
developfrom
coketaste/v2-config-driven
Open

coketaste wants to merge 3 commits into
developfrom
coketaste/v2-config-driven

Conversation

@coketaste

@coketaste coketaste commented May 3, 2026

Copy link
Copy Markdown
Collaborator

Summary

Adds --config as a composable, Hydra-based YAML alternative to --additional-context JSON strings, available on both run and build.

  • New src/madengine/config/ module — HydraConfigLoader (Compose API), ConfigTranslator (YAML → internal context key mapping), ConfigValidator (cross-field checks)
  • Hydra config groups under src/madengine/configs/ — default groups (platform, scheduler, hardware, launcher) and append-only groups (+profile, +env, +tools, +data, +build)
  • Mutual exclusion — --config cannot be combined with --additional-context / --additional-context-file
  • Empty-value filtering — empty lists and dicts are excluded from the translated context so they don't shadow existing defaults
  • examples/configs/ — annotated templates (local.yaml, slurm.yaml, k8s.yaml) plus ready-to-run demos under demo/local/, demo/slurm/, demo/k8s/
  • Fixture fix — dummy_torchrun / dummy_torchrun_multi n_gpus corrected from "1" to "4" so local multi-GPU demos show 1N×4G topology

Rebase onto develop

Merged current develop into this branch and resolved conflicts (pinned image digests, slurm_multi, timeout 0, launcher aliases, K8s mixin split, hatch artifacts packaging). Hydra configs/ are included in the wheel via hatch artifacts alongside scripts/.

Post-merge --config updates:

  • Inline --config key=value overrides now win over a user YAML file (previously YAML was merged last and silently overwrote them)
  • CLI applies YAML metadata for live_output, output, summary_output, data_config, model.skip_run, and model.timeout (including 0)
  • Validator accepts require_pinned_image and treats kubernetes as an alias of k8s for slurm conflict checks
  • Launcher aliases: megatronmegatron-lm, slurm_multi group, SLURM reservation / skip_gpus_directive in the slurm scheduler preset

Behaviour

Config group overrides:

madengine run --tags model --config scheduler=slurm --config launcher=torchrun

Append-only groups:

madengine run --tags model --config +profile=mi300x_8gpu --config +env=nccl_debug

User YAML file, then inline overrides (inline wins):

madengine run --config my_job.yaml
madengine run --config my_job.yaml --config distributed.nnodes=8

When a user YAML sets a distributed launcher, distributed.enabled: true must also be set explicitly — the default launcher: none group sets enabled: false and setting a launcher key alone does not override it.

Test plan

  • pytest tests/unit (789 passed after merge)
  • pytest tests/unit/test_config_schema.py tests/unit/test_config_translator.py tests/unit/test_config_integration.py tests/unit/test_hydra_config_loader.py tests/unit/test_cli.py
  • madengine run --config examples/configs/demo/local/single-gpu.yaml completes and writes perf.csv
  • madengine run --config examples/configs/demo/local/multi-gpu-torchrun.yaml shows 1N×4G topology and torchrun in the launcher column
  • madengine run --config my_job.yaml --additional-context '{}' exits with INVALID_ARGS (exit code 4)
  • pre-commit run --all-files passes

@coketaste coketaste self-assigned this May 3, 2026
Copilot AI review requested due to automatic review settings May 3, 2026 15:24

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

@coketaste

Copy link
Copy Markdown
Collaborator Author

Merged current develop into this branch (2f50ac9) and pushed — the PR should no longer be conflicted.

What landed in the merge besides conflict resolution:

  • --config key=value now overrides fields already set in a user YAML file
  • YAML live_output / output / summary_output / data_config / model.skip_run / model.timeout (including 0) are applied to the CLI
  • Schema knows about require_pinned_image and slurm+kubernetes conflicts
  • megatron canonicalizes to megatron-lm; added launcher=slurm_multi group

pytest tests/unit: 789 passed. Manual demo runs and pre-commit still need a pass on a GPU box.

@coketaste

Copy link
Copy Markdown
Collaborator Author

Pushed 1ba44c0 onto this branch (not merged).

--config stays a frontend for the existing additional-context dict:

  • Docs: job YAML uses JSON-shaped keys (slurm, distributed, …), not a Hydra defaults: list; only platform=docker is supported; hardware sets gpu_vendor/guest_os (engine Docker flags still come from vendor, not a runtime context key).
  • +build=ci / +build=multi_arch are real again (docker_clean_cache / build.target_archs), matching --clean-docker-cache and --target-archs.
  • YAML docker.keep_alive / docker.clean_cache apply to the CLI flags when true.
  • Golden tests: tests/unit/test_config_context_parity.py.

pytest tests/unit: 795 passed.

coketaste and others added 3 commits September 18, 2026 15:03
Accept a YAML file and Hydra group or key=value overrides on run and
build, then translate them into the same additional_context dict the
existing engine already consumes. --config is mutually exclusive with
--additional-context so current JSON and CLI paths stay unchanged.

Co-authored-by: Cursor <cursoragent@cursor.com>
Add unit tests for Hydra compose, validation, key mapping, and a
golden parity check that YAML produces the same context keys as JSON
additional_context.

Co-authored-by: Cursor <cursoragent@cursor.com>
Describe --config as the same additional_context contract in YAML form,
not a separate Hydra-native schema, and add runnable demo and template
job files.

Co-authored-by: Cursor <cursoragent@cursor.com>
@coketaste
coketaste force-pushed the coketaste/v2-config-driven branch from 1ba44c0 to 12b248f Compare September 18, 2026 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants