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

update warning for image processor loading #28209

Merged
merged 4 commits into from
Jan 9, 2024
Merged

update warning for image processor loading #28209

merged 4 commits into from
Jan 9, 2024

Conversation

ydshieh
Copy link
Collaborator

@ydshieh ydshieh commented Dec 22, 2023

What does this PR do?

Similar to #28108, there is not much we/user can do with the files on the Hub.

For this image processor loading, IMO, the attribute image_processor_class / feature_extractor_class are super unlikely specified by users (they are created automatically during saving for auto class to work). So logger.info is fine.

@ydshieh ydshieh requested a review from amyeroberts December 22, 2023 16:34
@ydshieh
Copy link
Collaborator Author

ydshieh commented Dec 22, 2023

cc @NielsRogge

Copy link
Contributor

@NielsRogge NielsRogge left a comment

Choose a reason for hiding this comment

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

@amyeroberts
Copy link
Collaborator

Thanks for opening this @ydshieh!

As mentioned in the internal slack channel - I'd rather we had a transition with this: at least two releases instructing the users how to update before downgrading the warnings. Unlike the text_config for CLIP, loading feature extractors for vision is deprecated and it's necessary for users to change their configs to ensure future compatibility.

@ydshieh
Copy link
Collaborator Author

ydshieh commented Jan 8, 2024

Thanks @amyeroberts Sound fair.

Do you want me to change the warning to include the instructions and the version number involved (i.e. 2 release after since the current one)?

@NielsRogge
Copy link
Contributor

NielsRogge commented Jan 8, 2024

@amyeroberts I'm not sure it's doable to expect users to update their config? That would upset a lot of users.

We have 8000+ preprocessor_config.json files on the hub => we can't update all of these right?

So my suggestion would be to gracefully handle this internally in the from_pretrained method without informing users.

@amyeroberts
Copy link
Collaborator

@ydshieh Yes please!

We have 8000+ preprocessor_config.json files on the hub => we can't update all of these right?

@NielsRogge Certainly not! :) This is why we will downgrade the warning after a cycle. However, we should give users the opportunity to correctly update their configs as necessary by giving them the necessary information as they will not be supported in the future.

@ydshieh ydshieh force-pushed the set_level_to_info branch from 77e9e81 to 2c5fff7 Compare January 8, 2024 14:23
Comment on lines 366 to 371
"Could not find image processor class in the image processor config or the model config. "
f"If you are the owner of {pretrained_model_name_or_path}, please update the file "
"`preprocessor_config.json` to use `image_processor_type` instead of `feature_extractor_type`. "
"Otherwise, you can open a pull request on this Hub repository. "
"This warning will be removed in v4.40. Currently, we try to load the image processor based on "
"pattern matching with the model's feature extractor configuration."
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@amyeroberts let me know if this is too wordy 🙏

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I put v4.40 to give it a bit more time

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 @ydshieh!

Just a suggestion on the warning. Could you update the PR title before merging?

src/transformers/models/auto/image_processing_auto.py Outdated Show resolved Hide resolved
ydshieh and others added 2 commits January 8, 2024 18:09
Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>
@ydshieh ydshieh changed the title Use logger.info for image processor loading update warning for image processor loading Jan 8, 2024
@ydshieh
Copy link
Collaborator Author

ydshieh commented Jan 8, 2024

I have to change a bit of the warning message - if you would like to take another look.

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 iterating!

@ydshieh ydshieh merged commit 5c7e11e into main Jan 9, 2024
21 checks passed
@ydshieh ydshieh deleted the set_level_to_info branch January 9, 2024 07:51
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.

3 participants