Conversation
|
Thanks for the contribution!
中文感谢你的贡献!
|
|
View unofficial run (performance): https://inferencex.semianalysis.com/inference?unofficialRun=36150685977 View unofficial run (accuracy): https://inferencex.semianalysis.com/evaluation?unofficialRun=36150685977 |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it introduces a new EP8+MegaMoE/EPLB code path, several new decode/prefill tuning flags (unified KV fp8, grouped-decode ASM, breakable CUDA-graph prefill), and shifts the DP-attention search-space arm from EP1 to EP8 with a new 384-concurrency point, a human (ideally one of the CODEOWNERS already on configs/amd-master.yaml) should still confirm the perf numbers before merge.
What was reviewed:
- New
BCG_ARGS/EP_SIZE-branch env vars in the script — these are recipe-local tuning constants derived from already-validatedCONC/EP_SIZE, not unvalidated caller config. perf-changelog.yamlentry — correctly appended at the tail; itspull/XXXplaceholder matches an existing untouched entry elsewhere in the file, so it's not a new deviation.- Image bump scope — recipe is
multinode: false, so the multi-nodemodel.container == imagecross-check doesn't apply. - New EP8 path writes expert-distribution stat files to shared
/tmprather than$RESULT_DIR(line 162) — a plausible minor collision risk on a reused AMD runner, but low severity since it's only diagnostic EPLB stat output, not benchmark results.
Extended reasoning...
The diff is confined to one AMD MI355X SGLang recipe: an image bump, a DP-attention search-space change (ep:1→ep:8, +conc 384), and a script restructure that adds a new EP8+MegaMoE/EPLB code path alongside fp8 unified KV, grouped decode, and breakable CUDA-graph prefill flags. No injection/auth/secrets surface is touched; the risk here is purely performance/behavioral correctness of a GPU serving recipe. The change is larger and more consequential than a mechanical config tweak (new code branch, several new env vars affecting memory fraction and expert placement), so despite the automated hunt finding nothing, a human with domain context should still confirm the new EP8 arm behaves as intended before it ships.
This review covers commit b0a5d2a, which is no longer the latest commit on this pull request; later commits are not covered by it.
|
InferenceX has switched away from unmaintainable bash scripts to YAML files that don't repeat the same stuff over and over again. Please merge the latest |
Summary: