[V1] Detokenizer: Respect Stop Tokens + not include_stop_str_in_output

[afeldman-nm]

This PR plumbs the engine core finish_reason into the incremental_detokenizer.update() method. The invariant is that if finish_reason == STOP in the engine core output, the engine core must have detected a token-based stop condition (EOS or stop-token.) The detokenizer will truncate the most recent token's detokenized text from the output text.

FIX #14623

[github-actions]

👋 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 fastcheck CI which starts running only a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of those by going to your fastcheck build on Buildkite UI (linked in the PR checks section) and unblock them. If you do not have permission to unblock, ping simon-mo or khluu to add you in our Buildkite org.

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 ready label to the PR or enable auto-merge.

🚀

[markmc]

I'm not super familiar with any of this. I'd need to study V0 behavior before being confident in this approach, basically to answer my questions in comments. Have you compared to V0?

[afeldman-nm]

Not quite finished yet (need to add unit tests, refactor, address feedback) but opening for review since we are trying to finish ASAP.

[njhill]

Thanks @afeldman-nm.

My thought is similar to @markmc and @robertgshaw2-redhat's.

My suggestion would be:

• Pass a bool stop_terminated flag to update() .. which when can be called with finish_reason == STOP
• Inside update(), if stop_terminated and self.include_stop_str_in_output is True, skip detokenizing the last token in new_token_ids (which is typically the only token but might not be). We still want to add that token to self.token_ids but don't need to do anything else for it that's inside the for loop.

I think that would also address @robertgshaw2-redhat's brittleness concerns.

---

An alternative suggestion would be to just have the detokenizer also handle the eos and stop token id evaluation like it does for stop strings, and move it out of the engine. The downside of this is that we would be generating an extra token in those cases (and eos is most common) so I'd probably still prefer to avoid it.

[njhill]

On second thoughts, maybe @robertgshaw2-redhat's suggestion is better, basically to do the eos and stop token id checking in the update method and handle selective detokenization of the final token accordingly. But also leave it in the engine.

An annoying thing about this is that we don't need to send an abort and it would be preferable not to. But then we need to have additional conditional logic in the calling output processor method to only send the abort if the stop_reason is a str (not if it's an int or None)

[afeldman-nm]

On second thoughts, maybe @robertgshaw2-redhat's suggestion is better, basically to do the eos and stop token id checking in the update method and handle selective detokenization of the final token accordingly. But also leave it in the engine.

An annoying thing about this is that we don't need to send an abort and it would be preferable not to. But then we need to have additional conditional logic in the calling output processor method to only send the abort if the stop_reason is a str (not if it's an int or None)

I implemented roughly a compromise between your and Robert's suggestions - the last token skips detokenization if a stop token was detected, and the detokenizer's stop-token check validates that the last token is actually a valid EOS or stop token.

[afeldman-nm]

JFYI, currently adding unit tests, still WIP

[robertgshaw2-redhat]

Is this ready to merge?

[njhill]

@afeldman-nm I think the changes can be much smaller, I don't think we need any new methods.

I've gone back to thinking my first suggestion would be better. I made the changes in a commit here to illustrate: njhill@bc26b30, feel free to pull them in.

[afeldman-nm]

@afeldman-nm I think the changes can be much smaller, I don't think we need any new methods.

I've gone back to thinking my first suggestion would be better. I made the changes in a commit here to illustrate: njhill@bc26b30, feel free to pull them in.

Good restructuring @njhill . Minor nit, I incorporated Rob's suggestion (actually validating that the engine core stop was triggered by an EOS or an element of stop_token_ids) into your commit; this restructures things slightly but overall I was able to avoid adding any helper functions and maintain simplicity.

[afeldman-nm]

I think that this PR is /ready

[njhill]

@afeldman-nm I think the changes can be much smaller, I don't think we need any new methods.
I've gone back to thinking my first suggestion would be better. I made the changes in a commit here to illustrate: njhill@bc26b30, feel free to pull them in.

Good restructuring @njhill . Minor nit, I incorporated Rob's suggestion (actually validating that the engine core stop was triggered by an EOS or an element of stop_token_ids) into your commit; this restructures things slightly but overall I was able to avoid adding any helper functions and maintain simplicity.

Thanks @afeldman-nm. IMHO though the extra validation (and associated additional complexity) here is unnecessary. Passing stop_terminated=True to that method is telling it that the request is stopping due to the last token being a stop token (which in turn has been communicated via STOP finish reason returned by the engine). So it seems a bit arbitrary to re-check. I really think we should keep it simpler.

[afeldman-nm]

@afeldman-nm I think the changes can be much smaller, I don't think we need any new methods.
I've gone back to thinking my first suggestion would be better. I made the changes in a commit here to illustrate: njhill@bc26b30, feel free to pull them in.

Good restructuring @njhill . Minor nit, I incorporated Rob's suggestion (actually validating that the engine core stop was triggered by an EOS or an element of stop_token_ids) into your commit; this restructures things slightly but overall I was able to avoid adding any helper functions and maintain simplicity.

Thanks @afeldman-nm. IMHO though the extra validation (and associated additional complexity) here is unnecessary. Passing stop_terminated=True to that method is telling it that the request is stopping due to the last token being a stop token (which in turn has been communicated via STOP finish reason returned by the engine). So it seems a bit arbitrary to re-check. I really think we should keep it simpler.

Okay @njhill , I made a change to reflect your suggestion.

[njhill]

@afeldman-nm just a few more small things (sorry!)

[njhill]

Thanks @afeldman-nm!