Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Evaluators ambiguity: class/module #1124

Open
r0mer0m opened this issue Jul 30, 2024 · 4 comments
Open

Evaluators ambiguity: class/module #1124

r0mer0m opened this issue Jul 30, 2024 · 4 comments
Milestone

Comments

@r0mer0m
Copy link

r0mer0m commented Jul 30, 2024

There is ambiguity between classes and modules under the evalutors module. Specifically, evaluators objects under mteb/evaluation/evaluators are named the same as the modules which contain them. For example:

mteb.evaluation.evaluators.RerankingEvaluator references the class RerankingEvaluator in mteb/evaluation/evaluators/RerankingEvaluator.py.

This is problematic because it does not allow to reference the mteb/evaluation/evaluators/RerankingEvaluator.py module and therefore referencing any other definition within it.

@KennethEnevoldsen
Copy link
Contributor

I agree that this is a general problem within mteb. Ideally, we should have the modules in lowercase to avoid confusing the module with the object. I believe this is also the pep8 recommendation.

We also have quite a high usage of * imports, which also leads to a few oddities e.g.:

from mteb import load_dataset

I would love a fix for this. Will just check in with @Muennighoff and @isaac-chung as well to make sure that everyone agrees

@isaac-chung
Copy link
Collaborator

Thanks for bringing this up! I think it's great if we can follow something like pep8. This would lead to a breaking change that might not be backwards compatible, so this would trigger a major version bump. We should consider this for perhaps for 2.0.

@KennethEnevoldsen KennethEnevoldsen modified the milestones: mmteb, v2.0.0 Aug 1, 2024
@Muennighoff
Copy link
Contributor

This is problematic because it does not allow to reference the mteb/evaluation/evaluators/RerankingEvaluator.py module and therefore referencing any other definition within it.

But there is no other definition within RerankingEvaluator.py besides RerankingEvaluator, no?

Note that we also name all task & abstask files in this style e.g. https://github.com/embeddings-benchmark/mteb/blob/main/mteb/tasks/Classification/ara/TweetEmotionClassification.py - I think if we change it for evaluators we should also change it there but still unsure if the change is needed. 🤔

@r0mer0m
Copy link
Author

r0mer0m commented Aug 1, 2024

Thanks @KennethEnevoldsen and @isaac-chung, +1 that following pep8 standards is a good path forward.

I think that @Muennighoff brings up a valid point and this is my bad as RerankingEvaluator might not be the best example. A more relevant example would be the RetrievalEvaluator.py module if we want to reference classes such as DenseRetrievalExactSearch within that module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants