GPT-neo generate is ignoring passed position ids
Author: SwordShieldMouse
Pull request: no
Labels: name: WIP

name: bug

### System Info

```shell
python version: 3.9
transformers version: 4.18
```


### Who can help?

@patil-suraj @patrickvonplaten 

### 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

``` 

from transformers import AutoTokenizer, AutoModelForCausalLM
import torch 

text = "hi there: "
modelname = "EleutherAI/gpt-neo-125M"
model = AutoModelForCausalLM.from_pretrained(modelname)
tokenizer = AutoTokenizer.from_pretrained(modelname)

inputs = tokenizer(text, return_tensors ="pt")
inputs["position_ids"] = inputs["attention_mask"].cumsum(-1)
print(inputs)
a = model.generate(**inputs)


inputs["position_ids"] = inputs["position_ids"] + 10
print(inputs)
b = model.generate(**inputs)

# a and b should be different because the position ids are different
print(a)
print(b)
```
the result
```
a = tensor([[5303,  612,   25,  220,  220,  220,  220,  220,  220,  220,  220,  220,
          220,  220,  220,  220,  220,  220,  220,  220]])
b = tensor([[5303,  612,   25,  220,  220,  220,  220,  220,  220,  220,  220,  220,
          220,  220,  220,  220,  220,  220,  220,  220]])
```

### Expected behavior

```shell
The outputs a and b should (almost always) be different because different position ids should be passed to the model's forward function, resulting in different activations. The issue seems to be here: https://github.com/huggingface/transformers/blob/ee393c009a243bbb86fa11d2efa771a1704d85cf/src/transformers/models/gpt_neo/modeling_gpt_neo.py#L699

Specifically, the else statement is such that if both an attention mask and position ids are passed, the position ids are erased. In such a scenario, the default position ids in the model's forward function (https://github.com/huggingface/transformers/blob/ee393c009a243bbb86fa11d2efa771a1704d85cf/src/transformers/models/gpt_neo/modeling_gpt_neo.py#L537) are used, rather than the passed-in position ids. 

Proposed fix: remove the `else` block on line 699 and unindent the `if past` block.
```


Comments:
Hey @SwordShieldMouse,

note that `position_ids` does not have a huge influence on the output. It is not very surprising to me that the generated output ids (that we generated by taking the `argmax(...)` logits) are the same. It essentially just means that different positions ids still lead to the model outputting the same logit as the highest logit. It doesn't mean that the logits are the same. E.g. if you execute the following code, you see that the logit outputs `a` and `b` differ.

```python
from transformers import AutoTokenizer, AutoModelForCausalLM
import torch 

text = "hi there: "
modelname = "EleutherAI/gpt-neo-125M"
model = AutoModelForCausalLM.from_pretrained(modelname)
tokenizer = AutoTokenizer.from_pretrained(modelname)

inputs = tokenizer(text, return_tensors ="pt")
inputs["position_ids"] = inputs["attention_mask"].cumsum(-1)
print(inputs)
a = model(**inputs).logits


inputs["position_ids"] = inputs["position_ids"] + 10
print(inputs)
b = model(**inputs).logits

print("a", a.abs().sum())
print("b", b.abs().sum())
```

Yes, I agree that that the logits are different in your example because the model `forward` indeed takes in the position ids. My point is that when the model `forward` is called through the `generate` function, the position IDs are not passed. Please [see here](https://github.com/huggingface/transformers/blob/ee393c009a243bbb86fa11d2efa771a1704d85cf/src/transformers/models/gpt_neo/modeling_gpt_neo.py#L693). If both an attn mask and position ids are passed, the position ids are set to `None`. The resulting dict, with `None` position ids, is passed to the model forward [here](https://github.com/huggingface/transformers/blob/4710702837a9262e730b798a30c0609e322d02ed/src/transformers/generation_utils.py#L1675). 

It's true that one shouldn't expect `generate` to output different results all the time. However, for a paper I'm working on atm that involves modifying position ids, and whose code would have been too long to put here, changing the position ids results in the same generation 100% of the time for every sample. It's only when I remove the `else` block on line 699 and unindent the `if past` block that I get different results for different position ids, for every sample.

@patrickvonplaten Here is a modified snippet with my proposed change to the code, which indeed gives different generation results.
```
from transformers import AutoTokenizer, AutoModelForCausalLM
import torch 

modelname = "EleutherAI/gpt-neo-125M"
model = AutoModelForCausalLM.from_pretrained(modelname)
tokenizer = AutoTokenizer.from_pretrained(modelname)

def prepare_inputs_for_generation(input_ids, past=None, **kwargs):
    token_type_ids = kwargs.get("token_type_ids", None)
    # only last token for inputs_ids if past is defined in kwargs
    if past:
        input_ids = input_ids[:, -1].unsqueeze(-1)
        if token_type_ids is not None:
            token_type_ids = token_type_ids[:, -1].unsqueeze(-1)

    attention_mask = kwargs.get("attention_mask", None)
    position_ids = kwargs.get("position_ids", None)

    if attention_mask is not None and position_ids is None:
        # create position_ids on the fly for batch generation
        position_ids = attention_mask.long().cumsum(-1) - 1
        position_ids.masked_fill_(attention_mask == 0, 1)
    if past:
        position_ids = position_ids[:, -1].unsqueeze(-1)
    # else:
    #     position_ids = None
    return {
        "input_ids": input_ids,
        "past_key_values": past,
        "use_cache": kwargs.get("use_cache"),
        "position_ids": position_ids,
        "attention_mask": attention_mask,
        "token_type_ids": token_type_ids,
    }

texts = [
    "what is your name?",
    "2 + 2 = ",
    "what is the capital of france?",
    "astnhoeruchau2918uh93u",
    "how many humans are there in the world?",
    "my favourite colour is ",
    "hi there: "
]
res = []

for text in texts:
    inputs = tokenizer(text, return_tensors ="pt")
    inputs["position_ids"] = inputs["attention_mask"].cumsum(-1)
    # print(inputs)
    a = model.generate(**inputs)

    inputs["position_ids"] = inputs["position_ids"] + 10
    # print(inputs)
    b = model.generate(**inputs)

    res.append((a == b).all())

# implement the fix
model.prepare_inputs_for_generation = prepare_inputs_for_generation

fixed_res = []
for text in texts:
    inputs = tokenizer(text, return_tensors ="pt")
    inputs["position_ids"] = inputs["attention_mask"].cumsum(-1)
    # print(inputs)
    a = model.generate(**inputs)

    inputs["position_ids"] = inputs["position_ids"] + 10
    # print(inputs)
    b = model.generate(**inputs)

    fixed_res.append((a == b).all())

# these are different but should be the same!
print(res)
print(fixed_res)
```
Results
```
res = [tensor(True), tensor(True), tensor(True), tensor(True), tensor(True), tensor(True), tensor(True)]
fixed_res = [tensor(False), tensor(True), tensor(True), tensor(True), tensor(True), tensor(True), tensor(True)]
```

The difference is slight, but exists.

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.

Bump.

On Thu., Jun. 16, 2022, 11:02 a.m. github-actions[bot], <
***@***.***> wrote:

> 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.
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/huggingface/transformers/issues/17283#issuecomment-1157763956>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ACMZS2BOVYUJBONE7562IM3VPM6YBANCNFSM5WB2QOEQ>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>


Hey @SwordShieldMouse,

Sorry for answering so late! Upon having taken a second look, you're right those two lines should not exist - i.e. we should ideally delete them completely - great job spotting the bug and sorry for the misunderstanding before - I now see what is meant! 

Would you mind opening a PR to fix it? Otherwise happy to do so myself :-)

Great!

I'm busy for the rest of the week so I could do it next week, but I'm happy
if you want to get it done this week :)

On Mon, 20 Jun 2022 at 18:19, Patrick von Platen ***@***.***>
wrote:

> Hey @SwordShieldMouse <https://github.com/SwordShieldMouse>,
>
> Sorry for answering so late! Upon having taken a second look, you're right
> those two lines should not exist - i.e. we should ideally delete them
> completely - great job spotting the bug and sorry for the misunderstanding
> before - I now see what is meant!
>
> Would you mind opening a PR to fix it? Otherwise happy to do so myself :-)
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/huggingface/transformers/issues/17283#issuecomment-1160685261>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ACMZS2EZU7EKBTBCEEYD2T3VQCRZBANCNFSM5WB2QOEQ>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>


-- 
Alan C.


Awesome - happy to wait a week! More than happy though to take over the issue if you find that you won't find time the next week(s) :-)

Hi Patrick,

Sorry for getting back to you late. It turns out I won't have time after
all :( Would you be able to do the fix?

On Wed., Jun. 22, 2022, 12:12 a.m. Patrick von Platen, <
***@***.***> wrote:

> Awesome - happy to wait a week! More than happy though to take over the
> issue if you find that you won't find time the next week(s) :-)
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/huggingface/transformers/issues/17283#issuecomment-1162450396>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ACMZS2AH7OOHB6KX67EPFILVQJD4DANCNFSM5WB2QOEQ>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>


Hey @SwordShieldMouse,

Started a PR here: https://github.com/huggingface/transformers/pull/18048 - it's actually much more work then I thought so will take a while maybe (cc @gante)

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.

(being worked on)