Fix embedding use of tokenizer to avoid already borrowed #369
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
markstur
requested review from
alex-jw-brooks,
gkumbhat,
evaline-ju,
gabe-l-hart,
tharapalanivel and
Ssukriti
as code owners
With high concurrency the fast tokenizer (Rust) hits "Already borrowed" exceptions with the truncation and padding (and max length) params change. Do not change those. Detect where to truncate (or return message) using the default settings and some code. Truncate texts and re-tokenize when needed. In addition, the error message which used to report number of tokens over the limit (but did not say which sentence) now returns the index(es) of the sentence(s) that was/were found to be too long. Signed-off-by: Mark Sturdevant <mark.sturdevant@ibm.com>
markstur
force-pushed
the
tokenizer_already_borrowed
branch
from
f07a634 to
1c09a02
Compare
Tokenizer is not completely thread-safe. This change copies the tokenizer for each thread (per model) to avoid future problems related to this. Eats up some memory, but small compared to models. Note: For embeddings server, we've been recommending 5 threads because more is slower anyway. Signed-off-by: Mark Sturdevant <mark.sturdevant@ibm.com>
gabe-l-hart
reviewed
Jul 24, 2024
| return self.tokenizer( | ||
|
|
||
| # Keep copies of tokenizer per thread (in each wrapped model instance) | ||
| thread_id = threading.get_ident() |
There was a problem hiding this comment.
One other primitive to take a look at here is threading.Local
There was a problem hiding this comment.
I tried using threading.Local instead of a map, but realized that each thread would need a map to get the correct tokenizer per model. So I preferred map keyed by thread in the model instance vs map keyed by model name (id?) in thread local. So I gave up experimenting with threading.Local, but was hoping it had some advantage here that I did not get to. Is a map in thread local better somehow for this?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With high concurrency the fast tokenizer (Rust) hits "Already borrowed" exceptions with the truncation and padding (and max length) params change. Do not change those. Detect where to truncate (or return message) using the default settings and some code. Truncate texts and re-tokenize when needed.
In addition, the error message which used to report number of tokens over the limit (but did not say which sentence) now returns the index(es) of the sentence(s) that was/were found to be too long.