Update generation_beam_search.py
Author: ZhaoQianfeng
Pull request: yes

BeamHypotheses.add() now behave differently depending on whether it finished with or without EOS token.

# What does this PR do?

see the discussion here #8722 

## Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors which may be interested in your PR.

<!-- 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.

 albert, bert, XLM: @LysandreJik
 GPT2: @LysandreJik, @patrickvonplaten
 tokenizers: @mfuntowicz
 Trainer: @sgugger
 Benchmarks: @patrickvonplaten
 Model Cards: @julien-c
 examples/distillation: @VictorSanh
 nlp datasets: [different repo](https://github.com/huggingface/nlp)
 rust tokenizers: [different repo](https://github.com/huggingface/tokenizers)
 Text Generation: @patrickvonplaten, @TevenLeScao
 Blenderbot, Bart, Marian, Pegasus: @patrickvonplaten
 T5: @patrickvonplaten
 Rag: @patrickvonplaten, @lhoestq
 EncoderDecoder: @patrickvonplaten
 Longformer, Reformer: @patrickvonplaten
 TransfoXL, XLNet: @TevenLeScao, @patrickvonplaten
 examples/seq2seq: @patil-suraj
 examples/bert-loses-patience: @JetRunner
 tensorflow: @jplu
 examples/token-classification: @stefan-it
 documentation: @sgugger
 FSMT: @stas00
 -->


Comments:
Hey @ZhaoQianfeng,

Thanks a lot for making the PR. I thought about this a bit and I think we don't have to change anything actually.

The reason is the following. Let's say you want to generate up to a length of 5.

BOS is the start token which should be counted as part of the input length. However EOS should not be counted towards the sequence length for the length penalty IMO since it's the trigger to finish generation. 

So the input:

[BOS, hey, there, EOS] -> is ok for me to be counted as a sequence length of 3 ([BOS, Hey, there]) for the length penalty. I don't think the EOS token itself should penalize.
However, un unfinished generation, such as [BOS, hey, there, how, are] should be counted to have a sequence length of 5 since it isn't finished.

If we would merge this PR, this would mean that [BOS, hey, there, peter, EOS] would receive the same length penalty as [BOS, hey, there, how, are], but IMO they should not. The first sequence is finished (*i.e.* shorter) than the second one.

So I'd prefer to leave it as it is. I think it's the right approach. Thanks a lot for looking into this however :-) 

Hey @patrickvonplaten ,
I think you are right!

For your example, my reason why ` [BOS, hey, there, peter, EOS]` and `[BOS, hey, there, how, are] `should have same length penalty is that the former probability is calculated by `log(P(hey))+log(P(there))+log(P(peter))+log(P(EOS))`, and the latter probability is calculated by  `log(P(hey))+log(P(there)+log(P(how)+log(P(are))`, both 4 elements.So I used to think that they should be divided by same length.

But I think your explanation is more reasonable and convincing, the former sentence is  actually shorter than the latter sentence!It should be what **length peanalty** really means.  Thank you for taking the time to discuss this issue!:-)