-
Notifications
You must be signed in to change notification settings - Fork 813
Replace JSON vector store with SQLite #6438
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
Conversation
....AI.Templates/src/ChatWithCustomData/ChatWithCustomData-CSharp.Web/Services/IngestedChunk.cs
Show resolved
Hide resolved
This will add over a mb of binaries to the git history. Are we ok with that? Or did you mean you'll update the pr before it merges? |
I applied
* NO MERGE *
|
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.
This looks great to me, @MackinnonBuck. Nice work! Let's get a review from at least one of @roji or @SteveSandersonMS though, and wait for the NuGet packages to be available before merging.
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 pinging me, see below for a few comments.
Note that there will still be a few API changes for GA; some of my comments can already be addressed now, but others can't. So we can do another sync/iteration on this PR (hopefully by Friday everything will be in), and in general only merge after the GA nugets are fully available on nuget.org.
...tensions.AI.Templates/src/ChatWithCustomData/ChatWithCustomData-CSharp.Web/Program.Aspire.cs
Outdated
Show resolved
Hide resolved
...soft.Extensions.AI.Templates/src/ChatWithCustomData/ChatWithCustomData-CSharp.Web/Program.cs
Outdated
Show resolved
Hide resolved
.../Snapshots/aichatweb.BasicAspire.verified/aichatweb/aichatweb.Web/Services/SemanticSearch.cs
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.
Looks superb - great job!
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.
Just the patch version bum in my comments, and then once @roji approves we're good to go. Nice work, @MackinnonBuck!
...src/ChatWithCustomData/ChatWithCustomData-CSharp.Web/ChatWithCustomData-CSharp.Web.csproj.in
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.
Looks great @MackinnonBuck! See the one long comment below about doing automatic embedding generation for upsert as well, not just for search. Other than that I think it's ready for merging.
...tweb.AzureOpenAI_Qdrant_Aspire.verified/aichatweb/aichatweb.Web/Services/IngestedDocument.cs
Outdated
Show resolved
Hide resolved
...s.IntegrationTests/Snapshots/aichatweb.Basic.verified/aichatweb/Services/IngestedDocument.cs
Outdated
Show resolved
Hide resolved
...chatweb.AzureOpenAI_Qdrant_Aspire.verified/aichatweb/aichatweb.Web/Services/IngestedChunk.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Jeff Handley <jeffhandley@users.noreply.github.com>
....Templates/src/ChatWithCustomData/ChatWithCustomData-CSharp.Web/Services/IngestedDocument.cs
Show resolved
Hide resolved
...zureOpenAI_Qdrant_Aspire.verified/aichatweb/aichatweb.Web/Services/Ingestion/DataIngestor.cs
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.
Looks great @MackinnonBuck, well done!
Co-authored-by: Jeff Handley <jeffhandley@users.noreply.github.com>
This PR removes the JSON vector store and replaces it with a SQLite implementation using
Microsoft.SemanticKernel.Connectors.SqliteVec
.Since
Microsoft.SemanticKernel.Connectors.SqliteVec
isn't shipping publicly yet, I've builtMicrosoft.SemanticKernel
locally and included the generated.nupkg
s directly in the repo. This change (41bd61e) will get reverted after the updatedMicrosoft.SemanticKernel
packages ship.Microsoft Reviewers: Open in CodeFlow