fix: logits processor state at each step #544
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Wallas Santos <[email protected]>
Signed-off-by: Wallas Santos <[email protected]>
wallashss
requested review from
nikolaospapandreou,
prashantgupta24,
rafvasq,
sducouedic,
tdoublep and
yannicks1
as code owners
|
👋 Hi! Thank you for contributing to vLLM support on Spyre. Or this can be done with Now you are good to go 🚀 |
tests/e2e/test_sampling_params.py
Outdated
| # after min tokens reached the logits processor is properly | ||
| # cleared. | ||
| assert len(output1.outputs[0].token_ids) < 20 | ||
| assert len(output2.outputs[0].token_ids) < 10 |
Collaborator
There was a problem hiding this comment.
If increase the eos_id logit bias to force it to be generated then we can assert on the exact output length, right?
assert len(output1.outputs[0].token_ids) == 11
assert len(output2.outputs[0].token_ids) == 1
(the values for those asserts may be off-by-one depending on how EOS is tracked in the outputs 😅)
Signed-off-by: Wallas Santos <[email protected]>
Signed-off-by: Wallas Santos <[email protected]>
github-actions
bot
added
the
ready
Collaborator
Author
|
bot:test |
tjohnson31415
added a commit
that referenced
this pull request
Oct 29, 2025
…545) # Description The MinTokensLogitsProcessor needs to get a `batch_update` at each step to detect when enough tokens have been generated. The `LogitProcessorWrapper` copied the typical logic of skipping updates when batch_update is None, but this meant that min tokens would not get the needed call to `update_state`. The fix here is to always call `update_state` on each of the wrapped logitsprocs in the batch, with a some extra code to not call `update_state` for a particular index more than once. ## Related Issues Follow up to #544 which fixed the behavior for static batching Cherry-picked improvement to test_sampling_params.py from #536 --------- Signed-off-by: Travis Johnson <[email protected]> Co-authored-by: Wallas Santos <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes the update of logits processors that need to be updated at each engine step. To validate the change, I updated the existing test for min tokens where we can identify the wrong behaviour. Note: the bug is reproducible in both CB and SB.