[AMD] dsv4-fp4-mi355x-atom: latest atom-dev image + opt recipe / DSv4-Pro FP4 MI355X ATOM：升级最新 atom-dev 镜像 + 优化配方 + MTP#2345
[AMD] dsv4-fp4-mi355x-atom: latest atom-dev image + opt recipe / DSv4-Pro FP4 MI355X ATOM：升级最新 atom-dev 镜像 + 优化配方 + MTP#2345zufayu wants to merge 10 commits into
Conversation
… MTP recipe - image: atom0.1.3 release -> rocm/atom-dev:nightly_202607231538 - grid: align ATOM models.json (MTP3 dp-off conc 4-256 + DPA MTP3 dp-on conc 64-1024); no TBO (incompatible with MTP) - recipe: add max_num_seqs=conc (avoid OOM on dp-on/conc>=64), --no-enable_prefix_caching, eval-context handling - bench: --dsv4 -> --use-chat-template (--dsv4 removed upstream; DSv4 encoder ships with model dir) - fix benchmark_lib source path (../ -> ../../)
|
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 关于重新运行失败任务的文档 |
seungrokj
changed the title
seungrokj
changed the title
seungrokj
changed the title
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Deferring — the reported bug (dropping --dsv4 for --use-chat-template) is a real, verified regression: benchmark_serving.py still gates the DeepSeek-V4 chat encoder behind a separate, live --dsv4 flag, and every sibling DSv4-Pro MTP script still passes --dsv4 for this exact model/tokenizer. This isn't a nit — it risks either crashing the sweep or silently corrupting the MTP acceptance-length numbers the recipe exists to produce, so it should be fixed before merge.
Extended reasoning...
See inline finding for details; verified independently that utils/bench_serving/benchmark_serving.py defines --dsv4 as a distinct argparse flag from --use-chat-template, and _apply_chat_template only routes through the DeepSeek-V4 encoder when dsv4=True.
| --result-filename "$RESULT_FILENAME" \ | ||
| --result-dir /workspace/ \ | ||
| --trust-remote-code \ | ||
| --dsv4 | ||
| --use-chat-template | ||
|
|
||
| # After throughput, run evaluation only if RUN_EVAL is true | ||
| if [ "${RUN_EVAL}" = "true" ]; then |
There was a problem hiding this comment.
🔴 Dropping --dsv4 in favor of plain --use-chat-template breaks the bench client: utils/bench_serving/benchmark_serving.py still has a live --dsv4 flag, and _apply_chat_template() only calls the DeepSeek-V4 encoder when dsv4=True — with the new invocation it falls through to tokenizer.apply_chat_template(), which reads a jinja template from tokenizer_config.json, not the model-dir Python encoder the PR description cites. Every sibling DSv4 MTP script (vllm/sglang/mi300x/mi325x variants) still passes --dsv4 and documents that the DSv4-Pro tokenizer has no jinja chat_template, so this will either crash the benchmark or silently misalign MTP acceptance-length measurements. Please restore --dsv4 (or confirm DSv4-Pro's tokenizer now ships a matching jinja template before merging).
Extended reasoning...
The bug: this PR replaces --dsv4 with plain --use-chat-template on the run_benchmark_serving call (line 96), on the premise that ATOM now auto-discovers a DeepSeek-V4 chat encoder from the model directory on both the server and the bench client, so the explicit flag is no longer needed. That premise doesn't hold for the client this script actually invokes.
The code path: utils/bench_serving/benchmark_serving.py still defines a live --dsv4 argparse flag (in the "DeepSeek-V4 chat template options" group, around line 1242), directly contradicting the PR description's claim that --dsv4 was "removed upstream." More importantly, _apply_chat_template(prompt, tokenizer, dsv4) (around line 161) only routes through dsv4_encode_messages — the local encoding_dsv4.py module imported at the top of the file — when dsv4=True. With the new invocation (--use-chat-template set, --dsv4 omitted), dsv4 is False, so the function falls through to tokenizer.apply_chat_template(...), i.e. whatever jinja chat_template is (or isn't) embedded in the tokenizer's tokenizer_config.json.
Why nothing else prevents this: there is no model-dir encoder auto-discovery anywhere in benchmark_serving.py. The PR's rationale conflates ATOM's server-side trust-remote-code-style auto-discovery of encoding/encoding_*.py with this independent, HF-tokenizer-based bench client, which has no equivalent mechanism — it only knows about the hardcoded local encoding_dsv4 module, gated by the --dsv4 flag. The comment this PR deleted made this explicit: "The DSv4-Pro tokenizer ships without a jinja chat_template, so plain --use-chat-template would crash." Every other DSv4-Pro MTP script still in the repo — dsv4_fp4_mi355x_vllm_mtp.sh, dsv4_fp8_mi300x_mtp.sh, dsv4_fp8_mi325x_mtp.sh, dsv4_fp4_mi355x_sglang_mtp.sh — still passes --dsv4 for the exact same model/tokenizer, and benchmarks/multi_node/amd_utils/bench.sh explicitly branches on model name (DeepSeek-V4-Pro -> --dsv4, everything else -> --use-chat-template) in the same ATOM framework context.
Impact: after merge, this recipe's benchmark client either (a) crashes when tokenizer.apply_chat_template() raises because the DSv4-Pro tokenizer has no jinja chat_template — failing the sweep — or (b) if some jinja template does exist, silently uses different chat framing than the encoding_dsv4 <bos><User>...<Assistant><think> format the server/model expects, corrupting the acceptance-length (AL) measurements this MTP sweep exists to produce. This is the exact MTP regression class AGENTS.md warns about: EAGLE/MTP acceptance numbers silently going wrong when prompt framing doesn't match server expectations.
Step-by-step proof:
run_benchmark_serving(line 90-96) is called with--use-chat-templatebut no--dsv4.benchmark_lib.sh'srun_benchmark_servingwrapper passes these straight through as CLI args tobenchmark_serving.py, so argparse setsargs.use_chat_template = True,args.dsv4 = False(default).- Somewhere in prompt generation (
sample_random_requests),_apply_chat_template(prompt, tokenizer, dsv4=False)is called becauseuse_chat_templateis set. - Inside
_apply_chat_template, theif dsv4:branch is skipped, so it executestokenizer.apply_chat_template([{...}], add_generation_prompt=True, tokenize=False). - Per the repo's own uniform documentation (repeated in every sibling DSv4-Pro script), the DSv4-Pro tokenizer ships without a jinja
chat_template— so this call raises, and the benchmark throughput run fails outright. If it somehow doesn't raise, the resulting prompt text differs from whatencoding_dsv4.encode_messageswould have produced, so the server (which expects the DSv4 framing) sees off-distribution prompts, and MTP draft-acceptance statistics collected are meaningless.
Fix: restore --dsv4 alongside --use-chat-template in the run_benchmark_serving call, matching every other DSv4-Pro MTP script, unless the author can show the DSv4-Pro tokenizer now ships a jinja chat_template that reproduces the same framing as encoding_dsv4.
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=30234857780 |
2 similar comments
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=30234857780 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=30234857780 |
…encoding_dsv4.py; DSv4-Pro has no jinja template so plain --use-chat-template yields no result)
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=30234857780 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=30238071409 |
|
Hi @seungrokj — could you review this PR and post the filled-in PR_REVIEW_CHECKLIST? You're the original atom-mtp author + CODEOWNER, so you know it best. It updates
This is the fixed-seq-len (8k/1k) variant — complementary to your #2346 agentic-mtp (different config keys, no overlap). bench uses Thanks! |
|
/reuse-sweep-run |
There was a problem hiding this comment.
As a PR reviewer and CODEOWNER, I have reviewed this and have:
- Verified that as of the moment of typing this, this is the latest version of PR_REVIEW_CHECKLIST.md
- Verified that the general code quality meets the InferenceX standard and does not make the code quality any worse.
- Verified that this PR has passed PR validation. Please link to GitHub Action workflow that shows this.
- Verified that this PR passes evals. Please link to GitHub Action workflow that shows this.
- Verified that speculative decoding PRs uses chat templates to align the AL distribution to real world
- For agentic workloads: verified that speculative-decoding configs (EAGLE / MTP / draft models) run with simulated synthetic acceptance, with the acceptance-length value taken from the committed golden AL curve in golden_al_distribution/ for that model, thinking mode, and draft length. A submission may choose any supported draft length, but it may not substitute a different acceptance target.
- Verified that the model architecture isn't changed with benchmark hacks like using --hf-overrides to skipping indexer for every x layers on models that don't natively support this. As a general rule, we won't accept optimizations that reduces the number of model architecture FLOPs. Anything that makes that same computation run faster is fair game; FLOPs at lower precisions is fine, given that the config passes private evals. As an general north star princple, we should only use optimizations which is used in production by customers that care about accuracy
- If an company claims that they support vLLM/SGLang as first class LLM inference engines on their hardware, I have verified that the respective vLLM submission made using upstream https://hub.docker.com/u/vllm docker repo, upstream SGLang https://hub.docker.com/u/lmsysorg docker repo. The only exceptions are for new hardware, such as MI455X UALoE72, Vera Rubin NVL72, Rubin NVL8, etc., and for new model architectures where there is an actual reason why vLLM/SGLang does not fundamentally support them yet as supported by vLLM/SGLang community maintainers
- If an company claims that they support vLLM/SGLang as first class upstream in-tree LLM inference engines on their hardware, I have have verified that the respective vLLM/SGLang submission has been made before additional frameworks (TRT-LLM, ATOM, etc.). The only exceptions are for new hardware, such as MI455X UALoE72, Vera Rubin NVL72, Rubin NVL8, etc., and for new model architectures where there is an actual reason why vLLM/SGLang does not fundamentally support them yet.
- Verified that every single-node vLLM/SGLang recipe in this PR is documented in the official vLLM recipes and/or the SGLang cookbook:
- I linked the corresponding upstream PR in the vLLM recipe repo or SGLang repo and verified that it is MERGED before this InferenceX PR merges. An opened, draft, or closed-without-merge upstream PR does not satisfy this requirement. If the matching recipe was already published, I linked the published recipe/cookbook page in the additional detail section below.
- Verified that this PR does not patch the inference engine or serving stack — the pinned image must run as shipped. This covers .patch files / git apply / patch, inline patches embedded in benchmark scripts (e.g. a python3/sed heredoc that rewrites installed engine sources before serving), in-place edits of site-packages, monkey-patching, overwriting container files, and installing forked/rebuilt engine wheels on top of the pinned image. The only exception is a patch covered by a filled-out waiver at docs/waiver/
<PR_NUMBER>.md— named after the PR that introduces the patch and filed in that same PR, stating what is patched, why the unmodified upstream image cannot run this benchmark, the upstream PR/issue link, and the removal plan — which I have linked below in the additional detail section. - If any of the above criteria cannot reasonably be satisfied, I have provided additional reasoning below.
Additional detail section:
- insert any additional info here
recipe at https://github.com/ROCm/ATOM/blob/main/recipes/DeepSeek-V4.md
Signed: seungrokj
✅✅✅ Verdict: PASS ✅✅✅✅ Check 0 (CODEOWNER): PASS — |
|
@functionstackx @Oseltamivir can you plz approve this ? |
…2327) - grid: DPA MTP3 conc-start 64->32 (dp-attn helps from c32) - recipe: --enable-tbo (prefill-only; argparse const=prefill => enable_tbo_decode=False) on dp-attn cells at conc>=64 + GPU_MAX_HW_QUEUES=5, mirrors non-MTP #2327. MTP-safe: only decode-TBO (--enable-tbo all) drops spec_decode_metadata. - changelog updated (triggers re-sweep) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=30257759947 |
1 similar comment
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=30257759947 |
TBO run 30257759947 vs non-TBO 30238071409 (same image/harness/axis): c64 -14.1% / c128 -9.8% output tput (TBO hurts) c256 +11.0% / c512 +8.3% / c1024 +13.9% (TBO helps) MTP already removes low-conc latency, so prefill-TBO overlap at c64/c128 is pure overhead; win only appears once high conc turns compute-bound. Matches ATOM models.json non-MTP DPA-TBO 256-2048 band. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
zufayu
added
full-sweep-enabled
full-sweep-fail-fast
and removed
full-sweep-fail-fast
full-sweep-enabled
labels
Mirrors non-MTP #2327 tp4 grid. Fewer TP ranks -> less TP comm overhead may win at low conc (c16-128); tp4 dp-on is the point of interest, tp4 dp-off is the intra-tp4 control. No TBO (all tp4 cells conc<256). Compare vs tp8 at same conc for the tp4-vs-tp8 low-conc crossover. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=30275335736 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=30277221626 |
tp4 gives 806G/4=201G/GPU weight, leaving negative KV budget even at gpu-mem-util 0.90 (run 30277221626 tp4 dp-on c64/c128 failed at engine init). #2327 tp4 works only because non-MTP (no draft KV). MTP stays tp8. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=30279404130 |
3 participants
What
Update
dsv4-fp4-mi355x-atom-mtpto the latest atom-dev image and align with ATOM's official MTP config (models.json).Changes
atom0.1.3release →rocm/atom-dev:nightly_202607231538models.json— MTP3 (dp-off, conc 4-256) + DPA MTP3 (dp-attn, conc 64-1024). No TBO — TBO is incompatible with MTP in ATOM (UBatchWrapper setsspec_decode_metadata=None; official DPA MTP3 has no--enable-tbo).--max-num-seqs=concon dp-on cells and conc≥64 (avoid OOM; MTP reserves q=num_spec+1 per req); add--no-enable_prefix_caching; eval-context handling.--dsv4→--use-chat-template(--dsv4removed upstream; DSv4 chat encoder now ships in the model dir and is auto-discovered — server & bench share it so AL stays aligned).benchmark_libsource path../→../../(stale since the fix: move dsv4-fp4-mi355x MTP scripts into fixed_seq_len/ #1860 script move).Note for reviewers
--use-chat-templatefor chat-formatted inputs (AL alignment per AGENTS.md).full-sweep-fail-fastlabel to run the sweep.中文说明
将
dsv4-fp4-mi355x-atom-mtp升级到最新 atom-dev 镜像并对齐 ATOM 官方 MTP 配方(models.json):镜像 atom0.1.3→nightly_202607231538;网格照官方 MTP3(dp-off 4-256)+DPA MTP3(dp-on 64-1024),不加 TBO(MTP 不兼容);recipe 加 max_num_seqs=conc 防 OOM、--no-enable_prefix_caching;bench 由 --dsv4 改 --use-chat-template(--dsv4 已上游移除,DSv4 encoder 随模型目录发布);修 benchmark_lib source 路径。