Skip to content
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

Make chat templates part of ProcessorMixin #30744

Merged
merged 12 commits into from
Jun 13, 2024
Merged

Conversation

Rocketknight1
Copy link
Member

Right now, IDEFICS2 supports chat templates, but in class-specific code. In this PR, we'll try to move that support to the generic ProcessorMixin so that we don't have to keep duplicating it as more VLMs (like LLaVA) start to need chat templates.

Draft PR for now until I make sure this doesn't break things!

@molbap
Copy link
Contributor

molbap commented May 10, 2024

Hi @Rocketknight1, very nice! FYI, I'm working on standardizing the processors here #30511 and I was thinking how to make this compatible with apply_chat_template - thought you might want to take a look at the design here, and I'll keep an eye here as well :)

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@Rocketknight1
Copy link
Member Author

@molbap I didn't see your PR, but we should definitely use your approach! Supporting chat templates should be simple enough - all they require is moving apply_chat_template to ProcessorMixin, and then supporting chat_template as an attribute for any processor that wants to use them. Do you want me to pause my PR until yours is merged?

@molbap
Copy link
Contributor

molbap commented May 10, 2024

I don't think you need to pause yours! I'll try and get mine merged soon - it's already approved, just need to make sure nothing breaks

@Rocketknight1
Copy link
Member Author

Cool! I'll probably wait until yours is merged, in that case, and then adding chat templates should be clean and straightforward.

@Rocketknight1 Rocketknight1 force-pushed the processor_chat_templates branch from 302cac4 to e844dd3 Compare June 7, 2024 15:33
@Rocketknight1 Rocketknight1 force-pushed the processor_chat_templates branch from 9ef139d to c440ca5 Compare June 10, 2024 13:04
@Rocketknight1 Rocketknight1 marked this pull request as ready for review June 10, 2024 13:04
@Rocketknight1
Copy link
Member Author

Rocketknight1 commented Jun 10, 2024

This should be ready - cc @amyeroberts for core maintainer review! I'm speeding things up because the lack of this feature is causing some user issues, and we have a lot of VLMs coming.

Copy link
Collaborator

@amyeroberts amyeroberts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding!

@Rocketknight1 Rocketknight1 merged commit 15b3923 into main Jun 13, 2024
21 checks passed
@Rocketknight1 Rocketknight1 deleted the processor_chat_templates branch June 13, 2024 13:35
zucchini-nlp pushed a commit to zucchini-nlp/transformers that referenced this pull request Jun 14, 2024
* Let's try moving chat templates out of IDEFICS and into the generic ProcessorMixin

* Chat templates should not be mandatory

* Chat templates should not be mandatory

* Not all classes will have default chat templates

* stash commit

* Add chat template docstring

* Clean up docstring

* Add chat templates to LLaVA/LLaVA-next

* Docstring fixup

* Quick IDEFICS2 fixup

* Remove some old references to the Conversation class

* make fixup
itazap pushed a commit that referenced this pull request Jun 17, 2024
* Let's try moving chat templates out of IDEFICS and into the generic ProcessorMixin

* Chat templates should not be mandatory

* Chat templates should not be mandatory

* Not all classes will have default chat templates

* stash commit

* Add chat template docstring

* Clean up docstring

* Add chat templates to LLaVA/LLaVA-next

* Docstring fixup

* Quick IDEFICS2 fixup

* Remove some old references to the Conversation class

* make fixup
itazap pushed a commit that referenced this pull request Jun 17, 2024
* Let's try moving chat templates out of IDEFICS and into the generic ProcessorMixin

* Chat templates should not be mandatory

* Chat templates should not be mandatory

* Not all classes will have default chat templates

* stash commit

* Add chat template docstring

* Clean up docstring

* Add chat templates to LLaVA/LLaVA-next

* Docstring fixup

* Quick IDEFICS2 fixup

* Remove some old references to the Conversation class

* make fixup
itazap pushed a commit that referenced this pull request Jun 17, 2024
* Let's try moving chat templates out of IDEFICS and into the generic ProcessorMixin

* Chat templates should not be mandatory

* Chat templates should not be mandatory

* Not all classes will have default chat templates

* stash commit

* Add chat template docstring

* Clean up docstring

* Add chat templates to LLaVA/LLaVA-next

* Docstring fixup

* Quick IDEFICS2 fixup

* Remove some old references to the Conversation class

* make fixup
itazap pushed a commit that referenced this pull request Jun 18, 2024
* Let's try moving chat templates out of IDEFICS and into the generic ProcessorMixin

* Chat templates should not be mandatory

* Chat templates should not be mandatory

* Not all classes will have default chat templates

* stash commit

* Add chat template docstring

* Clean up docstring

* Add chat templates to LLaVA/LLaVA-next

* Docstring fixup

* Quick IDEFICS2 fixup

* Remove some old references to the Conversation class

* make fixup
itazap pushed a commit that referenced this pull request Jun 20, 2024
* Let's try moving chat templates out of IDEFICS and into the generic ProcessorMixin

* Chat templates should not be mandatory

* Chat templates should not be mandatory

* Not all classes will have default chat templates

* stash commit

* Add chat template docstring

* Clean up docstring

* Add chat templates to LLaVA/LLaVA-next

* Docstring fixup

* Quick IDEFICS2 fixup

* Remove some old references to the Conversation class

* make fixup
@qgallouedec qgallouedec mentioned this pull request Jun 26, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants