-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Fix: Search methods configuration ignored (#788) #829
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: Search methods configuration ignored (#788) #829
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed everything up to 19833bc in 1 minute and 4 seconds. Click for details.
- Reviewed
144lines of code in2files - Skipped
0files when reviewing. - Skipped posting
6draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. graphiti_core/search/search.py:177
- Draft comment:
Good job conditionally building thesearch_taskslist in edge_search. Note that if config.search_methods is empty, no tasks will run and an empty result will be returned. Ensure this behavior is documented if intentional. - Reason this comment was not posted:
Confidence changes required:33%<= threshold80%None
2. graphiti_core/search/search.py:289
- Draft comment:
Similarly, node_search now builds its search_tasks based on config.search_methods. If no methods are configured, it returns empty results. Consider documenting this behavior or logging a warning for clarity. - Reason this comment was not posted:
Confidence changes required:33%<= threshold80%None
3. graphiti_core/search/search.py:1
- Draft comment:
Ensure that a proper copyright header is present at the top of the file. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. graphiti_core/search/search.py:177
- Draft comment:
There is a repeated pattern between edge_search and node_search for building search_tasks. Consider refactoring the duplicate logic into a shared helper function to adhere to the DRY principle. - Reason this comment was not posted:
Confidence changes required:33%<= threshold80%None
5. uv.lock:2
- Draft comment:
The version bump (revision from 2 to 3 and graphiti-core version from 0.18.4 to 0.18.5) looks appropriate. Ensure downstream dependencies are updated accordingly. - Reason this comment was not posted:
Confidence changes required:0%<= threshold80%None
6. graphiti_core/search/search.py:219
- Draft comment:
Line 219 appears to be incomplete. The call to 'search_results.append(' is missing an argument or closing parenthesis, which might cause a syntax error. Please review and complete the intended functionality. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_6hIP0aOdLIU3m1gb
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
|
Hello @prasmussen15, I’ve run several tests and everything looks good on my side—hope this is helpful. When you have a moment, could you please review this and pr #816? Thank you! |
The search functions were ignoring the configured search_methods and always executing all three methods (BM25, cosine similarity, and BFS). This was causing unnecessary performance overhead - running 3x the required computations when users only wanted specific search methods. Now the functions build search tasks dynamically based on what's actually configured in search_methods, only executing the requested search algorithms. This should significantly improve performance when users need only specific search methods. Would appreciate your review on this approach\! Fixes getzep#788
19833bc to
1860c19
Compare
|
Hi @prasmussen15, I've rebased the branch on the latest main to resolve the merge conflicts. The changes are ready for review. Regarding the CI failures:
The actual changes in this PR (respecting search_methods configuration) are working correctly and all relevant tests pass locally. The PR implements the fix for #788 as intended - now edge_search and node_search only execute the search methods specified in the configuration instead of always running all three methods. |
|
Thanks for the contribution! I will merge this in and it will go out with the next release |
I found that the search configuration issue could be resolved by making the search functions actually respect what users configure.
The problem was that both
edge_searchandnode_searchwere always running all three search methods regardless of what was specified in the config. So even if you only wanted BFS, you'd still get BM25 and cosine similarity running in the background - not ideal for performance.I've updated the code to build the search task list based on what's actually in
config.search_methods. Now it only runs what you ask for. Pretty straightforward fix that should help with the performance concerns mentioned in the issue.Ran all the tests and linting - everything looks good. Would be great if someone could review this approach and see if it makes sense.
Fixes #788
Important
edge_searchandnode_searchnow respect configured search methods, improving performance by executing only specified methods.edge_searchandnode_searchinsearch.pynow respectconfig.search_methods, executing only specified search methods (BM25, cosine similarity, BFS).uv.lockversion from0.18.4to0.18.5.This description was created by
for 19833bc. You can customize this summary. It will automatically update as commits are pushed.