feat: Support for vector search with Qdrant#381
feat: Support for vector search with Qdrant#381Anush008 wants to merge 11 commits intoMemPalace:developfrom
Conversation
Comprehensive Code Review AvailableA detailed review of this PR has been conducted and is available at: 📊 Review SummaryArchitecture: ⭐⭐⭐⭐½ (4.5/5) 🎯 Key FindingsStrengths:
Critical Issues (Must Fix Before Merge):
📋 Recommended ActionsP0 - Must Fix (2-3 hours):
P1 - Should Fix (4-6 hours):
🔗 Full AnalysisThe complete 508-line review includes:
Location: Estimated Effort: 7-11 hours to merge-ready state Review conducted using comprehensive PR analysis methodology. All findings documented with actionable recommendations. |
|
With regards to the AI recommended actions above,
|
web3guru888
left a comment
There was a problem hiding this comment.
Nice abstraction layer. The VectorCollection protocol in backends/__init__.py is the right approach — it lets integrators swap backends without touching application code.
One thing I noticed: the Qdrant path returns a proper QdrantCollection class that explicitly implements VectorCollection, but the ChromaDB path returns native ChromaDB collection objects that satisfy the protocol through duck typing. This works today, but it's fragile — if a future ChromaDB release renames or reorders parameters on .query() or .get(), the protocol match breaks silently (no isinstance check, no type error at construction time).
Might be worth wrapping Chroma in a thin adapter class too (like ChromaCollection(VectorCollection)), even if it's just delegation. That way both backends fail the same way if the contract drifts.
Also: The _scroll_cursor state on QdrantCollection makes .get() with pagination stateful at the instance level. If two callers paginate the same collection concurrently (e.g., MCP handler + CLI status), they'll clobber each other's cursor. The ChromaDB path doesn't have this issue because offset/limit are stateless. Consider making scroll state caller-managed or using a separate iterator pattern.
It's highly unlikely that Chroma would do this without a major release. If they did, even wrapping it in |
Formalizes the BaseCollection/BaseBackend contract introduced as a seam in #413 into an interchangeability spec that third-party backends can build to. Driven by six in-flight backend PRs (#574, #643, #665, #697, #700, #381) each implementing the interface differently. Key decisions captured: entry-point distribution, typed QueryResult/ GetResult replacing Chroma dict shape, daemon-first multi-palace model via PalaceRef, required where-clause subset (incl. $contains), mandatory embedder injection with model-identity validation, capability tokens, shared pytest conformance suite, and a backend-neutral migrate/verify CLI.
…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).
Scanned all 233 open upstream PRs today against our open PRs and fork-ahead / planned-work items. Findings merged into README: - P2 (decay) and P3 Tier-0 (LLM rerank): both covered by MemPalace#1032 (@zackchiutw, MERGEABLE, 2026-04-19 — Weibull decay + 4-stage rerank pipeline). Older simpler version at MemPalace#337. Dropped as fork work; watching MemPalace#1032. - P7 (alternative storage): formally out of scope. RFC 001 MemPalace#743 (@igorls) defines the plugin contract; four backend PRs already in flight (MemPalace#700, MemPalace#381 Qdrant; MemPalace#574, MemPalace#575 LanceDB). Fork consumes, does not rebuild. - P0 (multi-label tags): still fork/upstream candidate. MemPalace#1033 (@zackchiutw) ships adjacent privacy-tag + progressive disclosure but not the full multi-label scheme. - Merged MemPalace#1023 section acknowledges complementary MemPalace#976 (felipetruman) which adds broader mine_global_lock() + HNSW num_threads pin. Gives future-us a map so we don't re-file MemPalace#1036-style duplicates.
|
This PR is superseded by
It implements the new vector search backend specification using Qdrant. |
NOTE
This PR is superseded by
It implements the new vector search backend specification using Qdrant.
Description
This PR adds support for using Qdrant as a vector search provider in MemPalace.
Qdrant is an open-source vector search engine for high performance and massive scale.
Changes
Testing
I've unit tested this integration against a local Qdrant instance.
Setup
You can run Qdrant with
Set
The dashboard is accessible at http://localhost:6333/dashboard
Checklist
python -m pytest tests/ -v)ruff check .)