Padding idx in modeling RoBERTa
Author: AlexUmnov
Pull request: no

There's a padding_idx for positional embeddings in RoBERTa 
https://github.com/huggingface/transformers/blob/master/src/transformers/models/roberta/modeling_roberta.py#L98
```
        self.position_embeddings = nn.Embedding(
            config.max_position_embeddings, config.hidden_size, padding_idx=self.padding_idx
        )
```
This is most definitely a mistake:
First of all, why do you need a padding index for positional embeddings
And second of all, the index of padding is taken from vocab, so if you have a padding token in the end of your vocabulary, this will fail with 
`AssertionError: Padding_idx must be within num_embeddings`

Comments:
Hi,

Not sure what you mean. If we provide a sentence like "hello world" to RoBERTa that we pad up to a certain length:

```
from transformers import AutoTokenizer

tokenizer = AutoTokenizer.from_pretrained("roberta-base")

text = "hello world"

encoding = tokenizer(text, padding="max_length", max_length=10, return_tensors="pt")
print(tokenizer.decode(encoding.input_ids.squeeze())
```
Then it will look like:
```

hello world
<pad><pad><pad><pad><pad><pad>
```
The position_ids are created internally by the model using the `create_position_ids_from_input_ids` function as seen [here](https://github.com/huggingface/transformers/blob/6ac77534bfe97c00e0127bb4fc846ae0faf1c9c5/src/transformers/models/roberta/modeling_roberta.py#L108). Let's take a closer look by checking out the position IDs created:
```
from transformers.models.roberta.modeling_roberta import create_position_ids_from_input_ids

position_ids = create_position_ids_from_input_ids(encoding.input_ids, padding_idx=tokenizer.pad_token_id)
print(position_ids)
```
This prints:
```
tensor([[2, 3, 4, 5, 1, 1, 1, 1, 1, 1]])
```
So as we can see, the position IDs of padding tokens are set to 1. This is also the `padding_idx` of the position embedding layer. The `padding_idx` is explained in the [docs](https://pytorch.org/docs/stable/generated/torch.nn.Embedding.html) of PyTorch's `nn.Embedding`:

> padding_idx (int, optional) – If specified, the entries at padding_idx do not contribute to the gradient; therefore, the embedding vector at padding_idx is not updated during training, i.e. it remains as a fixed “pad”. For a newly constructed Embedding, the embedding vector at padding_idx will default to all zeros, but can be updated to another value to be used as the padding vector.

 Hence, for the padding tokens, no position vector will be learned.


Don't we zero out gradients for paddings using masks?
And also if you are right, this still is a problem that padding token should be synchronized between word embeddings and position embeddings 
As i'm saying, if you learn a tokenizer and then add special tokens in the end, this code breaks 

My biggest concern is:
Here we use a padding index id from our word token vocab 
```
        self.padding_idx = config.pad_token_id
        self.position_embeddings = nn.Embedding(
            config.max_position_embeddings, config.hidden_size, padding_idx=self.padding_idx
        )
```
I really don't understand why it has to be this way, especially if the padding id is bigger then `max_position_embeddings`

I also guess i have to mention that i'm talking about a custom RoBERTa model

Bump
I'm still struggling with this problem :( 

Please refer to the [forum](https://discuss.huggingface.co/) for training-related questions, as we'd like to keep Github issues for bugs/feature requests.

To answer your question, if you have special tokens defined, then the position embeddings will be created properly by the `create_position_ids_from_input_ids` function. No position embedding vector will be learned for padding tokens.



Sorry if i'm being too persistent, but i'm fairly certain that it's a bug. Let me bring everything in one comment, so it will draw a full picture. I can try and make a minimal breaking example, but it's fairly complicated, to be honest. 
So imagine we have a RoBERTa tokenizer with vocab size of 30000 and maximum length of 512. We trained the tokenizer agnostic to the model, so we add necessary special tokens in the end of the vocab, so our vocab now has tokens
```
{
    "<pad>": 30000,
    "
": 30001,
    "<\s>": 30002,
    "<mask>": 30003
}
```
I might've forgot something, but it doesn't matter in this example. 

Now we initialize RobertaModel and RobertaEmbeddings inside of it
https://github.com/huggingface/transformers/blob/master/src/transformers/models/roberta/modeling_roberta.py#L70

We get padding token from our vocab and it's index is 30000
https://github.com/huggingface/transformers/blob/master/src/transformers/models/roberta/modeling_roberta.py#L97
`        self.padding_idx = config.pad_token_id`
And then we define position embeddings using this index like so:
https://github.com/huggingface/transformers/blob/master/src/transformers/models/roberta/modeling_roberta.py#L98
```
        self.position_embeddings = nn.Embedding(
            config.max_position_embeddings, config.hidden_size, padding_idx=self.padding_idx
        )
```
Which turn into this line, if we consider out parameters:
```
        self.position_embeddings = nn.Embedding(
            512, 768, padding_idx=30000
        )
```
Which is going to give us an error from torch` AssertionError: Padding_idx must be within num_embeddings` because we tried to set the index of padding token as the index of it in the word vocab. This index is out of scope for positional embeddings.



Hey @AlexUmnov, thanks for opening an issue! Jumping in here to let you know that we aim to have implementations that are as similar as possible to the original implementation.

Here, for RoBERTa, we followed Fairseq's implementation in order to obtain as close as possible the same results. I invite you to follow their implementation and see that they use the `paddind_idx` in the same manner that we do:

https://github.com/pytorch/fairseq/blob/fcca32258c8e8bcc9f9890bf4714fa2f96b6b3e1/fairseq/models/transformer/transformer_encoder.py#L67-L76
-->
https://github.com/pytorch/fairseq/blob/fcca32258c8e8bcc9f9890bf4714fa2f96b6b3e1/fairseq/modules/positional_embedding.py#L25
-->
https://github.com/pytorch/fairseq/blob/fcca32258c8e8bcc9f9890bf4714fa2f96b6b3e1/fairseq/modules/learned_positional_embedding.py#L53-L61

If you find there is an issue with this implementation, we invite you to open an issue in Fairseq's repository. Thanks for your understanding.

Hey @LysandreJik, thanks for explaining that!
Yeah, it seems to me like the original design is flawed.
But, being honest, it seems like i'd be better of just fixing my own model to fit in those weird constraints.

@AlexUmnov did you finally solve your issue? 
I came across the same error. In my case, I got the IndexError because of the function:

`
  def create_position_ids_from_input_ids(input_ids, padding_idx, past_key_values_length=0):
      mask = input_ids.ne(padding_idx).int()
      incremental_indices = (torch.cumsum(mask, dim=1).type_as(mask) + past_key_values_length) * mask
      return incremental_indices.long() + padding_idx
`

So, when you have a sequence without any padding, this function will return an array of 2:512 positions which is out of range. 

@symelaz unfortunately it was more relevant for my project to just abandon that. 

Umm, I think one easy way is to remove the padding setting for position embedding.
I believe that the padding position itself in position embedding is learned by word embedding.

Compared to the original implementation, I think the learning load of padding in position embedding will increase, but this is semantically the same as `create_position_ids_from_inputs_embeds`, and it feels easier than creating data that always contains PAD.
- `create_position_ids_from_inputs_embeds`
https://github.com/huggingface/transformers/blob/39b4aba54d349f35e2f0bd4addbe21847d037e9e/src/transformers/models/roberta/modeling_roberta.py#L139
- image of fix points (Not a little smart...)
  - Creating position embeddings module
 https://github.com/huggingface/transformers/blob/39b4aba54d349f35e2f0bd4addbe21847d037e9e/src/transformers/models/roberta/modeling_roberta.py#L95
    ```python
        self.position_embeddings = nn.Embedding(
            config.max_position_embeddings, config.hidden_size
        )
    ```
  - Creating position ids
  https://github.com/huggingface/transformers/blob/39b4aba54d349f35e2f0bd4addbe21847d037e9e/src/transformers/models/roberta/modeling_roberta.py#L1562
    ```python
    mask = input_ids.ne(padding_idx).int()
    incremental_indices = (torch.cumsum(mask, dim=1).type_as(mask) + past_key_values_length)
    return incremental_indices.long() - 1
    ```

Just wanted to point out I faced the same issue. The current design is non-sensical. It can only result in bugs. If your padding token is the token 3 instead of 1, your model can only accept 510 tokens, and your position embeddings start at 3 instead of 1.

> Hi,
> 
> Not sure what you mean. If we provide a sentence like "hello world" to RoBERTa that we pad up to a certain length:
> 
> ```
> from transformers import AutoTokenizer
> 
> tokenizer = AutoTokenizer.from_pretrained("roberta-base")
> 
> text = "hello world"
> 
> encoding = tokenizer(text, padding="max_length", max_length=10, return_tensors="pt")
> print(tokenizer.decode(encoding.input_ids.squeeze())
> ```
> 
> Then it will look like:
> 
> ```
> 
hello world
<pad><pad><pad><pad><pad><pad>
> ```
> 
> The position_ids are created internally by the model using the `create_position_ids_from_input_ids` function as seen [here](https://github.com/huggingface/transformers/blob/6ac77534bfe97c00e0127bb4fc846ae0faf1c9c5/src/transformers/models/roberta/modeling_roberta.py#L108). Let's take a closer look by checking out the position IDs created:
> 
> ```
> from transformers.models.roberta.modeling_roberta import create_position_ids_from_input_ids
> 
> position_ids = create_position_ids_from_input_ids(encoding.input_ids, padding_idx=tokenizer.pad_token_id)
> print(position_ids)
> ```
> 
> This prints:
> 
> ```
> tensor([[2, 3, 4, 5, 1, 1, 1, 1, 1, 1]])
> ```
> 
> So as we can see, the position IDs of padding tokens are set to 1. This is also the `padding_idx` of the position embedding layer. The `padding_idx` is explained in the [docs](https://pytorch.org/docs/stable/generated/torch.nn.Embedding.html) of PyTorch's `nn.Embedding`:
> 
> > padding_idx (int, optional) – If specified, the entries at padding_idx do not contribute to the gradient;
/ therefore, the embedding vector at padding_idx is not updated during training, i.e. it remains as a fixed “pad”. For a newly constructed Embedding, the embedding vector at padding_idx will default to all zeros, but can be updated to another value to be used as the padding vector.
> 
> Hence, for the padding tokens, no position vector will be learned.

I'd like to highlight that I encountered the same problem. the position IDs of padding tokens are set to 1 instead of 0. This value may either remain unchanged or undergo modifications in the future.  

@FremyCompany @Areshey Just came across this bug as well! Was causing me a huge headache (but caused me to learn a lot, so a win in the end?). Makes zero sense why it was implemented this way