Conversation
Problem ------- `LightMemory.offline_update(..., offline_update_trigger=True)` raises `TypeError: LightMemory.offline_update_all_entries() got an unexpected keyword argument 'update_sim_threshold'`. The scheduled offline-update path is therefore unreachable: any caller that enables the trigger crashes before a single entry is updated. Root cause ---------- `offline_update` (src/lightmem/memory/lightmem.py:455) passed `update_sim_threshold=0.8`, but `offline_update_all_entries` (src/lightmem/memory/lightmem.py:541) declares its parameter as `score_threshold`. The two names were never reconciled and no test exercised the trigger branch, so the mismatch went unnoticed. Approach -------- Forward the same 0.8 value under the callee's real keyword, `score_threshold`. This preserves the behaviour the call site always intended (an explicit threshold of 0.8 rather than the callee default of 0.9) and is the smallest change that makes the branch executable. Adds `tests/test_lightmem_offline_update.py`, which drives the real `LightMemory.offline_update` method on a stubbed instance and patches `offline_update_all_entries` with `autospec=True`, so the recorded call is bound against the callee's real signature. A rename on either side of the pair therefore fails the test rather than passing silently. Verification ------------ `PYTHONPATH=src python -m pytest tests/test_lightmem_offline_update.py -q` -> 1 passed, 1 warning. `PYTHONPATH=src python -m pytest tests -q` -> 3 passed, 1 warning. The warning is a pre-existing Pydantic V2 deprecation raised by `src/lightmem/configs/logging/base.py`. Reverting the one-line source change with the test in place fails it with `TypeError: got an unexpected keyword argument 'update_sim_threshold'`; renaming the callee parameter instead fails it with the mirror-image `'score_threshold'` error. black, isort (black profile) and flake8 (88 columns) are clean on the new test; the modified module compiles and its flake8 findings are a strict subset of the same file before this change; `git diff --check` is clean. Impact ------ Offline batch updates can now run. No public API or default value changes; the threshold actually applied is unchanged from what the call site always intended.
Author
|
Closing as a duplicate of #80, which makes the same fix and was opened first. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Symptom
LightMemory.offline_update(..., offline_update_trigger=True)never updates anything. It raisesbefore the first entry is touched:
So the scheduled/triggered offline-update path documented on the public
offline_updatesignature(
src/lightmem/memory/lightmem.py:399) is unreachable — enabling the flag is a guaranteed crash, not aslow or partial update.
Root cause
The two halves of one call disagree on the keyword name. Line numbers are on the base commit
8449d57:src/lightmem/memory/lightmem.py:455-457— the trigger branch callsself.offline_update_all_entries(update_sim_threshold = 0.8).src/lightmem/memory/lightmem.py:541— the callee declaresdef offline_update_all_entries(self, score_threshold: float = 0.9, max_workers: int = 5):.score_thresholdis the settled name, notupdate_sim_threshold: every other call site in therepository already uses it —
README.md:327,experiments/longmemeval/offline_update.py:41,experiments/locomo/add_locomo.py:447,mcp/server.py:199-201, andweb/backend/app/instance.py:259.update_sim_thresholdappears exactly once in the tree, at thebroken call site (
git grep -c update_sim_thresholdreports a single line insrc/lightmem/memory/lightmem.pyon the base commit).Nothing in the repository passes
offline_update_trigger=True(git grep -n 'offline_update_trigger\s*=\s*True'returns nothing at the base commit), and the web backend reachesthe batch update through
offline_update_all_entriesdirectly rather than throughLightMemory.offline_update. That is why the mismatch survived: no in-tree caller and no test walkedthe branch.
What this change does
Two files,
2 files changed, 23 insertions(+), 1 deletion(-):src/lightmem/memory/lightmem.py:455-457— forward the same value under the callee's real keyword:score_threshold=0.8.tests/test_lightmem_offline_update.py(new, 22 lines) — one regression test that drives the realLightMemory.offline_updateand asserts the keyword that reaches the callee.Trade-off a reviewer should weigh
I kept the explicit
0.8. The alternative is to drop the argument entirely and let the callee defaultof
0.9apply. I chose0.8because it is the value the call site has always tried to pass, and itmatches most of what the other paths pass (
README.md:327andexperiments/longmemeval/offline_update.py:41both use0.8, andweb/backend/app/api.py:185defaults its
score_thresholdrequest field to0.8; the one in-tree exception isexperiments/locomo/add_locomo.py:447, which passes0.9). Since this branch was never executable,either choice is technically a free pick — if you would rather the trigger inherit the callee default, that is a
one-line change and the test's expected value moves with it.
The test uses
unittest.mock.patch.object(LightMemory, "offline_update_all_entries", autospec=True)rather than a bare
Mock().autospecbinds the recorded call against the callee's realsignature, so a rename on either side of the pair fails the test. A bare
Mock()would accept anykeyword and would only pin what the caller passes — both directions are demonstrated in the revert
proof below.
Testing
Environment: macOS (arm64), CPython 3.11.15 (the repo requires
>=3.10,<3.12,pyproject.toml:10),pytest 9.1.1, black 26.5.1, isort 9.0.1, flake8 7.3.0 — the four tools the
devextra declares(
pyproject.toml). Everything below was run in a cleangit worktreecheckout of this branch.The repository has no
conftest.pyand no pytest configuration, andlightmemis not installed inthe environment, so
PYTHONPATH=srcis required for collection to succeed at all. That is apre-existing condition of the checkout, not something this branch introduces.
The base commit's
tests/contains one file and runs2 passed; the delta is exactly the onenew test. The single warning is a pre-existing
PydanticDeprecatedSince20raised fromsrc/lightmem/configs/logging/base.py:7; it does not appear on base only because base's two testsnever import that module — importing
lightmem.configs.baseon the unmodified base commit emits theidentical warning.
The repository ships no
[tool.black],[tool.isort],setup.cfg,tox.inior.flake8, so thoseare black's own default line length and the black-compatible isort profile;
--max-line-length=88ispassed to flake8 so it agrees with black rather than with flake8's default of 79.
Flake8 on the modified module, base commit versus this branch:
Diffing the two sorted finding lists shows two deletions and nothing added:
That is the
update_sim_threshold = 0.8spacing disappearing with the rewritten call. The file is nototherwise reformatted, and this branch does not make it flake8-clean — 181 pre-existing findings
remain and are deliberately left alone.
Revert proof
Both mutations were applied in the same worktree with the new test file left in place, and reverted
afterwards.
1. Revert the one-line fix (
git checkout 8449d57 -- src/lightmem/memory/lightmem.py):Restoring the fix:
1 passed, 1 warning.2. Rename the callee parameter instead, keeping the fix — this is the direction a bare
Mock()would not catch:
Restored afterwards;
git status --shortclean andPYTHONPATH=src python -m pytest tests -qback to3 passed, 1 warning.What was NOT verified
offline_update_all_entriesnever runs. The test asserts the keyword that crossesthe call boundary and stops there. Actually consolidating entries needs a populated vector store,
which I do not have. This PR proves the branch is reachable, not that the update it performs is
correct.
LightMemoryvia__new__with a stub config and a mock logger; no factory and no network call isexercised. The
openaiextra is not installed in my environment, so any path that builds a realmemory manager fails on import before reaching this code.
offline_update_all_entriesdirectly and were already passingscore_threshold; they are cited asevidence of the intended name, not re-tested.
and I did not run it; nothing here is version-sensitive as far as I can tell, but I did not check.
did not time anything.
Checklist
This repository ships no pull request template and no
CONTRIBUTING.md(there is no.github/directory at all), so the items below come from the one contribution rule stated in the README:
"We welcome contributions from the community! If you'd like to contribute, please fork the repository
and submit a pull request. For major changes, please open an issue first to discuss what you would
like to change." (
README.md:614).tests/test_lightmem_offline_update.py, failing in both mutation directions above.black/isort --profile black/flake8 --max-line-length=88clean on the new test file.PYTHONPATH=src pytest testsgreen (3 passed), with no pre-existing test disturbed.I did not read it as a "major change" under the README rule. Happy to open one if you would
prefer every change tracked by an issue.
src/lightmem/memory/lightmem.pystill has 181 pre-existing flake8 findings; fixing them herewould bury a one-line change in noise.