fix: Error in siglip output conversion#4205
Conversation
There was a problem hiding this comment.
Not sure why my comments are not displayed in here, only in file changes.
Seems that siglip uses lasttokenpooling, not mean pooling https://github.com/huggingface/transformers/blob/aad13b87ed59f2afcfaebc985f403301887a35fc/src/transformers/models/siglip/modeling_siglip.py#L523-L525
>>> torch.mean(
text_outputs["last_hidden_state"], dim=1
)
tensor([[-0.3865, -0.5087, 0.1831, ..., 0.8009, 0.1759, -0.5749],
[-0.3301, -0.8056, 0.2536, ..., 0.7427, -0.2328, -0.5802],
[-0.2553, -1.1079, 0.4744, ..., 0.0874, -0.0792, -0.6855],
...,
[-0.4886, -0.6445, 0.0385, ..., 0.3665, -0.1208, -0.5695],
[ 0.0374, -0.6736, 0.1148, ..., 0.1854, 0.2513, -0.7220],
[-0.0087, 0.3741, -0.0740, ..., 0.1965, -0.2676, -0.6060]])
>>> text_outputs.pooler_output
tensor([[ 1.0248, 0.2559, 0.1031, ..., 0.1529, -0.5528, 0.2910],
[ 0.7734, -0.6904, -0.7753, ..., -0.5422, 0.6960, -0.1439],
[ 0.4678, -0.2966, -0.2353, ..., 0.1854, -0.6198, 0.6368],
...,
[ 0.0607, 0.5256, -0.2899, ..., -0.9362, 1.1828, -0.0575],
[ 0.0698, -0.8170, 0.1015, ..., -0.1641, 0.2055, 0.2031],
[ 0.5874, -0.5110, -0.3075, ..., 0.4028, 0.3083, -0.3046]])
Co-authored-by: Roman Solomatin <samoed.roman@gmail.com>
|
Good catch - let me run some comparisons to check |
|
Yeah so clearly better, but still bad: Vidore3FinanceEnRetrieval.v2.json Though this models isn't really designed for this so very unsure if we would expect it to be any better. |
|
I don't think that results will be better |
|
You can also try to reprocude scores from mieb |
|
I can't seem to find what the error is, and how to reproduce it? |
|
I tried on main |
|
When I ran that, I got an ModuleNotFoundError for sentence piece, and then for protobuf |
|
Ok, got the same AttributeError when i run but when I run with |
|
I think this error not related to siglip. We just handle this task incorrectly |
|
I ran it on CIFAR10 notably better than what is reported in the paper: 83.79. I then also checked for So I think the implementation is correct, and that we might have had some issues in earlier versions either of the task or the model. @isaac-chung also resolved the issue with missing dependencies. |
|
Looks good! Only thing left is likely the dependency conflicts then |
This should happen here: https://github.com/embeddings-benchmark/mteb/blob/ce7590dcc9c620450ca192a3ec101a62631e6b55/mteb/_create_dataloaders.py#L291-L292 Not sure why it is needed
added a fix. I also found another problem when running it on |
…nto fix-siglip
|
I think this is good to merge with the fixes |
Discovered when running for #4193