[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

[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

[KennethEnevoldsen]

I am note quite sure of the context of this PR. It would great with a short explanation of what the PR is trying to solve. Below are some questions to help getting that process started.

fix: Retrieve name and revision from SentenceTransformers metadata.

Can't see where this happens? Why do we want this?

feat: Introduce an Enum to represent all implementations of similarity functions.
feat: Refactor all models to utilize the ScoringFunction Enum.
rm: Eliminate literals used to describe scoring functions.

We generally havenøt used enum's anywhere else in the repo. Any strong reason to introduce it over the literal?

mv: Move the get_similarity_function method to ModelMeta.
feat: Map and load SentenceTransformers similarity functions to MTEB similarity functions.

Hmm, but why do we need a get_similarity function in the first place? The interface specified that the model should specify the similarity function. Is there anything that I am missing here?

fix: Resolve issues with bm25s.

What issue?

but totally how integrated ModelMeta with get_model. For now, all models duplicating model_name and revision. E.g.

I agree. I don't like this as well. However that is quite a large overhaul. We could consider reformatting models similar to tasks (so that we have a model where we can call model.metadata). However, the reason why we have the current system is that we don't want to load the model when only dealing with the metadata. This is solved in tasks using the load_data method. We could redo models in the same way, but currently I don't see a strong reason to do so. Thogh I would love to be convinced otherwise (however this is not for this PR)

[Samoed]

Ok, I'll create separate PR for updating work with ModelMeta

[sam-hey]

fix: Retrieve name and revision from SentenceTransformers metadata.
Can't see where this happens? Why do we want this?

Currently, when passing a model, the old way MTEB creates two folders with missing model name and revision. This happens because a type check in mteb/models/overview.py is on model, not model.model, which is of type SentenceTransformers.

We generally haven't used enums anywhere else in the repo. Any strong reason to introduce it over the literal?

This might be a subjective preference, but using an Enum reduces the code size and, in my opinion, makes it easier to work with when implementing a new model, as it provides type suggestions from the IDE.

I also believe it makes development in other parts of the project much easier, as the available types are clearly defined and suggested by the IDE.
Personally, I'd also love to see all the different language codes as an Enum.

Hmm, but why do we need a get_similarity_function in the first place?
The interface specifies that the model should specify the similarity function. Is there anything that I’m missing here?

The get_similarity_function maps the Enum in ModelMeta to the actual function that gets called. It’s not practical to define a function directly in ModelMeta, as it would require searching for every function.

What issue?

The BM25 function call does not match the defined search function.

[KennethEnevoldsen]

Ok, I'll create separate PR for updating work with ModelMeta

Maybe good to outline it in an issue first before you spend a lot of time on the refactor

[KennethEnevoldsen]

Currently, when passing a model, the old way MTEB creates two folders with missing model name and revision. This happens because a type check in mteb/models/overview.py is on model, not model.model, which is of type SentenceTransformers.

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

import mteb
task = mteb.get_task("BornholmBitextMining") 
model = mteb.get_model("sentence-transformers/all-MiniLM-L6-v2")
bench = mteb.MTEB(tasks=[task])
bench.run(model) # only created one folder

I do not get two folders.

re Enum

I understand where you are coming from. I think this is a more general change across the repo and not one that should happen just here. I would suggest that we stick with Literals as this will be a breaking change so no reason to add it unless we have a strong reason to. However do open an issue on Literals vs. Enums (most IDEs, e.g. vscode also allow suggestions when using literals).

The get_similarity_function maps the Enum in ModelMeta to the actual function that gets called. It’s not practical to define a function directly in ModelMeta, as it would require searching for every function.

But why not use the model.similarity interface:

mteb/mteb/encoder_interface.py

Line 71 in 0c5c3a5

def similarity(

This is modelled after sentence-transformers and freely allow the model to implement their own similarity function. Why is there a reason to fetch it?

[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]

Sorry, last changes! Maybe apply here pytest.importskip instead of pytest.mark.skip?

[sam-hey]

I'm totally here to improve my coding, so I really appreciate a thorough review, even if it means I have to redo parts! @Samoed, thanks for the feedback!

And you're absolutely right it is not so nice —pytest.importskip doesn't exist, as far as I can tell. However, pytest.importorskip is a similar function, but it’s not a marker.

We could go with something like this, or even write a custom decorator:

try:
    import pylate  # noqa
except ImportError:
    pylate_installed = False

@pytest.mark.skipif(not pylate_installed, reason="PyLate not installed")

https://docs.pytest.org/en/7.1.x/reference/reference.html#pytest-importorskip
pytest-dev/pytest#9548
https://stackoverflow.com/questions/73750328/make-pytest-ignore-gathering-a-file-if-a-module-is-not-installed

[Samoed]

Yes, I meaned importorskip

[sam-hey]

Have you reviewed the documentation and the related issue? You can find it here: pytest-dev/pytest#9548.

https://docs.pytest.org/en/7.1.x/reference/reference.html#pytest-importorskip

Using importorskip will work, but only if we split each component into its own file, which isn't very practical.

[Samoed]

You can use it inside test function