[Config, Caching] Remove `output_past` everywhere and replace by `use_cache` argument
Author: patrickvonplaten
Pull request: yes

The `config.output_past` variable is removed and replaced by a function argument `use_cache`.
The reasons for this were explained in PR: https://github.com/huggingface/transformers/pull/3682

Affected models are:
T5, Bart, GPT2, XLNet, CTRL, TFGPT2, TFCTRL, TFXLNET

It is made sure that the change **does not break backwards compatibility** by setting `use_cache=True` by default since `config.output_past` was set to `True` by default before.
This can also be checked by seeing that none of the tests had to be changed (except T5's where the last PR: https://github.com/huggingface/transformers/pull/3682 for GPT2 and CTRL broke the backward compatibility of the default output length of T5)

I made the behavior of using the `past` variable the same in GPT2, T5, and CTRL. The logic is the following: 
If the user decides to use `past` the `past` key-value states are cached and output.
The user can then optionally only input the last `input_ids` instead of all previous ones.
if the user decides to use `past`, the `last_hidden_states` output is reduced to only the last tensor instead of the same length as the `input_ids` (this is the same as before and cannot really be changed anyways because when caching keys and values earlier outputs cannot be calculated anymore and should not to improve speed).

It is made sure that if `use_cache` is False, nothing is cached! This means that a lot of memory can be saved when the user needs to be memory efficiency (this was not the case before).

All of this should be documented well in each of the models docstrings - let me know if something is badly documented and I'll change it :-) 

Would be nice if you could take a look @sshleifer @thomwolf @LysandreJik @yjernite 


Comments:
# [Codecov](https://codecov.io/gh/huggingface/transformers/pull/3734?src=pr&el=h1) Report
> Merging [#3734](https://codecov.io/gh/huggingface/transformers/pull/3734?src=pr&el=desc) into [master](https://codecov.io/gh/huggingface/transformers/commit/7972a4019f4bc9f85fd358f42249b90f9cd27c68&el=desc) will **increase** coverage by `0.00%`.
> The diff coverage is `89.47%`.

[![Impacted file tree graph](https://codecov.io/gh/huggingface/transformers/pull/3734/graphs/tree.svg?width=650&height=150&src=pr&token=9qOlN6Hb1c)](https://codecov.io/gh/huggingface/transformers/pull/3734?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master    #3734   +/-   ##
=======================================
  Coverage   78.26%   78.26%           
=======================================
  Files         106      106           
  Lines       17928    17956   +28     
=======================================
+ Hits        14031    14054   +23     
- Misses       3897     3902    +5     
```


| [Impacted Files](https://codecov.io/gh/huggingface/transformers/pull/3734?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/transformers/modeling\_ctrl.py](https://codecov.io/gh/huggingface/transformers/pull/3734/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ19jdHJsLnB5) | `97.03% <80.00%> (-1.22%)` | :arrow_down: |
| [src/transformers/modeling\_gpt2.py](https://codecov.io/gh/huggingface/transformers/pull/3734/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ19ncHQyLnB5) | `85.93% <86.66%> (-0.28%)` | :arrow_down: |
| [src/transformers/modeling\_tf\_gpt2.py](https://codecov.io/gh/huggingface/transformers/pull/3734/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ190Zl9ncHQyLnB5) | `95.43% <86.66%> (-0.75%)` | :arrow_down: |
| [src/transformers/modeling\_tf\_ctrl.py](https://codecov.io/gh/huggingface/transformers/pull/3734/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ190Zl9jdHJsLnB5) | `98.40% <87.50%> (-1.18%)` | :arrow_down: |
| [src/transformers/modeling\_utils.py](https://codecov.io/gh/huggingface/transformers/pull/3734/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ191dGlscy5weQ==) | `90.96% <90.90%> (-0.13%)` | :arrow_down: |
| [src/transformers/modeling\_tf\_utils.py](https://codecov.io/gh/huggingface/transformers/pull/3734/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ190Zl91dGlscy5weQ==) | `92.96% <91.66%> (+0.16%)` | :arrow_up: |
| [src/transformers/configuration\_utils.py](https://codecov.io/gh/huggingface/transformers/pull/3734/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9jb25maWd1cmF0aW9uX3V0aWxzLnB5) | `97.01% <100.00%> (ø)` | |
| [src/transformers/modeling\_bart.py](https://codecov.io/gh/huggingface/transformers/pull/3734/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ19iYXJ0LnB5) | `96.48% <100.00%> (-0.02%)` | :arrow_down: |
| [src/transformers/modeling\_t5.py](https://codecov.io/gh/huggingface/transformers/pull/3734/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ190NS5weQ==) | `83.74% <100.00%> (+0.53%)` | :arrow_up: |
| [src/transformers/modeling\_tf\_xlnet.py](https://codecov.io/gh/huggingface/transformers/pull/3734/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ190Zl94bG5ldC5weQ==) | `89.08% <100.00%> (+0.02%)` | :arrow_up: |
| ... and [2 more](https://codecov.io/gh/huggingface/transformers/pull/3734/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/huggingface/transformers/pull/3734?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/huggingface/transformers/pull/3734?src=pr&el=footer). Last update [7972a40...0d27b0e](https://codecov.io/gh/huggingface/transformers/pull/3734?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


IMO, it's not a configuration option because it's either not configurable or does not need to be configured at __init__.


Whether a model **implements** caching could be a property of the model, but the config should have nothing to do with that since it can't effect it.

If a model implements caching, you nearly always want to use it to speed up generation, but never want to use it during training. So if you generate during your validation step, should you reload your model with a new config? I think not.


I understand. I still feel that it should not be an input to the forward method which currently accepts tensors of inputs to be fed to the model. I don't think having a boolean flag here would make sense.

I see it exactly as `output_attentions` and `output_hidden_states`, which are configurable with the configuration and are not boolean flags passed during the forward method. How is that different? 

The only difference is that there is a common use case where you want the flag to be true during validation and false during training.

> I understand. I still feel that it should not be an input to the forward method which currently accepts tensors of inputs to be fed to the model. I don't think having a boolean flag here would make sense.
> 
> I see it exactly as `output_attentions` and `output_hidden_states`, which are configurable with the configuration and are not boolean flags passed during the forward method. How is that different?

Sorry, I probably should have waited until you answered on the conversation there was about `use_cache` vs `config.output_past` in #3682. 

I also noticed during the PR that the `forward()` function expects usually only tensors so that the flag does not fit in. 

I'm still in favor of using `use_cache` as an argument though because it gives the user more control over the memory vs. speed trade-off by setting the `use_cache` flag. As @sshleifer said that's especially important when you don't want to use caching during training, but want to speed it up during validation. When having `output_past` in this case, the user would have to change the config for every Attention Layer in the decoder. 
Another smaller reason for me to favor `use_cache` is that a lot of models cannot or do not output the `past` key value states. 

But happy to change it back or maybe there is another way that would give the user more control and not put it in the `forward` signature? @thomwolf 

Why is it important that `forward` only take tensors/None?

These are good points.

I think that as @LysandreJik mentioned `use_cache` indeed falls in the same category as `output_attentions` and `output_hidden_states`, i.e. parameters which modify the model behavior without changing its architecture it-self (i.e. can be changed without re-loading/re-instantiating the model).

I also agree with @sshleifer that people may want to alter this behavior between training and testing but I think they may also not want to have to specify this behavior each time they run the forward pass.

Overall, I think this is actually also the same category as the parameters we have in the `.generate()` method.

So I think the behavior we designed for `.generate()` with @patrickvonplaten could be the way to go here:
- have default parameter values in the configuration (that can thus be tweaked at model initialization as well), and
- allow the user to override these defaults parameters at run time with values provided to the `forward()` pass.

It would actually be great to have this behavior implemented for `output_attentions` and `output_hidden_states` as well (this wouldn't be a breaking change).

What do you think @LysandreJik @patrickvonplaten @sshleifer @julien-c ?

I like @thomwolf suggestion much better than the status quo.

I slightly prefer no mention of `use_cache` on config for a few minor reasons, but I don't feel too strongly:
1. to avoid the proliferation of  `if x is None: x = config.x`
2. logic can then be controlled in `prepare_inputs_for_generation` and tracked with version control.
3. configs are long and getting longer and maintaining them is costlier than maintaining tested code.

These arguments also apply to `output_attentions` and `output_hidden_states`, but there we have more of a backwards compatibility issue.


Ok those are fair points. I agree with @thomwolf's proposition as well. Adding those arguments to the forward's signature means that from now on we'll control the behaviour of models according to these arguments, and not what's written in the configuration.

I'm okay with this but it is a somewhat of a big change in the API, which we should document.

Is this good for merge? 

I can merge this and open a new PR regarding adding `output_hidden_states` and `output_attentions` to the models signature. I added `use_cache` to the docs when relevant. Should I add it anywhere else in the docs? @LysandreJik