vocab: refactor tokenizer to reduce the overhead of creating multi times tokenizer

[kylo5aby]

ref #9369

• I have read the contributing guidelines
• Self-reported review complexity:
  • Low
  • Medium
  • High

the PR mainly does:

• create a tokenizer object after initialize the llama_vocab
• put the initialization of mutable data of each tokenizer object in llama_tokenize_internal for thread safe

[ngxson]

This technically works, but IMO we could improve it a bit:

Currently, llm_tokenizer_data to store working data and llm_tokenizer to store "fixed" shared data. However, because tokenize() is still inside llm_tokenizer, there is nothing prevent it from mutating data inside llm_tokenizer (which is not desirable).

A better solution would be:

• Move tokenize() function to llm_tokenizer_data (maybe change the class name to llm_tokenizer_session to reflect that the object is short-loved)
• tokenize() take const llm_tokenizer * as argument. The const is to make sure that the shared object is read-only.

[kylo5aby]

This technically works, but IMO we could improve it a bit:

Currently, llm_tokenizer_data to store working data and llm_tokenizer to store "fixed" shared data. However, because tokenize() is still inside llm_tokenizer, there is nothing prevent it from mutating data inside llm_tokenizer (which is not desirable).

A better solution would be:

• Move tokenize() function to llm_tokenizer_data (maybe change the class name to llm_tokenizer_session to reflect that the object is short-loved)
• tokenize() take const llm_tokenizer * as argument. The const is to make sure that the shared object is read-only.

@ngxson Thanks your feedback, what approach do you think is better:

1. declare tokenize in llm_tokenizer_session, and pass const llm_tokenizer * tokenizer, and call tokenizer's related method in the inner, that means the logic implementation(eg. tokenize, append) is still belong to every llm_tokenizer. For example:

struct llm_tokenizer_bpe_session : llm_tokenizer_session {

    void tokenize(const llm_tokenizer * tokenizer, const std::string & text, std::vector<llama_vocab::id> & output) {
        tokenizer->tokenize(text, *this, output);
    }

    std::vector<llm_symbol> symbols;
    std::vector<llm_symbol> symbols_final;
    llm_bigram_bpe::queue work_queue;
};

2. Keep the structures as in current PR, and declare all methods as const for every llm_tokenizer , so the shared data in every llm_tokenizer are not allowed to modify. That maybe bring smaller change.

3. move all the logic implementation from every llm_tokenizer to llm_tokenizer_session, the llm_tokenizer will only contains shared data, every time want to use tokenize operations, it must create a llm_tokenizer_session, which maybe bring a break change.

[ngxson]

The 3rd option should be the proper way. The idea is:

• llm_tokenizer contains only fixed/shared data. It will be initialized with the model, stored in model.tokenizer
• Functions using the tokenizer like llama_tokenize_impl, llama_token_to_piece_impl, llama_detokenize_impl will take const llm_tokenizer & as input. They no longer take const vocab as a param because vocab is already inside const llm_tokenizer

every time want to use tokenize operations, it must create a llm_tokenizer_session, which maybe bring a break change.

I don't see why it's a breaking change compared to what you're currently doing with llm_tokenizer_data. Every time the tokenizer is used, a new llm_tokenizer_session must be created.

The constructor of the session will look like: llm_tokenizer_session(const llm_tokenizer & tokenizer)

[ngxson]

Suggested change

	GGML_ABORT("fatal error");
	GGML_ABORT("unknown vocab type");

[ngxson]

create would be a more appropriate name:

Suggested change

	llm_tokenizer * llama_get_tokenizer(const llama_vocab & vocab) {
	llm_tokenizer * llama_create_tokenizer(const llama_vocab & vocab) {

[ngxson]

Suggested change

	tokenizer = new llm_tokenizer_spm(vocab);
	tokenizer = new llm_tokenizer_spm(vocab);

[ggerganov]

@kylo5aby Are you interested in adding a test that accepts a vocab file (see ./models/ggml-vocab*.gguf and tokenizes random strings in parallel on multiple threads? The test-log test can be used as a starting point. The goal is to run it through thread sanitizer in order to guarantee thread-safety of the tokenization API.

[ggerganov]

Suggested change

	llm_tokenizer_wpm(const llama_vocab & vocab): llm_tokenizer(vocab) {}
	llm_tokenizer_wpm(const llama_vocab & vocab) : llm_tokenizer(vocab) {}

[ggerganov]

Suggested change

	llm_tokenizer_bpe(const llama_vocab & vocab): llm_tokenizer(vocab) {
	llm_tokenizer_bpe(const llama_vocab & vocab) : llm_tokenizer(vocab) {

[ggerganov]

Let's improve this a bit further - looking at your changes I realized we don't need to create a separate test. We can simply extend the existing test-tokenizer-0 to become multi-threaded. You've pretty much done it in test-tokenizer-parallel.cpp, but just need to store the results and print them to stdout/stderr after joining the threads. We also want to keep support for optional file tokenization at the end - this remains single-threaded.

[kylo5aby]

Let's improve this a bit further - looking at your changes I realized we don't need to create a separate test. We can simply extend the existing test-tokenizer-0 to become multi-threaded. You've pretty much done it in test-tokenizer-parallel.cpp, but just need to store the results and print them to stdout/stderr after joining the threads. We also want to keep support for optional file tokenization at the end - this remains single-threaded.

how about introduce a mutex to make the printing of results orderly?