Skip to content

Make image processors more general#27690

Merged
ydshieh merged 4 commits into
huggingface:mainfrom
NielsRogge:make_image_processors_more_general
Dec 5, 2023
Merged

Make image processors more general#27690
ydshieh merged 4 commits into
huggingface:mainfrom
NielsRogge:make_image_processors_more_general

Conversation

@NielsRogge

@NielsRogge NielsRogge commented Nov 24, 2023 •
edited
Loading

Copy link
Copy Markdown
Collaborator

What does this PR do?

This PR undoes #26965 and instead makes image processors more general, by removing the hardcoded "shortest_side" arguments, allowing to pass the following to image processors:

size = {"height": ..., "width": ...}
size = {"shortest_edge": ...}

Ideally it should also allow for the following (cc @amyeroberts)

size = {"longest_edge": ...}
size = {"shortest_edge": ..., "longest_edge": ...}

Image processors should not be limited to only {"shortest_edge": ...} for instance, as CLIPImageProcessor is at the moment. The size argument is now always a dictionary containing one of these 4 possibilities; hence it would be great to support them all.

@HuggingFaceDocBuilderDev

HuggingFaceDocBuilderDev commented Nov 24, 2023

Copy link
Copy Markdown

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

@amyeroberts

amyeroberts commented Nov 24, 2023

Copy link
Copy Markdown
Contributor

Thanks for opening this PR!

Ideally it should also allow for the following (cc @amyeroberts)

size = {"longest_edge": ...}
size = {"shortest_edge": ..., "longest_edge": ...}

As discussed on slack, enabling all image processors to accept {"height": h, "width": w} is a change I'm very much pro. Enabling all to accept {"shortest_edge": x, "longest_edge": y} isn't something I think we should as this behaviour isn't well-defined and varies between models.

@NielsRogge

NielsRogge commented Nov 26, 2023 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

cc @ydshieh this is failing for KOSMOS-2 - it would be great to revert that change for KOSMOS-2 (i.e. remove the "use_square_size" attribute) since this is introduced for legacy behaviour of size (which used to be an integer and is now a dictionary).

@ArthurZucker

ArthurZucker commented Nov 26, 2023

Copy link
Copy Markdown
Collaborator

It did but it's also very convenient not to have to use copied from when the only difference was this, so reverting IMO is not the best solution. Let's rather adapt / make it compatible

@NielsRogge NielsRogge mentioned this pull request Nov 27, 2023
Closed
3 tasks
@ydshieh

ydshieh commented Nov 27, 2023

Copy link
Copy Markdown
Collaborator

Reverting #26965 means we well have to add Kosmos2ImageProcessor (while currently it is CLIPImageProcessor with use_square_size=True). Probably it will go without problem, but still kind of breaking changes.

@NielsRogge

NielsRogge commented Nov 27, 2023

Copy link
Copy Markdown
Collaborator Author

@ydshieh would it also be possible to just set size to {"height": ..., "width" ...}? With this PR, image processors are made more general, making sure square sizes are supported

@ydshieh

ydshieh commented Nov 27, 2023 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Hi, could you give more details on

set size to {"height": ..., "width" ...}

(where should I set this, for example.)

Do you mean on the config file?

@NielsRogge NielsRogge mentioned this pull request Nov 27, 2023
Closed
@NielsRogge NielsRogge force-pushed the make_image_processors_more_general branch from f9a2b1f to 135bc9d Compare November 27, 2023 14:17
@NielsRogge

NielsRogge commented Nov 27, 2023

Copy link
Copy Markdown
Collaborator Author

Yes I meant in the preprocessor_config.json. Given that the model is only one month old, we could still update them. Alternatively, I've added a backwards compatibility in this PR, only for CLIPImageProcessor. Let me know what you think works best.

@ydshieh

ydshieh commented Nov 27, 2023

Copy link
Copy Markdown
Collaborator

We can keep your backwards compatibility code, and I could try to open Hub PRs to update them.
Let's also hear what the core maintainers say of course.

ArthurZucker
ArthurZucker reviewed Nov 28, 2023
View reviewed changes

@ArthurZucker ArthurZucker left a comment •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks like a better and more general solution so in favor, I'll let @amyeroberts review when she comes back unless it's urgent! 😉

@NielsRogge

NielsRogge commented Nov 28, 2023

Copy link
Copy Markdown
Collaborator Author

It's required to merge #27718

amyeroberts
amyeroberts reviewed Nov 28, 2023
View reviewed changes

@amyeroberts amyeroberts left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for adding!

The test file will need to be removed before merging - otherwise looks good!

Comment thread src/transformers/models/test.py Outdated
@NielsRogge NielsRogge force-pushed the make_image_processors_more_general branch from 1dfef5c to bd71da3 Compare December 4, 2023 17:36
@ydshieh

ydshieh commented Dec 5, 2023

Copy link
Copy Markdown
Collaborator

Merge now as full (slow) CI on 4 models touched by this PR pass

@ydshieh ydshieh merged commit df40edf into huggingface:main Dec 5, 2023
@ArthurZucker

ArthurZucker commented Dec 5, 2023

Copy link
Copy Markdown
Collaborator

cc @younesbelkada we have a regression on this

@Charizhardt Charizhardt mentioned this pull request Apr 27, 2024
Closed
4 tasks
@ydshieh ydshieh mentioned this pull request Apr 30, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ArthurZucker ArthurZucker ArthurZucker left review comments

+1 more reviewer

@amyeroberts amyeroberts amyeroberts approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@NielsRogge @HuggingFaceDocBuilderDev @amyeroberts @ArthurZucker @ydshieh