Fix issue with from pretrained and kwargs in image processors

[yonigozlan]

What does this PR do?

Fixes #41955.
Fixes an issue raised in #41954. Instead of setting attributes from kwargs after instantiating the image processor in from_pretrained, we update the image processor dict with the kwargs before instantiating the object. This allows custom logic in the init to take into account the custom kwargs passed to from_pretrained.

In the PR linked, the issue was that max_pixels is supposed to overwrite size["longest_edge"] when passed to the init, but in from_pretrained, max_pixels was never passed to the init and only set as an attribute after instantiating the image processor.

[ReinforcedKnowledge]

I have a quick question about the:

        image_processor_dict.update(kwargs)
        image_processor = cls(**image_processor_dict)

Fix. Because you are adding user given kwargs to image_processor which is built by using the processor kwarg. And maybe the initializing of the class doesn't support those kwargs? (Not trying to criticize just trying to learn more about transformers).

I think the only way to check wether the kwargs are or are not in the init function is by inspecting its signature, but I don't think it's clean and it doesn't solve the underlying core pattern. Because processors can do whatever they want in their init, even use env vars for all they care while from_pretrained can't solve those issues for them.

[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.

[yonigozlan]

I have a quick question about the:

    image_processor_dict.update(kwargs)
    image_processor = cls(**image_processor_dict)

Fix. Because you are adding user given kwargs to image_processor which is built by using the processor kwarg. And maybe the initializing of the class doesn't support those kwargs? (Not trying to criticize just trying to learn more about transformers)

No problem feel free to ask questions! The idea is that all the init logic/setting of attributes and checking of kwargs should be in the class init, to avoid having different results when loading models in different ways (from_pretrained, from_dict, direct instantiation etc.).
Fast image processors are becoming the default in the library, and they all include checks of kwargs in their init, to only accept kwargs that they should accept. So this fix should work without issues, as all fast image processors accept **kwargs in init, then filter them to set as attributes only the kwargs that are in "Model"ImageProcessorKwargs.

[ReinforcedKnowledge]

I agree, if you can enforce some kind of contract where the __init__ works with any **kwargs that the user passes then it's good. But I don't think it's easily doable. And also, the user might pass kwargs that are not used in the __init__ but used downstream in some other utility function of the processor, that checks wether an attribute exists, and then does some specific logic based on the existence of that attribute otherwise it does something else. But the __init__ might not care specifically about it. I don't know if my point is clear 🤔

And thank you for your openness!

[yonigozlan]

Added logic to more clearly use cls.valid_kwargs to update the image processor dict ;).

And also, the user might pass kwargs that are not used in the init but used downstream in some other utility function of the processor, that checks wether an attribute exists, and then does some specific logic based on the existence of that attribute otherwise it does something else. But the init might not care specifically about it. I don't know if my point is clear 🤔

In that case, if the kwarg is in the valid_kwargs attribute of the processor class, it will still be set as an instance attribute (see the logic here in the base fast image processor class)

[ReinforcedKnowledge]

Oh ok perfect thank you @yonigozlan didn't know about valid_kwargs, that's exactly what I wanted to do initially but didn't know about them. Nice chatting with you!

[molbap]

Looks fine to me, good use of valid_kwargs but imo we should also get rid of the mutated kwargs!

[github-actions]

[For maintainers] Suggested jobs to run (before merge)

run-slow: pix2struct

[zucchini-nlp]

sorry for nitty-picking after PR is merged, For my own understanding, do we need it in here, because not all image processors are VLM-specific and image_seq_length isn't always needed for them

[yonigozlan]

No worries, I agree with you we shouldn't really have this here, as this should be a processor attribute, but a lot of models on the hub have it as an image processor attribute, so I put it here for BC...