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

test: Add script to test model loading below n_parameters threshold #1698

Merged
merged 35 commits into from
Jan 9, 2025

Conversation

isaac-chung
Copy link
Collaborator

@isaac-chung isaac-chung commented Jan 3, 2025

Fixes #1690

  • first get the model meta from model registry and check for n_parameters, and omit API models.
  • if it is below threshold (2B for now), then run mteb.get_model and output the exception (None or raw text)
  • write these to a results.json file in the repo (in the scripts folder)
  • ability to continue running from where the script left off (args to specify running missing models, unsuccessful models, or by model names)
  • Trigger script to run when there are changes to the model files, then write the results out to the json file.

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 marked this pull request as draft January 3, 2025 08:56
@Samoed
Copy link
Collaborator

Samoed commented Jan 3, 2025

I think we could create a file to record whether a model load was successful, to avoid repeated loading attempts.

@isaac-chung
Copy link
Collaborator Author

Right now the main issue is disk running out of space. Looking into how not to save the weights into cache folder when loading a model.

@Samoed
Copy link
Collaborator

Samoed commented Jan 4, 2025

Also, test should skip API based models

scripts/failures.json Outdated Show resolved Hide resolved
@isaac-chung isaac-chung changed the title test: Add model load test below n param threshold misc: Add script to test model loading below n_parameters threshold Jan 6, 2025
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.

Looking good so far. Just to clarify the intention is to move this into the tests suite?

@isaac-chung
Copy link
Collaborator Author

@KennethEnevoldsen thanks, and yes.

@isaac-chung isaac-chung marked this pull request as ready for review January 6, 2025 22:17
@isaac-chung
Copy link
Collaborator Author

@KennethEnevoldsen @Samoed just pushed the latest changes, and updated the description. Would you mind reviewing and seeing if anything is unclear?

I'll try to push a "test" commit to see how it works.

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.

Generally looks good, very happy to accept this with just the changes the dependecy installations.

.github/workflows/model_loading.yml Outdated Show resolved Hide resolved
scripts/model_load_failures.json Outdated Show resolved Hide resolved
.github/workflows/model_loading.yml Show resolved Hide resolved
.github/workflows/model_loading.yml Outdated Show resolved Hide resolved
.github/workflows/model_loading.yml Outdated Show resolved Hide resolved
@isaac-chung
Copy link
Collaborator Author

Thanks both for your input. Right now the test fails at the merge base step (empty list is returned). If you have some time, it would be great if you could help take a quick look there. Otherwise I plan to return to this later tomorrow.

@Samoed
Copy link
Collaborator

Samoed commented Jan 7, 2025

I think the GitHub runner automatically merges branches during CI, so you don’t need to do it manually.

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.

Great work! I think we are almost there

Makefile Outdated Show resolved Hide resolved
.github/workflows/model_loading.yml Outdated Show resolved Hide resolved
mteb/models/bge_models.py Outdated Show resolved Hide resolved
scripts/extract_model_names.py Outdated Show resolved Hide resolved
tests/test_models/model_load_failures.json Show resolved Hide resolved
tests/test_models/model_load_failures.json Show resolved Hide resolved
tests/test_models/test_model_loading.py Outdated Show resolved Hide resolved
isaac-chung and others added 2 commits January 8, 2025 17:28
@isaac-chung isaac-chung changed the title misc: Add script to test model loading below n_parameters threshold test: Add script to test model loading below n_parameters threshold Jan 8, 2025
Copy link
Collaborator

@Samoed Samoed left a comment

Choose a reason for hiding this comment

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

Great work!

.github/workflows/model_loading.yml Show resolved Hide resolved
@isaac-chung
Copy link
Collaborator Author

Thanks again. Merging now. Note that the model loading "test" will likely require merging the target branch to get the accurate diffs.

@isaac-chung isaac-chung merged commit 8d033f3 into main Jan 9, 2025
11 checks passed
@isaac-chung isaac-chung deleted the add-model-load-test-below-n_param_threshold branch January 9, 2025 15:42
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

Successfully merging this pull request may close these issues.

Unittests to check model loading?
3 participants