Skip to content

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

Merged
merged 12 commits into from
May 20, 2025

Conversation

MackinnonBuck
Copy link
Member

@MackinnonBuck MackinnonBuck commented May 13, 2025

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 built Microsoft.SemanticKernel locally and included the generated .nupkgs directly in the repo. This change (41bd61e) will get reverted after the updated Microsoft.SemanticKernel packages ship.

Microsoft Reviewers: Open in CodeFlow

@MackinnonBuck MackinnonBuck requested review from a team as code owners May 13, 2025 23:19
@github-actions github-actions bot added the area-ai-templates Microsoft.Extensions.AI.Templates label May 13, 2025
@stephentoub
Copy link
Member

stephentoub commented May 13, 2025

I've built Microsoft.SemanticKernel locally and included the generated .nupkgs directly in the repo. This change (41bd61e) will get reverted after the updated Microsoft.SemanticKernel packages ship.

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?

@jeffhandley jeffhandley added the * NO MERGE * Do not merge this PR as long as this label is present. label May 14, 2025
@jeffhandley
Copy link
Member

I applied * NO MERGE * Do not merge this PR as long as this label is present. . We will wait to merge this until the package is published and we can reference it.

@jeffhandley jeffhandley requested a review from roji May 14, 2025 07:07
Copy link
Member

@jeffhandley jeffhandley 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 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.

Copy link
Member

@roji roji left a 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.

@dotnet-policy-service dotnet-policy-service bot added the waiting-author-feedback 📭 The author of this issue needs to respond in order for us to continue investigating this issue. label May 14, 2025
Copy link
Member

@SteveSandersonMS SteveSandersonMS left a 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!

@dotnet-policy-service dotnet-policy-service bot removed the waiting-author-feedback 📭 The author of this issue needs to respond in order for us to continue investigating this issue. label May 14, 2025
@MackinnonBuck MackinnonBuck requested a review from roji May 19, 2025 21:22
Copy link
Member

@jeffhandley jeffhandley left a 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!

Copy link
Member

@roji roji left a 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.

MackinnonBuck and others added 2 commits May 20, 2025 09:07
Co-authored-by: Jeff Handley <jeffhandley@users.noreply.github.com>
@MackinnonBuck MackinnonBuck requested a review from roji May 20, 2025 16:54
Copy link
Member

@roji roji left a 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!

@MackinnonBuck MackinnonBuck removed the * NO MERGE * Do not merge this PR as long as this label is present. label May 20, 2025
@MackinnonBuck MackinnonBuck merged commit 712d9aa into main May 20, 2025
6 checks passed
@MackinnonBuck MackinnonBuck deleted the mbuck/replace-json-vector-store branch May 20, 2025 18:17
joperezr added a commit to joperezr/extensions that referenced this pull request May 21, 2025
Co-authored-by: Jeff Handley <jeffhandley@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-ai-templates Microsoft.Extensions.AI.Templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants