refactor(runners): one launcher per pool for B200 Nscale - #3270
Conversation
The workflows run runners/launch_${RUNNER_NAME%%_*}.sh, so the only
launcher the b200-nscale-slurm_* pool ever resolves to is
launch_b200-nscale-slurm.sh. launch_b200-nscale-compat.sh (the renamed
legacy launch_b200-dgxc.sh) was reached only by an exec from that file,
which made it look like a second runner pool.
Fold the compat script into launch_b200-nscale-slurm.sh as three named
paths selected once at the top: native-srt (cluster-maintained multi-node
lanes), multinode-srt (other multi-node jobs) and single-node. Each path
body is carried over verbatim; the only shared code is the duplicated
enroot squash import, which now uses one helper with the caller's
SQUASH_DIR and SQUASH_LOCK_TIMEOUT. Path selection was checked equal to
the old two-file logic over 840 input combinations.
Drop the dead b200-nscale-compat alias from runtime_settings.sh, point
the GLM-5.2 FP8 bench comment at the real launcher, and document the
one-launcher-per-pool rule in AGENTS.md.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Thanks for the contribution!
中文感谢你的贡献!
|
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because this consolidates a production CI launcher into a single ~900-line file with three execution paths and has not been run on the cluster yet, a human look would still be worthwhile before merging.
- Diffed the new native-srt, multinode-srt, and single-node function bodies against their old sources in
launch_b200-nscale-slurm.sh/launch_b200-nscale-compat.sh— matches the described squash-helper consolidation andcheck_env_varsrelocation. - Checked the new shared
enroot_uri_for_image/import_squashhelper against the prior per-path logic; the FQDN-registry splitting mirrors the existing pattern inlaunch_gb200-nv.sh, but for the multinode-srt path it does change the enroot pull URI for jobs whose mainIMAGEis an FQDN registry ref (e.g. thenvcr.io/...image used bydsr1-fp4-b200-dynamo-trt), consistent with the one candidate the automated hunt already flagged. - Confirmed
AGENTS.md's new launcher-routing section and the droppedb200-nscale-compatalias inruntime_settings.shmatch the pool-to-launcher mapping inconfigs/runners.yaml.
Extended reasoning...
Overview
The PR merges runners/launch_b200-nscale-compat.sh into runners/launch_b200-nscale-slurm.sh, restoring a 1:1 pool-to-launcher mapping, and adds a routing rule to AGENTS.md. The new file selects one of three execution paths (native-srt, multinode-srt, single-node) up front and carries each old path's body over as a function, with a new shared enroot_uri_for_image/import_squash pair replacing the two files' separate squash-import logic. runners/runtime_settings.sh drops the now-dead b200-nscale-compat case alias, and a benchmark script comment is reworded to point at the surviving launcher.
Security risks
None of substance — this is CI/build-orchestration shell script for a self-hosted Slurm runner pool, not a service handling external input, auth, or secrets. The one behavior-sensitive area is which enroot/Docker registry a container image is pulled from; a wrong registry could pull an unexpected image, but that is an operational/availability risk (a broken benchmark job) rather than a security exposure, since IMAGE values come from the repo's own trusted config files, not untrusted input.
Level of scrutiny
This warrants more than a rubber stamp: it is a large (779 insertions / 770 deletions), structurally involved refactor of a production CI launcher covering three distinct execution lanes, and the author states it has not been exercised on the cluster (validation is deferred to the next live sweep). Comparing the old and new bodies line-by-line, the carried-over logic matches the PR's own description closely, and the consolidated enroot_uri_for_image helper mirrors an existing, already-used pattern from launch_gb200-nv.sh rather than being novel. However, I found one real, non-trivial behavior difference in the multinode-srt path: previously (in the compat launcher) the main IMAGE was imported via a literal docker://$IMAGE, with no FQDN-registry splitting — only the separately-injected DCGM exporter image got the nvcr.io# substitution. The new consolidated import_squash now applies enroot_uri_for_image to every image, including the main one, so an FQDN main image (e.g. the nvcr.io/nvidia/ai-dynamo/tensorrtllm-runtime image used by the live dsr1-fp4-b200-dynamo-trt config in configs/nvidia-master.yaml, which runs on cluster:b200-nscale with multinode: true and framework: dynamo-trt, landing in the multinode-srt path) now resolves to a different enroot URI than before. This is exactly the one candidate already surfaced and investigated by the automated bug hunt; I was not able to independently confirm from the repo alone whether the old or the new URI is the one that actually works against this registry, since that depends on enroot runtime behavior that isn't exercised in this sandbox. Given the change is unverified on the cluster and this ambiguity touches a real, in-use config, a maintainer with cluster access should confirm the import still succeeds for that job before merging.
Other factors
The author's own equivalence testing (840 combinations of path-selection inputs) and the AI disclosure are documented in the PR body, and the diff is broadly consistent with the described mechanical carry-over (removed dead minimaxm2.5 duplicate block, moved check_env_vars calls, one comment reword). There are no CODEOWNERS constraints evident and no outstanding third-party review objections in the timeline provided. The combination of "large, non-trivial infra refactor," "not run on the cluster," and "one plausible but unconfirmed registry-URI behavior change touching a live config" is enough that I'm deferring rather than approving, while still not raising this as a new inline finding since it echoes what the automated hunt already looked at.
runners/launch_b200-nscale-compat.sh was folded into runners/launch_b200-nscale-slurm.sh by #3270; carry this PR's dsv41flash sglang routing over to the merged launcher. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Description
There is exactly one B200 Nscale runner pool (
b200-nscale-slurm_00.._10, labelsb200-nscale/cluster:b200-nscale), and the workflows dispatch withbash ./runners/launch_${RUNNER_NAME%%_*}.sh, so the only launcher that pool ever resolves to isrunners/launch_b200-nscale-slurm.sh.runners/launch_b200-nscale-compat.sh(renamed from the retired pool'slaunch_b200-dgxc.shin #2746) was only ever reached by anexecfrom that file. Two files for one pool made it look like there were two pools, and theb200-nscale-compatalias inruntime_settings.shmatched no runner.This PR restores a 1:1 mapping between runner pools and launcher files for B200 Nscale and writes the rule down.
Changes
runners/launch_b200-nscale-slurm.sh: absorbs the compat script. The three execution paths are selected once near the top and named in the log:native-srt: the multi-node lanes whose srt-slurm recipes are maintained against this cluster (DSV4 / Kimi K2.6 / Kimi K3 / GLM-5.2 FP4, GLM-5.1 FP8 TileRT) — the old slurm launcher body.multinode-srt: every other multi-node job — the old compat multi-node body.single-node:salloc+srunof thebenchmarks/single_nodescript — the old compat single-node body.Each body is carried over verbatim as a function. The only consolidated code is the enroot squash import, which the two files had duplicated: one
import_squash/enroot_uri_for_image/ensure_writable_squash_dirnow serves both srt-slurm paths using the caller'sSQUASH_DIRandSQUASH_LOCK_TIMEOUT.SLURM_PARTITION/SLURM_ACCOUNTcome fromruntime_settings.shviacheck_env_varsinstead of being hardcoded a second time in the native path.runners/launch_b200-nscale-compat.sh: deleted.runners/runtime_settings.sh: drop theb200-nscale-compatcase alias that no runner resolves to.benchmarks/single_node/agentic/glm5.2_fp8_b200_sglang_mtp.sh: comment now names the real launcher.AGENTS.md: new "Runner launchers (one file per pool)" section stating the routing contract and forbidding fallback launchers and launcher-name aliases.Behavior preservation
uses_native_srt_lanewere run side by side over 840 combinations ofIS_MULTINODE×FRAMEWORK×MODEL_PREFIX/PRECISION×SPEC_DECODING×IS_AGENTICwith 0 mismatches (31 combinations select the native lane, as before).diffof each function against its source region in the old files shows only the squash-helper consolidation, thecheck_env_varsmoves to path entry, and one comment reword. The dead duplicatedminimaxm2.5squash-dir block and the unusedNSCALE_MODEL_ROOTconstant are gone.docker://nvcr.io#nvidia/k8s/dcgm-exporter:...reference and lock key that the old manualnvcr.io#substitution produced.bash -nandshellcheck -S warningare clean on both touched scripts (the old files had shellcheck findings).full-sweepon B200 Nscale is the real validation; a single-node job, amultinode-srtjob (for example DSR1 FP8 dynamo-sglang) and anative-srtjob (DSV4 FP4 dynamo-vllm) cover all three paths.Follow-ups outside this PR
Comparing
configs/runners.yamlpool prefixes withrunners/launch_*.sh:launch_h100-cr.shandlaunch_mi325x-tw.shhave no pool, and poolmi300x-twhas no launcher. Those should be reconciled under the same rule but are separate decisions.No benchmark performance impact, so no
perf-changelog.yamlentry.AI model disclosure
claude-fable-5-1(Claude Fable 5.1), run through Claude Code.Related Issue
N/A
Type of Change
Checklist
/use <run_id>before merging via reuse🤖 Generated with Claude Code