.Net MEVD: DI for relational DBs #12043

adamsitnik · 2025-05-13T18:10:29Z

contributes to #10549

adamsitnik · 2025-05-13T18:11:47Z

dotnet/src/Connectors/Connectors.Memory.PgVector/PostgresCollectionOptions.cs

+    {
+    }
+
+    internal PostgresCollectionOptions(PostgresCollectionOptions? source, IEmbeddingGenerator embeddingGenerator)


we have the copy ctor just to make it more likely to not miss extending the copy logic when a new property gets introduced in the future

nit: it's internal so it doesn't matter much, but I'd just have this a simple copy constructor - accepting only another PostgresCollectionOptions instance - and then set the embedding generator as usual at the call site:

var newOptions = new PostgresCollectionOptions(source) { EmbeddingGenerator = embeddingGenerator; }

This would make this usable for other needs and just be more standard (same with the other option constructors).

adamsitnik · 2025-05-13T18:13:27Z

dotnet/src/Connectors/Connectors.Memory.PgVector/PostgresServiceCollectionExtensions.cs

-        // Since we are not constructing the data source, add the IVectorStore as transient, since we
-        // cannot make assumptions about how data source is being managed.


I've added ServiceLifetime lifetime argument to let the users decide (we should not be using transient by default, as it's bad for perf).

adamsitnik · 2025-05-13T18:14:21Z

dotnet/src/Connectors/Connectors.Memory.SqliteVec/SqliteServiceCollectionExtensions.cs

This is literally a copy of the SqlServer file with a SqlServer -> Sqlite replace

adamsitnik · 2025-05-13T18:16:09Z

dotnet/src/Connectors/Connectors.PgVector.UnitTests/PostgresServiceCollectionExtensionsTests.cs

The DependencyInjectionTests provide more detailed test coverage, I decided to remove this test file to avoid a duplication of efforts.

adamsitnik · 2025-05-13T18:17:23Z

...t/src/IntegrationTests/Connectors/Memory/SqliteVec/SqliteServiceCollectionExtensionsTests.cs

-/// </summary>
-public sealed class SqliteServiceCollectionExtensionsTests
-{
-    private const string? SkipReason = "SQLite vector search extension is required";


Sqlite was testing the same thing in two different places, but at the same time not testing at all because it was always skipped. So I decided to remove it, as the new tests provide really deep test coverage.

adamsitnik · 2025-05-13T18:18:33Z

.../src/VectorDataIntegrationTests/PgVectorIntegrationTests/PostgresEmbeddingGenerationTests.cs

+                    builder.UseVector();
+                    return builder.Build();
+                })
+                .AddPostgresVectorStore(lifetime: ServiceLifetime.Transient),


this is an example of how our users can use ServiceLifetime.Transient with the existing overload

adamsitnik · 2025-05-13T18:19:40Z

dotnet/src/VectorDataIntegrationTests/PgVectorIntegrationTests/Support/PostgresTestStore.cs


    public NpgsqlDataSource DataSource => this._dataSource ?? throw new InvalidOperationException("Not initialized");

+    public string ConnectionString => this._connectionString ?? throw new InvalidOperationException("Not initialized");


Some of the tests Dispose the container and I did not want them to dispose the shared NpgsqlDataSource, so I had to expose the connection string (with password, as DataSource.ConnectionString hides the password) to be able to create an independent data source.

roji

Looks good @adamsitnik! See comments below.

Also... I think I mentioned this at some point, but we never fully discussed: when a user does AddPostgresVectorStore and then also AddPostgresCollection, don't they reasonably expect us to create the collection from the store rather than instantiate it directly? This could be important if AddPostgresVectorStore is passed some options, which the user wants to have applied to the collection (should be trivial to implement).

roji · 2025-05-14T07:03:02Z

dotnet/src/Connectors/Connectors.Memory.PgVector/PostgresServiceCollectionExtensions.cs

    /// <returns>The service collection.</returns>
-    public static IServiceCollection AddPostgresVectorStore(this IServiceCollection services, PostgresVectorStoreOptions? options = default, string? serviceId = default)
+    public static IServiceCollection AddPostgresVectorStore(


No Keyed variant of this (which doesn't accept a connection string)? I don't necessarily mind, just try to make sure we have a clear logic is all.

That is correct, the methods that do resolve NpgsqlDataSource from DI don't have keyed overloads.

I just don't want to add too many overloads without having a clear need from the customers.

Any reason to have them for SQLite but not here thouggh?

dotnet/src/Connectors/Connectors.Memory.PgVector/PostgresServiceCollectionExtensions.cs

roji · 2025-05-14T07:12:14Z

dotnet/src/Connectors/Connectors.Memory.PgVector/PostgresCollectionOptions.cs

+    {
+    }
+
+    internal PostgresCollectionOptions(PostgresCollectionOptions? source, IEmbeddingGenerator embeddingGenerator)


nit: it's internal so it doesn't matter much, but I'd just have this a simple copy constructor - accepting only another PostgresCollectionOptions instance - and then set the embedding generator as usual at the call site:

var newOptions = new PostgresCollectionOptions(source) { EmbeddingGenerator = embeddingGenerator; }

This would make this usable for other needs and just be more standard (same with the other option constructors).

dotnet/src/Connectors/Connectors.Memory.SqliteVec/SqliteServiceCollectionExtensions.cs

roji · 2025-05-14T07:30:55Z

dotnet/src/VectorDataIntegrationTests/VectorDataIntegrationTests/DependencyInjectionTests.cs

-        // And the IVectorSearchable abstraction.
-        Verify<IVectorSearchable<TRecord>>(serviceProvider, lifetime, serviceKey);
+            using ServiceProvider serviceProvider = services.BuildServiceProvider();
+            // Let's ensure that concrete types are registered.


FWIW the code seems pretty self-explanatory to me, not sure the comments are needed.

roji · 2025-05-14T07:31:57Z

dotnet/src/VectorDataIntegrationTests/VectorDataIntegrationTests/DependencyInjectionTests.cs

-        using ServiceProvider serviceProvider = services.BuildServiceProvider();
-        // let's ensure that concrete types are registered
-        Verify<TCollection>(serviceProvider, lifetime, serviceKey);
+            Assert.False(wasResolved); // it's lazy


This seems to be testing general DI flow that isn't related to MEVD

My purpose was to show that we don't create the object when the extension method gets called, but rather register a factory method that is lazy executed

dotnet/src/VectorDataIntegrationTests/VectorDataIntegrationTests/DependencyInjectionTests.cs

adamsitnik · 2025-05-14T08:27:22Z

when a user does AddPostgresVectorStore and then also AddPostgresCollection, don't they reasonably expect us to create the collection from the store rather than instantiate it directly

What if they provided a different lifetime for each of them? Or simply different connection strings? I am not sure if we have the ability to verify that.

And TBH I don't see the point of registering both. If you want AddPostgresCollection, just go and register it without AddPostgresVectorStore

…o diUnification # Conflicts: # dotnet/src/Connectors/Connectors.Memory.PgVector/PostgresServiceCollectionExtensions.cs # dotnet/src/Connectors/Connectors.Memory.SqliteVec/SqliteServiceCollectionExtensions.cs

- make the copy ctors more general purpose - add more overloads

- make serviceKey nullable (and reduce the need of heplers) - rename collectionName to name

roji

Thanks @adamsitnik, looks good - see remaining last nits.

roji · 2025-05-14T15:28:33Z

dotnet/src/Connectors/Connectors.Memory.PgVector/PostgresServiceCollectionExtensions.cs

    /// <returns>The service collection.</returns>
-    public static IServiceCollection AddPostgresVectorStore(this IServiceCollection services, PostgresVectorStoreOptions? options = default, string? serviceId = default)
+    public static IServiceCollection AddPostgresVectorStore(


Any reason to have them for SQLite but not here thouggh?

dotnet/src/Connectors/Connectors.Memory.PgVector/PostgresServiceCollectionExtensions.cs

roji · 2025-05-14T15:36:21Z

dotnet/src/Connectors/Connectors.Memory.SqliteVec/SqliteServiceCollectionExtensions.cs

+
+        services.Add(new ServiceDescriptor(typeof(SqliteVectorStore), serviceKey, (sp, _) =>
+        {
+            var connectionString = connectionStringProvider(sp);


nit: I still slightly prefer just inlining these variables and making everything a single lambda expression without a block - but not important.

Co-authored-by: Shay Rojansky <roji@roji.org>

adamsitnik added 2 commits May 13, 2025 17:24

refactor existing tests

a0e3563

Postgres

bbb2842

adamsitnik requested a review from a team as a code owner May 13, 2025 18:10

markwallace-microsoft added .NET kernel memory labels May 13, 2025

adamsitnik had a problem deploying to integration May 13, 2025 18:10 — with GitHub Actions Failure

adamsitnik commented May 13, 2025

View reviewed changes

Sqlite

492fbd7

adamsitnik force-pushed the diUnification branch from 4e0c866 to 492fbd7 Compare May 13, 2025 18:30

adamsitnik temporarily deployed to integration May 13, 2025 18:30 — with GitHub Actions Inactive

adamsitnik requested review from roji and westey-m May 13, 2025 18:36

fix the build

652e62a

adamsitnik temporarily deployed to integration May 13, 2025 18:45 — with GitHub Actions Inactive

roji reviewed May 14, 2025

View reviewed changes

roji mentioned this pull request May 14, 2025

Replace JSON vector store with SQLite dotnet/extensions#6438

Merged

adamsitnik added 2 commits May 14, 2025 11:07

Merge remote-tracking branch 'upstream/feature-vector-data-preb3' int…

f4f0a36

…o diUnification # Conflicts: # dotnet/src/Connectors/Connectors.Memory.PgVector/PostgresServiceCollectionExtensions.cs # dotnet/src/Connectors/Connectors.Memory.SqliteVec/SqliteServiceCollectionExtensions.cs

address code review feedback:

55e5bf6

- make the copy ctors more general purpose - add more overloads

adamsitnik temporarily deployed to integration May 14, 2025 09:46 — with GitHub Actions Inactive

design changes:

88bc437

- make serviceKey nullable (and reduce the need of heplers) - rename collectionName to name

adamsitnik temporarily deployed to integration May 14, 2025 14:59 — with GitHub Actions Inactive

roji reviewed May 14, 2025

View reviewed changes

roji approved these changes May 14, 2025

View reviewed changes

Apply suggestions from code review

523b319

Co-authored-by: Shay Rojansky <roji@roji.org>

adamsitnik temporarily deployed to integration May 14, 2025 15:38 — with GitHub Actions Inactive

adamsitnik merged commit f3d5b50 into microsoft:feature-vector-data-preb3 May 14, 2025
11 checks passed

		// Since we are not constructing the data source, add the IVectorStore as transient, since we
		// cannot make assumptions about how data source is being managed.


		public NpgsqlDataSource DataSource => this._dataSource ?? throw new InvalidOperationException("Not initialized");

		public string ConnectionString => this._connectionString ?? throw new InvalidOperationException("Not initialized");

.Net MEVD: DI for relational DBs #12043

.Net MEVD: DI for relational DBs #12043

Uh oh!

Conversation

adamsitnik commented May 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsitnik May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roji left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adamsitnik commented May 14, 2025

Uh oh!

roji left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

adamsitnik May 13, 2025 •

edited

Loading