Conversation
Signed-off-by: mgoin <mgoin64@gmail.com>
Signed-off-by: mgoin <mgoin64@gmail.com>
Signed-off-by: mgoin <mgoin64@gmail.com>
There was a problem hiding this comment.
Code Review
This pull request introduces several well-reasoned performance optimizations, primarily targeting beam search. The changes include adding various caching mechanisms, such as using cached_property, a new cache_if_not_none decorator, and memoizing results in KVCacheManager, as well as avoiding unnecessary work like cloning SamplingParams. Overall, these are solid improvements. However, I've identified a significant thread-safety issue in the new cache_if_not_none decorator that could lead to race conditions in a concurrent environment. I have provided a detailed comment with a suggested fix for this issue.
hmellor
left a comment
There was a problem hiding this comment.
LGTM, just one micro-optimisation nit
| # into a dictionary twice is the same as doing it once. | ||
| topk_ranks = range(1, num_logprobs + 1) | ||
| ranks = itertools.chain((rank,), topk_ranks) | ||
| ranks = [rank] + list(range(1, num_logprobs + 1)) |
There was a problem hiding this comment.
This is about 1.4x faster if we want to micro-optimise
| ranks = [rank] + list(range(1, num_logprobs + 1)) | |
| ranks = [rank, *range(1, num_logprobs + 1)] |
There was a problem hiding this comment.
As the construction of ranks are fixed, I was thinking about simply get rid of this list, and just pass in the sampled rank here.
Then we could just do
# insert sampled token
...
# insert top ranked tokens in order
for rank, (token, logprob) in enumerate(zip(tokens[1:], logprobs[1:]))
| #!/usr/bin/env python3 | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| # SPDX-FileCopyrightText: Copyright contributors to the vLLM project | ||
| """Simple benchmark script to test beam search performance via OpenAI API. |
There was a problem hiding this comment.
I think we have can benchmark beam search with vllm bench latency and vllm bench throughput
There was a problem hiding this comment.
I didn't realize, will try to use it
| if ( | ||
| request.block_hashes | ||
| and self._cache_hit_cache is not None | ||
| and max_cache_hit_length == self._cache_hit_cache.max_cache_hit_length |
There was a problem hiding this comment.
Does this constraint mean we need to recompute find_longest_cache_hit for each beam search iteration?
There was a problem hiding this comment.
Yes, each beam search iteration submits n identical requests to the engine core and the core is unaware of this
| and self._cache_hit_cache is not None | ||
| and max_cache_hit_length == self._cache_hit_cache.max_cache_hit_length | ||
| and len(request.block_hashes) == self._cache_hit_cache.num_block_hashes | ||
| and request.block_hashes[-1] == self._cache_hit_cache.last_block_hash |
There was a problem hiding this comment.
does this mean the optimization works only when only the last partial block of the current request and the previous request is different and has no effect if different candidates have different prefix at the beginning? Do you know the cache hit rate of this optimization?
There was a problem hiding this comment.
The intent of this optimization is just to deal with the case when the current request is identical to the previous request. My assumption was that because a given block_hash "includes" all the previous blocks, I could just look at the last one to check for equality. This is a huge boost to beam search n=30 because all requests perform the same search
Here are profiles showing that "Self duration" of schedule decreases from 6.060ms to 1.471ms
| self.coordinator.find_longest_cache_hit( | ||
| request.block_hashes, max_cache_hit_length | ||
|
|
||
| # Optimization: Reuse previous result when consecutive requests are the same. |
There was a problem hiding this comment.
can you make this optimization optional to beam search? e.g., add a flag?
There was a problem hiding this comment.
@mgoin @heheda12345 I thought about it before, and would love to see if it's possible. For find_longest_cache_hit, should we do binary search instead of linear search?
In this way, maybe we could consolidate the code without special handling for beam search?
| and max_cache_hit_length == self._cache_hit_cache.max_cache_hit_length | ||
| and len(request.block_hashes) == self._cache_hit_cache.num_block_hashes | ||
| and request.block_hashes[-1] == self._cache_hit_cache.last_block_hash | ||
| ): |
There was a problem hiding this comment.
We can reuse the computed_blocks of the previous request only when we can ensure these blocks are not evicted.
- If the model only has full attention, I guess it's fine but want some proof on it
- If the model has sliding window attention, I feel there may be some problem as the previous request may free the early blocks outside the sliding window and make these blocks evocable.
|
We are using prefix caching to do beam search. Does it mean with block_size 16 and prompt length 30, we always need to recompute the prefill of 14 tokens? |
| data that is expensive to copy. However, if not copied, the processor | ||
| needs to support parallel decoding for multiple sequences | ||
| See https://github.com/vllm-project/vllm/issues/3087 | ||
|
|
There was a problem hiding this comment.
We can also cleanup the logitsprocessor stuff in this method since they aren't supported in SamplingParams anymore
| max_tokens=1, | ||
| temperature=temperature, | ||
| detokenize=False, # We detokenize the output after the search | ||
| skip_clone=True, # Safe to reuse params without cloning |
There was a problem hiding this comment.
I think we could set this for all of the API server endpoints
| # into a dictionary twice is the same as doing it once. | ||
| topk_ranks = range(1, num_logprobs + 1) | ||
| ranks = itertools.chain((rank,), topk_ranks) | ||
| ranks = [rank] + list(range(1, num_logprobs + 1)) |
There was a problem hiding this comment.
As the construction of ranks are fixed, I was thinking about simply get rid of this list, and just pass in the sampled rank here.
Then we could just do
# insert sampled token
...
# insert top ranked tokens in order
for rank, (token, logprob) in enumerate(zip(tokens[1:], logprobs[1:]))
| skip_clone: bool = False | ||
| """Internal flag indicating that this SamplingParams instance is safe to | ||
| reuse without cloning. When True, clone() will return self without | ||
| performing a deep copy. This should only be set when the params object | ||
| is guaranteed to be dedicated to a single request and won't be modified | ||
| in ways that would affect other uses.""" |
There was a problem hiding this comment.
IIUC, this should be helpful for n-gen and beam search, correct?
And just curious, what's the risk to make it default to true? Or even make it as a consistent behaviour?
| self.coordinator.find_longest_cache_hit( | ||
| request.block_hashes, max_cache_hit_length | ||
|
|
||
| # Optimization: Reuse previous result when consecutive requests are the same. |
There was a problem hiding this comment.
@mgoin @heheda12345 I thought about it before, and would love to see if it's possible. For find_longest_cache_hit, should we do binary search instead of linear search?
In this way, maybe we could consolidate the code without special handling for beam search?
| logprobs=logprobs_num, | ||
| max_tokens=1, | ||
| temperature=temperature, | ||
| detokenize=False, # We detokenize the output after the search |
There was a problem hiding this comment.
+1 on this. I'm wondering if we should directly remove tokenized string in Logprob. IMO,
- detokenization should never live inside Engine.
- clients are rarely used the data, if they do, it should be cleaner to do it on their side instead.
|
Hi @mgoin, I’m currently using vLLM with beam search and was wondering if there are any updates on the status of this PR? I really appreciate all the work you're doing on this!! |
|
This pull request has been automatically marked as stale because it has not had any activity within 90 days. It will be automatically closed if no further activity occurs within 30 days. Leave a comment if you feel this pull request should remain open. Thank you! |
|
This pull request has been automatically closed due to inactivity. Please feel free to reopen if you intend to continue working on it. Thank you! |


Purpose
detokenize=Falsein SamplingParams during the beam search since we manually detokenize at the endskip_cloneparameter to SamplingParams to skip the defensive clone in processor when trustedappend_logprobs_for_next_positionto boost perf for default case (using FlatLogprobs was slower due to the amount of indexing needed)get_eos_token_id. These were tokenizing the eos text in every preprocess call for each requestis_encoder_decoderas it is expensive to read through the model's configkv_cache_manager::get_computed_blockswhere we reuse the previous result when consecutive requests are the same, allowing us to skip find_longest_cache_hit for the rest of the requestsBenchmark command for beam_search with n=30 on H100:
Before:
After
Test Plan
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.