-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Embeddings: simplify batching #51014
Conversation
b5366ca
to
c9a8491
Compare
c9a8491
to
4259658
Compare
e9332fd
to
1607c9d
Compare
4259658
to
051461d
Compare
051461d
to
bb16846
Compare
|
||
if len(embeddableChunks) > EMBEDDING_BATCHES*EMBEDDING_BATCH_SIZE { |
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.
It looks like EMBEDDING_BATCHES
is unused now. Was this just an unnecessary complexity that we removed?
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.
Good catch! I missed that was no longer used.
And yes, it's unnecessary complexity. I'm not sure why, but we were waiting until we had 5 batches worth of embeddings, then sending 1 batch at a time.
This is just some simplification of the batching code so the control flow is more clear. I'm going to be adding stats collection to this function, which will likey muddy things even more, so I wanted to clean this up first.
This is just some simplification of the batching code so the control flow is more clear. I'm going to be adding stats collection to this function, which will likey muddy things even more, so I wanted to clean this up first.
This PR does two things:
addToBatch
, which can callflush
if adding an embedding hits the batch size limit.Stacked on #50953Test plan
No change of behavior in existing tests covering this function.