-
Notifications
You must be signed in to change notification settings - Fork 247
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
Optimization for MF #17
base: main
Are you sure you want to change the base?
Conversation
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.
Thank you @thwu1! This will be an amazing improvement for MF - some comments.
use_proj, | ||
dim=128, | ||
num_models=64, | ||
text_dim=768, |
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.
Could we automatically set the text dim based on the selected embeddings?
num_classes=1, | ||
use_proj=True, | ||
embedding_model="all-mpnet-base-v2", |
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.
Maybe better to use OpenAI's embeddings by default to preserve our existing behavior? I'll add some docs to describe the different options here.
num_classes=1, | ||
use_proj=True, | ||
collapse_linear=False, | ||
embedding_model="all-mpnet-base-v2", |
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 it would be better if we didn't set any default args here and specify default args only in routers.py
so that it's easier to keep track of router configs.
#9 I added precompution for collapsing three linear transformations without modifying the loading part. @iojw Do you want to do a speed test?