Add GLM-5.2 RTX PRO 6000 TP/TEP AgentX sweep#2332
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 @Oseltamivir's task in 7m 39s —— View job PR Review — GLM-5.2 RTX PRO 6000 TP/TEP AgentX sweep
Summary1 blocking issue: 1 warning: the container image lives under the Everything else checked out: the benchmark script follows the repo's agentic conventions (all |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| # The low-concurrency arm keeps pure TP for interactivity; the | ||
| # high-concurrency arm uses B12X's replicated-input EP8 path to shard experts. | ||
| # Conc lists are disjoint between arms so exp-names stay unique. | ||
| glm5.2-fp4-rtx6000pro-vllm-agentic: |
There was a problem hiding this comment.
🔴 BLOCKING: Master config files were modified but perf-changelog.yaml was not updated. When changing configs/nvidia-master.yaml or configs/amd-master.yaml, you must add a corresponding entry to perf-changelog.yaml documenting the changes.
Why it matters: Per AGENTS.md, perf-changelog.yaml is what triggers which configs get benchmarked ("Add entries to configs/nvidia-master.yaml... append to perf-changelog.yaml, then validate"). Without an entry for glm5.2-fp4-rtx6000pro-vllm-agentic, the new config will not be picked up as a benchmark trigger after merge. The base branch's own MiniMax RTX config (#2306) followed this pattern with a tail entry.
Fix: Append (never insert mid-file — the file is read chronologically, oldest at top) an entry to the END of perf-changelog.yaml, e.g.:
- config-keys:
- glm5.2-fp4-rtx6000pro-vllm-agentic
scenario-type:
- agentic-coding
description:
- "Add full-context GLM-5.2 NVFP4 AgentX sweep on the 8x RTX PRO 6000 Latitude runner (TP8/DCP4, EP1 conc 1-4 and TEP8 conc 8-32, GPU-resident KV)"
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2332
| # high-concurrency arm uses B12X's replicated-input EP8 path to shard experts. | ||
| # Conc lists are disjoint between arms so exp-names stay unique. | ||
| glm5.2-fp4-rtx6000pro-vllm-agentic: | ||
| image: voipmonitor/vllm:gilded-gnosis-v20-vllm2167295-si6a92bcc-fi801d57a-cu132-20260721@sha256:0eb4b6710200e22162ede859f49a5ef4f5ff7deadcb1ee02246d1a17d2325877 |
There was a problem hiding this comment.
🟡 WARNING: Please confirm voipmonitor/vllm is a publicly pullable Docker Hub repository.
Why it matters: Every other RTX PRO 6000 config on this stack pulls from a recognized publisher (vllm/vllm-openai:... for the MiniMax-M3 recipes), and voipmonitor is the namespace of an unrelated VoIP-monitoring product — an unusual home for an SM120 vLLM stack. I could not verify pullability from the review environment. If the image is private or gets garbage-collected (as happened with vllm/vllm-router:nightly-20260629 per the changelog), every sweep on this config fails at docker run --pull=missing, and results aren't reproducible by others. The digest pin doesn't help if the repo itself isn't public.
Fix: Verify docker pull voipmonitor/vllm:gilded-gnosis-v20-vllm2167295-si6a92bcc-fi801d57a-cu132-20260721@sha256:0eb4b671... succeeds anonymously (no docker login), or re-host the image under a namespace controlled by the stack's maintainers and update this line plus the perf-changelog entry.
There was a problem hiding this comment.
Not approving — the missing perf-changelog.yaml entry means this new config won't actually get benchmarked, so it's worth a fix before merge; see inline comments for details.
Extended reasoning...
Beyond the inline findings, I also checked two other candidates and ruled them out: the voipmonitor/vllm image is an unrelated third-party Docker Hub namespace, but it's referenced by a full sha256 digest, so it's pinned and not a supply-chain risk beyond what any digest-pinned image carries. I also looked at the earlier sudo -n mkdir/chown lines in launch_rtx6000pro-lat.sh (setting up HF_HUB_CACHE_MOUNT/VLLM_JIT_CACHE_MOUNT) for the same unguarded-sudo-under-set-e concern raised for the EXIT trap — but a failure there is a normal top-level command failing under set -euo pipefail, which correctly aborts the script before any work starts, not a cleanup-trap issue that could mask a successful run.
|
|
||
| # Full-context NVIDIA GLM-5.2 NVFP4 AgentX sweep on 8x RTX PRO 6000. | ||
| # Gilded Gnosis v20 is a digest-pinned SM120 stack whose B12X sparse MLA, | ||
| # PCIe all-reduce, and DCP paths have been validated on this GPU family. | ||
| # The low-concurrency arm keeps pure TP for interactivity; the | ||
| # high-concurrency arm uses B12X's replicated-input EP8 path to shard experts. | ||
| # Conc lists are disjoint between arms so exp-names stay unique. | ||
| glm5.2-fp4-rtx6000pro-vllm-agentic: | ||
| image: voipmonitor/vllm:gilded-gnosis-v20-vllm2167295-si6a92bcc-fi801d57a-cu132-20260721@sha256:0eb4b6710200e22162ede859f49a5ef4f5ff7deadcb1ee02246d1a17d2325877 | ||
| model: nvidia/GLM-5.2-NVFP4 | ||
| model-prefix: glm5.2 | ||
| runner: cluster:rtx6000pro-lat | ||
| precision: fp4 | ||
| framework: vllm | ||
| multinode: false | ||
| scenarios: | ||
| agentic-coding: | ||
| - dram-utilization: 0.80 | ||
| search-space: | ||
| - { tp: 8, dcp-size: 4, ep: 1, dp-attn: false, kv-offloading: none, conc-list: [1, 2, 4] } | ||
| - { tp: 8, dcp-size: 4, ep: 8, dp-attn: false, kv-offloading: none, conc-list: [8, 16, 32] } |
There was a problem hiding this comment.
🔴 This PR adds a new master-config entry (glm5.2-fp4-rtx6000pro-vllm-agentic) to configs/nvidia-master.yaml but doesn't append a corresponding entry to perf-changelog.yaml. Since the changelog is what actually registers/triggers the sweep for a config, please add an entry (config-keys, description, PR link) to the end of perf-changelog.yaml before merge — otherwise this new config is defined but never benchmarked.
Extended reasoning...
configs/nvidia-master.yaml gets a brand-new top-level config block, glm5.2-fp4-rtx6000pro-vllm-agentic (lines 8138-8158), defining the model, image, runner, and the two-arm agentic-coding search space (TP8/DCP4 with EP1 for low concurrency, EP8 for high concurrency). This is a genuinely new benchmark recipe, not an edit to an existing one.
perf-changelog.yaml is unmodified — the PR's changed-file set is exactly four files (benchmark-tmpl.yml, glm5.2_fp4_rtx6000pro.sh, nvidia-master.yaml, launch_rtx6000pro-lat.sh). AGENTS.md is explicit that a master-config addition must be paired with a changelog append: line 82 ("Changes to perf-changelog.yaml trigger benchmark runs"), and line 138 under "Adding a benchmark configuration" ("Add entries to configs/nvidia-master.yaml..., append to perf-changelog.yaml, then validate"). Every recent precedent in the file's tail follows this — e.g. the minimaxm3-fp4-rtx6000pro-vllm and dsr1-fp8-b300-sglang-mtp entries each pair a master-config change with a config-keys + description + pr-link block appended to the end of perf-changelog.yaml.
This repo also has automated enforcement: .github/workflows/claude-pr-review.yml (lines 118-126) instructs the review bot to check whether configs/nvidia-master.yaml or amd-master.yaml was modified without a matching perf-changelog.yaml edit in the same PR, and if so, to post a blocking comment demanding the changelog entry. This PR will trip that exact check.
Concrete walk-through of the failure mode: the sweep-selection tooling (utils/process_changelog.py, referenced from AGENTS.md's changelog-driven trigger model) resolves which master-config keys actually get benchmarked by reading the config-keys lists out of perf-changelog.yaml entries, not by scanning nvidia-master.yaml directly. So even though glm5.2-fp4-rtx6000pro-vllm-agentic is now a syntactically valid config block, no changelog entry names it, meaning nothing selects it for the sweep. The PR carries the full-sweep-fail-fast label specifically to run this new AgentX sweep — without the changelog entry, that sweep never actually fires, defeating the purpose of the PR.
Fix: append a new entry to the very end of perf-changelog.yaml (never insert mid-file — the file is read chronologically) with config-keys: [glm5.2-fp4-rtx6000pro-vllm-agentic], a description of the new TP8/DCP4 EP1+EP8 AgentX sweep on RTX PRO 6000, and a link to this PR, matching the format of the existing tail entries and preserving exact surrounding whitespace (AGENTS.md line 176 warns CI depends on it).
| # radix cache (GPU hit 0.93->0.57 at 48->64) and thrashes on re-prefill, so it is | ||
| # strictly dominated by conc 48 on both throughput and interactivity. | ||
| - { tp: 8, ep: 8, dp-attn: true, kv-offloading: dram, kv-offload-backend: { name: hicache }, conc-list: [48], router: { name: sglang-router, version: "0.3.2" } } | ||
|
|
There was a problem hiding this comment.
🟡 The PR title 'Add GLM-5.2 RTX PRO 6000 TP/TEP AgentX sweep' is missing the required Simplified Chinese translation (' / <中文标题>'), and the PR body has no mirrored '## 中文说明' section, both mandated by AGENTS.md. Please update the title/description to add the Chinese translation before merging.
Extended reasoning...
AGENTS.md states explicitly (line 7): "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 a mandatory, explicit repo convention that applies to "every PR and every issue" — not a matter of subjective style, so it is squarely within scope for review even though it concerns PR metadata rather than the diff itself.
Proof: The PR title in the metadata is literally Add GLM-5.2 RTX PRO 6000 TP/TEP AgentX sweep — pure English, with no / <中文标题> suffix as the format requires. Scanning the PR timeline shows only the bot-generated recipe-reminder comment (which is explicitly exempted, since "bot-generated comments follow their own workflow templates") and no author-written body content at all, meaning there is no ## 中文说明 section mirroring the summary either. Both required elements — the bilingual title suffix and the mirrored Chinese body section — are absent.
The one documented exception in AGENTS.md is for the CODEOWNER sign-off template, which must stay English-verbatim so the sign-off verifier bot can match its exact phrase. That exception does not cover the PR title, so it does not excuse this omission.
Why nothing else catches this: there is no CI check enforcing bilingual titles — the sign-off bot only looks for the specific English sign-off phrase, and no other workflow in this PR (or generally) lints PR title/description language. So absent a human or review-bot catching it, a non-bilingual title/description would merge silently, which is exactly the gap this documented convention is meant to close.
Impact: this is a documentation/process-convention gap, not a functional defect — nothing in the benchmark code, CI workflow, or runner script is affected, and no build or benchmark run will fail because of it. It also costs the author very little to correct: edit the PR title to append the Chinese translation, and add a ## 中文说明 section to the body mirroring the English summary (following the terminology guidance and idiomatic-Chinese style also specified in AGENTS.md).
Fix: rename the PR title to something like Add GLM-5.2 RTX PRO 6000 TP/TEP AgentX sweep / 新增 GLM-5.2 RTX PRO 6000 TP/TEP AgentX 压测 and add a ## 中文说明 section to the PR body summarizing the change in natural technical Chinese, consistent with the style guidance in AGENTS.md (preserving hardware/framework/flag names in English).
| cleanup() { | ||
| docker rm -f "$server_name" >/dev/null 2>&1 || true | ||
| # The benchmark container runs as root and writes into the mounted | ||
| # checkout. Restore runner ownership for artifact upload and the next | ||
| # actions/checkout clean pass, including after cancellation. | ||
| sudo -n chown -R "$(id -u):$(id -g)" "$GITHUB_WORKSPACE" | ||
| } | ||
| trap cleanup EXIT |
There was a problem hiding this comment.
🟡 The new EXIT-trap cleanup in runners/launch_rtx6000pro-lat.sh (line 60) runs sudo -n chown -R $(id -u):$(id -g) $GITHUB_WORKSPACE without || true, unlike the adjacent docker rm -f ... || true. Under set -euo pipefail, a failing command in an EXIT trap overrides the script's exit status, so a transient chown failure (bad file, sudo/PAM hiccup, NFS contention) would flip an otherwise-successful multi-hour benchmark to FAILED. The identical unguarded pattern is duplicated in benchmark-tmpl.yml's resource-cleanup step (~lines 238-241, if: always()). Add || true to both to match the existing best-effort docker cleanup.
Extended reasoning...
The bug: cleanup() in runners/launch_rtx6000pro-lat.sh is registered via trap cleanup EXIT (line 62) and its last statement is:
sudo -n chown -R "$(id -u):$(id -g)" "$GITHUB_WORKSPACE"This line has no || true, while the line directly above it, docker rm -f "$server_name" >/dev/null 2>&1 || true, does. The script starts with set -euo pipefail. In bash, when errexit is active, a command that fails as the final action of an EXIT trap overrides whatever exit status the script was otherwise going to report — including an explicit exit 0 or the natural fall-through success of the script body. This was independently verified experimentally by multiple reviewers: a minimal repro (set -euo pipefail; trap 'false' EXIT; exit 0) exits with status 1, not 0.
Trigger path: benchmark-tmpl.yml's "Launch job script" step runs bash ./runners/launch_${RUNNER_NAME%%_*}.sh under Actions' default bash -eo pipefail. If docker run for the vLLM/SGLang server completes successfully (benchmark passed, results written), the script proceeds to its normal exit, firing the cleanup trap. If the recursive chown -R over the mounted $GITHUB_WORKSPACE then fails for any reason — a file the container still holds open, an unchownable path, a transient sudo/PAM denial, NFS/lock contention on a shared filesystem — the trap's exit code (non-zero) becomes the script's final exit code. The "Launch job script" step then fails, and the whole multi-hour GPU benchmark job is reported red in CI even though the benchmark itself succeeded.
Why nothing else prevents this: The script has no other exit-status protection for this specific line. The comment above it ("Restore runner ownership... including after cancellation") confirms the intent is best-effort cleanup, not a correctness-critical step — exactly the category the neighboring || true on the docker-cleanup line already covers. The chown line was simply not given the same treatment when it was added.
Step-by-step proof:
set -euo pipefailis active (line 2).trap cleanup EXITis registered (line 62);cleanup()'s last statement is the unguarded chown.docker run ...(the benchmark) executes and exits 0 — the run is a success.- The script reaches the end of its body with implicit exit status 0, which fires the EXIT trap.
- Inside
cleanup(),docker rm -f ... || truealways "succeeds" (guarded). sudo -n chown -R "$(id -u):$(id -g)" "$GITHUB_WORKSPACE"runs. Suppose it hits ENOENT on a file removed mid-traversal by the just-exited container, or a transient sudo/PAM error — it returns non-zero.- Because this is the last command in the trap and errexit is active, bash sets the script's final exit status to that non-zero value, discarding the success from step 3-4.
- The GitHub Actions "Launch job script" step (running under
bash -eo pipefail) sees a non-zero exit and marks the step — and therefore the whole job — as FAILED, despite the benchmark having actually completed successfully.
Duplication: The identical unguarded sudo -n chown -R "$(id -u):$(id -g)" "$GITHUB_WORKSPACE" also appears in the reused resource-cleanup YAML anchor in .github/workflows/benchmark-tmpl.yml (~lines 238-241), used by the "Resource cleanup (post-run)" step which runs with if: always(). There, a chown failure fails that step directly regardless of whether the benchmark step passed, which can turn an otherwise-green job red at cleanup time.
Fix: Append || true to both occurrences, matching the pre-existing best-effort pattern used for docker rm -f:
sudo -n chown -R "$(id -u):$(id -g)" "$GITHUB_WORKSPACE" || trueOn severity: This is not a guaranteed failure on every run — the chown will usually succeed since it's run via passwordless sudo. But when it does fail, the cost is a multi-hour GPU benchmark job spuriously marked FAILED and requiring a manual rerun, which is a real CI-correctness regression introduced by this PR (the ownership-repair logic is new). Given the low-but-nonzero probability and the trivial one-line fix, this is best treated as a robustness nit rather than a blocking issue.
05378be to
e7335d2
Compare
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=30166656890 |
1 participant
No description provided.