Whisper Encoder's positional encodings shouldn't be trainable
Author: gau-nernst
Pull request: no

### System Info

Currently Whisper Encoder's positional encodings in HF is an `nn.Embedding`, which makes it trainable.

https://github.com/huggingface/transformers/blob/aea761499f4b1193f2706f471442da6f9df65d65/src/transformers/models/whisper/modeling_whisper.py#L837

In the original implementation, it is fixed to sinusoidal positional encodings, and assigned as a buffer, thus not trainable.

https://github.com/openai/whisper/blob/e8622f9afc4eba139bf796c210f5c01081000472/whisper/model.py#L150

This creates diverging behavior when finetuning Whisper Encoder.

On a separate note, personally I found that OpenAI's weights for Whisper Encoder positional encodings are slightly different from the ones generated by `sinusoids()` function. Thus, loading these positional encodings weights (as buffers) is still necessary to match the original repo.

### Who can help?

@sanchit-gandhi @ArthurZucker 

### Information

- [ ] The official example scripts
- [ ] 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

NA

### Expected behavior

The positional encodings in Whisper Encoder should not be trainable. This can be done by replacing `nn.Embedding` with a buffer. To preserve weight compatibility, it might be necessary to wrap the buffer around a module, so that the state dict key remains the same.

e.g.

```python
self.embed_position = nn.Module()  # empty module
self.embed_position.register_buffer("weight", torch.zeros(self.max_source_positions, embed_dim))  # preserve the key embed_position.weight
```

Comments:
Regarding matching the repo, I'dont think there is much difference in the results as we have 1e-4 matching logits with our integration tests. 

Regarding whether or not this should be trainable is up to debate:
-  the original repo does not provide a training script, so training discrepencies are not something we are trying to avoid 
- the idea behind leaving it as is is that you can do whatever you want: you can set `requires_grad` to `False` or you could load another vector obtained using a different functions. 

The positional encodings in Whisper Encoder are fixed sinusoidal encodings. Similar to rotary embeddings, it is fixed, and it should not be trained. I don't think there is a case that they should be trainable. Even though OpenAI did not provide a training script, it's clear that the sinusoidal positional encodings are fixed, and not trainable.

Letting the user does whatever he/she wants is fine, but

1. At least inform the user about this behavior. I lost a few days of experiments due to this.
2. Even if you want the positional encodings to be possibly trainable, shouldn't the default behavior be that it is not trainable, and user can set them to be trainable if they want?
3. Making them as buffers (and thus not trainable) still allows user to load other kinds of positional encodings. Buffers are included in state dict (unless they are specified to be `persistent=False`).

Agreed that we should default to non trainable. We probably did not have a lot of people training the encoder as this was not notice 🤗 Would you like to open a PR? 
As you mentioned best solution would be to make it backward compatible, keeping the correct naming ! 
Would also say nice catch 👍🏻 

Which approach do you prefer

1. Keep it as `nn.Embedding`, but set it to `requires_grad=False` in `__init__()` i.e. default to non-trainable. The users can set it to trainable if they want.
2. Change it to buffer (my original suggestion, similar to OpenAI original repo). It can never be trainable this way. Loading positional encodings still works, since buffer is included in state dict.

Nice catch @gau-nernst! I would advocate for 1 to keep compatibility with Flax Whisper and TF Whisper which are using embedding layers as well, possibly with different dimension orders though. If we change to buffers, we'll have to check that the cross-platform loading logic works with these new weights

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.