Skip to content

feat(retrieval): scope summary search to node sets - #4828

Open
chinmayv095 wants to merge 1 commit into
topoteretes:devfrom
chinmayv095:feat/summaries-nodeset-scope
Open

feat(retrieval): scope summary search to node sets#4828
chinmayv095 wants to merge 1 commit into
topoteretes:devfrom
chinmayv095:feat/summaries-nodeset-scope

Conversation

@chinmayv095

Copy link
Copy Markdown
Contributor

Self-found, no issue.

SearchType.CHUNKS, RAG_COMPLETION, TRIPLET_COMPLETION, HYBRID_COMPLETION and every graph-completion variant take node_name and node_name_filter_operator. SearchType.SUMMARIES did not: the factory built SummariesRetriever with {"top_k", "session_id"} alone, and the retriever called vector_engine.search("TextSummary_text", query, limit, include_payload) with no filter.

So passing node_name to a summary search was accepted and ignored. A caller who scopes a chunk search to a node set and then switches to SUMMARIES for a shorter answer silently searches the whole corpus, with nothing in the result to say the scope was dropped. That is the same gap #4675 fixes for CHUNKS_LEXICAL, and after both, SUMMARIES is the last vector-backed retriever that was ignoring node sets.

Why this stays a two-argument change

The interesting part was checking the data was already there rather than assuming it, because a filter on a field that summaries do not carry would return nothing rather than everything, which is a worse failure than the one being fixed. The chain holds end to end:

  • summarize_text.py builds each TextSummary with belongs_to_set=chunk.belongs_to_set, and the chunk got it from its document in extract_chunks_from_documents.py. Summaries are node-set tagged already.
  • get_graph_from_model treats belongs_to_set as the one DataPoint-valued field it keeps as a node property, reducing it to names, and says why in _node_set_names: "Nodeset names as a scalar property, so the vector database can filter on them."
  • add_data_points hands those same nodes to index_data_points, so the vector payload carries the names, not the NodeSet objects. test_pgvector.py asserts exactly that against a real pipeline (nodeset in node_set for nodeset in result[0].payload["belongs_to_set"]).
  • Both adapters filter that key: PGVector with payload->'belongs_to_set' ?| ARRAY[...], LanceDB with array_has_any(payload.belongs_to_set, [...]).

Nothing new is needed at the adapter or graph layer, and the filter works wherever summary search already works.

Two things checked that could have made this wrong

An empty scoped result must not raise NoDataError. That error says the system holds no data and the user should add some, which would be false for a node set that simply has no summaries. Here the existing code was already right: NoDataError is raised only from CollectionNotFoundError, and an empty result list falls through to "" and []. Worth pinning rather than assuming, since a new filter can turn an existing error message into a lie, so there is a test for it.

Nothing else is derived from the filtered set. SummariesRetriever joins the payload texts and returns the payloads, with no corpus-level statistics that would end up describing the whole graph while the results describe a subset.

Verification

SummariesRetriever and the factory reverted to dev with the tests kept: 7 of 40 fail. Restored, all 40 pass.

FAILED test_get_context_success
FAILED test_get_objects_top_k_limit
FAILED test_get_objects_forwards_nodeset_filter_to_vector_search
FAILED test_scoped_search_that_matches_nothing_returns_no_summaries
FAILED test_init_defaults
FAILED test_init_custom_nodeset_filter
FAILED test_summaries_retriever_receives_nodeset_filter_arguments

The first two are existing tests whose assert_awaited_once_with now pins the two new arguments on the unfiltered path, so the default stays node_name=None rather than drifting. ruff check and ruff format --check are clean repo-wide at the pinned 0.15.11. ty does not cover cognee/modules/retrieval or cognee/modules/search, neither directory being in [tool.ty.src] include.

Tests mirror the ChunksRetriever ones so the two retrievers stay comparable, and they live in the existing summaries_retriever_test.py and test_get_search_type_retriever_instance.py rather than new files.

This touches get_search_type_retriever_instance.py, which my open #4675 also edits, but at a different entry in the registry (SUMMARIES rather than CHUNKS_LEXICAL) and in a different test region, so the two do not conflict.

SearchType.SUMMARIES was the last vector-backed retriever that ignored
node_name. Passing it to a summary search had no effect, so switching to
SUMMARIES from CHUNKS or RAG_COMPLETION silently widened the search back
to the whole corpus.

The data was already in place. summarize_text copies belongs_to_set from
the chunk onto the TextSummary, and get_graph_from_model reduces that
field to node set names on the node it indexes, which is the same payload
key both vector adapters filter on. So the retriever only had to forward
the two arguments ChunksRetriever already forwards.

Signed-off-by: Chinmay V <chinmayv095@gmail.com>

@linhongyu510 linhongyu510 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I independently verified head f562f88c. The summary datapoints already inherit belongs_to_set from their source chunks, and the unified vector interface plus LanceDB, PGVector, and Turso adapters all accept the forwarded node_name / node_name_filter_operator contract. The unfiltered path remains explicit (node_name=None, operator OR), while a scoped empty match returns [] / empty context instead of misreporting NoDataError.

Focused verification from the PR snapshot: the two retriever/factory modules are 40 passed; Ruff check passes and all four changed Python files are already formatted. I found no blocking behavior issue.

The overall GitHub rollup is red, but the distinction matters: Code Quality, mocked Unit Tests, CLI Unit Tests, Telemetry, DCO, and Community Tests are green. The broad test_suites.yml OS/E2E jobs I inspected completed as failures with no recorded steps (for example, the Python 3.11 unit job started and ended within four seconds), so they do not provide a code-level failure trace for this change. They still need an upstream rerun/repair before merge, but should not be described as failing summary-retrieval assertions.

@chinmayv095

Copy link
Copy Markdown
Contributor Author

Thanks for going through it independently, and for checking Turso as well. I verified the payload contract against PGVector (payload->'belongs_to_set' ?| ARRAY[...]) and LanceDB (array_has_any) and left Turso implicit, so that is a real addition to the coverage.

Agreed on the rollup. The two content gates, Code Quality and check-dco, are green, and the broad test_suites.yml jobs fail the same way on other fork PRs opened this week because they cannot reach the secrets and services they need. Nothing in them touches summary retrieval.

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