fix: guard tools build-context on directory existence and fix SLURM env var escaping#134
Merged
Conversation
…n SLURM - docker_builder: only pass --build-context tools= when scripts/common/tools exists - slurm: use double-quote escaping for env var values instead of shlex.quote to preserve special characters like spaces and paths correctly Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
coketaste
requested review from
Cemberk,
Rohan138,
gargrahul and
leconcio
as code owners
May 29, 2026 14:45
6 tasks
coketaste
added a commit
that referenced
this pull request
Jun 8, 2026
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>
coketaste
added a commit
that referenced
this pull request
Jun 13, 2026
…NNER resolver (#132) * refactor: canonicalize launcher aliases and extract MAD_MULTI_NODE_RUNNER 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> * fix: address code review findings from launcher-update review - 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> * fix: simplify sglang-disagg dispatch in _generate_launcher_command _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> * fix: use shlex.quote for slurm_multi env var exports 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> * fix: remove deprecated Typer is_flag/flag_value from --use-image 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> * fix: resolve launcher inconsistencies for sentinels and aliases 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> --------- Co-authored-by: Claude Sonnet 4 <noreply@anthropic.com>
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
only included when the directory is present.
Replaced with double-quote escaping (, ", $, `) to safely handle special characters while remaining compatible with SLURM's bash environment.
Test plan