-
Notifications
You must be signed in to change notification settings - Fork 56
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
feat: Add contraints and embedding directives #3405
feat: Add contraints and embedding directives #3405
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.
I think the calls out to 3rd party APIs needs expanded documentation and discussion with the team. I have reviewed very little so far but will continue whilst that issue is being resolved.
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've given the production code a first-pass review, and will shortly look through the tests. The code looks good, most of the comments are about moving stuff around, or questions/thoughts RE the feature design.
thought: It looks very tedious and error prone to introduce new types atm, although numerics are probably most painful. Perhaps we should review certain aspects in the future to improve this.
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've reviewed the tests, thanks for adding them, I think there are a couple of important test gaps though, please add tests for:
- Patching a
size
constraint, including removing it - Patching an
embedding
property, and it's children:
2.1 Adding an embedding description to a field that didnt have one before
2.2 Mutating each of the embedding properties on a field that declared one on create
2.3 Removing an embedding property from a field that had one on create - Including a
size
constraint in a View declaration, if you don't/haven't blocked it off, please include a test that shows what happens if that constraint is violated - Including an
embedding
property in a View declaration (I assume we want this to error for now).
tests/integration/mutation/create/constraints/size_constraint_test.go
Outdated
Show resolved
Hide resolved
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.
Marking this as "request for changes" (you can dismiss at any time), even tho there aren't any explcit todos
.
I have mainly focused on the definition/description code along with the collection/doc updating. Speed ran through the encoding section and the gql parsing.
Haven't reviewed the tests.
Main question/discussion points are "when" you are generating the embedding in the call path, and the location of the EmbeddingDescription
.
ae1d158
to
7f1cb9b
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3405 +/- ##
===========================================
+ Coverage 78.34% 78.60% +0.26%
===========================================
Files 395 396 +1
Lines 36286 37333 +1047
===========================================
+ Hits 28426 29342 +916
- Misses 6194 6315 +121
- Partials 1666 1676 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 16 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
117e1fd
to
e54b83e
Compare
9a2a8d4
to
58ca89e
Compare
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.
Thanks for doing the requested suggestions :) great work overall!
LGTM
Bug bash result: |
Relevant issue(s)
Resolves #3350
Resolves #3351
Description
Sorry for having 3 technically separate features as part of 1 PR. The reason for this is that I started working on embeddings and the size contraint was initially part of the embedding. Since we discussed that it could be applied to all array fields (and later even to String and Blob), I extracted it into a contraints directive that has a size parameter (more contraints can be added in the future). Furthermore, embeddings returned from ML models are arrays of float32. This caused some precision issues because we only supported float64. When saving the float32 array, querying it would return an float64 array with slight precision issues. I decided to add the float32 type.
You can review the first commit for contraint and embedding related code and the second commit for the float related changes. Some float stuff might have leaked in the first commit. Sorry for this. I tried hard to separate the float32 related changes.
Note that the
gql.Float
type is nowFloat64
internally.is the same as
The embedding generation relies on a 3rd party package called
chromem-go
to call the model provider API. As long as one of the supported provider API is configured and accessible, the embeddings will be generated when adding new documents. I've added a step in the test workflow that will run the embedding specific tests on linux only (this is because installation on windows and mac is less straight forward) using Ollama (because it runs locally).The call to the API has to be done synchronously otherwise the docID/CID won't be representative of the contents. The only alternative would be for the system to automatically update the document when returning from the API call but I see that as a inferior option as it hides the update step from the user. It could also make doc anchoring more complicated as the user would have to remember to wait on the doc update before anchoring the doc at the right CID.
We could avoid having embedding generation support and let the users do that call themselves and store the embedding vectors directly. However, having it as a feature allows us to support RAG vector search which would let users get started with AI with very little work. This seems to be something our partners are looking forward to.
I don't see the 3rd party API call inline with a mutation as a problem since this is something that has to be configured by users and users will expect the mutation calls to take more time as a result.
If you're interested in running it locally, install Ollama and define a schema like so
Next steps:
_similarity
operation to calculate the cosine similarity between two arrays.Tasks
How has this been tested?
make test and manual testing
Specify the platform(s) on which this was tested: