llama : fix tokenizer

[goerch]

This PR addresses @vjeux 's comment. The proposed changes are necessary to see reasonable results for the attached test cases.

To further support is_unknown, is_control, is_byte and is_unused and more special cases it seems reasonable (or necessary?) to extend the binary vocabulary format.

[ggerganov]

Hm, do I understand correctly, that even the simplest prompt of Hello world does not currently tokenize correctly on master?

If this is the case, this should be come very high priority.

[goerch]

@howard0su : thanks for your work on the tokenizer! I took the liberty to merge the relevant parts for the work on #2310. Would you like to check, if this is as envisioned by you?

[goerch]

@ggerganov , @howard0su and probably others: need guidance with a couple of points:

• What should we do about the buffer reservation for tokenization? I estimate the need for two times the string size in the worst case?
• What do we do with llama_token_to_str returning a std::string due to white space unescaping?
• How should the unescaped tokens be displayed?

[slaren]

What should we do about the buffer reservation for tokenization? I estimate the need for two times the string size in the worst case?

llama_tokenize returns the negated number of tokens, so that could be used to resize the buffer as needed, but allocating twice the string size would be an inconsequential amount of memory anyway, so I don't think it really matters.

What do we do with llama_token_to_str returning a std::string due to white space unescaping?

I think we don't make any specific guarantees about thread-safety, but generally it would be good to assume that each instance of llama_model, or any of the objects, should be thread safe with each other. So I think that only leaves two options:

• Allocate a buffer in llama_model and return that
• Take a buffer from the user

[vjeux]

Hm, do I understand correctly, that even the simplest prompt of Hello world does not currently tokenize correctly on master?

This changeset is misleading. There are two words and each have a different explanation.

for “Hello”, for a reason I don’t understand sentencepiece (the tokenizer used by llama) is adding a space at the beginning of every string that is tokenized. But it wasn’t the case in the way the test case was called (which I don’t know if it is the way it is called in “production”). This needs to be added in order to match what llama does. Note that it’s only going to change the very first token of the very first word so isn’t going to be a big deal for normal LLM use cases.

for “world” the capitalization got changed from “World” to “world”. So it is expected that the token number is different. The previous one has the correct token id as far as I can tell.

I need to check more of the inputs previously but it’s not as dire as what this change makes it look like.

[goerch]

I need to check more of the inputs previously but it’s not as dire as what this change makes it look like.

I see. So you think the change of token texts in convert.py

         text: bytes
            if tokenizer.is_unknown(i):
                text = " \u2047 ".encode("utf-8")
            elif tokenizer.is_control(i):
                text = b""
            elif tokenizer.is_byte(i):
                piece = tokenizer.id_to_piece(i)
                if len(piece) != 6:
                    raise Exception(f"Invalid token: {piece}")
                byte_value = int(piece[3:-1], 16)
                text = struct.pack("B", byte_value)
            else:
                text = tokenizer.id_to_piece(i).replace("\u2581", " ").encode("utf-8")

is the way to go? Does something like test-tokenizer-1 of the PR work for you? Does size(vocab.id_to_token) == size(vocab.token_to_id) (=32000 for llama) hold for you?

[vjeux]

All the tests are looking good to me now. Thanks for looking into it!

[ggerganov]

Is there a way to fix this without changing the convert.py script?
Otherwise, all ggml models would become obsolete after this change

[klosax]

There is a PR #1931 that may contain fixes and additions that should be included here to be merged to the gguf branch.

[klosax]

I think this should be merged to the gguf branch since the gpt2 tokenizer that was added there may have functions that could be reused by the llama tokenizer. There is also a unicode implementation that could be reused. We could even make a unified tokenizer library supporting both gpt2 and llama. In the future we would probably need to add the replit sentencepiece tokenizer and others as needed.

[goerch]

There is a PR #1931 that may contain fixes and additions that should be included here to be merged to the gguf branch.

PR #2053 could be related too (#2420 mentions Aquila and xgen). My biggest concern currently is that the only way I know how to distinguish tokenizers is by size of the vocabulary, Do you know of any better way?

[klosax]

The tokenizers can be distinguished by tokenizer_class in tokenizer_config.json

https://huggingface.co/BAAI/Aquila-7B/blob/main/tokenizer_config.json
https://huggingface.co/Salesforce/xgen-7b-8k-base/blob/main/tokenizer_config.json

Xgen uses tiktoken according to the model card,

[goerch]

The tokenizers can be distinguished by tokenizer_class in tokenizer_config.json

OK, if I understand you correctly I should extend convert.py to transfer the tokenizer_class into the ggml format? I could try that, but it might be a duplication of efforts with the gguf branch. I can only speculate which way forward (this PR or a corresponding one against gguf) gets the better test coverage?

[goerch]

I think this should be merged to the gguf branch since the gpt2 tokenizer that was added there may have functions that could be reused by the llama tokenizer. There is also a unicode implementation that could be reused. We could even make a unified tokenizer library supporting both gpt2 and llama. In the future we would probably need to add the replit sentencepiece tokenizer and others as needed.

OK. I'll look into it (although I'm not fully convinced: I once learned to fix bugs first).

[klosax]

This is the gpt2 tokenizer in gguf (it should probably have a better structure and unified with the llama tokenizer):
https://github.com/ggerganov/llama.cpp/blob/gguf/cmpnct_gpt2bpe.hpp

And here is the gpt-neox example using it:
https://github.com/ggerganov/llama.cpp/blob/gguf/gptneox-main.cpp

[ggerganov]

I think this should be merged to the gguf branch

Yes, let's look into merging this in the gguf branch

[klosax]

#2553 unicode fixes

[goerch]

Merged via gguf branch.