Adds Milvus to the Aspire hosting/component packages #4179
Conversation
| EndProject | ||
| Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Consumer", "playground\kafka\Consumer\Consumer.csproj", "{7AA4C56C-3BB2-4FF0-BB03-F3F0D6A4FDAB}" | ||
| Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Consumer", "playground\kafka\Consumer\Consumer.csproj", "{7AA4C56C-3BB2-4FF0-BB03-F3F0D6A4FDAB}" |
There was a problem hiding this comment.
Any idea why these keep getting updated? Seen it in other PRs too. Result of a merge or something?
There was a problem hiding this comment.
I've got a note in to the project team -- I noticed this a while back...I can't explain either :-(
playground/milvus/MilvusPlayground.ApiService/Properties/launchSettings.json
Show resolved
Hide resolved
playground/milvus/MilvusPlayground.AppHost/Properties/launchSettings.json
Show resolved
Hide resolved
|
This is looking pretty good. Mostly just some minor things and questions. @eerhardt for a once over for the component side of things. I gave it a once over but he is more attuned to that side of things. |
|
One thing that I'm thinking about is allowing unauthenticated access to Attu? |
Attu requires authenticated access to the Milvus instance -- much like things like PgAdmin have a login prompt as well. Is that what you're referring to? Attu starts with a login requirement to an instance of Milvus, it isn't open-ended. |
src/Components/Aspire.Milvus.Client/Aspire.Milvus.Client.csproj
Outdated
Show resolved
Hide resolved
| protected override void TriggerActivity(MilvusClient service) | ||
| { | ||
| service.GetVersionAsync().Wait(); | ||
| } |
There was a problem hiding this comment.
Do we have any tracing with Milvus?
What is the tracing, metrics, and health checks plan for this component?
There was a problem hiding this comment.
I was planning on logging issues on the component to add some of these. @roji do you know if these may already be in the works?
There was a problem hiding this comment.
HealthCheck added, tracing/metrics logged at Milvus.Client repo
There was a problem hiding this comment.
The tracking issue on the Milvus SDK side is milvus-io/milvus-sdk-csharp#85.
I'm planning on adding tracing support there, but it may take me a bit of time before I get around to it... Re metrics, I think it's more appropriate to get them from Milvus itself (the server), which emits them - see the issue above for some discussion on that. We may want to have a more general discussion on the client vs. server preference for metrics.
- Added health check - Added example in doc comments - Modified tests to use testcontainer - Added WithDataBindMount
|
Can you also add some end-to-end tests? aspire/tests/Aspire.EndToEnd.Tests/IntegrationServicesTests.cs Lines 26 to 35 in 5f2af96 See #3839 for an example. |
(handled in Client already)
|
|
||
| private static void ConfigureAttuContainer(EnvironmentCallbackContext context, MilvusServerResource resource) | ||
| { | ||
| context.EnvironmentVariables.Add("MILVUS_URL", $"{resource.PrimaryEndpoint.Scheme}://{resource.PrimaryEndpoint.ContainerHost}:{resource.PrimaryEndpoint.Port}"); |
There was a problem hiding this comment.
| context.EnvironmentVariables.Add("MILVUS_URL", $"{resource.PrimaryEndpoint.Scheme}://{resource.PrimaryEndpoint.ContainerHost}:{resource.PrimaryEndpoint.Port}"); | |
| context.EnvironmentVariables.Add("MILVUS_URL", $"{resource.PrimaryEndpoint.Property(EndpointProperty.Url)}"); |
There was a problem hiding this comment.
I don't think this is a correct edit @eerhardt. See: https://github.com/dotnet/aspire/blob/main/src/Aspire.Hosting.MySql/PhpMyAdminConfigWriterHook.cs#L37 (and Postgres as well) -- for the local dev experience and the admin tools, they are containers needing to access containers. The proxy ports don't help and need the container host network.
There was a problem hiding this comment.
@davidfowl @mitchdenny - would it make sense to make a "ContainerHostUrl" property on an Endpoint? That way would could simplify code like this?
There was a problem hiding this comment.
I think that we will be able to use the .Url property when this is addressed:
|
@eerhardt any other feedback to get this over the last mile? |
Adds the Milvus vector database to Aspire.
Requires that a parameter exists for the root auth api key
No healthchecks/tracing/metrics available in the C# Milvus.Client
2024-05-14_12-00-12.mp4
/cc @roji @luisquintanilla
Microsoft Reviewers: Open in CodeFlow