Possible error in MBart Tokenization script -- target lang code is only present in seq once
Author: Sun694
Pull request: no
Labels: name: Documentation

## Environment info

- `transformers` version: current
- Platform: Linux-4.19.112+-x86_64-with-Ubuntu-18.04-bionic
- Python version: 3.6.9
- PyTorch version (GPU?): 1.6.0+cu101 (False)
- Using GPU in script?: No.
- Using distributed or parallel set-up in script?: No.

### Who can help
MBart: @sshleifer 
## Information

Model I am using is MBart.

The problem arises when using:
* [x ] the official example scripts: (give details below)
* [ ] my own modified scripts: (give details below)

## To reproduce

Steps to reproduce the behavior:

```py
from transformers import MBartTokenizer
tokenizer = MBartTokenizer.from_pretrained('facebook/mbart-large-en-ro')
example_english_phrase = " UN Chief Says There Is No Military Solution in Syria"
expected_translation_romanian = "Şeful ONU declară că nu există o soluţie militară în Siria"
batch: dict = tokenizer.prepare_seq2seq_batch(
     example_english_phrase, src_lang="en_XX", tgt_lang="ro_RO", tgt_texts=expected_translation_romanian
)
```

```
-snip-
'labels': tensor([[ 47711,   7844, 127666,      8,  18347,  18147,   1362,    315, 42071,     36,  31563,   8454,  33796,    451,    346, 125577,      2, 250020]])}
```

The target language code is only present once in the target sequence.

`print(tokenizer.lang_code_to_id["ro_RO"])`
`250020`
## Expected behavior
```
'labels': tensor([[ 250020, 47711,   7844, 127666,      8,  18347,  18147,   1362,    315, 42071,     36,  31563,   8454,  33796,    451,    346, 125577, 2, 250020]])}
```

Here, the target language code is first and last, as I believe MBart (https://arxiv.org/pdf/2001.08210.pdf, top of page 3) says.

MBart Excerpt:
```
For each instance of a batch we sample a language id symbol <LID> ... 
sentences in the instance are separated by the end of sentence (</S>) token. Then, we append the selected<LID>
```

Here is the code I believe is wrong:
```py
    def set_tgt_lang_special_tokens(self, lang: str) -> None:
        """Reset the special tokens to the target language setting. Prefix [tgt_lang_code], suffix =[eos]."""
        self.cur_lang_code = self.lang_code_to_id[lang]
        self.prefix_tokens = []
        self.suffix_tokens = [self.eos_token_id, self.cur_lang_code]
```

To me, the comment implies the language code should be first as well.

I tested it locally, and merely adding `self.cur_lang_code` to `self.prefix_tokens` resolves the issue.

I do not know if I am misunderstanding the purpose of this script or misuing it. My above code is copied from the "MBartTokenizer" example at https://huggingface.co/transformers/master/model_doc/mbart.html#overview

If I didn't make a mistake, I'd be more than happy to open a PR to change that one lines and fix it.

Comments:
Note: I did find (https://github.com/pytorch/fairseq/issues/2258), a related issue.

As far as I can tell, the behavior there (attempting to zero-shot translate without the model having translated before, and merely getting the input as output regardless of language ID in target), is expected behavior (some fine-tuning is required on at least one language pair). 

I believe, for the target, `lang_code, text, <\s>, lang_code` is correct, and matches the paper.



I've spent a fair amount of time on the `mBart` tokenization. It's very complicated.
I ran the finetuning command documented in the README [here](https://github.com/pytorch/fairseq/tree/master/examples/mbart#finetune-on-en-ro) and set a breakpoint and looked at the various tensors: https://gist.github.com/sshleifer/cba08bc2109361a74ac3760a7e30e4f4

What you can clean from that is our `labels` match fairseq `samples['target']`.
```python
sample['target'][0]
tensor([  9345,    202,     10, 181684,     36,  21635,   8454,  48993,  45587,
            21,  57476,   1283,  98748,    451,    346,   8916,    202,     28,
             9,      7,    451,  11650, 128402,      5,      2, 250020],
       device='cuda:0')

```

So huggingface batches match fairseq code, but not the paper. This seems to improve translation finetuning and inference accuracy.

> I've spent a fair amount of time on the `mBart` tokenization. It's very complicated.
> I ran the finetuning command documented in the README [here](https://github.com/pytorch/fairseq/tree/master/examples/mbart#finetune-on-en-ro) and set a breakpoint and looked at the various tensors: https://gist.github.com/sshleifer/cba08bc2109361a74ac3760a7e30e4f4
> 
> What you can clean from that is our `labels` match fairseq `samples['target']`.
> 
> ```python
> sample['target'][0]
> tensor([  9345,    202,     10, 181684,     36,  21635,   8454,  48993,  45587,
>             21,  57476,   1283,  98748,    451,    346,   8916,    202,     28,
>              9,      7,    451,  11650, 128402,      5,      2, 250020],
>        device='cuda:0')
> ```
> 
> So huggingface batches match fairseq code, but not the paper. This seems to improve translation finetuning and inference accuracy.

Is there any chance to update the documentation, indicating this discrepancy?

I am using this in a multilingual translation setting -- having the lang_code only last, not first and last, means the model is not told what language to translate to. This is not an issue in bilingual settings. I expected the tokenization to match the paper and the documentation.

```
The source text format is X [eos, src_lang_code] where X is the source text. The target text format is `[tgt_lang_code] X [eos]`
```

Like before, if I'm not misunderstanding something, I'd be willing to open a PR for this.

Thanks for the quick response.

It's also worth noting that if there is no dedicated BOS (like MBart), then during inference, you have no natural way to tell the decoder to start generating during inference -- the model never has predicted the first token of a sequence.

The example at https://huggingface.co/transformers/master/model_doc/mbart.html#overview prepends the language code during inference, but if that is not done during training as well, this causes domain shift.

Unless the decoder (or something else) is editing targets behind-the-scenes (beyond "shifting" indexes one during training), I believe the current method of preparing batches is introducing domain shift.

You are missing the distinction between `decoder_input_ids` and `labels` I think.
For `mbart-large-en-ro` we have `decoder_start_token_id=250020` for this reason.

Then in [finetune.py](https://github.com/huggingface/transformers/blob/master/examples/seq2seq/finetune.py#L147):

```python
decoder_input_ids = shift_tokens_right(tgt_ids, pad_token_id)
outputs = self(src_ids, attention_mask=src_mask, decoder_input_ids=decoder_input_ids, use_cache=False)
```

`shift_tokens_right` moves the language code to the 0th column of `decoder_input_ids`.

You can also read [this](https://github.com/huggingface/transformers/issues/6156#issuecomment-678537995) which is related.

I would definitely welcome a contribution to the docs that explained this clearly!