Adds Milvus to the Aspire hosting/component packages #4179
Conversation
Aspire.sln
Outdated
| 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. |
| @@ -0,0 +1,12 @@ | |||
| var builder = DistributedApplication.CreateBuilder(args); | |||
|
|
|||
| var token = builder.AddParameter("milvusauth", true); | |||
There was a problem hiding this comment.
Is this necessary? Can we just generate an auth token instead?
There was a problem hiding this comment.
No. there is only one user in a default Milvus container (root:Milvus). This cannot be configured presently via environment variables or anything and can only be configured after the fact (or additional users after the fact). See: milvus-io/milvus#33058 and milvus-io/milvus#33032 (comment)
There was a problem hiding this comment.
How's this code going to work then?
aspire/src/Aspire.Hosting.Milvus/MilvusBuilderExtensions.cs
Lines 35 to 37 in 0a3cbec
There was a problem hiding this comment.
Me thinking aloud why I kept this in...
It is possible to reset the root token. But right now it has to be done after the fact. So in theory someone could do a data volume, have a script (or some other client operation) that changes the root login (or adds another user), and then this would need to be changed (they would populate the parameter. Correct here that the CreateDefaultPasswordParameter wouldn't really work in this instance. I was also trying to avoid hard-coding the default root creds here.
To me the ideal would look like apiKey?.Resource ?? DefaultMilvusApiKeyParameter - per discussion in teams channels, there is no easy way to create a ParameterResource with a string value default.
Thoughts welcome here...
There was a problem hiding this comment.
Changed to remove generation, made the apiKey notnull, not burning in the hard-coded default in code.
There was a problem hiding this comment.
@timheuer - thinking more about this - how is this going to work in a deployed/production environment? If I use azd to deploy an app that has a Milvus resource, will the ApiKey always be root:Milvus even in a deployed environment? That doesn't seem secure.
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? |
src/Components/Aspire.Milvus.Client/Aspire.Milvus.Client.csproj
Outdated
Show resolved
Hide resolved
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