search

embeddings-benchmark / mteb

MTEB: Massive Text Embedding Benchmark
https://arxiv.org/abs/2210.07316
Apache License 2.0
1.94k stars 270 forks source link

RetrivalEvaluator overrides encode kwargs #1325

Open Samoed opened 2 weeks ago

Samoed commented 2 weeks ago

When developing the wrappers, I tried to remove the explicit conversion from tensor to NumPy, but I encountered errors due to the convert_to_tensor parameter in encode_kwargs. I later found that RetrievalEvaluator overrides these parameters and sets batch_size. Maybe this should be changed?

https://github.com/embeddings-benchmark/mteb/blob/650e8b89ba856b21869e3fa6d5921ed7d9ed8d07/mteb/evaluation/evaluators/RetrievalEvaluator.py#L73-L78

KennethEnevoldsen commented 2 weeks ago

ahh, yes, these were added for backward compatibility when converting to encode_kwargs (#1030).

As you see in the diff, there are multiple of these, especially for batch size. For now, it is easy to just remove convert_to_tensor. However, it seems like the other two are being referred to in the code in multiple places. Not sure what the best choice is for these.

Samoed commented 2 weeks ago

I think that batch_size and convert_to_tensor can be removed and show_progress_bar can be passed directly to model.encode. I don't think that removing these kwargs will break something

KennethEnevoldsen commented 2 weeks ago

Removing batch size completely could lead to sudden OOM for many users, and it is also used in a few places in the evaluators.

Samoed commented 2 weeks ago

The current implementation often results in OOM errors because it sets the batch size too high. By default (if not specified), SentenceTransformer uses a batch size of 32. Another solution would be to explicitly set a default batch size in MTEB.run

KennethEnevoldsen commented 2 weeks ago

I am all for removing it completely (it is really a model concern, not an evaluation concern)