-
Notifications
You must be signed in to change notification settings - Fork 311
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
feat: add model memory usage #1934
base: main
Are you sure you want to change the base?
Conversation
Have you already discussed with @KennethEnevoldsen or @x-tabdeveloping ? I'd prefer not to have such back-and-forth: to add then remove then add back things. #1729 |
Opened issue for this #1935. Previously it was removed, because it was not filled and should be autocalculated based on the number of parameters of the model |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand now (we're not adding this back to be filled in - we are auto-calculating it here. Great stuff!). Thanks for the initiative! Just 2 clarifications then I think we're ready.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a couple of minor comments. Thanks for adding this
mteb/model_meta.py
Outdated
if self.n_parameters is None: | ||
return None | ||
# Model memory in bytes. For FP32 each parameter is 4 bytes. | ||
model_memory_bytes = self.num_params * 4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a good assumption to make? Do all models have FP32 parameters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Large models (>1B) params usally loaded with fp16/bp16, but I don't know how to handle this automatically
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose you could get this information using huggingface_hub.hf_api.get_safetensors_metadata
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And then do a cached_property
so that it doesn't have to be fetched every time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think integrating this could slow down leaderboard building. Maybe we could manually set memory usage instead or add information about the number of parameters for each model weight?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. We could fetch all of these in a script and manually keep count of them in ModelMeta perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calculated them
# Conflicts: # mteb/models/bge_models.py # mteb/models/promptriever_models.py
To make fully compatible old leaderboard with new, we can add model memory usage. Add property to
ModelMeta
.Code Quality
make lint
to maintain consistent style.Documentation
Testing
make test-with-coverage
.make test
ormake test-with-coverage
to ensure no existing functionality is broken.