Skip to content

feat(srt): check srt-slurm recipes against master images before sweep dispatch / feat(srt):在派发 sweep 前检查 srt-slurm 配方与主配置镜像是否一致 - #3624

Open
chunfangamd wants to merge 1 commit into
mainfrom
chun/srt-recipe-preflight
Open

chunfangamd wants to merge 1 commit into
mainfrom
chun/srt-recipe-preflight

Conversation

@chunfangamd

Copy link
Copy Markdown
Collaborator

Description

Follow-up to #3567; the two PRs belong together. #3567 aligns the two srt-slurm recipes whose container had drifted from the master-config image. This PR adds the check that would have caught the drift: each sweep now binds every planned srt-slurm point to the recipe variant it would launch, and stops before dispatching any job if a point doesn't bind.

Problem. An srt-slurm point names its container in two places: the master config's image and the recipe's model.container. An image bump has to edit both, as #3446 did, but nothing compares them before benchmark jobs start.

#3567 first added a pytest scan for this. Review found four problems with it:

  • It couldn't gate run-sweep, which is a separate workflow.
  • It compared image sets per recipe instead of the variant each point selects, so it missed an image swap inside the shared Qwen3.5 H200 recipe.
  • It skipped EVAL_CONFIG_FILE.
  • It pinned checked-in config, against the AGENTS.md test rules.

#3567 dropped the scan, and this PR replaces it.

Changes

  1. Validator. infx/srt_slurm/preflight.py (python -m infx.srt_slurm.preflight) reads the matrix on stdin. If every srt-slurm point binds, it writes the matrix unchanged to stdout; otherwise it prints each problem once with the points it affects and exits 1.
    • Single-node: it builds the environment that benchmark-tmpl.yml exports for the point and calls the runtime's own select_recipe. Exactly one variant must match on engine, model, image, precision, parallelism, GPU count, speculative decoding, concurrency and KV offloading. The check runs per point, so a recipe shared by keys with different images is checked against each key's image.
    • Multi-node: it expands CONFIG_FILE and EVAL_CONFIG_FILE with srtctl's own override expansion. Each variant's model.container must be the point's image (in either registry spelling) or a container alias that configs/runners.yaml defines for the runner's cluster. A declared identity.container.image must name the same image. TileRT points are skipped because they pair their own containers.
  2. Workflows.
    • run-sweep.yml: setup initializes the srt-slurm submodule and runs the validator after benchmark_schema --plan, before ci_priority. If the validator fails, setup fails, so canary-select and the benchmark jobs, which all require a successful setup, never start.
    • e2e-tests.yml (which trusted-external-sweep.yml and claude.yml also dispatch) and profile.yml: the trusted tooling's validator checks the measured tree's recipes, runner config and srt-slurm submodule. The step is skipped when either tree predates the module, so measuring an older revision is unaffected.
  3. Tests. infx/tests/srt_slurm/test_preflight.py uses small temporary recipes and runner inventories, per the AGENTS.md test rules. It covers:
    • a one-sided image update through the CLI;
    • a variant swap in a shared recipe;
    • an EVAL_CONFIG_FILE mismatch;
    • configured and misspelled aliases, and both registry spellings;
    • runner labels, pools and bare cluster ids;
    • TileRT points and a missing recipe.

A sweep that selects the B200 key without #3567's fix stops at setup with:

srt-slurm recipe preflight found 1 problem(s) affecting 13 point(s):
  srt-recipe=benchmarks/single_node/srt-slurm-recipes/dsv4/sglang/b200-fp4-mtp/agentic.yaml: Expected exactly one matching single-node SRT recipe; override_dep8_hicache_c128: Single-node SRT image: recipe/matrix 'lmsysorg/sglang:v0.5.19-cu130' != 'lmsysorg/sglang:v0.5.20-cu130'; ...
    - dsv4_tp8_conc1_kvnone_spec-draft_model on cluster:b200-nscale
    ...
    - dsv4_tp8_conc160_kvdram-hicache_spec-draft_model on cluster:b200-nscale | eval-only

Validation

  • test_preflight.py: 14 passed. Each of eight deliberate regressions in the validator fails at least one test:
    • skipping EVAL_CONFIG_FILE, single-node points, or the alias lookup for bare cluster ids;
    • ignoring identity.container.image, or rejecting the pyxis spelling;
    • checking TileRT points, or accepting any container;
    • dropping the fallback for an override-format recipe without overrides.
  • ci.yml commands, run locally: Lint is clean, and Tests has 2,349 passed. Two test_slurm_cli cases fail only on the test host because it has a real sacct; they fail the same way on clean main.
  • Full generated matrix (1,712 points, about 10 s): 3 problems affecting 35 points, with no false positives.
  • run-sweep setup replayed in a throwaway worktree with the workflow's exact commands under bash -e:

Notes for reviewers

  • Please merge fix(srt): align B200 and MI355X recipe images with master configs / fix(srt):对齐 B200 与 MI355X 配方镜像与主配置 #3567 first. Until it merges, a sweep that selects either of its keys fails at setup; it would fail at runtime anyway.
  • glm5.2-fp8-mi325x-sglang-agentic-mtp still drifts on main, so a sweep that selects it fails at setup until its recipe is fixed. Only the points a sweep plans are checked, so drift in other keys doesn't block unrelated PRs.
  • single_node_environment mirrors how benchmark-tmpl.yml exports matrix fields to the job. A change to that mapping needs the same change here.
  • run-sweep only triggers on PRs that edit perf-changelog.yaml, so this PR's own checks don't run the new setup step. The replay above stands in for that.
  • There is no perf-changelog.yaml entry, because no benchmark config or recipe changes.
  • All changed files are owned by @SemiAnalysisAI/core.

AI model disclosure

Related Issue

No issue. Follow-up to #3567. Related: #3428, #3446.

Type of Change

  • Bug fix
  • New feature
  • Configuration change
  • Documentation update
  • Other (please describe)

Checklist

  • I have completed the AI model disclosure and kept it current
  • I have tested my changes locally
  • I have updated documentation if necessary
  • For every change that can affect benchmark performance and every recipe addition or modification, I have appended a new entry to the physical end of inferencex-e2e/perf-changelog.yaml and have not edited historical entries
  • Before merging via reuse, an authorized maintainer (OWNER/MEMBER/COLLABORATOR) has commented /use <run_id> (or the legacy /reuse-sweep-run) on this PR. Do this only once there is a final full sweep that is all green with evals passing, since after this comment the sweep label will no longer automatically kick off new sweeps. Remove and re-add the label to force one.
中文

改动说明

本 PR 是 #3567 的后续,两者是一个整体。#3567 修正了两个 container 与主配置 image 不一致的 srt-slurm 配方;本 PR 加上本可以发现这种不一致的检查:每次 sweep 都会把每个计划中的 srt-slurm 点绑定到它实际会启动的配方 variant,任何一个点绑定失败,就在派发任何任务之前停止。

问题: srt-slurm 点在两个地方声明 container:主配置的 image 和配方的 model.container。升级镜像必须同时修改两处(如 #3446),但在 benchmark 任务开始之前,没有任何检查比较这两处。

#3567 最初为此加了一个 pytest 扫描。Review 指出它有四个问题:

  • 它无法拦住独立的 run-sweep workflow。
  • 它按配方比较镜像集合,而不是每个点实际选中的 variant,因此漏掉了共享的 Qwen3.5 H200 配方内部的镜像交换。
  • 它没有检查 EVAL_CONFIG_FILE。
  • 它在测试里固定了 checked-in 配置,违反 AGENTS.md 的测试规则。

#3567 已移除该扫描,由本 PR 取代。

改动:

  1. Validator: infx/srt_slurm/preflight.py(python -m infx.srt_slurm.preflight)从 stdin 读入 matrix。所有 srt-slurm 点都能绑定时,原样输出到 stdout;否则每个问题只打印一次并列出受影响的点,然后以退出码 1 结束。
    • 单节点: 按 benchmark-tmpl.yml 为该点导出的环境变量,调用运行时自己的 select_recipe。必须在引擎、模型、镜像、精度、并行配置、GPU 数、投机解码、并发和 KV offloading 上恰好匹配一个 variant。由于逐点检查,被多个不同镜像的 key 共享的配方会分别对照每个 key 的镜像。
    • 多节点: 用 srtctl 自己的 override 展开逻辑展开 CONFIG_FILE 和 EVAL_CONFIG_FILE。每个 variant 的 model.container 必须是该点的镜像(两种 registry 写法均可),或 configs/runners.yaml 为该 runner 所在 cluster 配置的 container alias。声明了 identity.container.image 时,它必须是同一个镜像。TileRT 点使用自己配对的 container,因此跳过。
  2. Workflow:
    • run-sweep.yml:setup 先初始化 srt-slurm submodule,在 benchmark_schema --plan 之后、ci_priority 之前运行 validator。validator 失败时 setup 失败,canary-select 和所有要求 setup 成功的 benchmark 任务都不会启动。
    • e2e-tests.yml(trusted-external-sweep.yml 和 claude.yml 也通过它派发)和 profile.yml:用可信工具树里的 validator 检查被测代码树的配方、runner 配置和 srt-slurm submodule。任一棵树还没有这个模块时跳过该步骤,因此测量旧版本不受影响。
  3. 测试: infx/tests/srt_slurm/test_preflight.py 按 AGENTS.md 的测试规则,只使用临时目录中的小型配方和 runner 配置,覆盖:
    • 通过 CLI 的单边镜像更新;
    • 共享配方中的 variant 交换;
    • EVAL_CONFIG_FILE 不一致;
    • 已配置和拼错的 alias,以及两种 registry 写法;
    • runner label、pool 和裸 cluster id;
    • TileRT 点和缺失的配方。

上方的示例输出,是一个选中 B200 key、但没有 #3567 修复的 sweep 在 setup 停止时打印的内容。

验证:

  • test_preflight.py:14 个通过。对 validator 故意做的 8 种破坏,每一种都会让至少一个测试失败:
    • 跳过 EVAL_CONFIG_FILE、单节点点,或裸 cluster id 的 alias 查找;
    • 忽略 identity.container.image,或不接受 pyxis 写法;
    • 检查 TileRT 点,或接受任意 container;
    • 去掉对没有 override 的 override 格式配方的兜底。
  • 在本地运行 ci.yml 的命令:Lint 干净,Tests 2,349 个通过。两个 test_slurm_cli 用例只在测试机上失败,因为它装有真实的 sacct;在干净的 main 上同样失败。
  • 完整生成的 matrix(1,712 个点,约 10 秒):3 个问题影响 35 个点,没有误报。
  • 在一次性 worktree 中用 workflow 的原样命令和 bash -e 重放 run-sweep 的 setup:

审阅注意事项:

  • 请先合入 fix(srt): align B200 and MI355X recipe images with master configs / fix(srt):对齐 B200 与 MI355X 配方镜像与主配置 #3567。在它合入之前,选中其两个 key 的 sweep 会在 setup 失败;这些 sweep 在运行时本来也会失败。
  • glm5.2-fp8-mi325x-sglang-agentic-mtp 在 main 上仍不一致,选中它的 sweep 在其配方修复之前会在 setup 失败。validator 只检查 sweep 计划中的点,其他 key 的不一致不会挡住无关的 PR。
  • single_node_environment 复刻了 benchmark-tmpl.yml 把 matrix 字段导出给任务的方式;那边的映射改动时,这里也要同步修改。
  • run-sweep 只在修改 perf-changelog.yaml 的 PR 上触发,因此本 PR 自己的检查不会运行新的 setup 步骤,由上面的重放代替。
  • 没有 perf-changelog.yaml 记录,因为本 PR 不改任何 benchmark 配置或配方。
  • 所有改动文件都归 @SemiAnalysisAI/core。

AI 模型使用说明

关联 issue

无。#3567 的后续;相关 PR:#3428、#3446。

改动类型

新功能。

…atch

run-sweep, e2e-tests and profile now pipe their matrix through infx.srt_slurm.preflight after benchmark_schema. Each single-node point must select exactly one variant through the runtime's select_recipe, and every CONFIG_FILE and EVAL_CONFIG_FILE recipe a multi-node point can launch must name a container the job's srt-slurm config resolves to the point's image. A mismatch now fails setup before any canary or benchmark job is dispatched.

Co-authored-by: Cursor <[email protected]>
@chunfangamd
chunfangamd requested a review from a team October 1, 2026 05:51
Comment on lines +334 to +339
if [ -f "$PRIORITY_ROOT/infx/srt_slurm/preflight.py" ] && [ -f "$MEASURED_ROOT/infx/srt_slurm/preflight.py" ]; then
git -C "$MEASURED_ROOT" submodule update --init utils/srt-slurm
CONFIG_JSON=$(printf '%s' "$CONFIG_JSON" | env PYTHONPATH="$PRIORITY_ROOT:$MEASURED_ROOT/utils/srt-slurm/src" \
uv run --no-project --exclude-newer PT12H --python 3.12 --with pydantic --with pyyaml \
--with marshmallow --with marshmallow-dataclass --with ruamel.yaml --with requests \
python -P -m infx.srt_slurm.preflight --root "$MEASURED_ROOT")

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.

🔴 This runs the "trusted" preflight check with srtctl imported from the PR's own utils/srt-slurm submodule checkout, letting a malicious PR execute code inside the trusted-tooling process instead of only the sandboxed measured tree. check_multi_node -> selected_recipes does from srtctl.core.config import generate_override_configs, and PYTHONPATH is $PRIORITY_ROOT:$MEASURED_ROOT/utils/srt-slurm/src (e2e-tests.yml:336) while only $MEASURED_ROOT ever gets git submodule update --init utils/srt-slurm (line 335) — $PRIORITY_ROOT never does, so srtctl can only resolve from the PR-controlled submodule commit. -P doesn't block PYTHONPATH imports. Fix: give PRIORITY_ROOT its own trusted srt-slurm checkout (or vendor/pin the srtctl it imports) so the trusted interpreter never imports a package whose source lives only in the untrusted MEASURED_ROOT tree; same pattern in profile.yml:111-116.

Why this was flagged

The whole point of checking out .ci-priority/PRIORITY_ROOT separately from MEASURED_ROOT in this job is to run decision logic with code the PR cannot modify. This diff defeats that: infx.srt_slurm.preflight (loaded from PRIORITY_ROOT) calls infx.srt_slurm.synthetic_acceptance.selected_recipes, which lazily imports srtctl.core.config.generate_override_configs; that package is only ever present via $MEASURED_ROOT/utils/srt-slurm/src (e2e-tests.yml:335-336), which is the PR's own branch/submodule pointer. A PR that points its utils/srt-slurm submodule gitlink at a malicious commit gets that code executed inside the trusted PRIORITY_ROOT python process when get-jobs runs (same for profile.yml:111-116). python -P only disables automatic sys.path prepending, not PYTHONPATH, so this import is not blocked. The output of this step feeds CONFIG_JSON/job outputs that downstream benchmark-dispatch jobs (which do use secrets.INFERENCEX_OFFICIAL_RO_HF_TOKEN, secrets.MODAL_TOKEN_ID/SECRET) consume, so compromising this step can also poison what those privileged jobs run.

Verification: e2e-tests.yml:335 runs git -C "$MEASURED_ROOT" submodule update --init utils/srt-slurm (PR-controlled submodule), and :336 sets PYTHONPATH="$PRIORITY_ROOT:$MEASURED_ROOT/utils/srt-slurm/src", so srtctl resolves only from the measured tree.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant