Windows: validate managed Python before package installation - #7763
Conversation
for more information, see https://pre-commit.ci
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eefa26940e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| substep "$VenvDir" | ||
| } | ||
|
|
||
| if (-not (Test-VenvPythonReady -PythonExe $VenvPython)) { |
There was a problem hiding this comment.
When a first install using UNSLOTH_STUDIO_HOME successfully creates Scripts\python.exe but this readiness probe fails, the early exit occurs before .unsloth-studio-owned is written. On the advised rerun, the env-mode guard at install.ps1:1836-1844 sees the executable without any ownership sentinel and refuses to replace it as a potentially unrelated venv, so restoring Python and rerunning cannot recover without manually deleting or moving the directory. Write the ownership marker immediately after successful uv venv creation, or remove the failed installer-created venv before returning.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c1c0b5e: the ownership marker is now written before readiness validation, so an installer-created broken venv remains replaceable on rerun.
| 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.
When the CWD-relative environment is migrated at install.ps1:1892-1896, it retains the base recorded in its own pyvenv.cfg; $DetectedPython is only the independently selected interpreter found earlier and may be a different, healthy installation. If the migrated interpreter fails this probe because its original base was removed, these lines instruct the user to reinstall an unrelated interpreter, which does not repair that venv. Either identify the base from the migrated environment or advise recreating/rerunning without claiming that $DetectedPython.Path is the base that needs restoration.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
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
| $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)) { |
There was a problem hiding this comment.
When a supported PATH Python has a .pth hook or sitecustomize.py that writes a startup banner to stdout, --version still matches but this -c invocation returns the banner and executable path together. Out-String.Trim() therefore produces a multi-line value that cannot pass Test-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.
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
|
@codex review |
1 similar comment
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…nslothai#7763 Get-PythonPlatformTag still probed without -S. Its result is compared with -eq "win-amd64", so a sitecustomize banner reads as "unknown", the x64-over-ARM64 preference is lost and Windows on ARM settles for a native ARM64 interpreter. Test-IsCondaPython gets -S for the same reason. Neither query needs site, and base_prefix and get_platform() are unchanged by -S on 3.11, 3.12 and 3.13. The new test module parametrizes over pwsh and powershell, but cross-platform-parity-ci.yml is the only three-OS job and its paths filter and pytest list are hardcoded, so the 5.1 leg never ran. Added the file to both. Also match the py launcher branch to the PATH branch with -LiteralPath -PathType Leaf, and fix the failure message: the empty base home leaked into the Exit-InstallFailure text, and the ownership marker is written before the gate, so a plain re-run already replaces the environment.
|
Pushed a follow-up commit to this branch. The change itself is right, it just stops one probe short.
End to end on a simulated ARM64 host with an x64 interpreter carrying a site hook, the branch as submitted returned The Windows PowerShell 5.1 leg of the new tests was never running. The module parametrizes over Smaller items in the same commit:
Everything else held up. I replayed the venv lifecycle against both main and this branch over 28 cells (fresh, current layout, legacy Thanks for the fix, the underlying report is real. uv hits the same thing in astral-sh/uv#11508 with One thing I deliberately left out of this PR: neither new probe has a timeout, so a wedged interpreter can hang the installer. |
|
@codex review |
|
pre-commit.ci run |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2 participants
Summary
Resolve Python PATH shims to their underlying CPython executable and verify that Studio's managed virtual-environment interpreter can launch before GPU detection or package installation.
Broken environments now fail with the managed and base Python paths instead of surfacing later as a misleading PyTorch installation error.
Motivation
A Windows virtual environment can retain
Scripts\python.exeafter its base Python installation has been removed, renamed, or damaged. The installer previously checked only whether the managed executable existed, then failed later whenuv piptried to inspect it.Healthy pyenv-win shims work with uv, so the installer should continue supporting them while detecting unusable environments earlier.
Changes
pythonandpython3PATH launchers throughsys.executableand require the resolved target to be a non-Conda executable ininstall.ps1:1335.Test-VenvPythonReadyto check that the managed interpreter exists and launches successfully ininstall.ps1:1684.uv pipcommand, with diagnostics for both interpreter paths, ininstall.ps1:1909..batshim resolution, missing and non-launchable interpreters, valid interpreters, and validation ordering intests/python/test_windows_python_venv_hardening.py:43.How to test
Run on Windows with PowerShell 7 and Windows PowerShell 5.1 available: