Repository navigation
graph : reorder get_rows for embeddings - #30160
Conversation
|
Hello @ggerganov I did local benchmarks with this PR and here are my results: RTX 4070 Laptop GPU, CUDA, Q4_K_M models,
master is c811cb8, the base of PR 30160 at 4e67ebb. b11399 is the release before PR 29622. pp512 varies about 3 percent from run to run. So in short it does handle the performance slowdown thank you! Of course, as already noted in the current TODO Gemma 4 still suffers from a performance hit, but perhaps that if for another PR? |
|
here are my results using the same prompt and text file as in my initial bug report: PP: 582.23 t/s vs #29622 : PP: ~365 t/s looks like it's good. |
baab8cc to
7f461fd
Compare
7f461fd to
4a0c227
Compare
|
@deadprogram The Gemma4 regressions should be fixed with the latest version of this branch. Please run the tests again. Thanks. |
4a0c227 to
92c605c
Compare
|
@ngxson This change should improve the input embedding logic, though it's still not perfect and not very easy to follow. Will be thinking how to make it even better. Also, I expect that with LoRAs enabled, we'll have similar issues - I've added TODO to add LoRA tests and figure out how to prevent unnecessary splits. |
|
Here are my benchmarks, however it appears that Gemma4 with embeddibng only batches now crashes. First the numbers: RTX 4070 Laptop GPU, CUDA, Q4_K_M models,
My coding tools gave this reason for the crash:
|
|
@deadprogram Thanks for the quick feedback - should be fixed in the new commit. |
|
@ggerganov thanks for working on this! Crash fixed, and here are the new numbers: RTX 4070 Laptop GPU, CUDA, Q4_K_M models,
master is e60eff9, the base of PR 30160 at bd40fd1. b11399 is the release before PR 29622. pp512 varies about 3 percent from run to run. |
24625f5 to
49b7638
Compare
ngxson
left a comment
There was a problem hiding this comment.
looks good, I'm running benchmark on my side, will report results a bit later
|
Results: CUDA box (RTX 5060 Ti, i7-12700KF)
Mac (M5 Max, Metal, 8 threads)
|
* graph : reorder get_rows for embeddings * cont : fix gemma4 and improve input embedding construction logic * cont : add TODO for lora * cont : fix raw embeddings path * gemma4 : avoid ple cast in embeddings path
* graph : reorder get_rows for embeddings * cont : fix gemma4 and improve input embedding construction logic * cont : add TODO for lora * cont : fix raw embeddings path * gemma4 : avoid ple cast in embeddings path
Overview
cont #29622
fix #30033
With mixed batch support, we have 3 possible graph branches that produce the input embeddings:
The way we currently implement it leads to interleaving graph ops (such as PAD, MUL, SCALE) with the token embeddings extraction (GET_ROWS). This is not desired because it creates unnecessary splits or leads to running some of the ops on the CPU.
To fix that, we have to rework the logic such that it orders all GET_ROWS to be the first nodes in the graph. We can achieve this with the
ggml_build_forward_order.The goal is SPLIT 0 to always contain just the GET_ROWS ops and nothing else:
Before:
After:
TODO:
Requirements