refactor(backends): RFC 001 §10 cleanup — typed results, PalaceRef, registry#995
Merged
refactor(backends): RFC 001 §10 cleanup — typed results, PalaceRef, registry#995
Conversation
…nd registry (RFC 001 §10) Advances RFC 001 §10 cleanup so backend-author PRs (#574 LanceDB, #665 Postgres, #700 Qdrant, #697 hosted, #643 PalaceStore, #381 Qdrant) have a stable target to align against. Scope (this PR): - Typed QueryResult / GetResult dataclasses replace Chroma's dict shape at the BaseCollection boundary (§1.3). A transitional _DictCompatMixin keeps existing callers working while the attribute-access migration proceeds. - BaseCollection is now kwargs-only across add/upsert/query/get/delete/update with ABC defaults for estimated_count/close/health and a non-atomic default update() (§1.1–1.2). - PalaceRef replaces raw path strings at the backend boundary (§2.2). - BaseBackend ABC with get_collection/close_palace/close/health/detect (§2.3). - mempalace.backends entry-point group + in-tree registry with resolve_backend_for_palace priority order matching §3.2–3.3. - ChromaCollection normalizes chroma returns into typed results; unknown where-clause operators raise UnsupportedFilterError (no silent drop, §1.4). - ChromaBackend absorbs the inode/mtime client-cache freshness check previously duplicated in mcp_server._get_client() (§10 + PR #757). - searcher.py migrated to typed-attribute access as the reference call site; remaining callers land in a follow-up. - pyproject: chroma registered via [project.entry-points."mempalace.backends"]. Out of scope (explicit follow-ups): - Full caller migration off the dict-compat shim across palace.py, mcp_server.py, miner.py, convo_miner.py, dedup.py, repair.py, exporter.py, palace_graph.py, cli.py, closet_llm.py. - Embedder injection + three-state EmbedderIdentityMismatchError check (§1.5). - maintenance_state() / run_maintenance() benchmark hooks (§7.3). - AbstractBackendContractSuite full coverage (§7.1–7.2). - mempalace migrate / mempalace verify CLI rewrites through BaseCollection (§8). Tests: 970 passed (up from 967 on develop); new coverage for typed results, empty-result outer-shape preservation, \$regex rejection, registry lookup, priority resolver, and PalaceRef-kwarg ChromaBackend.get_collection. Refs: #743 (RFC 001), #989 (RFC 002 tracking issue).
Contributor
There was a problem hiding this comment.
Pull request overview
This PR lands the RFC 001 §10 “backend seam cleanup” in core: it formalizes the backend contract (typed QueryResult/GetResult, PalaceRef, kwargs-only collection APIs), adds an entry-point–based backend registry, updates the in-tree Chroma backend to implement the new surface (including where-operator validation and client freshness caching), and migrates searcher.py to typed-result attribute access while remaining compatible with legacy dict-shaped mocks.
Changes:
- Introduce typed result dataclasses,
PalaceRef, and a consolidatedBaseBackend/BaseCollectioncontract. - Add backend registry + entry-point discovery and wire in the default
chromaentry point. - Update Chroma backend/collection to normalize dict responses into typed results and enforce unsupported where-operator errors; migrate
searcher.pycall sites accordingly.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
mempalace/backends/base.py |
Defines the RFC-backed backend/collection ABCs, typed results, and shared error/value types. |
mempalace/backends/chroma.py |
Updates Chroma backend + collection adapter to implement the new contract and typed result normalization. |
mempalace/backends/registry.py |
Adds registry, entry-point discovery, and backend resolution logic. |
mempalace/backends/__init__.py |
Exposes the new public backend surface and registry helpers. |
mempalace/searcher.py |
Migrates to attribute-access for typed results while keeping _first_or_empty polymorphic. |
pyproject.toml |
Declares the mempalace.backends entry point for the in-tree Chroma backend. |
tests/test_backends.py |
Adds/updates tests for typed results, empty-shape semantics, registry behavior, and Chroma detect/PalaceRef integration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+250
to
+266
| if documents is None and metadatas is None and embeddings is None: | ||
| raise ValueError("update requires at least one of documents, metadatas, embeddings") | ||
|
|
||
| existing = self.get(ids=ids, include=["documents", "metadatas"]) | ||
| by_id = { | ||
| rid: (existing.documents[i], existing.metadatas[i]) | ||
| for i, rid in enumerate(existing.ids) | ||
| } | ||
| merged_docs: list[str] = [] | ||
| merged_metas: list[dict] = [] | ||
| for i, rid in enumerate(ids): | ||
| prev_doc, prev_meta = by_id.get(rid, ("", {})) | ||
| merged_docs.append(documents[i] if documents is not None else prev_doc) | ||
| new_meta = dict(prev_meta or {}) | ||
| if metadatas is not None: | ||
| new_meta.update(metadatas[i] or {}) | ||
| merged_metas.append(new_meta) |
There was a problem hiding this comment.
BaseCollection.update()'s default implementation indexes documents[i] / metadatas[i] for each id without validating list lengths. If a caller passes ids and metadatas of different lengths (or documents of different length), this will raise IndexError or silently merge incorrect pairs. Consider validating that any provided documents / metadatas / embeddings lists have len(...) == len(ids) and raising a clear ValueError otherwise.
Comment on lines
+343
to
+350
| cached = self._clients.get(palace_path) | ||
| cached_inode, cached_mtime = self._freshness.get(palace_path, (0, 0.0)) | ||
| current_inode, current_mtime = self._db_stat(palace_path) | ||
|
|
||
| inode_changed = current_inode != 0 and cached_inode != 0 and current_inode != cached_inode | ||
| mtime_changed = ( | ||
| current_mtime != 0.0 and cached_mtime != 0.0 and current_mtime > cached_mtime | ||
| ) |
There was a problem hiding this comment.
ChromaBackend's inode/mtime freshness check currently won't invalidate the cached client in several important cases: (1) if chroma.sqlite3 didn't exist when the client was first created, _freshness stores (0, 0.0) and subsequent mtime changes will never trigger because cached_mtime != 0.0 is required; (2) if the DB file is deleted/rebuilt and _db_stat() returns (0, 0.0), both change flags remain false and the stale cached client can be reused. Consider matching mcp_server._get_client() semantics: explicitly invalidate when the DB file is missing while a cached client exists, and treat a transition from cached 0→nonzero stat as a change (optionally with a small epsilon for mtime).
Comment on lines
+146
to
+177
| def query( | ||
| self, | ||
| *, | ||
| query_texts=None, | ||
| query_embeddings=None, | ||
| n_results=10, | ||
| where=None, | ||
| where_document=None, | ||
| include=None, | ||
| ) -> QueryResult: | ||
| _validate_where(where) | ||
| _validate_where(where_document) | ||
|
|
||
| spec = _IncludeSpec.resolve(include, default_distances=True) | ||
| chroma_include: list[str] = [] | ||
| if spec.documents: | ||
| chroma_include.append("documents") | ||
| if spec.metadatas: | ||
| chroma_include.append("metadatas") | ||
| if spec.distances: | ||
| chroma_include.append("distances") | ||
| if spec.embeddings: | ||
| chroma_include.append("embeddings") | ||
|
|
||
| kwargs: dict[str, Any] = { | ||
| "n_results": n_results, | ||
| "include": chroma_include, | ||
| } | ||
| if query_texts is not None: | ||
| kwargs["query_texts"] = query_texts | ||
| if query_embeddings is not None: | ||
| kwargs["query_embeddings"] = query_embeddings |
There was a problem hiding this comment.
ChromaCollection.query() doesn't validate the query inputs. If both query_texts and query_embeddings are None (or both are provided), the call into Chroma may fail with a non-obvious error and num_queries defaults to 1 even though no query was specified. Consider raising a ValueError unless exactly one of query_texts / query_embeddings is provided (and ensuring the chosen one is non-empty).
Comment on lines
+136
to
+143
| def empty(cls, num_queries: int = 1) -> "QueryResult": | ||
| """Construct an all-empty result preserving outer dimension.""" | ||
| return cls( | ||
| ids=[[] for _ in range(num_queries)], | ||
| documents=[[] for _ in range(num_queries)], | ||
| metadatas=[[] for _ in range(num_queries)], | ||
| distances=[[] for _ in range(num_queries)], | ||
| embeddings=None, |
There was a problem hiding this comment.
QueryResult.empty() always sets embeddings=None. This conflicts with the class docstring which says fields not requested via include= are the ones that should be omitted/None (with embeddings being None when not requested). If include requests embeddings and the result is empty, callers would reasonably expect embeddings to preserve the outer dimension (e.g. [[] for _ in range(num_queries)]) rather than None. Consider either (a) adding an embeddings_requested/include parameter to empty() to build the spec-correct empty embeddings shape when requested, or (b) adjusting the docstring/spec for empty embeddings.
Suggested change
| def empty(cls, num_queries: int = 1) -> "QueryResult": | |
| """Construct an all-empty result preserving outer dimension.""" | |
| return cls( | |
| ids=[[] for _ in range(num_queries)], | |
| documents=[[] for _ in range(num_queries)], | |
| metadatas=[[] for _ in range(num_queries)], | |
| distances=[[] for _ in range(num_queries)], | |
| embeddings=None, | |
| def empty( | |
| cls, num_queries: int = 1, embeddings_requested: bool = False | |
| ) -> "QueryResult": | |
| """Construct an all-empty result preserving outer dimension. | |
| When ``embeddings_requested`` is true, ``embeddings`` preserves the | |
| outer query dimension with empty hit lists; otherwise it remains | |
| ``None`` to indicate embeddings were not requested. | |
| """ | |
| return cls( | |
| ids=[[] for _ in range(num_queries)], | |
| documents=[[] for _ in range(num_queries)], | |
| metadatas=[[] for _ in range(num_queries)], | |
| distances=[[] for _ in range(num_queries)], | |
| embeddings=[[] for _ in range(num_queries)] | |
| if embeddings_requested | |
| else None, |
Four defects surfaced by the automated review, fixed with targeted tests: 1. BaseCollection.update() default now validates that documents / metadatas / embeddings lengths match ids, raising ValueError instead of silently misaligning pairs or raising IndexError (base.py). 2. ChromaCollection.query() now rejects the two ambiguous input shapes up front — neither or both of query_texts / query_embeddings, and empty input lists — with clear ValueError messages rather than delegating to chromadb's less-obvious errors (chroma.py). 3. QueryResult.empty() accepts embeddings_requested=True to preserve the outer-query dimension with empty hit lists when the caller asked for embeddings, matching the spec rule that included fields carry the outer shape even when empty (base.py). ChromaCollection.query() threads this through on the empty-result path (chroma.py). 4. ChromaBackend cache-freshness check now matches the semantics from mcp_server._get_client (merged via #757) on three edge cases Copilot called out: (a) invalidate when chroma.sqlite3 disappears while a cached client is held, (b) treat a 0→nonzero stat transition as a change so a cache built when the DB did not yet exist is refreshed, (c) re-stat after PersistentClient constructs the DB lazily so freshness reflects the post-creation state (chroma.py). Tests: 978 passed (up from 970), 8 new tests covering the fixes.
Member
Author
|
Thanks @copilot — all four review items addressed in 42b940d. Recap:
Tests: 978 passed (up from 970), +8 targeted tests. Lint + format clean. CI is running — ping when green if you want me to mark ready. |
…ests test_base_collection_update_default_validates_list_lengths and test_base_collection_update_default_rejects_mismatched_lengths were spinning up a real ChromaBackend and calling add(documents=...), which triggered ChromaDB's default ONNX embedding function and attempted a network download — failing in offline/sandboxed CI. BaseCollection.update() validates list lengths before any DB access, so no items need to be pre-loaded for the length-check to fire. Switch both tests to use _FakeCollection (same as the rest of the unit tests in this file) so they are pure in-memory and network-free. Also fixes a structural bug in test 1: collection._collection.add() was accidentally placed inside the pytest.raises(ValueError) block, masking the real assertion. Agent-Logs-Url: https://github.com/MemPalace/mempalace/sessions/55fc663e-b256-4b8b-88ce-4271560def8d Co-authored-by: igorls <[email protected]>
Contributor
Two of the eight new tests were failing in CI: Fixed in 24bf97b by switching both tests to use Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
PermissionError [WinError 32] on Windows when Path.unlink() runs while chromadb.PersistentClient still holds a handle on chroma.sqlite3. Rewrite test_chroma_cache_invalidates_when_db_file_missing to prime backend._clients/_freshness with a sentinel object instead of opening a real PersistentClient, so the unlink runs against an unheld file. The assertion is also corrected: after invalidation, ChromaBackend's _client rebuilds a fresh PersistentClient which re-creates chroma.sqlite3 and re-stats it, so freshness ends up at the post-rebuild stat (not (0, 0.0) as the assertion previously expected). The meaningful invariant is "freshness advanced past the pre-unlink value AND the sentinel was replaced", which the test now checks. Ref: Windows CI failure on 995.
24bf97b (network-download fix) and my earlier Copilot-review commit both added tests for the same ValueError. Keep the broader one that covers both 'documents length' and 'metadatas length' mismatches; drop the narrower duplicate.
igorls
added a commit
that referenced
this pull request
Apr 18, 2026
…ry, PalaceContext Lands the read-side contract so third-party adapter authors (@Perseusxrltd, @JakobSachs, @adv3nt3, @zendesk-thittesdorf, @mfhens, @roip, @MrDys) have a stable target matching what RFC 001 §10 landed on the write side in #995. Scope (this PR): - mempalace/sources/base.py: BaseSourceAdapter ABC with kwargs-only ingest() / describe_schema() and default is_current() / source_summary() / close() (§1.1–1.2). Typed records: SourceRef, SourceItemMetadata, DrawerRecord, RouteHint, SourceSummary, AdapterSchema, FieldSpec (§1.3, §5.2). Error classes: SourceNotFoundError, AuthRequiredError, AdapterClosedError, TransformationViolationError, SchemaConformanceError (§2.7). Class-level identity contract: name / adapter_version / capabilities / supported_modes / declared_transformations / default_privacy_class (§2.1, §1.4, §1.5, §6). - mempalace/sources/transforms.py: reference implementations of the 13 reserved transformations (§1.4) — utf8_replace_invalid, newline_normalize, whitespace_trim, whitespace_collapse_internal, line_trim, line_join_spaces, blank_line_drop — as pure functions, plus identity shims for the six adapter-specific ones (strip_tool_chrome, tool_result_truncate, tool_result_omitted, spellcheck_user, synthesized_marker, speaker_role_assignment) that the conversations adapter will override when migrated. get_transformation(name) resolves by reserved name. - mempalace/sources/registry.py: entry-point discovery via importlib.metadata.entry_points(group="mempalace.sources") + explicit register()/unregister() surface (§3.1–3.2). resolve_adapter_for_source() implements the §3.3 priority order; crucially, no auto-detection on the read side (§3.3 is explicit about that — user intent never inferred from on-disk artifacts). - mempalace/sources/context.py: PalaceContext facade (§9) bundling the drawer/closet collections, knowledge graph, palace path, adapter identity, and progress hooks core passes into adapter.ingest(). upsert_drawer() applies the spec-mandated adapter_name/adapter_version stamps from §5.1. skip_current_item() signals laziness; emit() dispatches to hooks and swallows hook exceptions. - mempalace/knowledge_graph.py: add_triple() gains optional source_drawer_id and adapter_name kwargs (§5.5). Backwards-compatible column migration auto-adds the new columns on open of a pre-RFC 002 palace (PRAGMA table_info then ALTER TABLE ADD COLUMN), matching the pattern used for any new palace-side provenance fields. - pyproject.toml: mempalace.sources entry-point group declared. Empty on the first-party side for now — miners migrate in a follow-up; the group being present means third-party packages can begin registering today. Out of scope (explicit follow-ups): - miner.py → mempalace/sources/filesystem.py. Behavior-preserving rename that also moves READABLE_EXTENSIONS, detect_room(), detect_hall() into the adapter (§9). Larger refactor; lands separately. - convo_miner.py + normalize.py → mempalace/sources/conversations.py. The format-detection if-chain in normalize.py becomes per-format plugins; declared_transformations enumerates what the current pipeline already does to source bytes (§1.4 existing-code mapping). - Closet post-step wired into the conversations adapter (§1.7). - CLI --source flag + --mode deprecation alias (§3.3). - MCP mempalace_mine tool source parameter. - AbstractSourceAdapterContractSuite (§7.1–7.3): byte-preservation round- trip and declared-transformation round-trip tests. - Privacy-class floor enforcement (§6.2); depends on #389 for secrets_possible scanning. Tests: 1018 passed (up from ~990 on develop), +27 targeted tests covering the ABC instantiation rules, typed records, all reserved transformations, the registry register/get/unregister surface, PalaceContext upsert + skip + emit semantics, and both the new KG provenance kwargs and backwards- compatible legacy-schema migration. Refs: #989 (RFC 002 tracking), #990 (RFC 002 spec), #995 (RFC 001 §10 cleanup — sibling PR on the write side).
4 tasks
6 tasks
jphein
added a commit
to jphein/mempalace
that referenced
this pull request
Apr 18, 2026
README: - Fork-changes table: expand the None-metadata row to cover all 8 sites (searcher.py CLI + API + closet-boost, miner.status, 4 mcp_server handlers). Previous row only called out the CLI print path. - Add a new Search row: warnings + sqlite BM25 top-up contract (the "never silent miss" feature) with pointer to MemPalace#951 + MemPalace#823. - Open-PR table: expand MemPalace#999 scope line to mention 8 sites + architectural note, update MemPalace#1000 to reflect post-MemPalace#995 rebase, add MemPalace#1005 with Copilot fixes noted. CLAUDE.md: - PR status header: 7 open -> 8 open (adds MemPalace#1005). - Same PR row updates as README for MemPalace#999/MemPalace#1000/MemPalace#1005. - Fork Changes list: expand entry 11 (None guards) to 8 sites + adapter consolidation proposal on MemPalace#999; add entry 14 for the warnings+BM25 feature; keep 12 and 13 as-is. 42 README-claim tests still pass.
skuznetsov
pushed a commit
to skuznetsov/mempalace
that referenced
this pull request
Apr 19, 2026
Rebuild the stage 2 PostgreSQL backend on top of the RFC 001 backend contract merged in MemPalace#995 instead of carrying forward the stale Chroma-shaped adapter. The new backend implements BaseBackend/BaseCollection with PalaceRef, typed QueryResult/GetResult returns, registry discovery, and env/config backend selection while keeping Chroma as the default path.\n\nThe PostgreSQL collection supports both pg_sorted_heap and pgvector. It prefers pg_sorted_heap when the extension is installed, falls back to pgvector when only vector is installed, writes rows with INSERT ... SELECT FROM unnest(...) and ON CONFLICT, preserves first-wins semantics for add() and last-wins semantics for upsert(), supports required metadata filters, returns optional embeddings, keeps exact count() separate from estimated_count(), and creates vector indexes lazily after the threshold. Text embedding reuses Chroma's default local embedding function so the postgres extra only adds psycopg2-binary instead of a second ML dependency stack.\n\nAdd user-facing PostgreSQL backend documentation, a PGXS install helper for pg_sorted_heap/pgvector, README links, lockfile metadata for the postgres extra, and unit coverage for registry exposure, config/env routing, PostgreSQL validation, filter translation, batch upsert, typed result shapes, estimated count, and palace-level backend selection.\n\nVerification:\n- uv run --frozen python -m pytest tests/ -q --ignore=tests/benchmarks -> 1011 passed\n- uv run --frozen python -m pytest tests/test_backends.py tests/test_config.py tests/test_config_extra.py -q -> 77 passed\n- ruff check mempalace/backends/postgres.py mempalace/backends/registry.py mempalace/backends/__init__.py mempalace/config.py mempalace/palace.py tests/test_backends.py tests/test_config_extra.py\n- ruff format --check mempalace/backends/postgres.py mempalace/backends/registry.py mempalace/backends/__init__.py mempalace/config.py mempalace/palace.py tests/test_backends.py tests/test_config_extra.py\n- git diff --check && uv lock --check && bash -n scripts/install_pg_backend.sh\n- uv run --frozen --extra postgres import smoke for psycopg2/PostgresBackend\n- local PostgreSQL 18 integration smoke with pg_sorted_heap: add/upsert/query/get/delete/health and sorted_heap table AM\n- local PostgreSQL 18 integration smoke with pgvector fallback: add/query/count and heap table AM\n- local PostgreSQL 18 forced-threshold vector-index smoke for sorted_hnsw and hnsw\n- secret-pattern scan across changed files
skuznetsov
pushed a commit
to skuznetsov/mempalace
that referenced
this pull request
Apr 19, 2026
Rebuild the stage 2 PostgreSQL backend on top of the RFC 001 backend contract merged in MemPalace#995 instead of carrying forward the stale Chroma-shaped adapter. The new backend implements BaseBackend/BaseCollection with PalaceRef, typed QueryResult/GetResult returns, registry discovery, and env/config backend selection while keeping Chroma as the default path.\n\nThe PostgreSQL collection supports both pg_sorted_heap and pgvector. It prefers pg_sorted_heap when the extension is installed, falls back to pgvector when only vector is installed, writes rows with INSERT ... SELECT FROM unnest(...) and ON CONFLICT, preserves first-wins semantics for add() and last-wins semantics for upsert(), supports required metadata filters, returns optional embeddings, keeps exact count() separate from estimated_count(), and creates vector indexes lazily after the threshold. Text embedding reuses Chroma's default local embedding function so the postgres extra only adds psycopg2-binary instead of a second ML dependency stack.\n\nAdd user-facing PostgreSQL backend documentation, a PGXS install helper for pg_sorted_heap/pgvector, README links, lockfile metadata for the postgres extra, and unit coverage for registry exposure, config/env routing, PostgreSQL validation, filter translation, batch upsert, typed result shapes, estimated count, and palace-level backend selection.\n\nVerification:\n- uv run --frozen python -m pytest tests/ -q --ignore=tests/benchmarks -> 1011 passed\n- uv run --frozen python -m pytest tests/test_backends.py tests/test_config.py tests/test_config_extra.py -q -> 77 passed\n- ruff check mempalace/backends/postgres.py mempalace/backends/registry.py mempalace/backends/__init__.py mempalace/config.py mempalace/palace.py tests/test_backends.py tests/test_config_extra.py\n- ruff format --check mempalace/backends/postgres.py mempalace/backends/registry.py mempalace/backends/__init__.py mempalace/config.py mempalace/palace.py tests/test_backends.py tests/test_config_extra.py\n- git diff --check && uv lock --check && bash -n scripts/install_pg_backend.sh\n- uv run --frozen --extra postgres import smoke for psycopg2/PostgresBackend\n- local PostgreSQL 18 integration smoke with pg_sorted_heap: add/upsert/query/get/delete/health and sorted_heap table AM\n- local PostgreSQL 18 integration smoke with pgvector fallback: add/query/count and heap table AM\n- local PostgreSQL 18 forced-threshold vector-index smoke for sorted_hnsw and hnsw\n- secret-pattern scan across changed files
skuznetsov
pushed a commit
to skuznetsov/mempalace
that referenced
this pull request
Apr 19, 2026
Rebuild the stage 2 PostgreSQL backend on top of the RFC 001 backend contract merged in MemPalace#995 instead of carrying forward the stale Chroma-shaped adapter. The new backend implements BaseBackend/BaseCollection with PalaceRef, typed QueryResult/GetResult returns, registry discovery, and env/config backend selection while keeping Chroma as the default path. The PostgreSQL collection supports both pg_sorted_heap and pgvector. It prefers pg_sorted_heap when the extension is installed, falls back to pgvector when only vector is installed, writes rows with INSERT ... SELECT FROM unnest(...) and ON CONFLICT, preserves first-wins semantics for add() and last-wins semantics for upsert(), supports required metadata filters, returns optional embeddings, keeps exact count() separate from estimated_count(), and creates vector indexes lazily after the threshold. Text embedding reuses Chroma's default local embedding function so the postgres extra only adds psycopg2-binary instead of a second ML dependency stack. Add user-facing PostgreSQL backend documentation, a PGXS install helper for pg_sorted_heap/pgvector, README links, lockfile metadata for the postgres extra, and unit coverage for registry exposure, config/env routing, PostgreSQL validation, filter translation, batch upsert, typed result shapes, estimated count, and palace-level backend selection. Verification: - uv run --frozen python -m pytest tests/ -q --ignore=tests/benchmarks -> 1044 passed, 1 warning - uv run --frozen python -m pytest tests/test_backends.py tests/test_config.py tests/test_config_extra.py -q -> 77 passed - ruff check mempalace/backends/postgres.py mempalace/backends/registry.py mempalace/backends/__init__.py mempalace/config.py mempalace/palace.py tests/test_backends.py tests/test_config_extra.py - ruff format --check mempalace/backends/postgres.py mempalace/backends/registry.py mempalace/backends/__init__.py mempalace/config.py mempalace/palace.py tests/test_backends.py tests/test_config_extra.py - git diff --check && uv lock --check && bash -n scripts/install_pg_backend.sh - uv run --frozen --extra postgres import smoke for psycopg2/PostgresBackend - local PostgreSQL 18 integration smoke with pg_sorted_heap: add/upsert/query/get/delete/health and sorted_heap table AM - local PostgreSQL 18 integration smoke with pgvector fallback: add/query/count and heap table AM - local PostgreSQL 18 forced-threshold vector-index smoke for sorted_hnsw and hnsw - secret-pattern scan across changed files
3 tasks
igorls
added a commit
that referenced
this pull request
Apr 19, 2026
Version bumps across pyproject.toml, mempalace/version.py, README badge, uv.lock, and plugin manifests (.claude-plugin/*, .codex-plugin/*). CHANGELOG aligned with main (post-3.3.1) and a new [3.3.2] section added covering the 11 PRs merged on develop since v3.3.1 — silent-transcript-drop fix + tandem sweeper (#998), None-metadata guards (#999, #1013), chromadb ≥1.5.4 for Py 3.13/3.14 (#1010), Windows Unicode (#681), HNSW quarantine recovery (#1000), PID stacking guard (#1023), doc-path cleanup (#996, #1012), and RFC 001/002 internal scaffolding (#995, #1014, #990).
2 tasks
jphein
added a commit
to jphein/mempalace
that referenced
this pull request
Apr 24, 2026
…alace#1072 exist, not a write-from-scratch Previous version claimed Postgres was "out of scope — requires writing a new BaseBackend implementation". That was wrong: @skuznetsov's MemPalace#665 already ships a PostgreSQL backend (pg_sorted_heap preferred, pgvector fallback), and @malakhov-dmitrii's MemPalace#1072 wires MEMPALACE_BACKEND env var through palace._DEFAULT_BACKEND so entry-point backends actually get used. RFC 001 seam (MemPalace#413, MemPalace#995) is merged; registry.py advertises mempalace_postgres as the canonical example. Reframed as parallel track: palace-daemon is deployable today (no migration, wraps current ChromaDB palace); Postgres needs both PRs to land plus a drawer migration (export_palace() + importer — not code we'd write) but eliminates the entire 1.5.x failure class at the storage layer. Starting with palace-daemon since it's deployable now; the daemon is storage-agnostic so Postgres remains available as a later swap. Also fixed palace-daemon lock path: /tmp/palace-daemon-8085.lock (per-port), not /tmp/palace-daemon.lock.
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.
Summary
Advances the RFC 001 §10 cleanup prerequisite so the six in-flight backend PRs (#574 LanceDB, #665 Postgres, #700 Qdrant, #697 hosted Chroma, #643 PalaceStore, #381 Qdrant) have a stable target to align against. Resolves @skuznetsov's open coordination question on #743 — the spec surface this PR lands is what #665 can rebase onto.
Sibling spec RFC 002 #990 will get its parallel
mempalace/sources/cleanup in a follow-up PR once this lands.What's in this PR
BaseCollectionboundary (§1.3).QueryResultandGetResultare frozen dataclasses with the outer/inner list semantics the spec prescribes.QueryResult.empty(num_queries=N)constructs the spec-correct empty shape._DictCompatMixinon both result types soresult["ids"]/result.get("documents")keep working at roughly 60 existing call sites while the attribute-access migration proceeds file-by-file in follow-ups. Scheduled for removal once all callers migrate.BaseCollectionacrossadd/upsert/query/get/delete/update/count(§1.1). ABC defaults forestimated_count,close,health, plus a non-atomic defaultupdate()that does get+merge+upsert (§1.2). Backends advertisingsupports_updateoverride.PalaceRefreplaces raw path strings at the backend boundary (§2.2).idis always present (cache key),local_pathfor filesystem-rooted palaces,namespacefor server-mode tenant routing.BaseBackendABC withget_collection(palace=PalaceRef, …),close_palace,close,health, and an optionaldetect(path) -> boolclassmethod for the migration-path auto-detection rule (§3.3 (4)).[project.entry-points."mempalace.backends"]wired inpyproject.toml, discovery viaimportlib.metadata.entry_points(group="mempalace.backends"), explicitregister()for tests wins on conflict (§3.1–3.2).resolve_backend_for_palace()implements the §3.3 priority order.ChromaCollectionnormalizes chroma's dict returns into typed results with spec-correct empty-outer-shape preservation (fixes issue IndexError when ChromaDB query returns empty results #195 at the type layer). Unknown where-clause operators raiseUnsupportedFilterError— silent dropping is forbidden (§1.4).ChromaBackendabsorbs the inode/mtime client-cache freshness check previously duplicated in mcp_server._get_client() (§10 + PR fix: detect mtime changes in _get_client to prevent stale HNSW index #757).close_palace(ref)now flushes a single palace's handles.searcher.pymigrated to attribute access as the reference call site;_first_or_emptyis polymorphic overQueryResultand the legacy dict shape so test mocks and mid-migration callers keep passing.Explicitly out of scope (follow-up PRs)
These are called out in the spec and deferred so this PR stays reviewable:
palace.py,mcp_server.py,miner.py,convo_miner.py,dedup.py,repair.py,exporter.py,palace_graph.py,cli.py,closet_llm.py.EmbedderIdentityMismatchErrorcheck (§1.5).maintenance_state()/run_maintenance()benchmark hooks (§7.3).AbstractBackendContractSuiteconformance coverage (§7.1–7.2).mempalace migrate/mempalace verifyrewrites throughBaseCollection(§8).mcp_server._get_client()removal (its_metadata_cacheinvalidation is tangled with the client cache in ways worth disentangling separately).Test plan
uv run python -m pytest tests/ --ignore=tests/benchmarks— 970 passed (up from 967 on develop).uv run ruff check .— clean.uv run ruff format --check .— clean.tests/test_backends.py:QueryResult/GetResultreturn fromChromaCollectionQueryResult.empty(num_queries=2))result["ids"],result.get(...),inoperator)UnsupportedFilterErroron unknown where operator ($regex)KeyErrorresolve_backend_for_palacepriority orderChromaBackend.detect()PalaceRefkwarg path onChromaBackend.get_collectionCoordination
Refs #743 (RFC 001), #990 (RFC 002), #989 (RFC 002 tracking issue).
cc @skuznetsov @dekoza @cschnatz @jphein — this is the §10 spec surface I mentioned. #665 / #574 / #697 can now rebase against these interfaces. Happy to help review the alignment deltas.