Possible bug in BlipForQuestionAnswering loss computation due to redundant right-shift
Author: verityw
Pull request: no

### System Info

- `transformers` version: 4.28.1
- Platform: Linux-5.15.0-69-generic-x86_64-with-glibc2.29
- Python version: 3.8.10
- Huggingface_hub version: 0.14.0
- Safetensors version: not installed
- PyTorch version (GPU?): 1.10.1+cu111 (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?: <fill in>
- Using distributed or parallel set-up in script?: <fill in>

### Who can help?

@ArthurZucker @younesbelkada 

### Information

- [X] The official example scripts
- [ ] My own modified scripts

### Tasks

- [ ] An officially supported task in the `examples` folder (such as GLUE/SQuAD, ...)
- [ ] My own task or dataset (give details below)

### Reproduction

In `BlipForQuestionAnswering.forward()`, if the `labels` parameter is provided, then [lines 1218-1222](https://github.com/huggingface/transformers/blob/v4.28.1/src/transformers/models/blip/modeling_blip.py#L1218) set `decoder_input_ids` to the right-shifted version of `labels`, then passes both those variables into `self.text_decoder`, which is an instance of `BlipTextLMHeadModel`:

```python
if labels is not None and decoder_input_ids is None:
    # get decoder inputs from shifting lm labels to the right - this is used in training mode
    decoder_input_ids = self._shift_right(labels)
    # replace possible -100 values in labels by `pad_token_id`
    labels = labels.masked_fill(labels == self.decoder_pad_token_id, -100)

answer_output = self.text_decoder(
    input_ids=decoder_input_ids,
    attention_mask=decoder_attention_mask,
    encoder_hidden_states=question_embeds,
    encoder_attention_mask=attention_mask,
    labels=labels,
    return_dict=return_dict,
    reduction="mean",
)
```
However, in the code for `BlipTextLMHeadModel.forward()`, it seems like it's [already doing that shift for you](https://github.com/huggingface/transformers/blob/04ab5605fbb4ef207b10bf2772d88c53fc242e83/src/transformers/models/blip/modeling_blip_text.py#L888):
```python
if labels is not None:
  # we are doing next-token prediction; shift prediction scores and input ids by one
  shifted_prediction_scores = prediction_scores[:, :-1, :].contiguous()
  labels = labels[:, 1:].contiguous().to(shifted_prediction_scores.device)
  loss_fct = CrossEntropyLoss(reduction=reduction, label_smoothing=0.1)
  lm_loss = loss_fct(shifted_prediction_scores.view(-1, self.config.vocab_size), labels.view(-1))
  if reduction == "none":
      lm_loss = lm_loss.view(prediction_scores.size(0), -1).sum(1)
```
Am I just misinterpreting this, or is the shift done twice, i.e., the loss is for next-next token prediction??

EDIT: As another point, the official [Jupyter notebook for BLIP](https://github.com/huggingface/notebooks/blob/main/examples/image_captioning_blip.ipynb) creates and instance of and trains `BlipForConditionalGeneration`, which also uses `BlipTextLMHeadModel` as the decoder. In this case, the `input_ids` and `labels` are the same (not shifted):
```python
for idx, batch in enumerate(train_dataloader):
  input_ids = batch.pop("input_ids").to(device)
  pixel_values = batch.pop("pixel_values").to(device)
  outputs = model(input_ids=input_ids,
                  pixel_values=pixel_values,
                  labels=input_ids)
```
Inside `BlipForConditionalGeneration.forward()`, it also doesn't shift the tokens:
```python
outputs = self.text_decoder(
            input_ids=input_ids,
            attention_mask=attention_mask,
            encoder_hidden_states=image_embeds,
            labels=labels,
            return_dict=return_dict,
            reduction="mean",
        )
```

EDIT 2: Seems like the original BLIP code similarly only shifts once. In `BLIP_VQA.forward()`, located [here](https://github.com/salesforce/BLIP/blob/main/models/blip_vqa.py#L51), there is no shift:
```python
answer = self.tokenizer(answer, padding='longest', return_tensors="pt").to(image.device) 
answer.input_ids[:,0] = self.tokenizer.bos_token_id
answer_targets = answer.input_ids.masked_fill(answer.input_ids == self.tokenizer.pad_token_id, -100)      

question_output = self.text_encoder(question.input_ids, 
                                    attention_mask = question.attention_mask, 
                                    encoder_hidden_states = image_embeds,
                                    encoder_attention_mask = image_atts,                             
                                    return_dict = True)    

question_states = []                
question_atts = []  
for b, n in enumerate(n):
    question_states += [question_output.last_hidden_state[b]]*n
    question_atts += [question.attention_mask[b]]*n                
question_states = torch.stack(question_states,0)    
question_atts = torch.stack(question_atts,0)     

answer_output = self.text_decoder(answer.input_ids, 
                                  attention_mask = answer.attention_mask, 
                                  encoder_hidden_states = question_states,
                                  encoder_attention_mask = question_atts,                  
                                  labels = answer_targets,
                                  return_dict = True,   
                                  reduction = 'none',
                                 )      
```
and there is a shift in `self.text_decoder.forward()`, as seen [here](https://github.com/salesforce/BLIP/blob/main/models/med.py#L904):
```python
prediction_scores = self.cls(sequence_output)
        
if return_logits:
    return prediction_scores[:, :-1, :].contiguous()  

lm_loss = None
if labels is not None:
    # we are doing next-token prediction; shift prediction scores and input ids by one
    shifted_prediction_scores = prediction_scores[:, :-1, :].contiguous()
    labels = labels[:, 1:].contiguous()
    loss_fct = CrossEntropyLoss(reduction=reduction, label_smoothing=0.1) 
    lm_loss = loss_fct(shifted_prediction_scores.view(-1, self.config.vocab_size), labels.view(-1))
    if reduction=='none':
        lm_loss = lm_loss.view(prediction_scores.size(0),-1).sum(1)  
```

Only the `text_decoder` itself shifts the text (again, in the forward function).
### Expected behavior

N/A

Comments:
cc @younesbelkada I think the thread above is correct, either:
* `input_ids` and `labels` are the same but then one needs to shift the `logits` when computing the loss. Examples of models that do this are GPT-2, GPT-J, etc
* you shift the `input_ids` (actually `decoder_input_ids` in case of a decoder) before feeding them to the Transformer and then you don't need to shift the `logits`. Examples of models that do this are T5, BART, etc.

This can probably be confirmed by fine-tuning `BlipForQuestionAnswering` on 10 example image, question and answer triplets and see whether the model is able to overfit them.

@NielsRogge @younesbelkada I just tried out your suggestion, and sure enough, it could not overfit to them. All answers are of the form ([CLS], a, [some noun], ., [SEP]). With the current implementation, the first shift changes that to ("", [cls], a, [some noun], .). Then, the second shift changes the pairing to inputs = ("", [cls], a, [some noun]) and labels = (a, [some noun], ., [sep]), e.g., next-next token prediction.

The outputs there are:
```
['', 'a', '.', 'cat', '[SEP]']
['', 'a', '.', 'dog', '[SEP]']
['', 'a', '.', 'wolf', '[SEP]']
['', 'a', '.', 'bear', '[SEP]']
```
Due to it learning next-next token prediction, "" is always followed by 'a', which is followed by '.' It never learns what should come after '.' but it just outputs the noun, which is then two tokens away from [SEP].

However, I found a fix. Instead of shifting the decoder's `input_ids` but not the `labels`, shift _both_, but do NOT get rid of the final character (since that's [SEP], which it should learn as the final character). Here's my code:
```python
# Now, train without redundant shift!
model = BlipForQuestionAnswering.from_pretrained("Salesforce/blip-vqa-base").to(device)
processor = AutoProcessor.from_pretrained("Salesforce/blip-vqa-base")
optimizer = transformers.AdamW(model.parameters(), lr=3e-5)

return_dict = model.config.use_return_dict
output_attentions = model.config.output_attentions
output_hidden_states = model.config.output_hidden_states
for i in range(40):
    total_loss = 0
    for inputs in training_points:
        # Copy-pasted code from BlipForQuestionAnswering.forward()
        vision_outputs = model.vision_model(
            pixel_values=inputs.pixel_values,
            output_attentions=output_attentions,
            output_hidden_states=output_hidden_states,
            return_dict=return_dict,
        )
        image_embeds = vision_outputs[0]
        image_attention_mask = torch.ones(image_embeds.size()[:-1], dtype=torch.long)
        question_embeds = model.text_encoder(
            input_ids=inputs.input_ids,
            encoder_hidden_states=image_embeds,
            encoder_attention_mask=image_attention_mask,
            return_dict=return_dict,
        )
        question_embeds = question_embeds[0] if not return_dict else question_embeds.last_hidden_state
        
        # Shift both the labels AND the input_ids. However, do not delete final [SEP] character.
        labels = inputs.labels.new_zeros(inputs.labels.shape[0], inputs.labels.shape[1] + 1)
        labels[..., 1:] = inputs.labels
        labels[..., 0] = model.decoder_start_token_id
        
        output = model.text_decoder(
            input_ids=labels,
            encoder_hidden_states=question_embeds,
            labels=labels,
            return_dict=return_dict,
            reduction="mean",
        )

        
        loss = output.loss.mean() if return_dict else answer_output[0].mean()
        total_loss += loss
        optimizer.zero_grad()
        loss.backward()
        optimizer.step()
```
Then, when decoding:
```python
for inputs in training_points:
    outputs = model.generate(input_ids = inputs.input_ids,
                    pixel_values = inputs.pixel_values)
    print(processor.batch_decode(outputs[0]))
```
(where the `input_ids` are tokens for "What animal is this?")

The end result:
```
['', '[CLS]', 'a', 'cat', '.', '[SEP]']
['', '[CLS]', 'a', 'dog', '.', '[SEP]']
['', '[CLS]', 'a', 'wolf', '.', '[SEP]']
['', '[CLS]', 'a', 'bear', '.', '[SEP]']
```

(Sorry I can't link directly to my code -- if it's really necessary/convenient, let me know and I can convert it into a colab nb)

Hi @verityw 

Thanks for flagging this! I made an attempt to fix your issue in https://github.com/huggingface/transformers/pull/23153
I am not sure this fixes 100% your problem, as I don't have access to your code, can you try to uninstall `transformers` and install `transformers` from that branch and let us know if you still face the issue?
```bash
pip install git+https://github.com/younesbelkada/transformers.git@blip-qa-loss-fix
```