Make correct padding for text generation with GPT-NEO
Author: junoriosity
Pull request: no

### System Info

- `transformers` version: 4.28.1
- Platform: macOS-13.2.1-x86_64-i386-64bit
- Python version: 3.10.8
- Huggingface_hub version: 0.13.4
- Safetensors version: not installed
- PyTorch version (GPU?): 2.0.0 (False)
- Tensorflow version (GPU?): not installed (NA)
- Flax version (CPU?/GPU?/TPU?): not installed (NA)
- Jax version: not installed
- JaxLib version: not installed
- Using GPU in script?: no
- Using distributed or parallel set-up in script?: no

### Who can help?

@ArthurZucker @younesbelkada @gante

### Information

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

### Tasks

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

### Reproduction

In order to make generate text sequences with `GPT-NEO`, I first load all the relevant components for sequence generation for `GPTNeoForCausalLM`.

```
from transformers import AutoTokenizer, GPTNeoForCausalLM
import torch
from torch.nn import functional as F


tokenizer = AutoTokenizer.from_pretrained("EleutherAI/gpt-neo-125m")
model = GPTNeoForCausalLM.from_pretrained("EleutherAI/gpt-neo-125m")
```

There are two ways how I can generate `input_ids` and `attention_mask`.

1. I take the standard approach without padding
```
inputs = tokenizer("Hello, my dog is cute", return_tensors="pt")
``` 

2. I use padding instead
```
tokenizer.pad_token = tokenizer.eos_token
tokenizer.padding_side = 'left'
tokenizer.truncation_side = 'left'
no_items_for_history = 30

inputs = tokenizer.encode_plus("Hello, my dog is cute", max_length=no_items_for_history, padding='max_length', truncation=True, return_tensors="pt")
```

Then for both approaches, I iteratively loop through everything in order generate the sequence on token at a time.

```
input_ids = inputs['input_ids']
attention_mask = inputs['attention_mask']


for i in range(10):
    if i == 0:
        outputs = model(input_ids=input_ids, attention_mask=attention_mask, labels=inputs["input_ids"])
    else:
        outputs = model(input_ids=new_input_ids, attention_mask=attention_mask, past_key_values=past_key_values)
    loss = outputs.loss
    logits = outputs.logits[:, -1, :]

    logits = F.softmax(logits, dim=1)

    topk_values, topk_indices = torch.topk(logits, 5)
    inputs_in_topk = torch.multinomial(topk_values, num_samples=1, replacement=True)
    new_input_ids = torch.gather(topk_indices, 1, inputs_in_topk)

    past_key_values = outputs.past_key_values
    attention_mask = torch.concat((attention_mask, torch.ones(1, 1).to(attention_mask.device)), dim=1)
    input_ids = torch.concat((input_ids, new_input_ids), dim=1)


print(tokenizer.decode(input_ids.tolist()[0], skip_special_tokens=True))
```

### Expected behavior


**Here is the problem:**

The starting `input_ids` and `attention_mask` for the first approach look like:
```
input_ids = tensor([[15496,    11,   616,  3290,   318, 13779]])
attention_mask = tensor([[1, 1, 1, 1, 1, 1]])
```


The output looks very sensible:
```
Hello, my dog is cute! This post is about dogs and cats
```

However, for the second approach the starting `input_ids` and `attention_mask` look like

```
input_ids = tensor([[50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256, 15496,    11,   616,  3290,   318, 13779]])
attention_mask = tensor([[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 1, 1, 1, 1, 1]])
```

and it always generates nonsense like

```
Hello, my dog is cute pet is my pet pet pet is my dog is
```

**Question:** Do you know how to make it work with padding, i.e., the second approach?


Comments:
@mzamini92 Many thanks for getting back to me.

I know that the padding tokens should be ignored, when doing the generation. (However, it will be important for batch processing if there are multiple inputs).

What is strange is that, if I follow the approach with different models the output makes sense for both approaches, yet here the second approach is not working for gpt-neo-125m.

Hey @junoriosity 👋 

> if I follow the approach with different models the output makes sense for both approaches, yet here the second approach is not working for gpt-neo-125m.

Masking is not a perfect operation, as it adds a very large negative number to the attention scores. While the impact of masked tokens is very very small, it still exists. In some cases, it may change an output token at generation time, which may derail (or improve!) the generation process.

Have a go with other prompts and other model sizes for `GPTNeo`. Unless this phenomenon consistently happens, I'd say there is nothing to worry about :)

Hey @gante 

I tried the second approach with `EleutherAI/gpt-neo-1.3B` and got

```
Hello, my dog is cute dog is cute is cute is cute is cute is
```

so no improvement ...

@junoriosity we would need a much larger sample size to conclude it is not working correctly. And we can only afford to look deeper into the issue after we confirm that it is indeed an issue :)

Hi @junoriosity 
Is there any reason to force the padding_side to be `left` ? removing the lines 

```python
tokenizer.padding_side = 'left'
tokenizer.truncation_side = 'left'
```

Leads to "better" output (the default `padding_side` is `right` for that model):
```bash
>>> Hello, my dog is cute a little dog. He is so cute cute cute
>>> Hello, my dog is cuteh! She, and I have been in a
```

Maybe there is something wrong in the way we compute the position ids. Consider the case where `padding_side=left` and assume your text has 10 tokens and you want to add padding tokens on the first 20 tokens. 

Currently:

```python
        if position_ids is None:
            position_ids = torch.arange(past_length, input_shape[-1] + past_length, dtype=torch.long, device=device)
            position_ids = position_ids.unsqueeze(0).view(-1, input_shape[-1])
```

The position ids will be computed as such regardless the index of the first non-padding token. Maybe this is the culprit @gante - similarly as https://github.com/huggingface/transformers/pull/22382

Hi @younesbelkada 

even then there are repetitions etc. 

I tried the same feat with the smallest OPT-125m model and it worked like charm, also for others. The only model that does cause me trouble with that approach is gpt-neo. 

I use this padding-right strategy to align a batch of sequences to the right. I thought this makes most sense. So far at least it works quite nicely for all other models.

@gante 1.3 B is already the second largest, the largest being 2.7 B. Hence, there is no way to get much beyond that and I also doubt that just doubling the size will change much.

@younesbelkada oh definitely, the position ids should be computed from the attention mask in `prepare_inputs_for_generation` (like in the PR you linked)! That could be the cause for the mismatches

@gante @younesbelkada Okay, since I here from these things for the first time, could you

- tell me what it means
- how I could use it to solve the issue?

@junoriosity it appears there is no issue at all at the end. GPTNeo seems to already support the creation of correct position ids [here](https://github.com/huggingface/transformers/blob/main/src/transformers/models/gpt_neo/modeling_gpt_neo.py#L682-L700)

if you modify your script as follows:

```python
from transformers import AutoTokenizer, GPTNeoForCausalLM
import torch
from torch.nn import functional as F


tokenizer = AutoTokenizer.from_pretrained("EleutherAI/gpt-neo-125m")
model = GPTNeoForCausalLM.from_pretrained("EleutherAI/gpt-neo-125m")

inputs = tokenizer("Hello, my dog is cute", return_tensors="pt")

tokenizer.pad_token = tokenizer.eos_token
tokenizer.padding_side = 'left'
tokenizer.truncation_side = 'left'
no_items_for_history = 30

inputs = tokenizer.encode_plus("Hello, my dog is cute", max_length=no_items_for_history, padding='max_length', truncation=True, return_tensors="pt")

input_ids = inputs['input_ids']
attention_mask = inputs['attention_mask']

outputs = model.generate(input_ids=input_ids, attention_mask=attention_mask, max_new_tokens=40)
print(tokenizer.decode(outputs[0], skip_special_tokens=True))
>>> Hello, my dog is cute and I'm going to give you some tips on how to get your dog to sleep.

I'm going to give you some tips on how to get your dog to sleep.
```
Now in your case you need to properly call `prepare_inputs_for_generation` as @gante suggested to create the correct position ids and pass it to your model during forward pass. Let me get back to you with the updated script and explanation

@younesbelkada You are right, that this is a very elegant solution. :)

However, I would like this "step-by-step" solution to extract some information about the state of the generation.

Hence, is there any possibilty to do it that way? Again, for other models like OPT-125m it was no problem as well.

Hi @junoriosity 

Going back to the explanation; let's first try to understand what is the purpose of `position_ids`. These ids indicates the model the positional information of the input tokens. This information is extremely important for the model to capture the positional information of the input tokens. Here: https://github.com/huggingface/transformers/blob/abaca9f9432a84cfaa95531de4c72334f38a42f2/src/transformers/models/gpt_neo/modeling_gpt_neo.py#L582 the model extracts the so called "positional embeddings" that are added later on together with the input embeddings, to produce the first hidden states here: https://github.com/huggingface/transformers/blob/abaca9f9432a84cfaa95531de4c72334f38a42f2/src/transformers/models/gpt_neo/modeling_gpt_neo.py#L583

As you can see above, if no `position_ids` is passed to the model, it will create a new one: https://github.com/huggingface/transformers/blob/abaca9f9432a84cfaa95531de4c72334f38a42f2/src/transformers/models/gpt_neo/modeling_gpt_neo.py#L551 with the `torch.arange(xxx)` method. 

Blindly creating position ids like that can lead to silent bugs (as described on your issue) - for a classic input (no padding involved) there is no problem at all using `torch.arange(xxx)` as there is no special token we want the model to ignore during its forward pass.

Now assume your input is (consider `[PAD]` as the padding token produced by the tokenizer):
```python
"[PAD] [PAD] Hello my name is"
``` 

Therefore the (dummy) input ids would look like (assuming `0` is the pad token id):

```python
[ 0 , 0, 45, 32, 2, 86, ..]
```

If the position_ids are blindly created, it will result in the following :

```python
torch.Tensor([0, 1, 2, 3, 4, 5])
```

this is not correct and leads to wrong computation, the attention mask will ignore the first two tokens, however, the first non-padding token will have a positional ID of `2`, which in fact should be 0 - therefore one always needs to calculate separately the position ids before each generation step to handle corner cases such as the one your are facing.

`.generate()` API does everything for you under the hood. Before each forward pass, it calls a method called `prepare_inputs_for_generation` that ideally handles all these scenarios: https://github.com/huggingface/transformers/blob/abaca9f9432a84cfaa95531de4c72334f38a42f2/src/transformers/generation/utils.py#L2359 including correctly shifting the position ids if any.

Going back to your case, the fix is to prepare the model's input before the generation step 1, then at each generation step iteratively call `model.prepare_inputs_for_generation()` with the correct arguments and correctly pass the produced `position_ids`

Changing the script to the one below:

<details><summary>Working script</summary>

```python
from transformers import AutoTokenizer, GPTNeoForCausalLM
import torch
from torch.nn import functional as F


tokenizer = AutoTokenizer.from_pretrained("EleutherAI/gpt-neo-125m")
model = GPTNeoForCausalLM.from_pretrained("EleutherAI/gpt-neo-125m")

inputs = tokenizer("Hello, my dog is cute", return_tensors="pt")

tokenizer.pad_token = tokenizer.eos_token
tokenizer.padding_side = 'left'
tokenizer.truncation_side = 'left'
no_items_for_history = 30

inputs = tokenizer.encode_plus("Hello, my dog is cute", max_length=no_items_for_history, padding='max_length', truncation=True, return_tensors="pt")

input_ids = inputs['input_ids']
attention_mask = inputs['attention_mask']

position_ids = model.prepare_inputs_for_generation(input_ids, attention_mask=attention_mask, past_key_values=None, position_ids=None)["position_ids"]

for i in range(50):
    if i == 0:
        outputs = model(input_ids=input_ids, attention_mask=attention_mask, position_ids=position_ids)
        past_key_values = None
    else:
        outputs = model(**next_stage_input)
    loss = outputs.loss
    logits = outputs.logits[:, -1, :]

    logits = F.softmax(logits, dim=1)

    topk_values, topk_indices = torch.topk(logits, 5)
    inputs_in_topk = torch.multinomial(topk_values, num_samples=1, replacement=True)
    new_input_ids = torch.gather(topk_indices, 1, inputs_in_topk)

    past_key_values = outputs.past_key_values
    attention_mask = torch.concat((attention_mask, torch.ones(1, 1).to(attention_mask.device)), dim=1)
    input_ids = torch.concat((input_ids, new_input_ids), dim=1)

    next_stage_input = model.prepare_inputs_for_generation(input_ids, attention_mask=attention_mask, past_key_values=None, position_ids=None)

print(tokenizer.decode(input_ids.tolist()[0], skip_special_tokens=True))
```
</details>

seems to produce correct output. Let us know with @gante if you have more questions

The reason it works correctly for OPT is that the positional embeddings are computed directly using the attention mask, which indicates where is the first non-padding token: https://github.com/huggingface/transformers/blob/abaca9f9432a84cfaa95531de4c72334f38a42f2/src/transformers/models/opt/modeling_opt.py#L653

Also make sure to use the latest version of `transformers`:

```bash
pip install --upgrade transformers
```

@younesbelkada @gante Wow, this makes things a lot better by now. 🤗

However, please correct me if I am wrong, but we do not use `past_key_values`, which will force us to do an enormous amount of calculation again and again.

I tried some things to make use of it, but I did not succeed. Do you have an idea how to make the above code work while using `past_key_values` for speeding up the code?

@younesbelkada @gante I found how to solve it, you have to enter `position_id` into model, but this becomes 
```
position_ids = position_ids[:, -1:] + 1
```
Then things work like a charm.

In any case, many thanks for all your support. Without your effort, this progress wouldn't have been possible. 🤗

That's great to hear ! Thanks very much @junoriosity 

Also I believe we should support this, same way as it was done [here](https://github.com/raghavanone/transformers/commit/3d42b725fe357d45fe4f745e1bf700a09f06c1cc). I'll open a PR for both as the changes were reverted because TF version were not updated! I'll take care of it 😉 

@ArthurZucker That is awesome. Just out of curiosity: How long do you think it will take until a new `transformer` version with the changes is realeased? :)

Oups as @younes mentioned, the automatic creation of position ids seems to be correct for GPTNeo (not for GPT2). 
TLDR; position ids should be created and correctly support past key values and use of use_cache. If this is not, the case then should fix it! 

@ArthurZucker Okay, could you perhaps outline with an example how you mean it? I am a bit lost due to my lack of experience for the specific requirements.

Hi @ArthurZucker could you perhaps get back to me on that matter? 🤗

Personally, I would appreciate it a lot if I could handle GPTNeo just like the OPT models, as this would facilitate my life a lot.

Hey! really sorry had a bit of a sprint this week! Will get back to you soon 😃 

Hey @junoriosity, what I meant is that in this case, the problem does not seem to be from transformers:

https://github.com/huggingface/transformers/blob/f092997ca669750d4f32ada127b2624bd450aee5/src/transformers/models/gpt_neo/modeling_gpt_neo.py#L682-L707 


In the above snippet, we can see that the attention mask is correctly taken into account to create the positional ids. I just needed to check whether this was the case or not.

Hope this answers your final questions! 

@ArthurZucker So long story short:

I can proceed without position_ids for `OPT`, but have to implement it for `GPT-NEO`.

Is that a correct summary?

If what you are doing is:

> Then for both approaches, I iteratively loop through everything in order generate the sequence on token at a time.

then you just need to make sure your code is adapted yes. 
The other solution is for us to include the correct position id creation in the Model class instead of when `prepare_inputs_for_generation`. This might be better, wdyt @gante (gpt2 also needs this as it was reverted since tf does not use it) 

@junoriosity @ArthurZucker I'd favor adding it in `prepare_inputs_for_generation`. 

(Adding it in the model class is more elegant, but most models do it in `prepare_inputs_for_generation`. Keeping the same structure makes maintenance easier :) )

@gante Terrific, I am always a bit impatient, but is there a realistic time range until when this would be included in the library? 🤗

@junoriosity our bandwidth to retroactively add features/fix bugs runs short for the foreseeable months. My suggestion would be to have a go at it and open a PR :) 

Sure, but for training could this not be a problem? 😉 

cc @gante WDYT about making sure this is supported when calling a `model` this way training correctly computes `positions_ids`? Otherwise they are just wrong and we don't raise anything -> silent bug. 

The catch is that in some common cases, we can't infer the position IDs at train time -- e.g. if we concatenate multiple rows to maximize utilization (as in [this example](https://huggingface.co/docs/transformers/tasks/language_modeling#preprocess)). 

IMO, other than automating the inference-time case for ease of use, I don't see how we can do it reliably 😞 

@ArthurZucker @gante I have encountered a new issue and I do not know how so solve it, so perhaps you can help with your in-depth knowledge. 🤗

When I use the following `inputs_ids` 

```
tensor([[50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256,
         50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256,
         50256, 50256, 50256,  1532,   356,  1949],
        [50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256,
         50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256,
         50256, 50256, 50256, 50256, 50256,  1212]])
```

and `attention_mask` 

```
tensor([[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 1, 1],
        [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1]])
```

I can use as `position_ids` 

```
tensor([[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 2],
        [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]])
```

and then have success with the iterated approach as outlined above, i.e.,

```
model(input_ids=input_ids, attention_mask=attention_mask, position_ids=position_ids, past_key_values=None)
```

then I can use the iteration as outlined above with the `past_key_values` from the model. The result is sensible output like


```
[1] to get into the new programming language of the past, we have a lot of
[2] is a new idea for the idea for the idea about the idea about the idea
```

However, if I crop it further (and such things might happen in our use case), then the different variables look as follows:

`inputs_ids` 

```
tensor([[50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256,
         50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256,
         50256, 50256, 50256,  1532,   356],
        [50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256,
         50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256, 50256,
         50256, 50256, 50256, 50256, 50256]])
```

and `attention_mask` 

```
tensor([[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 1],
        [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]])
```

and `position_ids` 

```
tensor([[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1],
        [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]])
```

If I use the iterated approach as outlined above, i.e.,

```
model(input_ids=input_ids, attention_mask=attention_mask, position_ids=position_ids, past_key_values=None)
```

we can use `past_key_values` from the model for each new round, exactly analogous to the other input. However, the result is not a sensible output and looks like


```
[1] try to get into the new programming language, we’ve got to make sure
[2] This is a new idea for the idea idea idea idea idea idea idea idea idea idea
```

Hence, the first one is still working, but for the second, where the `attention_mask` and  `position_ids` were both completely 0, it just produces nothing sensible anymore.

Do you have an idea how I could still make it work? 🤗



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.