TF: inputs vs input_ids
Author: sshleifer
Pull request: no
Labels: name: wontfix

Why should TF encoder_decoder models take inputs instead of input_ids ?

https://github.com/huggingface/transformers/blob/master/tests/test_modeling_tf_common.py#L328

@patrickvonplaten

Comments:
Think it's required for some weird keras inner workings, or @LysandreJik ? I remember I had to change them to `inputs` in TF T5 at some point as well.

### Background for Keras inner workings:

(taken from the docs)

TF 2.0 models accepts two formats as inputs:

- having all inputs as keyword arguments (like PyTorch models), or
- having all inputs as a list, tuple or dict in the first positional arguments.

If you choose the second option, there are three possibilities you can use to gather all the input Tensors
in the first positional argument :

- a single Tensor with input_ids only and nothing else: `model(inputs_ids)`
- a list of varying length with one or several input Tensors IN THE ORDER given in the docstring:
  `model([input_ids, attention_mask])` or `model([input_ids, attention_mask, token_type_ids])`
- a dictionary with one or several input Tensors associated to the input names given in the docstring:
  `model({'input_ids': input_ids, 'token_type_ids': token_type_ids})`

The first argument name is therefore more appropriate as `inputs` rather than `input_ids`, since it can contain all the inputs. This is why you can see such  a snippet at the beginning of each transformer layer, in order to gather all inputs:

```py
        if isinstance(inputs, (tuple, list)):
            input_ids = inputs[0]
            past = inputs[1] if len(inputs) > 1 else past
            attention_mask = inputs[2] if len(inputs) > 2 else attention_mask
            token_type_ids = inputs[3] if len(inputs) > 3 else token_type_ids
            [...]
            assert len(inputs) <= 10, "Too many inputs."
        elif isinstance(inputs, (dict, BatchEncoding)):
            input_ids = inputs.get("input_ids")
            past = inputs.get("past", past)
            attention_mask = inputs.get("attention_mask", attention_mask)
            token_type_ids = inputs.get("token_type_ids", token_type_ids)
            [...]
            assert len(inputs) <= 10, "Too many inputs."
        else:
            input_ids = inputs
```

### Actual reason why things are done this way in the tests:

It stems from that PR: https://github.com/huggingface/transformers/pull/3547.

Previously it was written in the T5 forward pass as `decoder_input_ids`, while it could be a dict and, therefore, contain everything. Looking at it now, I guess it could be put as `input_ids` too (since it's a positional argument, the naming doesn't really matter). 

T5 is supporting 

```python
def call(inputs, **kwargs):
        if isinstance(inputs, dict):
            kwargs.update(inputs)
        else:
            kwargs["inputs"] = inputs

        # retrieve arguments
        inputs = kwargs.get("inputs", None)
		...
```
I will try to kwarg everything, because to me this is an explosion of input types and boilerplate.

I see what you mean now @sshleifer! Yes you are right in T5 the name was wrong IMO. Fixing this now in a bigger TF refactor PR.

So as you said this line:
https://github.com/huggingface/transformers/blob/master/tests/test_modeling_tf_common.py#L328
should be changed to just:
```
input_ids = inputs_keywords.pop("input_ids", None)
```



This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.