Clean up reduce_labels for image processors

[qubvel]

What does this PR do?

• Deprecate reduce_labels for Mask2FormerImageProcessor (it is already deprecated for all other image processors)
• Remove deprecated reduce_labels for other segmentation models
• Update examples and docs with do_reduce_labels

Before submitting

• This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
• Did you read the contributor guideline,
  Pull Request section?
• Did you make sure to update the documentation with your changes? Here are the
  documentation guidelines, and
  here are tips on formatting docstrings.
• Did you write any new necessary tests?

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

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

[amyeroberts]

Thanks for working on this - great to see more alignment between the image processors.

Only comment is to not raise an exception if reduce_labels is passed in

[amyeroberts]

Sadly we can't do this directly for two reasons:

• A deprecation version wasn't specified in the message (my bad)
• This is going to break a lot of things for users

We absolutely want to push users to try and use the correct argument, and we should update this value on all of the official checkpoints. However, because this is from the config, which users might not have control over or know how to change; and there might be hundreds of configs with the old value on and off the hub, we're at risk of breaking code and losing users.

What I'd suggest here is:

• We update the warning message to specify a specific version (typically two versions after the release version this commit will be included in).
• After this version, we remove the warning. We silently handle this to maintain backwards compatibility i.e. popping the value, and make sure all official references to the flag are removed; we don't guarantee any future maintenance for this value.

[qubvel]

Yes, that's breaking changes...

The idea was to support reading old configs overriding from_dict method with

@classmethod
def from_dict(cls, image_processor_dict: Dict[str, Any], **kwargs):
    ...
    if "reduce_labels" in image_processor_dict:
            image_processor_dict["do_reduce_labels"] = image_processor_dict.pop("reduce_labels")

Here we silently replace reduce_labels with do_reduce_labels.

By raising an exception in __init__, we prevent users from creating new image_processor with deprecated parameters. That might break some fine-tuning scripts, which the user should have control of.

Does it make sense, or am I missing some use cases?
Are such kinds of breaking changes possible only over major versions like v4->v5?

[amyeroberts]

Here we silently replace reduce_labels with do_reduce_labels.

This is great - we definitely want to do this! It means any old config, if saved out, will be fully compatible.

Are such kinds of breaking changes possible only over major versions like v4->v5?

Kind of, it's more to do with the type and scale of breaking change, and the controls users have over it.

We do deprecate methods, flags, etc. within the codebase semi-regularly. And we do tolerate a certain amount of breaking changes. For example, in musicgen recently, training was enabled. This technically created a breaking change, as one of the models previously returned a loss. The update meant that anyone using the model would see the loss suddenly change. However, as the previous loss was incorrect and the model use relatively low this was deemed OK.

There's two main reasons why we wouldn't want to raise an exception here at the moment:

• Ability to change the code behaviour. Users don't always have the option of updating the config: they might not have permissions to modify on the hub.
• Awareness of configs. Many users are not familiar with the configs. It's easier to deprecate calls to e.g. object.method(foo, bar) as we emit a warning, and the user just removes that line of code. Changing the config is less obvious.

The reason I think this is because when we first tried to deprecate flags for the image processors, we got quite a few messages from users either complaining or confused about why they were happening. I'm yet to see a single config updated because of them!

It is something we could do in v5, although we may not want to.

[qubvel]

Thanks a lot for the clarification, I will summarize PR just to make sure we are on the same page.

There are 3 most popular ways to create an image processor with deprecated parameters (sorted by popularity, high->low, IMHO)

Method	Is BC in current PR
image_processor = ImageProcessor.from_pretrained(   "repo/model_with_deprecated_params" )	✅ Yes Because the user does not have control of it (by overriding from_dict())
image_processor = ImageProcessor.from_pretrained(   "repo/model_with_deprecated_params",   reduce_labels=True, )	🟥 No, raise error Because it is explicit parameter setting
image_processor = ImageProcessor(   ...   reduce_labels=True, )	🟥 No, raise error Because it is explicit parameter setting

I hope this approach:

1. Pushes the user to update their codebase, which they have control over, while still allowing using previously created models.
2. Will allow us to gradually move backward compatibility for deprecated params only in one method from_dict (ideally), making code easier to support.
3. Probably, we can also get rid of **kwargs, have a strict set of arguments, and replace checks with a decorator for renamed/removed arguments:

@depecated("reduce_labels", replace_with="do_reduce_labels", from="4.42.0", to="4.45.0")
def preprocess(...):

The decorator will rename the argument and raise a warning in between, and then we can remove it with automated checks, or leave it for a while to raise an explicit error.
At the moment, because of **kwargs, anyone might easily make a typo in an argument name and it will be silently passed, leading to unexpected behavior that is hard to debug.
As a simple example:

from transformers import DetrImageProcessor

image_processor = DetrImageProcessor.from_pretrained(
    "facebook/detr-resnet-50",
    do_nomalize=False, <----- here set as False
)
print(image_processor)

# DetrImageProcessor {
# ...
#   "do_convert_annotations": true,
#   "do_normalize": true, <----- but we get True
#   "do_pad": true, 
#   "do_rescale": true,
#   "do_resize": true,
# ...
# }

We can test this strategy with some particular models, not super popular, just to see how it works.

I might overestimate the problem or underestimate risks, user behavior, or edge cases. You have much more experience maintaining the library, so let me know if any of these thoughts are valuable. And thank you once again for your clarifications with examples, it is much easier to follow 🤗

[amyeroberts]

100% aligned with you comment above, thanks for taking the time to write this up so clearly.

I really like the idea of a decorator. Let's ask @molbap his opinion on this, as he's been handling a lot of these considerations across processors and image processor.

[molbap]

OOO for a few days but checked and it's an interesting take. I like the decorator idea too. I wholeheartedly agree with

At the moment, because of **kwargs, anyone might easily make a typo in an argument name and it will be silently passed, leading to unexpected behavior that is hard to debug.

That is almost always true. That's why getting rid of **kwargs is impossible, as we need to be able to handle arbitrary inputs without breaking runtime in situations where people have non-canonical configs.

Hence the point of having a list of valid arguments: that way, we can at least raise a warning and list the arguments passed by a user that are not doing anything. In your case, if do_normalize is part of the valid image processor arguments but do_nomalize (typo) is not, then we can raise a clear warning saying that do_nomalize isn't used anywhere.
We can even push this a bit and do a quick levenshtein check conditioning a perhaps you made a typo? message. That might be too much.

For the decorator option to mark args as deprecated, LGTM too! Less loc, and forces us to write a version number. I'd say the "from" is not necessarily useful as we want to encourage using the latest version, imo.
Thanks for looking into this @qubvel! Kind of related is #30511 that I want to merge ASAP, didn't have time to get back to it with but it's closely connected to this problem.

[qubvel]

Thanks for looking into this @molbap

Probably we can use a function signature in decorator to filter not-used kwargs instead of passing them.
What do you think about the following?

import functools
import inspect
import warnings
import numpy as np


def filter_not_used_arguments(func):
    """Filter out named arguments that are not in the function signature."""
    @functools.wraps(func)
    def wrapper(*args, **kwargs):

        sig = inspect.signature(func)
        function_named_args = sig.parameters.keys()
        valid_kwargs = {k: v for k, v in kwargs.items() if k in function_named_args}
        invalid_kwargs = {k: v for k, v in kwargs.items() if k not in function_named_args}
        
        if invalid_kwargs:
            warnings.warn(f"Unused named arguments: {', '.join(invalid_kwargs.keys())}")
        
        return func(*args, **valid_kwargs)
    
    return wrapper


class ImageProcessor:

    @filter_not_used_arguments
    def preprocess(self, image, do_normalize=False, do_reduce_labels=False):
        pass


image_processor = ImageProcessor()
image = np.ones((100, 100, 3))

# passing invalid  `do_nomalize` instead of `do_normalize`
image_processor.preprocess(image, do_nomalize=True, do_reduce_labels=True)  
# UserWarning: Unused named arguments: do_nomalize

1. This will simplify validation, we will not need to store the list of available arguments
2. Make validation method-specific instead of class-specific
3. While this is backward compatible and we do not raise an error here, IDE will help users and force users to update parameters if no **kwargs is specified for the function

[molbap]

That looks like a great solution :) looks very good to me

[qubvel]

Here, in Mask2Former, and OneFormer: num_labels is not used somewhere across code of image_processor, however it is widely used in tests. I added it explicitly for backward compatibility, in case some pipelines rely on that

[qubvel]

Also could be passed in **kwargs and excluded from filter

@filter_out_non_signature_kwargs(extra=["max_size", "num_labels"])

[qubvel]

@molbap @amyeroberts I made a new iteration here. Added two decorators

1. @deprecate_kwarg("reduce_labels", new_name="do_reduce_labels", version="4.41.0")

• rename argument if new_name is provided
• raise warning if current version < version specified, then just do it silently

2. filter_out_non_signature_kwargs(extra=["max_size"])

• filter out all named args that are not in the function signature and not specified in extra. As mentioned by @molbap it is not always possible to get rid of all **kwargs (for example, it might include complex logic of parameters interaction).

Please, have a look, feedback is appreciated 🤗

[amyeroberts]

Really beautiful work - this could be very useful across the library and will help kill so much repeated code ❤️

Only major comment is about the new files src/transformers/utils/kwargs_validation.py and src/transformers/utils/deprecation.py, and whether we want to add these whole new modules into utils.

Personally, I think src/transformers/utils/deprecation.py makes sense. For filter_out_non_signature_kwargs, I'd place it under (the admittedly badly named) utils/generic.py for now

Would be great to get a second opinion on this from @molbap!

[amyeroberts]

nit - missing copyright header

[amyeroberts]

nit - missing copyright header

[amyeroberts]

nit - convention in the library is to not mention default value if it is None. If it controls specific behaviour, we normally refer to it being set or unset

Suggested change

	old_name (str): name of the deprecated keyword argument
	version (str): version when the keyword argument was (will be) deprecated
	new_name (Optional[str], optional): new name of the keyword argument. Defaults to None.
	raise_if_ge_version (bool, optional): raise ValueError if deprecated version is greater or equal to current version. Defaults to False.
	raise_if_both_names (bool, optional): raise ValueError if both deprecated and new keyword arguments are set. Defaults to False.
	old_name (`str`): name of the deprecated keyword argument
	version (`str`): version when the keyword argument was (will be) deprecated
	new_name (`Optional[str]`, *optional*): new name of the keyword argument.
	raise_if_ge_version (`bool`, *optional*, defaults to `False`): raise ValueError if deprecated version is greater or equal to current version.
	raise_if_both_names (`bool`, *optional*, defaults to `False`): raise ValueError if both deprecated and new keyword arguments are set.

[amyeroberts]

For the script, we'll need to deprecate reduce_labels e.g. like here

[amyeroberts]

Same here re deprecation

[amyeroberts]

nit - could we call this additional_message? add_message reads as a bool to me (don't feel strongly about this, so happy if you decide to stick with the original)

[amyeroberts]

nit - I'd separate this out between setting the minimum action and the raise/notify logic, just to make it a bit clearer

Suggested change

	# raise error or notify user
	if minimum_action == Action.RAISE:
	raise ValueError(message)
	elif minimum_action == Action.NOTIFY and raise_if_ge_version and is_already_deprecated:
	raise ValueError(message)
	if minimum_action == Action.NOTIFY and raise_if_ge_version and is_already_deprecated:
	miminum_action = Action.RAISE
	# raise error or notify user
	if minimum_action == Action.RAISE:
	raise ValueError(message)

[amyeroberts]

❤️