Repository navigation
Conversation
bf64c6f to
e073da1
Compare
|
This has been an issue since 5.3.0 I believe, would be great to get this fixed. |
|
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. |
|
Thanks for the review @itazap! (and thanks @qflen for the redirect 🙏) Thanks for flagging the redundant work — you're right that On the pure
Note the official What I'd propose instead — a hybrid that keeps the authoritative decision but kills the "parse on every llama load" cost: if (
tokenizer_auto_map is None
and TokenizersBackend is not None
and config_model_type in _DUAL_SCHEME_MODEL_TYPES
and not has_file(pretrained_model_name_or_path, "tokenizer.model", **kwargs) # SP Llama-1/2 short-circuits, no parse
and _tokenizer_json_is_byte_level(pretrained_model_name_or_path, **kwargs) # confirm byte-level by content
):
return TokenizersBackend.from_pretrained(...)So Llama-1/2 loads only do a cheap Separately, I can drop the double-parse by threading the already-parsed json (or resolved path) into Does the hybrid sound reasonable to you, or would you prefer the simpler existence check and accept the edge cases? @itazap the 7 red checks look unrelated to this PR (which only touches
Both came in with the main merge, not the diff — could you re-run once main is green? Thanks! |
138ee16 to
b0f5a9f
Compare
|
Following up on my own proposal above, @itazap — I tested the hybrid before pushing it, and it doesn't hold. The The edge I described above as hypothetical turns out to be real:
I also mis-diagnosed the cost above: it's one Hub round-trip, not the parse. Warm cache on So I'd keep the routing logic as it stands. I've pushed a no-network regression test pinning the case above — it fails on |
5bbdf64 to
e1053d3
Compare
|
Rebased onto main — this was 131 commits behind, so the previous green run predated both #47296 and #45936. CI is green on the rebase. Re-measured on
SentencePiece llama checkpoints are unaffected: The id allowlist covers the I also reordered — the byte-level check now runs after Happy either way on the shape, @itazap — keep this as the fallback, or if you'd rather stay with the allowlist I'll add these ids there instead and close this. |
Model type "llama" spans both SentencePiece (Llama-1/2) and byte-level (Llama-3 / tiktoken) tokenizers under one Hub tokenizer_class (LlamaTokenizerFast). In v5, LlamaTokenizer.__init__ unconditionally installs a Metaspace pre-tokenizer/decoder, which silently drops spaces for byte-level repos (see huggingface#45488), e.g. deepseek-ai/DeepSeek-R1-Distill-Llama-*. The existing MODEL_IDS_TO_TOKENIZERS_BACKEND allowlist only covers specific checkpoints (e.g. the 8B) and misses others (the 70B is still broken). Instead, for the small set of dual-scheme model types, inspect the serialized tokenizer.json: if it declares a ByteLevel pre_tokenizer/decoder, route to TokenizersBackend (which respects tokenizer.json). SentencePiece Llama-1/2 stays on LlamaTokenizer unchanged.
Quantization pipelines copy the SentencePiece tokenizer.model over from the checkpoint they were derived from, so byte-level llama repos that declare LlamaTokenizer and ship a root tokenizer.model exist in the wild (e.g. benyamini/DeepSeek-R1-Distill-Llama-8B-AWQ-w4g128). Routing on the presence of tokenizer.model sends those to LlamaTokenizer, whose Metaspace pre-tokenizer drops spaces, so pin that the decision is made from tokenizer.json.
Both branches return TokenizersBackend, so ordering does not change which tokenizer any checkpoint resolves to. Putting the allowlist first means the checkpoints already listed there resolve on an in-memory fnmatch and never read tokenizer.json, so only the byte-level repos that no id pattern covers pay for the lookup. That addresses the redundant-work concern from review while keeping the content check as the fallback that catches community re-uploads, quantized derivatives, and local directories.
e1053d3 to
529939d
Compare
|
Rebased onto main. The one red check is @ArthurZucker #47862 tightens the SentencePiece decoder on The row I would most value your read on is in the table above: |
|
I verified this fix in a real vLLM deployment, and it resolves the garbled Environment
Before the fixLoading the tokenizer from the local model directory resolved to A tokenizer round-trip already demonstrated the problem: When serving the original local checkpoint with vLLM, the chat completion {
"message": {
"role": "assistant",
"content": "<think>ĊĊ</think>ĊĊHello!ĠHowĠcanĠIĠassistĠyouĠtoday?"
}
}After applying this PRLoading the same local model directory resolved to: The tokenizer round-trip was correct: I then served the original model directory with vLLM, without a separate The OpenAI-compatible API returned correctly decoded output: {
"message": {
"role": "assistant",
"content": "Hello! How can I assist you today?"
}
}Spaces and newlines were preserved, and the output no longer contained the raw This confirms that the content-based tokenizer routing in this PR fixes local Thanks for working on this fix. I would be happy to test additional local or |
This is wrong. There seems to be a lack of understanding of why one checkpoints is a LlamaTokenizer (llama original sentecepiece) vs tokenizerrs Backend (llama3-so on) which could have explicit Llama3Tokenizer, but they don't |
|
That taxonomy is the premise of this PR — first two paragraphs of the description. There is no explicit >>> from transformers import AutoTokenizer
>>> def rt(p):
... t = AutoTokenizer.from_pretrained(p)
... return type(t).__name__, t.decode(t.encode("Hello world. I'm an AI.", add_special_tokens=False))
...
>>> rt("deepseek-ai/DeepSeek-R1-Distill-Llama-8B")
('TokenizersBackend', "Hello world. I'm an AI.")
>>> rt("./DeepSeek-R1-Distill-Llama-8B") # the same files, downloaded
('LlamaTokenizer', "Helloworld.I'manAI.")
>>> rt("neuralmagic/DeepSeek-R1-Distill-Llama-70B-FP8-dynamic")
('LlamaTokenizer', "Helloworld.I'manAI.")The second one is a checkpoint already on the allowlist. Routing rather than a fix inside the class, because Can an id list cover a local directory? If it can, I'll close this. |
|
I am happy to fix |
|
Thanks, that covers the rows above. Sweeping the most-downloaded llama/deepseek repos for I'll send the list change if you want it. |
Here's the list. I swept the 50,000 most-downloaded 1,866 repos, 929 orgs, 670k combined downloads. None of the 1,866 are matched by the list today, which is by definition since that's one of the I ran the top 30 of that 897 by hand on main @ CSV and the sweep script are in the gist. Tell me which ones you want in |
|
Up to now, the problem still hasn’t been solved. |
|
Was asked again mostly for llama cpp compat, having another look |
Signed-off-by: Park Soobin <101092510+subin9@users.noreply.github.com>
ArthurZucker
left a comment
There was a problem hiding this comment.
For transformers
TL;DR: we don't want to parse / read the content of the tokenizer.json.
In v5, AutoTokenizer / XTokenizer.from_pretrained gives you that class's tokenizer.
The class defines the normalizer, pre-tokenizer and decoder, and tokenizer.json only supplies the vocab, merges and added tokens. That holds for every model, not just Llama.
If you want exactly what's serialized, use TokenizersBackend.from_pretrained(path), that's the big refacto.
Parsing the tokenizer.json file for llama only:
- Breaks that contract for one model type.
AutoTokenizerandLlamaTokenizer.from_pretrainedwould return different tokenizers for the same repo, and no other model behaves like that. - Hides the real bug: wrong metadata. These checkpoints declare
tokenizer_class: LlamaTokenizerFastbut have byte-level BPE. Meta's own Llama-3 declares PreTrainedTokenizerFast and loads correctly. - Is a heuristic we'd maintain forever. Which components count? Sequence pre-tokenizers? Split + ByteLevel?
THe only "fix" is to add all models that are wrong on the hub path.
For llamacpp
The tokenizer issue: get_vocab_base uses AutoTokenizer to read back what's serialized.
In v5, AutoTokenizer returns the class's pipeline; for example, LlamaTokenizer always installs Metaspace, whatever tokenizer.json says. To read the file as-is, use TokenizersBackend:
Since transformers is pinned to v4, this should help:
from transformers import AutoTokenizer, TokenizersBackend
if (self.dir_model / "tokenizer.json").is_file():
tokenizer = PreTrainedTokenizerFast.from_pretrained(self.dir_model)
else:
tokenizer = AutoTokenizer.from_pretrained(self.dir_model)This fixes most issues.
Some hashes can also be re-computed.
I also advice bumping transformers there. I'll open a PR and check in details but TLDR this PR is not a fix!
|
Your call, and if the answer is still allowlist only, fine. But the bug doesn't go away with it, and two of the reasons above don't match the code. I applied your fix at its fullest: all 1,866 affected repos from the August sweep added to
So the "only fix" leaves every local load broken, including the models already on the list, silently: It also doesn't keep up. Since August, 125 more affected repos (41 orgs), zero matched by the list. Ten random ones all drop spaces, including On "we don't want to read tokenizer.json": Fair enough if you want the class to own the pipeline. That's the part I'd push back on: wrong output with no error is still a bug, and this list can't reach it. If there's a mechanism you'd accept that also covers a local directory, I'll write it. |
|
[For maintainers] Suggested jobs to run (before merge) run-slow: auto |
It's only pinned because v5 breaks.
I don't understand how this would fix anything (mismatching import/use, so probably not what you meant)? |
|
@ArthurZucker Good news: the "heuristic we'd maintain forever" made it into main via #48317, at the same spot in from_pretrained, so the llama.cpp folks should be happy. Since #48317 matches '"ByteLevel"' anywhere in the file rather than in pre_tokenizer/decoder, the "which components count?" question from the review above is still open. Happy to rebase this if a stricter check is wanted. Leaving it open in the meantime. |
|
@subin9 did you read the code changes? _hub_declared_class = tokenizer_config_class or getattr(config, "tokenizer_class", None)
if (
tokenizer_auto_map is None
and TokenizersBackend is not None
and _hub_declared_class is not None
and _hub_declared_class.removesuffix("Fast") == "LlamaTokenizer"
and (TOKENIZER_MAPPING_NAMES.get(config_model_type) or "").removesuffix("Fast") != "MistralCommonBackend"
and TokenizersBackend._tokenizer_json_has_byte_level(pretrained_model_name_or_path, **kwargs)this means:
So no #48317 does NOT matche '"ByteLevel"' anywhere in the file. Now, if you don't want to get banned, please refrain from spamming threads with AI, its tedious and really not fun for us to read 50 lines of bloat. Please make the effort of understanding what you are working on, and minimizing your answers. If there is indeed an issue, and it matters to you, we are more than obliged to help. But it needs to go both ways. |
|
@ArthurZucker If you can make your agent read the function, it would be nice 😉The four points you listed only decide which repos reach the check. The check itself: return '"ByteLevel"' in reader.read()A substring match on the whole file, not |
|
😃 |
Model type "llama" spans both SentencePiece (Llama-1/2) and byte-level (Llama-3 / tiktoken) tokenizers under one Hub tokenizer_class (LlamaTokenizerFast). In v5, LlamaTokenizer.init unconditionally installs a Metaspace pre-tokenizer/decoder, which silently drops spaces for byte-level repos (see #45488), e.g. deepseek-ai/DeepSeek-R1-Distill-Llama-*.
The existing MODEL_IDS_TO_TOKENIZERS_BACKEND allowlist only covers specific checkpoints (e.g. the 8B) and misses others (the 70B is still broken). Instead, for the small set of dual-scheme model types, inspect the serialized tokenizer.json: if it declares a ByteLevel pre_tokenizer/decoder, route to TokenizersBackend (which respects tokenizer.json). SentencePiece Llama-1/2 stays on LlamaTokenizer unchanged.
What does this PR do?
model_type == "llama"covers two incompatible tokenizer schemes under a single Hubtokenizer_class(
LlamaTokenizerFast):▁)deepseek-ai/DeepSeek-R1-Distill-Llama-*) — byte-level BPE (GPT-2 / tiktoken,Ġ)In v5,
LlamaTokenizer.__init__unconditionally installs aMetaspacepre-tokenizer/decoder, overwriting whatevertokenizer.jsondeclares. For byte-level repos this silently drops spaces on both encode and decode(
"Hello world"→"Helloworld"), which is a silent accuracy regression — see #45488.The current mitigation (
MODEL_IDS_TO_TOKENIZERS_BACKEND, from #46091) is a per-checkpoint allowlist. It fixesdeepseek-ai/deepseek-r1-distill-llama-8bbut misses others — for exampleDeepSeek-R1-Distill-Llama-70Bisstill broken on
main(it is not in the list).Instead of enumerating checkpoints, this PR adds a small content-based rule: for the (rare) set of dual-scheme
model types (
{"llama"}), inspect the serializedtokenizer.json; if it declares aByteLevelpre-tokenizer/decoder, route to
TokenizersBackend(which respectstokenizer.json). SentencePiece Llama-1/2(no
ByteLevelintokenizer.json) keeps usingLlamaTokenizerunchanged.LlamaTokenizer.__init__is notmodified, so there is no risk to the SentencePiece path.
This covers any byte-level Llama repo (8B, 70B, future distills) without maintaining a checkpoint allowlist.
Fixes #45488 for Llama-3-derived checkpoints.
Verification
Round-trip on
"Hello world.\nI'm an AI, so I don't have consciousness."(transformersmain):main)DeepSeek-R1-Distill-Llama-70B(not in allowlist)LlamaTokenizer, roundtrip False ("Helloworld.I'manAI.")TokenizersBackend, roundtrip TrueDeepSeek-R1-Distill-Llama-8BTokenizersBackend, TrueTokenizersBackend, TrueNousResearch/Llama-2-7b-hfLlamaTokenizer, TrueLlamaTokenizer, True (unchanged)A no-network unit test for the detection helper is added in
tests/models/auto/test_tokenization_auto.py::AutoTokenizerTest::test_tokenizer_json_is_byte_level.