refactor(palace): migrate off _DEFAULT_BACKEND compat shim (#35) - #122
Conversation
Replace all 5 call sites that used the fork-only `_DEFAULT_BACKEND`
module attribute with direct `get_backend("chroma")` calls:
- mcp_server.py: cache invalidation (_clients.pop, _freshness.pop)
- mcp_server.py: close_palace in tool_reconnect
- benchmarks/mine_bench.py: cache reset between runs
- tests/test_backends.py: monkeypatch target
- tests/test_mcp_server.py: monkeypatch target
Delete the 14-line transitional shim and its comment block from
palace.py. The shim was added in PR #21 (cherry-pick of upstream
MemPalace#665) and is no longer needed.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request completes the migration away from the transitional Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request removes the _DEFAULT_BACKEND compatibility shim from mempalace/palace.py and migrates all call sites in the benchmarks, MCP server, and test suites to use the get_backend("chroma") abstraction. The reviewer suggested using the public close_palace() method in mempalace/mcp_server.py instead of manually manipulating internal cache attributes to ensure proper resource cleanup and better maintainability.
| from .palace import get_backend | ||
|
|
||
| _DEFAULT_BACKEND._clients.pop(_config.palace_path, None) | ||
| _DEFAULT_BACKEND._freshness.pop(_config.palace_path, None) | ||
| chroma_backend = get_backend("chroma") | ||
| chroma_backend._clients.pop(_config.palace_path, None) | ||
| chroma_backend._freshness.pop(_config.palace_path, None) |
There was a problem hiding this comment.
Instead of manually popping from internal attributes ._clients and ._freshness, use the public close_palace() method. This is more maintainable and ensures that _close_client() is called to properly release SQLite file locks, which is particularly important for Windows compatibility.
| from .palace import get_backend | |
| _DEFAULT_BACKEND._clients.pop(_config.palace_path, None) | |
| _DEFAULT_BACKEND._freshness.pop(_config.palace_path, None) | |
| chroma_backend = get_backend("chroma") | |
| chroma_backend._clients.pop(_config.palace_path, None) | |
| chroma_backend._freshness.pop(_config.palace_path, None) | |
| from .palace import get_backend | |
| get_backend("chroma").close_palace(_config.palace_path) |
There was a problem hiding this comment.
Considered this — the manual pop is intentional here. _force_chroma_cache_reset() is a targeted retry-path operation that only drops the per-palace-path cache entries (client handle + freshness timestamp) without closing the underlying client. close_palace() does more: it calls _close_client() which releases file locks and tears down the chromadb PersistentClient. The retry path wants a cache miss on the next call (so get_collection rebuilds), not a full client teardown — reopening a PersistentClient is slower and could interfere with concurrent readers.
Note that tool_reconnect (line ~2430) already uses close_palace() for the full-teardown case. The two patterns serve different purposes.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
_DEFAULT_BACKENDmodule attribute to directget_backend("chroma")callspalace.pyCall sites migrated
mempalace/mcp_server.py(×2)_clients.pop,_freshness.pop) +close_palacein reconnectbenchmarks/mine_bench.pytests/test_backends.pymonkeypatch.setattrtargettests/test_mcp_server.pymonkeypatch.setattrtargetContext
The shim was added in PR #21 (cherry-pick of upstream MemPalace#665, commit
5e90c72) as a transitional bridge. This PR completes the planned migration.Test plan
_DEFAULT_BACKENDreferences remainCloses #35
🤖 Generated with Claude Code