Skip to content

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

Merged
isaac-chung merged 35 commits into
mainfrom
add-model-load-test-below-n_param_threshold
Jan 9, 2025
Merged

test: Add script to test model loading below n_parameters threshold#1698
isaac-chung merged 35 commits into
mainfrom
add-model-load-test-below-n_param_threshold

Conversation

@isaac-chung

@isaac-chung isaac-chung commented Jan 3, 2025 •
edited
Loading

Copy link
Copy Markdown
Collaborator

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

Samoed commented Jan 3, 2025

Copy link
Copy Markdown
Member

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

@isaac-chung

isaac-chung commented Jan 3, 2025

Copy link
Copy Markdown
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.

@isaac-chung isaac-chung mentioned this pull request Jan 3, 2025
Closed
@Samoed

Samoed commented Jan 4, 2025

Copy link
Copy Markdown
Member

Also, test should skip API based models

Samoed
Samoed reviewed Jan 4, 2025
View reviewed changes
Comment thread scripts/failures.json Outdated
@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
KennethEnevoldsen
KennethEnevoldsen reviewed Jan 6, 2025
View reviewed changes

@KennethEnevoldsen KennethEnevoldsen left a comment

Copy link
Copy Markdown
Contributor

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

isaac-chung commented Jan 6, 2025

Copy link
Copy Markdown
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

isaac-chung commented Jan 6, 2025

Copy link
Copy Markdown
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.

KennethEnevoldsen
KennethEnevoldsen requested changes Jan 7, 2025
View reviewed changes

@KennethEnevoldsen KennethEnevoldsen left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread .github/workflows/model_loading.yml Outdated
Comment thread scripts/model_load_failures.json
Comment thread .github/workflows/model_loading.yml
Comment thread .github/workflows/model_loading.yml Outdated
Comment thread .github/workflows/model_loading.yml Outdated
@isaac-chung

isaac-chung commented Jan 7, 2025

Copy link
Copy Markdown
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

Samoed commented Jan 7, 2025

Copy link
Copy Markdown
Member

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

Samoed
Samoed reviewed Jan 7, 2025
View reviewed changes
Comment thread scripts/extract_model_names.py Outdated
KennethEnevoldsen
KennethEnevoldsen reviewed Jan 8, 2025
View reviewed changes

@KennethEnevoldsen KennethEnevoldsen left a comment

Copy link
Copy Markdown
Contributor

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

Comment thread Makefile Outdated
Comment thread .github/workflows/model_loading.yml Outdated
Comment thread mteb/models/bge_models.py Outdated
Comment thread scripts/extract_model_names.py Outdated
Comment thread tests/test_models/model_load_failures.json
Comment thread tests/test_models/model_load_failures.json
Comment thread tests/test_models/test_model_loading.py Outdated
isaac-chung and others added 2 commits January 8, 2025 17:28
@isaac-chung @KennethEnevoldsen
Apply suggestions from code review
d479c5f 
Co-authored-by: Kenneth Enevoldsen <kennethcenevoldsen@gmail.com>
@isaac-chung @KennethEnevoldsen
Update .github/workflows/model_loading.yml
fb26eab 
Co-authored-by: Kenneth Enevoldsen <kennethcenevoldsen@gmail.com>
This was referenced Jan 8, 2025
Closed
Closed
@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
Samoed
Samoed reviewed Jan 8, 2025
View reviewed changes

@Samoed Samoed left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great work!

Comment thread .github/workflows/model_loading.yml
KennethEnevoldsen
KennethEnevoldsen approved these changes Jan 8, 2025
View reviewed changes
Comment thread .github/workflows/model_loading.yml
@isaac-chung

isaac-chung commented Jan 9, 2025

Copy link
Copy Markdown
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
@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

Reviewers

@Samoed Samoed Samoed left review comments
@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.

Unittests to check model loading?

3 participants

@isaac-chung @Samoed @KennethEnevoldsen