[Bug] Update Mistral chat template #918
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #918 +/- ##
=======================================
Coverage ? 51.43%
=======================================
Files ? 64
Lines ? 4705
Branches ? 0
=======================================
Hits ? 2420
Misses ? 2285
Partials ? 0 ☔ View full report in Codecov by Sentry. |
| CHAT_TEMPLATE_CACHE[phi3_template] = Phi3ChatTemplate | ||
|
|
||
|
|
||
| # -------------------------------------------------- | ||
| # @@@@ Mistral-7B-Instruct-v0.2 @@@@ | ||
| # -------------------------------------------------- | ||
| # [05/08/24] https://huggingface.co/mistralai/Mistral-7B-Instruct-v0.2/blob/main/tokenizer_config.json#L42 | ||
| mistral_7b_instruct_template = "{{ 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 %}" | ||
| mistral_7b_instruct_template = "{{ 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 %}" |
There was a problem hiding this comment.
On my phone so I can't comment properly, but the newspace added in the template needs to also be added as part of the get_role_start or get_role_end methods so that we match the new jinja format
There was a problem hiding this comment.
Ah, didn't see that this was already merged so I just fixed it in #919
There was a problem hiding this comment.
Hubris on my part to think that this was a nice, simple fix. I shall give some thought to adding tests to catch such oversights in future
There was a problem hiding this comment.
No worries, it was just a single character change! I was able to add it in straight from my phone :)
There was a problem hiding this comment.
Yes absolutely! I'm not even sure all the existing ones are totally correct. When I made these template -> class mappings, I would open up a notebook and check if the guidance classes matched what the Transformers tokenizer's "apply_chat_template" method did on a standard user/assistant "messages" array of dicts. i think formalizing a methodology around that into tests would be incredibly valuable
we could later use that testing infra to start helping us build an auto chat_template loader that we use as a fallback if something isn't in our cache (ie something that tries to convert the chat template into the guidance class format -- if that passes the tests we write with a variety of messages arrays, then we know we're good)
|
I'd also like to think more about if we should force a direct update to the template cache like this, or if we should e.g. have a separate version for the new mistral template update. I'm not sure how frequently model hosts/deployed models will pull new weights/config files down, so this would break them upon a guidance update, while keeping an extra template cache doesn't really cost us anything |
I'm not sure how that would work, although learning the details of these chat templates is another fixture on my ToDo list. One immediate question: what if the templates only differed in whitespace (e.g. for this particular one, suppose another update added a second space between the quotes). Since we strip out whitespace when matching keys, we wouldn't be able to detect that. |
|
good q -- I'm really not sure that whitespace stripping the way I did it for cache matching was actually the best decision. jinja ignores some whitespaces but not others, so I figured we'd try to go for maximal coverage at the beginning until figuring out something more clever |
|
(because the alternative was for the "space misaligned" template to default to the chatml format which would be way worse than just matching to something reasonably close...) |
It appears that the Mistral chat template has [had an update](https://huggingface.co/mistralai/Mistral-7B-Instruct-v0.2/commit/1296dc8fd9b21e6424c9c305c06db9ae60c03ace), so we need to match this
It appears that the Mistral chat template has had an update, so we need to match this