-
Notifications
You must be signed in to change notification settings - Fork 604
AutoGuess follow-up: adapter checks #1654
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
AutoGuess follow-up: adapter checks #1654
Conversation
|
|
||
|
|
||
| { | ||
| }, { |
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.
FWIW, I intentionally put this in here to ensure people didn't just stack more adapters at the end. That said, with these tests, it should error on the right occasions so no big deal.
33ffb10 to
7b72816
Compare
|
You might want to rebase this off latest. I am ok with the rwkv keyword change. I am not even sure what the correct one is - half the so called rwkv models are just using chatml so I suspect the template isnt even valid and should be removed. |
7b72816 to
8193e11
Compare
Done. Also updated kallewoof#1 to run rebased variant.
I think that will work fine, especially if we include e.g. "User:" in the search strings. ChatML content will be matched with the ChatML adapter(s). |
LostRuins
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.
will merge this first while we reexamine the templates
|
One thing I have learned previously: Not all official templates are ideal either. One major reason we deviated from using Jinja is because often times our own tried-and-tested templates outperform the official ones. Just something to keep in mind. |
Absolutely. I think knowing when we deviate is a good idea though. ;) |
Some of these failures are, I believe, actual issues with adapters. I don't know how big of an impact they have, and/or if it's the chat templates that are actually broken, but even then, I think aligning and then fixing is the right course of action.
This builds on top of, and includes, #1650. It should probably not be merged before that one.