.Net MEVD: Dependency Injection for SqlServer connector #11594
Conversation
markwallace-microsoft
added
.NET
adamsitnik
had a problem deploying
to
integration
GitHub Actions
Failure
| /// <param name="lifetime">The service lifetime for the store. Defaults to <see cref="ServiceLifetime.Singleton"/>.</param> | ||
| /// <returns>The service collection.</returns> | ||
| public static IServiceCollection AddSqlServerVectorStore(this IServiceCollection services, | ||
| Func<IServiceProvider, string> connectionStringProvider, |
There was a problem hiding this comment.
Not sure, but... I think the basic expectation any user would have is to be able to pass the connection string directly (without a function). I get the logic of accepting a Func (to construct a connection string based on config that's in DI, e.g. tenant ID as database name), but it's definitely a bit jarring/unexpected for the minimal demo to look like this: AddSqlServerVectorStore(_ => "blablabla").
FWIW IIRC EF only accepts a string directly; AddNpgsqlDataSource is the same (though there's an issue for adding an overload that accepts an IServiceProvider Func. I'm not sure what they did in Aspire - might be worth checking.
The easy answer here is obviously to just have two overloads - one with a string, one with a Func. The only concern here is that we risk going into DI overload explosion: we have:
- String vs. Func. For PG there's also NpgsqlDataSource (and this is also planned for SQL Server, Sqlite...).
- Regular vs. Keyed
- IVectorStore vs. IVectorStoreRecordCollection
- We may end up needing POCO vs. dynamic mapping - the former will be not safe for trimming. After Build we'll also introduce another POCO one which accepts source-generated stuff, for trimming-safe POCO mapping (so 3 overall)
So I'm not sure where this will go - let's maybe discuss this in parking lot soon.
There was a problem hiding this comment.
@roji, is you concern around having many overloads born from complexity of use, or from maintaining so many overloads?
There was a problem hiding this comment.
@roji @westey-m I've moved the conversation to the issue: #10549 (comment) PTAL
| { | ||
| protected abstract void PopulateConfiguration(ConfigurationManager configuration, object? serviceKey = null); | ||
|
|
||
| protected abstract void RegisterVectorStore(IServiceCollection services, ServiceLifetime lifetime, object? serviceKey = null); |
There was a problem hiding this comment.
Since there are several registration methods, maybe better to allow connectors to expose a list of delegates on their fixtures and perform tests over all of them.
| using Microsoft.Extensions.DependencyInjection; | ||
| using Microsoft.Extensions.VectorData; | ||
|
|
||
| namespace Microsoft.SemanticKernel.Connectors.SqlServer; |
There was a problem hiding this comment.
I think it's common practice to put DI registration methods in a very general namespace so that they light up with an extra using, e.g. Microsoft.Extensions.VectorData (though I seem to remember that this was slightly controversial).
There was a problem hiding this comment.
Other connectors went one step further and have defined their extension methods in Microsoft.SemanticKernel:
There was a problem hiding this comment.
I believe another approach is to have them in the namespace of the thing that is being registered on, in this case that of ServiceCollection, which does make sense to me too.
There was a problem hiding this comment.
@westey-m good point. I can see that this is also what Aspire is doing:
I am going to apply this suggestion.
- register concrete, provider-specific types, not just the abstractions - have overloads that accept raw connection string (and just options, not an option provider)
adamsitnik
had a problem deploying
to
integration
GitHub Actions
Failure
@roji @westey-m Once this PR gets approved, I would like to apply similar pattern to other connectors. Namely:
ServiceLifetime.Singletonis the default, but users can provide other values if they want to (idea taken from .Net: First round of MEVD DI changes #10960, this is what I've been doing in projects like Aspire or HealthChecks)AddKeyedprefix for the methods that register a keyed instance (also taken from .Net: First round of MEVD DI changes #10960)Func<IServiceProvider, string>so the users can read the value from the settings.Sample:
fixes #10948