Handle padding in decoder_inputs_id when using generate
Author: LittlePea13
Pull request: yes

# What does this PR do?

<!--
Congratulations! You've made it this far! You're not quite done yet though.

Once merged, your PR is going to appear in the release notes with the title you set, so make sure it's a great title that fully reflects the extent of your awesome contribution.

Then, please replace this with a description of the change and which issue is fixed (if applicable). Please also include relevant motivation and context. List any dependencies (if any) that are required for this change.

Once you're done, someone will review your PR shortly (see the section "Who can review?" below to tag some potential reviewers). They may suggest changes to make the code even better. If no one reviewed your PR after a week has passed, don't hesitate to post a new comment @-mentioning the same persons---sometimes notifications get lost.
-->

<!-- Remove if not applicable -->

Fixes #10478 for pytorch version of `generate()` in generate_utils.py

As described in the issue, when `decoder_input_ids` have different lengths and require padding, generation continues after the padding tokens. This PR modifies that behavior so that tokens are generated before the padding for each element in the batch.

## Before submitting
- [ ] 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/master/CONTRIBUTING.md#start-contributing-pull-requests),
      Pull Request section?
- [x] 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.
- [ ] Did you make sure to update the documentation with your changes? Here are the
      [documentation guidelines](https://github.com/huggingface/transformers/tree/master/docs), and
      [here are tips on formatting docstrings](https://github.com/huggingface/transformers/tree/master/docs#writing-source-documentation).
- [ ] Did you write any new necessary tests?


## Who can review?

I am adding those who I see more often in the git blame of the file:
@patrickvonplaten, @patil-suraj, @yjernite

<!-- Your PR will be replied to more quickly if you can figure out the right person to tag with @
 If you know how to use git blame, that is the easiest way, otherwise, here is a rough guide of **who to tag**.
 Please tag fewer than 3 people.

Models:

- albert, bert, xlm: @LysandreJik
- blenderbot, bart, marian, pegasus, encoderdecoder,  t5: @patrickvonplaten, @patil-suraj
- longformer, reformer, transfoxl, xlnet: @patrickvonplaten
- fsmt: @stas00
- funnel: @sgugger
- gpt2: @patrickvonplaten, @LysandreJik
- rag: @patrickvonplaten, @lhoestq
- tensorflow: @jplu

Library:

- benchmarks: @patrickvonplaten
- deepspeed: @stas00
- ray/raytune: @richardliaw, @amogkam
- text generation: @patrickvonplaten
- tokenizers: @n1t0, @LysandreJik
- trainer: @sgugger
- pipelines: @LysandreJik

Documentation: @sgugger

HF projects:

- nlp datasets: [different repo](https://github.com/huggingface/nlp)
- rust tokenizers: [different repo](https://github.com/huggingface/tokenizers)

Examples:

- maintained examples (not research project or legacy): @sgugger, @patil-suraj
- research_projects/bert-loses-patience: @JetRunner
- research_projects/distillation: @VictorSanh

 -->

This is my first PR, so let me use it to thank everyone involved in this library for all the cool work :)

Comments:
Some tests failed due to pad_token_id being None. Is this really a possibility for any Transformer model?

Added a check for `token_pad_id` equal to `None` and tests pass, but it is not very elegant, any feedback will be appreciated.

I just noticed that one test, `run_tests_flax`, failed, however no changes are made that should affect that. @patrickvonplaten or @patil-suraj, when you have the time let me know if there's anything else I should be aware of regarding that test.

Hi @LittlePea13 

Thanks a lot for the PR. 

I understand the problem but it seems like an edge case and overall I'm not really in favor of supporting this. The philosophy of `generate` is to keep it simple and extensible and not try to cover all use-cases.
We generally try to keep such if/else statements to a minimum. This change will introduce a lot of complexity in the code.

Also with this change, we won't be able to use `use_cache`, which will slow down generation significantly.

One could always just call generate multiple times if the `decoder_input_ids` are of different length. I would rater batch the sentences together which require the  `decoder_input_ids` of the same length and then pass those to `generate` instead of passing  `decoder_input_ids` of different lengths. Which would cover this use-case.

But thanks a lot for your work! It's a good practice to first propose and discuss the solution in the issues before opening a PR. 

What do you think @patrickvonplaten?

Hi @patil-suraj thanks for the feedback, I agree that this introduces something specific, and I am not too happy on how it is dealt with for the reasons you point out. However, it seemed useful to have a way to deal with it since it is what one would expect if one inputs `decoder_input_ids` to the `generate` function with some padding.

I opened an issue but was too impatient and opened a PR (sorry about that), basically because I needed this for my own work and coded it anyways.

Perhaps this doesn't belong here, but in any case I feel like including extra documentation about `decoder_input_ids` [here](https://huggingface.co/transformers/main_classes/model.html?highlight=beam%20search#transformers.generation_utils.GenerationMixin.generate) would be beneficial, maybe explaining this behavior (ie. they have to be of the same length).

Hey @LittlePea13, 

Thanks for raising awareness for your problem and thanks for opening a PR! I agree with @patil-suraj here and would prefer to not include such specific code in `generate()`. 

In general the philosophy for more specific use cases of `generate()` is to directly use the "sub"-generate methods, such as `sample()`, `greedy_search()`, and `beam_search()` as explained here:  https://discuss.huggingface.co/t/big-generate-refactor/1857

I think in your use case, we could do a similar trick that what was done for GPT2 for batched inference:
https://discuss.huggingface.co/t/batch-generation-with-gpt2/1517

This means that instead of passing `["This is <PAD> <PAD>", "This is a sentence"]` as `decoder_input_ids` you could pass `["<PAD> <PAD> This is", "This is a sentence"]` to make `generate()` work. Could you try this out? Also, I'd recommend to directly use `beam_search()` instead of `generate()` in your example. If it doesn't work feel free to post on the forum: https://discuss.huggingface.co/ and tag me - I'll try to help you make it work then :-) 

I think a well-written forum post would also be a great way of documenting this behavior. However I do think, it's a bit too specific for the general docs of `generate()` since they don't even include `decoder_input_ids` as an input argument to the function.

@patil-suraj and I will keep an eye out if more people run into this problem! Thanks a lot for bringing it up in any case :-)

Thanks both for the review. Indeed, I didn't think of just moving the padding to the left, much more elegant. I tried it out and it works but it produces different outputs than without padding.

The issue here is that by using directly the different "sub"-generate methods is not possible to apply the same changes, so if one wants to have the same results as if there was no difference in the sentences lengths they would still need to do a similar tweak as the one here on each method.

But this is very narrow, I don't even know if it affects performance in my case when compared to moving padding to the left. I am closing this and in case someone has a similar issue you can just refer to the changes here. 

Cheers!