You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
CompletionRetriever never widened its vector fetch when personalization weights were active, so
preference weights could only reorder the chunks top_k already returned. The comment above the
call says the opposite is the point: personalization has to change which chunks make the cut, not
just their order.
The search path reads the value with kwargs.get("wide_search_top_k") and no default, so None
reaches the retriever and the signature default of 100 never applies. GraphCompletionRetriever normalizes that; CompletionRetriever, the one completion retriever that
does not subclass it, did not. max(self.top_k, None or 0) is top_k, so the widening disappeared.
One line, mirroring graph_completion_retriever.py:75 exactly, same default:
Introduced in #4611, which changed the producer and normalized the graph retriever in the same diff
but not this one.
Test
test_unset_wide_search_top_k_still_widens_the_fetch in the existing personalization suite,
constructing the retriever the way the search path does rather than with an explicit value. That is
the gap the existing cases left: all of them pass wide_search_top_k=WIDE_SEARCH_TOP_K, so None
was never exercised.
Fails on main (limit is 2), passes with the fix (limit is 100).
The 10 failures are pre-existing. I ran the same selection on origin/main with the change stashed
and diffed the failure lists: identical, same 10 names, and the only difference is the one test this
PR adds.
I moved this to dev earlier because that is where most merges land. That was wrong for this change: the module it touches is not on dev. cognee/tests/unit/modules/user_preferences/ does not exist there, and completion_retriever.py has no wide_search_top_k at all.
The two branches also disagree about the bug. On dev the call site is kwargs.get("wide_search_top_k", 100), so None never reaches a retriever. On main it is kwargs.get("wide_search_top_k") with no default (get_search_type_retriever_instance.py:85), which is what makes max(self.top_k, self.wide_search_top_k or 0) collapse to top_k.
So this belongs on main, and it conflicts on dev for the same reason it does not apply there. Worth flagging that whenever the two reconcile, main's call site still needs the default or this normalization.
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
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.
Description
CompletionRetrievernever widened its vector fetch when personalization weights were active, sopreference weights could only reorder the chunks
top_kalready returned. The comment above thecall says the opposite is the point: personalization has to change which chunks make the cut, not
just their order.
The search path reads the value with
kwargs.get("wide_search_top_k")and no default, soNonereaches the retriever and the signature default of
100never applies.GraphCompletionRetrievernormalizes that;CompletionRetriever, the one completion retriever thatdoes not subclass it, did not.
max(self.top_k, None or 0)istop_k, so the widening disappeared.One line, mirroring
graph_completion_retriever.py:75exactly, same default:Introduced in #4611, which changed the producer and normalized the graph retriever in the same diff
but not this one.
Test
test_unset_wide_search_top_k_still_widens_the_fetchin the existing personalization suite,constructing the retriever the way the search path does rather than with an explicit value. That is
the gap the existing cases left: all of them pass
wide_search_top_k=WIDE_SEARCH_TOP_K, soNonewas never exercised.
Fails on
main(limitis 2), passes with the fix (limitis 100).Validation
The 10 failures are pre-existing. I ran the same selection on
origin/mainwith the change stashedand diffed the failure lists: identical, same 10 names, and the only difference is the one test this
PR adds.
Closes #4650