-
Notifications
You must be signed in to change notification settings - Fork 84
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
Chat templates by @maiqingqiang #104
Conversation
Nice 👍 |
Hi @pcuenca! Can you help review https://github.com/maiqingqiang/Jinja? This repo already meets most of the Jinja Chat Templates. Also, you're invited to be a Jinja Collaborator.🤝🏻 |
|
Thank you @maiqingqiang! I'm travelling and a bit slow to respond, I'll be happy to take a look next week! |
Thank you @pcuenca !🍻 |
Thoughts on merging this @FL33TW00D, @Vaibhavs10 ? When I'm back at my desk I'm planning to take a closer look at the template engine that @maiqingqiang created, but it works fine already. Some potential changes we would want to do before merging are:
Any other thoughts or details? |
Looking forward to this feature, thanks for the great work 🚀 |
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.
Sorry I missed this earlier:
Add some additional tests, currently we are just testing Mistral without a system prompt.
Remove the padding logic, as we are not using it anywhere else in the project. It might make more sense to address it as another PR globally for the project. But we can still keep it there if that's easier.
Nicely done (from my very limited swift knowledge! I think a few tests and we should merge it and follow up all other updates to a new PR.
Currently, I am developing a MacOS App - ChatMLX based on this PR. Looking forward to the merge of this PR. |
Hi @maiqingqiang! I'm going to try to merge to unblock you, sorry for the delay! Do you need the padding logic? There are some conflicts that would be easier to resolve if we can remove it and add in a different PR. |
Hi @pcuenca . Thank! I don't need the padding logic. You can remove it. |
We need to get back to this to support consistently.
Tests pass, merging now! Thanks a lot for this amazing contribution, @maiqingqiang, and also for your patience! 🙌 We can add tests and get back to supporting padding in a future PR :) |
Thank you very much 🤝🏻🤝🏻 @pcuenca |
Hi @pcuenca, could you please tag a new release for swift-transformers? |
Continuation of #77 by @maiqingqiang, with support for
addSpecialTokens
to mimic the reference tokenizers implementation.