Image pipelines spec compliance

[Rocketknight1]

This PR checks for Hub spec compliance for a lot of image pipelines that have very similar inputs. In most cases, no changes are required except deprecating the timeout argument, and renaming the images input to inputs.

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

[LysandreJik]

Looks good, but I wonder if we shouldn't mention in the deprecation/in the pipeline docs that the pipelines will now be 1-1 with the HF specs in general, to justify the change from images to inputs

[LysandreJik]

I wonder if these warnings (and the docs) shouldn't point to the global definition of specs across the HF ecosystem to justify such a change

[Rocketknight1]

What's the right document to link users to as the "source of truth" there? Just the folder in the Hub repo here?

[LysandreJik]

This file is lacking the copyright!
Can you add it?

[Rocketknight1]

Done!

[Wauplin]

Note for myself: this change won't break Inference API since it's using positional argument and not keyword one (see here - private link). So all good to change it, same for all other pipelines :)

[Wauplin]

I wonder if this is working as expected. Say you have a user script like this:

pipeline = ImageClassificationPipeline(...)
output = pipeline(images=<my-image>, foo="bar")

with this PR, the pipeline call will fail with something like "requires 1 positional argument, got 0" because inputs is not provided.

[Wauplin]

what I would suggest is to have a decorator like this:

def _rename_positional_arg(old_name: str) -> Callable:

    def _decorator(fn: Callable) -> Callable:

        def _inner(*args, **kwargs):
            if old_name in kwargs:
                if len(args) > 0 or "inputs" in kwargs:
                    raise ValueError(f"Cannot pass 'inputs' and '{old_name}' at the same time.")
                warnings.warn("Deprecation message 'you should use inputs instead of {old_name}' ...")
                kwargs["inputs"] = kwargs.pop(old_name)
        return fn(*args, **kwargs)

    return decorator

which you can use like this:

...

@_rename_positional_arg(old_name="images")
def __call__(self, inputs: Union[str, List[str], "Image.Image", List["Image.Image"]] = None, **kwargs):
    ...  # here you are guaranteed "inputs" is passed

This way, the pipeline will both if:

• inputs are passed as positional args (as done in Inference API)
• inputs are passed as inputs
• inputs are passed as images

(I did not find a satisfying decorator name but you have the idea^^)

[Rocketknight1]

Actually, it should work! I set a new default argument value of None for inputs, so the pipeline can be called without that argument. However, it will raise an exception if neither inputs nor images are passed.

[Rocketknight1]

In other words:

Case 1: Users are passing "images" as a positional argument. The argument is renamed to inputs and they don't notice any change

Case 2: Users are passing "images" as a kwarg. inputs is now None but **kwargs contains an "images" key. The code detects this and sets inputs = kwargs.pop("images"), then prints the deprecation warning

[Wauplin]

Ooh, sorry I did not see the added = None. You are completely right, forget what I meant above :)

[LysandreJik]

Actually thinking more about it and inline with having less and less warnings in transformers, I wonder if we want to have a deprecation warning for images vs outputs, or if we just want to enable both without warning

[Rocketknight1]

@LysandreJik yes, I'm fine with that! Want me to just drop all the deprecation warnings? I can maybe throw an error if users specify both arguments, or something.

[Wauplin]

If no deprecation warning is raised (i.e. accept both inputs and images on the long run), I would make sure the method signature is in line with the recommendation (i.e. passing inputs). In that sense, I don't think that having inputs: ... = None on the long run is desirable as it can be misleading to users. If it was only for a few versions before settling for the definitive signature (i.e. no images at all), it would have been fine but that's not the case, right?

[Rocketknight1]

@Wauplin Unfortunately, if we don't set inputs = None, then users have to pass inputs, and we can't accept images as an alternative.

I think it's okay, though! There are a lot of classes in transformers that have None defaults for mandatory inputs - it just means that we handle the error when the argument is missing somewhere else.

[LysandreJik]

I'll let you decide the best course of action in terms of signature/arguments @Rocketknight1, but dropping the deprecation warnings seems to me like the right thing to do here

[Rocketknight1]

Done! Warnings are removed, but I'll accept both keywords for now - I don't think the main input having a default of None should be too confusing, especially since the docstrings are clear.