Skip to content

[Spec Decode] Fix offline spec_decode.py #23461

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 48 commits into from
Closed

[Spec Decode] Fix offline spec_decode.py #23461

wants to merge 48 commits into from

Conversation

@ekagra-ranjan
Copy link
Contributor

@ekagra-ranjan ekagra-ranjan commented Aug 23, 2025 •
edited by github-actions bot
Loading

Purpose

Offline spec_decode.py was broken by this PR and the recent PR. This PR fixes it.

Test Plan

time VLLM_USE_V1=1 python3 examples/offline_inference/spec_decode.py --method eagle --num_spec_tokens 3 --dataset-name hf --dataset-path philschmid/mt-bench --num-prompts 80 --print-output

Test Result

Output

--------------------------------------------------
--------------------------------------------------
total_num_output_tokens: 16956
num_drafts: 7409
num_draft_tokens: 22227
num_accepted_tokens: 9552
mean acceptance length: 2.29
--------------------------------------------------
acceptance at token 0: 0.68
acceptance at token 1: 0.40
acceptance at token 2: 0.21

@mergify mergify bot added documentation Improvements or additions to documentation performance Performance-related issues speculative-decoding labels Aug 23, 2025
gemini-code-assist[bot]
gemini-code-assist bot reviewed Aug 23, 2025
View reviewed changes
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces two fixes to address issues in offline speculative decoding. The first change in examples/offline_inference/spec_decode.py corrects the argument passed to llm.generate for batched prompts, ensuring it receives a list of TokensPrompt objects as expected. The second change in vllm/benchmarks/datasets.py adds a default value for request_id_prefix to prevent AttributeError when calling get_samples. Both changes appear correct and address the underlying bugs. My review did not find any high or critical severity issues with these fixes.

@ekagra-ranjan
Copy link
Contributor Author

ekagra-ranjan commented Aug 23, 2025

Should we run spec_decode.py with default parameter as an end to end test in vLLM? If so, can someone point me to the right place where this can be added?

@tomasruizt
Copy link
Contributor

tomasruizt commented Aug 25, 2025

There is a test file tests/v1/e2e/test_spec_decode.py. That could be a potential location for and e2e test.

@mergify
Copy link

mergify bot commented Aug 30, 2025

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @ekagra-ranjan.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label Aug 30, 2025
@wwl2755
Copy link
Contributor

wwl2755 commented Sep 2, 2025

@ekagra-ranjan Hi Ekagra, Thanks for the PR! I just came across this PR and verified it did fix the bug for example code. What's the current state of this PR? I think it is a helpful fix.

@wwl2755 wwl2755 mentioned this pull request Sep 3, 2025
Open
5 tasks
@ekagra-ranjan
Copy link
Contributor Author

ekagra-ranjan commented Sep 3, 2025 •
edited
Loading

@wwl2755 - I am waiting for approval. It seems a later PR #23803 partially fixed it since i got merge conflict. I will update my PR now.

@mergify mergify bot removed the needs-rebase label Sep 3, 2025
@ekagra-ranjan ekagra-ranjan mentioned this pull request Sep 3, 2025
Open
ywang96
ywang96 approved these changes Sep 3, 2025
View reviewed changes
Copy link
Member

@ywang96 ywang96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix!

@ywang96 ywang96 added the ready ONLY add when PR is ready to merge/full CI is needed label Sep 3, 2025
@ywang96 ywang96 enabled auto-merge (squash) September 3, 2025 23:36
842974287 and others added 14 commits September 4, 2025 15:31
@842974287 @ekagra-ranjan
add an env var for path to pre-downloaded flashinfer cubin files (vll…
9191d57 
…m-project#22675)

Signed-off-by: Ekagra Ranjan <3116519+ekagra-ranjan@users.noreply.github.com>
@zhewenl @simon-mo @ekagra-ranjan
[CI/Build] add EP dependencies to docker (vllm-project#21976)
31705bf 
Co-authored-by: Simon Mo <simon.mo@hey.com>
Signed-off-by: Ekagra Ranjan <3116519+ekagra-ranjan@users.noreply.github.com>
@ekagra-ranjan
fix spec_decode.py
73911f7 
Signed-off-by: Ekagra Ranjan <3116519+ekagra-ranjan@users.noreply.github.com>
@ilmarkov @mgoin @ekagra-ranjan
[PERF] PyTorch Symmetric Memory All-Reduce (vllm-project#20759)
7847894 
Signed-off-by: ilmarkov <imarkov@redhat.com>
Signed-off-by: ilmarkov <markovilya197@gmail.com>
Signed-off-by: Michael Goin <mgoin64@gmail.com>
Co-authored-by: ilmarkov <imarkov@redhat.com>
Co-authored-by: Michael Goin <mgoin64@gmail.com>
Signed-off-by: Ekagra Ranjan <3116519+ekagra-ranjan@users.noreply.github.com>
@rasmith @ekagra-ranjan
[BugFix][AMD][Quantization] Fix torch.compile issue where wvSplitKQ n…
78e74a1 
…ot being called when it should when using quantized FP8 model (vllm-project#22281)

Signed-off-by: Randall Smith <Randall.Smith@amd.com>
Signed-off-by: Ekagra Ranjan <3116519+ekagra-ranjan@users.noreply.github.com>
@elvischenv @ProExpertProg @ekagra-ranjan
[NVIDIA][torch.compile] Support Flashinfer TRTLLM FP8-q/kv NVFP4-out …
edd48f5 
…Attention Kernel (vllm-project#22703)

Signed-off-by: elvischenv <219235043+elvischenv@users.noreply.github.com>
Co-authored-by: Luka Govedič <ProExpertProg@users.noreply.github.com>
Signed-off-by: Ekagra Ranjan <3116519+ekagra-ranjan@users.noreply.github.com>
@njhill @ekagra-ranjan
[BugFix] Fix batch updates for pooling models (vllm-project#23398)
03d6f9d 
Signed-off-by: Nick Hill <nhill@redhat.com>
Signed-off-by: Ekagra Ranjan <3116519+ekagra-ranjan@users.noreply.github.com>
@njhill @ekagra-ranjan
[BugFix] Fix MinPLogitsProcessor.update_states() (vllm-project#23401)
82f511d 
Signed-off-by: Nick Hill <nhill@redhat.com>
Signed-off-by: Ekagra Ranjan <3116519+ekagra-ranjan@users.noreply.github.com>
@david6666666 @CarrotShoo @ekagra-ranjan
[Model] Support DP for ViT on MiniCPM-V-4 (vllm-project#23327)
9ffa26d 
Signed-off-by: ycyaw66 <497410282@qq.com>
Co-authored-by: ycyaw66 <497410282@qq.com>
Signed-off-by: Ekagra Ranjan <3116519+ekagra-ranjan@users.noreply.github.com>
@mgoin @ekagra-ranjan
[UX] Move Dockerfile DeepGEMM install to tools/install_deepgemm.sh (v…
466ed1b 
…llm-project#23360)

Signed-off-by: mgoin <mgoin64@gmail.com>
Signed-off-by: Ekagra Ranjan <3116519+ekagra-ranjan@users.noreply.github.com>
@fengli1702 @mgoin @ekagra-ranjan
Quantization: support FP4 quantized models on AMD CDNA2/CDNA3 GPUs (v…
bc78210 
…llm-project#22527)

Signed-off-by: feng <fengli1702@gmail.com>
Signed-off-by: Michael Goin <mgoin64@gmail.com>
Co-authored-by: Michael Goin <mgoin64@gmail.com>
Signed-off-by: Ekagra Ranjan <3116519+ekagra-ranjan@users.noreply.github.com>
@chenxi-yang @ekagra-ranjan
Add glm4.5v tp2,4 fp8 config on H100_80GB (vllm-project#23443)
58d6fa6 
Co-authored-by: Chenxi Yang <cxyang@meta.com>
Signed-off-by: Ekagra Ranjan <3116519+ekagra-ranjan@users.noreply.github.com>
@DarkLight1337 @ekagra-ranjan
Revert "[PERF] Use faster way of decode in tokenizer: avoid useless l…
4f93bc2 
…ist-to-list conversion (vllm-project#20000)" (vllm-project#23396)

Signed-off-by: DarkLight1337 <tlleungac@connect.ust.hk>
Signed-off-by: Ekagra Ranjan <3116519+ekagra-ranjan@users.noreply.github.com>
@AzizCode92 @ekagra-ranjan
fix(tests): Correct unreachable assertion in truncation test (vllm-pr…
5546b68 
…oject#23425)

Signed-off-by: AzizCode92 <azizbenothman76@gmail.com>
Signed-off-by: Ekagra Ranjan <3116519+ekagra-ranjan@users.noreply.github.com>
@mergify mergify bot added ci/build deepseek Related to DeepSeek models frontend multi-modality Related to multi-modality (#4194) new-model Requests to new models qwen Related to Qwen models rocm Related to AMD ROCm structured-output v1 labels Sep 4, 2025
@mergify mergify bot added tpu Related to Google TPUs tool-calling labels Sep 4, 2025
@mergify
Copy link

mergify bot commented Sep 4, 2025

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @ekagra-ranjan.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label Sep 4, 2025
@ekagra-ranjan ekagra-ranjan mentioned this pull request Sep 4, 2025
Merged
@ekagra-ranjan
Copy link
Contributor Author

ekagra-ranjan commented Sep 4, 2025

superseeded by #24257

@ekagra-ranjan ekagra-ranjan mentioned this pull request Sep 9, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ywang96 ywang96 ywang96 approved these changes

@gshtras gshtras Awaiting requested review from gshtras gshtras is a code owner

@patrickvonplaten patrickvonplaten Awaiting requested review from patrickvonplaten patrickvonplaten is a code owner

@sighingnow sighingnow Awaiting requested review from sighingnow sighingnow is a code owner

@hmellor hmellor Awaiting requested review from hmellor hmellor is a code owner

@mgoin mgoin Awaiting requested review from mgoin mgoin is a code owner

@russellb russellb Awaiting requested review from russellb russellb is a code owner

@aarnphm aarnphm Awaiting requested review from aarnphm aarnphm is a code owner

@DarkLight1337 DarkLight1337 Awaiting requested review from DarkLight1337 DarkLight1337 is a code owner

@tlrmchlsmth tlrmchlsmth Awaiting requested review from tlrmchlsmth tlrmchlsmth is a code owner

@WoosukKwon WoosukKwon Awaiting requested review from WoosukKwon WoosukKwon is a code owner

@yewentao256 yewentao256 Awaiting requested review from yewentao256 yewentao256 is a code owner

@robertgshaw2-redhat robertgshaw2-redhat Awaiting requested review from robertgshaw2-redhat robertgshaw2-redhat is a code owner

@simon-mo simon-mo Awaiting requested review from simon-mo simon-mo is a code owner

@tdoublep tdoublep Awaiting requested review from tdoublep tdoublep is a code owner

@njhill njhill Awaiting requested review from njhill njhill is a code owner

@comaniac comaniac Awaiting requested review from comaniac comaniac is a code owner

@alexm-redhat alexm-redhat Awaiting requested review from alexm-redhat alexm-redhat is a code owner

@youkaichao youkaichao Awaiting requested review from youkaichao youkaichao is a code owner

@houseroad houseroad Awaiting requested review from houseroad houseroad is a code owner

@ProExpertProg ProExpertProg Awaiting requested review from ProExpertProg ProExpertProg is a code owner

@LucasWilkinson LucasWilkinson Awaiting requested review from LucasWilkinson LucasWilkinson is a code owner

@zou3519 zou3519 Awaiting requested review from zou3519 zou3519 is a code owner

@zhuohan123 zhuohan123 Awaiting requested review from zhuohan123 zhuohan123 is a code owner

+1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

ci/build deepseek Related to DeepSeek models documentation Improvements or additions to documentation frontend multi-modality Related to multi-modality (#4194) needs-rebase new-model Requests to new models performance Performance-related issues qwen Related to Qwen models ready ONLY add when PR is ready to merge/full CI is needed rocm Related to AMD ROCm speculative-decoding structured-output tool-calling tpu Related to Google TPUs v1

Projects

Structured Output
Status: Done
Tool Calling
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.