-
-
Notifications
You must be signed in to change notification settings - Fork 898
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
feat: Add LLaMA-3 instruct prompt strategies for fine-tuning #1553
feat: Add LLaMA-3 instruct prompt strategies for fine-tuning #1553
Conversation
@maziyarpanahi You can install THIS axolotl PR with THIS FastChat PR |
Hi @TJ-Solergibert |
I think this needs to be updated, at this point it generates some token duplication, this function might need to be updated:
To (in line with FastChat implementation):
|
Thanks @upr1ce yes it needs to be updated. Just waiting for the fastchat merge with final changes. I’ll update this once that happens |
@winglian Made necessary changes, fastchat PR is merged ShareGPT Original
Tokenised LLaMA-3-Instruct Format
LLaMA-3 Instruct format can be used with the config
Let me know if any changes are required |
@0-hero isn't the actual Llama-3 template (as they have set in their HF tokenizer config) like this:
condence:
|
if message: | ||
yield f"<|start_header_id|>{role}<|end_header_id|>\n\n", f"{message.strip()}<|eot_id|>" | ||
else: | ||
yield f"<|start_header_id|>{role}<|end_header_id|>\n\n", "" |
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.
return is missing here
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.
as @ryj0902 pointed out, when there is no system_message
, bos token is not added at the moment. should we do something like the following?
if self.sep_style == SeparatorStyle.LLAMA3:
if self.system_message:
# For llama3, the system message is NOT incorporated into the first human instruction
# All messages follow <|start_header_id|>' + role + '<|end_header_id|>\n\n'+ message + '<|eot_id|>
yield "", system_prompt
for i, (role, message) in enumerate(self.messages):
if message:
role_header = f"<|start_header_id|>{role}<|end_header_id|>\n\n"
if i == 0:
yield "<|begin_of_text|>" + role_header, f"{message.strip()}<|eot_id|>"
else:
yield role_header, f"{message.strip()}<|eot_id|>"
else:
yield f"<|start_header_id|>{role}<|end_header_id|>\n\n", ""
return
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.
unfortunately thats not going to work, it will lead to duplication of <|begin_of_text|>
example:
<|begin_of_text|><|start_header_id|>system<|end_header_id|>
asdf<|eot_id|><|begin_of_text|><|start_header_id|>user<|end_header_id|>
It is a test<|eot_id|><|start_header_id|>assistant<|end_header_id|>
WTF<|eot_id|><|end_of_text|>
After the changes, it will generate correct output, example (formatted)
|
if self.system_message: | ||
# For llama3, the system message is NOT incorporated into the first human instruction | ||
# All messages follow <|start_header_id|>' + role + '<|end_header_id|>\n\n'+ message + '<|eot_id|> | ||
yield "", "<|begin_of_text|>" + system_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.
this should be yield "", system_prompt
otherwise <|begin_of_text|> will be replicated twice, as bos tokens are added automatically by axolotl. Sorry for next message, I did not mark it correctly so it was visible only for me.
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.
I was just working on that, but even with yield "", system_prompt
I'm seeing <|begin_of_text|> twice. Trying t figure out what's going on there
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.
I think thats a cache issue, clear your huggingface dataset cache and it should be fine, something like rm ~/.cache/huggingface/datasets/
depending on your OS
Thanks @upr1ce works as expected now
Sorry, Missed the last 2 changes when I changed to the 2nd fast chat PR |
@upr1ce reran the workflows here - https://github.com/0-hero/axolotl/actions |
Shouldn't the bos_token be added regardless of whether the system prompt is present or not? According to the current implementation, if there is no system message(or empty string), bos token is not added. Was this intentional? case 1, system message exist:
case 2, system message is empty:
case 3, system message not exist:
|
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.
should we pass system_message
as one of the key word arguments?
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.
here is how llama-3-8b-instruct tokenizes:
|
I am having trouble adjusting it to work in all 3 cases mentioned by @ryj0902, case 1 and 3 are easy to fix, case 2 seems weird, if system message is empty, I am not able to force to include |
It would be great if code could be written to respond to all cases that a user might enter, but I am also aware that case 2 deals with a very rare, edge case. |
2470724
to
73a03cd
Compare
@0-hero I rebased this against main, and added a sharegpt tokenization test. I believe in order to tokenize it properly, you have to include this in your configuration, otherwise it includes an special_tokens:
eos_token: "<|eot_id|>" |
This is true! Unfortunately, Meta instead of fixing their tokenizer_config and changing the eos_token to |
They've just fixed official tokenizer config in Meta-Llama-3-8B-Instruct repo, today: |
73a03cd
to
9ae5649
Compare
@winglian thanks for taking over! I had to go on a short medical break |
* Add prompt strategies * Update modified URL * Update modified URL * Update fastchat_conversation_turns.py * Update register function * Remove extra /n for system prompt * Fix return * Fix BOS * Update requirements, pylint * Linting * Linting * fix tuples, make sure to set system message in template * tests for llama3 tokenization * fix conditionals for loading chat template --------- Co-authored-by: Ram <[email protected]> Co-authored-by: Wing Lian <[email protected]>
Description
This builds on top of and includes the changes in the below PR's
Fastchat PR from @TJ-Solergibert needs to be merged before merging this
Motivation and Context
Fine-tuning models in the llama-3 instruct conversation format, or continue fine-tuning llama-3 instruct models
How has this been tested?
Update
Added changes from TJs monkeypatch to support the latest Fastchat PR lm-sys/FastChat#3259