-
Notifications
You must be signed in to change notification settings - Fork 571
feat: LSH for in memory vector store #922
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
base: main
Are you sure you want to change the base?
Conversation
|
Hi @Dev79844, great work on this! I'll be reviewing sometime during the week. |
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.
Some general comments:
- Overall, pretty good work.
- I think we need a builder for
InMemoryVectorStoreand provideIndexStrategy::BruteForceas the default (which will prevent breaking changes from needing to adjust the currentfrommethods). - I am not hugely sold on the enum because it causes strange implementation behaviour (like having to fall back to brute forcing in
vector_search_lshwhen there's no LSH index), but as it's mostly internal implementation we can figure this out later on.InMemoryVectorStoreis not really supposed to be used for huge production stuff anyway.
|
Hey @joshua-mo-143! |
Yeah index strategy should be removed. The idea is that we provide good defaults for users to fall back to, and then when they want to use the higher-power options they can switch to it (so in this case we'd default to |
joshua-mo-143
left a comment
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.
Some minor stuff to be fixed, mostly just comment reworking/removal and some clarification for users who find the LSH stuff and have no idea how to use it
Fixes: #760