Fix prompt assembly for llama #952
Conversation
|
@winglian ok this is ready for your review |
Co-authored-by: Motoki Wu <tokestermw@gmail.com>
hamelsmu
changed the title
hamelsmu
changed the title
|
@winglian I fixed quite a few issues that I found. I added a whole bunch of tests. |
| if (i % 2 == 0 and not self.system_message) or ( | ||
| i % 2 != 0 and self.system_message | ||
| ): | ||
| role = "<s> " + role |
There was a problem hiding this comment.
The only thing I don't love is hardcoding the BOS token here but I couldn't see a better way to fix this ATM
|
@hamelsmu you're a legend and a hero for doing this |
| } | ||
| # fmt: off | ||
| mt_ids = tokenize(multi_turn_conv) | ||
| assert decode(mt_ids) == '<s> [INST] <<SYS>>\nlorem\n<</SYS>>\n\nabc [/INST] ipsum</s><s> [INST] 123 [/INST] sit</s>' |
There was a problem hiding this comment.
at least for mistral instruct, it doesn't seem to add a BOS_ID for every turn
[BOS_ID] +
tokenize("[INST]") + tokenize(USER_MESSAGE_1) + tokenize("[/INST]") +
tokenize(BOT_MESSAGE_1) + [EOS_ID] +
…
tokenize("[INST]") + tokenize(USER_MESSAGE_N) + tokenize("[/INST]") +
tokenize(BOT_MESSAGE_N) + [EOS_ID]
https://huggingface.co/mistralai/Mixtral-8x7B-Instruct-v0.1
and
https://huggingface.co/mistralai/Mistral-7B-Instruct-v0.2
i can try adding some tests separately
There was a problem hiding this comment.
Mistral instruct also doesn't have BOS tokens in between turns. It's probably best to separate llama and mistral prompt style?
There was a problem hiding this comment.
@tokestermw sorry what I mean to say is mistral instruct official template actually does NOT have BOS in between turns! If you read the link you sent carefully (I missed this too at first)
There was a problem hiding this comment.
does NOT have BOS in between turns
yes, that's what i meant as well :) agree on separating llama and mistral, but maybe can do on fastchat repo (or both)
|
Oh my, this is great @hamelsmu. We all mostly use ChatML for conversation by now (openorca, openhermes, dolphin), so it would be amazing to have more testing for other formats as well. The prompt formatting is something that I have seen issues with before, so it's great to see that it's being handled now! |
UPDATE: I found many issues with the llama templating after fixing the omission of the human message. I found the following issues:
[INST]where it wasn't supposed to beI added lots of tests for the following cases:
For the tests, I put a redundant decoded version of the string so that you can see the prompt which will help with code review and understanding