Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Added VectorRetriever class and remove GenAIClient #6

Merged
merged 3 commits into from
Apr 9, 2024
Merged

Added VectorRetriever class and remove GenAIClient #6

merged 3 commits into from
Apr 9, 2024

Conversation

willtai
Copy link
Contributor

@willtai willtai commented Apr 4, 2024 •
edited
Loading

Description

Added VectorRetriever class

Implementation

  • Added VectorRetriever in retrievers.py
  • Moved tests to test_retriever.py
  • Changed examples

@willtai willtai force-pushed the vector-retriever-class branch 2 times, most recently from 3bab0dd to 32b967d Compare April 4, 2024 13:01
@willtai willtai force-pushed the index-handling-methods branch from 20fb738 to 77817b1 Compare April 4, 2024 15:01
@willtai willtai force-pushed the vector-retriever-class branch 2 times, most recently from 37e5800 to f42e040 Compare April 4, 2024 15:05
@willtai willtai marked this pull request as ready for review April 4, 2024 15:10
@willtai willtai marked this pull request as draft April 4, 2024 15:11
Base automatically changed from index-handling-methods to main April 5, 2024 09:24
oskarhane
oskarhane reviewed Apr 5, 2024
View reviewed changes
examples/openai_search.py Outdated Show resolved Hide resolved
@willtai willtai force-pushed the vector-retriever-class branch from f42e040 to 6e4b3fe Compare April 5, 2024 10:14
@willtai
Copy link
Contributor Author

willtai commented Apr 5, 2024

Another design consideration

In the design we had

context = vector_search.search(text=question)

where context is text. Do we want this or do we want .search() to return vectors instead? Or do we want different methods for this?

@oskarhane
Copy link
Member

oskarhane commented Apr 5, 2024 •
edited
Loading

Another design consideration

In the design we had

context = vector_search.search(text=question)

where context is text. Do we want this or do we want .search() to return vectors instead? Or do we want different methods for this?

I would expect the nodes (or relationships) that has the matched embeddings on them to be returned.
Right?

Or maybe I'm misunderstanding you here.
What's the use case for returning the vectors?

@willtai
Copy link
Contributor Author

willtai commented Apr 5, 2024

Sorry I think didn't phrase carefully.

Yes we now .search() returns nodes and scores. However, in the design we want to return a "context", which I assume uses the nodes and score to build some kind of text/image etc. It's not clear to me whether VectorRetriever.search() here should return the "context".

Perhaps I misunderstand what is meant by the context

@willtai willtai marked this pull request as ready for review April 5, 2024 14:16
@oskarhane
Copy link
Member

Returning the matches and their scores would be expected. So the word "context" in that sense is confusing, I agree.

@willtai willtai requested review from a team and removed request for a team April 5, 2024 15:14
oskarhane
oskarhane reviewed Apr 8, 2024
View reviewed changes
examples/openai_search.py Outdated Show resolved Hide resolved
@willtai willtai force-pushed the vector-retriever-class branch from 8e1e641 to 6d7d6ec Compare April 8, 2024 09:43
willtai
willtai commented Apr 8, 2024
View reviewed changes
src/neo4j_genai/indexes.py
@@ -46,7 +46,7 @@ def create_vector_index(
raise ValueError(f"Error for inputs to create_vector_index {str(e)}")

query = (
f"CREATE VECTOR INDEX $name IF NOT EXISTS FOR (n:{label}) ON n.{property} OPTIONS "
f"CREATE VECTOR INDEX $name FOR (n:{label}) ON n.{property} OPTIONS "
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this should fail explicitly if an index on n.property already exists

Copy link
Member

Choose a reason for hiding this comment

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

On one side I agree because creating a new index also sets the dimension, and if this doesn't fail you might end up in a strange place where you're not really sure why things doesn't work.

On another side I disagree, because it's pretty annoying to have to try/catch for this in the code.

But overall, I agree, I think this should fail.

@willtai willtai force-pushed the vector-retriever-class branch from 6d7d6ec to 6c36546 Compare April 8, 2024 09:47
@willtai willtai force-pushed the vector-retriever-class branch from 6c36546 to 2670242 Compare April 8, 2024 09:49
@willtai willtai requested a review from a team April 8, 2024 10:11
oskarhane
oskarhane approved these changes Apr 9, 2024
View reviewed changes
Copy link
Member

@oskarhane oskarhane left a comment

Choose a reason for hiding this comment

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

This looks good now ⛵

@willtai willtai merged commit 6bd4f0c into main Apr 9, 2024
3 checks passed
@willtai willtai deleted the vector-retriever-class branch April 9, 2024 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stellasia stellasia stellasia left review comments

@oskarhane oskarhane oskarhane approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@willtai @oskarhane @stellasia