-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
Windows: validate managed Python before package installation #7763
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
Changes from 2 commits
0864b1f
eefa269
c1c0b5e
597aeeb
80cdb73
79f69a2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1335,8 +1335,15 @@ exit 0 | |
| try { | ||
| $out = & $cmd.Source --version 2>&1 | Out-String | ||
| if ($out -match "Python (3\.1[1-3])\.\d+") { | ||
| if (-not $preferX64) { return @{ Version = $Matches[1]; Path = $cmd.Source; Arch = "" } } | ||
| $candidates += @{ Version = $Matches[1]; Path = $cmd.Source } | ||
| $ver = $Matches[1] | ||
| # PATH entries can be launchers (for example pyenv-win's | ||
| # python.bat shim). Give uv the real CPython executable so | ||
| # the venv does not depend on wrapper re-resolution. | ||
| $resolvedExe = (& $cmd.Source -c "import sys; print(sys.executable)" 2>$null | Out-String).Trim() | ||
| if ($resolvedExe -and (Test-Path -LiteralPath $resolvedExe -PathType Leaf) -and -not (Test-IsCondaPython $resolvedExe)) { | ||
| if (-not $preferX64) { return @{ Version = $ver; Path = $resolvedExe; Arch = "" } } | ||
| $candidates += @{ Version = $ver; Path = $resolvedExe } | ||
| } | ||
| } | ||
| } catch {} | ||
| } | ||
|
|
@@ -1677,6 +1684,23 @@ exit 0 | |
| $script:StudioVenvRollbackTarget = $VenvDir | ||
| $script:StudioVenvRollbackActive = $false | ||
|
|
||
| function Test-VenvPythonReady { | ||
| param([Parameter(Mandatory = $true)][string]$PythonExe) | ||
| if (-not (Test-Path -LiteralPath $PythonExe -PathType Leaf)) { return $false } | ||
|
|
||
| $previousErrorActionPreference = $ErrorActionPreference | ||
| $ErrorActionPreference = "Continue" | ||
| try { | ||
| $global:LASTEXITCODE = -1 | ||
| $null = & $PythonExe -c "import sys; sys.exit(0)" 2>$null | ||
| return ($LASTEXITCODE -eq 0) | ||
| } catch { | ||
| return $false | ||
| } finally { | ||
| $ErrorActionPreference = $previousErrorActionPreference | ||
| } | ||
| } | ||
|
|
||
| function Start-StudioVenvRollback { | ||
| param([Parameter(Mandatory = $true)][string]$ExistingDir) | ||
| $stamp = Get-Date -Format "yyyyMMddHHmmss" | ||
|
|
@@ -1885,6 +1909,14 @@ exit 0 | |
| substep "$VenvDir" | ||
| } | ||
|
|
||
| if (-not (Test-VenvPythonReady -PythonExe $VenvPython)) { | ||
|
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.
When a first install using Useful? React with 👍 / 👎.
Collaborator
Author
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. Fixed in c1c0b5e: the ownership marker is now written before readiness validation, so an installer-created broken venv remains replaceable on rerun. |
||
| Write-Host "[ERROR] The managed Python interpreter is missing or cannot be launched." -ForegroundColor Red | ||
| Write-Host " Managed Python: $VenvPython" -ForegroundColor Yellow | ||
| Write-Host " Selected base Python: $($DetectedPython.Path)" -ForegroundColor Yellow | ||
| Write-Host " Restore or reinstall the selected base Python, then re-run install.ps1." -ForegroundColor Yellow | ||
|
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.
When the CWD-relative environment is migrated at Useful? React with 👍 / 👎.
Collaborator
Author
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. Fixed in c1c0b5e: failure diagnostics now read the recorded home from pyvenv.cfg instead of labeling the newly detected interpreter as the migrated venv base. Python documents this field at https://docs.python.org/3/library/venv.html#creating-virtual-environments |
||
| return (Exit-InstallFailure "Managed Python is unavailable at $VenvPython (selected base: $($DetectedPython.Path))") | ||
| } | ||
|
|
||
| # Mark the freshly-created venv as Unsloth-owned so a partial install can be | ||
| # repaired by re-running install.ps1; the env-mode deletion guard above | ||
| # accepts this marker as the primary sentinel. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,106 @@ | ||
| # SPDX-License-Identifier: AGPL-3.0-only | ||
| # Copyright 2026-present the Unsloth AI Inc. team. All rights reserved. | ||
|
|
||
| """Focused contracts for Windows Python wrapper and venv validation.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import os | ||
| import re | ||
| import shutil | ||
| import subprocess | ||
| import sys | ||
| from pathlib import Path | ||
|
|
||
| import pytest | ||
|
|
||
|
|
||
| REPO_ROOT = Path(__file__).resolve().parents[2] | ||
| INSTALL_PS1 = REPO_ROOT / "install.ps1" | ||
| POWERSHELLS = [shell for shell in ("pwsh", "powershell") if shutil.which(shell)] | ||
|
|
||
|
|
||
| def _extract(pattern: str, source: str) -> str: | ||
| match = re.search(pattern, source, flags = re.DOTALL) | ||
| assert match is not None, f"install.ps1 block not found: {pattern}" | ||
| return match.group(0) | ||
|
|
||
|
|
||
| def _run_powershell(shell: str, script: str, env: dict[str, str]) -> str: | ||
| result = subprocess.run( | ||
| [shell, "-NoProfile", "-NonInteractive", "-Command", script], | ||
| check = True, | ||
| capture_output = True, | ||
| text = True, | ||
| env = env, | ||
| timeout = 30, | ||
| ) | ||
| return result.stdout.strip() | ||
|
|
||
|
|
||
| @pytest.mark.skipif(not POWERSHELLS, reason = "PowerShell is unavailable") | ||
| @pytest.mark.parametrize("shell", POWERSHELLS) | ||
| def test_path_python_wrapper_resolves_to_real_executable(tmp_path: Path, shell: str): | ||
| source = INSTALL_PS1.read_text(encoding = "utf-8") | ||
| finder = _extract(r" function Find-CompatiblePython \{.*?\n \}\n", source) | ||
| wrapper = tmp_path / "python.bat" | ||
| wrapper.write_text(f'@"{sys.executable}" %*\n', encoding = "utf-8") | ||
|
|
||
| script = f""" | ||
| $ErrorActionPreference = "Stop" | ||
| $PythonVersion = "3.13" | ||
| $script:CondaSkipPattern = '(?i)(conda|miniconda|anaconda)' | ||
| function Get-HostMachineArch {{ return "x86_64" }} | ||
| function Test-IsCondaPython {{ param([string]$Exe) return $false }} | ||
| function Get-PythonPlatformTag {{ param([string]$Exe) return "win-amd64" }} | ||
| function Get-Command {{ | ||
| param([Parameter(Position = 0)][string]$Name, | ||
| [Parameter(ValueFromRemainingArguments = $true)]$Rest) | ||
| if ($Name -eq "python") {{ | ||
| return @([pscustomobject]@{{ Source = $env:TEST_PYTHON_WRAPPER }}) | ||
| }} | ||
| return @() | ||
| }} | ||
| {finder} | ||
| $found = Find-CompatiblePython | ||
| Write-Output $found.Path | ||
| """ | ||
| env = os.environ.copy() | ||
| env["TEST_PYTHON_WRAPPER"] = str(wrapper) | ||
| assert Path(_run_powershell(shell, script, env)).resolve() == Path(sys.executable).resolve() | ||
|
|
||
|
|
||
| @pytest.mark.skipif(not POWERSHELLS, reason = "PowerShell is unavailable") | ||
| @pytest.mark.parametrize("shell", POWERSHELLS) | ||
| @pytest.mark.parametrize("case", ["missing", "unlaunchable", "working"]) | ||
| def test_managed_python_readiness_probe(tmp_path: Path, shell: str, case: str): | ||
| source = INSTALL_PS1.read_text(encoding = "utf-8") | ||
| readiness = _extract(r" function Test-VenvPythonReady \{.*?\n \}\n", source) | ||
| python_exe = tmp_path / "broken-python.cmd" | ||
| expected = "False" | ||
| if case == "unlaunchable": | ||
| python_exe.write_text("@exit /b 17\n", encoding = "utf-8") | ||
| elif case == "working": | ||
| python_exe = Path(sys.executable) | ||
| expected = "True" | ||
|
|
||
| script = f""" | ||
| $ErrorActionPreference = "Stop" | ||
| {readiness} | ||
| Write-Output (Test-VenvPythonReady -PythonExe $env:TEST_MANAGED_PYTHON) | ||
| """ | ||
| env = os.environ.copy() | ||
| env["TEST_MANAGED_PYTHON"] = str(python_exe) | ||
| assert _run_powershell(shell, script, env) == expected | ||
|
|
||
|
|
||
| def test_readiness_gate_precedes_installs_and_names_both_interpreters(): | ||
| source = INSTALL_PS1.read_text(encoding = "utf-8") | ||
| gate = source.index("if (-not (Test-VenvPythonReady -PythonExe $VenvPython))") | ||
| first_uv_pip = source.index("uv pip install --python $VenvPython") | ||
| gpu_detection = source.index("function Invoke-AmdSmiNoElevate") | ||
|
|
||
| assert gate < gpu_detection < first_uv_pip | ||
| assert 'Write-Host " Managed Python: $VenvPython"' in source | ||
| assert 'Write-Host " Selected base Python: $($DetectedPython.Path)"' in source | ||
| assert 'return (Exit-InstallFailure "Managed Python is unavailable' in source |
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.
When a supported PATH Python has a
.pthhook orsitecustomize.pythat writes a startup banner to stdout,--versionstill matches but this-cinvocation returns the banner and executable path together.Out-String.Trim()therefore produces a multi-line value that cannot passTest-Path, so the newly added resolution discards a working interpreter and may repeatedly reinstall Python only to reject it again. Run this probe without site initialization (for example with-S) or extract a uniquely marked path rather than treating all stdout as the filename.Useful? React with 👍 / 👎.
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.
Fixed in c1c0b5e: both executable-path probes now use -S, and the regression fixture emits a startup banner. Python documents -S at https://docs.python.org/3/using/cmdline.html#cmdoption-S