- 
                Notifications
    
You must be signed in to change notification settings  - Fork 715
 
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
Conversation
Co-authored-by: davidfowl <[email protected]>
Co-authored-by: davidfowl <[email protected]>
…aths verbatim Co-authored-by: davidfowl <[email protected]>
| 
           @copilot Fix the duplicate logic in tests around asserting the path to python with a helper: if (OperatingSystem.IsWindows()) Also there's a TempDirectory helper in the project that can be used here.  | 
    
…lify directory naming Co-authored-by: davidfowl <[email protected]>
| 
          
 🚀 Dogfood this PR with: 
 curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 12616Or 
 iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 12616" | 
    
| 
           @copilot After some research this seems to be a reasonable pattern: 💡 Design takeaway For your ResolveDefaultVirtualEnvironmentPath helper, a good pragmatic default is: 
  | 
    
Co-authored-by: davidfowl <[email protected]>
          
 Implemented the recommended priority pattern. The helper now checks: 
 Using   | 
    
| 
           @copilot remove the VIRTUAL_ENV from configuration (if set and exists) check as it may yield unexpected results when you   | 
    
Co-authored-by: davidfowl <[email protected]>
          
 Removed the VIRTUAL_ENV configuration check. The lookup now uses a simpler three-tier system: 
 Commit: a78085f  | 
    
| 
           @copilot Add a comment to the code that shows what (if app is nearby and exists) means.  | 
    
…nt checks Co-authored-by: davidfowl <[email protected]>
          
 Addressed the feedback: 
 Commit: c866034  | 
    
| 
           @copilot > Change logic to only look up to the AppHost's direct parent, not siblings We want to start at the python app directory and walk up parents but not go outside of the apphost's parent folder. Scenarios: 
 
 In this case, we dont want to go any higher than the common parent of these 2 folders. 
 app1 should not detect the .vnev in the root folder  | 
    
Co-authored-by: davidfowl <[email protected]>
          
 Implemented the walk-up logic. The code now: 
 This handles all three scenarios: 
 Commit: 7881363  | 
    
| } | ||
| 
               | 
          ||
| // Stop if we've reached the AppHost's parent directory | ||
| if (string.Equals(currentDirectory, appHostParentDirectory, StringComparison.OrdinalIgnoreCase)) | 
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.OrdinalIgnoreCase on Windows (case-insensitive file system) and StringComparison.Ordinal on Unix (case-sensitive file system). This ensures correct path matching across platforms. Commit: 26930ab
Co-authored-by: eerhardt <[email protected]>
| 
           @copilot - this should also handle the case where the appDirectory isn't parented under the appHostParentDirectory. For example: When that happens, it should only look in the appDirectory.  | 
    
Co-authored-by: eerhardt <[email protected]>
          
 ... Added check for this scenario. The code now: 
 This prevents walking up the entire directory structure when the app and AppHost are in separate trees. Commit: bf13f27  | 
    
        
          
                src/Aspire.Hosting.Python/PythonAppResourceBuilderExtensions.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
      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.
Just one more suggestion. Then the changes look good to me.
I didn't test this though, so @davidfowl you might want to give it a whirl.
| 
           /backport to release/13.0  | 
    
| 
           Started backporting to release/13.0: https://github.com/dotnet/aspire/actions/runs/19060887411  | 
    
Implementation Complete: .venv Lookup by Walking Up Parent Directories
Successfully implemented virtual environment lookup that walks up parent directories from the Python app directory, stopping at the AppHost's parent directory. This allows finding shared
.venvin common parent folders while avoiding unrelated venvs.Changes Summary
Refactored Architecture
Before: Multi-location lookup logic was in
WithVirtualEnvironment, affecting all callsAfter: Logic moved to
AddPythonAppCore, only applies to default.venvduring initial creationCore Changes (
PythonAppResourceBuilderExtensions.cs)AddPythonAppCore Enhancement
.venvpathResolveDefaultVirtualEnvironmentPathhelper for smart lookupWithVirtualEnvironmentappDirectoryparameter for resolution (notWorkingDirectory)New Helper Method:
ResolveDefaultVirtualEnvironmentPath.venv.venvor defaults to app directory.venvduringAddPythonScript/Module/ExecutableSimplified
WithVirtualEnvironmentTest Improvements (
AddPythonAppTests.cs)AssertPythonCommandPathhelper to eliminate duplicate OS-specific path assertionsSupported Scenarios
Scenario 1: .venv in parent folder of Python app (happy path)
Scenario 2: .venv in common parent, AppHost in sibling folder
Scenario 3: Avoid unrelated .venv
Scenario 4: App not under AppHost's parent tree
Behavior Matrix
AddPythonScript("name", "dir", "script.py")AddPythonModule("name", "dir", "module")AddPythonExecutable("name", "dir", "exe").WithVirtualEnvironment(".venv").venvin app directory verbatim.WithVirtualEnvironment("custom")customin app directory verbatim.WithVirtualEnvironment("/abs/path")Key Benefits
WithVirtualEnvironment, it's used exactly as providedOriginal prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.