fix(storage): enable FTS fallback for NoopEmbedding across core and adapter layers - #1091
fix(storage): enable FTS fallback for NoopEmbedding across core and adapter layers#1091lightzt99 wants to merge 3 commits into
Conversation
479bb19 to
1882db3
Compare
|
Thanks for the PR. I cannot mark this ready yet because both CI test jobs are currently failing:
Both fail in |
wayyoungboy
left a comment
There was a problem hiding this comment.
Requesting changes because the current CI failures line up with the behavior change in this PR.
Both test (3.11) and test (3.12) fail in tests/integration/test_noop_embedding_mode.py::test_noop_embedding_search_returns_empty: the existing integration contract expects default search to return no results when embeddings are explicitly disabled, but this PR now returns an FTS hit.
Please settle the intended contract before merge: either document/update tests so automatic FTS fallback is the new default for EMBEDDING_PROVIDER=none, or keep default no-embedding search empty and require an explicit FTS retrieval mode. This should also be aligned with #1052 before either path lands.
The integration test asserted search() returns [] when EMBEDDING_PROVIDER=none, but PR oceanbase#1091 changes that contract to fall back to FTS5 keyword search when query_embedding is empty and query text is present. Update the test to expect the FTS hit and reflect the new behavior in the module docstring.
wayyoungboy
left a comment
There was a problem hiding this comment.
Rechecked the current head. The Python checks are green now, but the original contract issue is still unresolved: this PR changes default search with EMBEDDING_PROVIDER=none to automatic FTS fallback and updates the integration test to expect a hit.
That still conflicts with the direction in #1052, where explicit no-embedding mode stays empty by default and full-text lookup is exposed through an explicit retrieval mode such as retrieval_mode="fts". Please align these two PRs before merge. A clean path would be to drop the automatic default fallback here, wait for #1052 to land, and add the complementary explicit retrieval-mode exposure where this PR needs it.
Keeping changes requested until there is one consistent public search contract for no-embedding mode.
Cover the search_memories() paths when NoopEmbedding returns an empty vector: FTS results returned when query text is present, empty result when both embedding and query are missing, unrelated-query filtering, and user_id scoping.
The integration test asserted search() returns [] when EMBEDDING_PROVIDER=none, but PR oceanbase#1091 changes that contract to fall back to FTS5 keyword search when query_embedding is empty and query text is present. Update the test to expect the FTS hit and reflect the new behavior in the module docstring.
When EMBEDDING_PROVIDER=none (NoopEmbedding), the core layer short-circuited search() to return [] before the adapter's FTS fallback could run, so queries on stored memories silently returned no hits. Remove the early-return in Memory.search() and AsyncMemory.search() so query_embedding stays None and the request flows through to the storage adapter. The adapter already falls back to text-only search when the target store supports it. On the OceanBase side, _supports_text_search_without_vector() no longer gates on the hybrid_search flag, and OceanBaseVectorStore.search() now routes to _fulltext_search() when vectors is None and a query is present, so deployments with hybrid_search=False also get FTS fallback. The integration test that固化ed the old "returns empty" contract now asserts the FTS hit for both sync and async paths.
01c9ec8 to
d7b51de
Compare
|
Thanks for continuing on the NoopEmbedding fallback work. I do not think this is ready to merge yet because the public search contract is still unresolved. This PR changes Please align this PR with that contract before merge: either drop the automatic default fallback here and wait for the explicit retrieval-mode path, or get maintainer agreement to change and document the default behavior. |
|
Thank you for your contribution! PowerMem has evolved into PowerContext as part of a major project upgrade, so we’re closing this PR for now. We truly appreciate your work and would be happy to revisit it in PowerContext if it is still relevant. |
Summary
When
EMBEDDING_PROVIDER=none(NoopEmbedding, added in #1058), memory search returned[]unconditionally. Two layers blocked the FTS fallback:Memory.search()andAsyncMemory.search()short-circuited to{"results": [], "relations": []}as soon as_is_embedding_disabled()was true, before the adapter ever saw the request.StorageAdapter.search_memories()returned[]whenquery_embeddingwas falsy, and_supports_text_search_without_vector()gated OceanBase FTS fallback behind thehybrid_searchflag, so deployments withhybrid_search=Falsehad no text-only path even whenquerywas present.This PR removes both blockers so the request flows through to the storage layer's FTS path when embedding is disabled, giving users keyword-based recall instead of silent empty results.
Changes
Core layer
src/powermem/core/memory.py— drop the_is_embedding_disabled()early-return inMemory.search(); keepquery_embedding = Noneand let the adapter handle the fallback. LogsEmbedding disabled; falling back to FTS for queryfor visibility.src/powermem/core/async_memory.py— same change inAsyncMemory.search().Adapter layer
src/powermem/storage/adapter.py—_supports_text_search_without_vector()now returnsTruefor OceanBase unconditionally (no longer gated byhybrid_search). The existing FTS fallback path insearch_memories()(added in Fix memory search fallback when embeddings are unavailable #1052) already forwardsquery_vector=Nonewhenquery_embeddingis empty andqueryis present, so SQLite and OceanBase both route to full-text search.src/powermem/storage/oceanbase/oceanbase.py—search()now branches to_fulltext_search()whenvectors is Noneandqueryis present, so OceanBase deployments withhybrid_search=Falsealso get FTS fallback (previously thenot self.hybrid_searchbranch would call_vector_searchwith no vectors and return[]).Tests
tests/integration/test_noop_embedding_mode.py— renamedtest_noop_embedding_search_returns_empty→test_noop_embedding_search_returns_fts_hits; asserts the FTS hit for both syncMemoryand asyncAsyncMemory.tests/unit/test_adapter_fts_fallback.py— 5 unit tests covering:Noneembedding + query text → FTS hits[][]user_idscopingValidation
pytest tests/integration/test_noop_embedding_mode.py tests/unit/test_noop_embedding.py tests/unit/test_adapter_fts_fallback.py tests/unit/test_oceanbase_fts_quality_score.py tests/unit/test_sqlite_fts.py tests/unit/test_memory.py -q— 130 passedtest_list_memory_filters.py,test_search_score_threshold.py, etc. (138 related unit tests pass)Behavior matrix
EMBEDDING_PROVIDER=none+ query text[](core short-circuit)hybrid_search=True+none+ query text[](core short-circuit)hybrid_search=False+none+ query text[](core + adapter + store all block)none+ no query text[][](degenerate, unchanged)Notes
hybrid_search=Falsedeployments now also get FTS fallback. Previously thenot self.hybrid_searchbranch insearch()would call_vector_search(query, vectors=None, ...)and return[]; the newvectors is None and querybranch routes to_fulltext_searchfirst.add_skill/search_skills) was already correct and is not touched here —Memory._embed()returnsNone(not[]) when_is_embedding_disabled()is true, soOceanBaseSkillStore.search(query_embedding=None, query_text=...)already skipped vector search.upstream/main(71003f5) to pick up feat(cli): add profile, batch, import/export, quality, optimize commands #1092 and fix(config): honor disabled graph store flag #1130.