[v2] add similarity_fn in ModelMeta

[sam-hey]

Checklist

• Run tests locally to make sure nothing is broken using make test.

• Run the formatter to format the code using make lint.

• fix: Retrieve name and revision from SentenceTransformers metadata.

• mv: Move the get_similarity_function method to ModelMeta.

• fix: Resolve issues with bm25s.

• feat: Add a max_similarity function to utils.py.

• feat: Map and load SentenceTransformers similarity functions to MTEB similarity functions.

• ref: Changed max_sim to MaxSim to align with the pylate implementation of similarity_fn_name Update similarity_fn_name = 'cosine' to MaxSim lightonai/pylate#77

• test: add test for ColBERT and bm25s

[Samoed]

Overall, I really appreciate your suggestions, but I’m not sure if we should rely on the ModelMeta object for anything during evaluation.

[isaac-chung]

Added 2 comments as I see that this is meant to be partially complete at this time.

[sam-hey]

I made some additional changes, so the PR ended up being a bit larger than originally planned. Please take a look at what I’ve done and share your feedback—I’m looking forward to hearing your thoughts! @Samoed @isaac-chung

Just a heads-up, there’s one failing test: #1777.

Thanks!

[Samoed]

This currently doesn’t fix the issues with the similarity function, as it focuses only on the retrieval model wrapper. I think the changes should be made in get_model. The model name and revision can be used there, and the type of similarity function can be passed to the loader.

mteb/mteb/models/overview.py

Lines 156 to 157 in c3b46b7

	meta = get_model_meta(model_name, revision)
	model = meta.load_model(**kwargs)

Also ModelMeta can be changed to separate loader and additional loader_kwargs

[sam-hey]

This currently doesn’t fix the issues with the similarity function, as it focuses only on the retrieval model wrapper. I think the changes should be made in get_model. The model name and revision can be used there, and the type of similarity function can be passed to the loader.

mteb/mteb/models/overview.py

Lines 156 to 157 in c3b46b7

	meta = get_model_meta(model_name, revision)
	model = meta.load_model(**kwargs)

Also ModelMeta can be changed to separate loader and additional loader_kwargs

Yes, you're correct about the changes. Did you notice these changes? I’m using the st similarity function names and map them to the Enum.
https://github.com/embeddings-benchmark/mteb/pull/1759/files#diff-629817399c17e22b713b95ac5146dc9ddd81b3977bef3165c23fa6166534224b

[Samoed]

No, I missed, thanks! But still I think this should be changed to not duplicate model_name and revision

[sam-hey]

Not sure what you mean here. What is model in this case? When I run:

When I run the following code:

from sentence_transformers import CrossEncoder, SentenceTransformer

from mteb import MTEB
from mteb.models import ModelMeta

de_name = "average_word_embeddings_komninos"
revision = "21eec43590414cb8e3a6f654857abed0483ae36e"
de = SentenceTransformer(de_name, revision=revision)
ce = CrossEncoder("cross-encoder/ms-marco-TinyBERT-L-2-v2")
ce_revision = "e9ea2688951463fc2791a2ea2ddfce6762900675"

eval = MTEB(tasks=["SciFact"])
eval.run(
    de,
    output_folder="tests/results/stage1",
    overwrite_results=True,
    save_predictions=True,
    eval_splits=["test"],
)
eval.run(
    ce,
    eval_splits=["test"],
    output_folder="tests/results/stage2",
    save_predictions=True,
    top_k=10,
    previous_results="tests/results/stage1",
    )

Enums

VSCode does not provide autocompletion for Literal types by default when using the Python extension.

Accessing Available Types:
To find the available types, you need to manually explore ModelMeta and check the attribute.
This process is inconvenient and hampers productivity. Transitioning to Enums would:

• Drastically simplify type inspection
• Improve the ease of adding new models

But why not use the model.similarity interface:

The current implementation uses the similarity() method.
By aligning the approach with SentenceTransformers and incorporating ModelMeta, the implementation becomes more streamlined and consistent.

[KennethEnevoldsen]

Thanks for the clarification. For future PRs it would make it easier for the reviewer if what it intends to solve is specified in the first comment (it seems like this was discussed elsewhere, in which case do link to it). Besides the Enums (which is being discussed in #1784) I believe this is ready to merge as soon as tests pass.

[sam-hey]

This PR originally had a completely different scope, so you're absolutely correct—it ended up getting quite tangled with multiple different PRs and issues addressing the same topic. My apologies for the confusion.

Only #1777 is failing, but that's expected

[isaac-chung]

Only #1777 is failing, but that's expected

That's fixed in the latest commit in main. Feel free to copy that small commit over.

[KennethEnevoldsen]

There seems to be a conflict. @sam-hey if you resolve this remove the Enum part we can merge this in.

This is not at all a hard decision on Enums, but I think with two major merges coming up, this will cause a lot of conflicts (cc. @Samoed, @gowitheflow-1998). We can still do the transition after the merge (outlined in #1784).

[sam-hey]

@KennethEnevoldsen, what do you think about merging it in its current state? I’ve kept the Enum but reverted the changes in all the models.
This should minimize the effort required to merge the new changes while still supporting enums.

Personally, I think this is a good compromise. Backward compatibility is fully maintained.

[isaac-chung]

Model loading test has been fixed here in main as well: #1775

[Samoed]

@isaac-chung I've merged your updates to v2 branch and on next CI trigger everything should work

[sam-hey]

All Enums have been removed. The changes should be ready for merging now.

[Samoed]

Great work!

[sam-hey]

Thanks a lot to all of you, @KennethEnevoldsen, @isaac-chung, and @Samoed!

I have one more question: I believe it would be beneficial to add BM25 and ColBERT tests to the pipeline. I’ve read that the pipeline is currently taking too long, and I’d like to know your thoughts on incorporating the BM25 and ColBERT requirements into the install-for-tests section of the Makefile.

What do you think?

[Samoed]

I think they can be integrated, but let's make separate PR

[KennethEnevoldsen]

Great work on this @sam-hey! Thanks for taking the time