GLM-5.2-FP8 H200 llmd-vllm P/D disagg agentic benchmark - #2499
Conversation
Add GLM-5.2-FP8 (753B MoE) H200 benchmark configs using the llmd-vllm disaggregated P/D framework with NIXL KV transfer, matching the K8s wide-ep-lws reference deployment. Fixed-seq-len uses 1P DEP8 + 1D DEP8 (2 nodes / 16 GPUs); agentic uses 1P DEP16 + 1D DEP16 (4 nodes / 32 GPUs). Both scenarios include STP and MTP (3-token) arms. DeepGemm MoE backend, DeepEP high-throughput prefill, DeepEP v2 low-latency decode. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Elvir Crncevic <elvircrn@gmail.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Elvir Crncevic <elvircrn@gmail.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Elvir Crncevic <elvircrn@gmail.com>
|
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 关于重新运行失败任务的文档 |
1 similar comment
|
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 关于重新运行失败任务的文档 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=31015139388 |
There was a problem hiding this comment.
Deferring — the inline findings (missing --use-chat-template for the MTP arm, and the SLURM_ACCOUNT/SLURM_PARTITION export gap breaking llmd-vllm job submission) are significant enough to need human review before merge.
Extended reasoning...
Overview
This PR adds new GLM-5.2-FP8 H200 llmd-vllm P/D disagg benchmark recipes (STP and MTP arms), a new wrapper script (glm5.2_fp8_h200_llmd-vllm-disagg.sh), a new llmd-vllm framework branch in runners/launch_h200-dgxc-slurm.sh, plus config/changelog entries. This is the first PR to route a non-deepseek-v4 MTP config and the first llmd-vllm job through the H200 Slurm launcher, so it exercises several code paths for the first time.
Security risks
None of significance — this is benchmark infrastructure (recipe YAMLs, Slurm job submission scripts) with no auth, crypto, or externally-facing surface. The main risk is correctness of benchmark results rather than security.
Level of scrutiny
This warrants a real human look rather than a rubber stamp. Two of the inline findings are functional, not cosmetic: one would make the new llmd-vllm job submission path fail outright (unexported SLURM_ACCOUNT/SLURM_PARTITION not surviving the subprocess chain into submit.sh's check_env), and the other would silently produce misleading MTP acceptance-rate/throughput numbers (missing --use-chat-template on the newly-reachable non-deepseek-v4 MTP path) — which undermines the STP-vs-MTP comparison that is this PR's core purpose per AGENTS.md's explicit MTP rule.
Other factors
This is new infrastructure with no existing test coverage exercising the llmd-vllm branch end-to-end, and the PR description's own test plan lists an unchecked 'End-to-end agentic benchmark run on H200 cluster' — meaning these bugs likely have not yet been caught by a real run. Given the functional (not just stylistic) nature of two of the findings, I'm deferring rather than approving.
| --gpu-memory-utilization 0.935 | ||
| --max-model-len auto | ||
| --max-num-batched-tokens 2048 | ||
| --moe-backend deep_gemm |
There was a problem hiding this comment.
🔴 The new MTP recipe (--speculative-config {method:mtp,...} on GLM-5.2-FP8) is routed through benchmarks/multi_node/llm-d/server.sh, whose only --use-chat-template gate is MODEL_NAME == *deepseek-v4* — never true for zai-org/GLM-5.2-FP8. Per AGENTS.md:80, MTP benchmarks must pass --use-chat-template since EAGLE/MTP-style spec decoding is trained on chat-formatted prompts; without it the MTP arm will silently benchmark raw prompts and understate its acceptance-rate-dependent throughput, defeating the STP-vs-MTP comparison that is this PR's whole point.
Extended reasoning...
The bug: glm5.2-fp8-h200-mtp.yaml sets --speculative-config {"method":"mtp","num_speculative_tokens":3} on both prefill (line 58) and decode (line 77), and is dispatched via the new glm5.2_fp8_h200_llmd-vllm-disagg.sh wrapper -> submit.sh -> job.slurm -> the shared benchmarks/multi_node/llm-d/server.sh. That last file is not touched by this PR, but this PR is the first to route a non-deepseek-v4 MTP config through it, so a pre-existing gap becomes live and reachable for the first time.
Where it manifests: in server.sh the ONLY place --use-chat-template is ever added to the benchmark client invocation is inside bench_extra_args, gated solely on:
if [[ "${MODEL_NAME,,}" == *"deepseek-v4"* ]]; then
bench_extra_args+=(... --use-chat-template ...)
fiThere is no check on SPEC_DECODING/mtp anywhere in that file. run_benchmark_serving (server.sh:588-601) is called with only ${bench_extra_args[@]}, so for this recipe — where MODEL_NAME=zai-org/GLM-5.2-FP8 (set in launch_h200-dgxc-slurm.sh) — that condition is always false and bench_extra_args stays empty.
Why nothing else prevents it: --use-chat-template is a benchmark-client flag consumed by run_benchmark_serving/benchmark_lib.sh; the recipe YAML's extra-args only configure the vLLM server process, not the bench client, so glm5.2-fp8-h200-mtp.yaml has no way to supply it itself. The gating logic predates this PR (it was written for DeepSeek-V4's native MTP), but this PR is what newly exercises an MTP config for a model outside that one hardcoded string match.
Impact: AGENTS.md:80 states this exact failure mode explicitly — 'MTP scripts MUST pass --use-chat-template to run_benchmark_serving - EAGLE-style spec decoding is trained against chat-formatted inputs; benchmarking against raw prompts silently regresses acceptance rate.' The MTP arm is the specific new deliverable this PR adds (agentic-coding scenario has both an STP and an MTP spec-decoding entry in configs/nvidia-master.yaml), and it will silently report a lower, misleading acceptance rate and throughput number, undermining the STP-vs-MTP comparison that is the recipe's reason for existing.
Step-by-step proof:
- CI dispatches
glm5.2-fp8-h200-llmd-vllmwithspec-decoding: mtp, which setsCONFIG_FILE=glm5.2-fp8-h200-mtp.yamland (via the runner plumbing)SPEC_DECODING=mtp. launch_h200-dgxc-slurm.shsetsMODEL_NAME=zai-org/GLM-5.2-FP8for thisMODEL_PREFIX/PRECISIONcombo and invokes the wrapper script.- The wrapper calls
submit.sh->job.slurm->server.sh, threadingSPEC_DECODINGthrough but never consuming it for chat-template purposes. - In
server.sh:575,[[ "zai-org/glm-5.2-fp8" == *deepseek-v4* ]]evaluates false, sobench_extra_argsnever gets--use-chat-template. run_benchmark_servingruns with raw prompts against the MTP-enabled server, so measured acceptance rate/throughput for the MTP arm is understated versus what a properly chat-formatted benchmark would show.
Fix: extend the bench_extra_args gate in server.sh to also add --use-chat-template whenever SPEC_DECODING indicates MTP (e.g. || [[ "${SPEC_DECODING,,}" == "mtp" ]]), matching the AGENTS.md rule for all *_mtp.sh-equivalent paths rather than only the deepseek-v4 special case.
|
|
||
| source "$(dirname "${BASH_SOURCE[0]}")/slurm_utils.sh" | ||
|
|
||
| JOB_ID=$(bash "$BENCH_SCRIPT") |
There was a problem hiding this comment.
🔴 SLURM_PARTITION and SLURM_ACCOUNT are declared as plain (non-exported) shell variables at the top of this file, so they won't survive the new JOB_ID=$(bash "$BENCH_SCRIPT") → bash ./submit.sh subprocess chain the llmd-vllm branch introduces. submit.sh's check_env SLURM_ACCOUNT/check_env SLURM_PARTITION will fail immediately ("Error: SLURM_ACCOUNT not set"), aborting before sbatch ever runs — the entire new GLM-5.2 llmd-vllm path can't submit a job. Fix by adding export to both assignments (lines 5-6), matching launch_gb200-nv.sh/launch_gb300-nv.sh/launch_mi355x-amds.sh.
Extended reasoning...
The bug: runners/launch_h200-dgxc-slurm.sh defines SLURM_PARTITION="main" and SLURM_ACCOUNT="sa-shared" as plain shell variable assignments (lines 5-6), never exported. This was harmless until now because every existing use of these variables (the srtslurm.yaml heredoc, and the salloc call in the single-node branch) happens in the same bash process — plain variables are visible there regardless of export status.
How this PR breaks it: The new llmd-vllm branch added in this diff runs JOB_ID=$(bash "$BENCH_SCRIPT"), which forks a brand-new bash process to run benchmarks/multi_node/glm5.2_fp8_h200_llmd-vllm-disagg.sh. That wrapper script in turn runs bash ./submit.sh — a second forked process. Neither SLURM_PARTITION nor SLURM_ACCOUNT is exported by the launcher or re-exported by the wrapper (the wrapper only exports GPUS_PER_NODE, TIME_LIMIT, MODEL_PATH, MODEL_NAME, CONTAINER_IMAGE, and worker counts). Unexported shell variables are never inherited by child processes, so by the time submit.sh runs, SLURM_ACCOUNT and SLURM_PARTITION are simply unset in its environment.
Why nothing catches this today: submit.sh (in benchmarks/multi_node/llm-d/) explicitly guards against missing config via check_env SLURM_ACCOUNT and check_env SLURM_PARTITION (lines 28-29), which test ${!name:-} and exit 1 with Error: <name> not set if empty. This runs before the sbatch --partition "$SLURM_PARTITION" --account "$SLURM_ACCOUNT" call (lines 115-116), so the job submission fails at the very first check rather than silently using wrong values.
Proof by walkthrough:\n1. Launcher starts: SLURM_PARTITION="main" and SLURM_ACCOUNT="sa-shared" are set as local (non-exported) vars in the launcher's own shell.\n2. FRAMEWORK == "llmd-vllm" branch executes JOB_ID=$(bash "$BENCH_SCRIPT") — this spawns a new bash interpreter. Since the vars were never exported, the new process's environment does not contain them.\n3. Inside that new process, glm5.2_fp8_h200_llmd-vllm-disagg.sh runs bash ./submit.sh ... — another fresh fork, same problem compounds.\n4. submit.sh line 28 calls check_env SLURM_ACCOUNT, which evaluates ${!name:-} → empty, prints Error: SLURM_ACCOUNT not set, and exits 1.\n5. The launcher sees JOB_ID is empty/the subshell exited nonzero (under set -euo pipefail inside submit.sh, and the wrapper's own set -euo pipefail propagates the failure), triggering Error: failed to submit llm-d job and exit 1 in the launcher. No sbatch call is ever reached.\n\nWhy existing exports don't help: The launcher does export other variables needed downstream (MODEL_PATH, MODEL_NAME, DOCKER_IMAGE_NAME, BENCHMARK_LOGS_DIR), which is exactly the right pattern — it's just missing for SLURM_PARTITION/SLURM_ACCOUNT. The three sibling multi-node launchers (launch_gb200-nv.sh, launch_gb300-nv.sh, launch_mi355x-amds.sh) all explicitly export SLURM_PARTITION/export SLURM_ACCOUNT at the top of the file, confirming this is the established, required pattern whenever a submit.sh-style subprocess is involved.\n\nThe fix is a one-line change: add export to the two assignments at the top of launch_h200-dgxc-slurm.sh (lines 5-6), i.e. export SLURM_PARTITION="main" and export SLURM_ACCOUNT="sa-shared". This is safe for the pre-existing dynamo-sglang/dynamo-trt/single-node paths too, since they only read the values (in the same process or via the exported env for salloc), never depend on them being unexported.
| # Topologies: | ||
| # fixed-seq-len: 1P DEP8 + 1D DEP8 (p1w1d1w1, 2 H200 nodes / 16 GPUs) | ||
| # agentic: 1P DEP16 + 1D DEP16 (p1w2d1w2, 4 H200 nodes / 32 GPUs) |
There was a problem hiding this comment.
🟡 The new header comments for glm5.2-fp8-h200-llmd-vllm still document a "fixed-seq-len: 1P DEP8 + 1D DEP8" topology that was removed by commit ce3affa before this PR merged, leaving only the agentic-coding scenario. The same stale fixed-seq-len/DEP8 references appear in benchmarks/multi_node/llm-d-recipes/glm5.2-fp8-h200.yaml (lines 8-12 topology header, and line 85's "safe for DEP8 (slightly overprovisioned)" justification) — these should be trimmed to avoid misleading future readers about which scenarios actually exist.
Extended reasoning...
What the bug is
configs/nvidia-master.yaml (around lines 7888-7890) adds a comment header for the new glm5.2-fp8-h200-llmd-vllm config entry:
# Topologies:
# fixed-seq-len: 1P DEP8 + 1D DEP8 (p1w1d1w1, 2 H200 nodes / 16 GPUs)
# agentic: 1P DEP16 + 1D DEP16 (p1w2d1w2, 4 H200 nodes / 32 GPUs)
But the actual scenarios: map directly below it contains only agentic-coding, with both a STP and MTP arm — there is no fixed-seq-len scenario anywhere in the entry.
Code path / how it happened
This PR is composed of two commits: 7594c37 ("Add GLM-5.2-FP8 H200 llmd-vllm P/D disagg benchmark recipes"), which originally added both a fixed-seq-len scenario and the agentic-coding scenario, and ce3affa ("Remove fixed-seq-len DEP8 config — ISL doesnt fit at EP8"), which deleted the fixed-seq-len scenario block from the YAML because the ISL does not fit at EP8. The second commit removed the scenario definition but did not touch the comment header above it, so the header is now stale relative to the net diff this PR introduces.
The same problem exists in the newly-added recipe file benchmarks/multi_node/llm-d-recipes/glm5.2-fp8-h200.yaml:
- Lines 8-12 document
fixed-seq-len (p1w1d1w1): 1P DEP8 + 1D DEP8 ... PREFILL_NODES=1 DECODE_NODES=1as if it were an active topology. - Line 85 justifies the decode
max-num-seqs: 64/max-num-batched-tokens: 64sizing partly with "...(the primary DEP16 topology); safe for DEP8 (slightly overprovisioned)" — the DEP8 comparison point no longer exists.
Why existing code/CI does not catch this
matrix_logic and the sweep runner only read the scenarios: map — they never parse comments — so nothing about the benchmark matrix, job dispatch, or CI validation is affected. This is purely a documentation/comment accuracy issue, not a functional defect.
Step-by-step proof
- Run
git show ce3affa -- configs/nvidia-master.yaml— it deletes thefixed-seq-len:block (isl: 8192,osl: 1024,DEP8,PREFILL_NODES=1/DECODE_NODES=1,conc-list: [1, 16, 64, 128, 256]) from theglm5.2-fp8-h200-llmd-vllmentry, but the diff touches no comment lines. - Read the current state of
configs/nvidia-master.yamlat theglm5.2-fp8-h200-llmd-vllmentry: thescenarios:key has exactly one child,agentic-coding:, with the STP and MTP arms shown in this PR's diff. - Compare against the header comment two lines above
glm5.2-fp8-h200-llmd-vllm:— it still claims afixed-seq-lentopology exists ("1P DEP8 + 1D DEP8 ... 2 H200 nodes / 16 GPUs"), which is now false. - The same pattern repeats in
benchmarks/multi_node/llm-d-recipes/glm5.2-fp8-h200.yaml, added fresh by this PR (not modified afterward) — its header (lines 8-12) and inline decode-sizing comment (line 85) both reference the same removed DEP8 arm.
Impact
No functional/runtime impact — this only affects human readers of the config and recipe files, who could be misled into thinking a fixed-seq-len/DEP8 topology is configured and runnable, or who may misunderstand why max-num-seqs: 64 was chosen for decode.
Suggested fix
Update the comment header in configs/nvidia-master.yaml to only describe the agentic-coding topology, and similarly trim the benchmarks/multi_node/llm-d-recipes/glm5.2-fp8-h200.yaml header (lines 8-12) and the decode-sizing comment (line 85) to drop the now-nonexistent DEP8 reference — e.g., simply state the sizing rationale in terms of the single DEP16 topology that remains.
…im stale DEP8 comments - Export SLURM_PARTITION/SLURM_ACCOUNT so they survive the subprocess chain to submit.sh - Extend server.sh bench_extra_args to pass --use-chat-template when SPEC_DECODING=mtp (not just deepseek-v4) - Remove stale fixed-seq-len/DEP8 references from comments Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Elvir Crncevic <elvircrn@gmail.com>
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=31017098628 |
The docker engine path in job.slurm now auto-extracts epp, pd-sidecar, and envoy binaries (via extract-binaries.sh) when they are not already present, and bind-mounts them into the container. This lets vllm/vllm-openai:nightly work without a pre-built combined image. Also fix binaries.env platform detection: auto-detect x86_64 (H200) vs arm64 (GB200) instead of hardcoding arm64. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Elvir Crncevic <elvircrn@gmail.com>
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=31017891003 |
Summary
Files
benchmarks/multi_node/llm-d-recipes/glm5.2-fp8-h200.yaml— STP recipebenchmarks/multi_node/llm-d-recipes/glm5.2-fp8-h200-mtp.yaml— MTP recipebenchmarks/multi_node/glm5.2_fp8_h200_llmd-vllm-disagg.sh— wrapper scriptrunners/launch_h200-dgxc-slurm.sh— added llmd-vllm framework handlerconfigs/nvidia-master.yaml—glm5.2-fp8-h200-llmd-vllmconfig entryperf-changelog.yaml— changelog entryTest plan
🤖 Generated with Claude Code