Skip to content
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

Update to Aspire 9 #43

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Update to Aspire 9 #43

wants to merge 11 commits into from

Conversation

DamianEdwards
Copy link
Member

This updates Aspire to the upcoming 9.0-rc.1 release and retargets the projects to net9.0.

It also moves the Python inference app into a container so that Python doesn't need to be installed on the host machine when developing. The container bind-mounts the python app files and enables Uvicorn reloading when running in dev.

Comment on lines +10 to +21
<PackageVersion Include="Aspire.Azure.Storage.Blobs" Version="9.0.0-rc.1.24511.1" />
<PackageVersion Include="Aspire.AppHost.SDK" Version="$(AspireVersion)" />
<PackageVersion Include="Aspire.Hosting.AppHost" Version="9.0.0-rc.1.24511.1" />
<PackageVersion Include="Aspire.Hosting.Azure.AppContainers" Version="9.0.0-rc.1.24511.1" />
<PackageVersion Include="Aspire.Hosting.Azure.Storage" Version="9.0.0-rc.1.24511.1" />
<PackageVersion Include="Aspire.Hosting.PostgreSQL" Version="9.0.0-rc.1.24511.1" />
<PackageVersion Include="Aspire.Hosting.Python" Version="9.0.0-rc.1.24511.1" />
<PackageVersion Include="Aspire.Hosting.Qdrant" Version="9.0.0-rc.1.24511.1" />
<PackageVersion Include="Aspire.Hosting.Redis" Version="9.0.0-rc.1.24511.1" />
<PackageVersion Include="Aspire.Hosting.Testing" Version="9.0.0-rc.1.24511.1" />
<PackageVersion Include="Aspire.Npgsql.EntityFrameworkCore.PostgreSQL" Version="9.0.0-rc.1.24511.1" />
<PackageVersion Include="Aspire.StackExchange.Redis" Version="9.0.0-rc.1.24511.1" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These versions aren't in sync. It would be nice to keep using the common AspireVersion property.

Suggested change
<PackageVersion Include="Aspire.Azure.Storage.Blobs" Version="9.0.0-rc.1.24511.1" />
<PackageVersion Include="Aspire.AppHost.SDK" Version="$(AspireVersion)" />
<PackageVersion Include="Aspire.Hosting.AppHost" Version="9.0.0-rc.1.24511.1" />
<PackageVersion Include="Aspire.Hosting.Azure.AppContainers" Version="9.0.0-rc.1.24511.1" />
<PackageVersion Include="Aspire.Hosting.Azure.Storage" Version="9.0.0-rc.1.24511.1" />
<PackageVersion Include="Aspire.Hosting.PostgreSQL" Version="9.0.0-rc.1.24511.1" />
<PackageVersion Include="Aspire.Hosting.Python" Version="9.0.0-rc.1.24511.1" />
<PackageVersion Include="Aspire.Hosting.Qdrant" Version="9.0.0-rc.1.24511.1" />
<PackageVersion Include="Aspire.Hosting.Redis" Version="9.0.0-rc.1.24511.1" />
<PackageVersion Include="Aspire.Hosting.Testing" Version="9.0.0-rc.1.24511.1" />
<PackageVersion Include="Aspire.Npgsql.EntityFrameworkCore.PostgreSQL" Version="9.0.0-rc.1.24511.1" />
<PackageVersion Include="Aspire.StackExchange.Redis" Version="9.0.0-rc.1.24511.1" />
<PackageVersion Include="Aspire.Azure.Storage.Blobs" Version="$(AspireVersion)" />
<PackageVersion Include="Aspire.AppHost.SDK" Version="$(AspireVersion)" />
<PackageVersion Include="Aspire.Hosting.AppHost" Version="$(AspireVersion) />
<PackageVersion Include="Aspire.Hosting.Azure.AppContainers" Version="$(AspireVersion)" />
<PackageVersion Include="Aspire.Hosting.Azure.Storage" Version="$(AspireVersion)" />
<PackageVersion Include="Aspire.Hosting.PostgreSQL" Version="$(AspireVersion)" />
<PackageVersion Include="Aspire.Hosting.Python" Version="$(AspireVersion)" />
<PackageVersion Include="Aspire.Hosting.Qdrant" Version="$(AspireVersion)" />
<PackageVersion Include="Aspire.Hosting.Redis" Version="$(AspireVersion)" />
<PackageVersion Include="Aspire.Hosting.Testing" Version="$(AspireVersion)" />
<PackageVersion Include="Aspire.Npgsql.EntityFrameworkCore.PostgreSQL" Version="$(AspireVersion)" />
<PackageVersion Include="Aspire.StackExchange.Redis" Version="$(AspireVersion)" />

<PackageVersion Include="Microsoft.Extensions.Hosting" Version="8.0.0" />
<PackageVersion Include="Microsoft.Extensions.ServiceDiscovery" Version="$(AspireVersion)" />
<PackageVersion Include="Microsoft.Extensions.Hosting" Version="9.0.0-rc.2.24473.5" />
<PackageVersion Include="Microsoft.Extensions.ServiceDiscovery" Version="9.0.0-rc.1.24511.1" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<PackageVersion Include="Microsoft.Extensions.ServiceDiscovery" Version="9.0.0-rc.1.24511.1" />
<PackageVersion Include="Microsoft.Extensions.ServiceDiscovery" Version="$(AspireVersion)" />

@@ -1,27 +1,25 @@
<Project Sdk="Microsoft.NET.Sdk">

<Sdk Name="Aspire.AppHost.Sdk" Version="9.0.0-rc.1.24511.1" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<Sdk Name="Aspire.AppHost.Sdk" Version="9.0.0-rc.1.24511.1" />
<Sdk Name="Aspire.AppHost.Sdk" Version="9.0.0-rc.1.24509.13" />

<configuration>
<packageSources>
<!--To inherit the global NuGet package sources remove the <clear/> line below -->
<clear />
<add key="nuget" value="https://api.nuget.org/v3/index.json" />
<add key="dotnet8" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet8/nuget/v3/index.json" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this for the Aspire 9.x prerelease packages?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. These will hit public nuget.org tomorrow.

@@ -21,7 +22,7 @@ private async Task<DistributedApplication> InitializeAsync()
Environment.CurrentDirectory = Projects.AppHost.ProjectPath;
Environment.SetEnvironmentVariable("E2E_TEST", "true");
Environment.SetEnvironmentVariable("E2E_TEST_CHAT_COMPLETION_CACHE_DIR",
Path.Combine(Projects.E2ETest.ProjectPath, "ChatCompletionCache"));
Path.Combine(Projects.AppHost.ProjectPath, "ChatCompletionCache"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of interest, why change this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also if you do change this, I think you'd need to regenerate the cache or at least move all its files to the new directory, since they are meant to be stored in source control.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, I think I changed this as a test and forgot to change it back. For a moment at least the E2ETest project no longer showed up as a member of Projects.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SteveSandersonMS Actually, I take it back...this still is an issue. E2ETest is no longer in the Projects namespace nor is there any metadata created for it. I'm not sure how this code originally worked, before the update. Is there a better way to get the project path now?

@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Oct 14, 2024

It also moves the Python inference app into a container

I think this is absolutely great and we definitely should do it.

However I also think we shouldn't consider this to be a complete solution for #42. Since getting this branch, I've been trying for about 30 mins to make the app run but haven't yet got the Python container to build properly. It took over 10 mins to download the Python dependencies then stopped for a long time saying exporting to image. Eventually it continued but seems to have failed with this at the end of the Docker log:

2024-10-14T14:23:01 #9 DONE 45.0s
2024-10-14T14:23:01 
2024-10-14T14:23:01 View build details: docker-desktop://dashboard/build/desktop-linux/desktop-linux/vwqseg2b22idn618phjplc0xm
2024-10-14T14:23:03 
2024-10-14T14:23:03 Error response from daemon: No such image: sha256:b40b44cfaec46ee67768396149dc00c5df75c3ddf76e7f57ab3759b2cb38a109
2024-10-14T14:23:07 Error response from daemon: No such image: sha256:b40b44cfaec46ee67768396149dc00c5df75c3ddf76e7f57ab3759b2cb38a109
2024-10-14T14:23:12 Error response from daemon: No such image: sha256:b40b44cfaec46ee67768396149dc00c5df75c3ddf76e7f57ab3759b2cb38a109

I don't know what's going on with that, and will next try clearing out the Docker image it built and try starting it again. This is not to criticise anything in the implementation, which looks entirely correct, but just to acknowledge that even with this automation it remains prone to be a sticking point for new users.

So although I definitely think it's better to run this in Docker than not do, it's probably better still if we also make the Python dependency optional (defaulting to not used).

Update: After restarting, it did successfully complete building the Docker container and runs the Python code.

var pythonInference = builder.AddDockerfile("python-inference", "../PythonInference", /* default dockerfile */ null, stage)
.WithHttpEndpoint(targetPort: 62394, env: "UVICORN_PORT")
.WithContainerRuntimeArgs("--gpus=all")
.WithLifetime(ContainerLifetime.Persistent);
Copy link
Member

@SteveSandersonMS SteveSandersonMS Oct 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this definitely work? It does cause the Docker container to keep running even after AppHost is shut down (albeit removing the port mapping, which is a bit inconvenient).

But then each time I start the AppHost project using Ctrl+F5, it kills the previous Docker container and starts a new one, which seems to defeat the purpose of the feature.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this definitely work? It does cause the Docker container to keep running even after AppHost is shut down (albeit removing the port mapping, which is a bit inconvenient).

If you want stable ports inside and outside of Aspire for this resource you can pass the desired port numbers and disable the proxying on the endpoint.

But then each time I start the AppHost project using Ctrl+F5, it kills the previous Docker container and starts a new one, which seems to defeat the purpose of the feature.

Definitely shouldn't be doing that and I wasn't seeing that locally. It will only do that if it detects changes to the container definition but I'm not sure of the exact variables it includes. @danegsta?

Copy link
Member

@danegsta danegsta Oct 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default we consider the container image name (or image sha in the case of building via dockerfile), ports, volume mounts, environment, and the command/arguments used to launch the container.

The port mapping shouldn't be lost for a persistent container, but it generally won't show up in the Docker Desktop UI due to a seeming bug in how they report randomly assigned host ports (which we use). You have to inspect the container and look in the networking config section to see the real port, otherwise it just shows up as 0 in the other inspected port data and is missing from the UI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants