-
Notifications
You must be signed in to change notification settings - Fork 586
Set the secondary EOS for Gemma2 #527
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
Set the secondary EOS for Gemma2 #527
Conversation
jan-wassenberg
left a comment
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, @pculliton, any thoughts?
pculliton
left a comment
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.
Great changes - thank you for making them!
jan-wassenberg
left a comment
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.
We have a merge conflict, can you please rebase?
The secondary eos is usually `<end_of_turn>`, which can appear in the prompt, so we can only check it not in the prompt.
So that we can remove the `<end_of_turn>` filter that was set up specifically for Gemma2.
cbb84e1 to
d42deaa
Compare
|
I have rebased, but no conflict complained by git. |
jan-wassenberg
left a comment
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! It could be that our copybara import was stuck, hopefully this will unblock it.
This PR also removes the
<end_of_turn>filter that was set up specifically for Gemma2 and fixes the EOS checking.