-
Notifications
You must be signed in to change notification settings - Fork 12.5k
kv-cache : refactor + add llama_memory_state_i #13746
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
773b6e3
kv-cache : simplify the "struct llama_kv_cache" interface
ggerganov 9fc50dc
kv-cache : revert the (n_swa + n_ubatch) change (for next PR)
ggerganov c2c3591
kv-cache : some comments
ggerganov 8856782
context : fix graph reserve for multiple sequences
ggerganov bffb9d4
kv-cache : fix typo [no ci]
ggerganov 32cc9ea
kv-cache : fix find_slot() logic for free slots
ggerganov f97de9b
llama : add TODO for deprecating the defrag API in the future
ggerganov 7764d91
kv-cache : improve find_slot() using min/max seq pos info
ggerganov 780bba9
llama : handle aborts and compute errors
ggerganov dbcfa5f
memory : extract state into llama_memory_state
ggerganov f2ded9d
kv-cache : add comments
ggerganov e230e51
server : update batching logic to reset n_batch on successful decode
ggerganov 3cf5186
server : upon full re-processing, remove the sequence from the cache
ggerganov 71619f2
kv-cache : add TODO for doing split_equal when split_simple fails
ggerganov File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
kv-cache : improve find_slot() using min/max seq pos info
ggml-ci
- Loading branch information
commit 7764d91497d853f2c6c255e3ae0daa39e94ab2df
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In local testing trying to use
split_equal
with the unified portion of the hybrid cache, this line is causing problems. Specifically, I think it conflicts with the logic inllama_sbatch::add_seq_to_ubatch
(here) where theubatch.seq_id
is only populated with a non-null value at positionubatch.n_seqs
. I'll keep digging to see if there's a simple solution, but wanted to flag this in case it's an easy fix on your end.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably not the right solution, but this "fixed" the issue on my Granite 4 branch:
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intended way to traverse
ubatch.seq_id
since the splits were introduced in #8526 is by usingubatch.n_seqs
, notubatch.n_tokens
. In simple splits,ubatch.n_seqs
is equal toubatch.n_tokens
. Fixing this loop (and also the one inapply_ubatch
) should make it work properly with equal splits too.llama.cpp/src/llama-batch.cpp
Line 141 in a592c13
See also the comments explaining the sizes of the arrays in
ubatch
:llama.cpp/src/llama-batch.h
Lines 10 to 24 in a592c13
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correction, using
ubatch.n_seqs
for traversal only applies toubatch.n_seq_id
andubatch.seq_id
(in case anyone here relies on the notifications and missed the edit in my previous comment).Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, that's really helpful. Since this loop is indexing into both
pos
andseq_id
which in this case have different lengths, I'm not quite following the relationship that should be used to extractseq_id
for the givenpos
element. I think ifubatch.n_seqs > 1
here, that would automatically disqualify all of the other logic around reusing full cells?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually, this is relatively simple since the number of tokens per sequence is known in
ubatch.n_seq_tokens
(for simple splits, this is always1
). In fact, here it could probably be possible to usealthough there is another approach without divisions, but with nested loops and which would change the indexing for
ubatch.pos[i]
toubatch.pos[s * ubatch.n_seq_tokens + j]
wheres
is in[0, ubatch.n_seqs)
andj
in[0, ubatch.n_seq_tokens)
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can confirm that with this fix, the model safely produces output on my Granite 4 branch (no comment on cache correctness though!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had completely forgotten about this detail. We'll fix this, but I think the initial merge will be like it is, to keep this PR a bit simpler.