-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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: Update Milvus memory connector to 2.3 #5593
Conversation
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.
Nice. Thanks, Shay.
@roji - [Not blocking the PR, more for my understanding] - When I look at https://github.com/microsoft/semantic-kernel/actions/runs/8374907874/job/22931151816 for the test results page, I see it skipped over the Integration tests. How do you know if the test changes you have actually ran successfully? |
@SamMonoRT yeah, that's a good question - I'm obviously not a maintainer on this repo but here's what I suspect is the situation. The integration tests involve actually communicating with the service or vector database in question, and require the prior setup of that service/database; since that's hard to do for each and every service, I think the integration tests in this repo are executed manually, and it's up to the maintainer of each connector to ensure that everything works correctly (otherwise this build pipeline would have to start up all the supported vector databases, communicate to the Azure services, etc. etc.). BTW testcontainers are meant to solve this exact problem - the tests themselves are responsible for fetching, configuring and managing the container for the service in question. This PR integrates the new Milvus testcontainer into the integration tests, but most other tests are still manual-only. Maybe with time, more connectors will integrate testcontainers into their test suite, and it would make sense to run the integration test suite in CI. (of course, before submititng this PR I verified that all integration tests pass) |
dotnet/src/IntegrationTests/Connectors/Memory/Milvus/MilvusMemoryStoreTests.cs
Show resolved
Hide resolved
Changes cannot be merged until the integration tests are skipped by default
Thanks @markwallace-microsoft! |
This updates the version of the lower-level Milvus SDK from 2.2 to 2.3 ([recently released](https://www.nuget.org/packages/Milvus.Client)). In addition, does the following changes: * Changes the Milvus integration tests to use the [Milvus testcontainer module for .NET](https://testcontainers.com/modules/milvus/); this means that a Milvus container is brought up when the tests run, removing the need to manually configure an external one. * As part of this, I removed the skipping to make the Milvus tests run by default; they're very reliable as far as I can tell, but if you'd rather I roll back this change (so that tests are only executed explicitly as is typical in the repo), let me know and I'll do it. * Implement the IMemoryStore Upsert APIs over the new 2.3 Milvus upsert operation. * Allow specifying the consistency level of a MilvusMemoryStore on creation. /cc @shawncal @lemillermicrosoft /cc @stephentoub @luisquintanilla @SamMonoRT Co-authored-by: Mark Wallace <127216156+markwallace-microsoft@users.noreply.github.com>
This updates the version of the lower-level Milvus SDK from 2.2 to 2.3 (recently released).
In addition, does the following changes:
/cc @shawncal @lemillermicrosoft
/cc @stephentoub @luisquintanilla @SamMonoRT