Skip to content
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

[BugFix] Apply get_cached_tokenizer to the tokenizer setter of LLM #5207

Merged
merged 3 commits into from
Jun 5, 2024
Merged

[BugFix] Apply get_cached_tokenizer to the tokenizer setter of LLM #5207

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
89a5223
[BugFix] Change the method to get lengh of tokenizer from 'len' to 'v…
Jun 3, 2024
1de6859
[BugFix] LLM init tokenizer to cached tokenizer
Jun 3, 2024
08d3181
[BugFix] Add CachedTokenizer comparison before setting tokenizer in LLM
Jun 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion vllm/entrypoints/llm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
from vllm.outputs import EmbeddingRequestOutput, RequestOutput
from vllm.pooling_params import PoolingParams
from vllm.sampling_params import SamplingParams
from vllm.transformers_utils.tokenizer import get_cached_tokenizer
from vllm.usage.usage_lib import UsageContext
from vllm.utils import Counter, deprecate_kwargs

Expand Down Expand Up @@ -152,7 +153,7 @@ def set_tokenizer(
self,
tokenizer: Union[PreTrainedTokenizer, PreTrainedTokenizerFast],
) -> None:
self.llm_engine.tokenizer.tokenizer = tokenizer
Copy link
Member

Choose a reason for hiding this comment

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

better add a check, if the tokenizer is already cached, then just set it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A check is added. While the CachedTokenizer is dynamic, I have no choice but compare the class name. Is there any good ideal?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the string compare is fine here. If you wanted to be extremely safe you could change CachedTokenizer to be VLLMCachedTokenizer

self.llm_engine.tokenizer.tokenizer = get_cached_tokenizer(tokenizer)

@overload # LEGACY: single (prompt + optional token ids)
def generate(
Expand Down
2 changes: 1 addition & 1 deletion vllm/transformers_utils/tokenizer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ def get_cached_tokenizer(
tokenizer_all_special_tokens_extended = (
tokenizer.all_special_tokens_extended)
tokenizer_all_special_tokens = set(tokenizer.all_special_tokens)
tokenizer_len = len(tokenizer)
DriverSong marked this conversation as resolved.
Show resolved Hide resolved
tokenizer_len = tokenizer.vocab_size

class CachedTokenizer(tokenizer.__class__): # type: ignore

Expand Down
Loading