chore: bump srt-slurm to v2.30.0 and drop the #504 patch / 将 srt-slurm 升级至 v2.30.0 并移除 #504 补丁 - #3549
Conversation
- Bump `utils/srt-slurm` from 8dace5f9 (#496) to v2.30.0 (0b37c791). - Delete `runners/srt-slurm/patches/504-post-eval-srun-options.patch`; NVIDIA/srt-slurm#504 is merged and included in the new pin, so applying it would fail. AI model disclosure: claude-fable-5-1 via Perplexity Computer drafted the change; reviewed and submitted by the author.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Looks good — a straightforward submodule bump and patch removal; the two flagged issues are minor documentation nits (stale version references in RECIPES.md/RECIPES_zh.md, and the PR title missing its Chinese half) rather than functional problems with the change itself.
Extended reasoning...
The diff bumps the srt-slurm submodule pointer, deletes the now-redundant #504 patch, and updates the patches README table — no application code changes, and the patch's fix is confirmed to be upstream in the target commit per the PR description. Security-sensitive surface: none (no auth, crypto, or permission logic touched). The two inline findings are documentation staleness (RECIPES.md/RECIPES_zh.md still cite the old v2.23.2 pin) and a PR-metadata compliance nit (title bilingual format), neither of which affects runtime correctness of the bump itself. Ruled-out items (perf-changelog requirement, srun_options forwarding parity) were already investigated and found not to be blocking.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
inferencex-e2e/benchmarks/multi_node/srt-slurm-recipes/RECIPES.md— nit: sweep:v2\.23\.2|8dace5f9596907a5075bf056251563b2e9563e7d— RECIPES.md:7 and RECIPES_zh.md:7 still say the shared srt-slurm pin is v2.23.2 (8dace5f9...) after this PR bumps the actual submodule to v2.30.0 (0b37c791...). Maintainers reading either doc get a wrong minimum-version/feature baseline for recipes (e.g. still believe the #504 srun_options forward isn't available) until someone notices the mismatch. Fix: update the version and commit hash in both RECIPES.md:7 and RECIPES_zh.md:7 whenever the submodule pointer changes, per the doc's own instruction to do so.Why this was flagged
RECIPES.md:7 and RECIPES_zh.md:7 hard-code 'currently v2.23.2 (8dace5f9596907a5075bf056251563b2e9563e7d)' as the shared srt-slurm submodule version. This PR moves inferencex-e2e/utils/srt-slurm to 0b37c791 (v2.30.0, per the PR description) but does not touch either doc file. The doc itself instructs 'Update that submodule pointer when upgrading' implying this line is part of that update, so a recipe author or reviewer trusting it after this merge gets a stale minimum-version claim the base didn't have (the base's doc matched the base's pin). No test enforces the doc against the actual submodule commit, so nothing catches the drift.
Verification: nit. The claim is factually correct and the mismatch is introduced by this PR. On the base branch the submodule at inferencex-e2e/utils/srt-slurm points to
8dace5f9596907a5075bf056251563b2e9563e7d, and both RECIPES.md:7 ("currently v2.23.2 (8dace5f9596907a5075bf056251563b2e9563e7d)") and RECIPES_zh.md:7 ("目前为 v2.23.2（8dace5f9...）") name that same SHA — so the docs matched…
| | Patch | Upstream PR | Fix | | ||
| |-------|-------------|-----| | ||
| | `504-post-eval-srun-options.patch` | [NVIDIA/srt-slurm#504](https://github.com/NVIDIA/srt-slurm/pull/504) | Forward recipe `srun_options` (e.g. `container-writable`) to post-eval steps | | ||
| | _(none)_ | | No patches are currently carried; the pinned submodule (v2.30.0) includes everything the runners need. | |
There was a problem hiding this comment.
🟡 (optional) This PR's title, "chore: bump srt-slurm to v2.30.0 and drop the #504 patch", is English-only, violating AGENTS.md's bilingual-title rule that every PR title MUST be <English title> / <中文标题>. No Chinese half is present anywhere in the title. Fix: rename the PR title to include a <中文标题> half per AGENTS.md's requirement before merging.
Why this was flagged
AGENTS.md states 'PR titles MUST be bilingual: every PR title MUST use / <中文标题>... An English-only title is non-compliant; fix it before requesting review or merging.' The PR title quoted in this review's Pull request block, 'chore: bump srt-slurm to v2.30.0 and drop the #504 patch', carries no Chinese half. This is a direct, checkable violation of the stated repo convention for this PR.
Verification: nit (process/convention violation, no runtime effect). AGENTS.md line 19 states the rule verbatim: "PR titles MUST be bilingual: every PR title MUST use <English title> / <中文标题>... An English-only title is non-compliant; fix it before requesting review or merging." The applicable-conventions block also lists "bilingual PR title/body requirements" as relevant to this diff. The PR title…
cquil11
changed the title
1 participant
utils/srt-slurmsubmodule from 8dace5f9 (fix: correct vLLM Router rank routing for multi-node hybrid-DP workers NVIDIA/srt-slurm#496, 2026-09-23) to the latest upstream release v2.30.0 (0b37c791).runners/srt-slurm/patches/504-post-eval-srun-options.patchand its README row. NVIDIA/srt-slurm#504 (@adibarra, forward recipesrun_optionsto post-eval jobs) merged as c9344a87 and ships in v2.28.1+, so the pin now carries it natively andgit applyof the patch would fail insetup_srt_slurm().Upstream commits picked up (8dace5f9..v2.30.0):
All runtime changes are additive (new schema fields, new launcher script, new exporter groups); no recipe schema or launcher flag was removed. No patches remain in
runners/srt-slurm/patches/.AI model disclosure: claude-fable-5-1 via Perplexity Computer drafted the change; reviewed and submitted by the author.