Improved TF inputs
Author: jplu
Pull request: yes

# What does this PR do?

This PR aims to improve the input processing of the TF models. Currently the inputs of each model are processed at least twice (once in the model, once in the main layer) and at most four times for the Seq2Seq models (once in the model, once in the main layer, once in the encoder layer and once in the decoder layer). This is a bit overkill and slows down the performance of a forward pass.

To fix this issue, we introduce a flag in order to know if the incoming inputs are already processed or not, if yes we keep them as they are otherwise we run the input processing.


Comments:
As said offline, I feel like this adds unnecessary complexity for no real gain. I don't think this necessarily slows things down, and if it does I'm sure it's by a negligible margin. 

As you mentioned offline this also fixes a bug, so if you find a way to integrate this in the `input_processing` method as you've mentioned I may be in favor of this change.

Ok, I will rethink this to integrate it inside `input_processing`

@LysandreJik @patrickvonplaten the check is now inside `input_processing` (done for BERT only to show an example). Does-it fits you better?

Yes it's cleaner! I still don't really like the `already_processed=True`, but I understand why it's necessary.

By the way, is there a reason we re-specify all the inputs for base models after processing the inputs? Can't we just unpack the `inputs` directly in the transformer? 

Instead of:

```py
        inputs = input_processing(
            func=self.call,
            config=self.config,
            input_ids=input_ids,
            attention_mask=attention_mask,
            token_type_ids=token_type_ids,
            position_ids=position_ids,
            head_mask=head_mask,
            inputs_embeds=inputs_embeds,
            output_attentions=output_attentions,
            output_hidden_states=output_hidden_states,
            return_dict=return_dict,
            training=training,
            kwargs_call=kwargs,
        )
        outputs = self.bert(
            input_ids=inputs["input_ids"],
            attention_mask=inputs["attention_mask"],
            token_type_ids=inputs["token_type_ids"],
            position_ids=inputs["position_ids"],
            head_mask=inputs["head_mask"],
            inputs_embeds=inputs["inputs_embeds"],
            output_attentions=inputs["output_attentions"],
            output_hidden_states=inputs["output_hidden_states"],
            return_dict=inputs["return_dict"],
            training=inputs["training"],
        )
```

we would have:

```py
        inputs = input_processing(
            func=self.call,
            config=self.config,
            input_ids=input_ids,
            attention_mask=attention_mask,
            token_type_ids=token_type_ids,
            position_ids=position_ids,
            head_mask=head_mask,
            inputs_embeds=inputs_embeds,
            output_attentions=output_attentions,
            output_hidden_states=output_hidden_states,
            return_dict=return_dict,
            training=training,
            kwargs_call=kwargs,
        )
        outputs = self.bert(**inputs)
```

which looks cleaner and we there would be no need to mention `already_processed`. Looking at it, I would expect the `input_processing` method to do the full processing for the model inputs, so I don't see why we would need to redefine what inputs we're sending to the model; the selection should already have been made in the `input_processing`.

Please let me know if this has already been discussed or if I'm missing something.

That's cleaner indeed, but without the `already_processed` argument I don't see how we can know that the input has already been processed or not. 

How do you know from:
```python
 inputs = input_processing(
            func=self.call,
            config=self.config,
            input_ids=input_ids,
            attention_mask=attention_mask,
            token_type_ids=token_type_ids,
            position_ids=position_ids,
            head_mask=head_mask,
            inputs_embeds=inputs_embeds,
            output_attentions=output_attentions,
            output_hidden_states=output_hidden_states,
            return_dict=return_dict,
            training=training,
            kwargs_call=kwargs,
        )
```

That the given inputs have already been processed or not without having a flag that gives you this info? :(

OK, I might have found the solution, need to think a bit more about this, but wait the next push and you will let me know if it looks better :)

@LysandreJik Since the last push now `input_processing` handles 100% of the process, and now the calls can be like `outputs = self.bert(**inputs)`.

If everyone is ok with this last version I will update accordingly the other models.

To be honest, I don't really see the point of this PR (but maybe there is something I'm not seeing or misunderstood) - is this PR just to run `input_processing` 1 time instead of possibly 4 times? Or is it also fixing a bug/enabling functionality that didn't exist before? Is that speed-up even noticeable? 

I don't think the trade-off between an (I assume) tiny speed-up of the forward pass vs. added complex logic for the user + much more code is worth it here...also this PR would again change all forward functions of all models I think, no? So, we'll run into a bunch of merge conflicts here again (not that big of an issue though)

This is also for fixing an issue on the inputs that are a list. If the input is a list, the list is recursively processed. I thought you saw the thread we had with @LysandreJik . I'm copy pasting the explanation here.

If we have the input `input_ids=[[[1,2,3]], [[1,1,1]]]` after the first processing we get `{"input_ids": [[1,2,3]], "attention_mask": [[1,1,1]]}`, after the second processing we get `"input_ids": [1,2,3], "attention_mask": [1,1,1]`, after the third processing we get `input_ids=1, attention_mask=1` after the fourth processing we get an error.

So in order to avoid this issue, we should parse only once eveytime the input.

> This is also for fixing an issue on the inputs that are a list. If the input is a list, the list is recursively processed. I thought you saw the thread we had with @LysandreJik . I'm copy pasting the explanation here.
> 
> If we have the input `input_ids=[[[1,2,3]], [[1,1,1]]]` after the first processing we get `{"input_ids": [[1,2,3]], "attention_mask": [[1,1,1]]}`, after the second processing we get `"input_ids": [1,2,3], "attention_mask": [1,1,1]`, after the third processing we get `input_ids=1, attention_mask=1` after the fourth processing we get an error.
> 
> So in order to avoid this issue, we should parse only once eveytime the input.

Got it! Thanks for sharing this here! Then yes, adding as little new logic as possible and boilerplate code to fix it is fine with me

I agree with @patrickvonplaten comments here and I would like to avoid touching all the files for this (however touching all the files for the change `outputs = self.bert(**inputs)` would be welcome as it's more readable).

An option to do it all in the `input_processing` without hurting the readability of all model files is to have `input_processing` return a subclass of dict that we could call `ProcessedInputs`. Then testing for that subclass at the beginning of the function (and directly returning the result in that case) would be enough.

> An option to do it all in the input_processing without hurting the readability of all model files is to have input_processing return a subclass of dict that we could call ProcessedInputs. Then testing for that subclass at the beginning of the function (and directly returning the result in that case) would be enough.

WHOA!!! I love this idea! This is much better indeed, we get a better readability and a better checking of what has been processed or not. Thank you very much for sharing this idea ! I will close this PR and rethink it accordingly to what you proposed :)

If there is no bug, I think it may be wise not to spend too much time on it either. Doing the computation 4 times isn't much of an issue as it seems negligible compared to a forward pass' execution time.

Also, knowing how TF becomes annoying in graph mode, I would be very surprised if it could handle conditional statements with subclasses in its graph

Now that no issue has been identified, I would put this as an aside project. I don't mind checking this on my personal time :)