Skip to content

fix(searcher): use filter-fallback in CLI search path for HNSW drift (#1665) - #1750

Closed
rodboev wants to merge 1 commit into
MemPalace:developfrom
rodboev:fix/1665-wing-search-orphan-ids
Closed

fix(searcher): use filter-fallback in CLI search path for HNSW drift (#1665)#1750
rodboev wants to merge 1 commit into
MemPalace:developfrom
rodboev:fix/1665-wing-search-orphan-ids

Conversation

@rodboev

@rodboev rodboev commented Jun 10, 2026

Copy link
Copy Markdown

Summary

search() (CLI path) and search_memories() (MCP/API path) diverged in their handling of filtered HNSW failures. search_memories already routes through _query_drawers_with_filter_fallback (added for #1245/#1035), which retries unfiltered and post-filters in Python when ChromaDB raises Error finding id on a where=-scoped query. search() did not — its except Exception block at searcher.py:381 caught the error and re-raised as SearchError, so wing-filtered CLI searches failed with a cryptic message while the equivalent MCP call recovered cleanly.

The fix routes search() through the same helper, making CLI and MCP behavior consistent. No new infrastructure: the helper already handles the unfiltered-retry, Python post-filter, and warning log.

Fix

Replace the inline col.query(**kwargs) call in search() (searcher.py:379) with _query_drawers_with_filter_fallback(col, kwargs, query, n_results, wing, room). The helper signature accepts the same kwargs dict and the existing wing/room locals, so no refactoring is needed around the call site.

Add a regression in tests/test_searcher.py: mock a collection whose query raises only when where= is present, call search() with a wing filter, assert no SearchError is raised and results are returned.

Related work

Open PRs #951, #1005, #1463, and #1598 address overlapping search-fallback surfaces. This PR is the narrowest fix: it routes the CLI search() path through the existing _query_drawers_with_filter_fallback helper that search_memories() already uses. #1005 and #1598 both subsume this change as part of broader scope; if either lands first, this PR is redundant.

Test plan

  • python -m pytest tests/test_searcher.py -v — 31 passed
  • ruff check . — clean
  • ruff format --check . — clean

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the search mechanism in mempalace/searcher.py to use a fallback query function (_query_drawers_with_filter_fallback) when a filtered query fails, allowing it to fall back to an unfiltered query and post-filter the results in Python. A corresponding unit test has been added in tests/test_searcher.py to verify this behavior. I have no feedback to provide as there are no review comments.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@mvalentsev

Copy link
Copy Markdown
Contributor

The fix works, but check the #1665 thread before review: four open PRs already cover this same failure (#1005 delegates CLI search to search_memories and removes the CLI/API split entirely, #1598 hardens CLI and API together, #951 carries a get()-based variant, #1463 a BM25 fallback). This makes a seventh remedy on the cluster, and the description mentions none of them. Choosing between copying the fallback into the second path and collapsing the two paths is a maintainer call.

@rodboev

rodboev commented Jun 10, 2026

Copy link
Copy Markdown
Author

Fair point. I should have acknowledged those PRs in the description. Updated it to reference #951, #1005, #1463, and #1598 and note the overlap. This one is the narrowest surgical option for the CLI-only gap; if any of the broader PRs land first it's redundant.

@mvalentsev

Copy link
Copy Markdown
Contributor

@rodboev Just one request, please verify problem/fix by actually running MemPalace, rather than reviewing the code theoretically.

@rodboev

rodboev commented Jun 10, 2026

Copy link
Copy Markdown
Author

Ran it locally against a small test palace. CLI search works fine with and without wing filters on a healthy index. The tricky part is reproducing the actual drift scenario from #1665, since it requires orphaned HNSW IDs left behind after a bulk delete. The fix itself is straightforward routing: search() now goes through the same _query_drawers_with_filter_fallback helper that search_memories() already uses, so the fallback logic is shared instead of duplicated. I can set up a targeted repro if you'd like, but it would need a palace large enough for the bulk-delete path to leave stale IDs in the index.

@rodboev

rodboev commented Jun 11, 2026

Copy link
Copy Markdown
Author

Withdrawing this PR. The batch had serious quality problems, as you rightly pointed out. I genuinely did not see the issues and apologize for that. Closing everything and going back to the drawing board. Sorry for the noise.

@rodboev rodboev closed this Jun 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants