A question about the position of language indicator tokens of mBART
Author: speedcell4
Pull request: no

## Environment info

- `transformers` version: 4.17.0
- Platform: Linux-3.10.0-1160.49.1.el7.x86_64-x86_64-with-glibc2.10
- Python version: 3.8.5
- PyTorch version (GPU?): 1.11.0+cu102 (True)
- Tensorflow version (GPU?): not installed (NA)
- Flax version (CPU?/GPU?/TPU?): not installed (NA)
- Jax version: not installed
- JaxLib version: not installed
- Using GPU in script?: Yes
- Using distributed or parallel set-up in script?: No

### Who can help

Models:

- MBART, BART: @patil-suraj

## Information

<img width="655" alt="image" src="https://user-images.githubusercontent.com/3585459/162672533-17804a89-a22e-4716-bcea-bc89ea9c1dde.png">

According to Figure 1 of [the mBART paper](https://arxiv.org/pdf/2001.08210.pdf), I think,
* `input_ids = [s1, s2, ..., sn, 
, <SRC_LANG>]`
* `decoder_input_ids = [<TGT_LANG>, d1, d2, ..., dm, 
]`
* `labels = [d1, d2, ..., dm, 
, <TGT_LANG>]`

Then, I checked the output of the mBART tokenizer,

```python
tokenizer = AutoTokenizer.from_pretrained('facebook/mbart-large-50-many-to-many-mmt', src_lang='en_XX', tgt_lang='ro_RO')

src = tokenizer.tokenize('UN Chief Says There Is No Military Solution in Syria', add_special_tokens=True)
# ['en_XX', '▁UN', '▁Chief', '▁Say', 's', '▁There', '▁Is', '▁No', '▁Militar', 'y', '▁Solution', '▁in', '▁Syria', '
']

with tokenizer.as_target_tokenizer():
    tgt = tokenizer.tokenize('Şeful ONU declară că nu există o soluţie militară în Siria', add_special_tokens=True)
# ['ro_RO', '▁Şe', 'ful', '▁ONU', '▁de', 'cla', 'ră', '▁că', '▁nu', '▁există', '▁o', '▁solu', 'ţie', '▁militar', 'ă', '▁în', '▁Siria', '
']
```

Seems, the language indicator token is placed at the beginning of each sentence. However, I found `BartForConditionalGeneration` shifted the `labels` and feed it as the `decoder_input_ids` to the decoder while leaving the `input_ids` unchanged.

https://github.com/huggingface/transformers/blob/7c5d79912a21880ce13d77881940458e90d98917/src/transformers/models/bart/modeling_bart.py#L1343-L1346

Isn't this procedure changing the sequences to be like?

* `input_ids = [<SRC_LANG>, s1, s2, ..., sn, 
]`
* `decoder_input_ids = [
, <TGT_LANG>, d1, d2, ..., dm]`
* `labels = [<TGT_LANG>, d1, d2, ..., dm, 
]`

I think this is quite different from the original paper, Is this implementation correct?

Comments:
I noticed there is [a relevant issue](https://github.com/huggingface/transformers/issues/16583), @patil-suraj has mentioned
> In BART `eos` (`
`) token is used as the `decoder_start_token_id`

But why do we need `
` to be the `decoder_start_token_id`? In my understanding of the original paper, the `<TGT_LANG>` plays this role, according to Figure 1 again.

Hi! In the code snippet you are using `mbart-50` tokenizer, which uses a different format than mbart-25 (the model mentioned in the paper) as explained in the [doc](https://huggingface.co/docs/transformers/model_doc/mbart#mbart-and-mbart50). If you use the mbart-25 tokenizer in your code you'll see that that it follows the format from the paper.

```python
tokenizer = AutoTokenizer.from_pretrained('facebook/mbart-large-cc25', src_lang='en_XX', tgt_lang='ro_RO')
```

Also 

> But why do we need 
 to be the decoder_start_token_id? In my understanding of the original paper, the <TGT_LANG> plays this role, according to Figure 1 again.

This is an artifact of the `fairseq` repo. In fairseq the `labels` are shifted to the right to get the `decoder_input_ids`, which makes the `eos` token the first token in the sequence.

@patil-suraj Thanks for your reply.

I tried `mbart-large-cc25` as you mentioned as the following,

```python
from transformers import MBartForConditionalGeneration, AutoTokenizer
from transformers.models.mbart.modeling_mbart import shift_tokens_right

tokenizer = AutoTokenizer.from_pretrained(
    'facebook/mbart-large-cc25',
    src_lang='en_XX',
    tgt_lang='ro_RO',
)

model = MBartForConditionalGeneration.from_pretrained('facebook/mbart-large-cc25')

print(tokenizer.tokenize('UN Chief Says There Is No Military Solution in Syria', add_special_tokens=True))
# ['▁UN', '▁Chief', '▁Say', 's', '▁There', '▁Is', '▁No', '▁Militar', 'y', '▁Solution', '▁in', '▁Syria', '
', 'en_XX']

src = tokenizer('UN Chief Says There Is No Military Solution in Syria', return_tensors='pt')
with tokenizer.as_target_tokenizer():
    tgt = tokenizer('Şeful ONU declară că nu există o soluţie militară în Siria', return_tensors='pt')

hidden = model.forward(
    input_ids=src['input_ids'],
    attention_mask=src['attention_mask'],
    decoder_input_ids=shift_tokens_right(tgt['input_ids'], pad_token_id=tokenizer.pad_token_id),
    decoder_attention_mask=tgt['attention_mask'],
)

out = hidden.logits.argmax(dim=-1)
print(tokenizer.convert_ids_to_tokens(out[0].detach().tolist()))
# ['
', 'f', '▁of', '▁Say', '▁Say', 'ry', '▁Say', '▁Say', '▁Say', '▁Say', '▁Say', '▁Say', '▁Say', '▁Say', '▁Say', '▁Say', '▁Say', '▁Chief']
```

Yes, the tokenizer works fine, the `en_XX` is put at the end of the sentence. However, the predictions from `hidden` are quite different from the ground truth. Especially, the prediction starts with a `
` and ends with a `▁Chief`, we are not supposed to use `
`, are we?
I am not sure if I did something wrong.

Thanks anyway.