-
Notifications
You must be signed in to change notification settings - Fork 523
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(ai-help): index text-embedding-3-model embeddings #10818
Conversation
5fc9deb
to
53cb19d
Compare
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.
Conceptual feedback:
While there is no next we should set EMBEDDING_MODEL_NEXT
to null
. In that case an existing document has embedding_next
set we should also set it to null.
So when a model stabilized we can move from embedding_next
to embedding
and then set EMBEDDING_MODEL_NEXT
to null
.
8c120a3
to
61b1547
Compare
Sure, fixed in d8177d3. |
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.
My main point as per our slack conversation still stands. This is not necessary to evaluate the new model and ads complexity to our code. Since the PR is written and you keep updating it, even after me pointing out that we should just evaluate it fully locally first, there's no point blocking it because time has already been spend writing and reviewing.
It will be more hacky to test embedding dimensions and we don't yet know which embeddings we want to launch with but I won't block this as it eats up time.
Summary
Problem
We want to upgrade our embeddings to the
text-embedding-3-model
model.Solution
Index all documents in parallel using both the current and the new model.
How did you test this change?
Ran
yarn ai-help-macros update-index
locally, and it populated theembedding_next
column in the Supabase dev project (the column also exists in the stage/prod projects).