Conversation
Signed-off-by: haic0 <haichzha@gbt350-odcdh5-wbb3.png-odc.dcgpu>
|
Thanks for the contribution! For vLLM & SGLang, please ensure that your recipes is similar to the official vLLM recipes and/or the SGLang cookbook If it is not, please create a PR first before we can merge your PR into the master branch. Let's ensure that the documentation is first class such that the entire ML community can benefit from your hard work! Thank you 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. If re-running failed jobs is attempted, PR authors are responsible for ensuring it passes. See GitHub's docs on re-running failed jobs: https://docs.github.com/en/actions/how-tos/manage-workflow-runs/re-run-workflows-and-jobs#re-running-failed-jobs-in-a-workflow As a rule of thumb, generally, PR authors should request a review & get a PR approval from the respective companies' CODEOWNERS before requesting a review from core maintainers. If additional help is needed, PR authors can reach out to core maintainers over Slack. |
1 similar comment
|
Thanks for the contribution! For vLLM & SGLang, please ensure that your recipes is similar to the official vLLM recipes and/or the SGLang cookbook If it is not, please create a PR first before we can merge your PR into the master branch. Let's ensure that the documentation is first class such that the entire ML community can benefit from your hard work! Thank you 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. If re-running failed jobs is attempted, PR authors are responsible for ensuring it passes. See GitHub's docs on re-running failed jobs: https://docs.github.com/en/actions/how-tos/manage-workflow-runs/re-run-workflows-and-jobs#re-running-failed-jobs-in-a-workflow As a rule of thumb, generally, PR authors should request a review & get a PR approval from the respective companies' CODEOWNERS before requesting a review from core maintainers. If additional help is needed, PR authors can reach out to core maintainers over Slack. |
|
thanks for the PR @haic0 We haven't thought much about the guidelines for MTP for models that dont natively ship with it as we didn't think we would include it for inferencexv3 & have previously rejected submissions for it #1026 (review) do you think we should include it for inferencex v3 i.e. the questions that need some thought is:
We already have 3 different models that natively ship with MTP (deepseek, glm5, qwen3.5), is it worth it to spend time thinking about MTP for models that don't come natively with MTP? |
| kimik2.5-int4-mi300x-vllm-mtp: | ||
| image: vllm/vllm-openai-rocm:v0.19.0 | ||
| model: moonshotai/Kimi-K2.5 | ||
| model-prefix: kimik2.5 | ||
| runner: mi300x | ||
| precision: int4 | ||
| framework: vllm | ||
| multinode: false | ||
| seq-len-configs: | ||
| - isl: 1024 | ||
| osl: 1024 | ||
| search-space: | ||
| - { tp: 4, conc-start: 4, conc-end: 64, spec-decoding: mtp } | ||
| - isl: 8192 | ||
| osl: 1024 | ||
| search-space: | ||
| - { tp: 4, conc-start: 4, conc-end: 64, spec-decoding: mtp } | ||
|
|
||
| kimik2.5-int4-mi325x-vllm-mtp: | ||
| image: vllm/vllm-openai-rocm:v0.19.0 | ||
| model: moonshotai/Kimi-K2.5 | ||
| model-prefix: kimik2.5 | ||
| runner: mi325x | ||
| precision: int4 |
There was a problem hiding this comment.
🔴 The MI300X and MI325X runners hardcode the benchmark script path without any spec suffix, so the newly added kimik2.5_int4_mi300x_mtp.sh and kimik2.5_int4_mi325x_mtp.sh scripts are dead code — CI jobs for kimik2.5-int4-mi300x-vllm-mtp and kimik2.5-int4-mi325x-vllm-mtp will silently execute the old non-MTP scripts instead. Both runners need the same SPEC_SUFFIX logic already present in launch_mi355x-amds.sh (lines 182, 225).
Extended reasoning...
What the bug is and how it manifests
This PR adds two new YAML benchmark entries — kimik2.5-int4-mi300x-vllm-mtp and kimik2.5-int4-mi325x-vllm-mtp — each with spec-decoding: mtp in their search-space, plus corresponding benchmark scripts (kimik2.5_int4_mi300x_mtp.sh, kimik2.5_int4_mi325x_mtp.sh). When these CI jobs run, they dispatch through the MI300X and MI325X runner scripts, which are responsible for selecting the correct benchmark script. However, neither runner has logic to append the _mtp suffix to the script name, causing the wrong script to be executed.
The specific code path that triggers it
runners/launch_mi300x-amds.sh line 38 hardcodes:
bash benchmarks/single_node/__mi300x.shSimilarly, runners/launch_mi325x-amds.sh line 38 hardcodes:
bash benchmarks/single_node/__mi325x.shNeither runner inspects SPEC_DECODING or computes a SPEC_SUFFIX.
Why existing code doesn't prevent it
The MI355X runner (launch_mi355x-amds.sh lines 182–225) was already updated with the correct pattern:
SPEC_SUFFIX=
bash benchmarks/single_node/__mi355x.shThe MI300X and MI325X runners were simply not updated to match, leaving them permanently dispatching to the base (non-MTP) script regardless of the spec-decoding setting passed via the YAML.
What the impact would be
When kimik2.5-int4-mi300x-vllm-mtp or kimik2.5-int4-mi325x-vllm-mtp CI jobs run, the benchmark will silently execute the old kimik2.5_int4_mi300x.sh / kimik2.5_int4_mi325x.sh scripts. Those scripts use vLLM image v0.18.0 instead of the required v0.19.0, and they do not set VLLM_SPEC_CONFIG (the EAGLE3 speculative decoding config). Benchmark results will therefore reflect plain inference without MTP/speculative decoding, defeating the entire purpose of these new CI entries. The two newly added _mtp.sh scripts are effectively dead code.
How to fix it
Both launch_mi300x-amds.sh and launch_mi325x-amds.sh need the same SPEC_SUFFIX computation that already exists in launch_mi355x-amds.sh:
SPEC_SUFFIX=$([[ "$SPEC_DECODING" == "mtp" ]] && printf '_mtp' || printf '')
bash benchmarks/single_node/${EXP_NAME%%_*}_${PRECISION}_mi300x${SPEC_SUFFIX}.sh(and analogously for mi325x).
Step-by-step proof
- CI triggers the
kimik2.5-int4-mi300x-vllm-mtpjob; the workflow setsSPEC_DECODING=mtpand callslaunch_mi300x-amds.sh. - Inside the runner,
EXP_NAMEis set to something likekimik2.5_isl1024osl1024(fromgenerate_sweep_configs.pypattern{model_code}_{seq_len_str}). - The expression
${EXP_NAME%%_*}strips everything from the first underscore onward, yieldingkimik2.5. - Line 38 constructs the path:
benchmarks/single_node/kimik2.5_int4_mi300x.sh— the old non-MTP script. - The new
benchmarks/single_node/kimik2.5_int4_mi300x_mtp.shis never reached.
Thanks. I’m thinking along the same lines as you mentioned, and I agree we should discuss this further.Personally, when defining new rules for SD/MTP, we should prioritize fairness and comparability. In general, we could allow speculative decoding more broadly, but it would be better to place it into a separate track, rather than mixing it with models that natively ship with MTP. Thanks! What speculator weight / layers? Can different vendors submit different speculator weights? Can different vendors submit different speculator architectures? How maintain similar acceptance rate between vendors? Should we use lightseekorg/kimi-k2.6-eagle3? Is it worth it? |
|
thanks for your detailed insights @haic0 @chunfangamd can u add him to slack so we can discuss further? |
No description provided.