Bugfix: Removal of padding_idx in BartLearnedPositionalEmbedding
Author: mingruimingrui
Pull request: yes

# What does this PR do?

This PR removes the unnecessary padding_idx argument from positional embedding and instead uses the pre-determined offset.

In the event that padding_idx > 2, positional embedding at some position can be fixed to 0 instead of being learnable.

## Before submitting
- [x] This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
- [x] Did you read the [contributor guideline](https://github.com/huggingface/transformers/blob/master/CONTRIBUTING.md#start-contributing-pull-requests),
      Pull Request section?
- [ ] Was this discussed/approved via a Github issue or the [forum](https://discuss.huggingface.co/)? Please add a link
      to it if that's the case.
- [ ] Did you make sure to update the documentation with your changes? Here are the
      [documentation guidelines](https://github.com/huggingface/transformers/tree/master/docs), and
      [here are tips on formatting docstrings](https://github.com/huggingface/transformers/tree/master/docs#writing-source-documentation).
- [ ] Did you write any new necessary tests?


## Who can review?

Please help 😄 @patrickvonplaten @patil-suraj 

Comments:
Thanks for the PR @mingruimingrui !
But I'm not sure how filling the embeddings with 0 will avoid them being learnable, and `nn.Embedding` actually handles this itself, if pad index is specified then the output of the embedding layer at that index will be all zeros.

Plus BART's offest is very specific to the padding index, so I'm not sure if it's a good idea to change the padding index of BART.

@patil-suraj

Zeroing of weights at positions 0 and 1 is merely code sugar, they do not carry any implication to model behavior.
My point is that the current implementation causes some weights to be untrainable.

The issue this PR is attempting to solve is `padding_idx`.

Here's a demonstration of the effects of `padding_idx` on the training of `torch.nn.Embedding`.
```python
import torch

for padding_idx in [None, 0, 1]:
    print(f'padding_idx: {padding_idx}')

    module = torch.nn.Embedding(2, 1, padding_idx=padding_idx)
    print(f'Starting weight: {module.weight.data.tolist()}')

    x = torch.LongTensor([0, 1])
    y = torch.FloatTensor([1, 1]).reshape(2, 1)

    optimizer = torch.optim.Adam(module.parameters(), lr=1.0)
    for _ in range(10):
        optimizer.zero_grad()
        pred = module(x)
        loss = torch.sum((pred - y) ** 2)
        loss.backward()
        optimizer.step()

    print(f'Ending weight: {module.weight.data.tolist()}', end='\n\n')
```

You can expect output similar to the following.
```txt
padding_idx: None
Starting weight: [[-0.696786642074585], [1.2698755264282227]]
Ending weight: [[0.39832496643066406], [0.7758995294570923]]

padding_idx: 0
Starting weight: [[0.0], [0.7031512260437012]]
Ending weight: [[0.0], [1.2048938274383545]]

padding_idx: 1
Starting weight: [[-0.8316971659660339], [0.0]]
Ending weight: [[0.48533281683921814], [0.0]]
```

Notice the weight stays 0 at the respective `padding_idx` position.

`padding_idx` can easily be 2 or greater as this is dependent on how the user train/config their tokenizer.

Hey @mingruimingrui, 

The positional embedding for the padding token is not really relevant since padding tokens are only used in training for tokens that are discarded anyways during training/evaluation (all padding tokens by design cannot influence the attention mechanism). Could you give us an example/use case which would require your fix here?

@patrickvonplaten 

I am aware that the masks used for attention and loss computation allow model output to not get affected by the positions containing the padding token.

But positional embedding is not gathered using token id but rather sequence position.

Given padding_idx = 2, the positional embedding of the first token of every sequence will be untrainable regardless of what it is.

@patrickvonplaten try this out. This is tested on transformers 4.3.2 (latest release as time of writing)

```python
import torch
import transformers
from transformers.models.bart.modeling_bart import \
    BartLearnedPositionalEmbedding

print(f'Running script on transformers=={transformers.__version__}')

# Init positional embedding with padding_idx = 2
pe = BartLearnedPositionalEmbedding(128, 1, padding_idx=2)

# Fix input embedding to a tensor of seq_len = 5
input_ids = torch.randint(0, 32000, (4, 5))

# Print out pos_embs of input_ids
pos_embs = pe.forward(input_ids.shape)
print('Initial pos_embs:', pos_embs.tolist())

# Backprop to make positional embeddings = 1.0
optimizer = torch.optim.Adam(pe.parameters(), lr=1.0)
for _ in range(100):
    optimizer.zero_grad()

    pos_embs = pe.forward(input_ids.shape)
    target = torch.ones_like(pos_embs)
    loss = torch.sum((pos_embs - target) ** 2)

    loss.backward()
    optimizer.step()

# Print out pos_embs of input_ids after optimization
# Expectation: A tensor of arppox. 1.0
# Result: A tensor arppox. 1.0 but first embedding has a value of 0.0
pos_embs = pe.forward(input_ids.shape)
print('Initial pos_embs:', pos_embs.tolist())
```

stdout
```txt
Running script on transformers==4.3.2
Initial pos_embs: [[0.0], [-0.2676548659801483], [-0.31950631737709045], [-0.9886524081230164], [0.6115532517433167]]
Initial pos_embs: [[0.0], [1.0013480186462402], [0.9954316020011902], [1.0088629722595215], [0.9995125532150269]]
```

A major problem with the current behavior is when a user uploads a model with padding_idx >= 2, the position embedding at the padding_idx will be zeroed out.

Multi-head-attention-based encoder and decoders can be hugely affected (since the concept of the sequence is represented using this embedding).

Using an extreme but realistic example, an English to German translation model translates "hello world" to "halo welt" correctly.
However, when exported to huggingface BART, the model can produce "welt halo" instead (due to the mix-up of position).

When this happens, the root cause (this issue) can be extremely difficult to discover/debug.

Hey @mingruimingrui,

Thanks for clarifying! And yes, I agree with you now - sorry I missed your point the first time! This bug is then actually in multiple spots in the library...It would be awesome if you could fix the bug also for the following models:

- `modeling_mbart.py`: https://github.com/huggingface/transformers/blob/e94d63f6cbf5efe288e41d9840a96a5857090617/src/transformers/models/mbart/modeling_mbart.py#L117
- `modeling_blenderbot.py`: https://github.com/huggingface/transformers/blob/e94d63f6cbf5efe288e41d9840a96a5857090617/src/transformers/models/blenderbot/modeling_blenderbot.py#L115
- `modeling_blenderbot_small.py`:
https://github.com/huggingface/transformers/blob/e94d63f6cbf5efe288e41d9840a96a5857090617/src/transformers/models/blenderbot_small/modeling_blenderbot_small.py#L115
- `modleing_led.py`:
https://github.com/huggingface/transformers/blob/e94d63f6cbf5efe288e41d9840a96a5857090617/src/transformers/models/led/modeling_led.py#L115
- The cookie-cutter:
https://github.com/huggingface/transformers/blob/e94d63f6cbf5efe288e41d9840a96a5857090617/templates/adding_a_new_model/cookiecutter-template-%7B%7Bcookiecutter.modelname%7D%7D/modeling_%7B%7Bcookiecutter.lowercase_modelname%7D%7D.py#L1619

Great catch :-)

Thanks @patrickvonplaten

But before I make the changes, I thought it might be good to do something about backward compatibility when users use `XXLearnedPositionalEmbedding` directly.
I suggest that we raise a warning when `padding_idx` is not `None` so that the function interface can be kept as is.

```python
def __init__(self, num_embeddings: int, embedding_dim: int, padding_idx: Optional[int] = None):
    assert padding_idx is not None, "`padding_idx` should not be None, but of type int"
    if padding_idx is not None:
        warnings.warn(
            f'padding_idx should not be provided for {self.__class__.__name__}. '
            'An exception will be raised in future versions of transformers.'
        )

    ...
```

Let me know if this is alright.

@patrickvonplaten Btw can I also ask how to quote code chunks from repos in github comments like above?
> It is very cool and useful

> Thanks @patrickvonplaten
> 
> But before I make the changes, I thought it might be good to do something about backward compatibility when users use `XXLearnedPositionalEmbedding` directly.
> I suggest that we raise a warning when `padding_idx` is not `None` so that the function interface can be kept as is.
> 
> ```python
> def __init__(self, num_embeddings: int, embedding_dim: int, padding_idx: Optional[int] = None):
>     assert padding_idx is not None, "`padding_idx` should not be None, but of type int"
>     if padding_idx is not None:
>         warnings.warn(
>             f'padding_idx should not be provided for {self.__class__.__name__}. '
>             'An exception will be raised in future versions of transformers.'
>         )
> 
>     ...
> ```
> 
> Let me know if this is alright.

`XXLearnedPositionalEmbedding` is not available in init, *e.g.* one cannot do:

```python
from transformers import BartLearnedPositionalEmbedding
```

doesn't work, so in this case we don't have to care about backwards compatibility. As you pointed out, it is simply wrong to pass a padding_idx to the position embeddings.


@patrickvonplaten That should be all the changes that are required.

Hey @mingruimingrui,

Thanks a lot for applying the changes also to the other models - it looks very nice already :-) Could you also completely remove the `padding_idx` from the call to the `PositionalEmbeddings`, *e.g.* `BartLearnedPositionalEmbedding`

> Hey @mingruimingrui,
> 
> Thanks a lot for applying the changes also to the other models - it looks very nice already :-) Could you also completely remove the `padding_idx` from the call to the `PositionalEmbeddings`, _e.g._ `BartLearnedPositionalEmbedding`

Noted, what do you say about leaving `TFLearnedPositionalEmbeddings` as is?
This way, padding_idx wouldn't sneak its way into `kwargs`.

Anything else I've missed out on?

Looks great to me! I'll run the slow tests of the respective models just to be sure and then I think we are good to go! 

Slow tests are all passing. 

Pinging @LysandreJik @sgugger @patil-suraj for review. To be concise, this PR removes the dependency of the `LearnedPositionalEmbedding` on the `pad_token_id`. After discussion with @mingruimingrui , I think positional encodings should **not** be dependent on the padding idx and the padding idx should not get a special positional embedding. The reason is the following:
- The padding idx can be at every position so it doesn't make sense to have one positional embedding be reserved for the padding idx
- For a padded input - let's say `<pad><pad> Hello <pad> <pad>` my name, the position ids should be `0 0 0 1 1 1 2` IMO and not `pad_pos pad_pos 0 pad_pos pad_pos 1 2`
- Forcing one token to be a padding token, let's say 3 means that the positional embedding for 3 doesn't exist anymore which can then be very awkward when fine-tuning a model or training it from scratch

=> This PR fixes this behavior without any breaking changes.