a bug in generation_beam_search.py
Author: ZhaoQianfeng
Pull request: no

I wanted to implement my own beam_search function and read 

> src/transformers/generation_beam_search.py

source code for help.In the process of reading the source code, I found an unreasonable place, maybe it could be seen as a bug.
Here is the problem:
In the process() function of generation_beam_search.py, line 223:
```python
if (eos_token_id is not None) and (next_token.item() == eos_token_id):
                    # if beam_token does not belong to top num_beams tokens, it should not be added
                    is_beam_token_worse_than_top_num_beams = beam_token_rank >= self.num_beams
                    if is_beam_token_worse_than_top_num_beams:
                        continue
                    beam_hyp.add(
                        input_ids[batch_beam_idx].clone(),
                        next_score.item(),
                    )
```
when we generate a complete sentence(get a eos_token in top beam_size),we send the `input_ids[batch_beam_id]`(which is the sequence of tokens),and the score to the beam_hyp.add(). eos_token is not included in `input_ids[batch_beam_id]`, but it's ok, beacuse bos_token is included in `input_ids[batch_beam_id]`, when we calculate the score of whole sentence at line 334:
```python
  def add(self, hyp: torch.LongTensor, sum_logprobs: float):
        """
        Add a new hypothesis to the list.
        """
        score = sum_logprobs / (hyp.shape[-1] ** self.length_penalty)

        ...
```
wo got correct hyp.shape[-1],which can be  seen as how many elements are added to get the sum_logprobs.

for example, `input_ids[batch_beam_id] = ['<bos>', ' she', 'is', 'a', 'cute', 'girl']`,and assume the score of this beam is -1.2 .we know that -1.2 is the probability sum of `she`, `is`, `a`, `cute`, `girl`. In next step we generate a eos_token, the score of eos_token is -0.3 and we update socre to -1.5, -1.5 is probability sum of `she`, `is`, `a`, `cute`, `girl`,`<eos>`. we send `input_ids[batch_beam_id] = ['<bos>', ' she', 'is', 'a', 'cute', 'girl']` and -1.5 as parameters to beam_hyp.add(...), and we calculate the whole sentence score by `score = sum_logprobs / (hyp.shape[-1] ** self.length_penalty)`,
because **`len(['<bos>', ' she', 'is', 'a', 'cute', 'girl'])` == `len([' she', 'is', 'a', 'cute', 'girl','<eos>'])`**, so hyp.shape[-1] is correct.


**the real problem is in finalize() function:**
line 227:
```python
            # need to add best num_beams hypotheses to generated hyps
            for beam_id in range(self.num_beams):
                batch_beam_idx = batch_idx * self.num_beams + beam_id
                final_score = final_beam_scores[batch_beam_idx].item()
                final_tokens = input_ids[batch_beam_idx]
                beam_hyp.add(final_tokens, final_score)
```
we use finalize() function to manually add those hypothese which dont generate eos_toekn untill max_length to beam_hyp.
in instance, `final_tokens=  input_ids[batch_beam_idx] = ['<bos>', ' she', 'is', 'a', 'cute', 'and', 'smart']` and we need to manually add it to beam_hyp because it reaches the max_length.
look at this line above:
```python
beam_hyp.add(final_tokens, final_score)
```
now len(final_tokens) == len(['\<bos\>', ' she', 'is', 'a', 'cute', 'and', 'smart']) == 7, so hyp.shape[-1] equals 7.But the sum_logprobs is calculated by log probability sum of `she`, `is`, `a`, `cute`, `and`, `smart`, only 6 element! **It's a different case from process(),because we have not add eos_token probablity, so the hyp.shape[-1] should minus 1 in this case!** 

My English is poor, hope you can understand my meaning. Looking forward for some feedback.


Comments:
Hey @ZhaoQianfeng I think I know what you mean! So in short you are saying that `beam_hyp.add(...)` should be behaving differently depending on whether it finished with or without EOS token right? 

Still not sure whether this is a real problem though... -> Could you maybe open a PR that quickly shows the changes you would want beam search to have and we can take a look at some code? This would be super helpful! Thanks a lot for diving into the code :-) 

Hello @patrickvonplaten. Glad you know what i mean. Those hypotheses which finished without EOS are calculated with longer length than they should be, so they have higher scores than they should be, this may cause them to compete unfairly with those with EOS and make us incorrectly throw away those "with EOS" hypotheses . 
Maybe it is not a severe problem, above situation maybe not common. Anyway, I modify the code here #8890 but haven't test it.I will be happy if it helps.

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the [contributing guidelines](https://github.com/huggingface/transformers/blob/master/CONTRIBUTING.md) are likely to be ignored.