Skip to content

Commit 55d2d84

Browse files
committed
fix(activation): copy the KV cache only once per generation
The activation memory hands out an owned copy of the stored cache (`get_cache()`), and `HFLLM.generate` copied that copy again before it was handed to the model. A caller that follows the public API therefore paid for two full copies of every K/V tensor and kept three sets alive during the generation: the stored cache, the copy of the copy, and the one the model writes into. Ownership is now explicit. A cache that leaves the activation memory is an *owned copy*: it shares no tensor with the stored cache, so it may be handed to a model that appends to it in place. `clone_dynamic_cache()` and the merge path of `_concat_caches()` mark the cache they return, and the model boundary skips the copy for a cache that is already owned. A cache that is stored back into the memory loses the marker, so the model still copies it before it appends to it. The stored cache stays read-only in every case: the model never writes into a cache that aliases it, and the number of K/V tensor sets alive during a generation drops from three to two. Tests: - `tests/llms/test_hf.py::test_generation_from_get_cache_copies_the_kv_tensors_only_once` walks the public path (`get_cache()` -> `generate()`), counts the copies (exactly one) and pins that the model received the cache the memory handed out instead of a second copy of it. - `tests/llms/test_hf.py::test_generation_from_get_cache_keeps_the_stored_cache_read_only` pins that the stored cache is not rewritten, also when an owned copy is stored back into the memory. - `tests/memories/activation/test_kv.py` pins the ownership marker on the single-cache and on the merge path, and that a stored cache is never an owned copy. Fixes #2301.
1 parent f5d6e15 commit 55d2d84

4 files changed

Lines changed: 232 additions & 4 deletions

File tree

‎src/memos/llms/hf.py‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -121,15 +121,26 @@ def _clone_cache(cache: DynamicCache) -> DynamicCache:
121121
which would mutate the stored cache during the generation and leave
122122
it grown whenever the generation is interrupted.
123123
124+
A cache that is already an owned copy -- one that was handed out by
125+
the activation memory and therefore shares no tensor with the stored
126+
cache -- is not copied again: the copy would be pure overhead and
127+
would keep a second full set of K/V tensors alive during the
128+
generation. The stored cache stays read-only either way, because an
129+
owned copy never aliases it.
130+
124131
Args:
125132
cache (DynamicCache): The cache provided by the caller.
126133
127134
Returns:
128-
DynamicCache: A copy that does not share any tensor with ``cache``.
135+
DynamicCache: A cache that does not share any tensor with the
136+
stored activation memory.
129137
"""
130138
# Imported lazily: memos.llms.factory imports this module, so a
131139
# module-level import of memos.memories.activation.kv would be circular.
132-
from memos.memories.activation.kv import clone_dynamic_cache
140+
from memos.memories.activation.kv import clone_dynamic_cache, is_owned_cache
141+
142+
if is_owned_cache(cache):
143+
return cache
133144

134145
return clone_dynamic_cache(cache)
135146

‎src/memos/memories/activation/kv.py‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,50 @@
1515
from memos.memories.textual.item import TextualMemoryItem
1616

1717

18+
#: Attribute that marks a cache as an *owned copy*.
19+
#:
20+
#: A cache that leaves the activation memory is an owned copy when it shares
21+
#: no tensor with the stored cache: the caller may hand it to a model that
22+
#: appends to it in place without corrupting the stored cache. The marker
23+
#: records that ownership, so the model boundary can tell an already owned
24+
#: cache from a borrowed one and does not copy it a second time.
25+
_OWNED_CACHE_MARKER = "_memos_owned_cache_copy"
26+
27+
28+
def is_owned_cache(cache: Any) -> bool:
29+
"""
30+
Whether ``cache`` is an owned copy that the caller may mutate in place.
31+
32+
A cache is owned when it was produced by :func:`clone_dynamic_cache` or by
33+
the merge path of ``_concat_caches``: it shares no tensor with the
34+
activation memory, so handing it to a model that appends to it in place
35+
cannot corrupt the stored cache. Any other cache is *borrowed* and has to
36+
be copied before it is given to the model.
37+
38+
Args:
39+
cache: The cache to inspect.
40+
41+
Returns:
42+
bool: True when the cache is an owned copy.
43+
"""
44+
return bool(getattr(cache, _OWNED_CACHE_MARKER, False))
45+
46+
47+
def _mark_cache_as_stored(cache: Any) -> None:
48+
"""
49+
Drop the ownership marker of a cache that is stored in the memory.
50+
51+
A stored cache belongs to the activation memory, not to the caller: it
52+
must never be treated as an owned copy, otherwise the model would append
53+
to it in place and grow the stored memory again.
54+
55+
Args:
56+
cache: The cache that is about to be stored.
57+
"""
58+
if getattr(cache, _OWNED_CACHE_MARKER, False):
59+
setattr(cache, _OWNED_CACHE_MARKER, False)
60+
61+
1862
class KVCacheMemory(BaseActMemory):
1963
"""
2064
Key-Value Cache Memory for activation memories.
@@ -60,6 +104,9 @@ def add(self, memories: list[KVCacheItem]) -> None:
60104
memories: List of KVCacheItem to add
61105
"""
62106
for memory in memories:
107+
# A stored cache belongs to the memory: it must not be treated as
108+
# an owned copy, otherwise the model would append to it in place.
109+
_mark_cache_as_stored(memory.memory)
63110
self.kv_cache_memories[memory.id] = memory
64111

65112
def get_cache(self, cache_ids: list[str]) -> DynamicCache | None:
@@ -256,6 +303,10 @@ def _concat_caches(self, caches: list[DynamicCache]) -> DynamicCache:
256303
"DynamicCache object has neither 'layers' nor 'key_cache' attributes"
257304
)
258305

306+
# The merged cache is built from freshly concatenated tensors, so it
307+
# shares nothing with the stored caches: it is an owned copy as well.
308+
setattr(merged, _OWNED_CACHE_MARKER, True)
309+
259310
return merged
260311

261312

@@ -301,6 +352,10 @@ def clone_dynamic_cache(cache: DynamicCache) -> DynamicCache:
301352
* ``transformers >= 4.55``: a ``layers`` list whose layers carry either
302353
``keys`` / ``values`` or ``key_cache`` / ``value_cache``.
303354
355+
The copy is marked as an owned copy (see :func:`is_owned_cache`), so the
356+
model boundary does not copy it a second time when it is handed to a
357+
model that appends to it in place.
358+
304359
Args:
305360
cache: The cache to copy.
306361
@@ -315,6 +370,7 @@ def clone_dynamic_cache(cache: DynamicCache) -> DynamicCache:
315370
# cache it receives.
316371
cloned = copy.copy(cache)
317372
cloned.layers = [_clone_cache_layer(layer) for layer in layers]
373+
setattr(cloned, _OWNED_CACHE_MARKER, True)
318374
return cloned
319375

320376
key_cache = getattr(cache, "key_cache", None)
@@ -326,6 +382,7 @@ def clone_dynamic_cache(cache: DynamicCache) -> DynamicCache:
326382
cloned.key_cache = [_clone_tensor(tensor) for tensor in key_cache]
327383
if value_cache is not None:
328384
cloned.value_cache = [_clone_tensor(tensor) for tensor in value_cache]
385+
setattr(cloned, _OWNED_CACHE_MARKER, True)
329386
return cloned
330387

331388
raise AttributeError("DynamicCache object has neither 'layers' nor 'key_cache' attributes")

‎tests/llms/test_hf.py‎

Lines changed: 103 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,14 +9,43 @@
99
from memos.configs.llm import HFLLMConfig, LLMConfigFactory
1010
from memos.llms.factory import LLMFactory
1111
from memos.llms.hf import HFLLM
12-
from tests.utils import dynamic_cache_seq_len, make_dynamic_cache
12+
from memos.memories.activation.kv import clone_dynamic_cache
13+
from tests.utils import (
14+
dynamic_cache_key_tensors,
15+
dynamic_cache_seq_len,
16+
make_dynamic_cache,
17+
)
1318

1419

1520
# The KV cache is a torch structure; skip the whole module where torch is
1621
# not installed instead of failing the collection of the test suite.
1722
torch = pytest.importorskip("torch")
1823

1924

25+
def make_activation_memory(cache):
26+
"""Build a ``KVCacheMemory`` that stores a single KV cache.
27+
28+
Args:
29+
cache: The cache to store in the memory.
30+
31+
Returns:
32+
tuple: The memory and the stored ``KVCacheItem``.
33+
"""
34+
from memos.configs.memory import KVCacheMemoryConfig
35+
from memos.memories.activation.item import KVCacheItem
36+
from memos.memories.activation.kv import KVCacheMemory
37+
38+
config = MagicMock(spec=KVCacheMemoryConfig)
39+
config.extractor_llm = MagicMock()
40+
config.memory_filename = "test_kv_cache.pkl"
41+
with patch("memos.llms.factory.LLMFactory.from_config", return_value=MagicMock()):
42+
memory = KVCacheMemory(config)
43+
44+
item = KVCacheItem(memory=cache)
45+
memory.add([item])
46+
return memory, item
47+
48+
2049
class CacheGrowingModel:
2150
"""A stand-in for a transformers model that grows the cache in place.
2251
@@ -264,3 +293,76 @@ def test_generate_stream_does_not_mutate_caller_cache(self):
264293
self.assertGreater(dynamic_cache_seq_len(model.received_caches[0]), 3)
265294
# The caller's cache still holds only the original tokens.
266295
self.assertEqual(dynamic_cache_seq_len(cache), 3)
296+
297+
def test_generation_from_get_cache_copies_the_kv_tensors_only_once(self):
298+
# The public API path: the activation memory hands out a cache with
299+
# get_cache(), the caller passes it to generate(). The stored cache
300+
# must stay read-only, and the K/V tensors must not be copied twice
301+
# along the way -- one copy would already keep three full sets of
302+
# tensors alive (stored + copy 1 + copy 2).
303+
config = HFLLMConfig(
304+
model_name_or_path="qwen3:0.6b",
305+
max_tokens=5,
306+
do_sample=False,
307+
add_generation_prompt=True,
308+
)
309+
llm = self._create_llm(config)
310+
model = CacheGrowingModel()
311+
llm.model = model
312+
313+
stored_cache = make_dynamic_cache(seq_len=3)
314+
memory, item = make_activation_memory(stored_cache)
315+
316+
with patch(
317+
"memos.memories.activation.kv.clone_dynamic_cache", wraps=clone_dynamic_cache
318+
) as clone_spy:
319+
cached = memory.get_cache([item.id])
320+
resp = llm.generate([{"role": "user", "content": "Hello"}], past_key_values=cached)
321+
322+
self.assertEqual(resp, self.standard_response)
323+
# The whole path copies the K/V tensors exactly once.
324+
self.assertEqual(clone_spy.call_count, 1)
325+
# The model received the cache the memory handed out, not a second copy.
326+
self.assertIs(model.received_caches[0], cached)
327+
# The copy the model wrote into does not alias the stored tensors.
328+
self.assertNotEqual(
329+
dynamic_cache_key_tensors(model.received_caches[0])[0].data_ptr(),
330+
dynamic_cache_key_tensors(item.memory)[0].data_ptr(),
331+
)
332+
# The copy did grow, i.e. the model really appends in place.
333+
self.assertGreater(dynamic_cache_seq_len(model.received_caches[0]), 3)
334+
# The stored cache was not rewritten: it still holds only the
335+
# original tokens.
336+
self.assertEqual(dynamic_cache_seq_len(item.memory), 3)
337+
self.assertEqual(dynamic_cache_seq_len(stored_cache), 3)
338+
339+
def test_generation_from_get_cache_keeps_the_stored_cache_read_only(self):
340+
# The stored cache stays read-only even when it is handed out a second
341+
# time and when a caller stores an already owned copy back.
342+
config = HFLLMConfig(
343+
model_name_or_path="qwen3:0.6b",
344+
max_tokens=5,
345+
do_sample=False,
346+
add_generation_prompt=True,
347+
)
348+
llm = self._create_llm(config)
349+
model = CacheGrowingModel()
350+
llm.model = model
351+
352+
stored_cache = make_dynamic_cache(seq_len=3)
353+
memory, item = make_activation_memory(stored_cache)
354+
355+
cached = memory.get_cache([item.id])
356+
llm.generate([{"role": "user", "content": "Hello"}], past_key_values=cached)
357+
358+
# The stored cache kept its original length.
359+
self.assertEqual(dynamic_cache_seq_len(item.memory), 3)
360+
self.assertEqual(dynamic_cache_seq_len(stored_cache), 3)
361+
362+
# A cache that is stored back into the memory is owned by the memory
363+
# again: the model must copy it before it appends to it.
364+
memory.add([item])
365+
llm.generate([{"role": "user", "content": "Hello"}], past_key_values=item.memory)
366+
367+
self.assertIsNot(model.received_caches[-1], item.memory)
368+
self.assertEqual(dynamic_cache_seq_len(item.memory), 3)

‎tests/memories/activation/test_kv.py‎

Lines changed: 59 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,11 @@
66

77
from memos.configs.memory import KVCacheMemoryConfig
88
from memos.memories.activation.item import KVCacheItem
9-
from memos.memories.activation.kv import KVCacheMemory, clone_dynamic_cache
9+
from memos.memories.activation.kv import (
10+
KVCacheMemory,
11+
clone_dynamic_cache,
12+
is_owned_cache,
13+
)
1014
from tests.utils import (
1115
dynamic_cache_key_tensors,
1216
dynamic_cache_layer_count,
@@ -192,6 +196,9 @@ def __init__(self):
192196
assert torch.equal(cloned.key_cache[0], cache.key_cache[0])
193197
assert cloned.value_cache[0].data_ptr() != cache.value_cache[0].data_ptr()
194198
assert torch.equal(cloned.value_cache[0], cache.value_cache[0])
199+
# The copy of a legacy cache is an owned copy as well.
200+
assert is_owned_cache(cloned)
201+
assert not is_owned_cache(cache)
195202

196203

197204
def test_clone_dynamic_cache_prefers_the_layers_layout():
@@ -240,3 +247,54 @@ def __init__(self):
240247
assert torch.equal(cloned.layers[0].key_cache, cache.layers[0].key_cache)
241248
assert cloned.layers[0].value_cache.data_ptr() != cache.layers[0].value_cache.data_ptr()
242249
assert torch.equal(cloned.layers[0].value_cache, cache.layers[0].value_cache)
250+
251+
252+
def test_clone_dynamic_cache_hands_out_an_owned_copy():
253+
# A clone is an owned copy: it shares no tensor with the original, so the
254+
# caller may hand it to a model that appends to it in place. The model
255+
# boundary uses that ownership to avoid copying it a second time.
256+
cache = make_filled_cache()
257+
cloned = clone_dynamic_cache(cache)
258+
259+
assert is_owned_cache(cloned)
260+
assert not is_owned_cache(cache)
261+
262+
263+
def test_get_cache_hands_out_an_owned_copy(kv_memory):
264+
# The cache the memory hands out is an owned copy; the stored cache is not.
265+
item = KVCacheItem(memory=make_filled_cache())
266+
kv_memory.add([item])
267+
268+
cached = kv_memory.get_cache([item.id])
269+
270+
assert is_owned_cache(cached)
271+
assert not is_owned_cache(item.memory)
272+
273+
274+
def test_get_cache_merge_hands_out_an_owned_copy(kv_memory):
275+
# The merged cache is built from freshly concatenated tensors, so it is an
276+
# owned copy as well.
277+
item1 = KVCacheItem(memory=make_filled_cache())
278+
item2 = KVCacheItem(memory=make_filled_cache())
279+
kv_memory.add([item1, item2])
280+
281+
merged = kv_memory.get_cache([item1.id, item2.id])
282+
283+
assert is_owned_cache(merged)
284+
assert not is_owned_cache(item1.memory)
285+
assert not is_owned_cache(item2.memory)
286+
287+
288+
def test_stored_cache_is_never_an_owned_copy(kv_memory):
289+
# A cache that is stored in the memory belongs to the memory: storing an
290+
# already owned copy back must drop the ownership, otherwise the model
291+
# would append to the stored cache in place.
292+
item = KVCacheItem(memory=make_filled_cache())
293+
kv_memory.add([item])
294+
295+
cached = kv_memory.get_cache([item.id])
296+
assert is_owned_cache(cached)
297+
298+
kv_memory.add([KVCacheItem(memory=cached, metadata=item.metadata)])
299+
300+
assert not is_owned_cache(cached)

0 commit comments

Comments
 (0)