Error when merging image features and text features？

[slatter666]

https://github.com/huggingface/transformers/blob/be9cf070ee2cb6a9f0d162e5be32d9d68b9df3af/src/transformers/models/llava/modeling_llava.py#L336C9-L336C107

# 5. Fill the embeddings corresponding to the images. Anything that is not `text_positions` needs filling (#29835)
image_to_overwrite = torch.full(
    (batch_size, max_embed_dim), True, dtype=torch.bool, device=inputs_embeds.device
)
image_to_overwrite[batch_indices, text_to_overwrite] = False
image_to_overwrite &= image_to_overwrite.cumsum(-1) - 1 >= nb_image_pad[:, None].to(target_device)

I think the last line has something wrong in logic, look at the example

the input_ids is as follows, 32000 is the image token index, 0 is the padding index, we suppose num_patches=2
[[32000, 32000, 1, 2, 3],
 [1, 32000, 2, 3, 0]]

then new_token_positions is:
[[1, 3, 4, 5, 6],
 [0, 2, 3, 4, 5]]

nb_image_pad is: [0, 1]

before the last step, image_to_overwrite is:
[[True, True, True, True, False, False, False],
 [False, True, True, False, False, False, True]]

after the last step, image_to_overwrite is:
[[True, True, True, True, False, False, False],
 [False, False, True, False, False, False, True]]

however, the right result should be: 
[[True, True, True, True, False, False, False],
 [False, True, True, False, False, False, False]]

I think the code is only for left padding , if we use right padding, there should be some modifications, and here is my code:

image_to_overwrite = torch.full(
    (batch_size, max_embed_dim), True, dtype=torch.bool, device=inputs_embeds.device
)
image_to_overwrite[batch_indices, text_to_overwrite] = False
if left_padding:
    image_to_overwrite &= image_to_overwrite.cumsum(-1) - 1 >= nb_image_pad[:, None].to(target_device)
else:
    image_to_overwrite &= torch.ones_like(image_to_overwrite, dtype=torch.bool).cumsum(-1) - 1 <= new_positions[:, -1:].to(target_device)

[LysandreJik]

cc @ArthurZucker @zucchini-nlp

I believe you're under the legacy_processing flag, this is not recommended:

transformers/src/transformers/models/llava/modeling_llava.py

Line 464 in be9cf07

if legacy_processing:

[slatter666]

Well I check the version of transformers I'm using, I use transformers==4.43.4, and I I notice that this code is not in this version. I find the logic in latest version is right, however I still doubt the correctness in old version

[zucchini-nlp]

Hey @slatter666 !

Yes, we have been working on getting rid of this huuuge merging logic in favor of simple masked_scatter. The code is up on the latest v4.45, but we are in process if updating model checkpoints on the hub still. And we plan to support old-style checkpoints for a while, thus the older merging logic.

So, regarding your issue, we try to account for left-right padding here by changing new positions. Have you noticed any convergence issues in training or cuda-indexing errors? I'll verify once more to be sure, but afair for other llava models (with similar merging) this logic is tested and we havening changed anything lately. LMK if you have any short examples I can run to reproduce the bug :)

transformers/src/transformers/models/llava/modeling_llava.py

Lines 299 to 300 in be9cf07

	if left_padding:
	new_token_positions += nb_image_pad[:, None] # offset for left padding

[slatter666]

Well I don't run the training process, it's just that I want to slightly modify some logic of attention mask for my own design, then I check the logic of llava code and I find the code is only for left padding

[zucchini-nlp]

Can i get a small reproducer if possible, that would help us a lot to debug the code