Add support for running python projects #4142
Conversation
dotnet-issue-labeler
bot
added
the
area-components
dotnet-policy-service
bot
added
the
community-contribution
src/Aspire.Hosting.Python/FlaskProjectResourceExtensionBuilder.cs
Outdated
Show resolved
Hide resolved
|
@davidfowl I've been thinking about things like Node.js (and now Python). I'm wondering if whether to make things easier for people we should create a boilerplate Dockerfile if one doesn't already exist. The idea being that it gives people a starting point. So if someone does |
|
I don't think we should be writing files to disk using these APIs, but I like the idea that they would publish as a docker file by default. The publish operation would throw because it couldn't find one. That would instruct the user to add one for their language. |
src/Aspire.Hosting.Python/FlaskProjectResourceExtensionBuilder.cs
Outdated
Show resolved
Hide resolved
|
I can see I broke the build because I forgot to update the playground apphost. I will do that tomorrow morning (GMT+1) |
| public void AddPythonProject_SetsResourcePropertiesCorrectly() | ||
| { | ||
| var pythonProjectDirectory = Path.Combine(s_playgroundDirectory, "script_only"); | ||
| var appBuilder = DistributedApplication.CreateBuilder(); | ||
|
|
||
| appBuilder.AddPythonProject("python", pythonProjectDirectory, "main.py"); | ||
|
|
||
| var app = appBuilder.Build(); | ||
| var appModel = app.Services.GetRequiredService<DistributedApplicationModel>(); | ||
| var executableResources = appModel.GetExecutableResources(); | ||
|
|
||
| var pythonProjectResource = Assert.Single(executableResources); | ||
|
|
||
| Assert.Equal("python", pythonProjectResource.Name); | ||
| Assert.Equal(pythonProjectDirectory, pythonProjectResource.WorkingDirectory); | ||
| } |
There was a problem hiding this comment.
I would love to see some pure unit tests like this https://github.com/dotnet/aspire/blob/main/tests/Aspire.Hosting.Tests/Garnet/AddGarnetTests.cs
There was a problem hiding this comment.
@davidfowl I need a little advice on this. The python virtual env layout varies across platforms, on Windows there's a scripts directory while on linux and mac there's a bin directory for the executables. I could add OS detection and assume that the scripts/binaries are in the right spot. But I felt that it would be nicer to actually check on disk if the binaries are there.
Because of this choice I figured it would make sense to have local facts with an actual python environment instead of writing pure unit-tests.
I can make purer unit-tests by either: Making the assumption about the virtual env layout and not checking the disk. Or I can write temp dummy files to disk to make the unit-test pass.
What do you prefer? Or do you have other ideas?
There was a problem hiding this comment.
What I'm doing with the incoming WithDockerfile(...) extension is creating a temp directory and dropping the minimum number of files I need in there. If the tests are going to actually spin up Python that is what you'd need to do.
| string[] allowedExtensions = [".exe", ".cmd", ".bat", ""]; | ||
| string[] executableDirectories = ["bin", "Scripts"]; |
There was a problem hiding this comment.
Filter this based on operating system?
| /// <param name="executablePath">The path to the executable used to run the python project.</param> | ||
| /// <param name="projectDirectory">The path to the directory containing the python project.</param> | ||
| public class PythonProjectResource(string name, string executablePath, string projectDirectory) | ||
| : ExecutableResource(name, executablePath, projectDirectory), IResourceWithServiceDiscovery |
There was a problem hiding this comment.
Do we want this to implement IResourceWithServiceDiscovery?
wmeints
force-pushed
the
python-hosting-support
branch
from
0f772cc
to
30c850f
Compare
| /// </summary> | ||
| public static class PythonProjectResourceBuilderExtensions | ||
| { | ||
| /// <summary> |
There was a problem hiding this comment.
The <summary> should be a short one sentence thing. Move the rest into <remarks> (after <returns>).
Would be good to add an <example> as well for each of the extension methods.
| private static readonly string s_playgroundDirectory = Path.GetFullPath(Path.Combine(AppContext.BaseDirectory, "../../../../../playground/python/")); | ||
|
|
||
| [LocalOnlyFact("python")] | ||
| public void AddPythonProject_SetsResourcePropertiesCorrectly() |
There was a problem hiding this comment.
@radical heads up on this coming into the repo ... when we start running these tests in C# we are going to need Python available.
| [LocalOnlyFact("python")] | ||
| public void AddPythonProject_SetsResourcePropertiesCorrectly() | ||
| { | ||
| var pythonProjectDirectory = Path.Combine(s_playgroundDirectory, "script_only"); |
There was a problem hiding this comment.
You are going to need to use the testbuilder here (look at some of the other tests). Otherwise this will leak file watch handles.
| AddProjectArguments(scriptPath, scriptArgs, context); | ||
| }); | ||
|
|
||
| if(!string.IsNullOrEmpty(instrumentationExecutable)) |
| /// <param name="scriptArgs">The arguments for the script.</param> | ||
| /// <returns>A reference to the <see cref="IResourceBuilder{T}"/>.</returns> | ||
| public static IResourceBuilder<PythonProjectResource> AddPythonProject( | ||
| this IDistributedApplicationBuilder builder, string name, string projectDirectory, string scriptPath, string virtualEnvironmentPath = ".venv", params string[] scriptArgs) |
There was a problem hiding this comment.
Can you make the default value for virtualEnvironmentPath null and then check for it and replace it with the default inside the method. Also note the earlier comments I made about making <summary> shorter and more concise and adding a <remarks> section (where you can have your paragraphs.
This PR introduces support for running Python projects as part of an Aspire solution. I took the NodeJS extension as inspiration but had to add a couple of extra pieces of logic since Python is a lot less standardized when it comes to how projects are structured.
Initially I created this project: https://github.com/wmeints/aspire-python but I feel that this PR is a much more useful way to add support for Python. I think it's a great resolution for #2760.
Assumptions made by the extension
.venv. Although you can change that.