perf: add GB200 Qwen3.5 FP4 SGLang MTP AgentX - #2568
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 关于重新运行失败任务的文档 |
cquil11
added
the
agentx-fast
|
Claude finished @cquil11's task in 7m 58s —— View job Review of PR #2568
SummaryOne 🟡 WARNING posted inline: all four new recipes omit Everything else checks out: One pre-existing observation, not caused by this PR: the inter-concurrency drain gate in Note: I could not externally verify that srt-slurm v1.0.45 ships |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| WEKA_LOADER_OVERRIDE: semianalysis_cc_traces_weka_062126_256k | ||
| AIPERF_DATASET_MMAP_CACHE_DIR: /aiperf_mmap_cache | ||
| HF_HUB_CACHE: /hf_hub_cache | ||
| AIPERF_REQUIRED_SERVER_METRIC_PREFIX: "sglang:" |
There was a problem hiding this comment.
🟡 WARNING: Missing the AIPerf dynamo conv-aware-routing opt-out that every other SGLang agentic recipe sets (applies to all 4 new recipes in this PR).
Why it matters: With FRAMEWORK=dynamo-sglang and neither AIPERF_USE_DYNAMO_CONV_AWARE_ROUTING=0 nor AIPERF_HTTP_X_DYNAMO_SESSION_ID_FROM_CORRELATION_ID=true set, benchmarks/benchmark_lib.sh:1982-1991 defaults to appending --use-dynamo-conv-aware-routing, which makes AIPerf emit nvext.session_control bind/close actions in request bodies. That field targets the dynamo frontend's KV router — these recipes use frontend: { type: sglang }, which doesn't implement it (best case it's silently ignored payload noise; the same mechanism 400s on current dynamo builds per the comment in benchmark_lib.sh). Every existing SGLang agentic recipe (deepseek-v4, glm5.2, and the GB300 qwen3.5 siblings these recipes mirror, e.g. agg-gb300-tp2-c1-mtp-hicache-jid2530006.yaml:89-90) explicitly opts out.
Fix: Mirror the GB300 siblings in all four recipes:
| AIPERF_REQUIRED_SERVER_METRIC_PREFIX: "sglang:" | |
| AIPERF_REQUIRED_SERVER_METRIC_PREFIX: "sglang:" | |
| AIPERF_HTTP_X_DYNAMO_SESSION_ID_FROM_CORRELATION_ID: "true" | |
| AIPERF_USE_DYNAMO_CONV_AWARE_ROUTING: "0" |
There was a problem hiding this comment.
Looks good — the two findings are wording/log-convention nits (bare exception instead of sys.exit, and a stale vLLM-specific error message) in the new sglang synthetic-acceptance backend, not correctness issues.
Extended reasoning...
Overview
This PR adds a new GB200 Qwen3.5-397B-A17B NVFP4 SGLang MTP AgentX submission: four new recipe YAMLs, a new configs/nvidia-master.yaml search-space entry, a perf-changelog entry, a new branch in runners/launch_gb200-nv.sh that clones a pinned NVIDIA/srt-slurm v1.0.45 (with commit-hash verification), and a new runners/synthetic_injectors/sglang.py backend that rewrites *_environment: blocks to inject golden synthetic-acceptance env vars for throughput-only runs.
Security risks
None material. The new shell branch clones a pinned tag and verifies the resolved commit SHA before proceeding (matching the existing pattern used for other pinned srt-slurm/producer clones in this file), and the injector only operates on local recipe YAML text via regex substitution — no new user-controlled input or shell-injection surface.
Level of scrutiny
This follows the repo's well-established pattern for adding a new model+hardware AgentX recipe (mirrors prior Kimi K3 / GLM-5.2 / DSV4 additions in recent history), so moderate scrutiny is appropriate. The two reported issues are both nit-level: a bare ValueError on an unreachable double-injection guard (rather than the driver's sys.exit convention), and the shared driver's log wording still saying "speculative-config entries" for the sglang backend's environment-block injections. Neither affects correctness — the actual injection logic (regex matches on the 2-space aggregated_environment: blocks present in all four new recipes) is exercised correctly, and the explicit SYNTHETIC_ACCEPTANCE_LENGTH=3.39 setting means the AL auto-lookup / spec_tokens_from_recipe path (flagged and ruled out by the bug hunt as reading the wrong SGLang field) isn't even reached in this PR's usage.
Other factors
PR description reports recipe dry-runs, targeted srt-slurm/matrix-generator test passes, and verified synthetic-injection / EVAL_ONLY no-op behavior. The shell script changes follow the same commit-pinning and cleanup conventions already used elsewhere in launch_gb200-nv.sh.
| def rewrite(content, al, log): | ||
| """Add throughput-only golden-acceptance variables to each worker role.""" | ||
| if "SGLANG_SIMULATE_ACC_LEN" in content: | ||
| raise ValueError("recipe already contains SGLANG_SIMULATE_ACC_* variables") | ||
|
|
There was a problem hiding this comment.
🟡 In runners/synthetic_injectors/sglang.py (lines 18-22), rewrite() raises a bare ValueError on the double-injection guard instead of following the driver's error convention. inject_synthetic_acceptance.py:153 calls injector.rewrite() with no try/except, and every other failure path in that driver (and the sibling vllm.py backend design) reports errors via sys.exit("ERROR: ...") for a clean one-line CI message — this is the one path that lets a raw Python traceback escape into CI logs instead.
Extended reasoning...
The bug: sglang.py's rewrite() raises raise ValueError("recipe already contains SGLANG_SIMULATE_ACC_* variables") (line 21) when the double-injection guard trips. This is the only error-signaling path in the new backend, and it is a bare Python exception rather than the driver's established error convention.
Code path: inject_synthetic_acceptance.py's inject() calls injector.rewrite(content, al, _log) at line 153 with no try/except around it, and main() never wraps the call to inject() either — the exception is free to propagate all the way up to sys.exit(main(sys.argv)) at the bottom of the module. Every other failure branch in this same driver — missing AL lookup table entry (line 78), unresolvable YAML/model key/spec-token count (lines 95, 108, 118), missing injector or zero rewrites (lines 137, 156) — instead calls sys.exit("ERROR: ..."), which argparse/Python prints as a single clean line plus a non-zero exit code, no traceback.
Why nothing prevents it: the guard exists specifically to catch a should-never-happen state (a recipe that has already been synthetically injected), so it's rarely exercised, and no test or caller currently wraps injector.rewrite() defensively. There's nothing in the driver or in runners/synthetic_injectors/__init__.py's backend contract that requires backends to signal errors via return value rather than exception, so the new sglang backend silently diverges from the pattern every sibling error path follows.
Impact: if this guard ever fires (e.g., a recipe accidentally gets the injection step run twice, or a bad rerun/retry reuses an already-patched CONFIG_FILE), the CI job dies with a raw multi-frame Python traceback instead of the driver's usual one-line ERROR: ... message, breaking the convention that makes these failures easy to grep out of CI logs. Functionally the run still fails (which is correct), so this is a log-cleanliness/consistency issue, not a correctness bug — hence nit severity, matching all three verifiers' independent assessment.
Step-by-step proof:
- A recipe's
aggregated_environment:block already containsSGLANG_SIMULATE_ACC_LEN(e.g., because the injection step ran once and, for whatever reason, ran again on the same file). - The CI/launcher script invokes
python3 inject_synthetic_acceptance.py $CONFIG_PATH $FRAMEWORK. main()resolves the injector viaget_injector("dynamo-sglang"), which returns thesglangmodule, then callsinject(...)which callsinjector.rewrite(content, al, _log)at line 153.- Inside
rewrite(), the checkif "SGLANG_SIMULATE_ACC_LEN" in content:at line 20 is true, so line 21 executesraise ValueError(...). - Nothing catches it —
inject()has no try/except around the call,main()has no try/except aroundinject()— so the exception propagates to the top-levelsys.exit(main(sys.argv)), which Python renders as an uncaught exception: a full traceback is printed to stderr and the process exits with code 1, instead of the driver's cleanERROR: recipe already contains SGLANG_SIMULATE_ACC_* variablesline that every other failure mode produces.
Fix: either catch the ValueError in inject() (or in main()) and re-raise via sys.exit(f"ERROR: {exc}"), or have rewrite() itself return (content, 0) and let the driver's existing "count == 0" sys.exit(...) branch (line 156) handle it uniformly — the latter is more consistent with how the sibling vllm.py backend never raises at all.
|
|
||
| def rewrite(content, al, log): | ||
| """Add throughput-only golden-acceptance variables to each worker role.""" | ||
| if "SGLANG_SIMULATE_ACC_LEN" in content: | ||
| raise ValueError("recipe already contains SGLANG_SIMULATE_ACC_* variables") | ||
|
|
||
| variables = ( | ||
| f'\n SGLANG_SIMULATE_ACC_LEN: "{al:g}"' | ||
| '\n SGLANG_SIMULATE_ACC_METHOD: "match-expected"' | ||
| '\n SGLANG_SIMULATE_ACC_TOKEN_MODE: "real-draft-token"' | ||
| ) | ||
| rewritten, count = _ENV_BLOCK_RE.subn(lambda match: match.group(1) + variables, content) | ||
| if count: | ||
| log(f"Added SGLANG_SIMULATE_ACC_* to {count} worker environment block(s)") |
There was a problem hiding this comment.
🟡 The shared driver's diagnostics ('no speculative-config entries were found' / 'Modified {count} speculative-config entries' in runners/inject_synthetic_acceptance.py:155-163) hardcode vLLM-specific 'speculative-config' wording, but the new sglang backend (runners/synthetic_injectors/sglang.py) rewrites aggregated/prefill/decode_environment blocks — nothing to do with speculative-config JSON. This wording fires on every sglang injection, not just the count==0 error case, and could misdirect debugging. Consider generalizing to backend-neutral phrasing (e.g. 'injectable entries') in the shared driver.
Extended reasoning...
runners/inject_synthetic_acceptance.py is written as a framework-agnostic driver (per its own module docstring) that delegates the actual recipe rewrite to a backend registered in runners/synthetic_injectors/. However, its two diagnostic messages still bake in the original vLLM backend's terminology:
if count == 0:
sys.exit(
"ERROR: SYNTHETIC_ACCEPTANCE=true but no speculative-config "
f"entries were found in {config_file}"
)
...
_log(f"Modified {count} speculative-config entries")This PR wires in the new sglang backend via runners/synthetic_injectors/__init__.py (from . import sglang, vllm), and sglang.py's rewrite() derives count from _ENV_BLOCK_RE.subn(...), where _ENV_BLOCK_RE = re.compile(r"(?m)^( (?:aggregated|prefill|decode)_environment:\s*)$"). That regex matches YAML environment-variable blocks, not speculative-config JSON entries (that term belongs solely to the vLLM backend's _SPEC_CONFIG_RE). So the driver's wording no longer describes what the sglang path actually modified.
Step-by-step proof:
- A GB200 Qwen3.5 SGLang recipe (e.g.
agg-gb200-tp4-mtp.yamladded in this PR) setsSYNTHETIC_ACCEPTANCE=trueandFRAMEWORK=dynamo-sglangviaadditional-settingsinconfigs/nvidia-master.yaml. launch_gb200-nv.shinvokesinject_synthetic_acceptance.py "$CONFIG_PATH" "$FRAMEWORK".- The driver resolves the sglang backend via
get_injector("dynamo-sglang")and callssglang.rewrite(content, al, log). rewrite()matches the recipe'saggregated_environment:block with_ENV_BLOCK_RE, injectsSGLANG_SIMULATE_ACC_*env vars, and returnscount=1.- Back in the driver, since
count != 0, it logs"Modified 1 speculative-config entries"— inaccurate on every successful run, since what was actually modified was an environment-variable block, not a speculative-config entry. - If a future sglang recipe's env-block key or indentation ever drifted from the exact 2-space
_ENV_BLOCK_REpattern (e.g. a differently indented YAML style),countwould be0and the driver would exit with"no speculative-config entries were found in {config_file}"— pointing whoever is debugging toward the wrong recipe section (speculative-config JSON, which doesn't exist for sglang) instead of the actual environment-block regex mismatch.
This is purely a diagnostic-text accuracy issue — I confirmed both messages are reachable (the success message on literally every current sglang injection since all four new recipes have a 2-space aggregated_environment: block that matches count≥1, and the error message only on the more speculative future-drift scenario). Nothing about the injection logic itself is wrong, and the error message does include {config_file}, so a developer isn't completely lost — but the phrase "speculative-config entries" actively points at the wrong artifact type for sglang, since sglang recipes have no speculative-config JSON section at all.
The fix is simple: since inject_synthetic_acceptance.py is explicitly designed to be framework-agnostic (per its own docstring), generalize the wording in both messages to something backend-neutral, e.g. "no injectable entries were found" and "Modified {count} injectable entries", so the message stays accurate regardless of which backend (vLLM speculative-config, sglang env blocks, or a future trtllm backend) produced the count.
One verifier argued the term is defensible because both backends inject "speculative-acceptance configuration," just in different locations, and that the count==0 trigger requires a hypothetical future indentation drift. That's a fair point for the error path, but it doesn't address the success-path message, which is inaccurate on every single sglang run today, not just a hypothetical drift scenario — so this is a real (if low-severity) wording bug in the current code, not purely speculative.
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=31540458110 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=31563972895 |
2 similar comments
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=31563972895 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=31563972895 |
cquil11
removed
full-sweep-enabled
agentx-fast
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=31582035733 |
1 participant
Summary
Adds a new GB200 Qwen3.5-397B-A17B NVFP4 SGLang MTP AgentX submission using stable SGLang v0.5.17-cu130 and NVIDIA srt-slurm v1.0.45.
The complete agentx-fast discovery matrix mirrored the useful B200/B300 single-node search space and extended beyond the expected knee. Its 30 throughput points all completed successfully after failed-only infrastructure retries.
These 17 measured Pareto points form the official full sweep. TP2 HiCache and the remaining sampled points were dominated and removed. HiCache uses the established bounded DRAM convention rather than an ad hoc memory override.
Broad fast run: https://github.com/SemiAnalysisAI/InferenceX/actions/runs/31563972895
Metrics correctness
Validation