Skip to content

[V1][Perf] Faster incremental detokenization#15137

Merged
vllm-bot merged 16 commits intovllm-project:mainfrom
njhill:tokenizers-detok
Apr 17, 2025
Merged

[V1][Perf] Faster incremental detokenization#15137
vllm-bot merged 16 commits intovllm-project:mainfrom
njhill:tokenizers-detok

Conversation

@njhill
Copy link
Member

@njhill njhill commented Mar 19, 2025 •
edited by github-actions bot
Loading

Exploit tokenizers library's new DecodeStream functionality for "fast" HF tokenizers.

This won't help with mistral tokenizers unfortunately.

cc @robertgshaw2-redhat

@njhill
[V1] Faster incremental detokenization
0d1bb72 
Exploit tokenizers library's new DecodeStream functionality for "fast" HF tokenizers.

This won't help with mistral tokenizers unfortunately.

Signed-off-by: Nick Hill <nhill@redhat.com>
@github-actions
Copy link

github-actions bot commented Mar 19, 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.

🚀

@mergify mergify bot added the ci/build label Mar 19, 2025
njhill added 2 commits March 19, 2025 09:33
@njhill
update tokenizers version used in tests
c6ab7a3 
Signed-off-by: Nick Hill <nhill@redhat.com>
@njhill
add some comments
5fb62f2 
Signed-off-by: Nick Hill <nhill@redhat.com>
@njhill njhill changed the title [V1] Faster incremental detokenization [V1][Perf] Faster incremental detokenization Mar 19, 2025
@WoosukKwon
Copy link
Collaborator

WoosukKwon commented Mar 19, 2025

@njhill Thanks for doing this! Do you have any performance numbers by any chance?

@njhill
Copy link
Member Author

njhill commented Mar 19, 2025

@WoosukKwon looks like it's noticeable for higher QPS:

Before:

============ Serving Benchmark Result ============
Successful requests:                     6000      
Benchmark duration (s):                  81.47     
Total input tokens:                      1322102   
Total generated tokens:                  1205048   
Request throughput (req/s):              73.65     
Output token throughput (tok/s):         14791.01  
Total Token throughput (tok/s):          31018.76  
---------------Time to First Token----------------
Mean TTFT (ms):                          207.24    
Median TTFT (ms):                        179.43    
P99 TTFT (ms):                           1081.34   
-----Time per Output Token (excl. 1st token)------
Mean TPOT (ms):                          22.78     
Median TPOT (ms):                        22.36     
P99 TPOT (ms):                           34.43     
---------------Inter-token Latency----------------
Mean ITL (ms):                           26.03     
Median ITL (ms):                         25.08     
P99 ITL (ms):                            48.39     
==================================================

After:

============ Serving Benchmark Result ============
Successful requests:                     6000      
Benchmark duration (s):                  80.39     
Total input tokens:                      1322102   
Total generated tokens:                  1205048   
Request throughput (req/s):              74.64     
Output token throughput (tok/s):         14990.72  
Total Token throughput (tok/s):          31437.59  
---------------Time to First Token----------------
Mean TTFT (ms):                          166.37    
Median TTFT (ms):                        136.98    
P99 TTFT (ms):                           1054.43   
-----Time per Output Token (excl. 1st token)------
Mean TPOT (ms):                          22.49     
Median TPOT (ms):                        22.21     
P99 TPOT (ms):                           34.91     
---------------Inter-token Latency----------------
Mean ITL (ms):                           23.49     
Median ITL (ms):                         21.41     
P99 ITL (ms):                            44.67     
==================================================

@WoosukKwon
Copy link
Collaborator

WoosukKwon commented Mar 21, 2025

@njhill The speedup looks promising! 🚀

Is this PR ready for review? If so, could you please add tests?

@njhill
Copy link
Member Author

njhill commented Mar 21, 2025

@WoosukKwon yes it's ready apart from some additional tests.

@WoosukKwon
Copy link
Collaborator

WoosukKwon commented Mar 21, 2025

@njhill Got it. Could you please resolve the merge conflict?

@njhill
Copy link
Member Author

njhill commented Mar 21, 2025

@WoosukKwon I'm just updating tests, will push those and resolve conflict at the same time.

njhill added 3 commits March 21, 2025 19:33
@njhill
overhaul detokenizer tests
fc68b10 
Signed-off-by: Nick Hill <nhill@redhat.com>
@njhill
Merge remote-tracking branch 'origin/main' into tokenizers-detok
7863416
@njhill
uncomment other models
d546539 
Signed-off-by: Nick Hill <nhill@redhat.com>
@njhill
Copy link
Member Author

njhill commented Mar 22, 2025

Ready now @WoosukKwon, sorry the tests turned out to be a bit of a can of worms 😅

@njhill njhill added the ready ONLY add when PR is ready to merge/full CI is needed label Mar 22, 2025
njhill added 2 commits March 22, 2025 08:23
@njhill
more test fixes for mistral tokenizer
f339a85 
Signed-off-by: Nick Hill <nhill@redhat.com>
@njhill
fix test_decode_prompt_logprobs
1b097a0 
Signed-off-by: Nick Hill <nhill@redhat.com>
@WoosukKwon
Copy link
Collaborator

WoosukKwon commented Mar 23, 2025

@robertgshaw2-redhat would you be interested in reviewing this PR? I'm not sure if any reviewer is assigned.

@njhill
skip tests with special toks for mistral tokenizer
d045036 
Signed-off-by: Nick Hill <nhill@redhat.com>
@robertgshaw2-redhat robertgshaw2-redhat mentioned this pull request Mar 24, 2025
Closed
1 task
@njhill
Copy link
Member Author

njhill commented Mar 24, 2025

I will push a fix for the remaining test failure asap.

robertgshaw2-redhat
robertgshaw2-redhat reviewed Mar 24, 2025
View reviewed changes
vllm/v1/engine/detokenizer.py
prompt_suffix = suffix
break

# Prime the stream.
Copy link
Collaborator

@robertgshaw2-redhat robertgshaw2-redhat Mar 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

nice way to skip the priming.

robertgshaw2-redhat
robertgshaw2-redhat approved these changes Mar 24, 2025
View reviewed changes
Copy link
Collaborator

@robertgshaw2-redhat robertgshaw2-redhat left a comment

Choose a reason for hiding this comment

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

Nice work.

@njhill
Copy link
Member Author

njhill commented Mar 25, 2025

Looks like the remaining test failures are "real", need to dig into the behavior differences that are causing them.

@mergify
Copy link

mergify bot commented Apr 8, 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 8, 2025
@njhill
Merge remote-tracking branch 'refs/remotes/origin/main' into tokenize…
480f325 
…rs-detok

# Conflicts:
#	requirements/common.txt
#	requirements/test.in
@mergify mergify bot removed the needs-rebase label Apr 16, 2025
@njhill
Copy link
Member Author

njhill commented Apr 17, 2025

Addressed the remaining test failures (due to incorrect behaviour of the existing incremental detokenization which doesn't skip some special tokens when it should). So should now be good to merge.

@vllm-bot vllm-bot merged commit 05fcd1b into vllm-project:main Apr 17, 2025
64 of 69 checks passed
@njhill njhill deleted the tokenizers-detok branch April 17, 2025 14:51
tarukumar pushed a commit to tarukumar/vllm that referenced this pull request Apr 17, 2025
@njhill @tarukumar
[V1][Perf] Faster incremental detokenization (vllm-project#15137)
59ea0f3 
Signed-off-by: Nick Hill <nhill@redhat.com>
tarukumar pushed a commit to tarukumar/vllm that referenced this pull request Apr 17, 2025
@njhill @tarukumar
[V1][Perf] Faster incremental detokenization (vllm-project#15137)
779db30 
Signed-off-by: Nick Hill <nhill@redhat.com>
yangw-dev pushed a commit to yangw-dev/vllm that referenced this pull request Apr 21, 2025
@njhill @yangw-dev
[V1][Perf] Faster incremental detokenization (vllm-project#15137)
6535ed9 
Signed-off-by: Nick Hill <nhill@redhat.com>
Signed-off-by: Yang Wang <elainewy@meta.com>
@chaunceyjiang chaunceyjiang mentioned this pull request Apr 22, 2025
Closed
1 task
jikunshang pushed a commit to jikunshang/vllm that referenced this pull request Apr 29, 2025
@njhill
[V1][Perf] Faster incremental detokenization (vllm-project#15137)
cd9a27d 
Signed-off-by: Nick Hill <nhill@redhat.com>
lk-chen pushed a commit to lk-chen/vllm that referenced this pull request Apr 29, 2025
@njhill @lk-chen
[V1][Perf] Faster incremental detokenization (vllm-project#15137)
0ea548e 
Signed-off-by: Nick Hill <nhill@redhat.com>
adobrzyn pushed a commit to HabanaAI/vllm-fork that referenced this pull request Apr 30, 2025
@njhill @adobrzyn
[V1][Perf] Faster incremental detokenization (vllm-project#15137)
9bc9c71 
Signed-off-by: Nick Hill <nhill@redhat.com>
Signed-off-by: Agata Dobrzyniewicz <adobrzyniewicz@habana.ai>
RichardoMrMu pushed a commit to RichardoMrMu/vllm that referenced this pull request May 12, 2025
@njhill
[V1][Perf] Faster incremental detokenization (vllm-project#15137)
19a31f5 
Signed-off-by: Nick Hill <nhill@redhat.com>
Signed-off-by: Mu Huai <tianbowen.tbw@antgroup.com>
@ckhordiasma ckhordiasma mentioned this pull request May 14, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@robertgshaw2-redhat robertgshaw2-redhat robertgshaw2-redhat approved these changes

@WoosukKwon WoosukKwon Awaiting requested review from WoosukKwon

@ywang96 ywang96 Awaiting requested review from ywang96

@comaniac comaniac Awaiting requested review from comaniac

@alexm-redhat alexm-redhat Awaiting requested review from alexm-redhat

Assignees

No one assigned

Labels

ci/build frontend ready ONLY add when PR is ready to merge/full CI is needed v1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@njhill @WoosukKwon @robertgshaw2-redhat @vllm-bot