I would like to avoid adding a second diffusion example - we are increasing the maintenance efforts for not significant benefit. The diffusion architecture is not yet well established.

We can think about extending the llama_sampler functionality to support these use cases and since it is already modular it would make more sense to implement the sampling logic there. Ideally the diffusion CLI example would be just one for all diffusion models, with different samplers attached.

I would like to avoid adding a second diffusion example - we are increasing the maintenance efforts for not significant benefit. The diffusion architecture is not yet well established.

We can think about extending the llama_sampler functionality to support these use cases and since it is already modular it would make more sense to implement the sampling logic there. Ideally the diffusion CLI example would be just one for all diffusion models, with different samplers attached.

Yeah agree, I initially wrote them to be one example. However, passing arguments via CLI for two separate sets of sampling parameters/algorithms was quite confusing to me and would be even more so for the end-user, so for the sake of clarity I wrote them separately.
diffusion_generate_dream and diffusion_generate_llada are two different functions with the same outline, decode => sample => unmask, so there is an abstraction to be made, the only thing is to clarify is how we pass separate sets of parameters to the example without overloading the same thing (e.g. --diffusion-algorithm being supported in dream but not llada and vice versa), llama_sampler be used also, but I don't see how it would solve this particular problem

@ggerganov would having them in the same example and having extra CLI args for models be acceptable?

Yes, merging the examples into a single example would be better.

Yes, merging the examples into a single example would be better.

Made everything into a single example, please have another look when you have the time

@ggerganov you're right, we can combine the sampling methods. I was under the assumption that the only sampling methods that would work are their respective paper implementations, but I tried various sampling methods on both models and they seem to have coherent outputs, but I did not do any deep correctness checks.

Refactored to have a concept called schedule which is either timestep based (like dream) or block-based (like LLaDA). Both work for both models. Also refactored the sampling methods to be the same across the models.

The issues that do remain however,

1. Shifted logits - logits in Dream are shifted by -1 after a pp path, which is not the case in LLaDA. Ideally this should be a part of the GGUF, but I'm not sure.
2. The BOS token in LLaDA - add_bos_token is false in tokenizer_config.json, I think because the chat_template contains the bos_token.

"chat_template": "{% set loop_messages = messages %}{% for message in loop_messages %}{% set content = '<|start_header_id|>' + message['role'] + '<|end_header_id|>\n\n'+ message['content'] | trim + '<|eot_id|>' %}{% if loop.index0 == 0 %}{% set content = bos_token + content %}{% endif %}{{ content }}{% endfor %}{{ '<|start_header_id|>assistant<|end_header_id|>\n\n' }}",

However, this code removes this BOS

I'm not familiar with chat-template code and I was not able to work around this without adding a bos token

2. The BOS token in LLaDA - `add_bos_token` is false in `tokenizer_config.json`, I think because the chat_template contains the `bos_token`.

No, add_bos_token only applies to untemplated generation, it seems like a mistake. It was removed in LLaDA 1.5 chat template BTW.

Edit: Nvm, I'm blind, it's still there.

However, this code removes this BOS

llama.cpp/common/chat.cpp

Lines 746 to 755 in c35f9ea

	minja::chat_template_options tmpl_opts;
	// To avoid double BOS / EOS tokens, we're manually removing begining / trailing tokens
	// instead of using `chat_template_options.use_bos_token = false`, since these tokens
	// may be needed inside the template / between messages too.
	auto result = tmpl.apply(tmpl_inputs, tmpl_opts);
	if (string_starts_with(result, tmpl.bos_token())) {
	result = result.substr(tmpl.bos_token().size());
	}
	if (string_ends_with(result, tmpl.eos_token())) {
	result = result.substr(0, result.size() - tmpl.eos_token().size());

This probably needs to be improved.

I'm not familiar with chat-template code and I was not able to work around this without adding a bos token

Setting add_bos_token to True on conversion should fix that, but only applies to pre-1.5 models.

Setting add_bos_token to True on conversion should fix that, but only applies to pre-1.5 models.

Yep, this fixes it for regenerated gguf. Though it might be a problem downstream if people use the HF repo to create quants (unless they patch this in the HF repo)

I just tested LLaDA-1.5 btw, works great! :)

I just tested today and got a crash. As I understand this is still in early development, I'm not going to bother you with another issue, as the problem might already be known or expected. In short I've built llama.cpp at commit 5c0eb5e ("opencl: fix adreno compiler detection logic (#15029)") like I always do:

$ cmake -B build -DBUILD_SHARED_LIBS=OFF -DGGML_OPENMP=OFF  && cmake --build build --config Release -j $(nproc)

and tried the exact command above with the model linked above:

 ./build/bin/llama-diffusion-cli -m /mnt/models/llada-8b.gguf -p "Lily can run 12 kilometers per hour for 4 hours. After that, she runs 6 kilometers per hour. How many kilometers can she run in 8 hours?" --diffusion_steps 128 -ngl 99 --temp 0  -ub 128 --diffusion-visual

and got that one:

...
llama_context:        CPU  output buffer size =     0.48 MiB
/home/willy/ai/llama.cpp/examples/diffusion/diffusion-cli.cpp:616: GGML_ASSERT((params.diffusion.eps == 0) ^ (params.diffusion.block_length == 0)) failed
[New LWP 7410]
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/aarch64-linux-gnu/libthread_db.so.1".
0x0000fa000fe66800 in __GI___wait4 (pid=<optimized out>, stat_loc=0x0, options=0, usage=0x0) at ../sysdeps/unix/sysv/linux/wait4.c:30
30      ../sysdeps/unix/sysv/linux/wait4.c: No such file or directory.
#0  0x0000fa000fe66800 in __GI___wait4 (pid=<optimized out>, stat_loc=0x0, options=0, usage=0x0) at ../sysdeps/unix/sysv/linux/wait4.c:30
30      in ../sysdeps/unix/sysv/linux/wait4.c
#1  0x0000bbeeac7290b4 in ggml_print_backtrace ()
#2  0x0000bbeeac729244 in ggml_abort ()
#3  0x0000bbeeac480970 in main ()
[Inferior 1 (process 7409) detached]
Aborted (core dumped)

It wasn't built with debug symbols so the backtrace is not more precise. If this is news to you and you'd prefer getting an issue, let me know, and I'll do it. But logically it is trivially reproducible ;-)

@wtarreau you need to either supply --diffusion-block-length or --diffusion-eps, I recommend --diffusion-block-length 32 for LLaDA models. Might be useful to have a default value here which I'll look into

Confirmed indeed, I knew it was not necessary to create an issue ;-) Indeed, a default value would be useful, in general a program shouldn't crash due to missing cmdline args, at worst it should complain about their lack.

IMHO you should edit your first comment above to add this missing argument. Your short howto is super useful to try the feature given that you've provided the gguf file as well! Thanks!

I find the output a bit slow, but I couldn't time it, the (remote) machine hung, I'll have to time it on monday after I can reboot it :-)