-
Notifications
You must be signed in to change notification settings - Fork 27.4k
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
Fix regression on Processor.save_pretrained
caused by #31691
#32921
Fix regression on Processor.save_pretrained
caused by #31691
#32921
Conversation
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 right, thanks for fixing!
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. |
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.
LGTM, tho we need a small test to make sure we don't break that again 😓
I guess that had a tests, but it wasn't triggered when the prev PR was merged. I ran only VLMs when merging the prev PR... |
Okay, merging and patching! |
…1691 (huggingface#32921) fix save_pretrained
…1691 (huggingface#32921) fix save_pretrained
…1691 (huggingface#32921) fix save_pretrained
…1691 (huggingface#32921) fix save_pretrained
…1691 (huggingface#32921) fix save_pretrained
What does this PR do?
Fix regression on
Processor.save_pretrained
caused by #31691tl;dr: a month ago, we made a change that removed
"chat_template"
fromprocessor_dict
when saving a processor. This causedprocessor_config.json
to not get saved at all. See:but we kept these lines as is:
So, for a month now, we've not been saving
processor_config.json
but still returning[processor_config.json]
as the saved files. This has caused a test in my other PR (#32906) to failWho can review?
@amyeroberts @zucchini-nlp