-
Notifications
You must be signed in to change notification settings - Fork 7
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
Support vision models and function calling #8
Support vision models and function calling #8
Conversation
Thank you for your PR, it's great! Could you please provide some test cases? I may not have time until next month. I've been a bit busy lately. Hi @pcuenca ! Do you have time to help review this PR? |
I'll add some tests for images and function calling and try to polish this up a bit. I should have also formatted the code before editing it, to make the changes more legible. After this gets merged, maybe we can add some auto-formatting. |
Hi @DePasqualeOrg, thanks a lot for the effort! It's a long diff, I can try to take a look in a couple of days. Do we need everything at once, including namespaces, built-in functions and tool calling, or could this potentially be approached in a few phases? |
9a43ea0
to
1c85539
Compare
8365aec
to
0fdf32f
Compare
I've rebased this after formatting the repo with swift-format, to make it easier for @pcuenca to review it. |
0fdf32f
to
16375f3
Compare
5b4472f
to
a60b6b1
Compare
The existing tests as well as some additional ones I added pass. |
a982715
to
0e65018
Compare
Four of the six tool use tests from the TypeScript implementation are now passing. It's quite difficult to get these tests to pass, because dictionaries in Swift don't preserve the order of the keys. I've used OrderedDictionary in the tests, which presents its own challenges, since sometimes these values need to be converted to JSON. I've disabled the two problematic tests and will focus on testing with more recent models. It already appears to work well with Llama 3.2. I'll be testing this on vision models here: ml-explore/mlx-swift-examples#173 Tool use examples in LLMEval: ml-explore/mlx-swift-examples#174 |
7c0b020
to
9eb074b
Compare
Honestly, no, I don't think that's a good idea. I spent several whole days trying to get as close as possible to the Python Jinja2. I think that should be our reference point, not the TypeScript version. That is what is used for chat templates in Python. If you have tests from your implementation of the TypeScript version, you can share them and we can see if they pass on my branch. You can take my tests and see if they pass on your branch. It was an enormous amount of work to get this working, and I would be very disappointed if it gets thrown away. Also, to prevent duplication of work in the future, I would suggest opening a draft pull request here when you're working on something, so that your work isn't duplicated by someone else, which would be a waste of their time. |
I'm very sorry, but it won't be thrown away. I think the ported Jinja2 still needs further inspection and testing, so I want to split some of Jinja2's code into smaller modules for iterative updates. Secondly, the original intention of Swift Jinja is to replicate TypeScript Jinja; I hope to keep it synchronized with the TypeScript Jinja version and then port Jinja2 on this basis. Replicating first is the fastest method, and there are significant design differences between Swift Jinja and Jinja2. If we truly want to replicate Jinja2, a refactor of Swift Jinja might be necessary. |
I really don't know what you have in mind. What exactly do you want to separate out from my contribution? Is there any functionality in your branch that isn't reproduced in mine? Please also point out specific differences between the TypeScript version and the Python Jinja2 that you think are important. My pull request has been open for more than a month, and only now am I learning about your parallel effort, which you didn't share here. Since no progress has been made for several months, I decided to take the initiative and make chat templates for vision models and function calling work. Now they're working. If you're worried about correctness, I can remove the filters for which I disabled tests because fully implementing them would be too complex. However, considering the huge effort required to make this all work, I don't think we're going to be able to merge your version and mine. Since I was the first to share this here, I think we should use my branch as the basis for further work. I'm happy to remove anything that you think is not ready for production (please provide tests that demonstrate it's not correct), and to add any functionality from your branch that is missing in mine. The reason I feel confident about my work is that I've already covered a large part of the tests from the Python implementation, and they're passing. |
I've reviewed my PR, and there are no major architectural changes here. Therefore, I don't understand what you mean about refactoring. Furthermore, I'd like to emphasize that I first ported functionality from the TypeScript implementation and then (after almost entirely covering the TypeScript implementation) added missing functionality (mainly the filters and tests) from Jinja2 in Python. This PR is still comparable to the TypeScript implementation. I'm hoping we can move forward with this efficiently, because I want to start building actual features in apps using function calling and vision models instead of getting bogged down with this library. |
Ok. Before merging this PR, you still need to resolve the previous review feedback. @DePasqualeOrg Additionally, I hope @pcuenca can also participate in the review since swift-transformers is mainly being used at present. |
c6c893b
to
06804d1
Compare
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.
Thank you very much @DePasqualeOrg!
I have reviewed it and think it can be merged at any time.
But we still need @pcuenca to help review it again.
@pcuenca, could you let us know if you want to review this? If you don't have time, I think we should merge it so that I can move ahead with huggingface/swift-transformers#151, ml-explore/mlx-swift-examples#174, and ml-explore/mlx-swift-examples#173. |
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 haven't had the chance to test it in depth, but I'm supportive of merging to unblock downstream uses, as long as the repo owner is happy with the changes. In my opinion, please follow the lead of @johnmai-dev here.
There appears to be a failing test case, but it seems related to dictionary ordering. I would suggest to write test cases in such a way that ordering does not impact results, but we can do that in a different PR.
Package.resolved
Outdated
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.
Is it necessary to commit this file?
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'm not sure. It was created after I added swift-collections. Are lock files usually included for dependencies in Swift packages?
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 don't think so, Xcode should resolve the dependencies when you compile. I'd suggest to remove.
It's also a bit unfortunate that another dependency had to be added, is it just to guarantee reproducibility for tests? If so, we should be able to move it to the test target (but we can do it later).
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.
swift-collections is necessary for the reproducibility of some tests, and is also needed in the library itself. I'm not sure how else you would test stringification of dictionaries if you don't have a deterministic key order.
I see that mlx-swift has a Package.resolved file, and Claude says it's usually included in Swift packages, for what it's worth.
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.
Apple recommend committing the Package.resolved.
83e6773
to
30edf9f
Compare
Are there any other commits that need to be pushed? If not, I think we can merge. @DePasqualeOrg |
🚀 this should unblock the Deep Seek reasoning chat templates so looking forward to getting it landed! |
I have nothing more to add, so you can merge this, @johnmai-dev. |
Thank you for your efforts! @DePasqualeOrg |
@DePasqualeOrg , thanks for this awesome contribution! I am using FullMoon , a super clean swiftUI client, build with MLX and Jinja. Can you give some instructions on how can we parse a function call with the new Jinja 1.1? |
@alelordelo, when the model responds with the function call, it's up to the app to handle it. Jinja is not involved at all at that point. |
I've added functionality from the huggingface.js implementation. This is a work in progress.