[GemmaConverter] use user_defined_symbols
#29473
Conversation
|
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. |
pcuenca
left a comment
pcuenca
left a comment
There was a problem hiding this comment.
Great idea! There are a few additional tokens inuser_defined_symbols, I'll push PRs to the repos.
|
I will add a test and we can merge |
LysandreJik
left a comment
LysandreJik
left a comment
There was a problem hiding this comment.
Understood! cc @ydshieh that this will introduce some failing tests in the slow CI, we're aware (maybe we can skip those until we get to it? after the release)
| @require_tokenizers | ||
| class LlamaTokenizationTest(TokenizerTesterMixin, unittest.TestCase): | ||
| from_pretrained_id = "hf-internal-testing/llama-tokenizer" | ||
| from_pretrained_id = ["hf-internal-testing/llama-tokenizer", "meta-llama/Llama-2-7b-hf"] |
ArthurZucker
changed the title
GemmaConverte] use user_defined_symbolsGemmaConverter] use user_defined_symbols
|
Thank you for warning me. But could you share with me the reason to merge before we resolve the failing tests? |
|
We need to release ! |
|
It would be very appreciated that the expected failing tests to be skipped temporarily (before they would be fixed/updated after the release). |
|
We can open a PR to skip / fix them tomorrow! |
|
Failing tests are unrelated, merging! |
5 participants
What does this PR do?
Fixes #29440 by always adding the
user_defined_symbolsto the tokenizer, otherwise because no merges exist for them, they will be split.