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

Revert "Adds rule of thumb for determining embeddings size" #2069

Merged
merged 1 commit into from
May 29, 2022

Conversation

justinxzhao
Copy link
Collaborator

Reverts #2050

@github-actions
Copy link

Unit Test Results

       6 files  ±0         6 suites  ±0   2h 16m 54s ⏱️ -16s
2 798 tests ±0  2 758 ✔️ ±0    35 💤 ±0  5 ±0 
8 394 runs  ±0  8 280 ✔️ ±0  109 💤 ±0  5 ±0 

For more details on these failures, see this check.

Results for commit e5de092. ± Comparison against base commit d3419cc.

@tgaddair
Copy link
Collaborator

Can you add an explanation for why this is being reverted?

@w4nderlust
Copy link
Collaborator

Can you add an explanation for why this is being reverted?

There was a cat on slack, the tl;dr is that the previous PR introduced a new default that brake loading of older models. The solution would be to save the fully materialized config to avoid it, but that would require more though so the quick fix is reverting and reintroduce this feature later

@tgaddair
Copy link
Collaborator

Test was fixed upstream.

@tgaddair tgaddair merged commit a36fbb2 into master May 29, 2022
@tgaddair tgaddair deleted the revert-2050-embed_size_rule branch May 29, 2022 06:33
@justinxzhao
Copy link
Collaborator Author

Until we start saving the fully materialized config, there's an implicit dependency on whatever the default behavior is at the time of model creation. Filed #2066 to track.

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.

None yet

4 participants