Mismatch of the mask token id of BART between fairseq and huggingface
Author: twadada
Pull request: no

## 🐛 Bug
 
The mask token id of BART is different between fairseq (torch.hub) and huggingface, and this discrepancy leads to different results in mask_filling. So I wonder which token id is actually correct. 

(After checking the norm of the embedding at each mask token id, I feel that torch.hub might be correct. I have posted the same issue at fairseq github and been waiting for the reply.)

### To Reproduce
#### Code sample
```
from transformers import BartForConditionalGeneration, BartTokenizer
model = BartForConditionalGeneration.from_pretrained("facebook/bart-base", force_bos_token_to_be_generated=True)
tokenizer = BartTokenizer.from_pretrained("facebook/bart-base")
assert tokenizer.mask_token_id == 50264
example_english_phrase = "<mask> cat is <mask>."
batch = tokenizer(example_english_phrase, return_tensors='pt')
generated_ids = model.generate(batch['input_ids'],return_dict_in_generate = True, num_beams=10, num_return_sequences=1, output_scores = True)
print(" ".join(tokenizer.convert_ids_to_tokens(generated_ids[0][0])))

# 
 
 This Ġcat Ġis Ġadorable . 
**

import torch
bart = torch.hub.load('pytorch/fairseq', 'bart.base')
bart.eval()
assert bart.task.source_dictionary.indices["<mask>"]  == 51200
assert bart.task.source_dictionary.indices["madeupword0003"] == tokenizer.mask_token_id
# Somehow the huggingface model has a smaller vocab size, and 51200 is out of index
assert len(model.model.encoder.embed_tokens.weight) == 50265
assert len(bart.model.encoder.embed_tokens.weight) == 51201
# But the embedding at tokenizer.mask_token_id is the same between the two models
assert all(bart.model.encoder.embed_tokens.weight[tokenizer.mask_token_id] == model.model.encoder.embed_tokens.weight[tokenizer.mask_token_id])

def fill_mask(
        model,
        masked_inputs,
        topk = 1,
        match_source_len = False,
        masked_token = '<mask>',
        **generate_kwargs
):
    batch_tokens = []
    for masked_input in masked_inputs:
        assert masked_token in masked_input, \
            "please add one {} token for the input".format(masked_token)
        text_spans = masked_input.split(masked_token)
        text_spans_bpe = (' {0} '.format(masked_token)).join(
            [model.bpe.encode(text_span.rstrip()) for text_span in text_spans]
        ).strip()
        tokens = model.task.source_dictionary.encode_line(
            '
 ' + text_spans_bpe + ' 
',
            append_eos=False,
            add_if_not_exist=False,
        ).long()
        batch_tokens.append(tokens)
    generate_kwargs['beam'] = max(
        topk,
        generate_kwargs.get('beam', -1),
    )
    generate_kwargs['match_source_len'] = match_source_len
    batch_hypos = model.generate(batch_tokens, **generate_kwargs)
    return batch_hypos


masked_inputs=[example_english_phrase]
generate_kwargs = {}
generate_kwargs['beam'] = 10
generate_kwargs['match_source_len'] = False
batch_hypos = fill_mask(bart,masked_inputs, **generate_kwargs)
print(" ".join(tokenizer.convert_ids_to_tokens(batch_hypos[0][0]["tokens"])))

# 
 The Ġcat Ġis Ġdead . 
**

#### replace <mask> with madeupword0003 ####
example_english_phrase = "madeupword0003 cat is madeupword0003."
masked_inputs=[example_english_phrase]
batch_hypos = fill_mask(bart,masked_inputs, masked_token = "madeupword0003", **generate_kwargs)
print(" ".join(tokenizer.convert_ids_to_tokens(batch_hypos[0][0]["tokens"])))

# 
 This Ġcat Ġis Ġadorable . 

```
### Environment

 - PyTorch Version: 1.5.1+cu101
 - OS (e.g., Linux): Linux
 - Python version: 3.6.10
 - transformers version: 4.2.1
 - CUDA version: 10.1

### Additional context

<!-- Add any other context about the problem here. -->


Comments:
hi @twadada 

> Somehow the huggingface model has a smaller vocab size, and 51200 is out of index

50,265 is the actual vocab size, the rest of the tokens are dummy tokens as you can see in this issue pytorch/fairseq#2242

So I don't think 51200 can be the `mask_token_id`

Hi @patil-suraj 

Thank you for your quick reply!

> 50,265 is the actual vocab size, the rest of the tokens are dummy tokens as you can see in this issue pytorch/fairseq#2242

Yes, I am aware of that. But, the embedding of the mask token in huggingface-BART is exactly the same as that of the dummy token "madeupword0003" in torch.hub-BART, as confirmed in the following line
```
assert bart.task.source_dictionary.indices["madeupword0003"] == tokenizer.mask_token_id
assert all(bart.model.encoder.embed_tokens.weight[tokenizer.mask_token_id] == model.model.encoder.embed_tokens.weight[tokenizer.mask_token_id])
```
Here, "bart" and "model" are the torch.hub and huggingface models, resp. 

And this embedding looks very similar to that of the other dummy tokens, and the \<mask> token embedding in torch.hub-BART looks more accurate.

```
for dummy in  ["madeupword0003", "madeupword0030","madeupword0130", "madeupword0230", "<mask>"]:
    tokenid =  bart.task.source_dictionary.indices[dummy]
    emb_mean = bart.model.encoder.embed_tokens.weight[tokenid].mean().data
    emb_norm = bart.model.encoder.embed_tokens.weight[tokenid].norm().data
    print(emb_mean, emb_norm)

# tensor(-0.0083) tensor(0.9653) madeupword0003 (= huggingface mask embedding)
# tensor(-0.0087) tensor(0.9633) madeupword0030
# tensor(-0.0085) tensor(0.9688) madeupword0130
# tensor(-0.0084) tensor(0.9645) madeupword0230
# tensor(-0.0010) tensor(1.6455) torch.hub <mask> embedding

# torch.hub <mask> embedding is similar to that of other frequent words in terms of the norm.

for word in  [".", "," , "Ġthe"]:
    tokenid =  tokenizer.convert_tokens_to_ids(word)
    emb_mean = bart.model.encoder.embed_tokens.weight[tokenid].mean().data
    emb_norm = bart.model.encoder.embed_tokens.weight[tokenid].norm().data
    print(emb_mean, emb_norm)

# tensor(0.0391) tensor(1.6430)
# tensor(0.0512) tensor(1.8719)
# tensor(0.0483) tensor(1.8041)
```




Hi, any update on this? I've also tried loading BART from fairseq repository and confirmed that the model is identical to the one at torch.hub. Given that the original model is at fairseq, I assume there would be something wrong with the huggingface model. 

I think that registering a wrong embedding for the mask token is rather a serious bug and it needs fixing asap.

This issue has been stale for 1 month.

Has it been fixed?

I checked the HF Bart checkpoint and I agree with @twadada that the mismatch is a potential bug. Seems the HF model directly uses the first 50264 embeddings from fairseq. It should be the first 50263 embeddings concatenated with the last embedding (the embedding of `<mask>`). 

But I guess it's not a severe bug unless `<mask>` token exists in the input text. 

@zhaochaocs Thanks for confirming that. I think it can be a critical bug when you use BART as a masked language model, such as when you use it as a mask-filling model (e.g. a cloze test 
for probing), or when you fine-tune it on additional monolingual data using the MLM objective.


@twadada Yeah, I entirely agree. I tried some prompt ideas in the last project and found BART performed not as well as other PTMs even after post-training. Maybe the mismatch of the mask token is one of the reasons.

Hope HF can fix that someday, or we can temporarily replace the embedding of `<mask>` with the fairseq parameter.

Yea, we've gotta replace it with the correct embedding (assuming the fairseq parameter would be the correct one). Honestly, it was a little disappointing that this issue was sort of ignored and closed. Not sure whether this closed issue will draw attention from HF anymore.

Seems this issue was automatically closed by the Github bot. Maybe @patil-suraj or @patrickvonplaten can help us re-check if the embedding of `<mask>` token in BART should be fixed.

Leaving it to @patil-suraj as he has already looked at the PR, but I'm not so sure whether we have the wrong <mask> token simple because this example works quite well:

```python
from transformers import BartForConditionalGeneration, BartTokenizer
model = BartForConditionalGeneration.from_pretrained("facebook/bart-large", force_bos_token_to_be_generated=True)
tok = BartTokenizer.from_pretrained("facebook/bart-large")
example_english_phrase = "UN Chief Says There Is No <mask> in Syria"
batch = tok(example_english_phrase, return_tensors='pt')
generated_ids = model.generate(batch['input_ids'])
assert tok.batch_decode(generated_ids, skip_special_tokens=True) == ['UN Chief Says There Is No Plan to Stop Chemical Weapons in Syria']
```

Taken from: https://huggingface.co/transformers/model_doc/bart.html#mask-filling

I think that's why this potential bug is overlooked. 

The example is a good demo but not a reliable test case for this issue. If you replace the `<mask>` token of `example_english_phrase` as a random token (say `refin`), the generator will return the same sentence as using the `<mask>` token. In my machine it returns `UN Chief Says There Is No Plan to Stop Chemical Weapons in Syria`



@patil-suraj I think we actually indeed use the wrong token for <mask> as can be verified by running the following code:

```python 
import torch
from transformers import BartModel, BartTokenizer

# fsq bart base

bart = torch.hub.load('pytorch/fairseq', 'bart.base')
mask_token_id = bart.task.source_dictionary.indices["<mask>"]

mask_token_weight_fairseq = bart.model.encoder.embed_tokens.weight[mask_token_id].detach()

## Hf bart-base

hf_tok = BartTokenizer.from_pretrained("facebook/bart-base")
mask_token_id_hf = hf_tok.mask_token_id

hf_model = BartModel.from_pretrained("facebook/bart-base")
mask_token_weight_hf = hf_model.encoder.embed_tokens.weight[mask_token_id_hf].detach()

(mask_token_weight_hf - mask_token_weight_fairseq).abs().max()  # => gives value > 1.0
```

=> @zhaochaocs I'm afraid however that we can't do anything about it really...the original fairseq model weigths have a length of 51201 where as the HF model weights have only 50265 -> so we can't even access the "real" mask_token_id in HF. We could go into the official model weights and change the mask_token weights to the correct values, but this could lead to some ugly backward compatibility problems. And given how much bart is used in HF I'm not really in favor of doing this...The mask token is also only really relevant for pretraining and the mask-filling task IMO whereas for pretraining it's initialized from scratch anyways so this bug doesn't affect many use cases.

Keen to hear your opinion here @patil-suraj 

Also gently pinging @sshleifer - do you remember why we have different embedding shapes in HF vs. Fairseq?


@patrickvonplaten 
> given how much bart is used in HF I'm not really in favor of doing this...

I don't think it is a good idea to leave the bug unfixed just because many people have already used it. Rather, I believe it should be the reason for the bug to be fixed asap.

> The mask token is also only really relevant for pretraining and the mask-filling task

I think it can be a critical bug because nowadays more people are using cloze tests
to probe the linguistic knowledge stored in LM models. Some people also use MLM models for lexical substitution. It can also be a problem when you fine-tune BART on additional monolingual data using the MLM objective (only the mask token embedding is trained from scratch, with a small learning rate used for fine-tuning). 

Thanks a lot, @zhaochaocs and @twadada  for bringing this to our attention :) 

@patrickvonplaten I tend to agree with what @twadada said. Leaving this unfixed might cause issues when fine-tuning BART for MLM and this is a very sneaky bug so would be hard to detect.


IMO we could update the weights of the official models since BART is primarily used for downstream tasks mostly summarization and zero-shot classification which does not involve the mask token, so it wouldn't cause any issues for such models. Models which are already fine-tuned won't also be affected since we will only update the official pre-trained weights.

This should only probably break the mask-filling task, but will actually give better results as the current mask embeddings are incorrect.  

Here is a temporary solution.
I replace HF's <mask> embedding with fairseq's <mask> embedding. 
Here is the model
https://huggingface.co/liangtaiwan/bart-base-correct-mask-embedding

You can verify the new weight is corrected by the following script.

```python
import torch
from transformers import BartModel, BartTokenizer

# fsq bart=base

bart = torch.hub.load('pytorch/fairseq', 'bart.base')
mask_token_id = bart.task.source_dictionary.indices["<mask>"]

mask_token_weight_fairseq = bart.model.encoder.embed_tokens.weight[mask_token_id].detach()

# my bart-base

hf_tok = BartTokenizer.from_pretrained("liangtaiwan/bart-base-correct-mask-embedding")
mask_token_id_hf = hf_tok.mask_token_id
hf_model = BartModel.from_pretrained("liangtaiwan/bart-base-correct-mask-embedding")

mask_token_weight_hf = hf_model.encoder.embed_tokens.weight[mask_token_id_hf]
assert torch.equal(mask_token_weight_hf - mask_token_weight_fairseq)

# HF bart-base
hf_original_model = BartModel.from_pretrained("facebook/bart-base")

hf_original_model_state_dict = hf_original_model.state_dict()
hf_model_state_dict = hf_model.state_dict()
embeddings = ["shared.weight", "encoder.embed_tokens.weight", "decoder.embed_tokens.weight"]

# check weight 
for k in hf_model_state_dict.keys():
    if k in embeddings:
        continue
    assert torch.equal(hf_model_state_dict[k], hf_original_model_state_dict[k])

# check embedding
for k in embeddings:
    assert torch.equal(hf_model_state_dict[k][:-1], hf_original_model_state_dict[k][:-1])
```

However, I did some prompt language model experiments. The results are almost identical. The result of HF's one is even better sometimes. 


> Thanks a lot, @zhaochaocs and @twadada for bringing this to our attention :)
> 
> @patrickvonplaten I tend to agree with what @twadada said. Leaving this unfixed might cause issues when fine-tuning BART for MLM and this is a very sneaky bug so would be hard to detect.
> 
> IMO we could update the weights of the official models since BART is primarily used for downstream tasks mostly summarization and zero-shot classification which does not involve the mask token, so it wouldn't cause any issues for such models. Models which are already fine-tuned won't also be affected since we will only update the official pre-trained weights.
> 
> This should only probably break the mask-filling task, but will actually give better results as the current mask embeddings are incorrect.

Ok - I think I'm happy to go forward with this solution. Actually would be nice to get another opinion here. @sgugger, @LysandreJik  - would it be ok for you to updated existing pre-trained checkpoints?

I'm okay with updating the weights in a new commit since this fixes issues, as people can still revert to the previous commit if they really need to.

@patil-suraj - let me know if you want to handle it or if I should do it (I have some time tomorrow or next week if you're very busy ;-)) 

Ok for me too!

Great! I will update the weights today :) 

I've updated the weights for all 3 checkpoints pt, tf, flax https://huggingface.co/facebook/bart-base/tree/main

this issue is only associated with `bart-base`, `bart-large` does not have this problem, so no need to change the weights there :)

read the thread. i am the one who pushed the bug. the fix sounds good!

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/master/CONTRIBUTING.md) are likely to be ignored.

Closing this issue now.