Skip to content

[BugFix][Frontend] Fix LLM.chat() tokenization #16081

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Apr 25, 2025
Merged

[BugFix][Frontend] Fix LLM.chat() tokenization #16081

merged 3 commits into from
Apr 25, 2025

Conversation

njhill
Copy link
Member

@njhill njhill commented Apr 5, 2025 •
edited by DarkLight1337
Loading

Chat templates generally include special tokens and so add_special_tokens=False should be used when tokenizing the template outputs. This is already the default behaviour of the openai chat completions endpoint.

In particular this avoids the llama double-BOS token issue.

Also, LoRA adapter-specific tokenizers weren't being used by LLM.chat().

Fixes #16028
Fixes #16853

Also just found related issues:

@njhill
[BugFix][Frontend] Fix LLM.chat() tokenization
7c9ff40 
Chat templates generally include special tokens and so add_special_tokens=False should be used when tokenizing the template outputs.

In particular this avoids the llama double-BOS token issue.

Also, LoRA adapter-specific tokenizers weren't being used by LLM.chat().

Signed-off-by: Nick Hill <[email protected]>
@njhill njhill added bug Something isn't working frontend needs-tests Tests needed for this PR labels Apr 5, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 5, 2025

👋 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.

🚀

@njhill njhill requested a review from DarkLight1337 April 5, 2025 01:27
@njhill njhill mentioned this pull request Apr 5, 2025
Open
@njhill
Copy link
Member Author

njhill commented Apr 5, 2025

@DarkLight1337 I guess this as-is bypasses one of the length checks, but is at least more consistent with the chat completions api behaviour.

@DarkLight1337
Copy link
Member

As with the other PRs, can you add tests to avoid regressions in the future?

This was referenced Apr 18, 2025
Closed
Closed
mgoin
mgoin approved these changes Apr 22, 2025
View reviewed changes
Copy link
Member

@mgoin mgoin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we get this landed since several users have run into this issue?

@DarkLight1337 DarkLight1337 mentioned this pull request Apr 23, 2025
Open
1 task
@njhill njhill added this to the v0.8.5 milestone Apr 25, 2025
@mergify Mergify
Copy link

mergify bot commented Apr 25, 2025

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @njhill.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label Apr 25, 2025
njhill added 2 commits April 25, 2025 09:08
@njhill
Merge remote-tracking branch 'refs/remotes/origin/main' into fix-llm-…
fcd3f0c 
…chat-bos

# Conflicts:
#	vllm/entrypoints/llm.py
@njhill
Add test
343dccb 
Signed-off-by: Nick Hill <[email protected]>
@njhill njhill added ready ONLY add when PR is ready to merge/full CI is needed and removed needs-tests Tests needed for this PR labels Apr 25, 2025
@mergify mergify bot removed the needs-rebase label Apr 25, 2025
@njhill njhill mentioned this pull request Apr 25, 2025
Closed
@DarkLight1337 DarkLight1337 enabled auto-merge (squash) April 25, 2025 22:09
@DarkLight1337 DarkLight1337 merged commit 7011645 into vllm-project:main Apr 25, 2025
60 checks passed
@njhill njhill deleted the fix-llm-chat-bos branch April 25, 2025 22:36
jikunshang pushed a commit to jikunshang/vllm that referenced this pull request Apr 29, 2025
@njhill @dbyoung18
[BugFix][Frontend] Fix LLM.chat() tokenization (vllm-project#16081)
c99e037 
Signed-off-by: Nick Hill <[email protected]>
lk-chen pushed a commit to lk-chen/vllm that referenced this pull request Apr 29, 2025
@njhill @lk-chen
[BugFix][Frontend] Fix LLM.chat() tokenization (vllm-project#16081)
af5fd2b 
Signed-off-by: Nick Hill <[email protected]>
adobrzyn pushed a commit to HabanaAI/vllm-fork that referenced this pull request Apr 30, 2025
@njhill @adobrzyn
[BugFix][Frontend] Fix LLM.chat() tokenization (vllm-project#16081)
e822b0d 
Signed-off-by: Nick Hill <[email protected]>
Signed-off-by: Agata Dobrzyniewicz <[email protected]>
RichardoMrMu pushed a commit to RichardoMrMu/vllm that referenced this pull request May 12, 2025
@njhill
[BugFix][Frontend] Fix LLM.chat() tokenization (vllm-project#16081)
fd9d09f 
Signed-off-by: Nick Hill <[email protected]>
Signed-off-by: Mu Huai <[email protected]>
@ckhordiasma ckhordiasma mentioned this pull request May 14, 2025
Merged
zzzyq pushed a commit to zzzyq/vllm that referenced this pull request May 24, 2025
@njhill
[BugFix][Frontend] Fix LLM.chat() tokenization (vllm-project#16081)
525eae2 
Signed-off-by: Nick Hill <[email protected]>
Signed-off-by: Yuqi Zhang <[email protected]>
minpeter pushed a commit to minpeter/vllm that referenced this pull request Jun 24, 2025
@njhill @minpeter
[BugFix][Frontend] Fix LLM.chat() tokenization (vllm-project#16081)
fc53132 
Signed-off-by: Nick Hill <[email protected]>
Signed-off-by: minpeter <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mgoin mgoin mgoin approved these changes

@DarkLight1337 DarkLight1337 DarkLight1337 approved these changes

@robertgshaw2-redhat robertgshaw2-redhat Awaiting requested review from robertgshaw2-redhat robertgshaw2-redhat is a code owner

@simon-mo simon-mo Awaiting requested review from simon-mo simon-mo is a code owner

Assignees
No one assigned
Labels
bug Something isn't working frontend ready ONLY add when PR is ready to merge/full CI is needed
Projects
None yet
Milestone
v0.8.5
Development

Successfully merging this pull request may close these issues.

[Bug]: Two BOS when using chat [Bug]: Two beginning of sequence tokens for Lllama-3.2-3B-Instruct
3 participants
@njhill @DarkLight1337 @mgoin