[bugfix] fix bug when top_logprobs=0 with spec decoding#30059
[bugfix] fix bug when top_logprobs=0 with spec decoding#30059njhill merged 6 commits intovllm-project:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request effectively resolves a bug in speculative decoding where setting top_logprobs=0 would cause a server error. The fix, which changes the condition for checking sampling_metadata.max_num_logprobs from a truthiness check to an explicit is not None check, is correct and idiomatic. This ensures that logprobs are computed when top_logprobs is 0, aligning the rejection sampler's behavior with the standard sampler and preventing the reported server error. The change is minimal and precisely targets the bug.
Signed-off-by: realliujiaxu <realliujiaxu@163.com>
05339da to
b667710
Compare
|
Thank you @realliujiaxu! Would you be willing to add a simple test for this? |
Done. I've added a simple test, and tested it locally. Thanks! @njhill Before the fix, the newly added unit test failed: |
Signed-off-by: realliujiaxu <realliujiaxu@163.com>
|
@njhill Can we merge this PR? |
|
@realliujiaxu looks like there is a related test failure: https://buildkite.com/vllm/ci/builds/42680#019b0423-41b0-45c7-94f4-c9f2473a6183 |
Head branch was pushed to by a user without write access
|
@njhill the test failure if fixed, please review again |
|
Thanks @realliujiaxu ... re the test fix, it looks like you changed |
njhill
left a comment
There was a problem hiding this comment.
Just blocking until answer to prior question is understood!
(If I understand correctly, you are wondering why I changed the test and if having
If the actual sampler were executed,
Perhaps option 2 is more reasonable? Looking forward to your further advice. @njhill |
|
Thanks @realliujiaxu, makes sense, I'm happy with whichever you think is best. |
|
@njhill I think the current approach is simple and clear enough. Can we merge this PR? |
…#30059) Signed-off-by: realliujiaxu <realliujiaxu@163.com>
…#30059) Signed-off-by: realliujiaxu <realliujiaxu@163.com> Signed-off-by: Ubuntu <mjtaheri68@gmail.com>
…#30059) Signed-off-by: realliujiaxu <realliujiaxu@163.com> Signed-off-by: dsuhinin <suhinin.dmitriy@gmail.com>
Purpose
#26060 adds support for returning logprobs for v1 spec decoding. However, if top_logprobs is not set or set to 0, an error will occur:
The reason is that the rejection sampler returns logprobs as
None. The fix is to follow the sampler's approach: even whentop_logprobs=0, logprobs should still be collected.Test Plan
Run server
test with
top_logprobs=0Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.