Fix: llava-next fails to consider `padding_side` during Training
Author: jp1924
Pull request: yes

# What does this PR do?

<!--
Congratulations! You've made it this far! You're not quite done yet though.

Once merged, your PR is going to appear in the release notes with the title you set, so make sure it's a great title that fully reflects the extent of your awesome contribution.

Then, please replace this with a description of the change and which issue is fixed (if applicable). Please also include relevant motivation and context. List any dependencies (if any) that are required for this change.

Once you're done, someone will review your PR shortly (see the section "Who can review?" below to tag some potential reviewers). They may suggest changes to make the code even better. If no one reviewed your PR after a week has passed, don't hesitate to post a new comment @-mentioning the same persons---sometimes notifications get lost.
-->

<!-- Remove if not applicable -->

Hi, Hugging Face

I am opening a PR to address a bug discovered during the training of llava-next.

An error occurs during training when the `padding_side` of the tokenizer is set to `left`.

```python
Traceback (most recent call last):
  File "/usr/lib/python3.10/runpy.py", line 196, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/usr/lib/python3.10/runpy.py", line 86, in _run_code
    exec(code, run_globals)
  File "/root/.vscode-server/extensions/ms-python.debugpy-2024.8.0-linux-x64/bundled/libs/debugpy/adapter/../../debugpy/launcher/../../debugpy/__main__.py", line 39, in <module>
    cli.main()
  File "/root/.vscode-server/extensions/ms-python.debugpy-2024.8.0-linux-x64/bundled/libs/debugpy/adapter/../../debugpy/launcher/../../debugpy/../debugpy/server/cli.py", line 430, in main
    run()
  File "/root/.vscode-server/extensions/ms-python.debugpy-2024.8.0-linux-x64/bundled/libs/debugpy/adapter/../../debugpy/launcher/../../debugpy/../debugpy/server/cli.py", line 284, in run_file
    runpy.run_path(target, run_name="__main__")
  File "/root/.vscode-server/extensions/ms-python.debugpy-2024.8.0-linux-x64/bundled/libs/debugpy/_vendored/pydevd/_pydevd_bundle/pydevd_runpy.py", line 321, in run_path
    return _run_module_code(code, init_globals, run_name,
  File "/root/.vscode-server/extensions/ms-python.debugpy-2024.8.0-linux-x64/bundled/libs/debugpy/_vendored/pydevd/_pydevd_bundle/pydevd_runpy.py", line 135, in _run_module_code
    _run_code(code, mod_globals, init_globals,
  File "/root/.vscode-server/extensions/ms-python.debugpy-2024.8.0-linux-x64/bundled/libs/debugpy/_vendored/pydevd/_pydevd_bundle/pydevd_runpy.py", line 124, in _run_code
    exec(code, run_globals)
  File "/root/workspace/llava/bug-fix.py", line 37, in <module>
    outputs = model(**input_param)
  File "/root/.venv/lib/python3.10/site-packages/torch/nn/modules/module.py", line 1511, in _wrapped_call_impl
    return self._call_impl(*args, **kwargs)
  File "/root/.venv/lib/python3.10/site-packages/torch/nn/modules/module.py", line 1520, in _call_impl
    return forward_call(*args, **kwargs)
  File "/root/transformers/src/transformers/models/llava_next/modeling_llava_next.py", line 807, in forward
    inputs_embeds, attention_mask, position_ids, labels, _ = self._merge_input_ids_with_image_features(
  File "/root/transformers/src/transformers/models/llava_next/modeling_llava_next.py", line 627, in _merge_input_ids_with_image_features
    raise ValueError(
ValueError: image_to_overwrite.sum()=tensor(4694) != num_image_features=4680 The input provided to the model are wrong. The number of image tokens is 4678 while the number of image given to the model is 2. This prevents correct indexing and breaks batch generation.
```

The root cause of this issue is the line `left_padding = True if not self.training else False`.

Even though `self.padding_side` is `left`, it is forcibly changed to `right`, leading to a size mismatch in `image_to_overwrite &= val` even if padding is considered.

After changing the problematic line to left_padding = self.padding_side == "left", the code functions as expected.

In my opinion, determining the `padding_side` through `self.padding_side` rather than relying on `self.training` provides greater flexibility.

## Reproduction
```python
import numpy as np
import torch

from transformers import LlavaNextConfig, LlavaNextForConditionalGeneration, LlavaNextProcessor


model_name_or_path = "llava-hf/llava-v1.6-mistral-7b-hf"
config = LlavaNextConfig.from_pretrained(model_name_or_path)
config.text_config.num_hidden_layers = 1
config.vision_config.num_hidden_layers = 1
image_token_index = torch.tensor([config.image_token_index])

processor = LlavaNextProcessor.from_pretrained(model_name_or_path)
processor.tokenizer.padding_side = "left"

model = LlavaNextForConditionalGeneration(config)

image = torch.zeros(
    [10, 3, config.vision_config.image_size, config.vision_config.image_size], device=model.device, dtype=torch.float32
)

text_1 = "Today, we are thrilled to present LLaVA-NeXT"
text_2 = "with improved reasoning, OCR, and world knowledge. LLaVA-NeXT even exceeds Gemini Pro on several benchmarks."

text_1_input_ids = processor(text=text_1)["input_ids"][0]
text_2_input_ids = processor(text=text_2)["input_ids"][0]

text_1_input_ids = torch.concat((image_token_index, text_1_input_ids))
text_2_input_ids = torch.concat((image_token_index, text_2_input_ids))


input_param = processor.tokenizer.pad([{"input_ids": text_1_input_ids}, {"input_ids": text_2_input_ids}])
input_param["pixel_values"] = torch.stack((image, image))
input_param["image_sizes"] = torch.tensor([[1980, 2640], [1980, 2640]])
input_param = {k: v.to(model.device) for k, v in input_param.items()}

outputs = model(**input_param)
```

## environment
```
- `transformers` version: 4.44.0.dev0
- Platform: Linux-6.8.0-36-generic-x86_64-with-glibc2.39
- Python version: 3.10.14
- Huggingface_hub version: 0.23.4
- Safetensors version: 0.4.3
- Accelerate version: 0.30.1
- PyTorch version (GPU?): 2.2.0+cu121 (True)
- GPU type: NVIDIA A100 80GB PCIe
```

## Before submitting
- [ ] This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
- [x] Did you read the [contributor guideline](https://github.com/huggingface/transformers/blob/main/CONTRIBUTING.md#create-a-pull-request),
      Pull Request section?
- [ ] Was this discussed/approved via a Github issue or the [forum](https://discuss.huggingface.co/)? Please add a link
      to it if that's the case.
- [ ] Did you make sure to update the documentation with your changes? Here are the
      [documentation guidelines](https://github.com/huggingface/transformers/tree/main/docs), and
      [here are tips on formatting docstrings](https://github.com/huggingface/transformers/tree/main/docs#writing-source-documentation).
- [ ] Did you write any new necessary tests?


## Who can review?
@zucchini-nlp

<!-- Your PR will be replied to more quickly if you can figure out the right person to tag with @

 If you know how to use git blame, that is the easiest way, otherwise, here is a rough guide of **who to tag**.
 Please tag fewer than 3 people.

Models:

- text models: @ArthurZucker
- vision models: @amyeroberts
- speech models: @sanchit-gandhi
- graph models: @clefourrier

Library:

- flax: @sanchit-gandhi
- generate: @zucchini-nlp (visual-language models) or @gante (all others)
- pipelines: @Narsil
- tensorflow: @gante and @Rocketknight1
- tokenizers: @ArthurZucker
- trainer: @muellerzr and @SunMarc

Integrations:

- deepspeed: HF Trainer/Accelerate: @muellerzr
- ray/raytune: @richardliaw, @amogkam
- Big Model Inference: @SunMarc
- quantization (bitsandbytes, autogpt): @SunMarc

Documentation: @stevhliu

HF projects:

- accelerate: [different repo](https://github.com/huggingface/accelerate)
- datasets: [different repo](https://github.com/huggingface/datasets)
- diffusers: [different repo](https://github.com/huggingface/diffusers)
- rust tokenizers: [different repo](https://github.com/huggingface/tokenizers)

Maintained examples (not research project or legacy):

- Flax: @sanchit-gandhi
- PyTorch: See Models above and tag the person corresponding to the modality of the example.
- TensorFlow: @Rocketknight1

 -->


Comments:
@zucchini-nlp, Thank you for your response.

yes, I agree with improving the error/warning message.

and I did not fully understand what you meant by `can you give an example of when you have to use 'right' padding?` Could you please explain this in more detail?

---

Recently, many users have been setting the padding_side to left not only during inference 
but also during training when using LLM and LMM.
However, in the llava-next code, padding_side is automatically set to right when self.training is True, 
and to left when self.training is False.
This is not an issue if the tokenizer.padding_side is right during training, 
but if it is left, it can cause problems like the ones I encountered.

The [modeling_llava_next.py](https://github.com/huggingface/transformers/blob/v4.43.3/src/transformers/models/llava_next/modeling_llava_next.py#L363-L370) file has been implemented to allow self.padding_side to be user-defined, but it is not used anywhere. 
My intention is to modify it so that the padding_side of the tokenizer matches 
the model's padding_side, enabling it to process images correctly.

If the intention was to have padding_side as right during training and left during inference, it would be better to cancel this PR.

> Recently, many users have been setting the padding_side to left not only during inference
> but also during training when using LLM and LMM.

Ah, this is a trend I wasn't aware of. Out of curiosity, what is the reason behind this? If that is a commonly used thing, we can change the code as you propose and simply raise a warning that `padding is left even though in training mode. If that's intended, ignore this warning` by checking `self.training`

@zucchini-nlp 
Using left padding during training reduces the amount of code needed for convenience.

When handling ignore_idx for prompts, using left padding makes it easier to ignore the parts of the question and padding that are not part of the answer, simplifying the preprocessing code.

For example, if `World` is the answer that the LLM should output:

```python
<pad><pad><pad>### User:
Hello

### Assistance:
World
```

We can handle the parts after the answer using ignore_idx to exclude them from the loss calculation. Left padding allows for simple slicing to handle this efficiently, offering several advantages.

Another reason is consistency: it's better to keep the padding side the same rather than switching back and forth between padding sides during training and inference, aligning with `model.generate`.

Repositories commonly used for training LLMs, such as [TRL](https://github.com/huggingface/trl/blob/main/trl/trainer/utils.py#L138-L158), often have code written with the assumption of left padding

With the rise of instruction tuning in popular language models like Llama, Qwen, and Mistral, the use of left padding has also gained popularity.


@jp1924 

Not sure of the code you provided enforces left padding, as it only masks out the parts with "Human's turn". All the operations to ignore padding is done when calling `super()` class DataCollatorForLanguageModeling, which is usually padding-side agnostic.

But if there are people willing to train/tune with left padding, we can remove the hard check on `padding_side`. I am okay with the PR, can you please add a warning that checks `self.training` and warns that the user has left padding for training which is not the conventional way. Same warning if not training and padding is on the right. That way we will have more transparency on what is happening to help one debug their script :)

my traceback in #29850 . two comments:
1. technically doesn't matter in case of batch_size = 1. though warning probably won't hurt anyone much.
2. there still can be the case, by error or otherwise where `model.padding_side != processor.tokenizer.padding_side`, which will often break with the traceback as in mine and in @jp1924 case, which is not very helpful and points in a weird direction. worth adding to the ValueError message? @jp1924 see pr to ur fork.

I am not very pro of raising a `ValueError`, because that means we limit users to using only padding "left" or "right" in train/inference. As per the prev discussion, some users want to train with "left" padding, so we better give freedom to choose padding side.

For the potential `model.padding_side != processor.tokenizer.padding_side`, there is some info in the model docs, but not everyone would look there. So, we can get rid of the "training" in `if batch_size>1 and not self.training` and infer padding side from the inputs in all cases. After inferring we can raise warnings if the inferred padding doesn't match the convention that inference=left and training=right.

not sure we are talking about the same ValueError. i was referring to the specific error that **will** get raised if the inputs were prepared with a different padding from what model expects. i.e. the error in the traceback in original pr comment (line 626 of the code). whereas you seem to be talking about raising an error if the side is not what is 'conventional'

but potentially i misunderstood you?

@alyakin314 Got you! The way you suggest it is to add more details to error message, which I didn't see sorry. Yes, that totally works, just needs different wording imo

We still should try to infer padding when not training though, that is related to the current PR which is changing the way padding behaves for training mode. This should guide users to easier debugging 

>  different wording imo

word explanation without the variable names?

> We still should try to infer padding when not training though, that is related to the current PR which is changing the way padding behaves for training mode. This should guide users to easier debugging

that is probably true. presumably by inferring which side has a token == `self.pad_token_id`? there are cases when neither side has these (e.g. all are truncated), but that case wouldn't cause issues anyway, if i understand correctly. is there any other edge case that breaks this?

I meant smth like this

"This prevents correct indexing and breaks batch generation."
"Make sure you have same amount of special image tokens as there are images and that `model.padding_side == processor.tokenizer.padding_side`"

>  is there any other edge case that breaks this?
No, afaik the only edge case is that. This misunderstanding started because we expected users to use `right` padding when training, which came out to be not the case for all users

sure, changed the wording in the pr to the branch this pr is from (that's my only way of editing it without creating a separate pr, right?).

tbh, the only practical difference catching whether the pad token are on the 'wrong' side is that there will be an exception raised at the time of that check that will definitively say that what's wrong is the padding side. as opposed to current behavior where the one my edit is in being raised that says it's either this or that (because as we know - it breaks here anyway). happy to do it, but it's a few lines of extra code for the benefit of just having two separate error messages.

I agree with @alyakin314's opinion that it would be good to notify users when there is a mismatch in `padding_side` between the model and the tokenizer.

As @zucchini-nlp mentioned above, I think it would be better to modify [modeling_llava_next.py#L523-L533](https://github.com/huggingface/transformers/blob/v4.44.0/src/transformers/models/llava_next/modeling_llava_next.py#L523-L533) as follows to display a warning/error when a mismatch occurs.

I didn't add `self.training` separately, as this issue seems to occur regardless of whether it's during training or inference.

```python
    _left_padding = torch.any(attention_mask[:, 0] == 0)
    _right_padding = torch.any(attention_mask[:, -1] == 0)

    left_padding = self.padding_side == "left"
    if batch_size > 1:
        # There may be cases where batch_size is 2 or more, but there is no padding; in such cases, pass without else.
        if _left_padding and _right_padding:
            # In the following elif statements, either _left or _right is always False.
            raise ValueError(f"both side of attention_mask has zero, invalid. {attention_mask}")
        elif _right_padding and left_padding:
            "error or warnings"
            "If you want to add a warning, it would be good to set left_padding=False to avoid errors."
        elif _left_padding and not left_padding:
            "error or warnings"
            "If you want to add a warning, it would be good to set left_padding=True to avoid errors."
```

What do you think about the above modifications, @alyakin314 and @zucchini-nlp?

If I have misunderstood or pointed out something completely different, please let me know.

thank you

thanks @jp1924 
any specific reason to use this
```
    _left_padding = torch.any(attention_mask[:, 0] == 0)
    _right_padding = torch.any(attention_mask[:, -1] == 0)
```
over something like
```
    _left_padding = torch.any(input_ids[:, 0] == self.pad_token_id)
    _right_padding = torch.any(input_ids[:, -1] == self.pad_token_id)
```
?
checking the mask just feels like there can be a case it'll get triggered outside of the `model.padding_side == processor.tokenizer.padding_side` case if someone uses something unusual attention-wise. i haven't tested either though.

@alyakin314
> any specific reason to use this

The reason I used attention_mask is partly that the existing code uses it to check padding_side. 
More importantly, the `_merge_input_ids_with_image_features` method is designed to work under the assumption that attention_mask is always provided as input. Therefore, I didn't feel the need to modify it.

> checking the mask just feels like there can be a case it'll get triggered outside of the model.padding_side == processor.tokenizer.padding_side case if someone uses something unusual attention-wise. i haven't tested either though.

I agree with the suggestion that it is more reasonable to switch to using input_ids since attention_mask can be optional. 
To implement this change, we would need to add code to generate attention_mask 
that matches input_ids in case attention_mask is None. 
We should consult with @zucchini-nlp about this.


> I agree with the suggestion that it is more reasonable to switch to using input_ids since attention_mask can be optional.

Unfortunately the current LLaVa impl doesn't allow one to have no attn mask and will fail in any case down the line. Imo checking input ids is also not perfect and forces us to save pad token in model' config, along with tokenizer. In general, this padding thing is not the best solution, and we'll get rid of the huge merging block soon.

The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/transformers/pr_32386). All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@zucchini-nlp
What do you think about the idea in [this comment?](https://github.com/huggingface/transformers/pull/32386#issuecomment-2272460016)

It seems like a good idea to include a warning when there is a padding_side mismatch between the tokenizer and the model.


> I didn't add self.training separately, as this issue seems to occur regardless of whether it's during training or inference.

Yes, that is what I meant above as a possible option. We don't actually need to bloat with even more warnings, simple modify `if batch_size > 1 and not self.training:` -> `if batch_size > 1:` to infer padding in all cases. 

Oh, totally forgot, can you check the test I mentioned is passing and if not adapt it to the new changes?

(https://github.com/huggingface/transformers/blob/f5f1e52f6cf13cdf63ff25c311d33e2f2a842911/tests/models/llava_next/test_modeling_llava_next.py#L475-L507)

@zucchini-nlp, there's a conflict between the test code I wrote and the test code in the main branch. What should I do?

Oh, and could you check if there's any issue with the code I worked on? If there is, please let me know, and I'll fix it.

Oops...... @zucchini-nlp @amyeroberts, @alyakin314, my apologies. Embarrassingly, while fiddling with the settings to resolve the conflict, the PR was closed. I've reopened the #32679, can you check it out? 

@alyakin314 And if you don't mind, could you do a PR on the [Fix_llava_next_bug](https://github.com/jp1924/transformers/tree/Fix_llava_next_bug) branch again regarding the warning?