[staging CI] unslothai/unsloth#8575 - #302
Open
danielhanchen wants to merge 49 commits into
Open
Conversation
"Run Unsloth at login" registers the desktop through an HKCU Run value, which cannot carry a working directory, so Windows starts the app in C:\Windows\system32. Every `unsloth` CLI child inherited that folder, and the CLI refuses to run there, so a reboot produced a tray icon and no server (issue #8510). Desktop side: pick the working directory explicitly for every CLI child (backend, both preflight probes, the install check, auth provisioning, update, installer) instead of passing on whatever the launcher gave us. The inherited folder is kept whenever it is usable, so ./models and other cwd-relative defaults resolve exactly where they used to; only a Windows system folder is replaced, with ~/.unsloth. A home that cannot be reached at all now reports working_directory_unavailable rather than looking like a broken install, which stops the pointless automatic repair and gets its own message in the UI. CLI side: move the System32 guard into unsloth_cli/_system_dir_guard.py and run it before the command modules import, since commands.studio resolves STUDIO_HOME at import time. The commands the desktop itself issues take no path from the user, so they move to a safe folder and carry on; everything else keeps the hard error, since relocating a command would silently rebase the relative paths its caller typed. This half fixes anyone whose installed desktop build predates the change, without waiting for a desktop release.
for more information, see https://pre-commit.ci
…t profile from an absent install Two review findings on the working-directory work. WINDIR is an ordinary variable, so pointing it at the user's own profile made windows_roots() treat that profile as a Windows installation: an ordinary project folder underneath it then looked like a system folder, the fallback rejected the home for being "inside the Windows directory", and the backend could not start anywhere on that machine. Candidates are now checked rather than trusted, and a directory only counts if it actually contains System32. With nothing on the machine looking like Windows, the check falls back to SystemRoot or the default, never to the settable value. Same fix on both sides, since the CLI guard reads the same variables. The managed install lives under the user's profile, so a profile that is not mounted yet makes find_unsloth_binary() return None and preflight reported NotInstalled before the working-directory check could run. That is the exact case the check was added for, and it was sending those users to reinstall. Check whether the home is reachable before turning a failed lookup into "not installed", and report it as its own state with no binary path and no repair offered.
for more information, see https://pre-commit.ci
The branch that tells an unreachable profile from an outdated install sat inline in use-tauri-backend.ts, which pulls in React and the Tauri APIs and so cannot be imported from a test. Move the choice into its own module and drive it directly, so a new backend reason cannot silently land in the stale-install bucket again.
Studio resolves UNSLOTH_STUDIO_HOME and the cache overrides with Path.resolve(), which anchors a relative value to the working directory. So `UNSLOTH_STUDIO_HOME=.\custom unsloth studio update` from System32 moved first and then resolved the override against the new directory, silently targeting a folder the caller never named. That is the one thing this guard is supposed not to do to caller-supplied paths. Absolutise the relative overrides against the original directory before the move, so they keep meaning what they meant. A ~ value is left alone, since expanduser does not consult the working directory, and an environment that cannot be pinned is one we refuse to move underneath. Covers the Studio home pair plus the cache and llama.cpp overrides, which resolve the same way: UNSLOTH_LLAMA_CPP_PATH, UNSLOTH_COMPILE_LOCATION, HF_HOME, HF_HUB_CACHE, HUGGINGFACE_HUB_CACHE, HF_XET_CACHE.
for more information, see https://pre-commit.ci
…e profile - pin_relative_overrides() missed the Studio documents, projects and sandbox roots and the sd.cpp/whisper.cpp/llama.cpp engine paths, so a relative value would have been retargeted by the move, and the remaining single-path cache overrides are pinned for the same reason - an owned or ownerless-spawned backend that is stale no longer offers auto repair when the managed profile is unreachable: the repair runs through the same profile and stops a backend that still answers
…ecks - the desktop moved a CLI child out of a system folder without rewriting the relative path overrides it inherited, so the same install placed state in a different folder depending on whether the desktop or the CLI guard did the move; both layers now anchor those values to the directory being left, with a test that fails if the two lists drift apart - pin the diffusion cache dirs, OLLAMA_MODELS, DG_VISUAL_BIN and UNSLOTH_DG_SHIM, which are resolved against the working directory as well - home_dir_available() accepted a home the working directory resolver then rejected, so a SYSTEM account was offered an install that cannot start; both now go through usable_home_dir()
…pair Withholding auto repair was not enough: the result still carried the backend's own reason, so the frontend advised running the update, which needs the same profile the probe could not reach. Both stale paths now report working_directory_unavailable, and the backend's reason goes to the log.
A value such as HF_HOME=D:cache names the current directory on drive D, so joining it to the folder being left hands it straight back and the move retargets it. Both layers now ask the OS to resolve it first, GetFullPathNameW through ntpath.abspath and std::path::absolute, which is what tracks each drive's own directory. The CLI guard refuses to move at all if that resolution fails, rather than moving and silently changing where the value points.
for more information, see https://pre-commit.ci
…amily SD_CLI_PATH and SD_SERVER_PATH are the highest-priority binary locations for the sd.cpp engine, so a relative value pointed somewhere else after the move. Added alongside the llama.cpp and whisper.cpp equivalents, together with the HF and XDG names that belong to the same families as the ones already pinned.
…rofile - CUDA_PATH, HIP_PATH, HIP_PATH_57 and ROCM_PATH are joined with bin/ for DLL discovery, so a relative value pointed elsewhere after the move - a profile that has not mounted yet still has a writable parent, so makedirs built an empty second profile that would shadow the real one when it arrives; the guard now requires the home to exist, as the Rust resolver does - the public profile is refused whichever variable named it: allow_public only kept PUBLIC out of the candidate list, so a USERPROFILE or ~ that resolves there still put one account's state in a folder shared by every account
for more information, see https://pre-commit.ci
Both halves run repeatedly over state an earlier run touched: the desktop resolves the directory on every spawn, the guard runs in every CLI process, and a child's environment reaches its own grandchildren. Two tests pin the fixpoint, one for a command configured twice and one for a value that has already been anchored.
…iled - "studio update" is one of the commands that relocates, and it resolves a relative STUDIO_LOCAL_REPO against the working directory, so the move retargeted the checkout the user meant to install from - a profile can drop between the working directory check and the probes that follow, which reported cli_unusable or desktop_capability_probe_failed and offered a repair needing that same profile; both now ask again and report the profile when it is what went missing
…etarget Five independent reviews of the branch agreed on the same three gaps: - the marker is inherited by the backend and everything below it, so treating it as authorisation for any "studio" subcommand let a marked descendant relocate "studio run --model .\local.gguf", rebasing a path the caller chose. It now authorises only invocations that carry no path, which is what the desktop actually runs - a value like "\cache" is rooted to the drive of the current directory, not to a drive, so a profile on another drive moved it. Root-relative values now go through the OS with the drive-relative ones, and the extended prefix is matched case-insensitively, since the object manager accepts \\?\unc\ too - four more single-path overrides are pinned, and two path lists are anchored entry by entry, so one relative entry cannot change what a whole search or allowlist means The desktop also relocated a child out of any folder under the Windows tree, while the CLI only ever refused System32 and SysWOW64. It now uses the same definition, so a child running from somewhere like C:\Windows\Temp keeps the directory it had.
for more information, see https://pre-commit.ci
A platform-isolation audit found this PR's working-directory-unavailable
message asserting a Windows cause on every platform:
Unsloth cannot reach your user folder, so it has nowhere to run from.
This usually means a network or roaming profile is not available yet.
The Rust half of this PR is deliberately not #[cfg]-gated; isolation comes
from windows_roots() returning empty off Windows, which does hold for every
env-var rewrite. But home_dir_available() is called ungated from
preflight/managed.rs, so ManagedProbe::Unavailable{working_directory_unavailable}
is reachable on Linux and macOS, where the same symptom means an unmounted
home or a permissions problem. A roaming profile is a Windows concept and
naming it there sends the reader looking for something that is not present.
The symptom and the remedy are unchanged everywhere. Only the CAUSE moves
behind a platform check, so it is offered where it applies instead of
asserted everywhere.
Test covers both directions: Windows still gets the sentence, Linux and
macOS do not, and both keep the symptom and "Reconnect and try again".
studio/frontend/tests/backend-preflight-message.test.ts 4 passed
The Windows cross-platform CI caught this: ntpath.isabs answered True for a leading separator until Python 3.13 and False after it, so on 3.12 a root-relative "\cache" was treated as fully qualified and left to move with the working directory, which is the retargeting the pinning exists to prevent. Both halves now spell the test out, a drive plus a separator or a UNC share, so the same value names the same folder on every interpreter and on the Linux runner that tests the Rust half.
The guard runs on every CLI invocation, "unsloth studio update" included, but neither update workflow listed it, so the two suites that install Unsloth and then update it twice never ran for this change.
Three of the pinned names are read as something other than a path by the code that consumes them, so anchoring one changed its meaning instead of preserving it. MLX_HOSTFILE holds either a filename or the host list itself, huggingface_hub expands %VAR% in HF_HOME and its neighbours after the guard has run, and the pre-quant allowlist ignores a bare on/off token precisely so that there is no allow-all mode: anchoring the "1" would have turned it into a real allowlisted directory. All three are now left alone, in the CLI guard and in the desktop twin. Also in this pass: - studio --frontend=.\dist carries a path inside the option token, which the marker gate missed because it only looked for a bare argument after the subcommand. A marked child running it is refused rather than moved. - \Windows\System32\config\systemprofile names SYSTEM's profile without a drive, so it compared equal to no drive-qualified Windows root and was accepted as a home. The drive-less spelling of each Windows root is compared too. - The child is only told where to run when that differs from where the parent already is. Reopening an inherited directory by name can fail if an ancestor turned unreadable after launch, where inheriting the open handle would have worked, so the no-move case stays exactly as it was. - An override the OS declines to resolve now refuses the whole move on the desktop side rather than being dropped, which is what the CLI guard already did: moving with that value still relative would retarget it. - The tests that read the ambient environment take the crate-wide env lock; XDG_DATA_HOME is one of the pinned names now, and the test that swaps it was documented as the only reader.
Three follow-ups on the pinning: The exemptions for inline JSON, %VAR% / $VAR and bare on/off tokens now name the variables whose reader proves them, rather than applying to every pinned name. A directory really called [llama] or %data% is legal on Windows and UNSLOTH_LLAMA_CPP_PATH is read as exactly that, so the blanket form left it unpinned and the move retargeted it. PYTHONPATH joins the anchored search lists, and the guard anchors the relative entries this interpreter is already carrying in sys.path: those are resolved on every import, not at startup, so a move would let whatever sits in ~/.unsloth shadow a managed import. sys.path is best effort, unlike the environment: an import root is not worth refusing the move over, and refusing is how the login start broke to begin with. PATH is deliberately left out, being mostly other people's absolute entries. Preflight asks for the whole managed context, not just the directory. An override the OS declines to resolve fails the same spawn, and a probe that returned false for it was read as a broken CLI, which started an automatic repair that needed the same context and failed the same way.
for more information, see https://pre-commit.ci
Three more from the review: huggingface_hub expands %VAR% in HF_HOME and its neighbours, but Studio's own hf_cache_settings._canonical() does not, so leaving such a value as written sent the two readers to different folders once the process moved. The guard now expands those names before deciding, writes the expanded form back when expanding is what made it name a folder, and anchors it when it does not. A name the machine does not set stays as written, which is what expandvars does too. The desktop got the same, with a small %VAR% expander since std has none. The updater removes PYTHONPATH on Windows, and pinning an inherited relative one put it straight back. The removal now happens after the managed context is applied: -I only covers the first interpreter, and the update's PowerShell and setup descendants start further Python processes that do not clear it. The Typer callback classified sys.argv even when the app was called as a library, so a host whose own argv looked like a desktop command could move the process out from under the caller's relative paths. It reads the invocation Click is running instead, and where Click keeps the tail to itself the invocation is refused rather than relocated. The console script is unaffected: it is classified at import, from the real argv.
for more information, see https://pre-commit.ci
Four more from the review:
os.path.expandvars takes %NAME%, $NAME and ${NAME} on a Windows path; the
desktop expander only took the first, so a value written $LOCALAPPDATA\hf
was anchored under System32 by the desktop while the CLI guard would have
kept it. All three forms now, with a test that walks each one.
The Typer group records the tokens it was handed. Click keeps the tail on
the child context, so the callback saw only the subcommand name and
refused a library `studio --api-only` that it should have relocated.
Reading the recorded list classifies the invocation in full, whether it
arrives through app(args = [...]) or a runner.
HF_TOKEN_PATH is pinned like the caches beside it: huggingface_hub reads
the credential file from there, and a relative value would follow the
child and lose access to gated repos while everything still looked
healthy.
PYTHONPATH has two spellings that follow the process rather than the
caller: an empty component means the working directory itself, and `~` is
never expanded there, so Python reads ~\plugins as an ordinary relative
folder. Both are anchored to the directory being left, in the environment,
in sys.path and in the desktop twin.
for more information, see https://pre-commit.ci
Two findings from a fresh review round. sys.path holds other people's strings as well as folders. setuptools registers a relative sentinel for an editable namespace install and its own path hook accepts that sentinel by exact equality, and a relative .zip keeps the spelling its already imported packages hold in their loaders; rewriting either breaks the import the pinning was meant to protect. Both were reproduced: an editable namespace stopped importing after the rewrite, and a package loaded from a relative archive lost its submodules. Only an entry that is a directory right now is anchored, plus the empty entry, which is the working directory by definition. A value that cannot be pinned now says so. Windows caps an environment variable at 32767 characters, so a long enough list can stop fitting once every entry names its folder in full, and a drive with no current directory of its own cannot be resolved either. Both were reported as "check that the user profile for this account exists and is writable", which sends the reader looking in the wrong place.
for more information, see https://pre-commit.ci
The desktop expander read $NAME as far as the first non-word character, where ntpath counts a hyphen as part of the name. With CACHE and CACHE-ROOT both set, HF_HOME=$CACHE-ROOT\hf resolved to C:\right\hf in the CLI and C:\wrong-ROOT\hf in the desktop: one install, two folders. It now mirrors ntpath's pattern outright, including the single-quoted run that is copied through unexpanded, %% and $$ standing for one character, and anything unterminated staying as written. The guard's own tests call os.path.expandvars against the environment they describe rather than a stand-in that only knew %NAME%. Preflight tells its two context failures apart. A probe that could not be configured returns that instead of false, so a context which recovers between the failed apply and the check afterwards can no longer make an untested CLI look broken and start a repair. And an override the OS cannot resolve is reported as path_setting_unresolvable rather than as an unreachable user folder: the profile is fine, the value is not, and the frontend now says so instead of advising a reconnect.
Five more from the review. The backend spawn unwrapped the managed context, so a drive that went between the preflight check and the spawn took the desktop down with it. It reports through the same diagnostics path as every other start failure now. UNSLOTH_STUDIO_HOME and STUDIO_HOME are removed for every managed child, because Tauri uses the legacy root whatever the environment says. Trying to resolve them could only invent a failure for a value the child never sees, so they are skipped when pinning and removed by the context helper itself rather than only at the call sites. A child was moved with its relative overrides untouched when the original directory could not be read at all. That silently retargets each of them at the new directory, so it is refused unless there is nothing relative left to preserve. The Typer callback no longer relocates. It runs after the command modules are imported, and commands.studio resolves STUDIO_HOME at import time, so a host that reached that point cannot be moved without leaving the cached root behind. The console script is unaffected: it is checked before any command module loads. The environment and sys.path are put back if the move does not happen. Inside a host process both belong to the caller, and a chdir that fails after pinning left them rewritten as though it had succeeded.
Two more from the review round. A `~` value was left alone on the grounds that expanduser does not consult the working directory. That is true of expanduser and false of the readers: llama_cpp.py hands UNSLOTH_LLAMA_CPP_PATH and LLAMA_SERVER_PATH straight to Path(), as the whisper and stable diffusion overrides do with theirs, so `~\llama.cpp` was an ordinary relative path for them and followed the child to the new directory. It is written out now, on both sides, which is what the caller meant and what the readers that do call expanduser would have computed for themselves. That also covers the PYTHONPATH case more honestly than anchoring it did: `~` names the profile rather than a folder called "~" beside the old directory. UV_CACHE_DIR joins the pinned names. uv reads it as written, Studio treats a non-blank value as authoritative, and `unsloth studio update` runs uv through setup.ps1, so a relative one moved the install cache. The guard's tests and the simulations now call the real expanduser against the environment each case describes; the stand-in returned the profile for every input, which is what hid this.
for more information, see https://pre-commit.ci
Three more from the review. The lost-directory check read a whole path list as one value, so PYTHONPATH=C:\vendor;plugins looked qualified because of the drive at the front while `plugins` still depended on the directory that was gone. Each entry is judged on its own now, with the empty PYTHONPATH component counting as the directory itself. MLX_IBV_DEVICES is pinned beside MLX_HOSTFILE and exempted from anchoring the same way. `_json_rank_count_from_env` reads the two identically: either the device list inline as JSON, or a filename. STUDIO_LOCAL_REPO is read by the update and installer path alone (install_python_stack.py), so a stale drive-relative value was failing preflight, backend startup, capability probes and auth provisioning over a setting none of them look at. It stays pinned for the update child and is dropped for the rest.
…solute Two more from the review. The lost-directory check judged every value by Windows rules, and it is the one part of the pinning that runs off Windows: a desktop on Linux or macOS whose launch directory was deleted read XDG_CACHE_HOME=/var/cache as relative and failed every managed spawn with path_setting_unresolvable over a value that depends on no directory at all. The native spelling counts there too now. sys.path entries are anchored when they name something on disk rather than only a directory. Skipping every non-directory kept setuptools' editable sentinel safe, which is what it was for, but it also left a relative importable archive behind: os.environ is not sys.path, so after the move the next import from that archive looked for it beside the new working directory. A folder or an archive is anchored; a string that names nothing on disk is still left exactly as written.
for more information, see https://pre-commit.ci
The lost-directory fallback is the one part of the pinning that also runs off Windows, and absoluteness there was already fixed. The separator was not: the list was still split on ';', so "/opt/vendor:plugins" stayed a single entry, started with '/', and passed as absolute. The relative entry behind it was never seen, and the child got it resolved against the wrong directory. Deciding both from a parameter rather than cfg! is what makes this testable. The existing entry-by-entry test feeds a Windows list and runs on every platform, so a compile-time cfg would either break it off Windows or leave the POSIX path untested. Now each test names the rules it means, and the POSIX one also pins that a Windows-shaped value stays Windows-judged. Supersedes the narrower POSIX test added alongside the absoluteness fix: same fixture, and the new one additionally covers the separator and the Windows-stays-Windows direction.
When the launch directory is gone, the pin scan judged each override exactly as written, without the expansion and non-path exemptions the moving path applies. HF_HOME=%LOCALAPPDATA%\hf, an inline JSON MLX_HOSTFILE and UNSLOTH_ALLOW_LOCAL_PREQUANT_PATH=1 were all read as relative, so preflight and every managed spawn failed with path_setting_unresolvable over values that never depended on a directory. The same normalization now runs before the check.
Same intent, fewer lines: the reasons that are not obvious from the code stay, the restatements of it go. No code or behaviour change.
llama-server takes LLAMA_ARG_MODEL, LLAMA_ARG_MMPROJ and the two draft-model spellings straight from the environment and resolves a relative one against its own working directory, and Studio reads them back when it sizes a launch (llama_cpp.py). A managed child moved out of System32 with one of those still relative would look for the model or the projector beneath ~/.unsloth. Both mirrored lists now carry them. The URL and HF-repo spellings stay out: they name no local file.
Two defects found by a fresh round of idempotency review. A value whose expansion needs a second pass, LOCALAPPDATA holding %USERPROFILE% and HF_HOME holding %LOCALAPPDATA%, was anchored while still half expanded, so the reader that expands saw a folder name with a second drive in the middle of it. Expansion now runs to a fixpoint, and a value that never settles, HF_HOME holding itself, is left exactly as written rather than anchored or grown. Both layers, with a test each. The console script passes no list, so the guard pinned the real sys.path with no snapshot to put back: a chdir that then failed left the process carrying import roots it never agreed to, while the environment was restored. The snapshot is now taken from the list actually being pinned. The existing rollback test passed its own list and so missed the one path production takes; the harness can now leave it out.
…its profile Two success-to-failure changes found by the same review round. A process whose working directory has been deleted or unmounted can still spawn children from the handle it holds. Pinning cannot anchor anything to a directory it cannot name, and the answer was to refuse, which took the capability probe, the backend start, the auth provision and the update down over a setting the command may never read. The pins still report what a move would lose; that report now decides whether the child moves rather than whether it runs, so it stays where it is, exactly as it did before this file learned about working directories. The installer shared the managed resolver, which requires the home to exist so that a child never builds an empty folder shadowing a roaming profile that has not mounted yet. install.ps1 and install.sh detect a SYSTEM profile themselves, and before they shared the resolver a home that did not exist was simply created along with ~/.unsloth, so they get that policy back.
for more information, see https://pre-commit.ci
![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: 8399f9bdf0
ℹ️ 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".
| run: | | ||
| ./install.sh --local --no-torch || pip install -e . | ||
| nohup unsloth studio -H 127.0.0.1 -p 8888 > studio.log 2>&1 & | ||
| for i in $(seq 1 60); do curl -sf http://127.0.0.1:8888/healthz && break; sleep 5; done |
There was a problem hiding this comment.
If Studio fails to start or /healthz never returns 200, the final sleep 5 still exits successfully after all 60 iterations, so this step passes. The following _smoke_ui module starts its own fake server on port 18902 and never contacts this Studio instance, leaving the workflow green without having launched a usable Studio; add an explicit failure after the polling loop.
Useful? React with 👍 / 👎.
| nohup unsloth studio -H 127.0.0.1 -p 8888 > studio.log 2>&1 & | ||
| for i in $(seq 1 60); do curl -sf http://127.0.0.1:8888/healthz && break; sleep 5; done | ||
| - name: Playwright smoke (studio_test_kit) | ||
| run: PYTHONPATH=.github/scripts python -m studio_test_kit._smoke_ui || true |
There was a problem hiding this comment.
The unconditional || true converts every assertion, browser-launch, video-finalization, and import failure from _smoke_ui into a successful workflow step. Consequently this new smoke job cannot report regressions in the test kit it is intended to validate; allow the Python process's exit status to fail the step while retaining the if: always() artifact upload.
Useful? React with 👍 / 👎.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Disposable CI run for unslothai/unsloth#8575. Do not merge; closed after CI.