Generate: new length penalty docstring
Author: gante
Pull request: yes

# What does this PR do?

Fixes #18971 
Fixes #18208

The docstring for `length_penalty` was incomplete and incorrect -- it only has effect with beam-based strategies and the impact of this `float` argument is the other way around (> 0.0 promotes longer sequences, not shorter sequences). This PR rewrites it.

For context, here is the [line](https://github.com/huggingface/transformers/blob/main/src/transformers/generation_beam_search.py#L872) where it is applied: `score = sum_logprobs / (hyp.shape[-1] ** self.length_penalty)`. Since `sum_logprobs` is a negative value and `hyp.shape[-1]` is the length of the sequence (positive value), it implies that a positive `self.length_penalty` will lead to a score that is smaller in magnitude as the sequence grows = more positive = higher = this sequence has increased odds of being picked.

Finally, this means that there is a mismatch between the variable name and its effect. In practice, it is not a "penalty", as a positive value actually promotes "length". However, the alternatives are breaking changes (either changing how it is applied or the variable name), which is highly undesirable.



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

> So it means that we are always enabling length penalty by default, since 1.0 is not the neutral value, right?

@sgugger correct. It is set to `1.0` by default in `PretrainedConfig` ([here](https://github.com/huggingface/transformers/blob/420f6c5ee3fb15a683bdbaf771f751edb85f1c19/src/transformers/configuration_utils.py#L285)), which means that it is promoting LONGER sequences on beam-based generation.

This is something that we should keep in mind -- we might want to change it to `0.0`, for a more neutral default.

Merging to cherry-pick it on the release branch

Thanks for the fix @gante - I think it's quite common to use a length penalty for beam search. So not sure if it's worth switching here to "no-length-penalty" by default

@patrickvonplaten

> I believe it is quite common to employ a length penalty in beam search. Hence, I am unsure if it would be worth switching to the "no-length-penalty" option by default.

Apart from the discrepancy in terminology where "length penalty" actually refers to "length reward," let's consider the default value. In the context of beam search, the concept of length penalty implies a preference for shorter sequences. However, the current implementation seems to encourage generating longer sequences by default `(1.0)`, as you mentioned. This contradicts the common practice of using a length penalty in beam search to prioritize shorter sequences.

Wouldn't it be more logical to set the default value to `-1` instead? This adjustment would signify that, by default, we are giving priority to short-length sequences, aligning with the expected behavior of beam search's default implementation.


@zenquiorra 👋 

As Patrick wrote in another thread (that I can't find), slightly promoting longer sequences is often beneficial in practice. The probability of each token is `<=1.0`, which means the sequence score is expected to decrease quickly as more tokens are added. This implies that when the score of a particular sequence barely changes when more tokens are added, those additional tokens are likely to be important for the output. A small positive length penalty, promoting longer sequences, would capture this benefit.

Another important aspect is backward compatibility. While the benefits of this default value (and variable naming) are debatable, we have many projects and products built on top of `transformers`. Changing the API or default values must be done for a very strong reason (which is not the case here) 🤗