ParadoxZW / LLaVA-UHD-Better

A bug-free and improved implementation of LLaVA-UHD, based on the code from the official repo
Apache License 2.0
17 stars 3 forks source link

Attention mask的计算? #3

Open hust-nj opened 1 week ago

hust-nj commented 1 week ago

https://github.com/ParadoxZW/LLaVA-UHD-Better/blob/main/llava_uhd/adapt_llava.py#L136-L138

这里由于The first token is for CLS,是不是需要把

m[:w * h] = True

改成

m[:w * h+1] = True
hust-nj commented 1 week ago

以及check了下clip的code,attention mask在clip里应该-float('inf')才是表示mask而不是0或1表示mask?

ParadoxZW commented 6 days ago

https://github.com/ParadoxZW/LLaVA-UHD-Better/blob/main/llava_uhd/adapt_llava.py#L136-L138

这里由于The first token is for CLS,是不是需要把

m[:w * h] = True

改成

m[:w * h+1] = True

确实,您说的有道理,我会在后续更新中修改这个bug

ParadoxZW commented 6 days ago

以及check了下clip的code,attention mask在clip里应该-float('inf')才是表示mask而不是0或1表示mask?

这是我从~/miniconda3/envs/llv/lib/python3.10/site-packages/transformers/models/clip/modeling_clip.pyCLIPEncoder类的forward函数的注释里拷贝出来的

attention_mask (`torch.Tensor` of shape `(batch_size, sequence_length)`, *optional*):
                Mask to avoid performing attention on padding token indices. Mask values selected in `[0, 1]`:

                - 1 for tokens that are **not masked**,
                - 0 for tokens that are **masked**.

                [What are attention masks?](../glossary#attention-mask)

是不是我们的transformers版本不一样?

hust-nj commented 6 days ago

以及check了下clip的code,attention mask在clip里应该-float('inf')才是表示mask而不是0或1表示mask?

这是我从~/miniconda3/envs/llv/lib/python3.10/site-packages/transformers/models/clip/modeling_clip.pyCLIPEncoder类的forward函数的注释里拷贝出来的

attention_mask (`torch.Tensor` of shape `(batch_size, sequence_length)`, *optional*):
                Mask to avoid performing attention on padding token indices. Mask values selected in `[0, 1]`:

                - 1 for tokens that are **not masked**,
                - 0 for tokens that are **masked**.

                [What are attention masks?](../glossary#attention-mask)

是不是我们的transformers版本不一样?

我也注意到了这个注释,但是我去看了源代码,他是把这个attention_mask直接加到了attention weight上?

ParadoxZW commented 6 days ago

感谢您的关注与反馈。

P.S. 对于开放视觉encoder训练的支持,还有一点小问题,我会尽快更新(主要我这两天没卡了,训练推迟了)。如果您要跑实验,建议先不打开这个选项。

ParadoxZW commented 6 days ago

以及check了下clip的code,attention mask在clip里应该-float('inf')才是表示mask而不是0或1表示mask?

这是我从~/miniconda3/envs/llv/lib/python3.10/site-packages/transformers/models/clip/modeling_clip.pyCLIPEncoder类的forward函数的注释里拷贝出来的

attention_mask (`torch.Tensor` of shape `(batch_size, sequence_length)`, *optional*):
                Mask to avoid performing attention on padding token indices. Mask values selected in `[0, 1]`:

                - 1 for tokens that are **not masked**,
                - 0 for tokens that are **masked**.

                [What are attention masks?](../glossary#attention-mask)

是不是我们的transformers版本不一样?

我也注意到了这个注释,但是我去看了源代码,他是把这个attention_mask直接加到了attention weight上?

还真是!!!看起来这属于官方犯的一个小bug,估计他们没料到有人想要在视觉上做mask,所以忽视了这个问题。我看了代码,CLIPEncoder的定义会同时在CLIPTextTransformerCLIPVisionTransformer中使用。而前者的forward函数里有调用_expand_mask,这个函数似乎是会进行的正确的类型转换的;但是CLIPVisionTransformer中没有调用。但是不管怎么说,那个关于attention_mask解释的注释都不应该出现在CLIPEncoderforward里。

再次,感谢您的真知灼见!

ParadoxZW commented 6 days ago

如果您愿意,你可以基于现在的代码版本提交您的pr修复上述问题,我可以进行merge。当然我来update也可以。

hust-nj commented 6 days ago

以及check了下clip的code,attention mask在clip里应该-float('inf')才是表示mask而不是0或1表示mask?

这是我从~/miniconda3/envs/llv/lib/python3.10/site-packages/transformers/models/clip/modeling_clip.pyCLIPEncoder类的forward函数的注释里拷贝出来的

attention_mask (`torch.Tensor` of shape `(batch_size, sequence_length)`, *optional*):
                Mask to avoid performing attention on padding token indices. Mask values selected in `[0, 1]`:

                - 1 for tokens that are **not masked**,
                - 0 for tokens that are **masked**.

                [What are attention masks?](../glossary#attention-mask)

是不是我们的transformers版本不一样?

我也注意到了这个注释,但是我去看了源代码,他是把这个attention_mask直接加到了attention weight上?

还真是!!!看起来这属于官方犯的一个小bug,估计他们没料到有人想要在视觉上做mask,所以忽视了这个问题。我看了代码,CLIPEncoder的定义会同时在CLIPTextTransformerCLIPVisionTransformer中使用。而前者的forward函数里有调用_expand_mask,这个函数似乎是会进行的正确的类型转换的;但是CLIPVisionTransformer中没有调用。但是不管怎么说,那个关于attention_mask解释的注释都不应该出现在CLIPEncoderforward里。

再次,感谢您的真知灼见!

嗯嗯,我也把这个提给transformer 官方的issue了,原LLAVA-UHD的code真的bug太多了,您的codebase也在复现路上给了我很大的帮助 :)

ParadoxZW commented 6 days ago

@hust-nj 您好,我更新了代码中的这两个bug。您可以再看看写的有没有问题(特别是attention的部分)

ParadoxZW commented 5 days ago

@hust-nj 微调vision encoder训练的bug已修复

aosong01 commented 1 day ago

想问一下,我更新了最新的attention mask用法,但是训练的时候loss却是0,有没有可能是attention mask的相关问题呀

截屏2024-07-07 11 06 13
ParadoxZW commented 16 hours ago

@aosong01 现在这套代码在我的机器上loss是正常的。我之前遇到过这个情况,是学习率太大导致的。您试试看缩小学习率呢?