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
Llava: add default chat templates #31691
Llava: add default chat templates #31691
Changes from 11 commits
f577ecb
1831329
d684157
a22c149
c4c1880
ac07a33
41afdbd
07a91be
400a8b2
2be86fc
3ff974f
3ad481b
0f0b8a2
2a3df50
dd7aad9
7c215bd
78a5876
f9e47b8
b4deec5
6abf2d6
8ef3e2e
eba4512
62c4ac6
ac01cdd
109e198
54451dc
11fc70c
5cb21ec
a127f1d
4850a3b
ca2696f
97b227c
0809d57
6e2153f
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.
We can't do for two reasons:
chat_template.json
after this update. Even if we updated all public checkpoints on the hub to have the templates in the config, this would break things for anyone who is using local checkpoint as even custom templates.FWIW - I think it's a lot cleaner having the chat template in a separate file anyway. They can be verbose and can easily clutter up the config files. Similar to keeping the vocab files separate for the tokenizers
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.
Hmm I thought we could deprecate the
chat_template.json
when the old versions are very old to be in someone's env. I am okay with leavingchat_template.json
but wouldn't that be different from what we have in LLMs?Another option I proposed was to add the template in processor's tokenizer. If we don't want to have the template in
processor.config
, this will be the better option IMO. WDYT?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.
Oh, also, continuing on this. We can also discuss this internally on slack later. We recently added an option for some processors to accept any kwargs and I was hoping to start integrating new kwargs for VLMs. Does this comment mean that we can't do it and will need another hack to load those kwargs?
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.
The problem is - when would this happen? There's some people who are still installing transformers v3, so we can't assume everyone will be working from the latest versions, even if it's a few months out. We can deprecate things within the code which we have full control over. Unfortunately, as the configs sit outside the library it means people can use them from the first version they're introduced until now.
Just to be make sure we're talking about the same thing, this would mean adding into the tokenizer's config for the checkpoint e.g. the tokenizer config here for a llava checkpoint?
It's a solution, but I don't think it semantically makes sense: the tokenizer is for processing text, whereas this introduces information about images.
We definitely want this feature added for our processors, and gives us flexibility with future compatibility. It won't however fix things for older versions. My understanding is that if the chat_template gets added into the config, then older versions would still break when trying to load the config 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.
Yes, I meant that checkpoint. Now I see why it's not in tokenizer's config, then this workaround of having a separate
json
for templates is indeed better.Yeah, this will be the problem in any case as we won't have compatibility with older transformers versions given how breaking are the new changes. Dealing with hub-transformers compatibility is harder than it looked like 😅 I guess for that feature of VLM processor refactor we'll have to wait a while and try to change slowly, to see users reactions...
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 there a reason for doing this in two steps i.e. getting
text
thenjson.loads
instead ofjson.load
in the reader context directly?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.
ah, will simplify it. I was copying from processor
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.
Can't be loaded as
json
because it was saved asTextIOWrapper
. Actually I don't know why we save it this way, was copying from processors. Maybe it has something to do with safe-saving 🤔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.
huh, interesting. Well, good to know :) Thanks for investigating