Neo4j vector context retriever bug fix #14461
Merged
Conversation
This file contains 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
dosubot
bot
added
the
size:M
| if limit <= 0: | ||
| break | ||
| # Needs some optimization | ||
| response = self.structured_query( |
There was a problem hiding this comment.
Now we are making a db query per id -- this is extremely inefficient
Can't we use the "power of cypher" to force some ordering of the nodes to match the ordering of the IDs?
There was a problem hiding this comment.
Note: I am not a cypher expert, but it seems like a single query should be possible
There was a problem hiding this comment.
I've modified the code to only make one call to the DB and force ordering of the IDs.
There was a problem hiding this comment.
Please review the new query
dosubot
bot
added
size:S
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Previously, when using the
VectorContextRetrieverwith theNeo4jPropertyGraphStore, theVectorContextRetrieverwould callavector_queryto get thesimilarity_top_knodes and return the list of nodes with the first node being ranked with the highest similarity score, and the last being least similar. However, when we pass the nodes found fromavector_querytoaget_rel_map, the nodes returned could be in any order of similarity. So we need to fix this issue. For example, if I have a query:nodes = retriever.retrieve("Andres Mendez"), then then resulting node ids fromavector_querycould look like the following: ['Andres Mendez', 'Andres', 'Miguel Mendez', 'Joe Mamma']. But then the results fromaget_rel_mapcould provide only the top 30 results from the relationships of Joe Mamma. This is not expected behaviour, as we are querying for Andres Mendez, not Joe Mamma. The issue here is that we don't take similarity score into account when when calling aget_rel_map, but we need to.Fixes # (issue)
I modified the cypher query in
aget_rel_mapto return results based on the order of the node ids passed into this function. This will preserve the scores of the input nodes in the response of this function.Version Bump?
Did I bump the version in the
pyproject.tomlfile of the package I am updating? (Except for thellama-index-corepackage)Type of Change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
I also tested this by running different cases to make sure this works as expected.
Suggested Checklist: