-
-
Couldn't load subscription status.
- Fork 10.8k
[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 <qiong@phonic.co>
|
👋 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 <qiong@phonic.co>
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 <qiong@phonic.co> Signed-off-by: 汪志鹏 <wangzhipeng628@gmail.com>
Signed-off-by: Qiong Zhou Huang <qiong@phonic.co> Signed-off-by: Mu Huai <tianbowen.tbw@antgroup.com>
Signed-off-by: Qiong Zhou Huang <qiong@phonic.co>
Signed-off-by: Qiong Zhou Huang <qiong@phonic.co> Signed-off-by: Yuqi Zhang <yuqizhang@google.com>
When using the
bad_wordsSamplingParam with Mistral models with V1,bad_wordscompiles incorrectly.For example, when
bad_words=["the"]_bad_words_token_idsis[[1, 3265]]when it should be[[3265]], so the word "the" is never matched.We fix this by setting
add_special_tokens=Falsefor all tokenizers when we generate_bad_words_token_idsIt 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