fix(deployment): validate launcher names instead of silently defaulting - #174
Open
coketaste wants to merge 2 commits into
Open
fix(deployment): validate launcher names instead of silently defaulting#174coketaste wants to merge 2 commits into
coketaste wants to merge 2 commits into
Conversation
…t key The launcher field must be "megatron-lm" per VALID_LAUNCHERS in deployment/common.py — "megatron" silently falls through to the Docker fallback on SLURM. Also removes distributed.master_port, which is never read (SLURM reads distributed.port; K8s reads a separate top-level launcher.master_port object), across all affected example configs.
normalize_launcher() mapped any unrecognized launcher spelling to the docker/native sentinel with no error, so a typo'd launcher silently ran as a single-process job and reported success. This hid a real bug: the documented megatron-lm spelling was broken on both SLURM and Kubernetes because their dispatch arms compared against the literal "megatron". Replace normalization with validate_launcher(), which accepts exactly one canonical spelling per launcher (plus the documented slurm-multi hyphen alias) and raises ConfigurationError with did-you-mean suggestions for anything else. Validation is enforced at two chokepoints - cli/validators.py (CLI additional_context) and BaseDeployment.__init__ (manifest and model-card sources) - so all three config sources are covered on both backends. Also fixes the megatron-lm dispatch arms in slurm.py and k8s_template_context.py, and adds sglang-disagg to the Ray GPU-visibility guard in both job templates, which it had been missing from and would otherwise hit Ray's "Inconsistent values found" error. Consolidates three divergent launcher-read implementations in container_runner.py into one _resolve_launcher() helper. Adds tests/unit/test_launcher_dispatch.py, parametrized over every valid launcher on both backends, asserting each reaches a real dispatch arm rather than falling through silently. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
coketaste
requested review from
Cemberk,
Rohan138,
gargrahul and
leconcio
as code owners
August 20, 2026 01:17
There was a problem hiding this comment.
Pull request overview
This PR tightens distributed launcher handling across madengine’s deployment paths by replacing permissive launcher normalization with explicit validation, fixing megatron-lm dispatch on SLURM/Kubernetes, and aligning templates/tests so invalid launcher spellings fail loudly instead of silently running single-process jobs.
Changes:
- Replace silent launcher defaulting with
validate_launcher()+ enforce validation at CLI (cli/validators.py) and deployment initialization (BaseDeployment.__init__). - Fix backend dispatch/template logic for
megatron-lm(previously compared against"megatron"), and extend Ray GPU-visibility guards to includesglang-disagg. - Consolidate container-runner launcher resolution and update/add unit tests and example configs to use canonical launcher/port keys.
Reviewed changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_validators.py | Adds CLI-boundary launcher validation tests (accept/reject/canonicalize). |
| tests/unit/test_slurm_multi.py | Updates tests to validate_launcher() and asserts unknown launchers now error. |
| tests/unit/test_launcher_dispatch.py | New parity/dispatch tests to ensure each valid launcher reaches a real dispatch arm. |
| tests/unit/test_deployment.py | Replaces normalize_launcher tests with launcher_for_reporting + validate_launcher tests. |
| tests/unit/test_container_runner.py | Updates self-managed launcher tests to match canonical spellings only. |
| src/madengine/execution/container_runner.py | Consolidates launcher lookup into _resolve_launcher() and drops alias canonicalization. |
| src/madengine/deployment/templates/slurm/job.sh.j2 | Adds sglang-disagg to Ray GPU guard; switches megatron → megatron-lm in templated checks. |
| src/madengine/deployment/templates/kubernetes/job.yaml.j2 | Adds sglang-disagg to Ray GPU guard; updates distributed-env block to megatron-lm. |
| src/madengine/deployment/slurm.py | Removes normalization/canonicalization; fixes megatron-lm dispatch; uses launcher_for_reporting. |
| src/madengine/deployment/k8s_template_context.py | Removes alias canonicalization; fixes megatron-lm dispatch arms; treats sglang-disagg as canonical. |
| src/madengine/deployment/k8s_results.py | Uses launcher_for_reporting in results collection paths. |
| src/madengine/deployment/common.py | Introduces validate_launcher() and launcher_for_reporting(); updates self-managed detection. |
| src/madengine/deployment/base.py | Validates/canonicalizes launchers in __init__ across additional_context, manifest, and model cards. |
| src/madengine/cli/validators.py | Adds _validate_launcher_after_defaults() to fail fast on invalid launcher values. |
| examples/slurm-configs/README.md | Updates documented launcher list and examples to megatron-lm. |
| examples/slurm-configs/minimal/megatron-lm-minimal.json | Updates launcher to megatron-lm. |
| examples/slurm-configs/basic/09-megatron-lm-multi-node.json | Updates launcher to megatron-lm; switches master_port → port. |
| examples/k8s-configs/README.md | Updates docs to megatron-lm in launcher lists/descriptions. |
| examples/k8s-configs/minimal/megatron-lm-optimized.json | Updates launcher to megatron-lm; switches master_port → port. |
| examples/k8s-configs/minimal/megatron-lm-minimal.json | Updates launcher to megatron-lm. |
| examples/k8s-configs/minimal/megatron-lm-exclude-node.json | Updates launcher to megatron-lm. |
| examples/k8s-configs/basic/vllm-multi-node-basic.json | Switches master_port → port. |
| examples/k8s-configs/basic/torchtitan-multi-node-basic.json | Switches master_port → port. |
| examples/k8s-configs/basic/sglang-multi-node-basic.json | Switches master_port → port. |
| examples/k8s-configs/basic/sglang-disagg-multi-node-basic.json | Switches master_port → port. |
| examples/k8s-configs/basic/sglang-disagg-custom-split.json | Switches master_port → port. |
| examples/k8s-configs/basic/megatron-lm-multi-node-basic.json | Updates launcher to megatron-lm; switches master_port → port. |
| examples/k8s-configs/basic/06-data-provider-with-pvc.json | Switches master_port → port. |
| examples/k8s-configs/basic/05-torchrun-nvidia-gpu-example.json | Switches master_port → port. |
| examples/k8s-configs/basic/04-torchrun-multi-node-advanced.json | Switches master_port → port. |
| examples/k8s-configs/basic/03-torchrun-multi-node-basic.json | Switches master_port → port. |
| examples/k8s-configs/basic/02-torchrun-single-node-multi-gpu.json | Switches master_port → port. |
| examples/k8s-configs/basic/02-torchrun-single-node-multi-gpu-tools.json | Switches master_port → port. |
| docs/deployment.md | Updates supported launcher spelling to megatron-lm. |
Suppressed comments (1)
src/madengine/deployment/common.py:83
validate_launcher()trims whitespace intonormalized, but whitespace-only values (e.g. " ") still fall through to the "unknown launcher" error. This contradicts the function docstring (“Empty values mean 'no launcher configured'”) because after stripping, the value is empty and should be treated as unset.
normalized = launcher.strip().lower()
if normalized in _LAUNCHER_SENTINELS:
return normalized
normalized = _DOCUMENTED_ALIASES.get(normalized, normalized)
if normalized in VALID_LAUNCHERS:
return normalized
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
131
to
135
| # Normalize launcher based on deployment type and validity | ||
| launcher_type = normalize_launcher(launcher_type, "kubernetes") | ||
| launcher_type = launcher_for_reporting(launcher_type, "kubernetes") | ||
|
|
||
| is_ray_launcher = launcher_type in ["vllm", "sglang"] | ||
|
|
Comment on lines
+39
to
+42
| # Deployment-mode sentinels meaning "no distributed launcher". They are produced | ||
| # by launcher_for_reporting(), never typed by a user, and must pass validation | ||
| # unchanged so a round-trip through a report does not raise. | ||
| _LAUNCHER_SENTINELS = frozenset({"docker", "native"}) |
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.
Summary
normalize_launcher()silently mapped any unrecognized launcher spelling to the docker/native sentinel, so a typo'd launcher ran as a single-process job and reported success instead of failing. This masked a real bug: the documentedmegatron-lmspelling was broken on both SLURM and Kubernetes because their dispatch arms compared against the literal"megatron".validate_launcher(), which accepts exactly one canonical spelling per launcher (plus the documentedslurm-multihyphen alias) and raisesConfigurationErrorwith did-you-mean suggestions for anything else. Enforced at two chokepoints —cli/validators.py(CLIadditional_context) andBaseDeployment.__init__(manifest and model-card sources) — covering all three config sources on both backends.megatron-lmdispatch arms inslurm.pyandk8s_template_context.py, and addssglang-disaggto the Ray GPU-visibility guard in both job templates (it was missing and would otherwise hit Ray's "Inconsistent values found" error).container_runner.pyinto one_resolve_launcher()helper.megatron→megatron-lmlauncher value and drops the deaddistributed.master_portkey across affected example configs (examples/k8s-configs/,examples/slurm-configs/).Split out of
coketaste/docs-rewrite— this is a behavior change (new validation error path) and belongs in its own review rather than riding along with a docs rewrite. The docs branch depends on this landing first (it documents themegatron-lmspelling and the validation error text).Test plan
pytest tests/unit— 646 passedtests/unit/test_launcher_dispatch.py, parametrized over every valid launcher on both backends, asserts each reaches a real dispatch arm rather than falling through silentlytests/unit/test_validators.py,tests/unit/test_deployment.py,tests/unit/test_slurm_multi.py,tests/unit/test_container_runner.pyupdated/passing🤖 Generated with Claude Code