fix(memory)!: rename LocalMemoryStore to TestMemoryStore#3123
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
b57faa9 to
3bf8e27
Compare
|
Overall a clean, well-tested backwards-compatible rename. Two process/design points beyond the inline comments: API review label: This changes a public API contract (renaming exported classes/types customers construct). Per Naming vs. location (question, not blocking): The class is now |
|
Assessment: Comment (approve once the deprecation-message and API-label items are addressed) A clean, mechanical, and well-tested rename that preserves backwards compatibility correctly in both SDKs (Python identity via Review themes
Nicely scoped change with solid test coverage of the deprecation behavior on both sides. |
|
Question: Using data format for naming makes me wondering that will we have more storages for other data format ? |
|
@JackYPCOnline raises a good API-design point that's worth resolving before this merges, since it goes to the naming axis rather than the mechanics of the rename. There seem to be three candidate axes for naming this store:
The reason the format axis gives me pause: If that direction lands, This is exactly the kind of call that benefits from an |
|
@JackYPCOnline thanks for your comment, it made me check the notes and I realized we'd actually decided to name this |
|
Maybe i missed some conversation on this but: |
Just checked and you're right, it got released today only. I don't mind doing a breaking change in that case, @pgrayy wdyt? |
|
Re-reviewing after the pivot from Important — the
If the goal is to signal "not for production," names that convey that without the framework collision might serve better — e.g. On dropping backwards-compat entirely (re: @lizradway / @opieter-aws): if it genuinely shipped only today with ~zero adoption, a clean breaking rename is reasonable and would delete a lot of surface — the
Net: I'd resolve the name (with an API reviewer) and the keep-vs-break question first — several of my earlier inline comments become moot depending on those two outcomes. |
|
Re-reviewed the pivot to a clean breaking change. The mechanics are solid:
Dropping the deprecation shims was the right call given ~one-release-old adoption, and it moots my earlier deprecation-message inline comments (those files/aliases no longer exist). Two things I'd still like an explicit call on, now that @pgrayy is engaged — not blocking, but worth putting on record since a breaking rename is the expensive moment to get the name right:
Assessment: Comment — approve on the mechanics; the only open items are the naming/RFC calls above, which are a maintainer/API-review decision rather than a code issue. |
Description
LocalMemoryStorewas named for where it runs rather than what it is for. It's a zero-infrastructure, JSON-file-backed store meant for prototyping, offline use, and tests — not a production backend (each write rewrites the whole file; recall is lexical, not semantic). Renaming it toTestMemoryStoresignals that intended role at the call site and reads more clearly next to the production-gradeBedrockKnowledgeBaseStore.LocalMemoryStoreshipped only in the most recent release, so rather than carry a deprecation shim we make this a clean breaking change: the old names and the old import path are removed outright, with no aliases. This keeps the surface honest — there's no deprecated subclass, no__getattr__warning branch, and no lingeringlocalpath to migrate off later.The rename is applied across both SDKs in lockstep for cross-SDK parity. The subsystem directory is also renamed
local→test_memory_store(Python) /test-memory-store(TS) so the import path matches the class, mirroring the siblingbedrock_knowledge_base/convention. The on-disk JSON format and all record field names are unchanged, so a file written by the previous release still loads.Public API Changes (breaking)
Renamed symbols (both SDKs):
LocalMemoryStore→TestMemoryStore,LocalMemoryStoreConfig→TestMemoryStoreConfig,LocalMemoryAddResult→TestMemoryAddResult. The import path changes with the directory rename. The old names and the old path are gone.Breaking Changes
LocalMemoryStore/LocalMemoryStoreConfig/LocalMemoryAddResultand the.../vended-memory-stores/localimport path no longer exist. Because the on-disk format is unchanged, only import sites and type/constructor references need updating — no data migration.Migration
Implementation Note (reviewer callout)
Because the class name starts with
Test, pytest's default collection would try to loadTestMemoryStore(and its sibling types) as test suites. Each carries__test__ = Falseto opt out. For theTestMemoryStoreConfigTypedDict this is a post-class assignment with a scoped# type: ignore[attr-defined]— mypy strict rejects__test__both in a TypedDict body ([misc]) and post-class ([attr-defined]), so the suppression is load-bearing and correctly scoped. A regression test asserts__test__ is Falseon all three definitions so a future edit can't silently re-enable collection.Related Issues
Documentation PR
No documentation changes are needed: the store is documented via docstrings/TSDoc in the source, which are updated in this PR. The
team/designs/0014-storage.mdRFC references the old name in a few places; it is a point-in-time design doc and is intentionally left untouched to keep this change focused.Type of Change
Breaking change
Testing
Ran the affected unit suites in both SDKs (
hatch test tests/strands/vended_memory_stores/andvitest run src/vended-memory-stores/test-memory-store/) plus lint/type checks on the changed files, and re-ran the Python suite withPytestCollectionWarningescalated to errors to confirm theTest*classes are never collected. Verified the breaking change end to end from a REPL script: the new name and path import silently, while both the oldLocalMemoryStorename and the old.localimport path now raiseImportError/ModuleNotFoundError.hatch run prepareChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.