[NV] [PR3] feat(dsv4): enable synthetic-acceptance for dsv4-fp4-gb200 mtp2 sweep#2091
[NV] [PR3] feat(dsv4): enable synthetic-acceptance for dsv4-fp4-gb200 mtp2 sweep#2091qiching wants to merge 4 commits into
Conversation
Two changes needed to run dsv4 fp4 multinode sweeps on the watchtower (Oracle) gb200 cluster: - Point the dynamo-vllm dsv4 MODEL_PATH at the base DeepSeek-V4-Pro checkpoint instead of the -NVFP4 re-quant. The pinned v0.20.1 container's deepseek_v4 loader doesn't define the NVFP4 export's extra quant params (e.g. ffn.experts.w13_input_scale) and KeyErrors at load; the base checkpoint loads and matches the recipe's served identity. - Add dsv4/dynamo-vllm to uses_watchtower_shared_fs() so the srt-slurm workspace/outputs land on a compute-visible shared FS (the runner home is not cross-mounted to compute nodes, otherwise the batch step dies).
Framework-agnostic plumbing for optionally overriding a recipe's speculative-decoding acceptance length with a synthetic value, gated on SYNTHETIC_ACCEPTANCE=true (no-op otherwise, so this commit changes no existing behavior). - runners/synthetic_injectors/: backend registry (register/get_injector). No backends registered yet -> the generic layer alone injects nothing. - runners/inject_synthetic_acceptance.py: generic driver. Handles flag gating, acceptance-length resolution (explicit env or reference-yaml lookup) and file I/O, then delegates the recipe rewrite to the framework backend resolved from FRAMEWORK. Errors clearly if the flag is on but no backend is registered. - runners/launch_gb200-nv.sh: invoke the driver after the name override and before srtctl apply. With no backend registered this is a no-op.
Register a dynamo-vllm backend with the generic driver. When SYNTHETIC_ACCEPTANCE=true and FRAMEWORK=dynamo-vllm, every speculative-config JSON entry in the recipe is rewritten to add rejection_sample_method=synthetic and synthetic_acceptance_length=<al>, so the vLLM engine emits a controlled mean acceptance length instead of running the real draft model. Rewrite uses the json module on the captured JSON (no string-munging), preserving the recipe's compact style. - runners/synthetic_injectors/vllm.py: rewrite() + spec_tokens_from_recipe(), self-registers under "dynamo-vllm". - runners/synthetic_injectors/__init__.py: import the backend so it registers on package import. Still inert until a config opts in via SYNTHETIC_ACCEPTANCE (added for dsv4 mtp2 in a follow-up).
Opt the dsv4 fp4 gb200 dynamo-vllm mtp2 config into synthetic acceptance (length 2.27) on all four search-space cells, and add a matching -nosynthetic baseline config (identical topology/recipes, synthetic envs removed) so the sweep produces a before/after Pareto pair (throughput vs interactivity) in one run. This is the config that actually exercises the plumbing from the preceding PRs: base-checkpoint/shared-FS multinode fix, the generic injection driver, and the vLLM backend.
|
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 关于重新运行失败任务的文档 |
|
/sweep test-config --config-files configs/nvidia-master.yaml --config-keys dsv4-fp4-gb200-dynamo-vllm-mtp2 --no-evals |
qiching
requested review from
Ankur-singh,
jgangani and
kedarpotdar-nv
as code owners
|
Claude finished @qiching's task in 3m 48s —— View job Review of PR #2091
SummaryFound 1 blocking issue and 2 warnings (posted as inline comments):
Minor note (not blocking): the auto-lookup path in Otherwise the design is sound: the injector is a strict no-op when |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| # MTP2 variant of dsv4-fp4-gb200-dynamo-vllm. Uses the vLLM 0.20.1 image | ||
| # and hand-picked 8k/1k Pareto points mirrored from NVIDIA/srt-slurm. | ||
| dsv4-fp4-gb200-dynamo-vllm-mtp2: | ||
| image: vllm/vllm-openai:v0.20.1-ubuntu2404 |
There was a problem hiding this comment.
🔴 BLOCKING: Master config files were modified but perf-changelog.yaml was not updated. When changing configs/amd-master.yaml or configs/nvidia-master.yaml, you must add a corresponding entry to perf-changelog.yaml documenting the changes.
Why it matters: The perf changelog is how config-driven performance changes are tracked chronologically; this PR changes what dsv4-fp4-gb200-dynamo-vllm-mtp2 measures (synthetic acceptance, AL=2.27) and adds a new -nosynthetic baseline key.
Fix: Append an entry to the end of perf-changelog.yaml (the file is read chronologically, oldest at top) documenting the synthetic-acceptance enablement and the new baseline key, with a link to this PR.
| # Optionally inject synthetic acceptance into the recipe's speculative-config | ||
| # when SYNTHETIC_ACCEPTANCE=true (no-op otherwise). Must run after the name | ||
| # override and before srtctl apply so the rendered job picks it up. | ||
| python3 "$GITHUB_WORKSPACE/runners/inject_synthetic_acceptance.py" "$CONFIG_PATH" "$FRAMEWORK" |
There was a problem hiding this comment.
🟡 WARNING: The injector's exit status is ignored — this script runs under set -x only (no set -e), so if inject_synthetic_acceptance.py fails (sys.exit("ERROR: ...") on unregistered framework, bad SYNTHETIC_ACCEPTANCE_LENGTH, or an uncaught traceback), the sweep proceeds with the unmodified recipe.
Why it matters: The benchmark then runs real MTP acceptance but the results are recorded and published under the synthetic-acceptance config key — silently wrong data, which is worse than a failed job.
Fix:
| python3 "$GITHUB_WORKSPACE/runners/inject_synthetic_acceptance.py" "$CONFIG_PATH" "$FRAMEWORK" | |
| python3 "$GITHUB_WORKSPACE/runners/inject_synthetic_acceptance.py" "$CONFIG_PATH" "$FRAMEWORK" || { echo "Error: synthetic-acceptance injection failed" >&2; exit 1; } |
| if count == 0: | ||
| _log("WARNING: No speculative-config entries found to modify; leaving recipe unchanged") | ||
| return 0 |
There was a problem hiding this comment.
🟡 WARNING: When SYNTHETIC_ACCEPTANCE=true but the recipe contains no matching speculative-config: '<json>' line (e.g. the recipe uses double quotes, block style, or drops the field), this logs a warning and returns 0 — the sweep continues and publishes real-MTP numbers under the synthetic-acceptance config key.
Why it matters: The caller explicitly opted in, so "nothing injected" means the recipe and the config key have drifted apart; the resulting data is silently mislabeled rather than the job failing.
Fix: Treat this as an error when injection was explicitly requested:
| if count == 0: | |
| _log("WARNING: No speculative-config entries found to modify; leaving recipe unchanged") | |
| return 0 | |
| if count == 0: | |
| sys.exit( | |
| "ERROR: SYNTHETIC_ACCEPTANCE=true but no speculative-config " | |
| f"entries were found to modify in {config_file}" | |
| ) |
| # Optionally inject synthetic acceptance into the recipe's speculative-config | ||
| # when SYNTHETIC_ACCEPTANCE=true (no-op otherwise). Must run after the name | ||
| # override and before srtctl apply so the rendered job picks it up. | ||
| python3 "$GITHUB_WORKSPACE/runners/inject_synthetic_acceptance.py" "$CONFIG_PATH" "$FRAMEWORK" |
There was a problem hiding this comment.
🟡 Two-layer silent failure in the new synthetic-acceptance injection. launch_gb200-nv.sh:476 invokes inject_synthetic_acceptance.py without || exit 1, and the script has only set -x (no set -e), so any sys.exit(1) from the injector is swallowed and srtctl apply proceeds with the unmodified recipe. Compounding this, inject_synthetic_acceptance.py:146-148 returns 0 with only a WARNING when count == 0 (e.g. the vLLM backend's single-quoted-JSON regex fails against a future block-scalar/double-quoted recipe). When SYNTHETIC_ACCEPTANCE=true is set on the mtp2 configs, either path silently produces a run without synthetic acceptance whose results are then attributed to the synthetic-acceptance side of the -nosynthetic Pareto comparison this PR is built to enable. Neither path fires for the currently-shipped recipes — fix is trivial defense-in-depth: add || exit 1 on line 476, and have the injector sys.exit non-zero on count == 0 when SYNTHETIC_ACCEPTANCE=true was explicitly requested.
Extended reasoning...
What the bug is
This PR wires a new synthetic-acceptance injection step into launch_gb200-nv.sh and adds a dsv4-fp4-gb200-dynamo-vllm-mtp2-nosynthetic baseline so the sweep can Pareto-compare synthetic-AL results against a real-draft baseline. There are two independent silent-failure paths in that new plumbing:
Layer 1 — launcher swallows non-zero exits. runners/launch_gb200-nv.sh:5 has only set -x (no set -e), and line 476 invokes the injector as:
python3 "$GITHUB_WORKSPACE/runners/inject_synthetic_acceptance.py" "$CONFIG_PATH" "$FRAMEWORK"with no || exit 1 trailer. The injector has five sys.exit("ERROR: ...") paths (no injector registered for framework, missing reference YAML when SYNTHETIC_ACCEPTANCE_LENGTH is unset, missing model key, missing num_spec_tokens, missing THINKING_MODE). Any of these produce exit code 1 — but the launcher just continues to srtctl apply with the unmodified recipe. Every other critical step in this same file (mkdir/rsync/git clone/make setup at 448, 455, and the entire clone-branch block) uses || exit 1, so this omission is out of local convention.
Layer 2 — injector silently no-ops on zero matches. runners/inject_synthetic_acceptance.py:146-148:
if count == 0:
_log("WARNING: No speculative-config entries found to modify; leaving recipe unchanged")
return 0returns success even when SYNTHETIC_ACCEPTANCE=true was explicitly requested. The vLLM backend regex speculative-config:\s*'([^']+)' in runners/synthetic_injectors/vllm.py:22 only matches single-quoted JSON on a single line — a recipe converted to double-quoted style, block scalar (speculative-config: |), or bare flow style would silently yield count=0 and the injector would return 0, hiding the mismatch from the launcher entirely (even after fixing Layer 1).
Step-by-step proof (Layer 1)
- A future config sets
SYNTHETIC_ACCEPTANCE=trueandFRAMEWORK=dynamo-sglang(or any framework not in_INJECTORS). inject_synthetic_acceptance.py'sinject()hitsget_injector("dynamo-sglang")→None.- It calls
sys.exit("ERROR: SYNTHETIC_ACCEPTANCE=true but no synthetic-acceptance injector is registered for FRAMEWORK='dynamo-sglang'")— process exits 1. - Bash continues, because there's no
set -eand no|| exit 1.set -xechoes the exit; nothing stops the pipeline. srtctl applyruns with the unmodified recipe (real draft model MTP2, not synthetic AL=2.27).- The benchmark completes and the JSON is saved under the config key. Downstream
perf-changelog/ the-nosyntheticPareto comparison treats these numbers as the synthetic-acceptance side of the sweep — silently poisoned data.
Step-by-step proof (Layer 2)
- A future maintainer reformats
agg-gb200-low-latency-mtp2.yamlto use block scalars for the speculative-config JSON (or double quotes — a stylistically valid change). _SPEC_CONFIG_RE.finditer(text)yields zero matches.injector.rewrite()returns("<unmodified>", 0).- The
count == 0branch logs a WARNING and returns 0. - Launcher accepts exit 0 (even after fixing Layer 1) and proceeds to
srtctl applywith the unmodified recipe. - Same downstream mis-attribution as above.
Why this is nit-severity for THIS PR
I verified that none of the currently-shipped recipes trigger either failure mode:
- All four mtp2 recipes referenced by
dsv4-fp4-gb200-dynamo-vllm-mtp2(agg-gb200-low-latency-mtp2.yaml,disagg-gb200-{low-latency,mid-curve-megamoe,high-tpt-megamoe}-mtp2.yaml) use the exact single-quoted JSON formspeculative-config: '{"method":"mtp","num_speculative_tokens":2}'that the regex expects. - The config sets
SYNTHETIC_ACCEPTANCE_LENGTH=2.27explicitly on every scenario, so the reference-YAML lookup path (which has three of the fivesys.exitcalls) is skipped entirely. FRAMEWORK=dynamo-vllmis registered byrunners/synthetic_injectors/vllm.py:69, so the "no injector for framework" path doesn't fire.
So the immediate mtp2 vs mtp2-nosynthetic Pareto comparison this PR is enabling should be correct. This is defense-in-depth for the scaffolding that's explicitly designed to be reused across future frameworks (sglang, trtllm — see runners/synthetic_injectors/__init__.py's module docstring) and models.
How to fix
Two one-liners:
-python3 "$GITHUB_WORKSPACE/runners/inject_synthetic_acceptance.py" "$CONFIG_PATH" "$FRAMEWORK"
+python3 "$GITHUB_WORKSPACE/runners/inject_synthetic_acceptance.py" "$CONFIG_PATH" "$FRAMEWORK" || exit 1and in inject_synthetic_acceptance.py:
if count == 0:
- _log("WARNING: No speculative-config entries found to modify; leaving recipe unchanged")
- return 0
+ sys.exit(
+ "ERROR: SYNTHETIC_ACCEPTANCE=true was requested but no speculative-config "
+ f"entries matched in {config_file}; refusing to run an un-injected sweep"
+ )Both changes match the fail-fast pattern used throughout the launcher (mkdir/rsync/git-clone/make setup all use || exit 1).
1 participant
No description provided.