SPLIT PR: add user defined symbols and control symbols #31305
Merged
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
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
itazap
changed the title
itazap
changed the title
itazap
force-pushed
the
user_defined_symbols_add_30824
branch
6 times, most recently
from
97d0d35
to
befadd1
Compare
…en, so a space is not added
itazap
force-pushed
the
user_defined_symbols_add_30824
branch
from
befadd1
to
904121e
Compare
itazap
changed the title
| AddedToken(token, normalized=False, special=True) for token in self.proto.trainer_spec.control_symbols | ||
| ] | ||
|
|
||
| tokenizer.add_tokens(user_defined_symbols) |
There was a problem hiding this comment.
Cool! Related fix to only add once: huggingface/tokenizers@f2ec3b2
There was a problem hiding this comment.
maybe using map will be faster? but anyway it's good enough fro what we want !
There was a problem hiding this comment.
why did you change the encoded sequence here?
…e instead of trainer_spec for user_defined_symbols
ArthurZucker
approved these changes
Jun 20, 2024
| # Add user defined symbols | ||
| user_defined_symbols = [ | ||
| AddedToken(token, normalized=False, special=False) | ||
| for token in [p.piece for p in self.proto.pieces if p.type == 4] |
There was a problem hiding this comment.
let's add a comment that references the documentation of sentencepiece as to why we use 4 (arbitrary number)
| AddedToken(token, normalized=False, special=True) for token in self.proto.trainer_spec.control_symbols | ||
| ] | ||
|
|
||
| tokenizer.add_tokens(user_defined_symbols + control_symbols) |
There was a problem hiding this comment.
Ok this will work with the release of tokenizers, so fine for me! Good job
There was a problem hiding this comment.
we can also propagate to gemma! (only has this for now:
user_defined_symbols = [
AddedToken(token, normalized=True, special=False) for token in proto.trainer_spec.user_defined_symbols
]
tokenizer.add_tokens(user_defined_symbols)
itazap
added a commit
that referenced
this pull request
Jun 21, 2024
* PR SPLIT: moving origina changes for adding user defined symbols * adding gemma test and generalizing gemma converter * ruff * update common test * update serialization test * deberta v2 tests updates as rust version adds '.' as a user added token, so a space is not added * removing commented lines * applying feedback - user only added_tokens to add and check piece.type instead of trainer_spec for user_defined_symbols * add comment referencing sentencepiece
4 tasks
This was referenced Jul 10, 2024
# for free
to join this conversation on GitHub.
Already have an account?
# 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.
Fixes portion of #30824 ->
user_defined_symbolsandcontrol_symbols. @ArthurZucker thoughts on this?'.'in test cases' texts because it is a user added token for fast tokenizers, so the spacing around it differs from slow.add_special_tokensandadd_tokenshas same effect for now - but fix is already merged inTokenizersby @ArthurZucker remove enforcement of non special when adding tokens tokenizers#1521TODO in new PR:
SPMConverterdoes not always add the user defined symbol -> slow fast is thus not equivalent #30824) -> PR open: SPLIT PR: add_prefix_space fix #31315