Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 multiple chat templates per model #134
Support multiple chat templates per model #134
Changes from 7 commits
48f9167
ac91113
852ea26
b916247
cb47ba4
19a5da7
de740f8
62ccb41
c0355dd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 like this solution. But it's a breaking change, do you know if the current method is used by mlx or others?
Perhaps we could add an overload
applyChatTemplate(messages: [[String: String]], chatTemplateName: String)
that just builds aChatTemplateArgument
(literal
) and forwards the call.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 searched the mlx-swift and mlx-swift-examples repos and didn't find any instances of
applyChatTemplate
there. I have a draft PR to use the chat templates once this PR and the Jinja PR land.I think you mean the additional overload should have the argument
chatTemplate: String
. Sure, I can add that.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.
addGenerationPrompt
is false by default in this method (which mirrors the Python implementation), but it is currently set to true in the overload method. Should we make the behavior consistent in allapplyChatTemplate
methods? Should it be true or false by default?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 it should be
true
for the overloads as I expect that to be the most common use of the API, but I'm not fully sure.