fix(windows): fix stale venv deletion and CUDA torch stripping on Win…#4877
fix(windows): fix stale venv deletion and CUDA torch stripping on Win…#4877JyothishArumugam wants to merge 4 commits intounslothai:mainfrom
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2f7701053
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| $baseInstallExit = Invoke-InstallCommand { uv pip install --python $VenvPython --no-deps --upgrade-package unsloth "$PackageName" unsloth-zoo } | ||
| if ($baseInstallExit -eq 0) { | ||
| # Install runtime deps from requirements file if present | ||
| $RuntimeReq = Find-NoTorchRuntimeFile | ||
| if ($RuntimeReq) { |
There was a problem hiding this comment.
Preserve dependency install for custom
--package values
This branch now installs "$PackageName" with --no-deps and then tries to recover dependencies via Find-NoTorchRuntimeFile, which only works when the installed package contains Unsloth’s studio/backend/requirements/no-torch-runtime.txt. For the documented custom-package flow (--package roland-sloth), that file may not exist, so the install can succeed with missing runtime deps; because SKIP_STUDIO_BASE=1 is set later, setup won’t reinstall them. This is a regression from the previous direct install path that let pip/uv resolve the custom package dependencies normally.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I have updated the installation logic to ensuring --no-deps workaround only applies to the default unsloth package. For custom --package installs, the script now uses standard dependency resolution while explicitly ensuring the unsloth CLI is installed to prevent the 'missing CLI' error.
|
@JyothishArumugam yes for number 1. |
|
submit the fix for "Access Denied" separately so it can be tested and merged. |
|
The reason for the second fix (the --no-deps + re-pinning strategy) is to address a specific uv resolver behavior on Windows GPU machines. When installing unsloth and torch+cuXXX in a single pass, uv often 'simplifies' the dependency tree by stripping the +cuXXX suffix, which results in a Torch CPU installation. This causes a regression where setup.ps1 detects the wrong version and triggers an infinite venv-rebuild loop. By installing the package first with --no-deps and then explicitly re-pinning the CUDA version from the specific Torch index, we ensure the GPU 'engine' is correctly installed and the re-installation loop is broken. |
|
@JyothishArumugam read the comment i linked on the issue. Decouple the two fixes . Push the venv fix separately so i can test and merge. Thank you. |
df38778 to
55207be
Compare
|
@rolandtannous Thank you for the response. I want to respectfully clarify why Fix 1 alone Why Fix 1 alone is not enoughFix 1 (replacing So the real question is: why does setup.ps1 always see "torch cpu"? Why Fix 2 is needed — this is a
|
Fixes #4701
Changes
Replace
Remove-Itemwithcmd /c rd /s /qfor venv deletion on Windows.PowerShell's Remove-Item fails with "Access Denied" on .exe files inside
freshly created venvs due to Windows file handle locking, even when no
process is running.
Install unsloth with
--no-depsthen re-pin CUDA torch separately.uv's dependency resolver strips the +cuXXX CUDA suffix from torch when
resolving unsloth's dependencies in the same invocation, causing setup.ps1
to always detect "torch cpu != required cuXXX" and attempt a venv rebuild.
Since --no-deps skips requirements, we explicitly call Find-NoTorchRuntimeFile
to ensure the environment is fully populated before the final CUDA re-pinning.
Tested on