Skip to content

Forward allowed_special in Llama3Tokenizer.encode - #1121

Open
MohammadHijjawi97 wants to merge 1 commit into
rasbt:mainfrom
MohammadHijjawi97:fix-llama3-tokenizer-allowed-special
Open

MohammadHijjawi97 wants to merge 1 commit into
rasbt:mainfrom
MohammadHijjawi97:fix-llama3-tokenizer-allowed-special

Conversation

@MohammadHijjawi97

Copy link
Copy Markdown
Contributor

ChatFormat.encode() in llms_from_scratch.llama3 (and llms_from_scratch.kv_cache.llama3) takes an allowed_special argument and passes it to Llama3Tokenizer.encode(), but the tokenizer currently drops it via **kwargs and calls tiktoken with its defaults. So text that contains a Llama 3 special token can't be encoded, even when it is explicitly allowed:

tok = Llama3Tokenizer("tokenizer.model")
ChatFormat(tok).encode("Hello", system_message="Hi<|eot_id|>", allowed_special="all")

Before:

ValueError: Encountered text corresponding to disallowed special token '<|eot_id|>'.

After: the special token is encoded as its ID and the call works.

This PR forwards allowed_special to tiktoken in both Llama3Tokenizer classes. It defaults to an empty set, so calls that don't pass it behave as before (special tokens in plain text are still rejected). **kwargs is kept.

I also added a test to pkg/llms_from_scratch/tests/test_llama3.py that builds a tiny byte-level tokenizer file in a temp dir (no download) and checks the default behavior, allowed_special={...} / "all", and ChatFormat.encode(..., allowed_special="all") for both the regular and KV-cache variants. It fails without the change and passes with it.

ChatFormat.encode accepts an allowed_special argument and passes it to
Llama3Tokenizer.encode, but the tokenizer swallowed it via **kwargs and
called tiktoken with its defaults. As a result, text containing a Llama 3
special token (e.g. "<|eot_id|>") always raised a ValueError, even when
allowed_special="all" was passed.

Forward allowed_special to tiktoken (defaulting to an empty set, which
keeps the current behavior when it is not given) in both the regular and
the KV-cache Llama3Tokenizer, and add a test that uses a tiny byte-level
tokenizer file so no download is needed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant