DEV-1514: extract Retriever ABC + SearchService orchestration#160
Merged
Conversation
Refactors slayer/search/ so the three-channel BM25 + Tantivy + embeddings pipeline becomes a default arrangement of one Retriever ABC. Read-side and write-side public signatures preserved; internal channel methods replaced by a parallel gather across a configurable retriever list. Retriever ABC (slayer/search/retriever.py): - One async retrieve() returning a combined RetrievalResult (memory + entity rankings + text_by_id + warnings) so retrievers that share expensive setup (litellm embed, tantivy index handle) do it once per search call. - Symmetric write hooks: upsert_memory, refresh_model_subtree, refresh_datasource (defaults no-op) plus delete_* hooks reserved for future persistent retrievers. Three shipping retrievers in slayer/search/retrievers/: - BM25Retriever: ports _run_channel_1 (entity-overlap BM25, valid_canonicals filter). - TantivyRetriever: ports _run_channel_2 (two kind-filtered queries run sequentially in one call against the same in-memory tantivy.Index). - EmbeddingRetriever: absorbs the former EmbeddingService; one fetch_corpus + one embed_question + one dim-check per retrieve call; owns the litellm refresh pipeline on the write side. delete_* hooks no-op (StorageBackend owns the embedding cascade transactionally with the row delete). SearchService refactor: - Default retriever factory: [BM25, Tantivy, Embedding]. - Custom retrievers injectable via retrievers= kwarg. - valid_canonicals + corpus built once per search; gather across retrievers in parallel; warnings aggregated in retriever declaration order (not gather completion order); text_by_id precedence first-non-empty wins. - New public upsert_memory / refresh_model_subtree / refresh_datasource methods fan out to every retriever and isolate per-retriever exceptions as prefixed warnings. Call-site migrations: slayer/memories/service.py, slayer/engine/profiling.py, slayer/engine/ingestion.py now go through SearchService. Out of scope this PR: persistent tantivy (in-memory rebuild stays); pluggable RRF; embedding cascade ownership change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
More reviews will be available in 2 minutes and 40 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR refactors SLayer's search stack from a fixed "three-channel" architecture into a pluggable retriever-based design. ChangesRetriever Plugin Architecture and SearchService Orchestration Refactor
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
slayer/search/service.py (1)
594-597: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse keyword arguments per coding guidelines.
Same as the earlier occurrence:
_filter_memories_by_datasourcehas two parameters, so keyword arguments should be used.As per coding guidelines: "Use keyword arguments for functions with more than 1 parameter"
♻️ Proposed fix
- recency_memories = _filter_memories_by_datasource( - await self._storage.list_memories(entities=None), - datasource, - ) + recency_memories = _filter_memories_by_datasource( + memories=await self._storage.list_memories(entities=None), + datasource=datasource, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@slayer/search/service.py` around lines 594 - 597, The call to _filter_memories_by_datasource should use keyword arguments instead of positional ones: replace the positional invocation that passes await self._storage.list_memories(entities=None) and datasource with named parameters (e.g., memories=... and datasource=...) so the call becomes explicit; update the invocation where recency_memories is assigned to call _filter_memories_by_datasource with memories=<result of self._storage.list_memories(entities=None)> and datasource=datasource.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@slayer/search/retrievers/bm25.py`:
- Around line 18-20: The function _filter_memories_entities currently accepts
multiple positional parameters (memories, valid_canonicals) and should enforce
keyword-only arguments per guidelines; update the signature of
_filter_memories_entities to make valid_canonicals a keyword-only parameter
(e.g., insert a '*' after the first parameter) so callers must pass
valid_canonicals by name, and adjust any call sites to use the keyword if
necessary.
In `@slayer/search/retrievers/embeddings.py`:
- Around line 56-67: The three helper functions _column_canonical_id,
_measure_canonical_id, and _aggregation_canonical_id accept multiple positional
parameters; update their signatures to require keyword-only arguments by adding
a lone * before the second parameter (e.g., def _column_canonical_id(model:
SlayerModel, *, column_name: str) -> str) so callers must pass
column_name/measure_name/aggregation_name as keywords while keeping return
behavior unchanged.
In `@slayer/search/service.py`:
- Around line 389-392: Call _filter_memories_by_datasource using keyword
arguments to follow the guideline: replace the positional call with one that
passes memories=await self._storage.list_memories(entities=None) and
datasource=datasource (i.e., _filter_memories_by_datasource(memories=...,
datasource=...)) so the function name _filter_memories_by_datasource and the
await self._storage.list_memories(entities=None) expression are left intact but
passed as named parameters.
---
Outside diff comments:
In `@slayer/search/service.py`:
- Around line 594-597: The call to _filter_memories_by_datasource should use
keyword arguments instead of positional ones: replace the positional invocation
that passes await self._storage.list_memories(entities=None) and datasource with
named parameters (e.g., memories=... and datasource=...) so the call becomes
explicit; update the invocation where recency_memories is assigned to call
_filter_memories_by_datasource with memories=<result of
self._storage.list_memories(entities=None)> and datasource=datasource.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e530faba-05ff-453a-a7ee-f439711ca1a7
📒 Files selected for processing (25)
slayer/embeddings/__init__.pyslayer/embeddings/service.pyslayer/engine/ingestion.pyslayer/engine/profiling.pyslayer/memories/service.pyslayer/search/retriever.pyslayer/search/retrievers/__init__.pyslayer/search/retrievers/bm25.pyslayer/search/retrievers/embeddings.pyslayer/search/retrievers/tantivy.pyslayer/search/service.pyslayer/storage/sqlite_storage.pytests/conftest.pytests/test_bm25_retriever.pytests/test_cascade_strip_on_delete.pytests/test_embedding_retriever.pytests/test_embeddings_service.pytests/test_idempotent_ingestion.pytests/test_retriever_abc.pytests/test_search_invariance.pytests/test_search_service_dispatch.pytests/test_search_service_orchestration.pytests/test_search_three_channel.pytests/test_startup_ingest.pytests/test_tantivy_retriever.py
💤 Files with no reviewable changes (2)
- tests/test_embeddings_service.py
- slayer/embeddings/service.py
- CodeRabbit threads + outside-diff: switch the 4 helper functions with >1 parameter (`_filter_memories_entities`, `_column_canonical_id`, `_measure_canonical_id`, `_aggregation_canonical_id`, `_filter_memories_by_datasource`) to keyword-only signatures, and flip the call sites in `bm25._filter`, `embeddings.refresh_model_subtree`, and the two `service._filter_memories_by_datasource` invocations to `kwarg=value` form. Matches the project's "kwargs for >1 param" rule. - Sonar S6466 (CRITICAL, 2x): add explicit non-empty guards in `test_search_service_orchestration.py:138` and `:328` before indexing `captured_*[0]`. Turns a possible IndexError into a clear "retriever was not invoked" failure and clears the `new_reliability_rating` gate condition. - Sonar S7503 (MINOR, 13x — all invalid): the async-no-await signatures on the `Retriever` ABC default no-op hooks and on the test stubs are required by design (subclasses override with truly-async hooks; monkeypatched stubs must match the real async signature). Suppressed with `# NOSONAR(S7503) — ...` on each flagged line. Local: 3489 unit tests pass, ruff clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Integrates DEV-1513 (named-entity surfacing) into the retriever-based architecture: * BM25Retriever now augments memory entity lists with implicit ``memory:<self_id>`` before BM25 ranking — a user-supplied ``memory:<id>`` ref surfaces the named memory at the top of the memory ranking. * SearchService.search() runs a new ``_build_channel_1_entity_ranking`` pre-pass that contributes user-named canonical refs to the entity ranking (subject to datasource / hidden / missing filters). New ``LookupFound`` / ``LookupHidden`` / ``LookupMissing`` Pydantic types carry the lookup outcome. The entity-side fuse_entity_hits now takes a ``named_kind_text`` fallback for pure-named calls (no question). * New ``_memory_id_off_datasource_warnings`` + ``_stale_query_warnings_for_named_memory_refs`` emit warnings for explicitly-named memories filtered by datasource or carrying stale attached queries. * EmbeddingRetriever.refresh_model_subtree and refresh_datasource now route through the unified ``collect_model_entity_pairs`` / ``render_datasource_pair`` helpers in slayer.search.render (the single-source-of-truth dispatch DEV-1513 added). Drops the per-kind canonical-id helpers in favor of those shared with the corpus builder and the named-entity surfacing path. * DEV-1513 test files (test_search_named_entity_surfacing.py, test_search_render_unified.py) flipped to import EmbeddingRetriever from slayer.search.retrievers.embeddings and monkeypatch the new module path for ``collect_model_entity_pairs`` / ``embed_batch``. * slayer/embeddings/service.py stays deleted; DEV-1513 work is absorbed into EmbeddingRetriever instead. 3530 unit tests pass; ruff clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per Codex review on the merge commit: the truthy `if valid_canonicals` check bypasses filtering on an empty set. The ABC declares ``valid_canonicals: set`` (non-Optional), so an empty set is a legitimate value meaning "no entities are live — drop every stale tag". Direct BM25Retriever callers (third-party orchestrators) would otherwise rank against stale tags whenever they passed `set()`. The fix: always call ``_filter_memories_entities``; an empty set strips all tags, BM25 finds zero overlap, the ranking comes back empty — correct behavior for "everything is stale". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
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
1 participant
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
slayer/search/with a clean facade: every channel is now aRetrieverABC subclass, andSearchServiceorchestrates a configurable retriever list viaasyncio.gather.RetrievalResult(memory + entity rankings) from oneretrieve()call so retrievers sharing expensive setup (litellm embed, dim-check) do it once per search; orchestration warning order is deterministic in retriever-declaration order, not gather-completion order.EmbeddingServiceis absorbed intoEmbeddingRetriever;SearchServicegains publicupsert_memory/refresh_model_subtree/refresh_datasourcefan-out methods that isolate per-retriever exceptions. Persistent tantivy is explicitly out of scope (in-memory rebuild stays); future PR overridesTantivyRetriever's no-op write hooks without an ABC change.Test plan
poetry run pytest -m "not integration"— 3489 unit tests passpoetry run ruff check slayer/ tests/— cleanRetrieverABC compliance, BM25 / Tantivy / Embedding read-side, single fetch_corpus + single embed_question + single dim-check perEmbeddingRetriever.retrieve, two-kind-filtered Tantivy queries running sequentially against the same index, write-side fan-out + exception isolation for all three create/refresh hooks, warning order in retriever declaration order even under artificial retrieve delays,text_by_idfirst-non-empty-wins precedence, per-bucket invariance (DEV-1414) preservedtest_search_invariance,test_search_three_channel,test_cascade_strip_on_delete,test_idempotent_ingestion,test_startup_ingest,conftest) flipped to new symbols;test_embeddings_service.pydeleted (replaced bytest_embedding_retriever.py)slayer searchCLI + RESTPOST /search+ MCPsearchcontinue to return identicalSearchResponseshape🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor