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.
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
Add python binding for vectorstore #573
base: master
Are you sure you want to change the base?
Add python binding for vectorstore #573
Changes from 13 commits
36ff226
752c8dd
b436853
44bbe0e
c63f290
7e66438
6a3899f
3745f03
c0ae05f
32acf84
45df85e
8b8b946
b2797bd
216c328
bd71c33
a860c2a
05c939c
a563e12
0d2b050
344817e
18c33a3
22965f4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Overall, I don't like this query filter builder API - it's neither easier nor clearer than writing raw EdgeQL segment in a string. And this deviates from the JS-style query builder, at the same time, it's not Pythonic. Why did we want to add a filter API like this in the first place?
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.
I agree, it looks a bit complex, this is how LamaIndex did it, I think. @deepbuzin
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.
@elprans can you pls take a look at this too
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.
Same global reason as for building the entire vectorstore - to enable users to use Gel in LLM-related applications and not have to learn EdgeQL (or anything really). It's meant to be dumb and familiar enough to justify using it in place of other vectorstores.
We spent some time discussing this way of implementing filters on a call about a month ago (FWIW I'm not thrilled with it either), and couldn't think of a better way. That's not to say there isn't one, but we had to pick something and move on.
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.
Although now that I think about it, maybe raw EdgeQL in our own binding isn’t the worst idea… If someone wants a LlamaIndex-style filter constructor they might as well use the LlamaIndex binding.
But downsides of the raw string are 1. no help from LSP and 2. EdgeQL for filters is not very pretty or concise with all the json fiddling.
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.
Yeah I see. I've also left some comments in my proposal, but feel free to continue with what the team had agreed on.