Repository navigation
Milvus: add the MEVD provider - #77
Overvolted wants to merge 9 commits into
Conversation
Add the Milvus.Client-backed package, typed and dynamic collections, scalar and array filtering, vector and BM25 hybrid search, dependency injection, documentation, and unit and conformance tests. Co-authored-by: Federico Laurianti <laurianti@gmail.com>
|
@dotnet-policy-service agree |
There was a problem hiding this comment.
🟡 Changes recommended
Initialization recovery and reusable test-container lifetime issues remain unresolved.
2 open issues
What changed in this PR
Adds CommunityToolkit.VectorData.Milvus to the toolkit’s Microsoft.Extensions.VectorData providers, addressing #76 with Milvus 2.6 support.
Changes:
- Implements typed and dynamic collections, filtering, vector search, BM25 hybrid search, and dependency injection.
- Adds unit tests and container-backed conformance suites, including explicit checks for unsupported capabilities.
- Registers the projects and dependencies and documents setup and platform limitations.
| File | Description |
|---|---|
| README.md | Adds Milvus package links and badges. |
| MEVD/test/Milvus.UnitTests/MilvusTestRecord.cs | Defines a shared test model. |
| MEVD/test/Milvus.UnitTests/MilvusStringEscapingTests.cs | Tests Unicode and string escaping. |
| MEVD/test/Milvus.UnitTests/MilvusMappingTests.cs | Tests schemas, mapping, metrics, and validation. |
| MEVD/test/Milvus.UnitTests/MilvusHybridSearchTests.cs | Tests hybrid-search registration and validation. |
| MEVD/test/Milvus.UnitTests/MilvusFullTextTests.cs | Tests BM25 schemas and consumer definitions. |
| MEVD/test/Milvus.UnitTests/MilvusFilterTranslatorTests.cs | Tests scalar filters and null semantics. |
| MEVD/test/Milvus.UnitTests/MilvusEnsureCollectionTests.cs | Tests collection initialization and load timeouts. |
| MEVD/test/Milvus.UnitTests/MilvusDatabaseTests.cs | Tests database metadata and authentication options. |
| MEVD/test/Milvus.UnitTests/MilvusCollectionTests.cs | Tests collection services and input validation. |
| MEVD/test/Milvus.UnitTests/MilvusClientLifetimeTests.cs | Tests client ownership and error wrapping. |
| MEVD/test/Milvus.UnitTests/MilvusArrayTests.cs | Tests array schemas and mapping. |
| MEVD/test/Milvus.UnitTests/MilvusArrayFilterTests.cs | Tests array-membership translation. |
| MEVD/test/Milvus.UnitTests/Milvus.UnitTests.csproj | Configures the unit-test project. |
| MEVD/test/Milvus.UnitTests/.editorconfig | Configures test-specific diagnostics. |
| MEVD/test/Milvus.ConformanceTests/TypeTests/MilvusKeyTypeTests.cs | Exercises supported key types. |
| MEVD/test/Milvus.ConformanceTests/TypeTests/MilvusEmbeddingTypeTests.cs | Enables embedding-type conformance tests. |
| MEVD/test/Milvus.ConformanceTests/TypeTests/MilvusDataTypeTests.cs | Defines supported data-type coverage. |
| MEVD/test/Milvus.ConformanceTests/Support/MilvusTestStore.cs | Manages the test server and clients. |
| MEVD/test/Milvus.ConformanceTests/Support/MilvusFixture.cs | Provides the shared store fixture. |
| MEVD/test/Milvus.ConformanceTests/ModelTests/MilvusNoVectorModelTests.cs | Verifies rejection of vectorless models. |
| MEVD/test/Milvus.ConformanceTests/ModelTests/MilvusNoDataModelTests.cs | Enables models without data fields. |
| MEVD/test/Milvus.ConformanceTests/ModelTests/MilvusMultiVectorModelTests.cs | Enables multiple-vector model coverage. |
| MEVD/test/Milvus.ConformanceTests/ModelTests/MilvusDynamicModelTests.cs | Covers dynamic models and ordering rejection. |
| MEVD/test/Milvus.ConformanceTests/ModelTests/MilvusBasicModelTests.cs | Covers typed models and ordering rejection. |
| MEVD/test/Milvus.ConformanceTests/MilvusTestSuiteImplementationTests.cs | Checks shared-suite implementation. |
| MEVD/test/Milvus.ConformanceTests/MilvusRoundTripTests.cs | Exercises storage, retrieval, and search. |
| MEVD/test/Milvus.ConformanceTests/MilvusNullComparisonTests.cs | Checks filter null semantics against Milvus. |
| MEVD/test/Milvus.ConformanceTests/MilvusIndexKindTests.cs | Enables index-kind conformance tests. |
| MEVD/test/Milvus.ConformanceTests/MilvusHybridSearchTests.cs | Exercises hybrid ranking and search options. |
| MEVD/test/Milvus.ConformanceTests/MilvusFullTextTests.cs | Exercises consumer-style full-text models. |
| MEVD/test/Milvus.ConformanceTests/MilvusFilterTests.cs | Enables shared filter coverage. |
| MEVD/test/Milvus.ConformanceTests/MilvusEnsureCollectionTests.cs | Tests preservation of existing collections. |
| MEVD/test/Milvus.ConformanceTests/MilvusEmbeddingGenerationTests.cs | Covers embedding generation and registration. |
| MEVD/test/Milvus.ConformanceTests/MilvusDistanceFunctionTests.cs | Covers supported and rejected metrics. |
| MEVD/test/Milvus.ConformanceTests/MilvusDependencyInjectionTests.cs | Exercises registration overloads and lifetimes. |
| MEVD/test/Milvus.ConformanceTests/MilvusDatabaseTests.cs | Verifies selected-database isolation. |
| MEVD/test/Milvus.ConformanceTests/MilvusCollectionManagementTests.cs | Enables collection-management coverage. |
| MEVD/test/Milvus.ConformanceTests/MilvusArrayFilterTests.cs | Exercises array filters against Milvus. |
| MEVD/test/Milvus.ConformanceTests/Milvus.ConformanceTests.csproj | Configures .NET 10 container-backed tests. |
| MEVD/src/Milvus/SharedMilvusClient.cs | Shares client ownership through reference counting. |
| MEVD/src/Milvus/README.md | Documents setup, capabilities, and limitations. |
| MEVD/src/Milvus/MilvusVectorStoreOptions.cs | Configures default embedding generation. |
| MEVD/src/Milvus/MilvusVectorStore.cs | Implements store-level collection access. |
| MEVD/src/Milvus/MilvusServiceCollectionExtensions.cs | Adds keyed and unkeyed registrations. |
| MEVD/src/Milvus/MilvusModelBuilder.cs | Validates models and storage names. |
| MEVD/src/Milvus/MilvusMapper.cs | Maps records and vectors to storage. |
| MEVD/src/Milvus/MilvusFilterTranslator.cs | Translates expressions into Milvus filters. |
| MEVD/src/Milvus/MilvusFieldMapping.cs | Maps field types, columns, and literals. |
| MEVD/src/Milvus/MilvusErrorHandler.cs | Wraps SDK failures with operation metadata. |
| MEVD/src/Milvus/MilvusDynamicCollection.cs | Supports dictionary-based collections. |
| MEVD/src/Milvus/MilvusConstants.cs | Defines the provider identifier. |
| MEVD/src/Milvus/MilvusCollectionSearchMapping.cs | Maps scores and threshold direction. |
| MEVD/src/Milvus/MilvusCollectionOptions.cs | Defines collection configuration. |
| MEVD/src/Milvus/MilvusCollectionCreateMapping.cs | Builds schemas and BM25 functions. |
| MEVD/src/Milvus/MilvusCollection.cs | Implements collection operations and searches. |
| MEVD/src/Milvus/Milvus.csproj | Defines the provider package and target frameworks. |
| MEVD/src/Milvus/AssemblyInfo.cs | Adds the assembly-info file with license headers. |
| MEVD/MEVD.slnf | Includes the provider and test projects. |
| Directory.Packages.props | Adds Milvus SDK and Testcontainers versions. |
| CommunityToolkit.AI.slnx | Registers the three Milvus projects. |
Review effort: Balanced
| { | ||
| // The indexes are created with the collection. Those of an existing collection stay as they are: Milvus rejects | ||
| // an index with other settings on an indexed field, or on a field the collection does not have. | ||
| if (!await _milvusClient.Client.HasCollectionAsync(Name, cancellationToken: cancellationToken).ConfigureAwait(false)) |
There was a problem hiding this comment.
Out other existing providers also don't handle index creation failure really well. I think it's acceptable to keep it as implemented here, or just delete the collection on failed index creation and re-throw the index creation exception.
| _client?.Dispose(); | ||
| if (_container is not null) | ||
| { | ||
| await _container.DisposeAsync(); |
adamsitnik
left a comment
There was a problem hiding this comment.
@Overvolted big thanks for your contribution @Overvolted !
The contribution is very promising, I found some issues, but most of them are just subjective nits. PTAL at my comments
| #pragma warning restore CA1711 // Identifiers should not have incorrect suffix | ||
| { | ||
| /// <summary>Metadata about vector store record collection.</summary> | ||
| private readonly VectorStoreCollectionMetadata _collectionMetadata; |
There was a problem hiding this comment.
subjective: the order of fields is mixed, we have some private readonly, then private static readonly, then const then ``private readonly` again. This should be consistent, ideally following https://stackoverflow.com/a/310967/5852046
|
|
||
| internal MilvusCollection(Func<SharedMilvusClient> clientFactory, string name, Func<MilvusCollectionOptions, CollectionModel> modelFactory, MilvusCollectionOptions? options) | ||
| { | ||
| // Verify. |
There was a problem hiding this comment.
nit: Please don't add comments for obvious things (even if some of the code base already does that)
| // Verify. |
|
|
||
| options ??= MilvusCollectionOptions.Default; | ||
|
|
||
| // Assign. |
There was a problem hiding this comment.
| // Assign. |
| /// <inheritdoc /> | ||
| protected override void Dispose(bool disposing) | ||
| { | ||
| if (Interlocked.Exchange(ref _disposed, 1) == 0) |
There was a problem hiding this comment.
Do we really need to store this information in an extra field of SharedMilvusClient has it's own reference count?
| { | ||
| var indexType = MilvusCollectionCreateMapping.GetIndexType(vector); | ||
| var parameters = indexType == IndexType.Hnsw | ||
| ? new Dictionary<string, string> { ["M"] = "16", ["efConstruction"] = "200" } : null; |
There was a problem hiding this comment.
Please add some comments about what these magic values are.
| private const string DeleteName = "Delete"; | ||
|
|
||
| /// <summary>Milvus client that can be used to manage the collections and records in a Milvus store.</summary> | ||
| private readonly SharedMilvusClient _milvusClient; |
There was a problem hiding this comment.
subjective nit: I would call it ClientWrapper to follow existing convention (https://github.com/CommunityToolkit/AI/blob/main/MEVD/src/AzureCosmosDB/ClientWrapper.cs), there is also no need to use "milvus" in the field name, as it's Milvus Collection so we don't expect to have any non-Milvus clients here.
| private readonly SharedMilvusClient _milvusClient; | |
| private readonly ClientWrapper _clientWrapper; |
| var records = await GetAsync([key], options, cancellationToken).ToListAsync(cancellationToken).ConfigureAwait(false); | ||
| return records.FirstOrDefault(); |
There was a problem hiding this comment.
Do we need to call ToListAsync just to get a single element?
| { | ||
| Throw.IfNullOrWhitespace(name); | ||
|
|
||
| if (typeof(TKey) != typeof(long) && typeof(TKey) != typeof(string) && typeof(TKey) != typeof(Guid) && typeof(TKey) != typeof(object)) |
There was a problem hiding this comment.
Please introduce a helper for Key type check in MilvusModelBuilder.cs, as it already contains similar check
| private const int HybridSearchCandidates = 10; | ||
|
|
||
| // The most records a Milvus search ranks. | ||
| private const int MaxSearchLimit = 16384; |
There was a problem hiding this comment.
All these consts could live in MilvusConstants.cs
| using Xunit; | ||
| namespace Milvus.ConformanceTests.TypeTests; | ||
| public class MilvusKeyTypeTests(MilvusKeyTypeTests.Fixture fixture) : KeyTypeTests(fixture), IClassFixture<MilvusKeyTypeTests.Fixture> |
There was a problem hiding this comment.
Please separate the last using, namespace and class with an empty line. Do it in other files as well.
| using Xunit; | |
| namespace Milvus.ConformanceTests.TypeTests; | |
| public class MilvusKeyTypeTests(MilvusKeyTypeTests.Fixture fixture) : KeyTypeTests(fixture), IClassFixture<MilvusKeyTypeTests.Fixture> | |
| using Xunit; | |
| namespace Milvus.ConformanceTests.TypeTests; | |
| public class MilvusKeyTypeTests(MilvusKeyTypeTests.Fixture fixture) : KeyTypeTests(fixture), IClassFixture<MilvusKeyTypeTests.Fixture> |

Adds
CommunityToolkit.VectorData.Milvus, the Milvus provider proposed in #76, withMEVD/test/Milvus.UnitTestsandMEVD/test/Milvus.ConformanceTests. It follows the layout of the Qdrant provider and builds on Milvus.Client 2.6.0. It needs Milvus 2.6.The conformance tests start
milvusdb/milvus:v2.6.25with Testcontainers.Milvus, so themevd.ymlmatrix picks them up. Every shared suite runs. The overrides assertNotSupportedExceptionfor what Milvus does not have: ordering, models without vectors, and the distance functions other than cosine, dot product, Euclidean and squared Euclidean. The conformance project targets .NET 10 only: on .NET Framework, Milvus.Client needs TLS, and the test container uses plaintext gRPC. The README says what .NET Framework needs.The tests of full-text indexing and hybrid search mirror the collection definitions of the Semantic Kernel
TextSearchStoreand of the Agent FrameworkChatHistoryMemoryProvider, which mark their text as full-text indexed.Co-authored by @Laurianti
Related: microsoft/semantic-kernel#12509, microsoft/semantic-kernel#13391, milvus-io/milvus-sdk-csharp#102.
Fixes #76