DRAFT: PR to check diff between mamba2-sync and Falcon-H1

[younesbelkada]

No description provided.

[younesbelkada]

The only open question I have on my end is how we can refactor this so that it can also accept models where we have attn and ssm - do you have any idea how we can tackle this @gabe-l-hart ? 👀

[gabe-l-hart]

I think this should already be doable, but it will need to be done in llama_model::create_memory. My intent with the refactor starting here is that there should be default behavior for models based on llm_arch_is_recurrent and llm_arch_is_hybrid_recurrent, but that individual models that need special initialization can add themselves to the switch statement before the default and initialize the correct kind of memory for themselves (although, it would be even nicer if there were simply a way to override create_memory for a given model type's builder class).

For H1, I think you would essentially copy the clause used to initialize the hybrid recurrent cache (here) and use the option to add overriding filters to make "always on" filters for both child cache types.

[younesbelkada]

Thank you for the pointer @gabe-l-hart ! Will look into that

[younesbelkada]

Also this is hard-coded here we'll need to change it

[gabe-l-hart]

My goal with hard-coding this here was to make this a "sane default" for models that don't have specific override logic. For Granite 4, I overwrite this here when parsing hparams.

[gabe-l-hart]

Depending on what the core maintainers say about the addition of these new hparams for H1, I think the best approach here would to simply check if hparams.mlp_gate_multiplier is 0 rather than doing an architecture check. I think in general, the repo is trying to consolidate the number of places that individual architecture enums are checked in order to slowly encapsulate the logic for a given architecture into a self-contained class (this is speculation, but it looks like the direction that things are headed).

[younesbelkada]

Makes sense, will do that

[gabe-l-hart]

This is definitely interesting. My guess is that there will be some pushback on adding this many new hparams to the central struct that are specific to a single architecture. Many of these appear to have analogous params elsewhere in the struct, so the general preference is to reuse existing params (even if their names don't match with the HF names) and then do the name translation in the conver_hf_to_gguf.py / gguf-py portion.

[gabe-l-hart]

I'm in the same boat for Granite 4 where I've copy-pasted this build_mamba2_layer method. I've tried several other ways of making things more composable, but they each have their downsides:

1. Making build_mamba_model::build_mamba*_layer methods static and then passing this when calling them. This is nice because other model builders can call them by simply calling build_mamba_model::build_mamba2_layer(this, ..., but it makes the method signature gross and causes the "child model type" to know about the "parent model type" which is a bit of an inversion of how the logical composition should go.
2. Create a special base class that doesn't inherit from llm_graph_context as a mixin (llm_build_mamba_mixin) where it is initialized with a llm_graph_context * called self and then uses that in the implementations of the layer builders. This is nicer when using them from builders that need them because you simply have to inherit from the mixin class, initialize it with this, then call the method when needed like normal. The downside is that the mixin pattern isn't used anywhere else in the repo, so it would be a net-new design pattern that might add complexity.

I also tried direct diamond inheritance (llm_build_hybrid_mamba inherits from both llm_build_mamba and llm_build_granite which both inherit from llm_graph_context), but since the builder classes all implement their graph initialization in their constructors, this failed badly due to initializing all the models on the same graph.

At the moment, because of the complexity of these composition solutions, I'm sticking with the "copy-paste stuff" approach, but I'm going to keep poking at it to see if there's a composition approach that will actually feel clean for hybrid models like this that borrow layer builders from other models.

[younesbelkada]

Yes I also faced this !
For H1 it's a bit more complex because we use d_ssm from the config / hparams whereas other models use d_inner. Also in our case we have this additional mup_vector that we use right after the proejction of ssm_proj so I decided to stick into the copy-paste approach

[gabe-l-hart]

That makes sense. I think it's a good argument in favor of copy-paste: Even if a lot of the logic is the same, it gives each model full ownership over exactly how that logic is wired up and avoids the challenges when a "parent model" makes changes that accidentally break a "child model" that depends on it. This is very similar to how transformers does it, though they've swung back a bit with the modular approach. The more I think about it, though, the more I think I'll stick with this for Granite 4 as well.

[younesbelkada]

Thank you @gabe-l-hart for your review ! I also noticed that the hybrid cache PR got finally merged. Before addressing your comments, I was thinking of merging my branch with current llama.cpp master first, what do you think ?

[younesbelkada]

It seems like Mamba2 might be close to landing: ggml-org#9126 (comment) I might also wait for that PR being merged on master and re-merge this branch into master

[gabe-l-hart]

Yep, that makes total sense. I'm going to do the same with Granite 4 and the hopefully finally point it at master!