Skip to content

DeepseekV4: clear cache only for seq rather than full#25521

Merged
am17an merged 2 commits into
ggml-org:masterfrom
am17an:dsv4-clear-stream
Jul 11, 2026
Merged

DeepseekV4: clear cache only for seq rather than full#25521
am17an merged 2 commits into
ggml-org:masterfrom
am17an:dsv4-clear-stream

Conversation

@am17an

@am17an am17an commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Overview

Currently the dsv4 cache has amneisa (#25171, #25171), this is because on clear we clear the entire cache. Instead we should clear for the particular seq_id.

Additional information

Requirements

@am17an am17an requested a review from ggerganov as a code owner July 10, 2026 08:52
@drrros

drrros commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

This had fixed full prompt recomputations on every agentic turn for me, now it only computes last turn

Comment thread src/llama-kv-cache-dsv4.cpp Outdated
Comment on lines +1194 to +1198
if (seq_id >= 0) {
clear_compressed_stream((uint32_t) seq_id, true);
} else {
clear_compressed(true);
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of introducing clear_compressed and clear_compressed_stream, can't we call the seq_rm of all the caches?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moreover, do we even have to clear anything in the compressed caches? Wouldn't the masks produced by the kv_raw mask away any stale data?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we don't use masks for the compressed caches and instead rely on the position:

const int64_t n_visible = (int64_t) (pos + 1)/ratio;
plan.n_visible[i] = (int32_t) n_visible;
plan.n_kv = std::max(plan.n_kv, n_visible);

So similar comment as in #25325 (comment). This should eventually be reworked to follow the llama_kv_cache pattern of managing a host-side KV cells information, which is decoupled from the token positions and the contents of the buffers. This way, clearing here would not be necessary (hence why I was confused).

Maybe add a reference to this comment inside the code.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we rely on n_visible. Let me add a TODO

Comment on lines 1204 to 1207

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it correct to clear here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, all of these need the correct sequence wise remove

ddh0 added a commit to ddh0/llama.cpp that referenced this pull request Jul 10, 2026
- ggml-org#24231: LID GGML OP by fairydreaming:
ggml-org#24231 @ 428831b
- ggml-org#25521: clear cache only for seq rather than full by am17an:
ggml-org#25521 @
8026938
@am17an am17an force-pushed the dsv4-clear-stream branch from 8026938 to fcf2bb7 Compare July 11, 2026 02:24
@github-actions github-actions Bot added the testing Everything test related label Jul 11, 2026
@am17an am17an requested a review from fairydreaming July 11, 2026 10:18
@TacoTakumi

This comment has been minimized.

@am17an

am17an commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

@TacoTakumi I assume you have read the contributing guidelines. Copy pasting LLM outputs is not allowed

@TacoTakumi

TacoTakumi commented Jul 11, 2026

Copy link
Copy Markdown

@TacoTakumi I assume you have read the contributing guidelines. Copy pasting LLM outputs is not allowed

My apologies, I was tired and in a rush. I knew it was a bad look.

@am17an

am17an commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

@ggml-org/maintainers can I get another approval?

@am17an am17an merged commit 13f2b28 into ggml-org:master Jul 11, 2026
25 checks passed
@drrros

drrros commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

This had fixed full prompt recomputations on every agentic turn for me, now it only computes last turn

@am17an actually it's just better now, but full reprocessing still happens even after this PR merge (it now reprocess every 5-7 turns instead of every one (like before)), also tested with #25588 applied - same behavior

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testing Everything test related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants