Skip to content

refactor: standardize srt-slurm cluster configuration across launchers - #3234

Merged
cquil11 merged 1 commit into
mainfrom
codex/standardize-srt-cluster-configs
Sep 18, 2026
Merged

cquil11 merged 1 commit into
mainfrom
codex/standardize-srt-cluster-configs

Conversation

@cquil11

@cquil11 cquil11 commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator

Summary

Move inline srtslurm.yaml generation into checked-in cluster profiles and one shared writer. Rebased onto main after #3270; this is a configuration refactor, not an additional backend port.

  • Add six profiles under runners/srt-slurm/: B200 Nscale, B300 DSXE, GB200, GB300, H100 DGXC, and H200 DGXC.
  • Preserve refactor(runners): one launcher per pool for B200 Nscale #3270's one-launcher-per-pool structure. B200's native-srt and multinode-srt paths share one profile; the latter supplies its two additional container aliases explicitly. There is no compat launcher or compat profile.
  • Keep native srt-slurm settings in YAML. Supply job-local paths explicitly and merge optional model aliases, container aliases, and mounts through write_srt_cluster_config.
  • Replace heredoc/string assembly and exporter sed injection. Substitution happens after YAML parsing, so punctuation in paths remains data.
  • Preserve cluster scheduling flags, GPU counts, time limits, image aliases, cache selections, and workload-dependent mounts. Engine flags, recipes, routing, and the srt-slurm pin are unchanged.
  • Leave non-srt-slurm paths untouched, including B200 single-node and fixed-sequence TileRT execution. AMD launchers on this base have not yet adopted the separate srt-slurm port, so this PR does not convert them.

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

  • Six focused behavioral tests pass: real Bash-to-Python rendering, optional power exporter handling, missing-input failure, YAML-safe substitution, mapping overlays, and preservation of an existing output on invalid input.
  • Re-ran the Linux before/after comparison against the merged refactor(runners): one launcher per pool for B200 Nscale #3270 launcher: 164 configuration variants are semantically identical and pass the schema from InferenceX's pinned srt-slurm revision (984180e5b8755aef85e9995048b5a16cb5336bce), including both B200 srt-slurm paths.
  • Bash syntax checks for all changed shell files, full infx Ruff lint/format checks, and git diff --check pass.
  • No GPU sweep has been dispatched for this PR. These checks establish configuration equivalence, not hardware E2E validation.

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.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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_config path 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.sh appended mount lines with no default_mounts: header when IS_AGENTIC was unset, producing invalid YAML; the new --mount flag builds the mapping correctly.
  • Checked that variables referenced via ${NAME} in each new runners/srt-slurm/*.yaml profile are all supplied via --var/--model/--container/--mount in 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.

@cquil11
cquil11 force-pushed the codex/standardize-srt-cluster-configs branch from 2ac814d to ef59705 Compare September 18, 2026 18:59
@cquil11
cquil11 merged commit dc52054 into main Sep 18, 2026
3 checks passed
@cquil11
cquil11 deleted the codex/standardize-srt-cluster-configs branch September 18, 2026 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant