MPT: tokenization crashes

[jploski]

Testing against 5974d61 and #3538

While running

bin/main --mlock -m /mnt/f2fs/mpt/ggml-model-mpt-7b-storywriter-f16-q5_1.gguf -t 1 -ngl 999 -p 'Once upon a time' --temp 0.8 --top_p 0.98 -c 2048 --keep -1 --repeat_penalty 1 -n 1024

It consistently crashes after a few (~hundred) tokens with this backtrace:

Thread 1 "main" received signal SIGABRT, Aborted.
__GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
50	../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1  0x00007fffcefae535 in __GI_abort () at abort.c:79
#2  0x00007fffcf376983 in ?? () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#3  0x00007fffcf37c8c6 in ?? () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#4  0x00007fffcf37c901 in std::terminate() () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#5  0x00007fffcf37cb34 in __cxa_throw () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#6  0x00007fffcf37886b in ?? () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#7  0x0000555555612915 in std::__detail::_Map_base<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, unsigned char>, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, unsigned char> >, std::__detail::_Select1st, std::equal_to<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::hash<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<true, false, true>, true>::at (this=0x555555b2fa20 <unicode_to_bytes_bpe(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)::map>, __k=" ")
    at /usr/include/c++/8/bits/hashtable_policy.h:760
#8  0x000055555560b295 in std::unordered_map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, unsigned char, std::hash<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::equal_to<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, unsigned char> > >::at (this=0x555555b2fa20 <unicode_to_bytes_bpe(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)::map>, __k=" ") at /usr/include/c++/8/bits/unordered_map.h:991
#9  0x00005555555cc65a in unicode_to_bytes_bpe (utf8=" ") at /mnt/seagate/dalai/llama.cpp.bak/unicode.h:460
#10 0x00005555555f8398 in llama_decode_text (text="  ") at /mnt/seagate/dalai/llama.cpp.bak/llama.cpp:9703
#11 0x00005555555f8663 in llama_token_to_piece (model=0x5555a4966c40, token=50276, buf=0x555555e3c6b0 "", length=8) at /mnt/seagate/dalai/llama.cpp.bak/llama.cpp:9746
#12 0x000055555557e0ce in llama_token_to_piece[abi:cxx11](llama_context const*, int) (ctx=0x5555b579e350, token=50276) at /mnt/seagate/dalai/llama.cpp.bak/common/common.cpp:894
#13 0x00005555555b028b in llama_sampling_sample (ctx=0x5555b579e350, ctx_guidance=0x0, ctx_sampling=..., last_tokens=std::vector of length 2048, capacity 2048 = {...}, 
    candidates=std::vector of length 50432, capacity 50432 = {...}, idx=0, seq=0) at /mnt/seagate/dalai/llama.cpp.bak/common/sampling.cpp:151
#14 0x000055555556a2aa in main (argc=22, argv=0x7fffffffdcd8) at /mnt/seagate/dalai/llama.cpp.bak/examples/main/main.cpp:648

(The GGUF/vocab was exported using convert-mpt-hf-to-gguf.py from the aforementioned commit.)

[jploski]

I'm also seeing the same crash for mpt-30b-instruct (which is no wonder, same vocab/tokenizer).

[jploski]

@goerch Please see above. With this "obvious" hack/fix it does not crash:

diff --git a/unicode.h b/unicode.h
index aeca879..eaf7134 100644
--- a/unicode.h
+++ b/unicode.h
@@ -452,6 +452,7 @@ static std::unordered_map<std::string, uint8_t> unicode_to_bytes_map_bpe() {
             ++n;
         }
     }
+    map[codepoint_to_utf8(u' ')] = ' ';
     return map;
 }

But I don't understand how the particular hardcoded character ranges in unicode_to_bytes_map_bpe were selected, so not sure if this is the correct approach.

[staviq]

Testing against 5974d61 and #3538

I'm confused, #3538 didn't touch any of the code from that backtrace, and isn't even merged yet ?

[cebtenzzre]

This is because of the reasons described in #3405. I have not yet identified how to port the changes in that pull request to goerch's new tokenizer code. How do we currently differentiate between normal and added tokens for MPT in the C++ code?

[staviq]

@cebtenzzre

Looking at unicode.h

static uint8_t unicode_to_bytes_bpe(const std::string & utf8) {
    static std::unordered_map<std::string, uint8_t> map = unicode_to_bytes_map_bpe();
    return map.at(utf8);
}

There is no validation before .at()

I think it should be at least something along the lines of:

static uint8_t unicode_to_bytes_bpe(const std::string & utf8) {
    static std::unordered_map<std::string, uint8_t> map = unicode_to_bytes_map_bpe();
    return map.find(utf8) != map.end() ? map.at(utf8) : "";
}

[cebtenzzre]

I think it should be at least something along the lines of:

I think that's wrong. This code is assuming that all tokens in the vocabulary are specially encoded so they are valid JSON strings (which makes me wonder why we are now saving them this way in the GGUF files, as GGUF does not have the same restrictions). But added tokens are not specially encoded, so we must not call unicode_to_bytes_bpe on them. All normal tokens are guaranteed to be encoded this way.

[staviq]

@cebtenzzre I meant not as a fix but in general, the map doesn't contain all possible strings so map.at() will crash if something not in the map gets passed as the argument. It's not that important, doesn't seem to be the cause.

I sort of think I know what might be going on here

Unicode codepoint for space is the same as ascii 0x20 (32)

unicode_to_bytes_bpe crashes when a space is passed as an argument, because map doesn't have anything under that index

When the map is created, space is added by the last for loop, at index with offset of 256:

https://github.com/ggerganov/llama.cpp/blob/1e0e873c373c33989beb6bc64d83cd572ab7fe2b/unicode.h#L434-L456

Except that space comes from codepoint_to_utf8 in

https://github.com/ggerganov/llama.cpp/blob/1e0e873c373c33989beb6bc64d83cd572ab7fe2b/llama.cpp#L9437

Which doesn't use any offsets and returns a normal space for that codepoint:

https://github.com/ggerganov/llama.cpp/blob/1e0e873c373c33989beb6bc64d83cd572ab7fe2b/unicode.h#L225-L229

But tracing this back even further, the " " comes directly from the vocab / token.text, except I understand space in BPE vocab should be "Ġ" ( that's utf 256+32 )?

Tracing it even further back, wouldn't it mean convert-mpt-hf-to-gguf.py imports tokens ( their "text" ) wrong ?

@cebtenzzre, is that what you meant ? Am I understanding this correctly ?

Even if I'm wrong here, I think this bit of code could really use some, even rudimentary, comments

[cebtenzzre]

Tracing it even further back, wouldn't it mean convert-mpt-hf-to-gguf.py imports tokens ( their "text" ) wrong ?

Not anymore, because goerch recently moved all of the vocab conversion to llama.cpp. It is no longer the conversion script's responsibility. The different parts of the JSON have different ways of encoding the vocabulary - added tokens are stored as-is, whereas normal tokens need bytes_to_unicode/unicode_to_bytes. My PR changed the conversion script to only decode the normal tokens, but we no longer decode any tokens in the conversion script, so that will not work.

I believe in the map.at(utf8) call, the input comes directly from the vocabulary, so it should only crash if the code is incorrect (which it is in this case, because we should not call this function on added tokens) or if the GGUF file is corrupt/incorrectly converted. I don't think silently generating blank output tokens is a good idea in either of those cases.

[staviq]

I was testing something else when I noticed the entire vocab for mpt-7b-storywriter is marked as LLAMA_TOKEN_TYPE_NORMAL when it is loaded in llm_load_vocab, so just to be sure, I re-converted (and quantized) again with current master (11dc109) and it's the same.

So if all (BPE) tokens are marked as normal in the .gguf, how can we tell which tokens should get the bytes_to_unicode/unicode_to_bytes treatment ?

In case of mosaicml/mpt-7b-storywriter, special/added tokens are only listed in tokenizer.json, except during the conversion, that json file seems to only be used (in gguf.py) to get IDs for special_token_types: tuple[str, ...] = ('bos', 'eos', 'unk', 'sep', 'pad')

I'm not sure if I'm missing something or I got this wrong.

[jploski]

I was testing something else when I noticed the entire vocab for mpt-7b-storywriter is marked as LLAMA_TOKEN_TYPE_NORMAL when it is loaded in llm_load_vocab, so just to be sure, I re-converted (and quantized) again with current master (11dc109) and it's the same.

So if all (BPE) tokens are marked as normal in the .gguf, how can we tell which tokens should get the bytes_to_unicode/unicode_to_bytes treatment ?

In case of mosaicml/mpt-7b-storywriter, special/added tokens are only listed in tokenizer.json, except during the conversion, that json file seems to only be used (in gguf.py) to get IDs for special_token_types: tuple[str, ...] = ('bos', 'eos', 'unk', 'sep', 'pad')

I'm not sure if I'm missing something or I got this wrong.

I suspect you're right and the convert scripts (not just for MPT, but for any model with BPE tokenizer) should be updated to classify tokens (based on #3538 (comment)) as CONTROL (if found in added_tokens and special: true in tokenizer.json), USER_DEFINED (if found in added_tokens and special: false in tokenizer.json), UNUSED (if artificially added just to make vocab match model.vocab_size like the MPT pad tokens), NORMAL otherwise.

But as it stands, CONTROL is used for all BPE added_tokens in convert.py (regardless of the "special" flag)... and all the other convert scripts just output all tokens as NORMAL...

[ggerganov]

Catching up with PRs - did I merge #3538 too soon? Sorry if that's the case.
AFAIU, we need to implement the token classification as explained in @jploski comment above, correct?

[jploski]

Catching up with PRs - did I merge #3538 too soon? Sorry if that's the case. AFAIU, we need to implement the token classification as explained in @jploski comment above, correct?

I don't think the merge was too soon - at least in the sense that it doesn't make matters worse than they were without the merge. The bug is in most likely in the convert scripts, and these was already merged in earlier (I suspect I didn't catch the crash in my prior MPT testing because I was not generating random enough content to cause the special token to appear in the output).

I think it would make sense if @goerch also had a look at this issue - because convert-mpt was essentially me copying the convert-gptneox script with small adjustments.

[staviq]

Catching up with PRs - did I merge #3538 too soon? Sorry if that's the case. AFAIU, we need to implement the token classification as explained in @jploski comment above, correct?

I verified this is unrelated, forgot to post that info, it was reproducible on master prior to #3538 merge

[goerch]

With #3728 and

build/bin/Release/main.exe" -m models\mpt-7b-storywriter\ggml-model-f16.gguf -ngl 999 -p 'Once upon a time' --temp 0.8 --top_p 0.98 -c 2048 --keep -1 --repeat_penalty 1 -n 1024

I get

system_info: n_threads = 6 / 12 | AVX = 0 | AVX2 = 0 | AVX512 = 0 | AVX512_VBMI = 0 | AVX512_VNNI = 0 | FMA = 0 | NEON = 0 | ARM_FMA = 0 | F16C = 0 | FP16_VA = 0 | WASM_SIMD = 0 | BLAS = 0 | SSE3 = 0 | SSSE3 = 0 | VSX = 0 |
sampling:
        repeat_last_n = 64, repeat_penalty = 1.000, frequency_penalty = 0.000, presence_penalty = 0.000
        top_k = 40, tfs_z = 1.000, top_p = 0.980, typical_p = 1.000, temp = 0.800
        mirostat = 0, mirostat_lr = 0.100, mirostat_ent = 5.000
generate: n_ctx = 2048, n_batch = 512, n_predict = 1024, n_keep = 4


Once upon a time there was a girl who went by the name of Della.

**Della** , the girl who had come to live in a place called the Mansion. The girl who got herself into trouble and was now in trouble. The girl who couldn't tell what was real and what wasn't.

She was also the girl who had found a secret.

She couldn't seem to get away from it, even now, in the middle of the night.

Della wasn't afraid of a haunted house. Not really.

Della had lived in haunted houses all her life. The mansion and its secrets were just the latest haunted house Della had found herself in.

Della felt tired. She lay in the bed in the nursery, watching the door. "Hello?" she said. "Anybody home?"

Then she heard a voice.

"I heard you! I heard you!"

"Who's there?" Della whispered, but there was no answer.

She felt a presence.

"Hello?" Della called out. "Are you okay? Am I dreaming?"

"I'm not dreaming," the voice said. It was a ghost's voice.

Della looked around the room, at the toys on the shelves, the pictures on the walls.

In the middle of the room, under the window, a shadow appeared. It was dark and shaped like a woman.

"Who are you?" Della asked, and then she heard the ghost's response.

"I'm your guardian angel," the ghost whispered, and Della realized that she was not alone.

"Guardian angel?" Della asked. "What are you doing here?"

"I'm here to protect you," the ghost said.

Della stood up and looked at the shadow. It stood next to the crib.

"You're my guardian angel?" Della asked. "How old are you?"

"I'm as old as the oldest ghost," the ghost said, "and younger than the newest ghost."

"Okay," Della said. "I guess that makes sense. Does that mean you're a little bit of a ghost?"

"I'm a little bit of a ghost," the ghost said, "and a little bit of a girl. I was a girl once. Now I'm a ghost."

Della looked at the guardian angel's body, then at her face. She was wearing a long white dress with lace at the wrists and at the collar.

Della was a little afraid. "You're a ghost, but your body is not a ghost's body."

"I'm not a ghost," the guardian angel said. "But I'm like a ghost. I'm not alive. I'm not dead. I'm the living dead."

Della frowned. "Then what are you?"

"I'm a ghost with a ghost body," the guardian angel said. "I'm a ghost with a ghost body that looks like a body, but it's not alive."

Della frowned. "Why are you here?"

"I want to help you," the ghost said, "because I love you."

Della frowned. "I don't love you."

"You do," the ghost said. "I can tell. You love me."

"I don't love you," Della said.

"You love me," the ghost said. "I can feel it."

"You're crazy," Della said.

"I'll prove it," the ghost said. "I'll tell you the truth about who you are."

Della frowned. "Why are you telling me the truth?"

"Because you're in so much trouble," the ghost said. "You need to know the truth."

Della looked at the guardian angel's face. "You can't know the truth."

"Yes, I can," the ghost said.

Della looked at the guardian angel's face. "You're the only one who can tell me the truth."

"I can tell you the truth," the ghost said, "because I know everything."

Della's eyes widened. "You do?"

"Yes," the guardian angel said. "I know everything."

Della frowned. "You're lying."

"No," the ghost said. "I'm not a ghost. I'm not a girl. I'm a ghost with a ghost body. I'm a ghost with a ghost body and I know everything."

Della's eyes narrowed. "You mean you don't know everything."

"No," the ghost said, "but I know everything."

Della squinted. "You can't know everything."
llama_print_timings:        load time =    5862.36 ms
llama_print_timings:      sample time =      56.62 ms /  1024 runs   (    0.06 ms per token, 18084.84 tokens per second)
llama_print_timings: prompt eval time =    6125.56 ms /     4 tokens ( 1531.39 ms per token,     0.65 tokens per second)
llama_print_timings:        eval time = 2244789.36 ms /  1023 runs   ( 2194.32 ms per token,     0.46 tokens per second)
llama_print_timings:       total time = 2251565.26 ms
Log end

Not sure if we have a different problem, though.

[staviq]

@goerch With that particular model, you have to let it spin for quite a while sometimes to trigger the problem, and at some point it will produce token ( double space ), which causes the crash, because those spaces make to the detokenizer as ascii space 0x20 instead of expected 0xff+0x20.