clean up runner code a little

[metascroy]

Cleans up the run.cpp file a bit:

• Builds tokenizer in build_tokenizer instead of main
• Pulls text generation in chat out to generate_from_prompt_tokens. I will refactor generate to use this function too so that both chat/generate share the same code.
• Cleans up CLI a bit.

[mikekgfb]

Let's not call it checkpoint because checkpoint means something else in the rest of the distro. So when we say checkpoint in docs it becomes confusing.

Maybe model_path? Model is also pretty generic so if you come up with something clearer that'd be rad.

[mikekgfb]

Please update docs as appropriate!

[malfet]

Break after return is superfluous, isn't it?

Suggested change

	case 2:
	return ModelType::llama2;
	break;
	case 3:
	return ModelType::llama3;
	break;
	default:
	return ModelType::unknown;
	case 2:
	return ModelType::llama2;
	case 3:
	return ModelType::llama3;
	default:
	return ModelType::unknown;

[metascroy]

It is, but I intentionally add break statements after blocks in case they are edited in future to no longer return. It's to similar to why I always write:

if (condition) {
    do_one_line();
}

instead of:

if (condition)
    do_one_line();

Happy to change it, though.

[malfet]

Is there a situation when pos can be negative?

Suggested change

	int pos = 0;
	unsigned pos = 0;

[malfet]

Steps is a positive number, isn't it?

Suggested change

	int steps,
	unsigned steps,

[malfet]

One does not mutate a tokenizer state, does it?

Suggested change

	Tokenizer* tokenizer,
	const Tokenizer* tokenizer,

[metascroy]

Many methods in tokenizer are not defined const (e.g., encode) so this does not compile. We could go through and clean up the tokenizer classes, but I want to keep this contained to run.cpp for now.

[malfet]

C++y your code a bit (NULL is not defined by C++ standard afaik)

Suggested change

	char* checkpoint_path = NULL;
	char* tokenizer_path = NULL;
	char* checkpoint_path = nullptr;
	char* tokenizer_path = nullptr;

[malfet]

Nit (again, if this is C++ code, why not use auto?

Suggested change

	Tokenizer* tokenizer =
	build_tokenizer(tokenizer_path, model_type, vocab_size);
	auto tokenizer =　build_tokenizer(tokenizer_path, model_type, vocab_size);

[metascroy]

@mikekgfb are you OK with using auto, std::unique_ptr, and other modern c++ features in run.cpp. Originally the code started out as C, but the linked runner and now the linked tokenizers are not C, so I'm not sure what standard we're trying to target.

[malfet]

Raw pointers are so C/C++03, please use managed pointers

Suggested change

	Tokenizer* build_tokenizer(
	std::unique_ptr<Tokenizer> build_tokenizer(

[metascroy]

I wasn’t sure what C++ standard to target because originally the code was in C. @mikekgfb are you OK with using unique_ptr.

[malfet]

If one to use managed pointers, one should never worry about deleting objects

Suggested change

	void free_tokenizer(Tokenizer* tokenizer) {
	delete tokenizer;
	}

[malfet]

Calling exit from anything but main and parse_args is mauvais ton. Please raise an exception and then if you wish catch it in main

Suggested change

	fprintf(stderr, "No tokenizer defined for model type %d", model_type);
	exit(EXIT_FAILURE);
	raise std::runtime_error("No tokenizer defined for model type" + std::to_string(model_type));