-
Notifications
You must be signed in to change notification settings - Fork 61
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
Add web binding Tokenizer.tokenToId()
#41
base: main
Are you sure you want to change the base?
Conversation
The web portion looks good to me. cc @Ubospica for the change on |
@CharlieFRuan |
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.
Hi @grf53, sorry I didn't see this message before. That looks good for me, just one thing to point out.
int32_t TokenToId(const std::string& token) final { | ||
int32_t id = sentence_piece_.PieceToId(token); | ||
if (id == sentence_piece_.unk_id()) { | ||
return -1; | ||
} | ||
return id; | ||
} |
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.
Shall we still align with sentencepiece and still return unk when the token is not found?
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.
Hello @Ubospica. Thanks for your comment!
I am not sure about current circumstances with sentencepiece, but I just wanted to enable tokenToId
in web.
As I can see in the following description of Tokenizer.TokenToId()
, -1 would be returned when it could not find the corresponding id.
tokenizers-cpp/include/tokenizers_cpp.h
Lines 65 to 68 in 5de6f65
/*! | |
* \brief Convert the given token to its corresponding id if it exists. If not, return -1. | |
*/ | |
virtual int32_t TokenToId(const std::string& token) = 0; |
The mentioned function is implementing that virtual method, so I wanted to make it as that.
(For HFTokenizer, it is already covered as I saw.)
As I understood, UNK token from sentencepiece tokenizer could be sth other than -1 due to the configuration. So I added explicit check with sentence_piece_ .unk_id()
. But as I mentioned in the first comment, I am not sure this logic is really consistent considering practical uses, so you can suggest me to modify or revert that part to align it.
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.
Hello @grf53, thank you for your explanation!
I think we should still align with Huggingface tokenizers, which means returning unk_token instead of -1 when the token does not exist. Additionally, please add tests to ensure that unk_token is returned for non-existent tokens.
For the current implementation of HFTokenizer, we will later modify it to return unk_token instead of -1.
Thanks again for your contribution!
In
Tokenizer
class of web binding, there is notokenToId()
method whileidToToken()
is.There are already the original functions corresponding to
tokenToId()
.HFTokenizer.TokenToId()
SentencePieceTokenizer.TokenToId()
So I simply added function to use them on web.
Not sure logic related to
unk_id()
that I appended to theSentencePieceTokenizer.TokenToId()
is consistent.So please leave comments if there is any point to modify.
Thanks.