Make RobertaForMaskedLM implementation identical to fairseq
Author: BramVanroy
Pull request: yes
Labels: name: Core: Modeling

closes https://github.com/huggingface/transformers/issues/1874

The implementation of RoBERTa in `transformers` differs from the original implementation in [fairseq](https://github.com/pytorch/fairseq/tree/master/fairseq/models/roberta), as results showed (cf. https://github.com/huggingface/transformers/issues/1874). I have documented my findings here https://github.com/huggingface/transformers/issues/1874#issuecomment-588359143 and made the corresponding changes accordingly in this PR.

Someone should check, however, that removing `get_output_embeddings()` does not have any adverse side-effects.

In addition, someone who is knowledgeable about Tensorflow should check the TF implementation of RoBERTa, too.

Comments:
TODO: https://github.com/huggingface/transformers/pull/2913#issuecomment-588508153

# [Codecov](https://codecov.io/gh/huggingface/transformers/pull/2928?src=pr&el=h1) Report
> Merging [#2928](https://codecov.io/gh/huggingface/transformers/pull/2928?src=pr&el=desc) into [master](https://codecov.io/gh/huggingface/transformers/commit/59c23ad9c931ac4fe719abeb3c3851df046ef3a6?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/huggingface/transformers/pull/2928/graphs/tree.svg?width=650&token=9qOlN6Hb1c&height=150&src=pr)](https://codecov.io/gh/huggingface/transformers/pull/2928?src=pr&el=tree)

```diff
@@            Coverage Diff            @@
##           master   #2928      +/-   ##
=========================================
- Coverage    75.3%   75.3%   -0.01%     
=========================================
  Files          94      94              
  Lines       15424   15423       -1     
=========================================
- Hits        11615   11614       -1     
  Misses       3809    3809
```


| [Impacted Files](https://codecov.io/gh/huggingface/transformers/pull/2928?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/transformers/modeling\_roberta.py](https://codecov.io/gh/huggingface/transformers/pull/2928/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ19yb2JlcnRhLnB5) | `95.75% <100%> (-0.02%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/huggingface/transformers/pull/2928?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/huggingface/transformers/pull/2928?src=pr&el=footer). Last update [59c23ad...1f290e5](https://codecov.io/gh/huggingface/transformers/pull/2928?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Looks good. I tested it out and the outputs match exactly everywhere I can see. Requested review from @LysandreJik as well.

Regarding the test mentioned by @sshleifer, you can just test that a slice of the outputs match rather than the entire tensor. See [here](https://github.com/huggingface/transformers/blob/2184f87003c18ad8a172ecab9a821626522cf8e7/tests/test_modeling_roberta.py#L323) for an example.

> Looks good. I tested it out and the outputs match exactly everywhere I can see. Requested review from @LysandreJik as well.
> 
> Regarding the test mentioned by @sshleifer, you can just test that a slice of the outputs match rather than the entire tensor. See [here](https://github.com/huggingface/transformers/blob/2184f87003c18ad8a172ecab9a821626522cf8e7/tests/test_modeling_roberta.py#L323) for an example.

Thanks, will add tests later. I am still a bit confused why the weights of the embeddings are tied to the LMHead in the original implementation, though. I don't quite get the intention there.

Hm, perhaps this warning message should not be there.

> Weights of RobertaForMaskedLM not initialized from pretrained model: ['lm_head.weight']
> Weights from pretrained model not used in RobertaForMaskedLM: ['lm_head.decoder.weight']

- lm_head.weight is initialised because it takes the embedding weights
- the weights from the pretrained model are not used because they are not required


@BramVanroy Where are you getting that warning? I don't see it when I call `RobertaForMaskedLM.from_pretrained`

You can only see it if your logging level is set to INFO or lower. So you can put the following before loading the model.

```python
import logging
logging.basicConfig(
    format="%(asctime)s - %(levelname)s - %(name)s -   %(message)s",
    datefmt="%m/%d/%Y %H:%M:%S",
    level=logging.INFO)
```` 

Oh I see. Looks like the problem is just that the weight param introduced has a different name format than before. Rather than using the functional API as you did here, I would just manually override `decoder.weight` when `weight` is passed. I.e.,

```python
self.decoder = nn.Linear(config.hidden_size, config.vocab_size, bias=False)
if weight is not None:
    self.decoder.weight = weight
```
As you mentioned, it's not a huge issue since the weights are correctly loaded from the embeddings anyway, but probably a bit cleaner if the names align.

For those interested, I found the answer to the why on Twitter because of a helpful comment. Apparently this is common practice and has been introduced a while back in [Using the output of embeddings to improve language models](https://arxiv.org/abs/1608.05859).

> Hi @BramVanroy! I can see there's an issue here but I don't think this is the way to solve it.
> 
> We actually _do_ tie the weights together, so there's no need to do any additional tying. We actually tie the weights for every model that has an LM head (Masked or causal).
> 
> The issue here is because of the `bias` I introduced a few weeks ago with #2521. The way I did it means that the bias was actually applied twice.
> 
> The correct way to fix it would be to change
> 
> ```python
>         x = self.decoder(x) + self.bias
> ```
> 
> to
> 
> ```python
>         x = self.decoder(x)
> ```
> 
> in the forward method. The bias is already part of the decoder, so no need to apply it once more.
> 
> Do you want to update your PR, or should I do one to fix it?

Aha, my bad. I thought I finally contributed something useful!  :flushed: You can add a PR, I'll close this one. (Perhaps the updated test is still useful so that something like this doesn't happen in the future.)

Can you link to the lines where the weight tying is happening, though? I must have completely missed it.

Your contributions are more than useful, @BramVanroy, and I'm glad you tried to fix an issue when you discovered one, thank you.

To answer your question, the `PreTrainedModel` abstract class has an [`init_weights` method](https://github.com/huggingface/transformers/blob/53ce3854a16ad2a715bc6ac8af3e30c18b5a1d11/src/transformers/modeling_utils.py#L156) which ties the input embeddings to the output embeddings.

This method is not directly called by any model class, but it is called by the [`init_weights` method](https://github.com/huggingface/transformers/blob/53ce3854a16ad2a715bc6ac8af3e30c18b5a1d11/src/transformers/modeling_utils.py#L251) of that same abstract class.

It is this last method that is called by every model during their instantiation, for example with [`RobertaModel`](https://github.com/huggingface/transformers/blob/master/src/transformers/modeling_roberta.py#L152).

This is only the PyTorch way though, the TensorFlow way is different. In TensorFlow, we use a single layer that can be called as an `embedding` or a `linear` layer, as you may see in the [`BertEmbeddings` class](https://github.com/huggingface/transformers/blob/master/src/transformers/modeling_tf_bert.py#L132-L152). Please note the `mode` flag which makes possible the choice between the layers.