Refresh GLM-5.2 FP8 H200 AgentX 2P2D with MTP - #2529
Conversation
|
Thanks for the contribution! Please reach out to respective companies' CODEOWNER to fill in the latest PR_REVIEW_CHECKLIST.md before pinging core maintainer on Slack for review. In order for the signoff PR check bot to trigger, you must follow the PR_REVIEW_CHECKLIST.md template correctly, including the phrase For PR verification, add the PR authors are responsible for ensuring that after merging, all GitHub Action jobs fully pass. A lot of the time, failures are just flakes and simply re-running the failed jobs will fix it. See GitHub's docs on re-running failed jobs 感谢你的贡献！请联系相应公司的 CODEOWNER 填写最新的 PR_REVIEW_CHECKLIST.md，然后再在 Slack 上联系核心维护者进行审阅。为了触发 signoff PR 检查机器人，你必须正确遵循 PR_REVIEW_CHECKLIST.md 模板，包括保留英文语句 如需进行 PR 验证，请为此 PR 添加 PR 作者有责任确保合并后所有 GitHub Action 任务完全通过。 很多时候失败只是偶发抖动（flake），重新运行失败的任务即可解决。参见 GitHub 关于重新运行失败任务的文档 |
|
Claude finished @cquil11's task in 6m 36s —— View job PR Review: GLM-5.2 FP8 H200 AgentX 2P2D MTP refreshLGTM - no blocking issues found Verified:
Note: I could not execute |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=31219733724 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the reported issues are nits (bilingual PR description, and two DRY/duplication opportunities) with no correctness impact.
Extended reasoning...
Overview
This PR adds an MTP-enabled GLM-5.2 FP8 H200 AgentX recipe (new YAML), a corresponding nvidia-master.yaml scenario entry, a perf-changelog.yaml entry, and matching changes to runners/launch_h200-dgxc-slurm.sh: model-path routing for glm5.2/fp8, a new srt-slurm v1.0.44 clone branch for the glm5.2 agentic case, a squash-file build/import block for the glm5.2 container, and a refactor that resolves CONFIG_FILE into a stripped CONFIG_PATH used consistently for in-place YAML edits.
Security risks
None identified. No new network endpoints, auth, or credential handling. The enroot import docker:// and git clone targets are the same trusted upstreams (NVIDIA/srt-slurm) already used elsewhere in this file, just pinned to a new tag (v1.0.44). Shell quoting around the new srun ... bash -c heredoc-style block looks consistent with the pattern already used in this file's non-multinode branch.
Level of scrutiny
This is benchmark-runner infrastructure (not a production/user-facing code path), and it closely mirrors patterns already established in sibling launchers (launch_gb200-nv.sh, launch_gb300-nv.sh, launch_b200-nscale-slurm.sh, launch_b300-nv.sh all already use the same CONFIG_PATH=${CONFIG_FILE%%:*} idiom this PR introduces here). Notably, the refactor from raw $CONFIG_FILE to $CONFIG_PATH in the two sed calls actually fixes a pre-existing inconsistency in this file (previously one sed used configs/nvidia-master.yaml has a CODEOWNERS entry but runners/*.sh does not.
Other factors
The three findings from the bug-hunting pass are nits: (1) the PR title/description lacks the AGENTS.md-mandated Chinese translation, (2) the new squash-build block duplicates an existing flock+enroot pattern instead of reusing a shared helper, and (3) the new generic CONFIG_FILE staging overlaps with the pre-existing kimik3-specific recipe copy. None affect runtime correctness, and all are appropriately scoped as non-blocking cleanup suggestions.
|
|
||
| if [[ "$IS_MULTINODE" == "true" ]]; then | ||
|
|
||
| if [[ -z "${CONFIG_FILE:-}" ]]; then | ||
| echo "Error: CONFIG_FILE is not set. The srt-slurm path requires a CONFIG_FILE in additional-settings." >&2 | ||
| exit 1 | ||
| fi | ||
| CONFIG_PATH="${CONFIG_FILE%%:*}" | ||
| LOCAL_CONFIG_FILE="$GITHUB_WORKSPACE/benchmarks/multi_node/srt-slurm-recipes/${CONFIG_PATH#recipes/}" | ||
|
|
||
| # MODEL_PATH: Override with pre-downloaded paths on H200 runner | ||
| # The yaml files specify HuggingFace model IDs for portability, but we use | ||
| # local paths to avoid repeated downloading on the shared H200 cluster. |
There was a problem hiding this comment.
🟡 This PR's title and description are English-only, but AGENTS.md requires PR titles/bodies to be bilingual (title format <English> / <中文标题>, plus a ## 中文说明 section in the body). Please add a Chinese title counterpart and a Chinese translation section to the description.
Extended reasoning...
The rule. AGENTS.md line 7 explicitly states: "PR and GitHub-issue titles & descriptions must be bilingual — include a Simplified Chinese version in addition to English. Title format: <English title> / <中文标题>. In the PR/issue body, follow the English content with its Chinese translation (e.g. a ## 中文说明 section mirroring the summary...)." This is phrased as a hard requirement ("must be bilingual") and applies "to every PR and every issue" — it is not a subjective style preference, it's a mandated repo convention that CLAUDE.md points to as authoritative.
How this PR violates it. The PR title is "Refresh GLM-5.2 FP8 H200 AgentX 2P2D with MTP" — English only, with no / <中文标题> counterpart. The body contains What/Why/Impact/Checks sections, all in English, with no ## 中文说明 (or equivalent) section translating the summary into Simplified Chinese.
Why the PR's own checklist doesn't cover this. The PR's Checks section includes "Diff is English-only" — but that check item is about the code/config diff (recipe YAML, shell script, master-config keys) correctly staying in English, per the separate rule that internal config/code should not be localized. It says nothing about the PR title/description prose, so satisfying that check does not satisfy the bilingual title/body rule.
Proof by walkthrough.
- Open AGENTS.md and read line 7 — it mandates
<English title> / <中文标题>as the title format for "every PR." - Look at the actual PR title in the metadata:
Refresh GLM-5.2 FP8 H200 AgentX 2P2D with MTP. There is no/followed by a Chinese title segment. - Look at the PR body: it has
## What,## Why,## Impact,## Checks— no## 中文说明or any Chinese paragraph. - Compare to the norm already established in this repo's history: e.g. commit 8cc24c3 in the recent log is titled "...Kimi-K2.5/2.6/2.7-Code retirement / 执行 2026 年 8 月 6 日 Kimi-K2.5/2.6/2.7-Code 完全退役 ([Klaud Cold] Enact the August 6, 2026 Kimi-K2.5/2.6/2.7-Code retirement / 执行 2026 年 8 月 6 日 Kimi-K2.5/2.6/2.7-Code 完全退役 #2527)" — bilingual, as the rule requires. This PR breaks that pattern.
- Therefore the PR fails the AGENTS.md-mandated bilingual title/body convention.
Fix. Update the PR title to Refresh GLM-5.2 FP8 H200 AgentX 2P2D with MTP / 使用 MTP 刷新 GLM-5.2 FP8 H200 AgentX 2P2D, and append a ## 中文说明 section to the body mirroring the What/Why/Impact summary in Simplified Chinese.
This is a documentation/process convention issue, not a code-correctness bug — it doesn't affect runtime behavior of the shell script or recipe changes, so it should not block merge but should be fixed per repo policy.
| if [[ $MODEL_PREFIX == "glm5.2" ]] && ! unsquashfs -l "$SQUASH_FILE" >/dev/null 2>&1; then | ||
| DOCKER_IMAGE=$(echo "$IMAGE" | sed 's/#/\//g') | ||
| LOCK_FILE="${SQUASH_FILE}.lock" | ||
| mkdir -p "$(dirname "$SQUASH_FILE")" | ||
| srun --partition="$SLURM_PARTITION" --account="$SLURM_ACCOUNT" \ | ||
| --nodes=1 --ntasks=1 --time=30 --job-name="$RUNNER_NAME" \ | ||
| bash -c " | ||
| set -euo pipefail | ||
| exec 9>\"$LOCK_FILE\" | ||
| flock -w 1800 9 | ||
| if unsquashfs -l \"$SQUASH_FILE\" >/dev/null 2>&1; then | ||
| exit 0 | ||
| fi | ||
| rm -f \"$SQUASH_FILE\" | ||
| export ENROOT_CACHE_PATH=\${HOME}/.cache/enroot | ||
| mkdir -p \"\$ENROOT_CACHE_PATH\" | ||
| enroot import -o \"$SQUASH_FILE\" docker://$DOCKER_IMAGE | ||
| " | ||
| fi |
There was a problem hiding this comment.
🟡 This new multinode squash-build block re-implements the existing flock+unsquashfs-check+enroot-import sequence (already present in this same file's non-multinode else-branch, and factored into a shared import_squash() helper in sibling launchers like launch_gb200-nv.sh, launch_b200-dgxc.sh, launch_gb300-nv.sh, and launch_b200-nscale-slurm.sh) instead of reusing it. This is a pre-existing pattern in this repo (this PR is the third copy in this file), and the new copy also drops the descriptive || { echo Failed to acquire lock...; exit 1; } error message the other copies have and uses a different lock-wait (1800s vs 600s). Not a correctness blocker, but worth factoring into a shared helper to avoid further drift.
Extended reasoning...
What the duplication is: runners/launch_h200-dgxc-slurm.sh:123-141 adds a new block that acquires an flock on a per-squashfs lock file, checks whether the squashfs already exists via unsquashfs -l, and if not, removes any stale file and runs enroot import to rebuild it. This exact sequence — lock, check, remove, import — already exists in this very file's non-multinode else-branch (lines ~347-357), and is factored into a reusable import_squash() helper in several sibling launcher scripts (launch_gb200-nv.sh:266, launch_b200-dgxc.sh:206, launch_gb300-nv.sh:112, launch_b200-nscale-slurm.sh:103).\n\nWhere it diverges, and why that's a problem: the new copy is not identical to the existing in-file copy — it uses flock -w 1800 instead of flock -w 600, and critically drops the descriptive error handler || { echo 'Failed to acquire lock for ...'; exit 1; } that the other copy has. Concretely, walking through the new code at line 132 (flock -w 1800 9) versus the existing code at line 351 (flock -w 600 9 || { echo 'Failed to acquire lock for '; exit 1; }): if the new block's flock call times out after 1800 seconds without acquiring the lock, execution falls through to the subsequent commands under set -euo pipefail inside the bash -c subshell — flock's own nonzero exit under set -e will abort the subshell, but with no descriptive message, so the srun job simply fails with an opaque non-zero exit and no explanation of what went wrong (vs. the sibling copy which fails loudly with 'Failed to acquire lock for ...'). This is exactly the kind of silent drift that happens when the same logic is copy-pasted rather than shared.\n\nWhy nothing in the existing code prevents this: there's no shared helper in this file (unlike launch_gb200-nv.sh and others, which already define a local import_squash() function used from multiple call sites). Because this file inlines the pattern per-callsite, each new addition is free to subtly diverge, and lint/tests don't catch behavioral differences between near-identical shell blocks.\n\nImpact: low — this is not a correctness bug in the sense of producing wrong output; a lock timeout is a rare edge case (contention on the same squash file for over 30 minutes), and when it happens the job merely fails without a clear diagnostic message instead of failing with one. It does not block merging.\n\nSuggested fix: factor the flock+unsquashfs-check+enroot-import sequence into a local import_squash() helper within this file (mirroring launch_gb200-nv.sh), parameterized by squash file path, docker image, and lock wait time, and call it from both the multinode and non-multinode branches. This would eliminate the divergence, restore the descriptive lock-failure message in the new block, and prevent a fourth copy from drifting further in the future.
| echo "Running make setup..." | ||
| make setup ARCH=x86_64 | ||
|
|
||
| if [[ -f "$LOCAL_CONFIG_FILE" ]]; then | ||
| mkdir -p "$(dirname "$CONFIG_PATH")" | ||
| cp "$LOCAL_CONFIG_FILE" "$CONFIG_PATH" | ||
| fi |
There was a problem hiding this comment.
🟡 Nit: the new generic recipe-staging block (runners/launch_h200-dgxc-slurm.sh:195-198) duplicates the pre-existing kimik3-specific mkdir -p recipes/vllm/kimi-k3/agentic; cp -rT ... copy earlier in the script (~line 76). For kimik3 runs, both mechanisms now stage the same recipe file into the same destination — harmless but redundant.
Extended reasoning...
This PR introduces a generic single-file staging mechanism for CONFIG_FILE recipes. Lines 18-19 compute CONFIG_PATH="${CONFIG_FILE%%:*}" and LOCAL_CONFIG_FILE="$GITHUB_WORKSPACE/benchmarks/multi_node/srt-slurm-recipes/${CONFIG_PATH#recipes/}" for every IS_MULTINODE run, and lines 195-198 (if [[ -f "$LOCAL_CONFIG_FILE" ]]; then mkdir -p "$(dirname "$CONFIG_PATH")"; cp "$LOCAL_CONFIG_FILE" "$CONFIG_PATH"; fi) copy that single recipe file into the cloned srt-slurm repo after make setup. This was added specifically to support the new GLM-5.2 recipe, but the guard is generic and fires for any model whose local recipe file exists — including kimik3.
Kimi K3 already has a bespoke staging path, unchanged by this PR, right after the repo clone (around line 76): mkdir -p recipes/vllm/kimi-k3/agentic; cp -rT "$GITHUB_WORKSPACE/benchmarks/multi_node/srt-slurm-recipes/vllm/kimi-k3/agentic" recipes/vllm/kimi-k3/agentic. For a kimik3 H200 agentic run, CONFIG_FILE resolves (per configs/nvidia-master.yaml) to recipes/vllm/kimi-k3/agentic/agg-h200-tp16dp2ep32-latency-agentic.yaml, so LOCAL_CONFIG_FILE resolves to the exact same source file already staged by the kimik3-specific block, and CONFIG_PATH resolves to the exact same destination. Both mechanisms therefore copy the identical recipe file into the identical destination on every kimik3 run: one via a whole-directory cp -rT immediately after clone (staging all sibling recipes too), the other via a single-file cp keyed off CONFIG_FILE that runs after make setup.
This isn't a functional bug — the copy is idempotent and both copies produce identical file content, so nothing breaks. But it does leave two divergent, overlapping code paths doing the same job for kimik3: one broad (whole directory, pre-setup) and one narrow (single file, post-setup). Since the new generic mechanism already covers per-model recipe staging (and the PR's own design proves make setup doesn't require the recipe to be pre-staged, since GLM-5.2 relies solely on the post-setup copy), the kimik3-specific mkdir -p .../agentic; cp -rT ... staging lines are now redundant and could be removed — while keeping the surrounding kimik3-specific git clone/git checkout of the forked srt-slurm-nv repo, which is still required.
Step-by-step proof for a concrete kimik3 run:
MODEL_PREFIX=kimik3,FRAMEWORK=vllm,IS_AGENTIC=1, andCONFIG_FILE=recipes/vllm/kimi-k3/agentic/agg-h200-tp16dp2ep32-latency-agentic.yaml:...(fromconfigs/nvidia-master.yaml).- Lines 18-19 compute
CONFIG_PATH=recipes/vllm/kimi-k3/agentic/agg-h200-tp16dp2ep32-latency-agentic.yamlandLOCAL_CONFIG_FILE=$GITHUB_WORKSPACE/benchmarks/multi_node/srt-slurm-recipes/vllm/kimi-k3/agentic/agg-h200-tp16dp2ep32-latency-agentic.yaml. - The kimik3-specific
elifbranch clonesfunctionstackx/srt-slurm-nv, checks out the pinned commit, then runscp -rT "$GITHUB_WORKSPACE/benchmarks/multi_node/srt-slurm-recipes/vllm/kimi-k3/agentic" recipes/vllm/kimi-k3/agentic— this stages the target file (plus 7 siblings) intorecipes/vllm/kimi-k3/agentic/inside the cloned repo. make setupruns.- The new generic block checks
-f "$LOCAL_CONFIG_FILE"(true — the file exists), createsrecipes/vllm/kimi-k3/agentic(already exists, no-op) and runscp "$LOCAL_CONFIG_FILE" "$CONFIG_PATH", copying the exact same source file to the exact same destination path a second time. srtctl apply -f "$CONFIG_FILE"then applies the file, which is unaffected by having been copied twice.
Since every original bug source for this finding is prefixed [quality] and the issue is a DRY/dedup cleanup with no behavioral impact, this should be graded nit — worth cleaning up but not blocking merge.
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=31219733724 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=31220295579 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=31220862350 |
…200-sglang-agentic-mtp-2p2d # Conflicts: # configs/nvidia-master.yaml
Keep the 2P2D MTP topology and c8/c12/c16 sweep while removing the unsupported HiSparse and MTP combination. Rename the recipe and config key so the submission accurately describes GPU-resident decode KV cache.
Enable SGLang cache reporting on both prefill and decode workers so AIPerf captures prompt-cache read-token usage alongside all four logical-worker Prometheus endpoints.
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=31222299608 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=31224565623 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=31235207041 |
|
/stage-results 31235207041 |
|
@cquil11 staged run 31235207041: https://inferencemax-app-git-staging-semianalysisai.vercel.app/inference?i_dates=2026-08-08~r31235207041 This run remains available across future @cquil11 已将运行 31235207041 发布到预发布环境：https://inferencemax-app-git-staging-semianalysisai.vercel.app/inference?i_dates=2026-08-08~r31235207041 后续的 |
|
/reuse-sweep-run 31235207041 |
1 participant
What
Why
This creates an MTP refresh of the established H200 AgentX performance without changing its serving topology or search-space neighborhood.
Impact
Adds one new three-point H200 AgentX MTP configuration. Performance replay uses synthetic rejection sampling; evals retain real target-model verification.
Checks