-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Removed extra spaces from Mistral instruction template that were causing model to misbehave #5517
Removed extra spaces from Mistral instruction template that were causing model to misbehave #5517
Conversation
Merge dev branch
Merge dev branch
Merge dev branch
Merge dev branch
Merge dev branch
Merge dev branch
Merge dev branch
Merge dev branch
Merge dev branch
Merge dev branch
Merge dev branch
Merge dev branch
Merge dev branch
Merge dev branch
Merge dev branch
Merge dev branch
Merge dev branch
Merge dev branch
Merge dev branch
Merge dev branch
Merge dev branch
Merge dev branch
Merge dev branch (oobabooga#5257)
Merge dev branch
Merge dev branch
Merge dev branch
Merge dev branch
Merge dev branch
Merge dev branch
Merge dev branch
Merge dev branch
…ing Mistral to misbehave
I thought it did contain space before [INST] https://huggingface.co/mistralai/Mistral-7B-Instruct-v0.1
Unless I'm misrepresenting what < s > means? |
You can add a space before [INST] and it won't break it. For consistency with Mistral's official template though, I've removed both extra spaces on this fix. |
@@ -4,7 +4,7 @@ instruction_template: |- | |||
{{- message['content'] -}} | |||
{%- else -%} | |||
{%- if message['role'] == 'user' -%} | |||
{{-' [INST] ' + message['content'].rstrip() + ' [/INST] '-}} | |||
{{-'[INST] ' + message['content'].rstrip() + ' [/INST]'-}} | |||
{%- else -%} | |||
{{-'' + message['content'] + '</s>' -}} |
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.
Maybe the space should go here then? Like
{{-' ' + message['content'] + '</s>' -}}
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.
Maybe the space should go here then? Like
{{-' ' + message['content'] + '</s>' -}}
Mistral's official template doesn't add extra spaces for the assistant messages:
{% elif message['role'] == 'assistant' %}{{ message['content'] + eos_token}}
For consistency, I'd keep it as is, which is like Mistral's default.
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.
Yes, that's correct. But there seems to be a space after the </s>
at least:
{{ bos_token }}
{% for message in messages %}
{% if (message['role'] == 'user') != (loop.index0 % 2 == 0) %}
{{ raise_exception('Conversation roles must alternate user/assistant/user/assistant/...') }}
{% endif %}
{% if message['role'] == 'user' %}
{{ '[INST] ' + message['content'] + ' [/INST]' }}
{% elif message['role'] == 'assistant' %}
{{ message['content'] + eos_token + ' ' }}
{% else %}
{{ raise_exception('Only user and assistant roles are supported!') }}
{% endif %}
{% endfor %}
https://huggingface.co/mistralai/Mistral-7B-Instruct-v0.1/blob/main/tokenizer_config.json#L32
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.
Maybe not for the Mixtral one (see below). It's kind of frustrating that not even Mistral seems to know what the Mistral prompt should be.
{{ bos_token }}
{% for message in messages %}
{% if (message['role'] == 'user') != (loop.index0 % 2 == 0) %}
{{ raise_exception('Conversation roles must alternate user/assistant/user/assistant/...') }}
{% endif %}
{% if message['role'] == 'user' %}
{{ '[INST] ' + message['content'] + ' [/INST]' }}
{% elif message['role'] == 'assistant' %}
{{ message['content'] + eos_token }}
{% else %}
{{ raise_exception('Only user and assistant roles are supported!') }}
{% endif %}
{% endfor %}
https://huggingface.co/mistralai/Mixtral-8x7B-Instruct-v0.1/blob/main/tokenizer_config.json#L42
Mistral seems to be very sensitive to any spaces after [/INST] in its instruction template.
Note that the default instruction template provided by Mistral AI (found in the model's tokenizer_config.json) does not contain any spaces before [INST] and after [/INST]:
Unfortunately, that default template doesn't support system messages, but the one in text-generation-webui does.
However, the instruction template in text-generation-webui has some extra spaces in it.
While running some tests with langchain, I found Mistral-7b-instruct responding incorrectly with this instruction template.
Mixtral-8x7B-instruct was similarly affected.
Here's is a prompt that can reproduce the issue (tested with ExLlamav2_HF):
Correct instruction template (with no extra spaces before [INST] and after [/INST]):
Prompt:
Response (correct):
Incorrect instruction template (with an extra space before [INST] and after [/INST]):
Prompt:
Response (incorrect):
Note that the incorrect response above does not follow the format in the prompt's instructions.
Removing the extra spaces from the instruction template fixes it.
Checklist: