-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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.
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
[V1] [Spec Decode] Support random sampling for spec decode #13933
[V1] [Spec Decode] Support random sampling for spec decode #13933
Conversation
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add 🚀 |
Could you explain this claim? Why it that the case? Is this a problem with our implementation or a fundamental limitation? |
Algorithm-wise, it's unclear. For example, what's the accept criteria? And how to sample from the adjusted the distribution? We need some math here to prove the equivalence. |
Pardon my ignorance if I am not fully informed on how we implement sampling for speculative decoding, but the Leviathan paper on speculative decoding talks about "Speculative Sampling", and how sampling techniques (top-k, nucleus) can be emulated by sampling based on the modified logits distribution. Is it possible to do something similar here? Does vLLM v0 also ignore these sampling parameters? |
TODO: check quality with humaneval |
vllm/v1/sample/rejection_sampler.py
Outdated
|
||
# 3. Accept or reject the samples. | ||
# [batch_size, max_spec_len] | ||
accepted = uniform_samples <= target_token_probs / draft_token_probs |
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.
As we discussed offline, please consider division by zero.
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.
after second thoughts, why do we need to avoid division by zero? if draft_token_probs is 0, then target_token_probs / draft_token_probs is inf, so the token will always be accepted.
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.
(1) It may cause error in L187 (sum)
(2) It may cause NaN if target_token_probs
is also zero.
Co-authored-by: Woosuk Kwon <woosuk.kwon@berkeley.edu>
Co-authored-by: Woosuk Kwon <woosuk.kwon@berkeley.edu>
Co-authored-by: Woosuk Kwon <woosuk.kwon@berkeley.edu>
Co-authored-by: Woosuk Kwon <woosuk.kwon@berkeley.edu>
@LiuXiaoxuanPKU As a sanity check, can you please run a simple perf benchmark? I'm just wondering if we missed anything critical. |
Hi, I always got the following error when my server ran for a long time (a whole night).
Memory is sufficient with my two 3090 24GB. My config is
|
I evaluate the quality of
|
@LiuXiaoxuanPKU Is the PR ready for merge? |
Yes, I checked more about the quality. For greedy, it's steady. For random sampling, it fluctuates (sometimes better, sometimes worse). Overall it looks correct to me. |
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.
LGTM! Thanks for the great work! 👍
…ect#13933) Co-authored-by: Woosuk Kwon <woosuk.kwon@berkeley.edu> Signed-off-by: DefTruth <31974251+DefTruth@users.noreply.github.com>
After syncing with @WoosukKwon, we change the scope of this PR,
This PR tries to:1. Support random sampling in rejection sampler. This should be general to different drafting methods, not limited to ngram spec decode.6. Clean up and reuse rejection sampling tests from V0.This PR does not:1. Change model runner to use rejection sampler with random sampling. We need one extra PR to support ngram with random sampling.