MBart prepare_seq2seq_batch
Author: Chiyu-Song
Pull request: no
Labels: name: wontfix

- `transformers` version: 4.1.1
- mBART: @patrickvonplaten
## Information
Model I am using (Bert, XLNet ...): mBART

The problem arises when using:
* [x] the official example scripts: (give details below)
`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 = tokenizer.prepare_seq2seq_batch(example_english_phrase, src_lang="en_XX", tgt_lang="ro_RO", tgt_texts=expected_translation_romanian, return_tensors="pt")
model(input_ids=batch['input_ids'], labels=batch['labels']) # forward pass`

The encoded target sequence from "prepare_seq2seq_batch" is inconsistent with its description "[tgt_lang_code] X [eos]".
Moreover, "labels" doesn't seem to be the appropriate param for the model input.


Comments:
Hi @Chiyu-Song 

Not sure what exactly is the issue here.  What do you mean by 
> encoded target sequence from "prepare_seq2seq_batch" is inconsistent with its description

Also would be nice if the code snippet is formatted, bit hard to read :)

Thank you for your reply @patrickvonplaten, let me rephrase the issue a bit.

![image](https://user-images.githubusercontent.com/66252554/103333727-cd58a800-4aa9-11eb-8e6a-3d0921dbe94c.png)

The screenshot above is from the [MBart official documentation](https://huggingface.co/transformers/model_doc/mbart.html), introducing how to use "prepare_seq2seq_batch()" to encode input sequences for fine-tuning. However, after running the example code on the screenshot, I got something unexpected:

1. "prepare_seq2seq_batch()" returns a dict with three keys "input_ids", "attention_mask" and "labels". The value of "labels" is actually the encoder_input_ids, so I think this key name is a bit confusing.

2. The returned "labels"(encoder_input_ids) has a format "X [eos, tgt_lang_code]", but according to the description on the screenshot, it supposes to be "[tgt_lang_code] X [eos]".

3. On the last line of the code snippet, "labels" doesn't seem to be the appropriate param for the model input, I believe "encoder_input_ids" should be used instead for fine-tuning.

Opinions are my own, plz feel free to correct any of my misunderstandings.

1. `labels` is the correct name. `labels` are the tokens/output we expect the model (specifically the `decoder` to generate).  The `MbartForConditionalGeneration` model prepares the `decoder_input_ids`  (which are fed as input to  the decoder) using the `labels`. It's a convention to use the name `labels` for the output of models.

here `input_ids` is the input to the `encoder`. We don't use the name `encoder_input_ids`

2. As said above the model prepares the `decoder_input_ids` from `labels`. It does so by shifting the `labels` to the right and wrapping around the last token.

so if the `labels`  are 
`X [eos, tgt_lang_code]`

then `decoder_input_ids` are prepared as follows
`[tgt_lang_code] X [eos]`

i.e shift to the right and wrap around the last token. which is the target format expected by the model.

3. `labels` is not the name for model input, it's the output name used by all library models. `input_ids` is the model input.

 
And yes,  you are right. The doc is a little confusing. In the doc target text actually refers to the decoder input which is prepared using `labels`. Feel free to raise a PR to fix the doc :) 

Hi Suraj,

First of all, for the sake of clarity, 
I'd use the name "tokenizer.labels" to represent the prepared decoder_input_ids returned by "prepare_seq2seq_batch()".
And use "model.labels" to represent the input param for "BartForConditionalGeneration" model. According to the documentation, this param is used for computing the MLM loss and should has nothing to with the decoder_input_ids.

I double checked the source code of Bart model, in "BartForConditionalGeneration" it has a line of code like this:
 `decoder_input_ids = shift_tokens_right(labels, self.config.pad_token_id)`  
which means it gets decoder_input_ids  by shifting model.labels. 
It indeed fixes the thrid point I mentioned in the previous comment, but also breaks the designed functionality of model.labels.
To me, it seems like using a bug to cover another, and I really believe someone confused tokenizer.labels with model.labels during implementation.

-Chiyu

> I'd use the name "tokenizer.labels

It's already called `labels`. `prepare_seq2seq_batch` returns `input_ids`, `attention_mask`, `labels` and `decoder_attentoion_mask`

> but also breaks the designed functionality of model.labels

AFAIk it doesn't break any functionality. Could you show an example where it breaks ? 


This issue has been automatically marked as stale and been closed because it has not had recent activity. Thank you for your contributions.

If you think this still needs to be addressed please comment on this thread.