Different behaviours for `tf/flax` and `pt` on `generate(max_length = len of input id)`
Author: ArthurZucker
Pull request: no
Labels: name: WIP

name: bug

### System Info

```shell
- `transformers` version: 4.20.0.dev0
- Platform: macOS-12.4-arm64-arm-64bit
- Python version: 3.9.12
- Huggingface_hub version: 0.6.0
- PyTorch version (GPU?): 1.13.0.dev20220521 (False)
- Tensorflow version (GPU?): 2.9.0 (True)
- Flax version (CPU?/GPU?/TPU?): 0.4.2 (cpu)
- Jax version: 0.3.6
- JaxLib version: 0.3.5
- Using GPU in script?: <fill in>
- Using distributed or parallel set-up in script?: <fill in>
```


### Who can help?

@patrickvonplaten @LysandreJik 

### Information

- [ ] The official example scripts
- [X] My own modified scripts

### Tasks

- [ ] An officially supported task in the `examples` folder (such as GLUE/SQuAD, ...)
- [ ] My own task or dataset (give details below)

### Reproduction

Simply try the `generate` function in the pytorch model and the the `tf` model. Stumbled up upon that issue when I was working on the OPT model tests. In pytorch, even if the `max_length` argument is smaller than the length of the input sequence, a token is still generated. 

The following example using GPT2 is quite clear. The source of the bug is from [generation_utils](https://github.com/huggingface/transformers/blob/56b35ce3ebeb1edb53ef98b3ad3557f79ce788e2/src/transformers/generation_utils.py#L1217)  which only throws a warning in `pytorch` while throwing an error in both Flax and TF. 

Not sure how this should be approached, but IMO we should probably adapt the `tf/flax` code to throw the same warning (if it generates a single token like in pytorch). 

```python
>>> from transformers import GPT2LMHeadModel, TFGPT2LMHeadModel, GPT2Tokenizer
>>> pt_model = GPT2LMHeadModel.from_pretrained('gpt2')
>>> tf_model = TFGPT2LMHeadModel.from_pretrained('gpt2')
>>> tokenizer = GPT2Tokenizer.from_pretrained('gpt2')
>>> text = "Today is a beautiful day and I want to thank"
>>> pt_input_ids = tokenizer(text,return_tensors = 'pt').input_ids
>>> tf_input_ids = tokenizer(text,return_tensors = 'tf').input_ids
>>> pt_output = model.generate(pt_input_ids,max_length = 10)
Setting `pad_token_id` to `eos_token_id`:50256 for open-end generation.
Input length of input_ids is 10, but ``max_length`` is set to 10. This can lead to unexpected behavior. You should consider increasing ``config.max_length`` or ``max_length``.
>>> tokenizer.batch_decode(pt_output,skip_special_tokens = True)
["Today is a beautiful day and I want to thank everyone"]
>>> tf_output = model.generate(tf_input_ids,max_length = 10)
Setting `pad_token_id` to `eos_token_id`:50256 for open-end generation.
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "transformers/src/transformers/generation_tf_utils.py", line 569, in generate
    return self._generate(
  File "transformers/src/transformers/generation_tf_utils.py", line 1543, in _generate
    raise ValueError(
ValueError: The context has 10 number of tokens, but `max_length` is only 10. Please make sure that `max_length` is bigger than the number of tokens, by setting either `generate(max_length=...,...)` or `config.max_length = ...`
```


### Expected behavior

```shell
Input length of input_ids is 10, but ``max_length`` is set to 10. This can lead to unexpected behavior. You should consider increasing ``config.max_length`` or ``max_length``.
```


Comments:
@ArthurZucker small tip, it's nicer to show code + error message as follows:

```py
>>> from transformers import GPT2LMHeadModel, TFGPT2LMHeadModel, GPT2Tokenizer

>>> pt_model = GPT2LMHeadModel.from_pretrained('gpt2')
>>> tf_model = TFGPT2LMHeadModel.from_pretrained('gpt2')

>>> tokenizer = GPT2Tokenizer.from_pretrained('gpt2')

>>> text = "Today is a beautiful day and I want to thank"

>>> pt_input_ids = tokenizer(text,return_tensors = 'pt').input_ids
>>> tf_input_ids = tokenizer(text,return_tensors = 'tf').input_ids

>>> pt_output = model.generate(pt_input_ids,max_length = 10)
```
```
Setting `pad_token_id` to `eos_token_id`:50256 for open-end generation.
Input length of input_ids is 10, but ``max_length`` is set to 10. This can lead to unexpected behavior. You should consider increasing ``config.max_length`` or ``max_length``.
```
```py
>>> tokenizer.batch_decode(pt_output,skip_special_tokens = True)
```
```
["Today is a beautiful day and I want to thank everyone"]
```
```py
>>> tf_output = model.generate(tf_input_ids,max_length = 10)
```
```
Setting `pad_token_id` to `eos_token_id`:50256 for open-end generation.
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "transformers/src/transformers/generation_tf_utils.py", line 569, in generate
    return self._generate(
  File "transformers/src/transformers/generation_tf_utils.py", line 1543, in _generate
    raise ValueError(
ValueError: The context has 10 number of tokens, but `max_length` is only 10. Please make sure that `max_length` is bigger than the number of tokens, by setting either `generate(max_length=...,...)` or `config.max_length = ...`
```


cc @patil-suraj @gante @Narsil 

Think we should discuss on how to correct this. Currently we see a different behavior between PyTorch and Tensorflow.

@patil-suraj @gante any ideas on how to go about it (without breaking backwards comp?)

Hi there! [Tensorflow](https://github.com/huggingface/transformers/blob/main/src/transformers/generation_tf_utils.py#L1980) (and [FLAX](https://github.com/huggingface/transformers/blob/main/src/transformers/generation_flax_utils.py#L439)) have to pre-allocate the output arrays with length=`max_length`, to be compatible with XLA. That implies that even if the exception above didn't exist, it would fail when writing into these arrays if we allow it to enter the generation loop.

I see three options:
1. Do nothing: annoying because of the different outputs;
2. Upgrade the severity of PT from a warning to an exception: annoying because API/pipeline users might start getting exceptions where things were running before;
3. On the three frameworks, return the first `max_length` tokens when the input is longer than `max_length`: not fully backward compatible, but probably the most correct exception-free behavior, as the request is for a output with length=`max_length`.

WDYT?

Think we can follow 2. and go from warning to deprecation warning stating that it'll lead to an error in future versions (it is indeed a weird PT behavior). @patil-suraj what do you think?

> Upgrade the severity of PT from a warning to an exception: annoying because API/pipeline users might start getting exceptions where things were running before;

`max_length` is impossible to use for pipelines users because there is no way they can know how many tokens are being used by their `string`. The option that's  actually controllable is `max_new_tokens` since it means the same thing in both the case of `encoder-decoder` and `decoder-only`, AND it means something similar for all models (ByT5 does require more new tokens than GPT2 for same string length but at least it does not depend on the string users send).

Maybe `pipeline` could absorb the cost if it makes more sense (personally I think `max_length` is always hard to deal with in `generate` but it has been here for a long time, so probably not going away It's just that `max_length = max_new_tokens_length + input_ids.shape[0] if decoder_only else max_length = max_new_tokens + decoder_start_ids`  and that knowledge is not trivial to understand)

For instance `gpt2` has max_length =50 which is quite small compared to its `512` capacity: https://huggingface.co/gpt2/blob/main/config.json. So enforcing an error is likely to trigger issues (making `max_length=512`  is not ideal either tbh).


tldr; I would like to propose option 4:

4- Move away from `max_length` and towards `max_new_tokens` (`max_length` can take precendence because of BC). That makes arguments orthogonal in terms of catching exceptions. `max_new_tokens<=0` raises exception regardless of input. Allocation can still be done correctly  See `max_length` calculation from `input_ids` or `deocder_input_ids`.
Basically we discard the entire class of error since now the arguments don't depend on each other like `max_length` and `input_ids` do. We can still raise an warning if `max_new_tokens + input_ids.shape[0] > max_model_length` and simply truncate the command.

Thanks a lot for all the important background info here @Narsil ! 

Think it'll be impossible to replace `max_length` with `max_new_tokens` and change the `max_length` default (it's been 20 since a long time in `configuration_utils.py` and changing any of this behavior would be a massive  backward breaking behavior). 

However, I think what we could do is to give `max_new_tokens` priority over `max_length` if it's passed. Maybe we could then add a safeguard in pipelines that checks if `input_ids.shape[0] >= max_length` then `max_new_tokens = input_ids.shape[0] + 1` is passed with a warning and then `max_new_tokens` is being given priority over `max_length` (maybe we need to do this also slowly with warning and then change).

However, I feel like this should be handled by the pipelines. What do you think @Narsil ?

I very much don't like how it's currently done in PyTorch's `generate`, which silently adds +1 to `max_length` instead of throwing an error -> it should really throw an error IMO. So I'd like to escalate this to a nice error message sometime soon (the latest in v5) (option 2. of @gante)

What do you think @gante @Narsil @patil-suraj 

> However, I feel like this should be handled by the pipelines. What do you think @Narsil ?

As I said, yes `pipeline` can very much swallow the difference. It's really important for pipeline because users don't even have access to their current length. 

> Think it'll be impossible to replace max_length with max_new_tokens and change the max_length default (it's been 20 since a long time in configuration_utils.py and changing any of this behavior would be a massive backward breaking behavior).

I know, we can't break BC, I just want to emphasize that some pipeline usage would break (and possibly `generate` usage too) instead of working if we upgrade that to a hard error.

I was merely trying to point out another option which was trying to avoid hard errors at all.

If you're OK with moving on hard error, I will make the necessary adjustements on `pipeline` (which might be already done actually I don't remember)

It's an important error, maybe let's jump on a quick offline call for it. @patil-suraj @gante - also curious to hear your thoughts on it here

> However, I think what we could do is to give `max_new_tokens` priority over `max_length` if it's passed.

I like this idea! There would be no change for existing users, and new users could benefit from a clearer argument. However, we should be aware that `max_new_tokens` can be sneaky and result in more tokens than what the model can handle if `input_ids` is large enough -- we should build proper exceptions from the start.

As for the original issue (`input_ids.shape[0] >= max_length`), we can start with a warning and then move it into an exception, pointing at the `max_new_tokens` argument.

Just had a call with @Narsil . 

We agreed on the following:

- 1. TF, Flax, PT should have the same behavior regarding `max_length` and `max_new_tokens`
- 2. `max_new_tokens` should be favored over `max_length` meaning that if both are provided then `max_new_tokens` should be used
- 3. PT should escalate the warning when `input_ids.shape[0] >= max_length` to a `"This will lead to an error in a future version"` warning
- 4. generation pipelines should absorb the case when `input_ids.shape[0]` >= max_length` by just passing `max

What do you think @gante @patil-suraj - good for you?

This leads to a couple of PRs we should open:

1. @gante could you maybe add `max_new_tokens` logic to Flax and TF generate?
2. Escalate the warning in PT and add a deprecation warning -> @gante could you maybe also open a PR for this?
3. I could take care of changing the docs to advertise `max_new_tokens` instead of `max_length`.
4. @Narsil could you maybe make sure that the PT pipelines correctly absorb the use case when `input_ids.shape[0] >= max_length`?

Would this work for you?

> @Narsil could you maybe make sure that the PT pipelines correctly absorb the use case when input_ids.shape[0] >= max_length?

Yes !

Sounds good 👍 

Cool! I'll take care of the docs then :-)

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/main/CONTRIBUTING.md) are likely to be ignored.

Hi, is this issue still exists guys? 

With the recent merge of #18018, it should not exist anymore. 😉 

Let's close the issue then,