refactor: standardize srt-slurm cluster configuration across launchers - #3234
Conversation
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it's a fairly large refactor (18 files) that changes how srt-slurm cluster configs are generated for seven different GPU cluster launchers, a human look would still be worthwhile.
- Traced the new
write_srt_cluster_config/render_cluster_configpath end-to-end (template substitution after YAML parse, mapping overrides for models/containers/mounts, error handling that leaves an existing output file untouched on failure). - Compared each rewritten launcher (b200-nscale-compat/slurm, b300-dsxe, gb200/gb300-nv, h100/h200-dgxc-slurm) against its pre-PR heredoc to confirm settings, aliases, and mounts are preserved.
- Confirmed the claimed TileRT fix: the old
launch_b200-nscale-slurm.shappended mount lines with nodefault_mounts:header whenIS_AGENTICwas unset, producing invalid YAML; the new--mountflag builds the mapping correctly. - Checked that variables referenced via
${NAME}in each newrunners/srt-slurm/*.yamlprofile are all supplied via--var/--model/--container/--mountin the corresponding launcher.
Extended reasoning...
Overview
This PR replaces inline heredoc/sed-based generation of srtslurm.yaml in seven srt-slurm launcher scripts with checked-in YAML profiles under runners/srt-slurm/, a shared write_srt_cluster_config bash helper, and a new infx/srt_slurm/cluster_config.py module that does string.Template substitution on parsed YAML followed by dict-merge overlays for model/container/mount aliases. A new test file (utils/test_srt_cluster_config.py) exercises the bash-to-python path, error handling, and mapping merges.
Security risks
None identified. The Python module only does local file I/O (yaml.safe_load/yaml.safe_dump, no yaml.load), argument parsing, and string templating with an explicit variable dict — no shell-outs, no eval, no network calls. Substitution happens post-YAML-parse specifically to avoid the old sed-injection and heredoc-quoting risks, which is a genuine improvement over the prior approach.
Level of scrutiny
I traced through the diff for all seven launchers and their corresponding YAML profiles, comparing each against the pre-PR heredoc byte-for-byte (accounting for order-independent YAML mappings), and verified variable plumbing (e.g., CONTAINER_KEY, MODEL_ROOT, AIPERF_MMAP_CACHE_HOST_PATH, HF_HUB_CACHE_HOST_PATH/HF_HUB_CACHE_MOUNT, DYNAMO_WHEELS_CACHE_HOST_PATH) is present as --var args matching each profile's ${NAME} placeholders. I could not execute the test suite or run bash -n syntax checks in this sandbox (execution commands were denied), so I relied on manual static tracing rather than confirming the author's own test/validation claims directly.
Other factors
The bug-hunting run reported zero findings and I found nothing contradicting that in my own pass. The change is mechanical in spirit but touches production benchmark automation across many GPU cluster types (B200, B300, GB200, GB300, H100, H200) with per-launcher variable wiring that is easy to get subtly wrong (e.g., a missing --var for a placeholder used in a profile) — the kind of change where a human's familiarity with each cluster's quirks (referenced inline in the diff, like the GB200 segment-directive comment) adds value beyond what static review can catch, especially since I couldn't independently execute the author's cited test/validation suite.
This review covers commit a30bfa7, which is no longer the latest commit on this pull request; later commits are not covered by it.
2ac814d to
ef59705
Compare
Summary
Move inline
srtslurm.yamlgeneration into checked-in cluster profiles and one shared writer. Rebased ontomainafter #3270; this is a configuration refactor, not an additional backend port.runners/srt-slurm/: B200 Nscale, B300 DSXE, GB200, GB300, H100 DGXC, and H200 DGXC.native-srtandmultinode-srtpaths share one profile; the latter supplies its two additional container aliases explicitly. There is no compat launcher or compat profile.write_srt_cluster_config.sedinjection. Substitution happens after YAML parsing, so punctuation in paths remains data.The earlier TileRT bug-fix claim and associated changelog entry have been removed: fixed-sequence TileRT bypasses this configuration-generation path, so synthetic execution of that block did not establish a reachable runtime bug.
Validation
984180e5b8755aef85e9995048b5a16cb5336bce), including both B200 srt-slurm paths.infxRuff lint/format checks, andgit diff --checkpass.AI model disclosure
OpenAI Codex prepared and rebased this PR. The exact underlying model/version identifier could not be verified from the runtime. No delegated agent contributed to this InferenceX PR.