Pull request overview

This PR migrates MemPalace’s storage stack to a LanceDB-first architecture, introduces a pluggable embedding system (ONNX default + optional GPU/Ollama), and moves the knowledge graph into LanceDB tables while preserving a legacy Chroma/SQLite migration path.

Changes:

• Added a LanceDB/ChromaDB collection abstraction (db.py) and routed consumers through palace.get_collection(), plus CLI migration and reindex tooling.
• Implemented pluggable embedders (embeddings.py) with model aliases, caching, and CLI discovery.
• Migrated knowledge graph storage to LanceDB tables by default, updated MCP/server and benchmarks, and refreshed tests/docs accordingly.

Reviewed changes

Copilot reviewed 31 out of 34 changed files in this pull request and generated 9 comments.

    except Exception:
        print(f"\n  No palace found at {palace_path}")
        print("  Run: mempalace init <dir> then mempalace mine <dir>")
        raise SearchError(f"No palace found at {palace_path}")

        col = get_collection(palace_path)
        if col.count() == 0:
            raise Exception("Empty palace")
    except Exception as e:
        logger.error("No palace found at %s: %s", palace_path, e)
        return {
            "error": "No palace found",
            "hint": "Run: mempalace init <dir> && mempalace mine <dir>",
        }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

This is the PR I've been waiting for. We've been pinning chromadb>=0.6.0,<0.7.0 in our integration for months specifically because of ChromaDB API surface volatility — having a backend abstraction that isolates the rest of the codebase from that is a significant quality-of-life improvement.

The LanceCollection / ChromaCollection + unified get_collection() factory is clean. A few things I noticed in db.py:

Where-clause translation (_chroma_where_to_sql): The current implementation handles $and, $or, and the common comparison operators. One gap: $in / $nin (used in some of MemPalace's own code via {"wing": {"$in": ["astrophysics", "economics"]}}). If any callers depend on those, they'll silently fall through without adding a condition. Worth either handling or adding an explicit NotImplementedError for unsupported operators.

get() with large offset and no limit: The fallback of limit(100_000) when only an offset is provided could return 100k rows into memory. The pattern appears intentional (used in get_changes_since in PR #575), but on large palaces this materializes everything. A generator/cursor approach would be better long-term, but I understand that's a bigger change.

upsert() fallback path: The merge_insert failure → delete+add fallback is pragmatic, but it means on a partial failure (merge fails midway, then delete+add also fails on some records) you get a partially-committed batch with no error surfaced to the caller. The outer try/except Exception on _to_records calls would hide this. Consider at minimum logging logger.warning("upsert partial failure...") in the fallback.

_check_dimension(): The RuntimeError message is excellent — actionable and includes the exact model names. Good pattern.

Embeddings: ONNX default (all-MiniLM-L6-v2, ~60MB vs 3GB torch) is the right default for most users. The reindex command is essential — you can't switch models without it, and it's not obvious why search quality drops when someone changes their config. The error message from _check_dimension should cite mempalace reindex as the fix, which it does. Nice.

Benchmark results committed: The results_v4_comparison.json in the diff is a nice touch for reproducibility, though committed benchmark artifacts can get stale. Consider a note in BENCHMARKS.md about when they were generated.

222 passing with 2 intentional skips is solid coverage for a change this large. I'd want to see the ChromaDB → LanceDB migration path tested with a real palace that has KG triples before merging, but that may be in the existing test suite.

Overall this is solid architectural work. The abstraction boundary is clean and the migration story (mempalace migrate) is well thought out.

Pull request overview

Copilot reviewed 31 out of 34 changed files in this pull request and generated 9 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@dekoza — the four key fixes in commit 7a15a461 address the main issues I raised:

$in/$nin in _chroma_where_to_sql — the silent data loss path is now closed. Mapping $in/$nin to IN (...)/NOT IN (...) with proper quoting is correct, and the ValueError on unknown operators is the right defensive response.

KG count_rows(filter=) vs len(get(limit=100_000)) — this is the right fix. count_rows is O(1) at the Lance layer vs the full materialization that get(limit=100k) was doing. This also removes the silent truncation bug at exactly 100k triples.

ImportError catch in _open_chroma with actionable message — the "install mempalace[chromadb]" message makes the missing-dependency case debuggable without reading source.

merge_insert fallback log upgraded to warning — correct, it signals a performance regression, not a debug detail.

The searcher empty-palace / missing-palace / missing-chromadb distinction is a nice UX improvement.

One remaining observation (minor, non-blocking): the upsert fallback in the case where merge_insert isn't available still swallows partial failures by iterating per-record. Worth a follow-up to surface the partial failure count in the warning log. Not a blocker for merge.

Approving — the backend abstraction is solid and this is a significant capability addition.

[MemPalace-AGI integration — 215 tests, 710 KG entities]

This is a significant architectural change and the scope is substantial (21 files, full backend abstraction). The LanceCollection/ChromaCollection unified API approach is the right design — a storage seam that lets the two backends be swapped without changing call sites.

A few notes from the design review:

LanceDB V4 Windows blocker: This is the same issue flagged in #529 — lancedb still imports fcntl on Windows in some builds. If this PR is meant to work cross-platform, that needs to be resolved before merge. The [extras] approach (LanceDB as optional) helps, but the import at the top of db.py may still fail on Windows even without the extra.

Migration path completeness: mempalace migrate for ChromaDB → LanceDB is the right surface. The key question is whether the migration handles embedding re-indexing — LanceDB uses its own vector format and ChromaDB's serialized embeddings are not directly transferable. If the migration re-embeds from stored text (slower but correct), that should be documented explicitly.

ChromaDB as [chroma] optional extra: Making chromadb optional is the right long-term direction, but it means every existing installation breaks unless they either install the extra or get a clean upgrade path. The pyproject.toml changeset should include ChromaDB in a compatibility extra (e.g. pip install mempalace[chroma]) and the migration docs should be prominent.

Breaking change scope: This touches palace.py, searcher.py, miner.py, mcp_server.py — every major path. Given the number of community PRs currently open against these files (#542, #544, #562, etc.), the merge conflict surface is enormous. Worth coordinating with the maintainer before the review cycle closes on those PRs.

Benchmark data: benchmarks/BENCHMARKS_V4.md + results_v4_comparison.json is good evidence. Would be useful to see the search latency comparison at 10K+ drawers where LanceDB's columnar scan should outperform ChromaDB's HNSW.

This is the right architectural direction — LanceDB's embedded model without a server process is a better fit for local-first tooling. The coordination and migration story are the main things to nail before this is merge-ready.

[MemPalace-AGI integration — 215 tests, 710 KG entities]