perf(kimik2.5-fp4-mi355x): enable vLLM compile-time fusion passes / 为 kimik2.5-fp4-mi355x-vllm 启用 vLLM 编译期融合 pass #2264
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -74,7 +74,10 @@ $EP \ | |
| --block-size=1 \ | ||
| --no-enable-prefix-caching \ | ||
| --trust-remote-code \ | ||
| --compilation_config.pass_config.fuse_norm_quant true \ | ||
| --compilation_config.pass_config.fuse_act_quant true \ | ||
| --compilation_config.pass_config.fuse_allreduce_rms true \ | ||
| --compilation_config.pass_config.fuse_mla_dual_rms_norm true \ | ||
|
Comment on lines
+77
to
+80
Contributor
There was a problem hiding this comment. 🔴 BLOCKING: Missing - config-keys:
- kimik2.5-fp4-mi355x-vllm
description:
- "Enable vLLM compile-time fusion passes: fuse_norm_quant, fuse_act_quant, fuse_allreduce_rms, fuse_mla_dual_rms_norm"
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2264 |
||
| --mm-encoder-tp-mode data > $SERVER_LOG 2>&1 & | ||
|
|
||
| SERVER_PID=$! | ||
|
Comment on lines
74
to
83
Contributor
There was a problem hiding this comment. 🟡 PR #2264's title and description are English-only, violating AGENTS.md's mandatory bilingual convention (title must be Extended reasoning...AGENTS.md line 7 states, in mandatory language: "PR and GitHub-issue titles & descriptions must be bilingual — include a Simplified Chinese version in addition to English. Title format: This PR's title is only Step-by-step verification:
This is not a subjective style nit invented by the reviewer — it is a literal, checked-in repository rule (AGENTS.md, loaded via CLAUDE.md) using 'must' language and covering titles, descriptions, and even PR comments. Existing repo tooling (e.g. the CODEOWNER sign-off bot comment on this very PR) already posts bilingual English/Chinese text, showing the convention is actively enforced elsewhere in the repo's workflow, even though there's no automated CI gate for the PR title/body itself. Fix: rename the PR title to Why nit and not normal: this has zero effect on the benchmark script's correctness or runtime behavior — nothing breaks, no incorrect results, no crash. It's a documentation/process compliance gap that's trivial to fix by editing the PR title and body, so it doesn't justify blocking merge, but per the repo's explicit convention it should still be raised rather than silently ignored. |
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 WARNING: This is the first ROCm enablement of
fuse_allreduce_rmsin this repo — and the only other AMD reference,kimik2.5_int4_mi355x.sh:46on the same MI355X SKU, explicitly disables it (--compilation-config '{"pass_config": {"fuse_allreduce_rms": false}}'). The sibling scripts cited in the PR description (kimik2.5_fp4_b300.sh,kimik2.5_int4_b200.sh, agenticint4_h200, etc.) are all NVIDIA.Why it matters: If the int4 script's opt-out was working around a ROCm crash/accuracy/perf issue with the fused allreduce+RMSNorm path, this flag could regress rather than help — especially combined with
VLLM_ROCM_QUICK_REDUCE_QUANTIZATION=INT4(line 49), whose quantized custom all-reduce may not compose with the fused collective op. Note also thatfuse_norm_quant,fuse_act_quant, andfuse_mla_dual_rms_normhave no prior usage anywhere in this repo; if any name is invalid in thevllm/vllm-openai-rocm:v0.24.0image,PassConfigvalidation rejects it and the server fails to start.Fix: The planned CI sweep + eval run (already in your test plan) covers this — please also confirm with the int4 script's author why
fuse_allreduce_rmswas disabled there before merging, and check the server log shows the passes actually fired (fusion counts in the compile logs) rather than silently no-op'ing.