[Bugfix] Handle the edge case in detokenizer where processed tokens contain both stop str and eos token
#23938
Conversation
… test Signed-off-by: dtransposed <damian.bogunowicz@gmail.com>
dtransposed
requested review from
WoosukKwon,
alexm-redhat,
comaniac,
njhill,
robertgshaw2-redhat and
ywang96
as code owners
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a bug where stop strings were ignored if an EOS token was present in the same batch of tokens. The proposed fix correctly reorders the logic to prioritize stop string evaluation before handling EOS token termination. The change is simple, effective, and accompanied by a new, thorough unit test that validates the corrected behavior for both including and excluding the stop string in the output. The fix appears correct and complete.
|
@dtransposed presumably you're talking about the spec decoding case? What about when the eos token occurs in |
Yes, this is precisely how i stumbled upon this edge case - when I tested a very efficient speculator. Is it possible though for detokenizer to see |
|
@njhill Could I ask you for further review please? |
njhill
left a comment
njhill
left a comment
There was a problem hiding this comment.
Thanks @dtransposed yes I see what you mean.
Please see inline comment and then we can merge this.
Signed-off-by: dtransposed <damian.bogunowicz@gmail.com>
|
@njhill. Could you take a look at the failing tests? They seems orthogonal to the PR logic. If it looks good to you can I ask you to update the branch and force merge? |
…ontain both `stop` str and `eos` token (vllm-project#23938) Signed-off-by: dtransposed <damian.bogunowicz@gmail.com>
…ontain both `stop` str and `eos` token (vllm-project#23938) Signed-off-by: dtransposed <damian.bogunowicz@gmail.com>
…ontain both `stop` str and `eos` token (vllm-project#23938) Signed-off-by: dtransposed <damian.bogunowicz@gmail.com>
…ontain both `stop` str and `eos` token (vllm-project#23938) Signed-off-by: dtransposed <damian.bogunowicz@gmail.com> Signed-off-by: xuebwang-amd <xuebwang@amd.com>
…ontain both `stop` str and `eos` token (vllm-project#23938) Signed-off-by: dtransposed <damian.bogunowicz@gmail.com> Signed-off-by: xuebwang-amd <xuebwang@amd.com>
…ontain both `stop` str and `eos` token (vllm-project#23938) Signed-off-by: dtransposed <damian.bogunowicz@gmail.com>
3 participants
Bug description 🐛
Right now,
BaseIncrementalDetokenizercan stop generation in two separate cases:EOS token found
If
<eos>appears innew_token_idsandstop_terminated=True, we hit the check at line 124 → and eventuallyreturn None.Stop string found
Otherwise, we use
StopCheckerto look for stop strings inside the decoded text fromnew_token_ids→ and eventuallyreturn stop_string.The missing case
We don’t handle the situation where both conditions happen at once:
new_token_idscontain an<eos>token andstopstring.In this case, the current code only respects the EOS path and skips the stop string check.
The fix
We should:
StopCheckerfirst (to handle stop strings properly).stop_terminated).By swapping the order, stop strings take priority over EOS, which produces the expected behavior.