Include input token count in results #334
Conversation
|
FYI: @markstur |
Signed-off-by: Mynhardt Burger <Mynhardt.Burger@ibm.com>
mynhardtburger
force-pushed
the
inlude-input_token_count-in-results
branch
from
89fd252
to
3c1f4b9
Compare
Signed-off-by: Mynhardt Burger <Mynhardt.Burger@ibm.com>
|
|
mynhardtburger
requested review from
alex-jw-brooks,
gkumbhat,
evaline-ju,
gabe-l-hart,
tharapalanivel and
Ssukriti
as code owners
| source_sentence, truncate_input_tokens=truncate_input_tokens | ||
| ) | ||
| embeddings = self._encode_with_retry( | ||
| embeddings, embeddings_token_count = self._encode_with_retry( |
There was a problem hiding this comment.
nit: embeddings_token_count is a confusing var name.
sentences_token_count or well even count2 would be better "embeddings_token_count", but doesn't matter. Varable name can be fixed whenever.
There was a problem hiding this comment.
Renaming to sentences_token_count.
| source_sentences, truncate_input_tokens=truncate_input_tokens | ||
| ) | ||
| embeddings = self._encode_with_retry( | ||
| embeddings, embeddings_token_count = self._encode_with_retry( |
There was a problem hiding this comment.
Renaming to sentences_token_count.
| to_tokenize = [texts] | ||
| else: | ||
| assert 0 | ||
| to_tokenize = [texts] |
There was a problem hiding this comment.
given this change, I think this line can go after the asserts
There was a problem hiding this comment.
Agreed. reodering.
|
|
||
| def encode( | ||
| self, | ||
| sentences: Union[str, List[str]], | ||
| sentences: Union[str, Collection[str]], |
There was a problem hiding this comment.
We already have a signature does not match the base class situation (added an arg we need and chose to leave some out that we ignore), but I don't see much value in these new tweaks. Were we already abusing the hints on those?
There was a problem hiding this comment.
It was an attempt to follow this guidance which states to use abstract base classes for arguments and concrete types for return types.
I'm reverting this to List[str] since I also personally find it easier to read (the specifics of ABC's are not widely known).
Signed-off-by: Mynhardt Burger <Mynhardt.Burger@ibm.com>
|
@mynhardtburger caikit 0.26.14 is available with the data model update |
Signed-off-by: Mynhardt Burger <Mynhardt.Burger@ibm.com>
Signed-off-by: Mynhardt Burger <Mynhardt.Burger@ibm.com>
Signed-off-by: Mynhardt Burger <Mynhardt.Burger@ibm.com>
Signed-off-by: Mynhardt Burger <Mynhardt.Burger@ibm.com>
Signed-off-by: Mynhardt Burger <Mynhardt.Burger@ibm.com>
There was a problem hiding this comment.
A few questions, and it'd probably be good for @markstur to review again as well
Co-authored-by: Evaline Ju <69598118+evaline-ju@users.noreply.github.com> Signed-off-by: Mynhardt Burger <mynhardt@gmail.com>
Signed-off-by: Mynhardt Burger <Mynhardt.Burger@ibm.com>
Signed-off-by: Mynhardt Burger <Mynhardt.Burger@ibm.com>
Signed-off-by: Mynhardt Burger <Mynhardt.Burger@ibm.com>
Signed-off-by: Mynhardt Burger <Mynhardt.Burger@ibm.com>
Signed-off-by: Mynhardt Burger <Mynhardt.Burger@ibm.com>
Depends on caikit data model updates in this PR: caikit/caikit#675
This extends the embedding module to include the
input_token_countin the results of theEmbeddingModule'srun_methods.The
sum_token_count(tokenized: BatchEncoding) -> intfunction calculates the count of tokens requiring model attention, based on theEncoding.attention_maskproperty, as returned bySentenceTransformerWithTruncate.tokenizer(). [PAD] is irrelevant for truncation and max_token_count parameters, while [CLS] and [SEP] are counted by the model when it considers the max length and truncation.Additionally tests to confirm sort order is maintained was added.
Various other quality of life type hints were added.