fix arg name in BLOOM testing and remove unused arg document
Author: shijie-wu
Pull request: yes

# What does this PR do?

* Fix argument name in BLOOM testing (`hidden_dropout` and `attention_dropout`)
* Remove document of unused arguments (`skip_bias_add`, `skip_bias_add_qkv` and `attn_pdrop`)


## Before submitting
- [x] This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
- [x] Did you read the [contributor guideline](https://github.com/huggingface/transformers/blob/main/CONTRIBUTING.md#start-contributing-pull-requests),
      Pull Request section?
- [ ] Was this discussed/approved via a Github issue or the [forum](https://discuss.huggingface.co/)? Please add a link
      to it if that's the case.
- [x] Did you make sure to update the documentation with your changes? Here are the
      [documentation guidelines](https://github.com/huggingface/transformers/tree/main/docs), and
      [here are tips on formatting docstrings](https://github.com/huggingface/transformers/tree/main/docs#writing-source-documentation).
- [ ] 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.

@thomasw21 @stas00 @TevenLeScao 

Comments:
_The documentation is not available anymore as the PR was closed or merged._

Thank you for the fix, @shijie-wu 

Wrt `use_cache` won't it be better to actually set the default to `True` - most models have it set to `True` and most users will get bad peformance out of the box with the default during `generate` with it being `False`.

I'm aware that the cat is out of the box, but there have been a lot of tweaks to the model post its release so perhaps such change would still be in the grace period of backward compatibility. 

actually, looking at `generate` it says it defaults to `True`:

https://github.com/huggingface/transformers/blob/4157e3cd7e2bb5a7be6dc065a3e20c49cc1300ab/src/transformers/generation_utils.py#L1037-L1039

but it doesn't appear to be so, as the default it uses is `None`:

https://github.com/huggingface/transformers/blob/4157e3cd7e2bb5a7be6dc065a3e20c49cc1300ab/src/transformers/generation_utils.py#L919

but since most models have `use_cache == True` it's sometimes true.

tagging @patrickvonplaten - should the `generate` doc be corrected to say that the default is `None` to match the actual code? and say that unless set explicitly the model's config's `use_cache` setting is used?

I missed that `use_cache = True` for most models by default. If that's the case, I could update the default instead and revert the change to the doc.

So it's pretty clear it's most likely a unintended default as 1/3rd but 2 models have it set to `True` and 2/3 of models don't have it set at all:

```
$ grep -Ir 'use_cache=True' src/transformers/models/*/config* | wc -l
45
$ grep -Ir  'use_cache=False' src/transformers/models/*/config* | wc -l
2
$ grep -Ir  'use_cache=False' src/transformers/models/*/config*
src/transformers/models/bloom/configuration_bloom.py:        use_cache=False,
src/transformers/models/trocr/configuration_trocr.py:        use_cache=False,
```

so `generate's default was relying on all the models having it set to `True` which as we can see isn't always the case.

though I think my regex missed many models, let me check where the missing 100+ are.

edit: the remaining ones don't have `use_cache` set in the `configuration_foo.py` files.

~If all models set `use_cache=True` shouldn't `.generate` also have `use_cache=True` by default?~

edit: based on new info, this is no longer a question. I am happy to fix default for `bloom` and `trocr`.

Please let's wait for others to chime in. Perhaps setting it to `False` was not an omission but an intentional move. 

also tagging @younesbelkada 

The `use_cache` interior mechanisms are indeed a bit hard to understand. Think we haven't done a good job here. Thanks for pointing this out. 

IMO, `use_cache` should be set to `True` for all models and IMO it was probably a mistake to not set it to `True` for BLOOM.
Or is there a maybe a reason behind it (cc @thomasw21 @younesbelkada ?). If `use_cache=False` it means that no tensors containing past generated keys and values are moved around - was this maybe done on purpose given the size of the model? More specifically, by setting it to False we save `num_layers` x `hidden_size` x `2` x `num_previous_generated_tokens` memory.

 Regarding the generate docs we decided with @gante  to write the defaults to how the default to depending on what's set in the config, so I think the docstring is ok/correct there. Also note that `generate` will soon go through a major refactor regarding the configuration.

Regarding whether we should change the default to True  - I actually don't know really. If it would have been for a small model I would have said definitely yes, but for BLOOM which requires multi-gpu for inference I'm not 100% sure how much the memory consumption goes up when enabling it and running generate with `accelerate` - could we try it out ? cc @younesbelkada 


Thank you for this great feedback, Patrick.

In addition as I have just discovered there are many models that don't set `use_cache` in their config at all. And I don't think there is a super-class default that is inherited. Perhaps there should be one?

Also it appears that `use_cache` is often a tri-state:

https://github.com/huggingface/transformers/blob/4157e3cd7e2bb5a7be6dc065a3e20c49cc1300ab/src/transformers/models/t5/modeling_t5.py#L918

so perhaps `generate` could reflect that - or if things change with refactor then I trust the new way will take care of this.

wrt memory usage in Bloom - recalculating say 100 tokens for each new token would be quite slow. So it's a question of whether `use_cache=False` + large BS gives better throughput than `use_cache=True`+small BS using the same amount of memory - probably can measure empirically? though the outcome would be highly hardware dependent.

@LysandreJik, could we please make a note in the next release that 

* BLOOM's `use_cache`'s original default value was incorrectly set to `False` and this release it has been corrected to `use_cache=True` - as this is somewhat backward compatibility breakage. and we hope our users will forgive us as this is still a new model that is going through minor fixes. `use_cache=True` leads to a much faster generation at the cost of larger memory requirements to store the cached values.

Thank you!

@stas00 I was thinking that we should probably also change it for tcocr model for consistency with other models on the library as suggested by @shijie-wu ?

I thought so too, but perhaps let's check in with the porter of that model to see if perhaps it was by design?

Would you like to do that? and perhaps in a separate PR so that it's loud and clear in the history of the project? with reference to this PR for context.

Thank you!

Sure, happy to take care of that! 
Will tag also the porter of the model and double check 
Thanks again!