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

Optimization for MF #17

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Optimization for MF #17

wants to merge 3 commits into from

Conversation

thwu1
Copy link
Collaborator

@thwu1 thwu1 commented Jul 9, 2024

#9 I added precompution for collapsing three linear transformations without modifying the loading part. @iojw Do you want to do a speed test?

@thwu1 thwu1 changed the title Optimization for MF: Precompute collapsed linear transformation Optimization for MF Jul 9, 2024
@thwu1 thwu1 requested a review from iojw July 9, 2024 18:46
Copy link
Collaborator

@iojw iojw left a 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,
Copy link
Collaborator

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",
Copy link
Collaborator

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",
Copy link
Collaborator

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.

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.

2 participants