16 commits

Author	SHA1	Message	Date
Daniel Han	50b9deacb0	Trim comments in tests (#13844) ... Comment and docstring changes only, in test files, split out of #12968 so the test-only part can merge first.	2026-10-15 05:30:27 -07:00
Daniel Han	e2104761c5	Windows installer: always build Studio on a uv-managed Python, never one already installed (#13399) ... * Windows installer: always build Studio on a uv-managed Python, never one already installed A fresh install on a machine with Python 3.12 took that interpreter, so the cu130 torch 2.13 route (cp313 only) closed and the box got torch 2.11 with no prebuilt flash-attn / causal-conv1d / mamba-ssm. uv only provided Python when nothing 3.11-3.13 was installed, and Windows on ARM never used it. The environment is now always built on a uv-managed CPython, requested by minor and architecture (cpython-3.13-windows-x86_64-none, or aarch64 for the native Windows on ARM CUDA stack and UNSLOTH_ALLOW_ARM64_PYTHON), resolved after uv is installed and before the existing environment is touched. A reinstall keeps the environment's minor so its cached wheels are reused; UNSLOTH_TORCH_UPGRADE=1 moves it to 3.13. When uv cannot provide one, an earlier uv-installed runtime is used, else the installer stops with the mirror / pre-install fix; there is no system fallback. --python X.Y[.Z] now works as the header always said, and an explicit request is exact. Legacy layouts on a non-uv base are rebuilt instead of moved in. * Tests: drop the system-Python bootstrap cases; winget now only installs uv * Legacy environments are reused only on the Python this install resolved A legacy .venv / ~/unsloth_studio on a uv-managed base was moved in and kept even when --python, UNSLOTH_PYTHON or UNSLOTH_TORCH_UPGRADE asked for another version. It is now reused only when its pyvenv.cfg version matches the resolved interpreter's minor (its exact patch when one was requested). * Trim comments * Windows installer: legacy layouts keep their minor and arch, UTF-8 pyvenv.cfg, older cached patch past a skip - A legacy .venv / ~/unsloth_studio reinstall keeps its Python minor, as the current layout does. - A legacy environment is reused only on a uv build of the selected architecture. - pyvenv.cfg is read as UTF-8 (uv writes it without a BOM; Windows PowerShell 5.1 read it as ANSI and rejected a non-ASCII store path). - Past a skipped patch the resolver tries the next one, then an older one, so an offline machine with 3.13.7 cached still installs. * Trim comments	2026-10-12 06:27:51 -07:00
Michael Han	be95e417b9	Use Unsloth branding in remaining user-facing text (#11254)	2026-09-18 01:58:29 -07:00
Daniel Han	d728b5fc4e	installer: the adaptive uv cache selection was unreachable on every writable install (#10626) ... * installer: the adaptive uv cache selection was unreachable on every writable install Two halves of `install.sh`, each correct to read, wrong in the order they run. The block below `# Keep uv's cache on the same filesystem as the venv it fills` exports `UV_CACHE_DIR="$STUDIO_HOME/cache/uv"` whenever the caller left it unset. It has to run there and not later: uv aborts on a cache it cannot create, and several install steps come before `_configure_uv_cache`. `_configure_uv_cache` runs much further down, and its first `case` matched any non-blank `${UV_CACHE_DIR-}`, set `_UV_CACHE_MODE=custom` and returned. It cannot tell a caller's choice from the installer's own default, because from inside the variable they are the same string. So on every writable install the two together took the `custom` branch: * the `env -u UV_CACHE_DIR uv cache dir` probe never ran; * `shared` was never selected, so a machine with a populated uv cache re-downloaded multi-gigabyte Torch and CUDA wheels into a second one; * `_prepare_studio_uv_cache_for_launch`, which opens `[ "${_UV_CACHE_MODE:-}" = shared ] || return 0`, was dead code; * and `--isolated-uv-cache` never reached its own branch either. The directory it produced was right by coincidence -- `custom` and `isolated` both leave `$STUDIO_HOME/cache/uv` -- but the mode was wrong, so the "already-cached packages may download again" warning the flag exists to print was never printed. Fixed by recording whether the value is ours: `_UV_CACHE_DIR_INSTALLER_DEFAULT`, set true only where the installer assigns the default and cleared again on the unwritable-cache fallback. A caller's `UV_CACHE_DIR` keeps its priority; the installer's own default falls through to the selection it was always meant to reach. `tests/sh/test_uv_cache_adaptive_selection.sh` runs the two halves in the order the installer runs them, which is the only way this is visible -- neither half is wrong on its own. Anchored on text that predates the fix, so it exercises the old code rather than failing to find it: against unmodified main it reports 5 failures, and 13/13 with the fix. Nothing else moved. `test_uv_cache_colocation.sh` 17/17, `test_install_uv_cache_root.sh` all passed, `test_uv_pinned_release.sh` 72/72, `test_uv_cache_discipline.py` and `test_ci_shell_suite_coverage.py` 39/39. `sh -n` and `bash -n` clean. * Probe the shared uv cache for writes and absolutize it before scanning * Probe every populated uv cache bucket, not just the cache root * Keep a warm bucket warm when the scan pipeline reports SIGPIPE * Treat a cache bucket we cannot enter as one we cannot write * Probe every existing cache bucket and expand the scan globs under set -f * Cut the rationale comments this change added down to the load-bearing lines * Refuse a cache bucket that is not a directory * Keep CACHEDIR.TAG from failing the bucket probe, and stop the WSL reroute forwarding our own cache path * Let a rerun keep the warm cache it already recorded * Probe only the cache root and uv's own buckets * Match uv bucket names exactly, and fail the probe when it cannot be cleaned up * Apply the bucket-name rule to the warmth scan too, and stand down under UV_NO_CACHE * Read UV_NO_CACHE the way uv reads it * Require a uv artifact kind before counting a bucket as warm * Select the uv cache by the same rules on Windows, and stop the POSIX side losing a cache it already has Three things the selection could not see, all found by running it rather than reading it. ## install.ps1 never had the rules install.sh grew here install.ps1 sets UV_CACHE_DIR in one place, inside the selector, so it never had the ordering defect this branch fixes and its adaptive selection has always run. What it does not have is everything the selection learned since. Measured by running its own selector: a rerun abandoned a warm Studio cache the moment one unrelated wheel made uv's default read as warm, UV_NO_CACHE still probed and recorded a cache uv was not using, `archive-v0.backup` counted as warmth, and a populated-but-unwritable cache was selected, which uv aborts on -- that last one fails the install outright. Two of those are the same bugs this branch fixed on POSIX, and the update path (unsloth_cli/commands/studio.py) already behaved the new way on Windows, so install.ps1 was the odd one out on its own platform. So the marker precedence, the UV_NO_CACHE stand-down, the <kind>-v<N> bucket rule and the create-and-delete write probe are now on both sides, along with the `studio`-not-`shared` verdict when the chosen cache already is the Studio cache. tests/python/test_windows_uv_cache_selection.py runs install.ps1's real selector under pwsh and asserts the behaviour, the way tests/sh/test_uv_cache_adaptive_selection.sh does for the POSIX half. Against install.ps1 as it was: 12 of its cases fail. ## An install from before the marker loses the cache it already has The candidate list was [marker, uv's default]. An install from before b66d2a4c8 has a populated $STUDIO_HOME/cache/uv and nothing recording it, so one unrelated wheel anywhere in uv's default read as warm, the install switched to it, and the Torch and CUDA already on disk were re-downloaded and then orphaned -- while the step line said it was avoiding duplicate downloads. That is the whole installed base from before the marker, and on POSIX the case could not arise before this branch, because the custom branch always fired. A warm Studio cache is self-evidently ours, since nothing else writes there, so it is a candidate when there is no marker. With a marker it is not: a marker naming somewhere else is a decision. Both installers. ## The launch could repoint at a cache uv would abort on shared mode is reachable only when the early block failed its own write probe on $STUDIO_HOME/cache/uv and unset UV_CACHE_DIR, which is what lets the selection run at all. The launch then repointed at that same directory unconditionally, so an install that succeeded handed the autostarted backend a cache uv aborts on. Both installers now probe before repointing and keep the shared cache otherwise: that is the cache this install actually filled. ## Also * The sh marker read was the only one of the three that did not tolerate a BOM or a CR. install.ps1 writes the file with Set-Content -Encoding utf8, which under Windows PowerShell 5.1 means both, and a WSL install shares $STUDIO_HOME with the Windows one. A trailing CR failed [ -d ] and the warm cache was abandoned in silence; a BOM made the value non-absolute, so $PWD was prepended to it. * A trailing separator now comes off in _absolutize_uv_cache_dir and Resolve-StudioUvCachePath. It compared unequal to the Studio cache, so a marker written with one was announced as a shared cache and got the launch repoint the studio branch exists to avoid. * UV_NO_CACHE takes clap's literals, which is what uv binds it to (BoolishValueParser over `y yes t true on 1`). `y` and `t` were missing from all three copies, and missing one leaves us probing and recording a cache uv is not using. studio.py also stopped trimming, for the same reason install.sh never did: uv rejects a padded value rather than reading it as true. * An unreadable first candidate no longer costs us the second. Under ErrorActionPreference = Stop a Test-Path inside an ACL-denied directory throws, and the throw reached the loop's own catch, so a marker naming a cache the user can no longer stat took uv's default down with it. install.sh's `[ -d ] && [ -r ]` simply reads false, which is the behaviour to match. * $scanBlocked is per candidate on the ps1 side. The warmth scan can report blocked and still return true, so a shared flag pinned the NEXT candidate's name into the warning and suppressed the one message that named the cache actually refused. * Neither "cached packages may download again" warning names the directory being fallen back TO. The Studio cache is a candidate itself now, so it can be the one that was refused. ## Test tests/sh/test_uv_cache_adaptive_selection.sh gains the cases above and two arrangements it never used: install.sh is #!/bin/sh with set -e on line 5, and every case ran one shell without errexit. The core verdicts now run under every /bin/sh on the box, with errexit both ways. tests/sh, all 64 files pass test_uv_cache_adaptive_selection.sh 81 pass, against main 18/81 test_windows_uv_cache_selection.py 36 pass, against install.ps1 as it was 24/36 test_windows_python_venv_hardening.py 51 pass test_cross_platform_parity.py 83 pass test_studio_update_uv_cache.py 76 pass test_windows_python_venv_hardening.py's harness had to learn the new helper names: the selector wraps its candidate loop in a catch, so a helper it cannot find does not raise, it quietly turns every case in that file into "studio, could not be inspected". * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Hand PowerShell the path it was given, so the Windows cases assert on Windows Found by running the new suite on real Windows runners rather than only under pwsh on Linux. `json.dumps` produced a double-quoted PowerShell string, and PowerShell does not read `\` in a double-quoted string as an escape. So `C:\Users\runneradmin\...` reached the selector as a path with DOUBLED separators, and every Windows assertion in the file compared a doubled path against a real one: 6 of the cases failed on windows-latest and windows-11-arm, and one of them (`a trailing separator in the marker is the same directory`) failed on the mode rather than the path, because the marker Python wrote held the real path while `$studioCache` held the doubled one, so they compared unequal and the selection read `shared`. It passed on Linux and macOS because a POSIX path has no backslashes to double, which is exactly the shape of bug a cross-platform run exists to catch and a single-platform run cannot. A single-quoted PowerShell string is literal and `''` is its only escape, so the paths arrive as given. The last case in the file is the guard: it hands pwsh a Windows path, a UNC path, a path with a space and a path with a quote, and asserts each comes back byte-identical. * A fallback we cannot write is not a fallback The write probe refuses a whole cache for one bucket uv may never touch, and the early block has already given up on $STUDIO_HOME/cache/uv when it could not write there -- which is the only reason the selection is running at all. So the certain failure and the merely suspect cache can both be on the table at once, and the selection landed on the certain one. Reproduced with real uv 0.10.7, not a stub: a warm shared cache carrying an unused read-only bucket, beside a Studio cache whose root is 0555. Before this change the install selected the Studio cache and `uv pip install` exited 2, unable to create CACHEDIR.TAG. It now keeps the populated cache, and the warning below still names it as the one that failed the probe. The rule is narrow on purpose: only when a warm candidate was actually refused, only when it is not the Studio cache itself, and only when the Studio cache really cannot be created and written, tested the same way everything else here is -- a real create and delete rather than a mode bit. A writable Studio cache still wins over a refused candidate, which the second new case pins. `_uv_cache_root_is_writable` / `Test-StudioUvCacheRootWritable` also replace the inline probe the launch repoint grew in the previous commit, so there is one answer to "can we fill this" per installer rather than two copies drifting apart. test_uv_cache_adaptive_selection.sh 86 pass test_install_uv_cache_root.sh 89 pass test_windows_uv_cache_selection.py 43 pass test_cross_platform_parity.py 83 pass test_windows_python_venv_hardening.py 51 pass test_studio_update_uv_cache.py 76 pass Both extraction harnesses had to learn the new helper name. That is the same trap as last time and it fails loudly in only one of them: test_install_uv_cache_root.sh rebuilds the selector from a named list and a missing name is a `command not found` inside `|| return 0`, so the launch repoint silently stopped happening in three unrelated cases rather than erroring. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Spawn pwsh through the shared runner in the installed-release printer test Repo tests (CPU) went red on this branch with Unhandled exception. System.IO.FileLoadException: The given assembly name was invalid. from tests/studio/install/test_installed_release_backend_line.py, across 40-odd parametrisations at once. That is not the printer. It is the failure tests/_shared/unsloth_pwsh_runner.py was written for, and its module docstring names this exact exception: every -NonInteractive startup reads and rewrites one ~83 KB StartupProfileData file under $XDG_CACHE_HOME, all four xdist workers share one $HOME, and a startup that deserialises a half-written one dies before it reaches the script. Measured there at 7 deaths per 4000 startups on a shared cache dir and 0 on private ones. This file spawned pwsh with a bare subprocess.run, so it never got the private cache or the retry. It is also one of the densest pwsh users in the job, which is why it is where the race surfaced once this branch added another 43 pwsh-spawning cases to the same run. No behaviour of the test changes: run_pwsh is subprocess.run plus a private cache directory per worker and a retry for an interpreter that died without producing a verdict. A pwsh that runs to completion and gives the wrong answer still fails, first time, with its own message. 496 passed, 2 skipped locally. * Reject an empty version in the uv bucket-name rule `##*-v` strips through the LAST `-v`, so a name ending in one leaves an empty suffix behind, and no `*[!0-9]*` matches an empty string. `archive-v1-v` read as a bucket, the write probe covered it, and one read-only directory a user happened to name that way condemned an otherwise usable warm cache, sending a machine that already holds Torch and CUDA back to redownload them. Every other numeric-suffix case in install.sh already spells this `''|*[!0-9]*`, and Test-StudioUvBucketName has rejected it on IsNullOrEmpty since it was written, so this was a real sh/ps1 split rather than a spelling difference. The parity test now asserts both halves of the rule instead of just the function names. * Cut the 4688 compiler window back to the window it was given The no-compiler lane failed on this branch reporting one compiler process. The record it named is timestamped 17:51:57.107, and the step it was scored against did not print its first line until 17:51:58.58, so nothing in install.ps1 can have spawned it. It belongs to the positive control one step earlier, which compiles a type on purpose. That is the case $prior exists to subtract, and it did not: the Security log is written asynchronously, so a record 1.5s old can be absent when $prior samples it and present by the time the measurement reads back. Either way the record's own TimeCreated is below the floor, so the floor is the thing to enforce. The query now asks for a second more than the window at each end and cuts the result back by TimeCreated, which is the instant the claim is actually about. The pad at the far end also removes the opposite risk, of a bound that rounds the wrong way dropping a real compile in the window's last fraction of a second. $prior stays as it was. This narrows what can be counted, and nothing that a compile inside the window did stops being counted. * Fail the root write probe when the probe cannot be removed uv renames distributions into these directories, so a root that grants create and denies unlink fails the install later rather than not at all, and the probe file is left behind in the user's cache. NTFS carries DELETE as its own ACE and an append-only directory does the same on ext4, so this is expressible on both platforms. The bucket probes already decided this: install.sh:812 fails the candidate when rm does, and the PowerShell bucket loop uses -ErrorAction Stop, with a comment naming the NFSv4 and CIFS case. Only the three root probes disagreed with their own sibling, by way of `|| true` and SilentlyContinue. They now agree. The regression case simulates a failing rm rather than an ACL, because the mode bits cannot express create-without-unlink and a suite running as root would hide it either way. It fails against the old helper. test_cross_platform_parity.py had one slice that ran from the populated scan to the marker reader, two functions on, so it read "no -ErrorAction Stop here" over a helper that uses Stop deliberately. Its comment already said the scan only; the slice now ends where the scan does. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Believe the filesystem, not the delete, and treat a stale marker as stale Three findings from re-auditing the branch, each reproduced before and after. A probe that could not be deleted condemned the cache on the first failure. An indexer or a scanner holding the handle open makes one delete fail and the next succeed, and PowerShell's Remove-Item -ErrorAction Stop throws outright for a probe something else already removed, where rm -f exits 0. So the two sides disagreed, and a transient error could divert an install away from the Studio cache and into a shared one already known to be worse: an offline install that used to succeed then failed. Both helpers now retry and judge by whether the probe is GONE. install.sh's early block kept its own copy of that rule. It now calls the same helper, so the two cannot drift, and tests/sh/test_uv_cache_colocation.sh extracts the helper alongside the block: a missing function is `command not found`, which exits 127, which `if !` reads as "not writable", so every case would have quietly reported an unset UV_CACHE_DIR. A marker naming a directory that no longer exists was treated as a decision and suppressed the Studio cache, so a warm Studio cache holding Torch and CUDA lost to uv's default. A stale pointer is not a decision. The other half of the rule is kept as its own case: a recorded cache that still EXISTS and is merely cold does still fall through to shared, which is the machine that really has moved on. Test-StudioUvBucketName used ^[0-9]+$, and in .NET `$` also matches before a final newline, so `archive-v1<LF>` passed there and failed in sh. \A and \z. * Source the helpers before the early block, and prove the harness can My own last commit put a call to _uv_cache_root_is_writable inside install.sh's early block, and this harness sourced that block BEFORE the file defining the helper. A missing command exits 127, `if !` reads 127 as "not writable", and the block then unset the very default whose survival every ordering case depends on. The suite went on reporting 93 passes while no longer exercising the writable install ordering it was written to guard. install.sh defines those helpers around line 622 and runs the early block at line 1001, so definitions first is also the faithful order. The order now lives in one file, $_BOOT, sourced by the runner and by a new check alike. The first version of that check repeated the order instead of sharing it, so reversing the runner left it green -- the same shape of mistake one level up. Reversing $_BOOT now fails the suite before any case runs. The check asserts only that the block kept a cache it could create, not that _UV_CACHE_DIR_INSTALLER_DEFAULT is true: that flag is what this branch adds, and the suite has to keep running against code that predates it. Against the merge base and against main it is 27 passed, 66 failed; on this branch, 93 and 0. * Tighten the comments added by this PR for #10626 * Honor UV_NO_CACHE before recording a cache for PR #10626 The custom and isolated branches both choose a directory without probing, and both returned before the UV_NO_CACHE check, so they recorded a marker naming a cache uv never touched. Checked against uv 0.10.7: with UV_NO_CACHE=1 a caller's UV_CACHE_DIR is left completely empty, CACHEDIR.TAG included. A later repair reads that marker and can prefer an unrelated cache that happens to hold bytes. Both installers were the outliers here, the same way as the earlier fixes on this branch: unsloth_cli/commands/studio.py has always asked _uv_no_cache_requested() before choosing or recording anything. The spelling test is now one helper per installer rather than a case and an inline -in, so the three copies of clap's literal set cannot drift apart. Each branch keeps its own directory: a caller's UV_CACHE_DIR is still exported exactly as written, and isolation still points at the Studio cache. Only the marker write is skipped, and the custom step line says so. Tests cover both directions on both sides, since a guard that simply stopped recording would also pass the first half. The new helper is added to every extraction list; a name missing from one of those does not fail loudly. * Give the CLI updater the two cache rules the installers gained for PR #10626 The installers stopped abandoning a warm Studio cache on a pre-marker or stale marker install, and stopped counting `archive-v0.backup` as warmth. The CLI is what an existing installation actually runs on `unsloth studio update`, and it had neither, so the update could undo the fix on the same machine. _uv_cache_has_packages matched bucket names by prefix, which also takes `archive-v0.backup` and `archive-backup-v0`, whose bytes uv cannot reuse. Its docstring already claimed the same rule as install.sh. It now validates the whole `<kind>-v<N>` suffix through a helper that mirrors both installers. The precedence is POSIX-only, and the asymmetry is historical rather than a shortcut. install.sh always took its `custom` branch before the marker shipped, so shared mode was unreachable and nothing else wrote to $STUDIO_HOME/cache/uv: verified by running the selector from b66d2a4c8~1. A warm unmarked Studio cache there is this install's own. install.ps1's selection already worked, so a pre-marker Windows install could have used the shared cache and repointed the backend at the Studio one, leaving on-demand wheels there while Torch and CUDA sit in the shared cache. Windows therefore keeps uv's default ahead of it, which is what the test being retargeted here was protecting. A marker naming a cache that still EXISTS and has gone cold stays a decision on both platforms, so the machine that really moved on still reaches uv's default. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Ask about buckets, not just the root, where the answer decides for PR #10626 Two sites asked _uv_cache_root_is_writable about a cache whose buckets the candidate probe had already rejected, so both could hand back the cache uv is about to abort on. A `sudo` run leaves a root-owned bucket and a dangling link is an existing path to mkdir(2), so the root can be perfectly writable while the cache is not. The fallback chose the Studio cache unconditionally when nothing was warm, since a cold cache never reaches the warn path. Reproduced: a Studio cache holding a dangling archive-v0 and a cold default selected the Studio cache, and uv fails on the first bucket it touches. It now falls through to the populated cache that only failed the probe, then to uv's default when that is usable, which at worst costs the downloads this cache was never going to save. The launch repoint had the same root-only question. Reproduced: with a warm default the selection correctly chose shared, then the repoint sent the autostarted backend into the rejected Studio cache anyway. The rule now lives in one helper per installer, _uv_cache_is_writable and Test-StudioUvCacheUsable, and the candidate probe calls it too, so the three sites cannot drift. Both new names went into every extraction list in the same commit: a missing one is `command not found`, which reads as a clean answer. * Put uv's default ahead of an unmarked Studio cache for PR #10626 I had the history backwards. The marker arrived in b66d2a4c8 on 2026-09-06 and the early UV_CACHE_DIR block only in e12963071 the day AFTER, so an install old enough to have no marker is old enough that `shared` was reachable on POSIX, not too old. In that mode the launch repoint leaves a few on-demand backend wheels in $STUDIO_HOME/cache/uv while Torch and CUDA sit in uv's default. Ordering the unmarked Studio cache ahead of the default therefore picked the leftovers: reproduced as a real offline install that succeeds on main and fails on this branch. The earlier check that "confirmed" the opposite hardcoded the early block into its own harness, so it proved nothing about that revision. The unmarked Studio cache is now the LAST candidate in both installers, which is the order unsloth_cli/commands/studio.py already used, so the platform split I added there is gone with it. A warm Studio cache still wins whenever uv's default is cold, which is the install the rule was added for, and `unsloth studio update` is now byte-identical to main across all five marker states an install reaches. Separately, the fallback treated a Studio cache that is WARM but failed the probe as unusable and moved to a cold default, losing the bytes. A warm cache that only failed a speculative bucket probe beats a cold one we can write; a cold Studio cache with an unusable bucket still loses to uv's default, which is the case that prompted the probe. * Keep the CLI bucket rule ASCII for PR #10626 str.isdigit() is true for Arabic-Indic and superscript digits, so the helper added in c592585ab counted `archive-v<Arabic-Indic one>` as a bucket where the sh `*[!0-9]*` case and the PowerShell \A[0-9]+\z both reject it. uv writes ASCII, so Python was the only one of the three that would have read such a directory as warmth. Exotic, but it is a three-way rule and the other two already agreed. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: oobabooga <112222186+oobabooga@users.noreply.github.com> Co-authored-by: danielhanchen <elliegouldingstuff@gmail.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-09-13 20:52:07 -07:00
Michael Han	a42840c358	Studio: install the supported MLX stack on fresh macOS installs (#10403) ... * Studio: install the MLX stack on fresh macOS installs install.sh sets SKIP_STUDIO_BASE=1 so the core packages are not reinstalled, but that flag also gated the by-name MLX install. On the fresh path mlx-lm then only arrived transitively, nothing held tokenizers inside its <=0.23.0 window, and Train and Export came up disabled until `unsloth studio update` was run. * Keep GGUF-only installs free of the MLX stack The ungated MLX step also ran under --no-torch on Apple Silicon, where install.sh sets both SKIP_STUDIO_BASE and UNSLOTH_NO_TORCH. These are direct package args so NO_TORCH_SKIP_PACKAGES cannot filter them. Gate on NO_TORCH, matching the MLX health probe below. * Keep fresh MLX installs on supported versions * Expand MLX installer compatibility and failure coverage * Skip the MLX step where the pinned wheels do not exist The pins are not installable everywhere Apple Silicon is, and pip_install exits on failure, so on those hosts this step ends the whole install. mlx 0.32.1 / mlx-metal 0.32.1 macosx_14_0_arm64 wheels only, no sdist the whole pinned set cp310 and up, no cp39 wheel Read off the index rather than reasoned about: uv pip install --python-platform aarch64-apple-darwin mlx==0.32.1 -> no wheels with a matching platform tag (e.g. macosx_13_0_arm64) the same resolve at --python-version 3.9 -> no wheels with a matching Python implementation tag (e.g. cp39) Before this branch, macOS 13 arm64 reached neither failure: the fresh path skipped the step, and the update path was unpinned and resolved mlx 0.29.3. Running it on fresh installs and pinning it turns both into an abort with no manifest written, i.e. a venv the CLI then reports as half-built. Nothing is lost by stepping aside, because those hosts cannot have a usable stack in the first place. mlx_repair's own floors (mlx>=0.22, mlx-lm>=0.31.2, mlx-vlm>=0.4.4) are unsatisfiable there: uv pip install --python-platform aarch64-apple-darwin 'mlx-lm>=0.31.2' -> mlx>=0.30.4 has no wheels with a matching platform tag The unpinned resolve the update path used was worse than nothing: mlx-lm 0.29.1 and mlx-vlm 0.3.9 are both under those floors, so Studio rejected the stack anyway, and it dragged transformers to 5.16.1, the version overrides-darwin-arm64.txt documents as stranding tokenizers and breaking every `import transformers` on macOS arm64. Both branches spend a progress slot, so the denominator does not depend on the host, and the gate the budget is computed from is unchanged. test_no_torch_skips_mlx_stack.py reads the guard out of the AST and the step now sits one branch deeper, so it walks the enclosing chain and asserts on the outermost test, which is still the platform gate it exists to protect. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Tighten the comments on the MLX install step Comment lines only, AST-verified: 66 removed for 30 added across the three files. Every load-bearing fact stays, including the wheel provenance the two floor constants are read from. --------- Co-authored-by: danielhanchen <elliegouldingstuff@gmail.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-09-08 05:19:48 -07:00
Daniel Han	6dcfd43196	Make the venv-hardening decode strict, and say which codepoints differed (#10432) ... * Make the venv-hardening decode strict, and say which codepoints differed #10476 fixed the red Cross-platform parity run by decoding PowerShell's stdout as UTF-8, and its own windows-latest run is green, so the transport bug itself is closed. This is the part that failure exposed and that fix did not cover. errors is strict. `encoding = "utf-8"` alone still defaults to errors="strict" for subprocess text mode today, but the default is not the contract, and the cost of being wrong here is the exact failure #10476 just fixed: an undecodable byte becomes U+FFFD and fails an assertion about a value that was written correctly. Saying it makes the intent unambiguous at the one place that reads non-ASCII output. The assertion is the reason this cost an investigation rather than a glance. `assert result["After"] == expected, result` renders its diff through the same stdout under suspicion, so on a cp1252 console the EXPECTED side came back as U+FFFD while the actual side round-tripped, and the report read as though the correct half were the broken one. The raw log needed byte-level reading and a caret count to establish which operand was mangled. It now prints codepoints, which survive any code page, sliced from the common prefix so the message stays short. It names a cause only when the bytes show one: mojibake is the actual reading as expected's UTF-8 bytes back through a single-byte code page, so that is tested by re-encoding through cp1252, latin-1, cp437 and cp850 rather than pattern-matched. A genuine restore bug prints the codepoints and no explanation, because explaining a real bug away as an encoding artefact would be worse than saying nothing. Deliberately not included: an earlier revision of this branch also prepended install.ps1's UTF-8 preamble to pin the emit side. #10476's green run shows the runners already emit UTF-8, so that was defence against a condition that does not hold, and it is dropped rather than carried. 45 passed locally, kwarg-spacing check 62 passed, and both branches of the new assertion exercised directly: the cp1252 mojibake case names the code page, a substituted path does not. * Point the mojibake hint at the script, not the transport The hint said the shell's stdout had been read with the Windows ANSI code page and that the script did not lose the path. That is backwards now: _run_powershell decodes stdout as strict UTF-8, so a transport mis-decode raises there and never reaches this helper. Mojibake that does reach it was produced inside the script, which is the product defect these cases exist to catch, and a wrong explanation on a real restore bug costs more than none. Same byte test, opposite conclusion, plus where to look: a missing -Encoding utf8 on the marker read/write path. 45 tests in the file pass, and the rendered message on a seeded cp1252 round trip now reads 'the mis-decode happened inside the script, not in transport'.	2026-09-07 23:44:27 -07:00
Daniel Han	9fa8d60855	Decode PowerShell stdout as UTF-8 in the Windows venv hardening tests (#10476)	2026-09-07 21:52:31 -07:00
Daniel Han	b66d2a4c89	Give unsloth studio update the same uv cache the backend already uses (#10386) ... * Give `unsloth studio update` the same uv cache the install used Three places pick a uv cache and only two of them agreed. install.sh and install.ps1 choose one for the install itself (#10204), and storage_roots._setup_cache_env seeds cache_root()/uv for the backend server. An update reaches neither: `unsloth studio update` runs setup.sh / setup.ps1 straight from the CLI process, nothing on that path sets UV_CACHE_DIR, and _setup_cache_env is only called from the backend, so uv fell back to its own user-wide default. On a fresh install the installer downloads into <StudioHome>/cache/uv and the first update downloads all of it again into ~/.cache/uv or %LOCALAPPDATA%\uv\cache, where uninstall cannot reclaim it. The rule is the installer's own rather than an unconditional redirect: prefer the Studio cache, and defer to uv's default only when the Studio one holds no package bytes and uv's does. _setup_cache_env mkdirs an empty Studio cache on every server start, so a shared-mode install would otherwise be sent to a cache holding nothing -- one duplicate download online, and with UV_OFFLINE, --offline or uv.toml offline = true, where uv may read only what is cached, an update that cannot resolve at all. Whichever cache wins is passed to the child as an explicit absolute path. uv reads UV_CACHE_DIR as --cache-dir, so a blank inherited value is a missing argument rather than an unset one (`UV_CACHE_DIR= uv cache dir` exits 2 with "a value is required"), and uv answers a relative cache-dir with the relative spelling, which setup.sh would resolve from a different directory. * Resolve uv's cache answer the way uv does Two ways the probe could name a directory uv never uses, both measured on the uv this repo pins, 0.10.7. A configured `cache-dir = "~/.myuv"` is printed verbatim and uv treats the tilde as an ordinary relative segment: it creates a literal "~" directory in its working directory, not one in $HOME. expanduser therefore probed a path that does not exist, so a warm configured cache read as cold, or an unrelated $HOME cache was picked up. A relative cache-dir resolves against uv's working directory, and --directory (env alias UV_WORKING_DIR) moves that out from under ours. Measured: with UV_WORKING_DIR set, `uv cache dir` prints "relcache" and the cache lands under UV_WORKING_DIR, not under the caller's cwd. * Fall back to uv's platform cache when the probe cannot answer uv discovers configuration from the CURRENT directory, so a malformed uv.toml beside whoever ran `unsloth studio update` makes `uv cache dir` exit nonzero even though setup.sh changes directory before it runs uv at all. Treating that as "there is no default cache" sent the update to a cold Studio cache: a duplicate download online, and an offline update that cannot resolve despite a populated platform cache. install.sh:646 already falls back to XDG_CACHE_HOME/uv or HOME/.cache/uv in exactly this situation. The probe now does the same, adding LOCALAPPDATA/uv/cache for Windows, and returns nothing only when there is no anchor to build a path from. * Trim the comments this branch added Delete first, collapse second. The rationale that survives names a mistake the next reader would otherwise make: metadata-only caches reading as warm, UnicodeDecodeError escaping an OSError handler, a blank UV_CACHE_DIR exiting 2, uv's literal tilde. The measured numbers and the alternatives considered are in the PR description, which is where they belong. Prose in the added block: 51 lines to 16. * Record which uv cache the install used, instead of inferring it later (#10410) * Record which uv cache the install used, instead of inferring it later Content cannot answer the question the update asks. In shared mode the installers point the running backend at the Studio cache (_prepare_studio_uv_cache_for_launch, Set-StudioUvCacheForLaunch), so one on-demand install from the server (wheel_utils.py:405) leaves package bytes there. Both caches then hold packages, and the update stops preferring the one the install filled: a duplicate download online, and under UV_OFFLINE or uv.toml offline = true an update that cannot resolve. The mirror case is just as wrong, a studio-mode install losing once the user warms their own default cache with an unrelated uv pip install. Both installers now write the cache they chose to <StudioHome>/cache/uv-cache-dir and the update reads it, falling back to inspecting both caches for installs that predate the marker. Every mode records, a caller's own UV_CACHE_DIR included, since that is where the install put its wheels and an earlier marker left in place would aim later updates at a cache this install never filled. A caller keeps control either way: a nonblank UV_CACHE_DIR outranks the marker at update time too. The marker is honoured only while the cache it names still holds packages, so `uv cache clean` does not leave a stale pointer outranking a warm cache. The marker describes the environment, so it travels with it. It is written absolute, since the update resolves it against its own working directory (on PowerShell that needs $PWD explicitly: GetFullPath resolves against the .NET process directory, which Set-Location does not move). The previous contents are saved before the first write and restored wherever a failed install rolls the venv back, and the snapshot resets per run because under `irm | iex` the script scope is the caller's session. An unwritable Studio root skips the marker rather than failing the install. For installs that predate the marker, the update records what the fallback worked out once setup has returned 0, and replaces a marker whose cache has since been emptied. A caller's UV_CACHE_DIR is never promoted: that belongs to one run, and the installers are what record an install. * Reset the marker snapshot before the selector, and restore it on any failure Four fixes, all on the marker's lifecycle. The per-run reset ran AFTER Set-StudioUvCacheEnvironment, which is what writes the marker, so it cleared the snapshot that call had just taken and made both Restore-StudioUvCacheMarker calls no-ops. A failed reinstall kept the new marker. Moved above the call, and the parity test now pins it to the lines directly above rather than anywhere in the file, since searching the file matches the copy inside the restore helper and passes either way. Restoring only from inside the venv rollback missed every failure that happens before a replacement is in flight: a first install has no previous venv, the ownership guard can refuse the directory, the backup move can fail. The restore now runs from the exit and signal handlers on the sh side and from Exit-InstallFailure and the commit guard's finally on the PowerShell side. Test-Path was unsuppressed under $ErrorActionPreference = "Stop". Probing a path inside a directory the ACL denies throws UnauthorizedAccessException rather than returning false, so an optional marker could abort the install. Measured: Test-Path THREW UnauthorizedAccessException on a chmod 000 parent. The reader stripped whole lines. The installers write UV_CACHE_DIR through verbatim and a directory name may legitimately begin or end with a space, so stripping probed a different path and read a warm cache as cold. splitlines already drops the terminator; blank lines are still skipped, since blank means unset. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Read the marker the way uv reads a path Same reason the probe stopped expanding: the installers write this path through as uv would read it, and uv treats a leading tilde as an ordinary segment rather than $HOME. Both installers already make the value absolute, so expanduser could only ever have been wrong here, never right. * Tie the marker's lifetime to the environment it describes Eight findings, all on when the marker may be written, reverted or kept. Committing the environment commits the marker. The unconditional restore I added reverted it on any nonzero exit, including after the venv was already committed, so a later failure left the installed environment pointing at the previous install's cache. Both shells now clear the snapshot at the commit. The PowerShell reset moves to the first lines of Install-UnslothStudio. It sat after the lock checks, and nothing clears the flag on success, so under `irm | iex` a second install failing early reverted the marker belonging to the first, which had succeeded. A marker that cannot be read is left alone. `cat` failing was recorded as an empty snapshot with existed=true, so a rollback wrote a blank marker over the previous install's; Get-Content failing left $null, and TrimEnd on it throws inside the rollback, before -ErrorAction can apply. Writes no longer follow symlinks. A redirection, Set-Content and write_text all truncate the target rather than replacing the entry, so a marker path linked elsewhere silently destroyed an unrelated file. Relative caches resolve against uv's working directory, which --directory and UV_WORKING_DIR move, rather than the installer's $PWD. The marker directory is created with the .NET API, since New-Item -Path treats brackets in a custom Studio root as a wildcard, as this installer already notes at two other call sites. The backfill is suppressed during a staged update: STUDIO_HOME names the live install there, and the stage can still be rejected. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Anchor a relative uv working directory, and clear the marker with the venv flag UV_WORKING_DIR may itself be relative, and uv resolves it against the directory the installer ran from before it resolves the cache under it. Recording "work/cache" would name somewhere else at update time. Both shells anchor the base first; on PowerShell that has to be $PWD.Path, since GetFullPath would use the .NET process directory instead. The commit cleared the marker flag after the venv flag and after the backup deletion. A signal in that window kept the newly committed environment, because venv rollback was already inactive, and reverted the marker it came with, because that flag was still set. Both are cleared together now, and a parity test asserts the order rather than mere presence. * Commit the marker on a first install, gate its write, and record paths as bytes Three items from the last review. The commit cleared _UV_MARKER_SAVED inside the rollback branch, and a first install has no previous environment, so it took no branch and left the marker revertible. Anything failing after that point, the shim-path guard or a signal, kept the newly installed environment and reverted the marker it came with. Both shells clear it before the branch now. The unlink that keeps a symlinked marker from truncating what it points at can fail silently, on a directory that denies deletion while the target stays writable. Both writes now happen only once the entry is confirmed gone; on PowerShell that probe is Get-Item -Force, since Test-Path follows the link and reports a broken one as absent. A POSIX path may hold bytes that are not UTF-8, and Python hands those over as surrogates. Encoding them raised UnicodeEncodeError, which is not an OSError and escaped the best-effort handler, failing an update whose setup had already succeeded and whose old marker was already unlinked. The path is written with os.fsencode and read back with surrogateescape. Two of the existing marker cases in the rollback suite were passing vacuously: _record_uv_cache_choice sits outside the spliced block, so the harness exited 127 before writing anything, which satisfied every expectation about a marker that must not change. The helper is spliced alongside the others and the cases now fail if the harness writes to stderr or dies on a missing command. * Tighten the comments added by this PR Comments and docstrings only, no code. Verified with comment_tools.py check and by rerunning the suites: 183 passed, the two sh suites under sh, dash and bash, and all 24 pwsh studio tests. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * Publish a staged update's cache choice, and treat a malformed marker as cold Two items from the last review. A desktop update never runs the direct path, so suppressing the backfill under --stage left exactly the installs the marker exists for permanently on the content fallback. The staged child still cannot write the live marker, since STUDIO_HOME names the environment it is replacing and the stage may never activate, so it parks the choice in the stage root and _studio_stage.stage promotes it after the probes pass. A stage that fails verification publishes nothing, and a stage that parks nothing leaves the live marker alone. A marker holding an embedded NUL builds a Path fine and then raises ValueError, not OSError, out of scandir, so a corrupted or hand-edited file aborted every update before setup ran. That cache reads as cold now, which is what the rest of this code already does with a cache it cannot inspect. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Probe uv where setup will run it, and record the marker unambiguously Two items from the last review. Both setup scripts change into their own directory before the dependency pass (studio/setup.sh:1788), and uv discovers uv.toml and pyproject.toml from its working directory. The probe asked from the caller's, so running an update while standing in an unrelated project answered with that project's cache, and the answer is then forced on the child through UV_CACHE_DIR. It now runs with the setup script's directory as its working directory, and resolves a relative answer against that same base. A newline is legal in a POSIX path and uv reports one verbatim, but the marker reader split the record into lines and kept the last fragment, so such a cache read as absent and the update redirected uv elsewhere. The record is one path followed by one delimiter now: strip that delimiter, keep everything else. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Make one install agree with itself about its cache, and commit in one step Six items from the last review. setup.ps1 never changes directory: it addresses everything through PSScriptRoot and hands install_python_stack.py the cwd it inherited, so probing from the script's directory answered for a configuration that child never sees. The probe now uses the directory setup will actually run uv from, which is the script's on POSIX and the inherited one on Windows. UV_WORKING_DIR may itself be relative, and uv resolves it after starting where the probe started, so it is anchored to the probe's directory rather than to wherever the update was launched. uv reports a cache path verbatim, and stripping the reported line changed a directory name that legitimately ends in a space. Blank lines are still dropped. A relative UV_CACHE_DIR named a different directory in each phase of one install, because setup.sh changes into its own directory before the dependency pass. It is resolved once, before anything reads it, so both phases and the marker agree. What is restored afterwards is still the caller's own spelling. Windows PowerShell 5.1 decodes a BOM-less file with the active ANSI code page, and the update writes this marker BOM-less UTF-8, so a non-ASCII path was read back as mojibake and restored that way. It is read as UTF-8 explicitly. The commit cleared two flags, and a signal between them put the previous environment back while keeping the marker of the attempt that replaced it. One flag decides both rollbacks now, set first and alone, and both restores consult it. * Resolve the Windows cache through one helper, and leave ambiguous installs alone Two items from the last review. The custom branch on Windows anchored a relative UV_CACHE_DIR to $PWD alone, ignoring UV_WORKING_DIR, and rooting it that way also stopped the marker writer from applying its own resolution. From C:\work with UV_WORKING_DIR=subdir and UV_CACHE_DIR=cache, uv uses C:\work\subdir\cache and the installer recorded C:\work\cache. Both call Resolve-StudioUvCachePath now, which mirrors _absolutize_uv_cache_dir. An install that predates the marker cannot say which cache it used, and content cannot tell either: the running backend is pointed at the Studio cache even in shared mode, so one on-demand wheel warms it. Where both caches are warm and no marker settles it, the update keeps uv's default, which is what such an install has been updating from all along. Preferring the Studio cache there was a new way for an offline update to fail on an install that cannot record its way out. A Studio cache that wins on content alone still wins when uv's default is cold, which is the case this PR exists for, and a recorded marker still settles both. * Tighten the comments this branch added Comments and docstrings only, no code. Verified with comment_tools.py check and by rerunning the suites: 238 passed, both sh suites under sh, dash and bash, and all 24 pwsh studio tests. * Stay out of the way when uv is told not to cache Confirmed against uv 0.10.7: UV_NO_CACHE=1 makes `uv cache dir` report a temporary directory that is discarded when the process exits. The probe inherited that, read the throwaway as cold, and the update then seeded a UV_CACHE_DIR that --no-cache outranks anyway and recorded it as the install's cache. On an install predating the marker that redirected every later update to a cache nothing had ever filled. Neither seeds nor records now, on uv's own boolish spelling of the variable: 0 and false leave caching on, and a value uv cannot parse is one it refuses to run on at all, which is not the same as running without a cache. * Tighten the comments the last change added Comments and docstrings only, no code. Verified with comment_tools.py check and by rerunning the suites: 246 passed, both sh suites under sh, dash and bash, and all 24 pwsh studio tests. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-09-06 23:53:12 -07:00
Wasim Yousef Said	919922c416	Use hybrid uv cache migration for Studio installs (#10204) ... * Fix Studio install-time uv cache root * Restore absent uv cache environment exactly * Round-trip blank PowerShell cache variables * Test platform-representable cache state * Improve Studio uv cache migration UX * Honor uv cache config during Studio install * Fix PowerShell uv cache query capture * Only treat a uv cache with package bytes as warm Three ways the selector reached the wrong verdict, found by running both selectors over a matrix of real cache shapes under sh, bash, dash, busybox ash and pwsh. wheels-v* is metadata, not packages. On a real uv 0.10 cache it holds .msgpack and .http exclusively; the package bytes live in archive-v0, in builds-v0, and in the .whl and extracted contents of sdists-v9. Accepting any file in wheels-* meant one `uv pip install --dry-run`, or an install that failed before downloading anything, left a single .msgpack behind and made a cold cache read as warm. That sent the whole install to the global cache: no download saving, and the Torch and CUDA wheels land outside the Studio root, where uninstall cannot reclaim them. Verified end to end with real uv: before this change the "resolved but never downloaded" case grew the global cache, now it grows the Studio cache. The PowerShell scan failed closed. `-ErrorAction Stop` plus the catch-all turned one ACL-denied subdirectory into "the cache is empty", which is exactly the multi-gigabyte redownload the selector exists to avoid, and it said nothing about why. `find` on the sh side already skips what it cannot read and keeps going, so the two installers disagreed on the same machine state. Both now skip unreadable corners, and when a bucket cannot be opened at all they say so instead of falling back silently. `find` without -L does not descend a symlinked start point, so a bucket symlinked onto another volume read as empty on POSIX while Get-ChildItem -Recurse followed it on Windows. Another disagreement on identical state. Also: take the last nonblank line of `uv cache dir` rather than the first, so a notice printed ahead of the path cannot be mistaken for the path, and add builds-* alongside the built-wheels-* name modern uv no longer uses. The selector matrix is 394 checks over 106 cache and platform shapes across the five shells, and it now passes with no cross-runner disagreements; it had 54 failures and 9 disagreements before. Every pre-existing pathway is unchanged: fresh machine, venv-only scaffolding, genuinely warm global cache, upgrade over a warm Studio cache, CI-preset UV_CACHE_DIR and forced isolation all select what they selected before, each confirmed with a real `uv venv` plus `uv pip install` plus import. Tests: 12 new sh cases (45 total, under both sh and bash), 5 new PowerShell cases (121 total), and parity assertions pinning the bucket list, the metadata suffixes, SilentlyContinue and -L so the two selectors cannot drift apart again. `uv cache dir` was checked against 0.8.16, 0.9.3 and 0.12.1, covering the whole accepted range. This also picks up the pre-existing ruff-format drift in test_windows_python_venv_hardening.py, which the pinned 0.6.9 formatter wanted on the base tree as well. * Document the uv cache isolation option in the README Both installer headers say usage and supported options live in the README under "Unsloth Studio (web UI)" and are deliberately not repeated in the script, and Advanced launch options already carries every peer switch this branch sits next to: UNSLOTH_NO_TORCH, UNSLOTH_SKIP_AUTOSTART, UNSLOTH_PYTHON, UNSLOTH_STUDIO_HOME. This branch added UNSLOTH_ISOLATE_UV_CACHE and --isolated-uv-cache to those headers without adding them where the headers point, so the only way to find the switch was to read the installer. Documents the piped env var and the local flag for both shells, and states that an explicit UV_CACHE_DIR still wins over it, which is what the code does: the custom branch returns before the isolation branch is reached. Every invocation shown was run against this branch's own parsers rather than transcribed: --local --isolated-uv-cache and the env var in 1/true/ yes/on all select isolation, absent and 0 do not, on both installers. * Tighten the comments this branch added Comments only, no code. Two sweeps over the 103 comment lines this branch adds, down to 41. Deleted the ones a test label already says (the artifact lands next to the metadata; a deeper unreadable corner keeps it warm; metadata is not a warm cache) and collapsed the rest to the fact that would otherwise be re-derived: wheels-* is metadata only, -L is what makes find match Get-ChildItem -Recurse, SilentlyContinue is what stops one denied subdirectory reading as an empty cache, unreadable is not empty. comment_tools.py check: 5/5 OK, code unchanged in every file. Tests still green: 45 sh checks under both shells, 121 pytest. --------- Co-authored-by: danielhanchen <elliegouldingstuff@gmail.com>	2026-09-06 04:27:45 -07:00
Daniel Han	971eb36f48	Trim comments in tests, scripts, CI helpers and unsloth_cli (#10113) ... * Trim comments in tests, scripts and CI helpers Comment-only pass over 453 changed files under tests/ (excluding the studiobench harness), scripts/ and .github/scripts/. 17,749 -> 10,635 comment lines (-40.1%) 1,183,079 -> 757,190 comment characters (-36.0%) Distinctive-fact retention against the base commit: 93.2%. A sample of 100 restoration and deletion decisions was judged by hand: 98 correct. No comment line exceeds 120 physical characters. Nothing outside comments changes: no code, docstrings, string literals, licence headers or pragmas. Verified with an AST code-only signature of every file against the base. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Put the moved comments back on the code they describe The signal-handler rationale in launch.py had been reattached to the handler registration loop, which has no try/finally, and its except-branch note had been split into a fragment starting mid-sentence with the noqa marker duplicated into the prose. The PORT_FILE launcher comments had been reattached to the _studio_root_id caching test, which does not touch PORT_FILE. * Put the section banners back, and use plain dashes in comments The trim removed the rule-line comments that index these files. Tests locate a region by finding its banner and end it on the next one, so a missing banner is a broken anchor rather than lost decoration. Each is restored above the code line it preceded at 1d2679f; where that anchor is not unique the banner is left out rather than guessed at. Em and en dashes are rewritten as plain dashes inside comment tokens only. String literals are code and are untouched, so the diff stays comment-only. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Put reattached comments back on the code they describe The comment pass moved a number of explanatory comments off the code they were written for and onto unrelated statements nearby, so they now read as false or as truncated fragments. This restores each one at its correct site: - lint_duplicate_definitions: the import-key rationale goes back on the emitted Finding key instead of being half-pasted onto `implicit =`. - lockfile_supply_chain_audit: "Empty by default" belongs on the empty CARGO_IOC_STRINGS tuple, not on the non-empty source allowlist; the missing-lockfile and traceback notes go back on their own branches. - scan_packages: "archives landed in dest" describes the successful pip download, not `if not downloaded:`; the backslash-continuation note goes back on the continuation check in _scan_line_end. - interrupted_install_probe: the start_new_session/getpgid note belongs on the posix branch that calls getpgid, not on the win32 branch. - virustotal_scan: the missing-secret rationale goes back on the skip path and the :⚠️: note back on the detection print. - scan_npm_packages: the rmtree cleanup note goes back on the rmtree. - check_frontend_dep_removal: one per-key rationale per key again, instead of five concatenated onto browserslist. - profile_startup: failed-launch and nothing-measured notes swapped back. - enforce_kwargs_spacing: fold-rejection reasons back on the guards that reject for them, and the f-string fold comment back in order. - tests/conftest: the bitsandbytes broken-wheel note back on the bitsandbytes handler. Comments only; no code, string literal, docstring or pragma changes. * Repair the banner restore: drop duplicate headings, keep the blank line The scripted restore re-added each section banner without removing the de-bannered plain heading the trim had left behind, so a heading appeared twice, once ruled and once bare. It also inserted the banner flush against the next line of code where the base had a blank line after it, which turns a section separator into a doc comment for whichever def follows. Both are the same fault the trim itself was being screened for, introduced by the remediation rather than by the trim. * Reattach 12 review comments to the code they describe Each of these was a rationale that ended up on a neighbouring test or assertion, or a fragment cut mid-sentence, so it described something other than the line under it. - test_gguf_disk_preflight: the 3GB-embeddings arithmetic belongs to the untied case, not the tied one (2GB there). - lockfile_supply_chain_audit: reassemble the workflow-command escaping rationale on the skip branch. - studio_smoke/multi_turn_chat: restore the retry-policy explanation in main(). - test_virustotal_scan: restore the advisory-posture rationale on the scan step. - test_amd_fastpath_probe: return each ROCm rationale to its parameter row. - test_cuda_repair: the Blackwell span note goes back on the Blackwell test; complete the Kepler sentence. - test_windows_rocm_bnb_version: realign the sitecustomize / opt-out rationales with their cases. - test_dataset_num_proc: the Pool(1) note belongs to the low-memory auto path. - test_install_whisper_prebuilt_logic: decoy-DLL, libomp, build-bump and same-ggml notes back on their own tests; the link copy fallback needs none. - playwright_loaded_models_indicator: stale-eject notes back on their sections and the preference banner restored. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Restore the review comments that were cut mid-sentence or reattached to the wrong code * Restore the comment tails the 120-column pass cut off The trim enforced the column limit by chopping the physical line rather than rewrapping it, so a block's last sentence stopped mid-thought and the clause carrying the reason was gone: -# ...fails in a second rather than +# ...fails in a second rather than after the quantise and the multi-gigabyte save. -# fp8 / mxfp8 need bf16 weights, so skip non-bf16 Linears; +# fp8 / mxfp8 need bf16 weights, so skip non-bf16 Linears; nvfp4 handles fp32. Mirrors the runtime gate. 883 blocks across 266 files, restored from the merge base and rewrapped at word boundaries within 120 columns. Blocks that end on proper punctuation followed by a new sentence are left alone: those were shortened on purpose, and only a base that continues the same sentence where the head stops counts as a cut. No word is invented; every restored word comes from the base. Pragmas, shebangs, coding lines, licence text and rule-line banners are skipped. Dangling truncations drop from 336 to 77. Full suite is 65 failed / 18683 passed, matching main exactly, so no test changes behaviour. * Restore comment tails cut inside a parenthetical or abbreviation The first repair pass keyed on terminal punctuation, so a block cut after "(e.g." or mid-code-span looked finished and was left alone. Balance the brackets and backticks instead, and treat a trailing abbreviation as mid-sentence: -# ...skip the whole module up front on runners without it (e.g. +# ...skip the whole module up front on runners without it (e.g. CPU-only) before importing unsloth, otherwise +# collection errors instead of producing a skip. 22 further blocks across 20 files. _ts_deps.py is fixed by hand: it had also lost a sentence from the MIDDLE of the block, so it was not a prefix of the base text and no prefix rule could reach it. * Reassemble the review comments that were left scrambled or cut short Restore the full rationale for the comments the trim pass left as reversed or truncated fragments, and put the openenv, reasoning-effort and recovery window notes back on the code they describe. * Put the scrambled comment blocks back over the code they describe * Put the inline labels back on the entries they label * Restore an inline label lost in the merge * Restore the facts the reworded comments dropped * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Move the displaced notes back onto the tests they describe * Re-pair the fixture rationales with their fixtures * Put the relocated notes back and finish the sentences the trim cut * Restore the dropped facts and correct the ones that were never true * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Unfuse the merged notes and drop the reformatting the trim smuggled in * Restore the clauses the trim cut off and unfuse the notes it merged Thirty sites where a comment was shortened past the point it still said anything. Most ended on a colon or a dangling word, so the reason the branch exists was gone while the sentence introducing it stayed. The rest had absorbed a neighbouring test's note, which left one row documented as another row's behaviour. Each restored clause was checked against the code under it rather than copied back on trust. Three were not restored: a pointer to a measurement that is not in the function it names, a claim that a missing import fails as NameError when it fails at collection, and a redundant restatement in the autoscroll check. tests/kaggle/t4_smoke/training_evidence.py ends up identical to main, which is the right outcome for a file whose only trimmed sentence was load-bearing. * Give back the halves of the comments the trim cut off Two hundred and twenty comments across a hundred and twenty files had been shortened to the sentence that introduces a fact and none of the fact. A note ending on a colon or on "so" or "carried over" still reads as a comment and still parses, which is why every structural check has been passing over them: a truncated comment is a well-formed comment. Most of these were only findable by asking what a block ENDS on rather than what it starts with. The seven passes before this one all keyed on the first line, so they were blind to the commonest damage in the whole trim. The supply-chain audit had seven "Mini Shai-Hulud May-12 wave:" headers with the package names deleted, which was the entire content of each one. A handful were worse than truncated. A "can't fit on one line, so leave it" note had moved from the test asserting changed is False onto the one asserting changed is True; the sm_61 and sm_70 rationales in the CUDA repair tests were stacked on a test that exercises neither, leaving their own tests bare; an unpinned-latest note sat on a test that stubs a working install on disk. Restored clauses were checked against the code under them, not copied back on trust. Around a dozen were left out or rewritten because the base was wrong about the code as it stands: a pip stub credited with an append it does not do, five exports said to be filled by a loop that fills one, a __dict__ rationale that argues for the opposite of what the loop needs, and UI strings quoted as current that the sources no longer use. * Put the fused row notes back on the rows they are true of Ten sites where a comment described a sibling. The torchao skew table had three rows' notes concatenated onto the 0.18.0+cu130 row; the revision-forwarding table had the adapter-tokenizer exception one row below the only row it holds for; the vision LoRA pair had the tied and untied cases on a single anchor. Four of these were mine, not the trim's. The 120-column rewrap left an orphaned continuation line sitting on a class, one repair copied an untagged-build note onto a fixture that pins a URL and has no untagged build in it, another duplicated a namespace-package note and pushed the interrupted-install label onto the wrong row, and a fourth moved a collision note onto a fixture with a single host, where it is false. A pass that rewrites 220 comments will get some of them wrong, and these are the ones it got wrong. Two reported sites were left alone. The masking note in the VirusTotal client already carries its full clause, reflowed; the IOC scan comment reads complete, and its missing "4." went with the numbering on every other step in that function. * Move the row and branch notes back onto what they describe Nineteen sites. The token-count parity table had every row comment one param too low, so each seeded reasoning style was documented as the next one; the font-scale contract had two rationales slid down a continue each; the validator fixture banner for R-INST-001 sat under R-INST-005, leaving three tests unlabelled and two double-labelled. Three of these I caused. The parity table was misplaced by the previous commit, which moved fused notes onto rows without checking the seeds underneath them; a timing bound landed in a test that has no timing assertion; a restore note was reinserted between the two commands it claims to cover instead of above them. The pacing benchmark had lost its measurement table entirely, including the #7892 result that is the reason there are two budgets rather than one headline number. Restored with the columns intact. * Undo the damage the last four repair passes did Twenty-five sites, and almost none of them were the trim's. Round nine was called "move the row and branch notes back onto what they describe", and it moved a good many onto things they do not describe: the xpu swap tests had the pinned-mirror rationale on the test that asserts the index is dropped, the cuda repair file had a whole run of rationales slid one test upward, and several notes were duplicated onto a neighbour rather than moved. Where a clause was already correct on its real anchor, the copy is deleted rather than parked somewhere new. Separately, this PR had been changing code, and the gate could not see it. Five files: three where a repair split a tuple across lines to make room for a restored trailing comment, two where the trim deleted a trailing comment and then joined up the wrapping that existed only to carry it. Parentheses and line breaks leave the parse tree untouched, so an AST comparison calls all of it comments. Every one is back to base byte for byte, with the comments that justified the wrapping restored. scripts/code_reflow.py is the check for it: same tokens in a different arrangement is a code change, whatever the AST says. * Let ruff rejoin the two lines the shorter comments freed Both were split only to make room for a trailing comment. Round ten cut those comments back to what is true of the row, the expressions now fit, and the formatter joins them. Running it is not optional: pre-commit.ci was red on exactly these two. * Delete the stray copies rather than move them again Five sites. Three were duplicates: the correct sentence still sits on the test it describes, and a repair pass left a second copy on a neighbour. Moving those would have been the fourth round of shuffling one clause between two tests, so the copies are simply gone. The mislabelling was the point in each case. An empty handover tag and an absent one are different precedence rules, and the PowerShell 7.5 parsing note had been copied from the empty-string test onto the one whose variable is unset. A non-zero-exit rationale sat on a parser test that asserts a successful exit. The other two are the trim's own: "#5854)," with its subject cut away, and a regex description that stopped before naming the self. prefix it matches, which is what explains the narrower filter below it. * Put the nine slid notes back on what they assert Seven had moved one branch or one call down. The autocast one had landed on the case that forces float32 and asserts dtype float16, directly under a note saying there is no dtype at all; the JAX rationale sat on the USE_FLAX env-var case, whose module map has no jax in it; the kill-and-sentinel note sat on a test that only checks a GPU row regex. Each is back on the assert it describes, with a tenth found in the same file as the probe timeouts. Two are the trim's own. A mid-block deletion in the settings-tabs driver spliced two half sentences into "blocking a / error-handling regression and was a stale constant"; the macOS slot table lost the only statement of why macos-14 is absent from a set named for images GitHub still schedules. The retirement dates are restored, verified against all three workflows that carry them. The settings-tabs note is shorter than base on purpose. It now says the invariant, that blocking a panel must not collapse the dialog, and drops the tab count, which nothing asserts. * Move twelve more notes back onto the branch they are about All twelve had slid onto an adjacent test or branch, and in each the displaced note contradicted what it now sat on. The CPU-backend note about a wheel being left alone had landed on the test asserting a repair runs; the XPU one about a dispatched shard sat on the single-device case; the redirect-body note sat in a test that never binds served; the draft-state refusal sat on the test where the target is present and the run returns 0. One lost text on the way as well: the missing-target rationale had been cut to its first two sentences, dropping the reason the draft and prerelease refusal above it would otherwise degrade into no check. Also a rewrap that duplicated its own tail instead of replacing it, so the merge-budget note read "else a sharded own device, not GPU0, else a sharded model OOMs". * Return six notes to the test whose behaviour they describe Eight reports, six defects: two pairs described the same misplacement from either side. One was worse than misplaced. "A comment naming a probe is not a probe" had moved onto the metadata scan, whose predicate matches the raw line with no comment handling at all, so it claimed an exemption that scan does not implement. It belongs on the Test-Path scan, the only one that splits on # before matching. Moved, not reworded. The rest argued against the test they sat on. The lazy-trl note was on a stub with no __getattr__ to raise; the bogus-variant note was above a request sending a valid variant; the imported-TensorFlow note was on the case that supplies no tensorflow and asserts the flag is cleared; the 62-token rationale was on the test that asserts 12. * Move ten more notes onto the statement they explain Seven had slid off their anchor, and in most the stray was also cut short, so the fix was to restore the whole sentence where it is true and delete the fragment. The torchao dependency reason sat on a test about re-verification rather than on the --no-deps assertion it justifies; the bitsandbytes heading sat on a test that fetches quantizers_utils.py; the no-selection latch note sat on the delete block, which asserts nothing reaches the action. Two are the trim's own truncations. The stand-aside rationale in the shortcut driver stopped at a colon, losing why Enter on a focused Deny button would otherwise be defaulted away into approve. The kwargs-union case stopped at a semicolon, and the trim had eaten the leading ** from **kwargs as well. One was about the driver rather than a test: the JSON note sat beside the final print, where it described the document, instead of on the stderr redirection that is what actually keeps stdout clean. * Restore what the reflow cut, including three markers it mangled Nine sites. Most are the trim's own truncations: the dill fixture had lost the property that makes lookup by reference fail, so an otherwise unexplained __module__ = "builtins" sat there with no reason; the rope drift test had two clauses spliced into "every buffer keeps scaling in a buffer"; the playwright guard had lost the three command shapes it matches and why the regex is not anchored on an engine list. Three are a shape I had not seen. The reflow rewrote the `#:` variable documentation markers in the fast-copy constructs to `# :`, and dropped stray colons into the wrapped prose, leaving "text : node". Those markers are syntax, not punctuation to be tidied. One was misplacement rather than truncation. The dotted-import rationale had moved onto the second copy of its case and lost its tail, leaving a dangling "and" above a capitalised "Still caught" and the first row bare. Restored where base has it, stray deleted. lint_duplicate_definitions self-test still passes, 63 cases. * Put five drifted comments back on the code they describe The trim slid several notes onto neighbouring tests, where they describe something they no longer sit on: - test_qat.py: the TODO moved off _test_full_model_fake_quantize, whose leading underscore is what keeps pytest from collecting it, onto a test that is collected and was never disabled. - playwright_chat_ime_i18n.py: 6f carried a stray copy of 6e's compositionstart note above an Enter KeyboardEvent, and had lost its own rationale for why Enter reports isComposing=false while composingRef is pinned. - test_install_node_prebuilt_logic.py: the short-circuit note landed on the transient-download-failure test, leaving the SHA-enforcement test bare and dropping the carve-out that a different usable version is still kept for offline resilience. - test_chat_autoload_failure_gate.py: the default/namespace note moved onto the named-import loop, and the mixed-form note explaining that regex's optional prefix was dropped. - test_frontend_dep_removal.py: the unquoted CSS url() note moved onto the quoted fixture, and the HTML package-name-boundary note moved off the script-src case onto the link-href case. Comments only; the gate reports code unchanged on all five files. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: danielhanchen <michaelhan2050@gmail.com>	2026-09-04 15:31:50 -07:00
oobabooga	f1f95f71fa	Installer: replace an existing venv whose interpreter is gone (#9501) ... * Installer: replace an existing venv whose interpreter is gone * Installer: clear an empty venv dir before the legacy migration Simulating this branch against every install layout Studio has produced turned up one shape it does not cover, and two gaps in the new predicate. An empty $VENV_DIR next to a healthy legacy $STUDIO_HOME/.venv still wedges. _dir_has_entries correctly answers false for an empty directory, so the chain falls through to the migration, and `mv` into a directory that already exists nests the environment as $VENV_DIR/.venv rather than renaming it. uv then refuses the occupied target with the same error as #9479, on every re-run. This reproduces identically on the merge base, so it is not a regression here, but it is the same bug and the same wedge. Clear the directory first, with rmdir rather than rm -rf so it can never take one that gained an entry since the check, and unlink a symlink without following it to its target. install.ps1 had the same two Move-Item calls, for the legacy .venv and for the CWD-relative layout. PowerShell's provider rewrites the destination when it is an existing container, so Windows nests exactly the same way, and -Force does not change it. Clear-MigrationTargetDirectory does the same job there. Directory.Delete unlinks a reparse point without following it, but throws on a link to a file or a dangling link, so those fall back to File.Delete. Both call sites report through Exit-InstallFailure, which a bare throw would have skipped along with the Tauri error log and the TMP restore. _dir_has_entries reported a directory empty when it could not be enumerated, which is a directory uv still refuses, so the repair loop stayed stuck. Treat it as occupied, matching install.ps1's enumeration catch; the rename only needs write on the parent. Both halves matter: without read the globs cannot expand, and without search the -e tests fail on every name. The helper also depended on the caller's pathname expansion, so a caller with set -f saw every occupied directory as empty and silently got the old behaviour. Save and restore the flag around the globs, mirroring _path_has_dir. Also report a failed rollback move instead of dying under set -e with nothing on screen but mv's stderr, which is what install.ps1 already does. Still not covered, unchanged from before: $VENV_DIR as a regular file. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Installer: match the venv predicate to what uv actually refuses Codex is right that a dangling $VenvDir link is skipped and then wedges. It is one of three shapes the predicate gets wrong, all from the same cause: it asks "is this a non-empty directory" when the question is "will uv refuse this path". Measured against uv 0.12.1, the version install.sh pins: absent exit 0 symlink to empty dir exit 0 empty directory exit 0 symlink to occupied dir exit 2 occupied directory exit 2 symlink to a file exit 2 regular file exit 2 dangling symlink exit 2 So uv creates only into a path that is absent or resolves to an empty directory. mkdir(2) explains the rest: EEXIST is "pathname already exists (not necessarily as a directory)", which the man page states covers a symbolic link "dangling or not". uv reports the dangling case as `failed to create directory: File exists (os error 17)`, not the "already exists" message, which is why it reads like a different bug. _dir_has_entries led with `[ -d "$1" ] || return 1`, and -d follows the link, so a dangling link, a link to a file, and a plain file all answered "empty" and skipped the replacement. Test-DirectoryHasEntries had the same hole through -PathType Container; Get-Item -Force sees the link itself. Both now answer on all eight shapes exactly as uv does, verified by executing each helper against every shape rather than by reading the source. This also closes the regular-file case that was left open earlier, since it is the same fault. * Installer: tighten the venv replacement comments Comment-only pass over the lines this branch adds. The _dir_has_entries and Test-DirectoryHasEntries headers also still described the old predicate, which answered whether the path was a non-empty directory rather than whether uv would refuse it. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Installer: restore rollbacks that are not directories Both Codex items are right, and both are fallout from widening the predicate. Whatever _dir_has_entries now calls occupied can end up under the rollback name, including a regular file and a symlink with no target, but the restore still tested for a directory. _restore_studio_venv_replacement gated on [ -d "$_VENV_ROLLBACK_DIR" ], so for those two shapes it deactivated the rollback and returned. Reproduced: with $VENV_DIR a regular file or a dangling link, a failure after the move leaves the half-built venv at $VENV_DIR and the original stranded under unsloth_studio.rollback.<stamp>.<pid>. Now -e/-L, matching the predicate. _commit_studio_venv_replacement had the same test, so on success the backup was never cleaned up either. install.ps1 is the same bug through Test-Path, which follows a link and reports a dangling one absent. Test-StudioPathPresent asks Get-Item instead, and Restore-StudioVenvRollback and Complete-StudioVenvRollback both use it. Remove-StudioVenvTreeWithRetry keeps Test-Path: its post-check only asks whether the removal happened, and Remove-Item unlinks a dangling link fine. tests/python/test_windows_python_venv_hardening.py extracts a fixed list of functions to run under pwsh, so the new helper had to join it or Restore-StudioVenvRollback dies on command-not-found. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Installer: tighten the rollback comments Final comment-only pass over the lines the last two rounds added. --------- Co-authored-by: danielhanchen <unslothai@gmail.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-22 22:12:39 -07:00
Daniel Han	827d25931b	Stop 19 test files racing on one PowerShell startup cache (#9371) ... * Stop 19 test files racing on one PowerShell startup cache Backend CI run 32341628757 on `1c3dde199` finished `284 failed, 8498 passed`. Every one of the 284 was a pwsh subprocess ending `died with <Signals.SIGABRT: 6>`, across 19 files that all read as Windows-installer regressions. None of them were. 222 of the aborts land inside a two-second window, 88 at 07:09:30 and 133 at 07:09:31, which is a mass kill of every live pwsh rather than independent per-test flakiness. The cause ------------------------------------------------------------------------ Every `-NonInteractive` startup reads and rewrites an ~83 KB `$XDG_CACHE_HOME/powershell/StartupProfileData-NonInteractive`, and XDG_CACHE_HOME defaults to `$HOME/.cache`. Under `-n 4` all four xdist workers share one HOME, so the whole job's pwsh processes race on one file and a startup that deserialises a half-written one dies before it reaches our script. `Stack overflow.` is .NET's failfast, which cannot unwind a blown stack, so it prints one line and calls abort(); that is the SIGABRT (PowerShell/PowerShell#24461). Measured twice, independently, 4000 startups per arm: run 1 shared cache dir 7/4000 died {-11: 3, -6: 4} private cache dirs 0/4000 run 2 shared cache dir 11/4000 died {-11: 10, -6: 1} private cache dirs 0/4000 Three distinct crash shapes appeared, and each names the torn file rather than our scripts: `Stack overflow.`, `System.IO.FileLoadException: The given assembly name`, and `System.ArgumentException: String cannot have zero length.` 18 deaths in 8000 shared startups, 0 in 8000 private. CI agrees from the other direction. Of the pwsh-heavy files in that run, exactly one had zero failures, tests/test_windows_amd_gpu_scan_fallback.py, and it is the only one that hands its child a private HOME, across roughly 80 startups where the run's own rate predicts about 16 failures. What is NOT established ------------------------------------------------------------------------ Neither experiment reproduces CI's rate. Roughly 20% of pwsh startups died there against 0.2 to 0.3% here, and at CI's actual `-n 4` on this box I measured 0/1200 in both arms: the race needed 48-way concurrency before it appeared at all. The likely reason is that four workers on a 4-core runner are in real contention while four threads on a 192-core box almost never overlap in the critical section, but that is reasoning and not a measurement, so treat the mechanism as proven and the magnitude as unexplained. That is also why this does not stop at removing the shared file. Three layers, in order ------------------------------------------------------------------------ 1. Remove the contended resource. One cache directory per xdist worker, fresh per session. Workers run their tests one at a time, so within a worker the startups stay sequential and the cache still does its job warm; across workers the directories are disjoint and there is nothing left to race on. Fresh rather than a stable path, because a cache torn by an earlier run would otherwise poison every later session on the same box. 2. Retry a run that produced no verdict. Three attempts, unslept, because the trigger is process startup rather than a resource that frees up. 3. Attribute what is left. A crash raises PwshInterpreterCrash naming the interpreter. Layer 1 is the fix; 2 and 3 exist because of the unexplained magnitude above. Deliberately NOT done: bounding pwsh concurrency with a lock, or giving up `-n 4`. The workflow records 806.1s to 219.7s from that flag, and the contended resource can be removed rather than rationed. The rule that keeps this honest ------------------------------------------------------------------------ A signal is not a verdict, so retrying it papers over nothing: the script never ran to its end. A normal exit is returned untouched on the first attempt whatever its code, so a pwsh that runs and gives the WRONG answer still fails with its own message. Getting that second half wrong would turn this into a way to retry real regressions into green, which is worse than the bug it fixes, so both directions are executed in tests/studio/test_pwsh_interpreter_crash_attribution.py against a real SIGABRT rather than reviewed. Mutation-tested: relaxing the crash test from `returncode < 0` to `returncode != 0` fails test_a_clean_run_with_the_wrong_answer_still_fails_with_its_own_message and test_a_clean_run_is_not_retried, which are exactly the two that guard that direction. This also generalises `_run_pwsh` from tests/studio/test_install_phase_timing.py, added earlier today for a second, signal-free shape: pwsh printing its "The PowerShell process will exit" banner and exiting normally with empty stdout. That one cannot be seen in the exit status, so it stays a text match. Verified ------------------------------------------------------------------------ tests/python/test_windows_xformers_installer.py, tests/studio/test_install_phase_timing.py, tests/studio/install/ and the new guard: 2635 passed, 3 skipped. Guard alone: 5 passed. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Drop the subprocess import the pwsh conversion left behind Source lint's import-hoist check is right: every subprocess.run in test_windows_xformers_installer.py became run_pwsh, so `import subprocess` has no references left except the one inside a comment explaining why run_pwsh is used instead. Its wording names the shape exactly -- "was used before, now unused (references re-pointed)" -- which is what a mechanical call-site rewrite leaves behind. Swept the other 18 converted files the same way with an AST pass rather than by eye. This was the only real one: the remaining hits are `from __future__ import annotations`, which every such scan reports, and a PropertyMock in test_rocm_support.py that is present on main unchanged. 42 passed. * Suppress the core dump on the forged SIGABRT tests/test_deliberate_crashes_suppress_cores.py caught this: the abort child had no PR_SET_DUMPABLE=0, so each of these aborts piped a multi-MB core to apport before the child could be reaped. The guard is right and its message names the fix. The child still exits -6 and PR_GET_DUMPABLE reads 0, so all five verdicts are unchanged. Linux-only and non-fatal elsewhere: Windows has no CDLL(None) and pipes no core, so arming it there would trade a no-op for a lost test. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: danielhanchen <unslothai@gmail.com>	2026-08-20 04:22:41 -07:00
Daniel Han	a151ac875c	Make install.ps1 work with the user's PowerShell profile loaded (#8161) ... * Make install.ps1 work with the user's PowerShell profile loaded Installing from a normal console failed where the same install from a console started with -NoProfile succeeded. A profile runs before `irm https://unsloth.ai/install.ps1 | iex` does and shares its scope, and that entry point has no script file to re-launch without it, so the individual couplings are cut instead. install.ps1, at the top of Install-UnslothStudio: - Set-StrictMode -Off. The script tests environment variables that are legitimately unset and reads $script: state only some branches assign, both of which a profile's `Set-StrictMode -Version Latest` turns into terminating errors. - $PSDefaultParameterValues is filtered down to proxy keys. An entry like 'Start-Process:WindowStyle' silently rebinds cmdlets here and fails the install with an error naming none of it. Proxy entries are kept because they can only ever enable a download, and on a locked-down host may be the only route to python.org and the uv release. - $PSNativeCommandUseErrorActionPreference = $false. With a profile turning it on, the "Stop" preference makes a failing native command throw out of the `unsloth studio setup` handoff instead of reaching Exit-InstallFailure, skipping rollback and the Tauri error record. All three assign without a scope qualifier, so they apply to the installer and everything it calls and leave the caller's session alone. uv is resolved once through Resolve-UvExecutable, which uses `Get-Command uv -CommandType Application -All` and falls back to the bare token when nothing is on PATH. PowerShell ranks aliases and functions above PATH, so a profile `Set-Alias uv ...` was answering the version probe and ending the install at "uv could not be installed" on machines that had a working uv. Test-UvVersionOk pins the executable that answered in $script:UvExe, and the 27 install scriptblocks invoke that path. $script:UvExe and $script:UvInstallDestDir are reset per invocation, since $script: is the caller's session under irm | iex. unsloth_cli/commands/studio.py passes -NoProfile to setup.ps1 unconditionally. It was only added when stdout was not a tty, which is never the case for the console install this fixes, so setup.ps1 ran under the profile with its own bare uv calls exposed. tests/test_installer_profile_hardening.py runs the extracted prologue and uv probe under a hostile profile and checks the caller's session is left intact. The four existing tests that anchored on the literal `uv venv $VenvDir` are re-anchored past the command token. * Plant the real-profile fixture where pwsh actually looks test_a_real_profile_reproduces_the_same_state failed on ubuntu-latest with every probed setting at its default, meaning the planted profile never loaded. It passed here and on macos-14. PowerShell resolves $PROFILE from $XDG_CONFIG_HOME when that is set and only falls back to $HOME/.config when it is not, and GitHub's ubuntu image writes XDG_CONFIG_HOME into /etc/environment. The fixture redirected HOME alone, so on a hosted runner the inherited value went on naming the real account and the profile was written to a path pwsh never opened. Setting XDG_CONFIG_HOME to the same directory HOME already implies reproduces the CI failure exactly on this machine, and removing it makes the failure go away. _hostile_env now redirects XDG_CONFIG_HOME alongside HOME, so the two rules agree whichever one the host applies, and the test asks pwsh for the path instead of hardcoding the fallback branch. The two guards are precise rather than blanket: a machine-wide profile loads into the real leg only, and a $PROFILE that lands outside the fixture cannot be planted into. Neither is reachable on Linux or macOS with the redirect in place. The other tests in the file never shared the premise; every other pwsh launch there passes -NoProfile and dot-sources the profile explicitly. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Close the uv wrapper hole, harden winget the same way, and restore module autoloading Validating the profile hardening against real pwsh turned up three things the first pass missed. Six of the nineteen hostile-profile scenarios I exercised are genuinely broken on main and genuinely fixed by this branch, so the shape of the fix is right -- these are gaps in its coverage, not a change of direction. Resolve-UvExecutable still handed back the bare token when nothing named uv was on PATH. That was meant to keep a working non-Application uv working, but it reopens the exact hole the function exists to close: a profile `function uv { Write-Output "uv 99.0.0" }` clears the version gate, gets pinned into $script:UvExe, and then receives every install command the script runs, with the user's torch, index URL and venv path as arguments. The existing test missed it because its hostile alias reports no version at all, and an alias to a missing file fails loudly. Follow an alias as far as an Application and return that resolved path, since aliasing uv at a specific build is a legitimate thing to do; return $null for anything else, which puts the caller back on its install-uv branch and the gate re-probes against the real thing. winget had the identical defect and was left untouched. It is detected with a bare Get-Command and invoked as a bare token at five sites, and it is what installs both Python and uv -- so a `function winget` wrapper, which people write to inject --accept-* or pin a source, owns the whole bootstrap. Same treatment: resolve once to an Application and invoke through the path. A profile setting $PSModuleAutoLoadingPreference to 'None' is fatal here and was not covered. PowerShell 7 loads no modules at startup, so that one line removes Test-Path, Write-Host, Select-Object, ConvertFrom-Json, Get-FileHash, Invoke-WebRequest, Expand-Archive, Start-Process and Get-Content, and the script dies on its first step naming a cmdlet the reader assumes is always there. Windows PowerShell 5.1 preloads Utility and Management and survives, which is exactly what makes this reproduce on one machine and not another. Also: use [regex]::IsMatch in the defaults filter so it leaves no $Matches behind; add -NoProfile unconditionally in _refresh_desktop_shortcuts, which launches install.ps1 and had it gated on the hidden branch, so the visible console path -- the one where a profile IS loaded -- was the one that missed it; and record that the preserved proxy defaults do not reach setup.ps1, which is launched with -NoProfile, along with why that trade is accepted. Tests: the assertion that the bare token must come back now asserts the opposite, and there are new ones for a convincing uv function, an alias to a real uv, the winget call sites and the autoloading reset. The two new pwsh tests execute against a genuinely planted profile rather than reading source. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Carry the profile proxy across the setup handoff, not just inside install.ps1 install.ps1 deliberately keeps proxy-shaped $PSDefaultParameterValues entries out of the profile table it discards, because on a locked-down corporate host that entry can be the only route out. Adding -NoProfile to the setup launch unconditionally then threw them away one process later, and setup.ps1 downloads on its own: the VC++ runtime through Invoke-WebRequest and the uv installer through Invoke-RestMethod. A PowerShell variable does not cross a process boundary, so the kept entries travel as JSON in _UNSLOTH_PS_PROXY_DEFAULTS and the child re-applies them before running setup.ps1. Nothing else from the profile comes with them. A credential is left behind on purpose: PSCredential does not survive ConvertTo-Json, and the environment is the wrong place for one. A stale variable is cleared when there is nothing to hand off. Three tests, one static and two driving real pwsh, covering the round trip, the credential and non-proxy keys being dropped, and the prelude staying silent when the variable is absent, empty or corrupt. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Harden the proxy handoff: ordering, key casing, uri values, scope, and the standalone update Five follow-ups, all on the handoff added last round. The handoff serializes with ConvertTo-Json, from Microsoft.PowerShell.Utility, and ran before the module-autoloading reset. Under a profile's $PSModuleAutoLoadingPreference = 'None' a fresh PowerShell 7 session therefore died right there, taking out the one configuration the handoff exists to support. The reset moves to the front of the prologue. The key filter was a case-sensitive .NET regex, but cmdlet and parameter names bind case-insensitively, so 'invoke-webrequest:proxy' was dropped. And [uri] is the type the Proxy parameter actually takes, so a careful profile assigns one; the serializer accepted only string and bool, and it disappeared at the process boundary. Both are accepted now, a uri by its AbsoluteUri. A PSCredential is still deliberately left behind. Under "irm ... | iex" the prologue runs in the caller's own session, so writing the environment variable there outlived the install on every path, early returns included, and a later `unsloth studio update` from that console would reapply stale JSON over a proxy that had since changed. The prologue now holds the value and it is published around the setup child only, saved and restored beside the other child-scoped variables. A standalone `unsloth studio update` has no installer above it, so there was nothing to restore and -NoProfile left it with no route out. It now asks: a throwaway PowerShell that does load the profile prints just the proxy-shaped defaults as JSON, validated before use, entirely best effort. Same filter as install.ps1's. Five tests, two driving real pwsh, including one against a profile with strict mode on, autoloading off, a lowercase key and a uri value. * Ask the profile the caller actually has, and follow a uv alias first - the standalone update probed powershell.exe only, so a proxy living in the PowerShell 7 profile never reached the -NoProfile child; both editions are asked now, the caller's first, and their answers merged. - Resolve-UvExecutable checked PATH before the alias, which is the reverse of PowerShell's own resolution and made the alias branch unreachable on any machine with some uv on PATH. - the parity workflow did not run this suite when unsloth_cli/commands/studio.py changed, though the suite asserts that module directly. Its own path-filter parser also treated a comment inside the list as the end of it, which would have hidden the addition. * Give the parity job the imports it needs, and fold proxy keys the way PowerShell does Three tests in the profile-hardening suite import unsloth_cli.commands.studio to drive the profile probe directly, and that pulls typer, pyyaml, pydantic and click. The job installed pip and pytest only, so on a clean setup-python both matrix legs died with ModuleNotFoundError before a single test ran. Installed, with a test that keeps the step in step with what the suite imports. $PSDefaultParameterValues keys are case-insensitive and a Python dict is not, so "Invoke-WebRequest:Proxy" from the caller's own host and "invoke-webrequest:proxy" from the other one both crossed over; the prelude then replayed them in order and the lower-priority host's value landed last, reversing the earlier-host-wins rule this merge exists for. Keys are folded now, first spelling seen wins, within one profile's answer as well as across two. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Frame the proxy record, and quote the union so 3.9 can still import the CLI The probe runs after the profile, and the profile is free to print: a MOTD, a "loading personal and system profiles took 812ms" line, a corporate banner. With the record bare, that arrived ahead of the JSON, the parse threw and the whole answer was dropped -- so the locked-down host that needed the proxy handed the -NoProfile child nothing and every download failed, which is worse than before, since the old visible-console path loaded the profile itself. The record is emitted between two markers now and cut out of whatever else was said. And `str | list[str]` was evaluated at def time in a module with no postponed annotations, so on the 3.9 this project still supports it raised TypeError and took the whole CLI import with it. Quoted, with a test that walks every annotation in the module for an unquoted PEP 604 union. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Let the uv gate move past a stale alias, and decode the probe output lossily An alias pointing at a real but stale uv was the only binary the version gate ever probed, so a current uv already on PATH -- or one winget or the pinned release had just installed -- could not rescue the run and the install ended at "uv could not be installed" on a machine that had one. The resolver hands back every candidate in the order the bare token would pick them, alias first, and the gate walks them until one passes, pinning the one that answered. The profile probe decoded its child with text=True alone, which is the locale codec with STRICT errors. A UTF-8 banner on an ANSI console then raised UnicodeDecodeError, which is neither OSError nor SubprocessError, so it escaped the handler and took the update down before the -NoProfile child ever ran -- and before the framing could discard the banner. UTF-8 with replacement now; the record itself is ASCII. rich is named in the parity job's install line too. It arrives through typer today, but unsloth_cli imports it directly, and this suite's imports should not rest on somebody else's dependency list. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Assert the proxy handoff by parsing it, not by substring CodeQL reads the bare membership test as an incomplete URL sanitization, which is a fair reading of the shape even though this is an assertion on a compressed JSON payload rather than a check on untrusted input. Parsing it and comparing the value exactly is the stronger assertion anyway. * Read the caller edition by order, pin the probe's encoding, claim cmdlets whole A machine can carry both PowerShell module trees on PSModulePath at once, so inferring the caller from the absence of the other edition handed precedence to the wrong profile and let its proxy override the console the command was typed into. Each host puts its own module directory first, so the earliest tree names the caller; neither present keeps the previous order. Windows PowerShell 5.1 writes redirected output in the console code page while this process decodes UTF-8, so a non-ASCII proxy value came back with replacement characters, still parsed as JSON, and handed setup a proxy that does not resolve. The probe pins its own output encoding first. And the merge claims a cmdlet whole rather than filling missing companion parameters from the other profile, which built a configuration neither host had -- one profile's proxy with the other's credential forwarding. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Evaluate script-block proxy defaults, read the caller's host profile, drop the secret A profile can set a dynamic default as a script block, which is PowerShell's supported form and which Invoke-WebRequest evaluates per call. Both serializers dropped it, so the caller downloaded fine and the -NoProfile setup child got no proxy at all. Both now invoke the block and hand over the resulting URI or string; executable code does not cross the handoff. The probe spawns pwsh.exe or powershell.exe, which load the CONSOLEHOST profile. A caller in the VS Code Integrated Console or the ISE keeps its defaults in Microsoft.VSCode_profile.ps1 or Microsoft.PowerShellISE_profile.ps1 instead, so the probe reported no proxy on exactly the host that needed one. It dot-sources the caller's other CurrentUser host profiles, from their own directory, before reading the table. And the prelude clears _UNSLOTH_PS_PROXY_DEFAULTS the moment it has read it. A profile proxy routinely carries credentials, and every native process setup.ps1 starts inherited the environment it was launched with. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Probe only the caller's own host profile, on one shared budget Sourcing every Microsoft.*_profile.ps1 in the profile directory ran profiles belonging to hosts nobody was using: they can overwrite the console's own $PSDefaultParameterValues, have side effects, or exit before the framed record is written. The probe now sources exactly one, named by _UNSLOTH_PS_HOST_PROFILE, and only when the caller identifies itself (VS Code does, via TERM_PROGRAM). A host we cannot name gets no extra profile rather than someone else's. install.ps1 removed the handoff variable when it had no proxy to pass, and its absence is precisely how the CLI recognises a standalone update -- so an installer launch, including one started with -NoProfile or by the desktop app, went and reloaded the profiles it had deliberately discarded. It publishes an explicit empty handoff instead, and the CLI keys on presence. And the probe's timeout is one budget for the whole call rather than one per host, so two installed editions with two hung profiles no longer cost twice the documented best-effort delay before setup starts. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Run the proxy probe with -NoProfile and dot-source the caller's own two Without -NoProfile the probe host loaded its own ConsoleHost profile before the script ran, so an unrelated profile could print, rewrite $PSDefaultParameterValues or exit before the record was written -- and it still was not the profile a VS Code caller keeps its defaults in. The child runs with -NoProfile now and dot-sources exactly the two the caller's session would have loaded: $PROFILE.CurrentUserAllHosts and either the host profile named in _UNSLOTH_PS_HOST_PROFILE or $PROFILE.CurrentUserCurrentHost. $PROFILE is fully populated under -NoProfile, since the paths are computed rather than loaded, so this is exact rather than incidental. Checked against pwsh with a fixture profile directory: a VS Code caller picks up its own profile plus the all-hosts one and never runs the console profile's banner, and a plain console caller picks up the console profile plus the all-hosts one. * Probe the all-users profiles too, and clear profile defaults before emitting A machine-managed proxy commonly lives in AllUsersAllHosts on a domain-joined box while the user's own profile never mentions it, so sourcing only the current-user pair reported no proxy on exactly the host that has one. The probe now walks PowerShell's own startup order, all-users first, so the user's profile still gets the last word. The profile's $PSDefaultParameterValues was also still active when the record was serialized. ConvertTo-Json:AsArray = $true is a legitimate setting and turns the payload into a JSON array, which the reader rejects for not being a dictionary. $out already holds copies by then, so the table is cleared first. * Harden the proxy probe against profile overrides, and drop the handoff copy Five fixes from the review round: install.ps1 kept the serialized proxy defaults in $script:, which under the documented irm | iex path IS the caller's session scope, so an authenticated proxy URI stayed readable in that console after the installer returned. Cleared in the same finally that restores the environment handoff. A profile setting [Console]::OutputEncoding overrode the probe's UTF-8 pin, and the parent decodes that stream as UTF-8, so the framed record could come back corrupted. Re-pinned after the last profile is sourced. The record was emitted through bare Write-Output and ConvertTo-Json, which a profile alias or function shadows; clearing $PSDefaultParameterValues does not cover a command override. Both are module-qualified now. TERM_PROGRAM=vscode is set by every VS Code integrated terminal, not only the PowerShell extension's host, so substituting Microsoft.VSCode_profile.ps1 for the current-host profile missed the proxy a plain pwsh terminal there actually has. The named host profile is added rather than substituted, with the current-host profile last. The per-cmdlet ownership check compared command strings literally, so a wildcard key from one host and a literal key for a matching cmdlet from the other were both merged, which is how one invocation ends up configured from two profiles. Overlap is matched in either direction now. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Hold the proxy handoff in the frame, and treat two wildcards as one family Three fixes from the review round: The serialized handoff lived in $script:, which under the documented irm | iex path is the caller's session scope, and the only cleanup ran after the setup child. Dozens of exits return earlier -- ShortcutsOnly, an argument error, lock contention, a failed dependency install -- so an authenticated proxy URI stayed readable in that console. It is a function-local now, which dies with the frame on every path including a throw. install.ps1 serialized that record through a bare ConvertTo-Json, which a profile alias or function shadows exactly as it does in the probe. Module qualified. The cmdlet-ownership check compared two wildcard patterns as strings, and Invoke-Web* and *-WebRequest both apply to Invoke-WebRequest while neither matches the other. Two patterns are now assumed to overlap: the cost is a second host's unrelated wildcard entry going unmerged, against handing setup a credential setting from a profile that never asked for one. * Tighten the profile-hardening comments Comments, docstrings and whitespace only; no code changes. Each comment keeps the reason it records and drops the retelling. * Cut the profile-hardening comments down again Comments, docstrings and whitespace only. The install.ps1 prologue and the studio.py proxy probe kept one causal claim per decision, with the probe's profile-loading essay split into a short note beside each line it justifies. * Pin the probe's add-both profile order in its own test name * Keep disjoint wildcard proxy families, and give each probed host its own module path * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: Daniel Han <moonshotaisubstack@gmail.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-09 04:27:28 -07:00
Daniel Han	07df95079e	Studio: route every Windows installer line through the UTF-8 stdout sink (#8148) ... * Studio: route every Windows setup line through the UTF-8 stdout sink The desktop setup log rendered "?? Unsloth Studio Setup" over a rule of replacement characters. Tauri spawns Windows PowerShell 5.1 with CREATE_NO_WINDOW (install.rs), so the [Console]::OutputEncoding setter throws and both entry scripts rebind [Console]::Out to a UTF-8 writer. step/substep already write only through that writer when stdout is redirected, so they came out right. Every other line did not: Write-Host is written by 5.1's console host with its own writer on the OEM code page, and U+1F9A5 has no OEM form while U+2500 becomes a bare 0xC4, which from_utf8_lossy turns into U+FFFD. The banner and the footer are not steps, so they kept arriving as mojibake, and install.ps1 had neither the IsOutputRedirected probe nor a mirror at all. Add Write-StudioLine above the first write in studio/setup.ps1 and install.ps1: console handle when redirected, Write-Host when interactive, since it is the only writer that colorizes. Rewrite 164 call sites in setup.ps1 and 155 in install.ps1 onto it, including install.ps1's own step/substep. Write-Host now survives only inside helpers that have already ruled out the redirected sink, and the launcher script install.ps1 generates keeps its own, since it runs as a separate process. No behaviour change for an interactive console user: same text, same colors, same single record per line. test_windows_setup_output_encoding.py gains byte-level coverage that the real banner and footer, sliced out of setup.ps1, survive both launch shapes as valid UTF-8 exactly once, plus a source contract that runs on Linux and names any file:line that reaches for Write-Host outside the allow-list. Studio.Setup.Output.Tests.ps1 covers Write-StudioLine in both modes and pins install.ps1's copy to setup.ps1's. Harnesses that splice these scripts apart now stub or dot-source Write-StudioLine: two PowerShell harnesses, one Python harness, and the VC++ redist leg of studio-windows-inference-smoke. pytest tests/python tests/test_installer_*.py: 1077 passed (2 pre-existing sandbox failures unrelated to this change). All 16 tests/studio harnesses and 57 Pester cases pass. Both scripts parse clean. * CI: spawn install.ps1 as a child process so its lines reach install.log * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Stub the output sink in the llama.cpp backend PowerShell harness * Guard the console-less spawn on a Windows runner The byte-level cases in this file run with a console attached, and a GitHub runner gives a CREATE_NO_WINDOW child one, so the UTF-8 setter succeeds there and every version of these scripts emits a clean banner. Those cases cannot tell this fix from what preceded it. Add cases that call FreeConsole() in the child first, which is the state install.rs's own comment assumes CREATE_NO_WINDOW produces. There Write-Host has no screen buffer to query, throws, and takes the script down: 2 bytes of stdout and exit 1 rather than the banner. The probe is assembled entirely out of text sliced from the script under test and spawned with install.rs's own interpreter, flags and creation flags. No Windows job ran this file, so its byte-level half was only ever exercised under pwsh 7 on the Linux Backend CI leg, which is UTF-8 by default. Add it to the cross-platform parity matrix, which already has a windows-latest row and already triggers on install.ps1 and studio/setup.ps1. * Report skips in the parity step A platform-gated case that stopped running on the row it exists for still reports green with -q alone. * Slice the error preference too It is what turns the Write-Host throw into a dead script rather than a skipped line, so restating it would be assuming the result. * Say what the comments actually mean * Make the console-less cases fail on a lost banner, not just a mangled one * Stub the output sink in every harness that splices these scripts The Write-Host rewrite left four spliced-source harnesses reaching Write-StudioLine without defining it. An undefined command is a terminating error, so each one either aborted or was swallowed by the harness's own catch, and the test kept passing while no longer testing anything. - test_windows_python_venv_hardening.py, partial-rollback case: the five-line split-move warning was lost. The assertion that "both halves are named" only stayed green because $existing is a prefix of the rollback dir, so it matched the dir= line instead. Pin it to the warning text. - test_path_probe_access_denied.ps1, ownership guard: the catch scored the command-not-found as the intended failure and never reached Exit-SetupFailure. Pin the check to the EXIT-SETUP message. - test_windows_installer_concurrency_guard.py: the decision block prints before Exit-InstallFailure, so on Windows the active case aborted at exit 1 and never produced RESULT:blocked. - Studio.Setup.Vs2026.Tests.ps1: on a host without cmake, Ensure-BuildToolsForLlamaSourceBuild hits the sink first and the no-op case fails on the throw. Also stub the three remaining harnesses that splice sink-calling helpers but do not reach the sink on the paths they exercise today, so the next case added to them cannot reintroduce this. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-08 06:47:03 -07:00
Tai An	6ea2b91d04	fix(studio/install): keep the rollback reference when a venv move stops partway (#7810) (#7823) ... * fix(studio/install): keep the rollback reference when a venv move stops partway (#7810) Start-StudioVenvRollback moves the existing environment aside with a single Move-Item and treats any failure as "the original is still in place". On Windows that is not the shape the failure takes: an open handle inside the tree -- a running Studio backend holds its own unsloth.exe there -- fails the rename *after* it has already walked part of it, so the entries handled before the locked one sit at the rollback path while the rest stay behind. Both paths then exist, and the catch tests only the source, so it scores the split tree as an untouched original, clears StudioVenvRollbackDir and drops the sole record of where the other half went. The reporter was left with a unsloth_studio\ holding 7 Scripts entries and no python.exe, an intact venv under unsloth_studio.rollback.<stamp>.<pid>\, and no output naming either. Retries cannot recover: the create branch keys off python.exe, and uv refuses to build a venv over the directory the stranded files still occupy. Clear the rollback state only when the destination is genuinely absent. When both paths exist the move is partial, so keep it active -- the existing finally-block Restore-StudioVenvRollback then reverses it -- and print both locations plus the "close Unsloth Studio" hint the launcher-shim path at install.ps1:3078 already gives for the same underlying cause. install.sh is unaffected: POSIX rename ignores open descriptors and both paths are siblings under , so that move really is atomic. Regression test extracts the function from install.ps1 and runs it under real PowerShell with Move-Item stubbed to fail after creating the destination. On current main the partial case reports active=False with an empty rollback dir -- the stranding itself; the clean-failure case is asserted alongside it so the untouched-original path keeps clearing state as before. Signed-off-by: Tai An <antai12232931@outlook.com> * fix(studio/install): merge a split venv back instead of clearing the target Keeping the rollback active after a partway move sent the failure path into Restore-StudioVenvRollback, which removes $target before moving $backup back. In the split case $target is not an incomplete *new* environment -- it holds the half of the previous one the move never reached -- so that removal deleted files present nowhere else and restored a corrupted venv. Flag the split and give restoration a merge path: move each entry of the backup into the target without overwriting, recursing where the move stopped inside a subtree, and only drop the backup once it is empty. Anything ambiguous is left in place and both locations are named. Regression test pins the file that never moved to surviving restoration. * Installer: fix split-move merge nesting siblings and walking through junctions Two problems in Merge-StudioVenvRollbackTree, both only reachable once a partway move leaves the venv split. Sibling nesting. The per-entry variable was named $destination, and PowerShell variable names are case-insensitive, so it reassigned the $Destination parameter. Only the first entry at a level landed correctly; every later sibling was joined onto the previous one's path, so a restored venv came back with pyvenv.cfg inside Lib. Renamed to $entryTarget. Junction traversal. Recursion keyed on "directory on both sides", which a junction or directory symlink satisfies. If the half left behind holds the link, venv files move through it and land outside $StudioHome. If the moved half holds it, the recursion enumerates the link target and pulls those files into the venv, emptying a directory that was never part of the environment. Either way the link is replaced by a real directory, which the whole-tree Move-Item this path replaced never did. Now checks both sides for a reparse point and falls through to keep-both-copies. Attributes are read via Get-Item on both sides, since Get-ChildItem has reported them inconsistently. Tests cover sibling placement and both link directions. The link test uses a junction on Windows so it does not need SeCreateSymbolicLinkPrivilege. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Signed-off-by: Tai An <antai12232931@outlook.com> Co-authored-by: danielhanchen <danielhanchen@gmail.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-06 02:09:52 -07:00
Etherl	1770182b5a	Windows: validate managed Python before package installation (#7763) ... * Harden Windows pyenv interpreter handling * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fix Windows managed Python recovery * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Tighten-installer-comments * Finish the -S probe hardening and run the new tests on Windows for PR #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. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: danielhanchen <unslothshared@gmail.com>	2026-08-03 05:08:47 -07:00