[MEVD] DI registration method changes

[roji]

• Our current DI Add* APIs - e.g. AddSqliteVectorStore - currently accept a string? parameter for the service key; but in the DI APIs (e.g. AddKeyedTransient), it can be anything (object?). I don't think MEVD connectors should arbitrary limit service key types to string when the general DI APIs allows object here.
• The naming here should be serviceKey, not serviceId - this is what it's called in the DI APIs.
• We should align to the standard .NET (and MEAI) pattern of having AddKeyedSqliteVectorStore alongside AddSqliteVectorStore (which doesn't accept a service key).
• Add methods should have an optional parameter to allow the user to determine the service lifetime.
• In some cases, the Add methods resolve a service from DI (SearchIndexClient, AzureAISearchVectorStoreOptions); in some cases it seems that we register the vector store as transient because of this (SearchIndexClient, we don't know what lifetime the dependency has), in others we don't (AzureAISearchVectorStoreOptions).
  • We may want to reexamine the idea of looking AzureAISearchVectorStoreOptions up in DI - I'm not sure why a user would want to do that, rather than simply passing it as a parameter. Possibly same with SearchIndexClient.
  • In general having AddXX methods with the same name that register the same thing (IVectorStore) with different lifetimes seems problematic.
• On that note, we sometimes register as Transient really heavyweight types like CosmosClient, which are very explicitly meant to be singletons. This seems like quite a perf pit of failure.
• IVectorizableTextSearch isn't actually registered, despite documentation.
• All extension methods are currently in the Microsoft.SemanticKernel namespace - this needs to change to Microsoft.Extensions.VectorData.
• Consider also registering the concrete types (e.g. AzureAISearchVectorStore) and not just the interfaces (IVectorStore) - that's important in case users want to access database-specific APIs etc.

[markwallace-microsoft]

@roji if you want someone on the core team to take this please let me know

[roji]

Thanks @markwallace-microsoft, I'll probably handle this.

[westey-m]

in some cases it seems that we register the vector store as transient because of this In general having AddXX methods with the same name that register the same thing (IVectorStore) with different lifetimes seems problematic.

@roji The idea here was that the DB clients should be registered with the lifetime recommended by the DB provider. Of course, if someone wants to register it themselves, they can, with the lifetime they prefer. Also, there is not much value in registering an IVectorStore with a lifetime that is shorter than the client it depends on, and a longer lifetime would cause issues, so they should preferably be registered with similar lifetimes to dependencies or shorter where the dependency lifetime is not known.

[roji]

Thanks @westey-m, I plan to look at this a bit later (a first PR is coming that doesn't deal with this yet). FWIW my inclination (without having gone too deep) is that all connectors should always be registered as singletons by default. Yes, in some cases we look up a dependency in DI, and have no idea what the lifetime for that is, but DI allows resolving a transient/scoped service from a singleton service - the transient/scoped service gets "promoted" to be a singleton. Yes, in some case this isn't the behavior that the user wants, but they should simply be able to tell us which lifetime they want via an optional parameter.

IMHO this singleton-by-default usage should represent the 99% case of users, and not cause any hard-to-detect performance pit-of-failures (where e.g. a CosmosClient gets instantiated again and again because of transient scopes).

But I've purposefully not changed anything around this yet so we can discuss first.

[dmytrostruk]

@roji Since you are changing current DI methods, I'm wondering if in this scope of work we should also include DI methods that instead of IServiceCollection will return {Service}Builder, so it will be possible to apply decorators using chaining such as UseLogging(), UseOpenTelemetry() and so on. Do we need both types of methods that return IServiceCollection and {Service}Builder or just pick one of them? If we will have both of them, the only difference would be a return type, so we will have to come up with some naming strategy in order to be able to support both of them at the same time.

If we decide to support both types of methods, then this work can be done in parallel, and I can handle DI methods that return {Service}Builder as part of my telemetry work. If we decide to support only methods that return {Service}Builder, it probably makes sense to wait with this task and handle it when Builders will become available.

What do you think?

[roji]

Hey @dmytrostruk. Regardless of this discussion, I'm planning at least another batch of work on these DI APIs, so this isn't the one and only DI-related PR - I think it's OK if we merge this and later do another one changing the return types to builder (if that's what we choose to do). Or do you see a specific problem in doing that?

Now on the actual discussion...

I'm wondering if in this scope of work we should also include DI methods that instead of IServiceCollection will return {Service}Builder

Is this pattern - builder-returning extension methods directly over ServiceCollection - used elsewhere, e.g. in MEAI? I'm not sure I've seen that (but I may have missed it) - here's e.g. the OpenAI DI docs).

I'm not a huge fan of duplicating all of our current Add* DI registration methods (one version of each returning IServiceCollection as today, another returning the new builder) - there's already a large amount of methods, plus it feels like the added APIs would be potentially quite confusing... The other option of having only builder-returning methods would mean that users who aren't interested in decoration can't chain further Add* methods over ServiceCollection (maybe not a big deal).

BTW not necessarily pushing back against any of this, but it would be good to follow common .NET/MEAI patterns around this (registration of IChatClient, IEmbeddingGenerator...) unless we have a good reason not to...

[dmytrostruk]

@roji

I think it's OK if we merge this and later do another one changing the return types to builder (if that's what we choose to do). Or do you see a specific problem in doing that?

No problems at all, just wanted to understand how we could optimize the work.

Is this pattern - builder-returning extension methods directly over ServiceCollection - used elsewhere, e.g. in MEAI? I'm not sure I've seen that (but I may have missed it) - here's e.g. the OpenAI DI docs).

Here is an example, although it's not an extension method for specific AI provider, it's an extension method for ChatClient abstraction: https://github.com/dotnet/extensions/blob/0e167021401fc051d056e8161dd8d1bb1ec3c11e/src/Libraries/Microsoft.Extensions.AI/ChatCompletion/ChatClientBuilderServiceCollectionExtensions.cs#L40

I already did the same in my initial telemetry PR where I've added AddVectorStore that returns Builder, so you can start chaining.

The usage will be something like this:

// Option #1
// Add Redis vector store
serviceCollection.AddRedisVectorStore();

// Register RedisVectorStore with enabled logging.
serviceCollection
    .AddVectorStore(s => s.GetRequiredService<RedisVectorStore>())
    .UseLogging(this.LoggerFactory);

// ========

// Option #2
// It's also possible to initialize vector store directly
serviceCollection
    .AddVectorStore(new RedisVectorStore(...))
    .UseLogging(this.LoggerFactory);

Option 1 is not ideal, since you have to "configure" the same vector store in two places. There was also a comment from @westey-m regarding this: https://github.com/microsoft/semantic-kernel/pull/10865#discussion_r1989116023.

But I think this approach is consistent with MEAI, so if it works for MEVD - we should be good for now. Please let me know.

[roji]

although it's not an extension method for specific AI provider

public static ChatClientBuilder AddChatClient(
    this IServiceCollection serviceCollection,
    Func<IServiceProvider, IChatClient> innerClientFactory,
    ServiceLifetime lifetime = ServiceLifetime.Singleton)

Right, this is the important thing IMHO - this isn't a duplication of every Add method of every chat client implementation, to have another variant which return ChatClientBuilder instead of ServiceCollection: it's just one generic method that can be used for all providers (plus another keyed one).

It's true that this has the drawback of having to do two things (AddRedisVectorStore(), AddVectorStore()), but I still think it's the least bad option here (compared to duplicating everything, or making all methods return only the builder). My vote would be to be aligned with MEAI here, so everything works the same way. We can also have this conversation with the MEAI folk, maybe there's some extra context to the design: @stephentoub @SteveSandersonMSS, any input here?

[dmytrostruk]

My vote would be to be aligned with MEAI here, so everything works the same way.

I agree with this approach. Let's proceed with it unless there will be an additional input from MEAI folks.

In terms of scope of work, we can handle these DI methods independently, so all good here.

[roji]

Design decision: do a pass to make sure that registration of the database client/data source should be singleton (for all databases). If so, we're OK with making all our connectors' DI registration methods register as Singleton by default, but expose a lifetime parameter to allow users to override this.

[adamsitnik]

I am moving the discussion from https://github.com/microsoft/semantic-kernel/pull/11594#discussion_r2049040464 to here so it's easier to discover:

Regular vs. Keyed

Microsoft.Extensions.DependencyInjection itself and other projects that went over API review process do provide Add$Name and AddKeyed$Name methods, so I would take it for granted.

IVectorStore vs. IVectorStoreRecordCollection

True, it give us another x2 multiplier.

We may end up needing POCO vs. dynamic mapping

You mean a situation where just specyfing the generic arguments for IVectorStoreRecordCollection won't be enought and we will have to expose some additional options?

String vs. Func. For PG there's also NpgsqlDataSource (and this is also planned for SQL Server, Sqlite...).

There are two connector types:

• A: create the client/connection on it's own based on arguments provided by the user (example: SqlServer, Sqlite). We need those arguments and I would expect most of the real world apps to need to read something from the config. But I do get that for the sake of simplicity not everyone needs to be willing to use IServiceProvider when they have a hardcoded value. But if we are willing to take this complexity, it's another x2 overload multiplier for such connectors.
• B: take an existing client/connection instance and use it. Since most of them should be registered as singleton, we should be by default resolving them from DI. This is something that we already do:

https://github.com/microsoft/semantic-kernel/blob/9c9aec0d76fe2fd916185a5c08af714c5eafa8db/dotnet/src/Connectors/Connectors.Memory.AzureAISearch/AzureAISearchServiceCollectionExtensions.cs#L37

but we should also allow the users to provide their own instance. But I am against having an overload for every set of arguments supported by given client ctor. Example:

https://github.com/microsoft/semantic-kernel/blob/9c9aec0d76fe2fd916185a5c08af714c5eafa8db/dotnet/src/Connectors/Connectors.Memory.AzureAISearch/AzureAISearchServiceCollectionExtensions.cs#L57

https://github.com/microsoft/semantic-kernel/blob/9c9aec0d76fe2fd916185a5c08af714c5eafa8db/dotnet/src/Connectors/Connectors.Memory.AzureAISearch/AzureAISearchServiceCollectionExtensions.cs#L88

We should just accept a factory method and delegate the creation to the users. This is something I have introduced to the Healtchecks project when I was working on Aspire (it has millions of downloads and was rather well received with very small pushback): https://github.com/Xabaril/AspNetCore.Diagnostics.HealthChecks/issues/2040

For the sake of simplicity, I am not including the mandatory first argument this IServiceCollection services and optional last ServiceLifetime lifetime = ServiceLifetime.Singleton in all the examples below:

Sql Server

Sql Server bare minimum

We expose only overloads that use Func<IServiceProvider> as it allows to read from config but also provide hardcoded values.

// IVectorStore

IServiceCollection AddSqlServerVectorStore(
    Func<IServiceProvider, string> connectionStringProvider, Func<IServiceProvider, SqlServerVectorStoreOptions>? optionsProvider = null);
IServiceCollection AddKeyedSqlServerVectorStore(object serviceKey,
    Func<IServiceProvider, string> connectionStringProvider, Func<IServiceProvider, SqlServerVectorStoreOptions>? optionsProvider = null);

// IVectorStoreRecordCollection

IServiceCollection AddSqlServerVectorStoreCollection<TKey, TRecord>(
    string collectionName, Func<IServiceProvider, string> connectionStringProvider, Func<IServiceProvider, SqlServerVectorStoreRecordCollectionOptions<TRecord>>? optionsProvider = null);
IServiceCollection AddKeyedSqlServerVectorStoreCollection<TKey, TRecord>(object serviceKey,
    string collectionName, Func<IServiceProvider, string> connectionStringProvider, Func<IServiceProvider, SqlServerVectorStoreRecordCollectionOptions<TRecord>>? optionsProvider = null);

Sql Server convenience

We expose both overloads that use Func<IServiceProvider> but also raw string connectionString.

// IVectorStore

IServiceCollection AddSqlServerVectorStore(
    string connectionString, SqlServerVectorStoreOptions? options = null);
IServiceCollection AddKeyedSqlServerVectorStore(object serviceKey,
    string connectionString, SqlServerVectorStoreOptions? options = null);

IServiceCollection AddSqlServerVectorStore(
    Func<IServiceProvider, string> connectionStringProvider, Func<IServiceProvider, SqlServerVectorStoreOptions>? optionsProvider = null);
IServiceCollection AddKeyedSqlServerVectorStore(object serviceKey,
    Func<IServiceProvider, string> connectionStringProvider, Func<IServiceProvider, SqlServerVectorStoreOptions>? optionsProvider = null);

// IVectorStoreRecordCollection

IServiceCollection AddSqlServerVectorStoreCollection<TKey, TRecord>(
    string collectionName, string connectionString, SqlServerVectorStoreOptions? options = null);
IServiceCollection AddKeyedSqlServerVectorStoreCollection<TKey, TRecord>(object serviceKey,
    string collectionName, string connectionString, SqlServerVectorStoreOptions? options = null);

IServiceCollection AddSqlServerVectorStoreCollection<TKey, TRecord>(
    string collectionName, Func<IServiceProvider, string> connectionStringProvider, Func<IServiceProvider, SqlServerVectorStoreRecordCollectionOptions<TRecord>>? optionsProvider = null);
IServiceCollection AddKeyedSqlServerVectorStoreCollection<TKey, TRecord>(object serviceKey,
    string collectionName, Func<IServiceProvider, string> connectionStringProvider, Func<IServiceProvider, SqlServerVectorStoreRecordCollectionOptions<TRecord>>? optionsProvider = null);

Azure AI:

IServiceCollection AddAzureAISearchVectorStore(
    Func<IServiceProvider, SearchIndexClient>? clientProvider = null, Func<IServiceProvider, AzureAISearchVectorStoreOptions>? optionsProvider = null);
IServiceCollection AddKeyedAzureAISearchVectorStore(object serviceKey,
    Func<IServiceProvider, SearchIndexClient>? clientProvider = null, Func<IServiceProvider, AzureAISearchVectorStoreOptions>? optionsProvider = null);

IServiceCollection AddAzureAISearchVectorStoreCollection<TKey, TRecord>(
    string collectionName, Func<IServiceProvider, SearchIndexClient>? clientProvider = null, Func<IServiceProvider, AzureAISearchVectorStoreOptions>? optionsProvider = null);
IServiceCollection AddKeyedAzureAISearchVectorStoreCollection<TKey, TRecord>(object serviceKey,
    string collectionName, Func<IServiceProvider, SearchIndexClient>? clientProvider = null, Func<IServiceProvider, AzureAISearchVectorStoreOptions>? optionsProvider = null);

[adamsitnik]

1. Keep Regular and Keyed
2. Focus on IVectorStoreRecordCollection first
3. Register both the implementation and abstraction
4. String vs. Fun: have both
5. Have the overloads for most frequently used ctors

[roji]

@adamsitnik additional note to not forget AddVectorStoreTextSearch, which also registers as transient by default (needs to be singleton but allow overriding), and doesn't have standard Add/AddKeyed overloads.

[adamsitnik]

A thing that I've just realized during API review: should we also register the collections as IVectorSearch<TRecord>? So users can resolve for searching scenarios only?

[roji]

@adamsitnik I think we're actually already doing that when the user registers a collection in DI (as opposed to a store). For example, here's the Azure AI Search code. For providers that support hybrid search, I think we also register IKeywordHybridSearch as well.

Probably a good idea to make sure we're doing that systematically across all providers...

[adamsitnik]

Closed by https://github.com/microsoft/semantic-kernel/pull/12097