Skip to content

fix: stop activation KV cache from growing every turn - #2359

Open
yetuge wants to merge 10 commits into
MemTensor:mainfrom
yetuge:fix/activation-cache-aliasing
Open

fix: stop activation KV cache from growing every turn#2359
yetuge wants to merge 10 commits into
MemTensor:mainfrom
yetuge:fix/activation-cache-aliasing

Conversation

@yetuge

@yetuge yetuge commented Sep 11, 2026

Copy link
Copy Markdown

Fixes #2301.

Root cause

The stored activation cache and the cache handed to generation were the same object, so every turn permanently grew the store (and the re-dumped memory file):

  1. MemOS.chat (mem_os/core.py), mem_os/main.py, mem_chat/simple.py and mos_for_test_scheduler.py pass the stored kv_cache.memory straight into HFLLM.generate(past_key_values=...);
  2. HFLLM._prefill forwards it to model(past_key_values=...), and transformers appends the new tokens' K/V in place (DynamicLayer.update rebinds keys/values on the same object; identity is preserved);
  3. KVCacheMemory._concat_caches returns caches[0] unchanged for a single id, so the get_cache() merge path hands out the stored object as well.

No caller reads the cache back expecting it to have grown — the growth is only observable as leaked state (which ActivationMemoryManager then re-dumps to disk).

Fix

Treat stored caches as read-only by construction, at the two boundaries where a cache leaves the store or enters the model:

  • clone_dynamic_cache() (new, memories/activation/kv.py): independent copy with cloned K/V tensors, compatible with both the legacy key_cache/value_cache structure (transformers <= 4.55, per poetry.lock) and the newer layers structure (>= 4.56, still < 5.0.0 in the supported range);
  • KVCacheMemory._concat_caches: the single-cache case now returns a clone instead of the stored object;
  • HFLLM.generate / generate_stream: clone the incoming past_key_values once at the boundary. This fixes all four call sites without touching them; the cost is one cache copy per generate call, the same order as the prefill work itself.

Tests

  • test_generate_with_cache_does_not_mutate_caller_cache (tests/llms/test_hf.py): mocks a model forward that appends K/V in place, asserts the caller's cache is unchanged — fails on current main, passes with this fix;
  • test_get_cache_single_item_returns_independent_copy + test_get_cache_multi_item_merge_does_not_alias_inputs (tests/memories/activation/test_kv.py): get_cache() never aliases the store;
  • test_clone_dynamic_cache_copies_legacy_tensors + test_clone_dynamic_cache_handles_layers_structure: cover both cache structures;
  • full tests/memories/ + tests/llms/: 117 passed (+4 subtests), no regressions (Python 3.13, torch 2.14 CPU, transformers 4.53.2 per poetry.lock).

cc @issue reporter — thanks for the exceptionally detailed write-up; the line-level analysis made this straightforward to confirm and fix.

Generation appends new K/V tensors to the DynamicCache object it
receives, but the stored activation cache was handed to the model by
reference, so every chat turn permanently grew the store (and the
re-dumped memory file).

Make stored caches read-only by construction:

- add clone_dynamic_cache() in memories/activation/kv.py, compatible
  with both the legacy key_cache/value_cache structure and the newer
  layers structure (transformers >= 4.56);
- _concat_caches now returns a clone in the single-cache case instead
  of the stored object;
- HFLLM.generate / generate_stream clone the incoming past_key_values
  once at the boundary, which fixes all four call sites
  (mem_os/core.py, mem_os/main.py, mem_chat/simple.py, scheduler
  analyzer) without changing them.
Copilot AI lite review requested due to automatic review settings September 11, 2026 05:20
@Memtensor-AI Memtensor-AI added area:memory 记忆存储、检索、更新、召回逻辑 area:model llm + embedder + reranker status:in-progress Someone or AI is working on it | 人工或 AI 正在处理 labels Sep 11, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Layered cache cloning may remain incorrect on transformers ≥4.56, and streaming generation lacks regression coverage.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Fixes activation KV-cache aliasing that causes stored caches to grow across turns.

Changes:

  • Adds cache cloning for legacy and layered cache formats.
  • Clones caches at retrieval and HF generation boundaries.
  • Adds cache-isolation regression tests.
File summaries
File Summary
tests/memories/activation/test_kv.py Tests cloning and merge isolation.
tests/llms/test_hf.py Tests that generation does not mutate caller caches.
src/memos/memories/activation/kv.py Adds cache cloning. Critical (3 votes): layered cache cloning may reset required layer state on transformers ≥4.56; clone layer state and test a real update.
src/memos/llms/hf.py Clones caches before generation. Nit (1 vote): add regression coverage for streaming generation.
Review details

Suppressed comments (2)

src/memos/llms/hf.py:116

  • The new generate_stream boundary is not covered by the regression test, which only exercises generate. A future change could preserve the caller cache for non-streaming generation while reintroducing aliasing on the streaming path; add a streaming test that performs the same in-place K/V append and checks the original cache length.
            from memos.memories.activation.kv import clone_dynamic_cache

            yield from self._generate_with_cache_stream(
                prompt, clone_dynamic_cache(past_key_values)
            )

src/memos/memories/activation/kv.py:279

  • The new-layer branch only copies keys/values, but this codebase already supports layer objects that expose the alternative key_cache/value_cache names in move_dynamic_cache_htod. For those caches, the cloned layer is appended without any tensors, so generation receives an empty cache or fails instead of using the stored activation memory. Copy both attribute variants.
            if getattr(layer, "keys", None) is not None:
                new_layer.keys = layer.keys.clone()
                new_layer.values = layer.values.clone()
  • Files reviewed: 4/4 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/memos/memories/activation/kv.py Outdated
Comment on lines +276 to +280
new_layer = type(layer)()
if getattr(layer, "keys", None) is not None:
new_layer.keys = layer.keys.clone()
new_layer.values = layer.values.clone()
cloned.layers.append(new_layer)
@Memtensor-AI

Memtensor-AI commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

🤖 Open Code Review

Target: PR #2359
Task: 0cb44b46b1ce0f46
Base: main
Head: fix/activation-cache-aliasing

🔍 OpenCodeReview found 6 issue(s) in this PR.


1. tests/cache_helpers.py (L40-L43)

cache_value_layer_count is functionally identical to cache_layer_count in every branch — both return len(cache.layers) on the new API and len(cache.key_cache) / len(cache.value_cache) on the legacy API (these are always equal for a consistent cache). The two helpers already appear side-by-side in test_get_cache_merge asserting the same value on the same object, which reinforces that they measure nothing different.

Having two names that imply distinct semantics but produce the same number is misleading: a reader will wonder what invariant the test is checking, and a future contributor who renames one attribute but not the other will believe the mismatch is caught.

Suggestion: remove cache_value_layer_count and replace every call site with cache_layer_count. If there is ever a genuine need to verify key/value symmetry independently, a single helper that asserts both sides are equal would be clearer than two that silently return the same count.


2. src/memos/memories/activation/kv.py (L268-L270)

torch is imported at function-call time rather than at module level. Every other function in this file uses torch after a module-level import (from torch import ... or similar via the decorator machinery), but this function buries the import inside the body. Since torch is a hard dependency of this module (not optional), move the import to the top of the file alongside the other imports. The current placement adds a dict-lookup cost on every clone call and makes the module's dependency surface harder to audit.

💡 Suggested Change

Before:

    import torch

    cloned = DynamicCache()

After:

# At the top of the file, alongside existing imports:
import torch

# Remove the `import torch` line inside clone_dynamic_cache

3. src/memos/memories/activation/kv.py (L281-L310)

The vars(layer) loop above already correctly clones every tensor attribute, including all four of keys, values, key_cache, and value_cache. The entire has_per_layer_cache block that follows then re-clones the winning pair and nulls the losing pair — duplicating work the loop already did. This also means the loop wastes time cloning tensors that are immediately overwritten or nulled. Worse, the two-pass structure makes the invariant hard to reason about: a reader must track which of the two passes "wins" for each attribute.

Consider doing the prioritisation in a single pass: skip the four K/V tensor names in the generic loop, then apply the has_per_layer_cache logic once to fill them in correctly. This removes the redundancy and makes the precedence rule the only place that touches those attributes.

💡 Suggested Change

Before:

        for attr, value in layer_attrs.items():
                setattr(
                    new_layer,
                    attr,
                    value.clone() if isinstance(value, torch.Tensor) else copy.deepcopy(value),
                )
            # transformers>=4.56 layers expose keys/values, but some versions
            # instead carry per-layer key_cache/value_cache (see
            # move_dynamic_cache_htod); a clone that skips one shape would
            # silently return a content-empty layer.
            # Select one naming scheme, matching move_dynamic_cache_htod's
            # precedence, while retaining independent guards for asymmetric
            # test doubles and cache layers.
            has_per_layer_cache = any(
                getattr(layer, name, None) is not None for name in ("key_cache", "value_cache")
            )
            if has_per_layer_cache:
                if "keys" in layer_attrs:
                    new_layer.keys = None
                if "values" in layer_attrs:
                    new_layer.values = None
                if getattr(layer, "key_cache", None) is not None:
                    new_layer.key_cache = layer.key_cache.clone()
                if getattr(layer, "value_cache", None) is not None:
                    new_layer.value_cache = layer.value_cache.clone()
            else:
                if "key_cache" in layer_attrs:
                    new_layer.key_cache = None
                if "value_cache" in layer_attrs:
                    new_layer.value_cache = None

After:

            KV_ATTRS = {"keys", "values", "key_cache", "value_cache"}
            for attr, value in layer_attrs.items():
                if attr in KV_ATTRS:
                    continue
                setattr(
                    new_layer,
                    attr,
                    value.clone() if isinstance(value, torch.Tensor) else copy.deepcopy(value),
                )
            has_per_layer_cache = any(
                getattr(layer, name, None) is not None for name in ("key_cache", "value_cache")
            )
            if has_per_layer_cache:
                new_layer.keys = None if "keys" in layer_attrs else getattr(new_layer, "keys", None)
                new_layer.values = None if "values" in layer_attrs else getattr(new_layer, "values", None)
                if getattr(layer, "key_cache", None) is not None:
                    new_layer.key_cache = layer.key_cache.clone()
                elif "key_cache" in layer_attrs:
                    new_layer.key_cache = None
                if getattr(layer, "value_cache", None) is not None:
                    new_layer.value_cache = layer.value_cache.clone()
                elif "value_cache" in layer_attrs:
                    new_layer.value_cache = None
            else:
                if "key_cache" in layer_attrs:
                    new_layer.key_cache = None
                if "value_cache" in layer_attrs:
                    new_layer.value_cache = None
                if getattr(layer, "keys", None) is not None:
                    new_layer.keys = layer.keys.clone()
                if getattr(layer, "values", None) is not None:
                    new_layer.values = layer.values.clone()

4. tests/llms/test_hf.py (L214-L221)

The if hasattr(kv, 'layers') branch is dead code in this test. _make_filled_cache() always returns a plain DynamicCache built via cache.update(...), which never has a layers attribute in any standard transformers version (the layers-based layout only appears on transformers ≥4.56 when DynamicCache is constructed with a model config). clone_dynamic_cache preserves the same layout, so the cloned cache passed to forward is always a key_cache/value_cache-style object. The if branch never executes, leaving the layers-path of the mutation logic untested.

If coverage of the layers layout matters, split this into two parameterized test cases — one using make_filled_cache() (standard DynamicCache) and one using make_real_hybrid_cache() from cache_helpers.py (which already skips on older transformers). If only the standard path is needed, remove the dead branch entirely.


5. tests/memories/activation/test_kv.py (L104-L107)

If merged and item.memory share tensor storage (the bug this test is detecting), the assert fires before zero_() runs, leaving item.memory's tensor permanently set to 99.0 for the rest of the test session. Subsequent tests that reuse the same kv_memory fixture or inspect the same stored item will then see corrupted state.

Use torch.clone() on the filled tensor for the assertion, or restore the value unconditionally inside a try/finally:

orig = cache_keys(item.memory).clone()
merged_keys.fill_(99.0)
try:
    assert not torch.all(cache_keys(item.memory) == 99.0), "get_cache shares storage with store"
finally:
    merged_keys.zero_()

Alternatively, assert storage independence without mutating, e.g. merged_keys.data_ptr() != cache_keys(item.memory).data_ptr().


6. tests/memories/activation/test_kv.py (L146-L149)

The skip condition checks for key_cache but the comment says the guard is for _seen_tokens. These are different attributes — a cache can have key_cache without _seen_tokens (e.g., on newer transformers that still expose the legacy list API). If _seen_tokens is absent the test will raise AttributeError on cache._seen_tokens = 2 instead of being skipped cleanly.

Guard on the attribute actually being tested:

if not hasattr(cache, "_seen_tokens"):
    pytest.skip("_seen_tokens is not present in this transformers version")

Generated by cloud-assistant via Open Code Review.

@Memtensor-AI

Copy link
Copy Markdown
Collaborator

⚠️ Automated Test Results: INCONCLUSIVE

Automated tests inconclusive (auto-generated test defect); treated as non-blocking. Manual review recommended. Details: Both new test files import torch unconditionally at the module level, causing collection-time failures in an environment where torch is not installed. The tests never execute.

Branch: fix/activation-cache-aliasing

- Use deterministic non-EOS argmax logits in the no-mutation regression
  mock so sampling cannot end the loop early (~1% flake), and fall back
  to positional args for past_key_values.
- Assert tensor-storage independence via in-place fill_ mutations, so a
  clone that shares storage is caught, not just slot rebinding.
- Guard keys/values independently in clone_dynamic_cache legacy-layer
  path (keys without values no longer raises AttributeError) and cover
  it with a dedicated test.
@yetuge

yetuge commented Sep 11, 2026

Copy link
Copy Markdown
Author

Thanks for the automated review — all 4 findings are addressed in 2808420:

  1. test_hf.py: the mock now reads past_key_values via .get() with a positional fallback, and uses deterministic argmax logits (-1e9 everywhere except a non-EOS token) so the generation loop always runs all max_tokens turns instead of risking an early EOS sample (~1% chance).
  2. kv.py: clone_dynamic_cache now guards keys and values independently in the legacy-layer path, so a layer with only one side populated no longer raises AttributeError; added test_clone_dynamic_cache_layers_guard_keys_and_values_independently to cover it.
  3. & 4. test_kv.py: both clone tests now also mutate the cloned tensors in place (fill_(99.0)) and assert the original is untouched, so a future regression to a storage-sharing clone is caught rather than only slot rebinding.

All 16 tests in the two affected files pass locally.

@Memtensor-AI

Copy link
Copy Markdown
Collaborator

⚠️ Automated Test Results: ENV ISSUE

The test environment encountered an issue that requires manual attention.

Details: Both test files fail at collection time because PyTorch (torch) is not installed in the test execution environment. No test logic ran at all.
Branch: fix/activation-cache-aliasing

- clone_dynamic_cache: also copy per-layer key_cache/value_cache
  attributes (some transformers versions carry that shape instead of
  keys/values, mirroring move_dynamic_cache_htod), with a dedicated
  storage-independence test.
- test_hf mock: drop the unreachable positional fallback for
  past_key_values and document why .get() stays.
- get_cache independence test: add an in-place fill_ assertion so a
  storage-sharing clone is caught, matching the clone tests.
@yetuge

yetuge commented Sep 11, 2026

Copy link
Copy Markdown
Author

Second review round addressed in 04e3cc4:

  1. clone_dynamic_cache now also copies per-layer key_cache/value_cache attributes (some transformers versions carry that shape instead of keys/values, as move_dynamic_cache_htod handles), with a dedicated test asserting storage independence.
  2. test_hf mock: removed the unreachable positional fallback — .get() stays with a comment explaining that _prefill always passes the cache by keyword.
  3. test_get_cache_single_item_returns_independent_copy now also mutates in place (fill_) so a storage-sharing clone cannot slip through.

On the ENV ISSUE flag: the two new test files import torch at module level because they exercise real tensor semantics (shape growth, in-place mutation, storage sharing); with torch available they pass locally (17 passed on Python 3.13 / CPU torch). No test logic is skipped — the collection failure only occurs in environments where torch is absent. If the CI image can't install torch, an alternative is skipping these two files via a pytest collection hook there, but that would leave the regression unguarded, so I'd rather keep them and let the env provide torch.

@Memtensor-AI

Copy link
Copy Markdown
Collaborator

❌ Automated Test Results: FAILED

Auto-fix retry 1/2 triggered.

Failed tests:

  • collection
  • collection
Error details
Tests failed. Failed cases: collection, collection [advisory, non-gating] AI-generated tests on branch test/auto-gen-18f1ad369c72d2ca-20260911185234: 69/80 passed, 11 failed — these do NOT affect the PR verdict; review the branch manually.

Branch: fix/activation-cache-aliasing

@yetuge

yetuge commented Sep 14, 2026

Copy link
Copy Markdown
Author

Follow-up on the latest CI run: all build-matrix jobs stopped at the Ruff checks before tests. The failure was a formatting-only result naming exactly src/memos/llms/hf.py and tests/memories/activation/test_kv.py; ruff check itself passed. I applied Ruff 0.11.8 formatting to those two files in commit d5649e6 (no behavior changes).

Local verification: ruff check passed, ruff format --check passed, the applicable pre-commit hooks passed, and python -m pytest tests/memories/activation/test_kv.py tests/llms/test_hf.py -q passed (17 tests). The separate autotest failure still reports the known missing-torch environment issue and was not changed.

@Memtensor-AI

Copy link
Copy Markdown
Collaborator

⚠️ Automated Test Results: ENV ISSUE

The test environment encountered an issue that requires manual attention.

Details: Both tests fail at collection time with ModuleNotFoundError for 'torch', meaning PyTorch is not installed in the test environment. No test logic or application code was actually executed.
Branch: fix/activation-cache-aliasing

@yetuge

yetuge commented Sep 14, 2026

Copy link
Copy Markdown
Author

Thanks for the updated Open Code Review in comment 5629951647 (edited 2026-09-14). I rechecked all four findings against the current code and agree that they are actionable:

  1. Layer attribute precedence: clone_dynamic_cache now selects the per-layer key_cache/value_cache naming scheme as a unit before falling back to keys/values, matching the precedence in move_dynamic_cache_htod. The inner guards remain independent so the existing keys-only and values-only compatibility case is preserved. I added test_clone_dynamic_cache_prefers_per_layer_cache_attributes for a layer exposing both schemes. I did not copy the suggested snippet literally because its final elif would drop a values-only layer, which the existing regression test intentionally covers.
  2. Single-item alias test: fill_ now runs before replacing the list slot with the simulated appended tensor; the clone is reset and the append/shape assertion then runs. This makes the storage-alias check effective.
  3. Legacy clone alias test: the same ordering fix is applied before assigning a new tensor to the list slot.
  4. Layered value storage: the per-layer test now mutates value_cache in place and verifies the original value tensor is unchanged, symmetrically with key_cache.

The new hybrid-layer regression failed on the previous implementation (10 passed, 1 failed), then passed after the fix. Final local verification on commit 873782a3:

  • python -m pytest tests/memories/activation/test_kv.py tests/llms/test_hf.py -q18 passed, 1 pre-existing Pydantic warning
  • Ruff check, Ruff format check, and the applicable pre-commit hooks — passed

The fix is pushed as a new commit without rewriting history. The separate Python workflow remains action_required because this is a fork workflow awaiting maintainer approval; that is external CI state, not a code failure.

@yetuge

yetuge commented Sep 14, 2026

Copy link
Copy Markdown
Author

Correction to the verification paragraph above: the additional latest-edited review findings were implemented in commit aae1cbbe305e20339b2619292f72777ca104d2b7 (following 873782a3). The final local verification on that commit is:

  • python -m pytest tests/memories/activation/test_kv.py tests/llms/test_hf.py -q20 passed, 1 pre-existing Pydantic warning
  • Ruff check, Ruff format check, and the applicable pre-commit hooks — passed

The fork Python workflow for this head is still action_required while awaiting maintainer approval; no code failure is reported by that workflow state.

@yetuge

yetuge commented Sep 14, 2026

Copy link
Copy Markdown
Author

Follow-up on the current edited version of Open Code Review comment 5629951647 (updated 2026-09-14T10:47:25Z, now reporting three findings):

  1. DynamicLayer state: agreed. clone_dynamic_cache now copies every non-tensor attribute from the source layer, including is_initialized and _seen_tokens, before cloning K/V tensors. test_clone_dynamic_cache_preserves_layer_state models the first update() decision and verifies that existing history is appended rather than replaced.
  2. Unknown cache shape: agreed. clone_dynamic_cache now raises AttributeError when neither layers nor key_cache is available, matching _concat_caches instead of silently returning an empty cache. test_clone_dynamic_cache_rejects_unknown_shape covers this contract.
  3. Layered key alias probe: agreed. The fill_ check now runs before the test replaces cloned.layers[0].keys, then resets the clone before the slot-replacement assertion.

The earlier edited version's four findings (layer naming precedence, the two legacy alias probes, and the missing value_cache probe) remain covered in the same head. The final local verification on aae1cbbe305e20339b2619292f72777ca104d2b7 is 20 passed with one pre-existing Pydantic warning; Ruff, format, and applicable pre-commit hooks pass. The fork Python workflow remains action_required pending maintainer approval.

@yetuge

yetuge commented Sep 14, 2026

Copy link
Copy Markdown
Author

Thanks for the second in-place OCR update. I re-read comment 5629951647 at updated_at=2026-09-14T10:58:00Z and verified both findings against the current code.

  1. Legacy cache-level state — confirmed. The legacy branch now copies cache-level non-tensor attributes (including _seen_tokens) with copy.deepcopy, while still rebuilding key_cache/value_cache from cloned tensors. Added test_clone_dynamic_cache_preserves_legacy_cache_state, which seeds _seen_tokens=2, clones, updates the clone to 3, and verifies the stored cache remains at 2.

  2. Mutable layer metadata aliasing — confirmed. Layer non-tensor attributes are now copied with copy.deepcopy, so nested mutable state is independent as well as the K/V tensors. Added test_clone_dynamic_cache_copies_mutable_layer_state, which mutates a nested list through the clone and verifies the original layer is unchanged.

The earlier findings remain covered, including cache naming precedence, pre-replacement alias probes for single-item/legacy/layered tests, and symmetric value_cache coverage. The new commit is dac93a1a39a9301008504f770497a99155f795bd (only src/memos/memories/activation/kv.py and tests/memories/activation/test_kv.py), pushed without rewriting history.

Verification on the new head: python -m pytest tests/memories/activation/test_kv.py tests/llms/test_hf.py -q22 passed, 1 warning (the existing Pydantic serializer warning); Ruff check/format and applicable pre-commit hooks pass. The fork Python workflow remains externally gated by maintainer approval (action_required), not a code failure.

@yetuge

yetuge commented Sep 14, 2026

Copy link
Copy Markdown
Author

Thanks for the latest in-place OCR update. I re-read comment 5629951647 at updated_at=2026-09-14T11:10:36Z and verified both findings against the new head.

  1. Mismatched legacy K/V layer counts — confirmed. The legacy clone now uses zip(..., strict=True), so a corrupted or partially populated cache raises immediately instead of silently truncating. Added test_clone_dynamic_cache_rejects_mismatched_legacy_layers, which verifies the ValueError on unequal key/value list lengths.

  2. Unknown-shape exception contract — confirmed. An object with neither supported cache representation is an unsupported cache input, so TypeError is more accurate than AttributeError. Both clone_dynamic_cache and _concat_caches now raise TypeError with the same message, and test_clone_dynamic_cache_rejects_unknown_shape asserts that contract.

The earlier findings remain covered: cache naming precedence, pre-replacement alias probes, symmetric value-cache coverage, cache/layer state preservation, deep copying of mutable layer metadata, and explicit rejection of unknown shapes. The new commit is 507de420c8206cbf42750516f17925ccdb366f6e (only src/memos/memories/activation/kv.py and tests/memories/activation/test_kv.py), pushed as a new commit without rewriting history.

Verification on the new head: python -m pytest tests/memories/activation/test_kv.py tests/llms/test_hf.py -q23 passed, 1 warning (the existing Pydantic serializer warning); Ruff check/format and applicable pre-commit hooks pass. The fork Python workflow remains action_required pending maintainer approval, not a code failure.

@Memtensor-AI

Copy link
Copy Markdown
Collaborator

⚠️ Automated Test Results: INCONCLUSIVE

Automated tests inconclusive (auto-generated test defect); treated as non-blocking. Manual review recommended. Details: Newly added tests in test_kv.py use a make_filled_cache() helper that directly appends to DynamicCache.key_cache, but the installed transformers version exposes only the layers attribute, so the helper fails before exercising the code under test. The same helper is imported by test_hf.py::test_generate_with_cache_does_not_mutate_caller_cache, causing that test to fail for the same reason.

Branch: fix/activation-cache-aliasing

@yetuge

yetuge commented Sep 14, 2026

Copy link
Copy Markdown
Author

Thanks for the latest in-place edit of Open Code Review comment 5629951647. I re-read the current body at updated_at=2026-09-14T11:19:43Z and verified each of its three findings against the new head.

  1. test_hf.py cache guard — fixed. The mocked forward now asserts that past_key_values is present before accessing it, so a call-shape regression cannot be swallowed by the surrounding try/finally. The test helper also populates both the legacy key_cache API and the modern DynamicCache.update()/layers API, and the test snapshots both caller K/V shapes before generation.

  2. CPython-specific zip(strict=True) message — fixed. test_clone_dynamic_cache_rejects_mismatched_legacy_layers now asserts the controlled ValueError type without matching CPython's implementation-specific message. The strict length check remains in the legacy clone path.

  3. Empty layer data-loss — not a defect in this implementation. I reproduced the supported transformers==4.56.2 API with a real hybrid cache. DynamicCache(config=...) intentionally creates lazy DynamicLayer and DynamicSlidingWindowLayer instances whose keys and values are both None before their first update; this is valid cache state, not an unsupported empty layer. The clone now preserves those real layer types, sliding_window, cumulative_length, and the uninitialized None state, covered by test_clone_dynamic_cache_preserves_uninitialized_real_hybrid_layers. An object with neither supported cache representation still raises TypeError.

The underlying constructor failure is fixed in commit 93ec8b1e: clone_dynamic_cache copies the layer instance without invoking a constructor that may require sliding_window, then deep-copies non-tensor state and clones tensor attributes while preserving the existing naming precedence. The real populated hybrid regression also verifies that updating the clone does not mutate the original.

The separate AutoTest comment 5663159190 is classified as INCONCLUSIVE / non-blocking because its generated helper used the removed legacy direct-list API. Both test_kv.py and test_hf.py now use the public update API on modern transformers while retaining the legacy branch.

Verification:

  • Red before the source fix on transformers 4.56.2: 22 passed, 1 skipped, 2 failed with DynamicSlidingWindowLayer.__init__() missing sliding_window.
  • Project environment transformers 4.53.2: python -m pytest tests/memories/activation/test_kv.py tests/llms/test_hf.py -q23 passed, 2 skipped.
  • Modern transformers 4.56.2: the same command — 24 passed, 1 skipped.
  • Ruff check/format and all applicable pre-commit hooks — passed.

The new commit was pushed without rewriting history. The fork's Python workflow for the new head is run 34860047308, currently action_required pending maintainer approval; AutoTest is pending on Open Code Review, so those are external gating states rather than local test failures. Earlier findings remain covered by the preceding commits and replies.

@yetuge

yetuge commented Sep 14, 2026

Copy link
Copy Markdown
Author

Thanks for the latest Open Code Review pass. I re-read the current in-place version of comment 5629951647 at updated_at=2026-09-14T15:11:58Z (fingerprint sha256:aa2a2e4a38649680a09e32f334757e4e316b87bbe29aa6624e3d9aee55c11ba6) and checked all seven findings against head c1bd4a55.

  1. The HF isolation test should assert clone identity — fixed. The mock now captures the cache passed to forward and asserts it is present and is not the caller's kv_cache, in addition to the caller K/V shape checks. This directly proves HFLLM.generate() hands generation an independent cache.

  2. The HF legacy helper should use update() — fixed. The shared make_filled_cache() helper now populates both legacy and modern transformers caches through the public DynamicCache.update() API, so legacy bookkeeping is initialized consistently instead of being bypassed by direct list append.

  3. The version-branching helpers should be shared — fixed. The helpers are now centralized in tests/cache_helpers.py; both test_kv.py and test_hf.py import the same implementation, including the real hybrid-cache factory and value-layer count accessor.

  4. Legacy top-level tensor state should be copied — fixed. The legacy clone branch now clones every cache-level tensor attribute other than the K/V lists, while still deep-copying non-tensor state. test_clone_dynamic_cache_copies_legacy_tensor_state verifies storage independence on the legacy DynamicCache API.

  5. copy.copy(layer) is supposedly wasted — not changed deliberately. It is the constructor-safe compatibility boundary: it preserves any instance state held in slots or supplied by a custom __copy__, while avoiding DynamicSlidingWindowLayer.__init__() and its required sliding_window. The following vars() loop overwrites every dictionary attribute with an independent clone/deep copy. Replacing this with object.__new__ would be less compatible with layer types that do not keep all state in __dict__; this is an allocation tradeoff, not a correctness failure.

  6. The value-layer count assertion was weakened — fixed. test_get_cache_merge now checks both cache_layer_count(merged) == 1 and the explicit symmetric cache_value_layer_count(merged) == 1, as well as the value tensor itself.

  7. The hybrid test should skip intermediate versions without config= — fixed. The real hybrid-cache helper now catches TypeError only around DynamicCache(config=...) and skips with a clear reason. Supported 4.56.2 continues to execute the real hybrid regression rather than skipping.

The previous constructor regression remains covered: the pre-fix 4.56.2 run failed at DynamicSlidingWindowLayer.__init__() because the old code called type(layer)(); the fixed clone preserves the real layer instance and metadata. The current verification is:

  • Project transformers 4.53.2: python -m pytest tests/memories/activation/test_kv.py tests/llms/test_hf.py -q24 passed, 2 skipped.
  • Modern transformers 4.56.2: the same command — 24 passed, 2 skipped.
  • 26 tests collected; Ruff check/format and all applicable pre-commit hooks — passed.

The separate AutoTest comment 5663159190 remains INCONCLUSIVE / non-blocking: its generated test directly appended to the removed legacy key_cache API. The shared helper now uses DynamicCache.update() with the legacy-compatible path and the modern layers path.

Commit c1bd4a55 was pushed as a new commit without rewriting history. Earlier OCR findings remain covered by the preceding commits and replies. The fork Python workflow for this head is externally gated as action_required pending maintainer approval; AutoTest/OCR status is external gating, not a local test failure.

@Memtensor-AI

Copy link
Copy Markdown
Collaborator

✅ Automated Test Results: PASSED

All tests passed (22/22 executed, 4 skipped). memos_python_core/changed-repo-python: 22 passed, 4 skipped. Duration: 10s [advisory, non-gating] AI-generated tests on branch test/auto-gen-0cb44b46b1ce0f46-20260914233703: 49/49 passed — these do NOT affect the PR verdict; review the branch manually.

Branch: fix/activation-cache-aliasing

@Memtensor-AI Memtensor-AI added status:ready Ready for implementation; waiting for assignee or AI dispatch | 可进入实现,等待认领或派发 and removed status:in-progress Someone or AI is working on it | 人工或 AI 正在处理 labels Sep 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:memory 记忆存储、检索、更新、召回逻辑 area:model llm + embedder + reranker status:ready Ready for implementation; waiting for assignee or AI dispatch | 可进入实现,等待认领或派发

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The stored activation DynamicCache is mutated in place by generation, so activation memory grows every turn

4 participants