refactor: canonicalize launcher aliases and extract MAD_MULTI_NODE_RUNNER resolver by coketaste · Pull Request #132 · ROCm/madengine

coketaste · 2026-05-29T12:29:49Z

Summary

Refactors distributed launcher handling to eliminate scattered alias branching, extracts the local Docker MAD_MULTI_NODE_RUNNER resolution into a dedicated helper, and fixes SLURM env-var quoting. Also includes a few small follow-up fixes surfaced during review.

Changes

Launcher alias canonicalization

Add canonicalize_distributed_launcher() in deployment/common.py with a single _LAUNCHER_ALIASES map (sglang_disagg → sglang-disagg). New aliases are added in one place instead of branching on alternate spellings at every dispatch site.
Replace the launcher == "sglang-disagg" or launcher == "sglang_disagg" checks in slurm.py and k8s_template_context.py with a canonicalize-then-compare. In slurm.py, canonicalization runs before normalize_launcher so sglang_disagg is no longer incorrectly mapped to docker.

Local Docker MAD_MULTI_NODE_RUNNER resolver

Extract the inline resolution block in container_runner.py into _resolve_local_multi_node_runner_env(). Same resolution order (additional_context → model_info → MAD_LAUNCHER, fallback torchrun), now routed through canonicalize_distributed_launcher and unit-testable in isolation.

SLURM env-var quoting

Replace the hand-rolled backslash-escaping in slurm.py with shlex.quote(str(value)) for export KEY=VALUE lines in the slurm_multi wrapper. Correct and simpler; simple values like xP=1 emit unquoted.

CLI fix

Remove the deprecated is_flag=False, flag_value="auto" from --use-image. That combination emitted a DeprecationWarning at import and the bare --use-image form was already non-functional (errored "requires an argument"). --use-image auto is now the documented auto-detect form. Docs updated accordingly.

Docs

Add a "Behavioral Guidelines" section to CLAUDE.md (LLM coding-mistake guardrails). (Unrelated to the launcher work — included here for convenience.)
Update cli-reference.md, launchers.md, deployment.md, wiki/index.html to drop the bare --use-image form.

Tests

test_container_runner.py — coverage for _resolve_local_multi_node_runner_env (~110 new lines).
test_slurm_multi.py — assertions updated to the shlex.quote export format.
test_deployment.py — alias canonicalization coverage.

Test plan

pytest tests/unit/test_container_runner.py tests/unit/test_slurm_multi.py tests/unit/test_deployment.py -v
pytest (full suite)
madengine build --use-image auto ... auto-detects from model card
slurm_multi wrapper exports render correctly for values with spaces/special chars

…NNER resolver - Add canonicalize_distributed_launcher() to common.py with sglang_disagg → sglang-disagg alias - Replace inline duplicate checks in k8s_template_context.py and slurm.py with the helper - Extract _resolve_local_multi_node_runner_env() method in container_runner.py; self-managed launchers (vllm/sglang/sglang-disagg/primus) now always set MAD_MULTI_NODE_RUNNER="" so downstream scripts under set -u don't fail - Add test coverage for new helper and extracted method, including sglang_disagg alias path Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>

Copilot

Pull request overview

This PR refactors launcher alias handling and Docker-local MAD_MULTI_NODE_RUNNER resolution so distributed launcher names are canonicalized consistently and local runs get a defined runner environment variable.

Changes:

Adds canonicalize_distributed_launcher() with an alias map for sglang_disagg → sglang-disagg.
Extracts Docker-local multi-node runner env resolution into _resolve_local_multi_node_runner_env().
Adds unit coverage for launcher canonicalization and local runner command/env generation.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
`src/madengine/deployment/common.py`	Adds shared launcher alias canonicalization helper.
`src/madengine/deployment/slurm.py`	Uses canonicalization in the SGLang disaggregated launcher dispatch.
`src/madengine/deployment/k8s_template_context.py`	Uses canonicalization in Kubernetes SGLang disaggregated launcher dispatch.
`src/madengine/execution/container_runner.py`	Extracts and updates Docker-local `MAD_MULTI_NODE_RUNNER` resolution.
`tests/unit/test_deployment.py`	Adds tests for launcher canonicalization.
`tests/unit/test_container_runner.py`	Adds tests for local launcher command and env resolution behavior.
`CLAUDE.md`	Adds behavioral guidance documentation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- Merge origin/develop imports: both canonicalize_distributed_launcher (this branch) and is_self_managed_launcher (develop) are now imported - Fix slurm_multi export generation: shlex.quote() returns values unquoted for safe strings, but bash convention and tests expect double-quoted exports (export KEY="value") Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>

- common.py: add missing blank line after canonicalize_distributed_launcher (E305 flake8 violation) - slurm.py: canonicalize launcher aliases before normalize_launcher in _prepare_template_context so sglang_disagg reaches _generate_launcher_command as the canonical sglang-disagg instead of being silently mapped to "docker" - slurm.py: revert slurm_multi env-var export to shlex.quote (shell injection safe) from the double-quote form that was not injection-safe - test_slurm_multi.py: align export assertions with shlex.quote output Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>

…uncher-update

_prepare_template_context() already canonicalizes launcher aliases before calling normalize_launcher(), so launcher_type is always in canonical form by the time it reaches _generate_launcher_command(). Replace the redundant canonicalize_distributed_launcher() call with a direct equality check, consistent with all other launcher branches. Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.

A merge brought in manual double-quote escaping (PR #134) for env var values while this branch's tests expected shlex.quote output, causing two test_slurm_multi failures. Revert to shlex.quote, which is Python's standard shell-safe quoting and is already used for the script path and model args a few lines above. This restores consistency and reliably handles spaces, paths, and special characters. Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>

The is_flag=False/flag_value="auto" workaround emitted a DeprecationWarning at import time and was already non-functional in Typer 0.24.2 (bare --use-image errored "requires an argument"). Make --use-image a normal value option and document --use-image auto as the auto-detect form, which already works. Update docs that referenced the broken bare-flag form. Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

# Conflicts: # tests/unit/test_container_runner.py

Copilot

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Suppress the "Unrecognized launcher" warning for deployment-mode sentinels (docker/native) since defaulting them to torchrun is expected, not an error. Persist the normalized SLURM launcher back onto distributed_config so reporting paths see the same launcher the template used instead of re-mapping aliases (e.g. sglang_disagg) to "docker". Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>

Merges 4 upstream commits (ef87d05..3a68be8) into the multi-node SLURM branch: - feat: canonicalize launcher aliases; extract MAD_MULTI_NODE_RUNNER resolver into _resolve_local_multi_node_runner_env() (upstream ROCm#132) - fix(run): --skip-model-run now starts container before skipping model script (ROCm#145) - chore: consolidate optional deps into default dependencies (ROCm#146) - Guyen/develop build-context folder change 2 (ROCm#144) Conflict in src/madengine/execution/container_runner.py resolved: - Upstream's if/else skip_model_run wrapper is the outer structure - Our try/except RuntimeError (container diagnostics on failure) is placed inside the else: branch around model_docker.sh() - Our elif skip_perf_collection: status branch and fallback or-clause are preserved in the else: branch status logic

Second merge of upstream/develop (ef87d05) on top of prior merge a6b88b2. Brings in: ROCm#145 (--skip-model-run), ROCm#132 (launcher canonicalize), ROCm#144 (build context). Conflict in src/madengine/execution/container_runner.py resolved.

coketaste self-assigned this May 29, 2026

Copilot AI review requested due to automatic review settings May 29, 2026 12:29

coketaste requested review from Cemberk, Rohan138, gargrahul and leconcio as code owners May 29, 2026 12:29

Copilot started reviewing on behalf of coketaste May 29, 2026 12:30 View session

Copilot AI reviewed May 29, 2026

View reviewed changes

Comment thread src/madengine/deployment/slurm.py Outdated

coketaste and others added 4 commits May 29, 2026 13:50

Merge branch 'develop' of github.com:ROCm/madengine into coketaste/la…

900f2bb

…uncher-update

Copilot AI review requested due to automatic review settings June 8, 2026 14:23

Copilot started reviewing on behalf of coketaste June 8, 2026 14:24 View session

Copilot AI reviewed Jun 8, 2026

View reviewed changes

Comment thread src/madengine/execution/container_runner.py

Comment thread src/madengine/deployment/slurm.py

Comment thread tests/unit/test_slurm_multi.py

Comment thread tests/unit/test_slurm_multi.py

Comment thread tests/unit/test_slurm_multi.py

coketaste and others added 2 commits June 8, 2026 10:14

Copilot AI review requested due to automatic review settings June 8, 2026 15:49

Copilot started reviewing on behalf of coketaste June 8, 2026 15:50 View session

Copilot AI reviewed Jun 8, 2026

View reviewed changes

Comment thread src/madengine/cli/commands/build.py

Comment thread CLAUDE.md

coketaste added 2 commits June 12, 2026 10:25

Merge branch 'develop' into coketaste/launcher-update

53fc2d7

Merge branch 'develop' into coketaste/launcher-update

64ab9d5

# Conflicts: # tests/unit/test_container_runner.py

Copilot AI review requested due to automatic review settings June 12, 2026 23:36

Copilot started reviewing on behalf of coketaste June 12, 2026 23:37 View session

Copilot AI reviewed Jun 12, 2026

View reviewed changes

Comment thread src/madengine/execution/container_runner.py Outdated

Comment thread src/madengine/deployment/slurm.py Outdated

coketaste merged commit 8255d91 into develop Jun 13, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: canonicalize launcher aliases and extract MAD_MULTI_NODE_RUNNER resolver#132

refactor: canonicalize launcher aliases and extract MAD_MULTI_NODE_RUNNER resolver#132
coketaste merged 10 commits into
developfrom
coketaste/launcher-update

coketaste commented May 29, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

coketaste commented May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Launcher alias canonicalization

Local Docker MAD_MULTI_NODE_RUNNER resolver

SLURM env-var quoting

CLI fix

Docs

Tests

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coketaste commented May 29, 2026 •

edited

Loading