Repository navigation
Fix 24486: TP: allows the usage of 4-10 gpus for stepfun and laguna - #24554
JohannesGaessler merged 1 commit into
Conversation
|
I am seeing some problems with tensor splits by going larger with granularity. I will try to implement the discovery of additional tensors during the std::vec initialization. this will include the passthrough of the user data+model into the ggml_backend_meta_get_split_state function so I can ask the model about additional tensors by tensor name. |
22c7460 to
6df847c
Compare
|
This is way better. |
|
Sorry for only responding now. I agree that the granularity may need to be adjusted but this value must be derived from hyperparameters, not hard-coded. I don't have the model in question on my machine but based on the compute graph in |
|
that's a good idea. I will change that! |
5099756 to
1ec7b8c
Compare
|
@JohannesGaessler sorry for the late fix. just remembered because laguna is failing with the same thing. so this would fix both. |
1ec7b8c to
31f63c6
Compare
|
anecdotally tested ok against b10107 |
thank you for the testing! out of curiosity what pp/s tg/s do you get with the mi50 and |
| if (ud->model->arch == LLM_ARCH_STEP35 || ud->model->arch == LLM_ARCH_LAGUNA) { | ||
| GGML_ASSERT(segments.size() == 1); | ||
| const int64_t max_el = segments[0].first; | ||
| if (std::regex_match(tensor_name, pattern_attn_gate_weight)) { | ||
| return {hparams.n_head(il) / hparams.n_head_kv(il)}; | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't think this is the right way to calculate the granularity. The logical interpretation is that this function should return a value to which the tensor dimension is rounded when splitting data between GPUs. That should be hparams.n_head(il), not the GQA ratio. (It may be that for some combinations of models and hardware an incorrect implementation happens to work by chance.)
There was a problem hiding this comment.
So I keep the architecture checks since it is needed for both to run with 4-10 gpus?
Or should i change this differently?
There was a problem hiding this comment.
The way tensors are named is a lot of the time inconsistent, so either way may need adjustements in the future. My intuition is that removing the check in this PR will have fewer issues in the future.
Layer:
build: 54ce507 (10111) Tensor:
build: 54ce507 (10111) So, not much in terms of tg, but a decent bump in pp. numa to pex is PCIe 3.0 x16 EDIT: TG at deep context (270k) is MUCH faster; ~25tps (tensor) vs ~7tps (layer) |
That's awesome! |
31f63c6 to
2d807d0
Compare
2d807d0 to
22f16f4
Compare
|
@CISC @JohannesGaessler how can I continue from here? imho this is a small isolated change to get 2 models to run in tensor parallel and not crash during load. What should I change? |
I'm not sure exactly what the suggested change is, this will have to wait for feedback from Johannes. |
22f16f4 to
b8ffdcd
Compare
JohannesGaessler
left a comment
There was a problem hiding this comment.
Sorry for the long radio silence, I had taken a break from llama.cpp and am currently working through my backlog. Looking at the way the compute graph is being constructed the submitted implementation is correct after all. The wqkv_gate tensor is applied after FlashAttention and before the attention output matrix multiplication as a gate on the FA output. Therefore the granularity needs to be the number of Q heads per K/V head since that is the FA granularity. If you remove the unused variable I'll approve and merge.
b8ffdcd to
ddf8934
Compare
|
@JohannesGaessler welcome back! |
ddf8934 to
8b2327d
Compare
|
Maion-coder fails and only on mac, since my adjustments are guarded by a model arch check this is unrelated. probably a rerun needed? same model again in the llama-context test: |
8b2327d to
96211ba
Compare
|
Pushed up another rebase, fingers crossed. |
96211ba to
a13cd76
Compare
Overview
This fixes #24486 and allows the usage of 4-10 and probably more gpu's. I cannot test with <4 because of too little vram.
Requirements