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.

Sign up for GitHub

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

Jump to bottom

fix: Add E5 model test case in test CLI #958

Merged
merged 10 commits into from
Jun 20, 2024
Merged

fix: Add E5 model test case in test CLI #958

merged 10 commits into from
Jun 20, 2024

Conversation

isaac-chung
Copy link
Collaborator

@isaac-chung isaac-chung commented Jun 19, 2024 •
edited
Loading

addresses #957

Checklist

  • Run tests locally to make sure nothing is broken using make test.
  • Run the formatter to format the code using make lint.

@isaac-chung isaac-chung changed the title E5 model test fix: Add E5 model test case in test CLI Jun 19, 2024
@isaac-chung isaac-chung marked this pull request as draft June 19, 2024 10:05
@isaac-chung isaac-chung marked this pull request as ready for review June 19, 2024 10:52
KennethEnevoldsen
KennethEnevoldsen reviewed Jun 19, 2024
View reviewed changes
Copy link
Contributor

@KennethEnevoldsen KennethEnevoldsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this @isaac-chung I believe the fix is in #953 so I don't believe this is needed. However, it might be worth adding a test for e5.

tests/test_mteb.py Outdated Show resolved Hide resolved
tests/test_mteb.py Outdated Show resolved Hide resolved
mteb/evaluation/evaluators/model_encode.py Outdated Show resolved Hide resolved
@isaac-chung
Copy link
Collaborator Author

I believe the fix is in #953 so I don't believe this is needed.

Ooooooh okay! Should we wait till that PR gets merged then? Or should I add to your PR?

This still fails at the moment. Would love to have a fix either way.

mteb run -m intfloat/multilingual-e5-small -t BornholmBitextMining --verbosity 3 --output_folder tests/results/test_model --model_revision e4ce9877abf3edfe10b0d82785e83bdcb973e22e

@KennethEnevoldsen
Copy link
Contributor

Does it fail on the new PR?

@isaac-chung
Copy link
Collaborator Author

It passes here, but fails on master.

@isaac-chung isaac-chung marked this pull request as draft June 20, 2024 07:18
@KennethEnevoldsen
Copy link
Contributor

But we haven't tested it on #953

@isaac-chung
Copy link
Collaborator Author

isaac-chung commented Jun 20, 2024 •
edited
Loading

I can close this PR and add the extra test case in #953 instead, as it's collecting quite a lot of changes 👍 wdyt?

I'll merge that into this.

@isaac-chung isaac-chung marked this pull request as ready for review June 20, 2024 09:20
@KennethEnevoldsen
Copy link
Contributor

This looks fine to add as it is

@isaac-chung isaac-chung enabled auto-merge (squash) June 20, 2024 09:26
@isaac-chung isaac-chung merged commit 6c0f597 into main Jun 20, 2024
7 checks passed
@isaac-chung isaac-chung deleted the e5-model-test branch June 20, 2024 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KennethEnevoldsen KennethEnevoldsen KennethEnevoldsen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@isaac-chung @KennethEnevoldsen