-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
.Net: [MEVD] Added GetService and Metadata for vector store abstractions #11055
base: feature-vector-data-preb2
Are you sure you want to change the base?
.Net: [MEVD] Added GetService and Metadata for vector store abstractions #11055
Conversation
dotnet/src/Connectors/VectorData.Abstractions/VectorSearch/VectorizedSearchMetadata.cs
Outdated
Show resolved
Hide resolved
dotnet/src/Connectors/VectorData.Abstractions/VectorData.Abstractions.csproj
Show resolved
Hide resolved
dotnet/src/Connectors/VectorData.Abstractions/VectorSearch/VectorizedSearchMetadata.cs
Outdated
Show resolved
Hide resolved
...t/src/Connectors/Connectors.Memory.AzureAISearch/AzureAISearchVectorStoreRecordCollection.cs
Show resolved
Hide resolved
dotnet/src/Connectors/VectorData.Abstractions/VectorStorage/IVectorStore.cs
Show resolved
Hide resolved
dotnet/src/Connectors/VectorData.Abstractions/VectorStorage/IVectorStore.cs
Show resolved
Hide resolved
dotnet/src/Connectors/VectorData.Abstractions/VectorStorage/IVectorStore.cs
Show resolved
Hide resolved
...t/src/Connectors/Connectors.Memory.AzureAISearch/AzureAISearchVectorStoreRecordCollection.cs
Show resolved
Hide resolved
serviceType == typeof(VectorizableTextSearchMetadata) ? this._vectorizableTextSearchMetadata : | ||
serviceType == typeof(KeywordHybridSearchMetadata) ? this._keywordHybridSearchMetadata : | ||
serviceType == typeof(SearchIndexClient) ? this._searchIndexClient : | ||
serviceType == typeof(SearchClient) ? this._searchClient : |
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 isn't against your work or this PR at all, but I honestly am not sure why we need to expose an Azure AI Search via a GetService() API on our own abstraction... If the user needs a SearchClient (or any other connector-specific service), isn't it more reasonable for them to simply register that in DI themselves and get that directly, rather than getting it through our abstraction? In other words, it seems much more reasonable for the user to have a SearchClient in DI, have that picked up by the MEVD connector, and wherever they need the SearchClient, get that directly.
Again, I know that this is the MEAI approach and you're just aligning to it - so this isn't really about your work. Let me have a quick conversation about this offline to understand what I'm missing.
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.
Sure thing. From my point of view, I guess there is no harm to expose underlying services via GetService
if we already do that for metadata classes. I imagine when I work on some class, and I'm already injecting IVectorStoreRecordCollection
, but for some reason I also need an access to underlying SDK, I can simply do collection.GetService()
instead of introducing another dependency to that class, updating all the places where it's registered, unit tests and so on. It's also possible to mock GetService
so it can return anything you want for your test scenarios.
As you said, in order to get an access directly, you will have to register that underlying service in DI on your side, but I don't think it should be required. In my app, I can simply do services.AddRedisVectorStore
and with GetService
I still can get an underlying service without registering it in DI, which sounds useful.
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.
If it's OK, let me think about this for another day or two, OK? I'm trying to figure out exactly in which scenarios this makes sense etc.
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.
@roji Another option - if we are not sure about returning underlying services, we can't remove this logic for now and we can always add it later if we decide otherwise without breaking changes.
### Motivation and Context <!-- Thank you for your contribution to the semantic-kernel repo! Please help reviewers and future users, providing the following information: 1. Why is this change required? 2. What problem does it solve? 3. What scenario does it contribute to? 4. If it fixes an open issue, please link to the issue here. --> This PR fixes links that were found unreachable by `linkspector`: https://github.com/microsoft/semantic-kernel/actions/runs/13954045797/job/39063140670?pr=11055 ### Contribution Checklist <!-- Before submitting this PR, please make sure: --> - [x] The code builds clean without any errors or warnings - [x] The PR follows the [SK Contribution Guidelines](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md) and the [pre-submission formatting script](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts) raises no violations - [x] All unit tests pass, and I have added new tests where possible - [x] I didn't break anyone 😄
…/semantic-kernel into vector-data-metadata
Motivation and Context
Related: #11013
This PR marks
IVectorStoreRecordCollection.CollectionName
property as obsolete in favor ofMetadata
classes andGetService
method (the approach which is also used in MEAI).GetService
method is added to each vector store interface and each implementation is updated to returnMetadata
classes or underlying services used for breaking-glass scenarios. (Note: this is a breaking change for already existing implementations, since newGetService
method should be implemented).The changes allow to:
CollectionName
, but other properties likeVectorStoreName
,DatabaseName
and any other potential properties in the future. These properties are going to be used in telemetry decorators, which will be implemented in separate PR.@westey-m @roji
Contribution Checklist