mBart prefix and suffix for language id
Author: RQuispeC
Pull request: no

## Environment info
- `transformers` version: 3.5.1

### Who can help
 mBART: @patrickvonplaten
 documentation: @sgugger

## Information

It seems that there is error or inconsistency between [mbart tokenizer](https://github.com/huggingface/transformers/blob/55e8d0cea25be18b044523b30f4bef58fec63289/src/transformers/tokenization_mbart.py) and comments/docs inline the code. 

comments on code explain
```
def build_inputs_with_special_tokens(
        self, token_ids_0: List[int], token_ids_1: Optional[List[int]] = None
    ) -> List[int]:
        """
        Build model inputs from a sequence or a pair of sequence for sequence classification tasks by concatenating and
        adding special tokens. An MBART sequence has the following format, where ``X`` represents the sequence:
        - ``input_ids`` (for encoder) ``X [eos, src_lang_code]``
        - ``decoder_input_ids``: (for decoder) ``[tgt_lang_code] X [eos]``
```

but `set_tgt_lang_special_tokens`  considers that `decoder_input_ids` is **X [eos][tgt_lang_code]** instead of **[tgt_lang_code] X [eos]**

```
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]
```

Shouldn't it be:
```
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.cur_lang_code]
        self.suffix_tokens = [self.eos_token_id]
```





Comments:
doublechecking the paper, it is expected that the language id as suffix so there is not a bug in the code but a error in the inline documentation.

Hey @RQuispeC - thanks a lot for checking! Do you feel like opening a PR to fix the doc? That would be super helpful :-)

Just added the PR @patrickvonplaten  :) 

Hi, I've ran into the same issue - but it seems that the documentation was correct, the target language tag should be the bos (and maybe also eos?). 
eos is set to be the target language id here: https://github.com/pytorch/fairseq/blob/c8a0659be5cdc15caa102d5bbf72b872567c4859/fairseq/tasks/translation_from_pretrained_bart.py#L116
but then for inference, it is also used as bos by default, as far as I can tell:
https://github.com/pytorch/fairseq/blob/c8a0659be5cdc15caa102d5bbf72b872567c4859/fairseq/sequence_generator.py#L173
The paper states "A language id symbol <LID> is used as the initial token to predict the sentence. "
I get very strange predictions with the pretrained mBART model unless I set "decoder_start_token_id" to the target language id in model.generate().

Hi, I didn't check FAIR's code but if you check the figure 1 of the paper ("Multilingual Denoising Pre-Training (mBART)") you can see that the language id is at:
* eos of input (source lang text)
* bos of decoder (I guess this is what author's mean with "A language id symbol is used as the initial token to predict the sentence. ")
* eos of output (target lang text)

The documentation refers to input and output, but not the decoder . `decoder_start_token_id` sets the decoder input symbol so it's expected that not using it gives weird results.
That's what I understand, Am I missing something?

The tokenizer produces `X [eos, tgt_lang_code]` for the target text, but to be consistent with the pretrained model, it should be `[tgt_lang_code] X [tgt_lang_code] `  (refering to the situation where you have a target side text)? `decoder_start_token_id` is only for inference, not for training, right?
 At least that's what I understand from the paper/fairseq code.

`decoder_start_token_id` is used during training and testing, it's the first token of the decoder, I found this easier to understand with the figure 1 of the paper.

Where did you get the input used for the pretrained model?

I don't have acces to the input used for pretraining (that would be so much easier :) ), I'm just trying to understand the format from the paper and code. 
Maybe a stupid question, but how can I set `decoder_start_token_id` for training (in the forward pass of the module)? I could only find it as an argument of generate() (I'm using version 3.1.0).
Also I've noticed that the models seems to have problems in inference if I replace `eos` with the language token on the target side (it randomly stops generating mid sentence), so I guess `eos` has to be present for generate() to function properly (I did set `eos_token_id` to the language tag)? 


Not stupit question :)
You can find an example of you to set source and target languages here 
https://github.com/huggingface/transformers/blob/master/examples/seq2seq/finetune_trainer.py#L222
```
        model.config.decoder_start_token_id = tokenizer.lang_code_to_id[data_args.tgt_lang]
```
Not sure if it's available in version 3.1, I was using version 3.5

The caveat is that (at least in up to version 3.5) the hugging face implementation only allows 1 language  for all your data in source and 1 language for all your data in the target, hence `decoder_start_token_id`, `tgt_lang_code` and `src_lang_code` are just one string. 
Depending in your problem this may be fine. For instance, if you are working on a translation then you are good to go with hugging face implementation because for translation you usually aim to translate one language (e.g. english) to another (e.g. spanish).

In my case I needed something more generic. I was working on a project that needed to train multiple languages at the same time, so my training data looked something like:

* source

| text | lang |
|:--------- | ----------: |
| my source text 1 | en_XX | 
| my source text 2 | es_XX | 
| my source text 3 | fr_FR | 

* target


| text | lang |
|:--------- | ----------: |
| my target text 1 | en_XX|
| my target text 2 | es_XX|
| my target text 3 | fr_FR|

In this case `decoder_start_token_id`, `tgt_lang_code` and `src_lang_code` should be `['en_XX', 'es_XX', 'fr_FR']` but this is not supported by hugging face. I implemented a custom version of `MBartForConditionalGeneration` and `MBartTokenizer`.

About the `eos` I have just used it as in the seq2seq example so I'm not sure about the behavior you mention.





Thank you for the explanations! There are major differences to the (old) version I'm using, so I think the best way for me is to update my code to work with the newest huggingface version. Hopefully, that will solve the problem :) But just in case, where would I find your custom versions of `MBartForConditionalGeneration` and `MBartTokenizer` ?

sorry for answering so late,  I was really busy at work.

I'm not sure if can open source it at this time but the main ideas was to check the flow of the parameters `tgt_lang_code` and `src_lang_code` then update the function calls to support vectors of strings instead of only strings.

I find this problem too
```
class MyMBartTokenizer(MBartTokenizer):
    def set_tgt_lang_special_tokens(self, lang: str) -> None:
        """Reset the special tokens to the target language setting. No prefix and suffix=[eos, tgt_lang_code]."""
        self.cur_lang_code = self.lang_code_to_id[lang]
        self.prefix_tokens = [self.cur_lang_code]
        self.suffix_tokens = [self.eos_token_id]
```