-
-
Notifications
You must be signed in to change notification settings - Fork 10.7k
[Bugfix] Fix bad words for Mistral models #17753
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
Conversation
Signed-off-by: Qiong Zhou Huang <[email protected]>
👋 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 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 🚀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the bugfix! LGTM but I think the following code needs to be updated as well?
Lines 32 to 37 in 98c89e1
if isinstance(tokenizer, MistralTokenizer): | |
# Mistral tokenizers should not add special tokens | |
prompt_token_ids = tokenizer.encode(text=prompt) | |
else: | |
prompt_token_ids = tokenizer.encode(text=prompt, | |
add_special_tokens=False) |
if isinstance(tokenizer, MistralTokenizer): | ||
# Mistral tokenizers should not add special tokens | ||
prompt_token_ids = tokenizer.encode(text=prompt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the issue was caused by the BOS token so this makes sense
vllm/vllm/transformers_utils/tokenizers/mistral.py
Lines 369 to 374 in 98c89e1
if add_special_tokens is not None: | |
return self.tokenizer.encode(text, | |
bos=add_special_tokens, | |
eos=add_special_tokens) | |
else: | |
return self.tokenizer.encode(text, bos=True, eos=False) |
Signed-off-by: Qiong Zhou Huang <[email protected]>
Ah yes, I only updated it for V1. Just added it now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Signed-off-by: Qiong Zhou Huang <[email protected]> Signed-off-by: 汪志鹏 <[email protected]>
Signed-off-by: Qiong Zhou Huang <[email protected]> Signed-off-by: Mu Huai <[email protected]>
Signed-off-by: Qiong Zhou Huang <[email protected]>
Signed-off-by: Qiong Zhou Huang <[email protected]> Signed-off-by: Yuqi Zhang <[email protected]>
When using the
bad_words
SamplingParam with Mistral models with V1,bad_words
compiles incorrectly.For example, when
bad_words=["the"]
_bad_words_token_ids
is[[1, 3265]]
when it should be[[3265]]
, so the word "the" is never matched.We fix this by setting
add_special_tokens=False
for all tokenizers when we generate_bad_words_token_ids
It seems that for non-Mistral models too, we don't want to add the special tokens either, so it's a little unclear why this logic was included, as other models like Llama also include start of sequence tokens.

We can replicate this bug with any Mistral model using the original e2e testing code from #13376