-
Notifications
You must be signed in to change notification settings - Fork 789
Python hosting: Support .venv lookup by walking up parent directories #12616
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
Merged
Merged
Changes from 10 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
d729045
Initial plan
Copilot faf5a7b
Implement .venv lookup in both Python app and AppHost directories
Copilot 2d05cae
Update documentation for WithVirtualEnvironment fallback behavior
Copilot 9d9c521
Refactor: Move .venv lookup logic to AddPythonAppCore, use explicit p…
Copilot 7c32cb4
Refactor tests: Add helper method for Python path assertions and simp…
Copilot af30659
Add VIRTUAL_ENV configuration support to virtual environment lookup
Copilot a78085f
Remove VIRTUAL_ENV configuration check from virtual environment lookup
Copilot bb55867
Add clarifying comments for "nearby" check in virtual environment lookup
Copilot c866034
Address PR feedback: simplify logic, use appDirectory, remove redunda…
Copilot 7881363
Walk up parent directories from Python app, stop at AppHost's parent
Copilot 26930ab
Use platform-aware path comparison for directory boundary check
Copilot bf13f27
Only walk up directories when app is under AppHost's parent tree
Copilot 08eb9ab
Update src/Aspire.Hosting.Python/PythonAppResourceBuilderExtensions.cs
eerhardt File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -76,7 +76,12 @@ public static IResourceBuilder<PythonAppResource> AddPythonApp( | |
| /// <remarks> | ||
| /// <para> | ||
| /// This method executes a Python script directly using <c>python script.py</c>. | ||
| /// By default, the virtual environment folder is expected to be named <c>.venv</c> and located in the app directory. | ||
| /// By default, the virtual environment is resolved using the following priority: | ||
| /// <list type="number"> | ||
| /// <item>If <c>.venv</c> exists in the app directory, use it.</item> | ||
| /// <item>If <c>.venv</c> exists in the AppHost directory, use it.</item> | ||
| /// <item>Otherwise, default to <c>.venv</c> in the app directory.</item> | ||
| /// </list> | ||
| /// Use <see cref="WithVirtualEnvironment{T}(IResourceBuilder{T}, string)"/> to specify a different virtual environment path. | ||
| /// Use <c>WithArgs</c> to pass arguments to the script. | ||
| /// </para> | ||
|
|
@@ -350,6 +355,14 @@ private static IResourceBuilder<T> AddPythonAppCore<T>( | |
| ArgumentException.ThrowIfNullOrEmpty(entrypoint); | ||
| ArgumentNullException.ThrowIfNull(virtualEnvironmentPath); | ||
|
|
||
| // When using the default virtual environment path, look for existing virtual environments | ||
| // in multiple locations: app directory first, then AppHost directory as fallback | ||
| var resolvedVenvPath = virtualEnvironmentPath; | ||
| if (virtualEnvironmentPath == DefaultVirtualEnvFolder) | ||
| { | ||
| resolvedVenvPath = ResolveDefaultVirtualEnvironmentPath(builder, appDirectory, virtualEnvironmentPath); | ||
| } | ||
|
|
||
| // python will be replaced with the resolved entrypoint based on the virtualEnvironmentPath | ||
| var resource = createResource(name, "python", Path.GetFullPath(appDirectory, builder.AppHostDirectory)); | ||
|
|
||
|
|
@@ -362,7 +375,7 @@ private static IResourceBuilder<T> AddPythonAppCore<T>( | |
| Entrypoint = entrypoint | ||
| }) | ||
| // This will resolve the correct python executable based on the virtual environment | ||
| .WithVirtualEnvironment(virtualEnvironmentPath) | ||
| .WithVirtualEnvironment(resolvedVenvPath) | ||
| // This will set up the the entrypoint based on the PythonEntrypointAnnotation | ||
| .WithEntrypoint(entrypointType, entrypoint); | ||
|
|
||
|
|
@@ -627,6 +640,52 @@ private static void ThrowIfNullOrContainsIsNullOrEmpty(string[] scriptArgs) | |
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Resolves the default virtual environment path by checking multiple candidate locations. | ||
| /// </summary> | ||
| /// <param name="builder">The distributed application builder.</param> | ||
| /// <param name="appDirectory">The Python app directory (relative to AppHost).</param> | ||
| /// <param name="virtualEnvironmentPath">The relative virtual environment path (e.g., ".venv").</param> | ||
| /// <returns>The resolved virtual environment path.</returns> | ||
| private static string ResolveDefaultVirtualEnvironmentPath(IDistributedApplicationBuilder builder, string appDirectory, string virtualEnvironmentPath) | ||
| { | ||
| var appDirectoryFullPath = Path.GetFullPath(appDirectory, builder.AppHostDirectory); | ||
|
|
||
| // Walk up from the Python app directory looking for the virtual environment | ||
| // Stop at the AppHost's parent directory to avoid picking up unrelated venvs | ||
| var appHostParentDirectory = Path.GetDirectoryName(builder.AppHostDirectory); | ||
| var currentDirectory = appDirectoryFullPath; | ||
|
|
||
| while (currentDirectory != null) | ||
| { | ||
| var venvPath = Path.Combine(currentDirectory, virtualEnvironmentPath); | ||
| if (Directory.Exists(venvPath)) | ||
| { | ||
| return venvPath; | ||
| } | ||
|
|
||
| // Stop if we've reached the AppHost's parent directory | ||
| if (string.Equals(currentDirectory, appHostParentDirectory, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| break; | ||
| } | ||
|
|
||
| // Move up to the parent directory | ||
| var parentDirectory = Path.GetDirectoryName(currentDirectory); | ||
|
|
||
| // Stop if we can't go up anymore or if we've gone beyond the AppHost's parent | ||
| if (parentDirectory == null || parentDirectory == currentDirectory) | ||
| { | ||
| break; | ||
| } | ||
|
|
||
| currentDirectory = parentDirectory; | ||
| } | ||
|
|
||
| // Default: Return app directory path (for cases where the venv will be created later) | ||
| return Path.Combine(appDirectoryFullPath, virtualEnvironmentPath); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Configures a custom virtual environment path for the Python application. | ||
| /// </summary> | ||
|
|
@@ -646,6 +705,11 @@ private static void ThrowIfNullOrContainsIsNullOrEmpty(string[] scriptArgs) | |
| /// Virtual environments allow Python applications to have isolated dependencies separate from | ||
| /// the system Python installation. This is the recommended approach for Python applications. | ||
| /// </para> | ||
| /// <para> | ||
| /// When you explicitly specify a virtual environment path using this method, the path is used verbatim. | ||
| /// The automatic multi-location lookup (checking both app and AppHost directories) only applies when | ||
| /// using the default ".venv" path during initial app creation via AddPythonScript, AddPythonModule, or AddPythonExecutable. | ||
| /// </para> | ||
| /// </remarks> | ||
| /// <example> | ||
| /// Configure a Python app to use a custom virtual environment: | ||
|
|
@@ -660,9 +724,12 @@ public static IResourceBuilder<T> WithVirtualEnvironment<T>( | |
| ArgumentNullException.ThrowIfNull(builder); | ||
| ArgumentException.ThrowIfNullOrEmpty(virtualEnvironmentPath); | ||
|
|
||
| var virtualEnvironment = new VirtualEnvironment(Path.IsPathRooted(virtualEnvironmentPath) | ||
| // Use the provided path verbatim - resolve relative paths against the app working directory | ||
| var resolvedPath = Path.IsPathRooted(virtualEnvironmentPath) | ||
| ? virtualEnvironmentPath | ||
| : Path.GetFullPath(virtualEnvironmentPath, builder.Resource.WorkingDirectory)); | ||
| : Path.GetFullPath(virtualEnvironmentPath, builder.Resource.WorkingDirectory); | ||
|
|
||
| var virtualEnvironment = new VirtualEnvironment(resolvedPath); | ||
|
Comment on lines
+748
to
+753
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For my understanding, the logic here hasn't changed at all. It is just refactored into an intermediate variable. Correct? |
||
|
|
||
| // Get the entrypoint annotation to determine how to update the command | ||
| if (!builder.Resource.TryGetLastAnnotation<PythonEntrypointAnnotation>(out var entrypointAnnotation)) | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@copilot is this the correct way to compare directory paths?
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.
Good catch! Updated to use platform-aware comparison:
StringComparison.OrdinalIgnoreCaseon Windows (case-insensitive file system) andStringComparison.Ordinalon Unix (case-sensitive file system). This ensures correct path matching across platforms. Commit: 26930ab