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.

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

Make initialization of tokenizer and detokenizer optional #3748

Merged
merged 47 commits into from
Apr 21, 2024

Conversation

GeauxEric
Copy link
Contributor

Add a flag to initialize the LLM engine to disable tokenizer.
If tokenizer is disabled, so does detokenizer.

FIX #3635
FIX #3647

@ywang96
Copy link
Member

ywang96 commented Mar 31, 2024

Thanks for making this PR! It appears to me that unlike #3749 to allow skipping detokenization at sampling param level, this PR simply disables the use of tokenizer as a whole, so I guess the two PRs can still co-exist.

We might want to be careful with all these API changes though - cc @simon-mo

@ywang96 ywang96 self-assigned this Apr 1, 2024
@GeauxEric GeauxEric marked this pull request as draft April 2, 2024 05:24
@GeauxEric GeauxEric marked this pull request as ready for review April 2, 2024 16:47
@GeauxEric
Copy link
Contributor Author

GeauxEric commented Apr 2, 2024

Ready for review.
@simon-mo @ywang96
could you pls take a look?

@ywang96
Copy link
Member

ywang96 commented Apr 4, 2024

@GeauxEric Thank you for the contribution! Could you update this branch with the changes merged in the main branch? Happy to review after the conflicts are resolved!

@GeauxEric
Copy link
Contributor Author

GeauxEric commented Apr 6, 2024

@GeauxEric Thank you for the contribution! Could you update this branch with the changes merged in the main branch? Happy to review after the conflicts are resolved!

@ywang96
conflicts resolved.

@GeauxEric GeauxEric marked this pull request as ready for review April 17, 2024 19:57
@GeauxEric
Copy link
Contributor Author

Maybe others can help take a look? @simon-mo?

Copy link
Member

@ywang96 ywang96 left a comment

Choose a reason for hiding this comment

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

Sorry for the late review - I can see the purpose of having this. Since it doesn't affect other downstream components too much, we should be fine with having this feature!

@@ -95,6 +96,10 @@ def add_cli_args(
type=str,
default=EngineArgs.tokenizer,
help='name or path of the huggingface tokenizer to use')
parser.add_argument(
'--skip_tokenizer_init',
Copy link
Member

Choose a reason for hiding this comment

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

format nit

Suggested change
'--skip_tokenizer_init',
'--skip-tokenizer-init',

Comment on lines +343 to +346
eos_token_id = None
if self.tokenizer:
eos_token_id = self.tokenizer.get_lora_tokenizer(
lora_request).eos_token_id
Copy link
Member

Choose a reason for hiding this comment

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

I still think it's worth having an warning here about eos_token_id being None.

@GeauxEric GeauxEric marked this pull request as draft April 18, 2024 17:48
@GeauxEric GeauxEric marked this pull request as ready for review April 18, 2024 22:18
@GeauxEric GeauxEric requested a review from ywang96 April 18, 2024 22:18
@GeauxEric
Copy link
Contributor Author

Could you please take another look and consider merging/approving it when you have a moment?

Copy link
Member

@ywang96 ywang96 left a comment

Choose a reason for hiding this comment

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

@GeauxEric Sorry for the delayed review and thank you for addressing the comments and contributing this PR!

@ywang96 ywang96 enabled auto-merge (squash) April 21, 2024 21:33
@ywang96 ywang96 merged commit a37d815 into vllm-project:main Apr 21, 2024
46 of 47 checks passed
robertgshaw2-neuralmagic pushed a commit to neuralmagic/nm-vllm that referenced this pull request Apr 21, 2024
…ct#3748)

Co-authored-by: Yun Ding <yunding@nvidia.com>
Co-authored-by: Roger Wang <ywang@roblox.com>
ZackBradshaw pushed a commit to ZackBradshaw/vllm that referenced this pull request Apr 22, 2024
…ct#3748)

Co-authored-by: Yun Ding <yunding@nvidia.com>
Co-authored-by: Roger Wang <ywang@roblox.com>
ZackBradshaw pushed a commit to ZackBradshaw/vllm that referenced this pull request Apr 22, 2024
…ct#3748)

Co-authored-by: Yun Ding <yunding@nvidia.com>
Co-authored-by: Roger Wang <ywang@roblox.com>
ZackBradshaw pushed a commit to ZackBradshaw/vllm that referenced this pull request Apr 22, 2024
…ct#3748)

Co-authored-by: Yun Ding <yunding@nvidia.com>
Co-authored-by: Roger Wang <ywang@roblox.com>
xjpang pushed a commit to xjpang/vllm that referenced this pull request Apr 25, 2024
…ct#3748)

Co-authored-by: Yun Ding <yunding@nvidia.com>
Co-authored-by: Roger Wang <ywang@roblox.com>
robertgshaw2-neuralmagic pushed a commit to neuralmagic/nm-vllm that referenced this pull request Apr 26, 2024
…ct#3748)

Co-authored-by: Yun Ding <yunding@nvidia.com>
Co-authored-by: Roger Wang <ywang@roblox.com>
alexeykondrat pushed a commit to alexeykondrat/ci-vllm that referenced this pull request May 1, 2024
…ct#3748)

Co-authored-by: Yun Ding <yunding@nvidia.com>
Co-authored-by: Roger Wang <ywang@roblox.com>
z103cb pushed a commit to z103cb/opendatahub_vllm that referenced this pull request May 7, 2024
…ct#3748)

Co-authored-by: Yun Ding <yunding@nvidia.com>
Co-authored-by: Roger Wang <ywang@roblox.com>
Temirulan pushed a commit to Temirulan/vllm-whisper that referenced this pull request Sep 6, 2024
…ct#3748)

Co-authored-by: Yun Ding <yunding@nvidia.com>
Co-authored-by: Roger Wang <ywang@roblox.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants