mirror of
https://github.com/unslothai/unsloth.git
synced 2026-08-24 00:04:14 +00:00
124 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Pascal André
|
489fab4a71
|
Support OpenCode V2 in unsloth start (#9275)
* Support OpenCode V2 in unsloth start * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Keep stable OpenCode guide coverage * Follow the OpenCode V2 stable release * Honor OpenCode V2 policy and server semantics * Fix OpenCode V2 launch command recipes --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> |
||
Long Yixing
|
b69bfe0216
|
fix(studio): keep a slow install alive and name what it is downloading (#8805)
A desktop install was killed after two hours of wall clock regardless of progress. On a slow link the cu126 torch wheel takes longer than that on its own, so the install could never finish, and each kill wasted every byte already fetched because uv restarts an interrupted download from zero. The installer also keeps uv's output in a log it only prints on failure, so those two hours looked identical to a hang, both to the user watching and to anyone reading the logs afterwards. install.sh and install.ps1 now turn uv's announcements for downloads of at least 50 MiB into `[TAURI:DL]` / `[TAURI:DL_DONE]` protocol lines, while the per-package chatter stays in the log where it was: a full dependency set is dozens of announcements plus a line per installed package, which would bury the installer's own output. The app consumes the markers without displaying them, so a healthy install looks exactly as it did before. Once five minutes pass with nothing printed it reports the step, the package being fetched, its size and elapsed time, and it now gives up only at a twelve-hour backstop. Markers travel on stderr from install.sh, alongside the other protocol lines that function already writes there. That also keeps them clear of the verbose path's redactor, whose sed block-buffers its output: a marker queued behind it would reach the app only once the download it announces had finished. install.ps1 writes them on stdout, so both reader threads filter them out of the UI. Where awk is absent, which this script already supports elsewhere, the filter degrades to plain capture rather than closing the pipeline under the child. There is deliberately no rule that stops an install for being quiet. That needs evidence that work is happening, and the markers cannot supply it everywhere: the uv calls under `studio setup` capture their output rather than streaming it, so a silence rule would stop healthy installs in exactly the phase this change exists to protect. Extending the markers to those wrappers is follow-up work. Validated by driving both installers' real command wrappers against recorded `uv pip install` output -- `run_install_cmd` under sh, and `Invoke-InstallCommand` under PowerShell across both its quiet and verbose arms -- plus a timing check that a marker is readable while its download is still running, and the existing installer and watchdog suites. Both installers are pinned to the same 50 MiB threshold, so they cannot drift apart. Refs #8698 |
||
Daniel Han
|
1b48147d8e
|
Windows: stop depending on the generated unsloth.exe console script (#8592)
* Windows setup: install uv from a pinned release instead of running remote script text
studio/setup.ps1 piped astral's install.ps1 straight into Invoke-Expression. That
download-and-execute shape is the single construct AMSI providers and cloud ML
scanners score hardest, and install.ps1 already replaced it with a pinned-SHA-256
archive download. Port the same implementation across.
Progress goes to the pipeline rather than the console, so the quiet path swallows
it exactly as it swallowed astral's installer output and the printed lines around
the call site are unchanged.
* Windows: stop pairing a hidden window with a bypassed execution policy
The Studio shortcut launched launch-studio.ps1 with -WindowStyle Hidden and
-ExecutionPolicy Bypass on the same command line. That pair is what Microsoft's
own detections key on, and studio/src-tauri/src/install.rs already refuses it for
the app's own launch of install.ps1.
The installer writes launch-studio.ps1 itself, so the file carries no
mark-of-the-web and RemoteSigned loads it. The hidden window is unchanged, so the
shortcut behaves exactly as before. The generated launcher's own child launch
moves to RemoteSigned for the same reason: it runs an inline -Command against an
executable, where no script file is loaded and the two policies are equivalent.
Also refresh a stale comment in studio/setup.ps1 that attributed the PSModulePath
fix to astral's uv installer, which no longer runs in-process.
* Installers: keep download-and-run command lines out of the shipped script text
AMSI scans install.ps1 in full before a single line of it runs, and generic
script classifiers read install.sh the same way inside the Linux bundle. Both
headers rehearsed the piped web one-liner five times over, plus a scriptblock
form and an execution-policy bypass, none of which anything in the scripts reads
and all of which the README already documents.
Point at the README instead and reword the in-body comments that quoted the
one-liner as shorthand. Every printed line is untouched: the remediation text the
installers show users still spells out the command in full.
Same treatment for scripts/uninstall.ps1's header.
* Windows: resolve process image paths with one Win32_Process query
install.ps1's venv-holder probe opened a handle to every running PID through
inline C# compiled at runtime. Opening a handle per process is a shape AV
heuristics score hard, and it bought nothing: Win32_Process reports
ExecutablePath for exactly the processes those handles could be opened against,
and answers for all of them in a single query instead of once per PID.
The remaining file-canonicalisation imports stay -- handle-based resolution of
linked ancestors has no faithful Windows PowerShell 5.1 equivalent, and it runs
on security-relevant paths.
Falls back to the per-process .Path when the query is unavailable, so a degraded
WMI repository degrades exactly as the old code did on a process it could not
open.
* Desktop: say who blocked the install when AMSI stops the script
PowerShell hands the whole top-level script block to AMSI while compiling it, so
a security product's verdict arrives as a parse error over the entire file before
install.ps1 runs a statement: no [TAURI:ERROR] marker, no phase log, and a stderr
tail the user cannot act on. unsloth#8523 shows what that looks like in the UI --
"Installation failed: + FullyQualifiedErrorId : ScriptContainedMaliciousContent".
Recognise the two stable error ids on either stream and append what the user
actually needs: nothing was installed, nothing was changed, it is a false
positive, update definitions and retry, do not turn off endpoint protection. The
raw id stays in the message, because the diagnostics report and any vendor
submission both need it.
Matches the id, never the message text, which is localized, and tolerates the
cmdlet suffix the Invoke-Expression form carries.
* Desktop: ship each bundle only the installer it can run
resolve_install_script picks install.sh on unix and install.ps1 everywhere else,
but the shared Tauri config bundled both into every target. The Linux AppImage
therefore carried 280 KB of Windows PowerShell it can never execute -- and it is
the largest script body a generic classifier walking the squashfs reads, which is
where Microsoft's Trojan:Script/Wacatac.B!ml verdict on 0.1.701-beta landed.
Move the resource map into the per-platform configs. The clean-machine job
already fails when a Linux bundle ships no install.sh; it now also fails when one
ships install.ps1, so the split cannot silently regress in either direction.
The .deb scanned clean with the same payload, so this is surface reduction rather
than a proven fix for that verdict.
* POSIX installers: install uv from a pinned release before falling back
install.sh downloaded astral's install.sh to a temp file, ran it and deleted the
file; studio/setup.sh piped it straight into a shell. Both are, shape for shape,
what a dropper does, and generic ML script classifiers score them accordingly --
the 0.1.701-beta Linux AppImage came back Trojan:Script/Wacatac.B!ml while the
.deb carrying the same scripts came back clean.
Fetch the pinned release archive and verify a hardcoded SHA-256 instead, matching
what install.ps1 already does on Windows. Only the four mainstream targets are
pinned: musl, armv7 and any host without a digest tool keep the path they have
today, because guessing a target triple wrong would break the install outright
and that costs far more than the heuristic score of the fallback.
Destination, PATH handling and every printed line are unchanged, so a host that
takes either path ends up in the same state it did before.
* tests: pin the installer shapes antivirus heuristics score
One file collecting what was removed, so it cannot drift back: no remote script
run in-process, no encoded or base64 payload, no hidden window paired with a
bypassed execution policy, no handle opened against another process, and no new
runtime-compiled native import outside an allowlist that carries a reason for
each entry that stays.
The last test is the other half of the contract. Hardening must not change what a
user sees, so the remediation lines the installers print -- which still spell out
the web one-liner in full -- are asserted verbatim. Removing the one-liner from
comments is the point; removing it from what the user is told to run would be a
regression.
Runs on the existing discovery-based pytest step, no workflow list to update.
* release: emit a false-positive submission packet for whatever gets flagged
The build job assembles a Microsoft submission packet, but only for the Windows
-setup.exe. The detection that actually arrived on 0.1.701-beta was
Trojan:Script/Wacatac.B!ml on the Linux AppImage, so nothing was produced for the
one asset that needed it.
The VirusTotal job already knows which assets were flagged and by which engines,
so put the packet there: hash, size and both portals, for every flagged asset
whatever platform it came from, with a note that clearance is per hash and per
vendor. Engine names are not repeated -- they are third-party text and already
appear escaped under Flagging engines.
The gate stays advisory; this only makes acting on it take seconds.
* Revert "Windows: resolve process image paths with one Win32_Process query"
This reverts commit
|
||
|
Daniel Han
|
715535d1c5
|
Windows: start the backend from a usable folder on login autostart (#8575)
* Windows: start the backend from a usable folder on login autostart "Run Unsloth at login" registers the desktop through an HKCU Run value, which cannot carry a working directory, so Windows starts the app in C:\Windows\system32. Every `unsloth` CLI child inherited that folder, and the CLI refuses to run there, so a reboot produced a tray icon and no server (issue #8510). Desktop side: pick the working directory explicitly for every CLI child (backend, both preflight probes, the install check, auth provisioning, update, installer) instead of passing on whatever the launcher gave us. The inherited folder is kept whenever it is usable, so ./models and other cwd-relative defaults resolve exactly where they used to; only a Windows system folder is replaced, with ~/.unsloth. A home that cannot be reached at all now reports working_directory_unavailable rather than looking like a broken install, which stops the pointless automatic repair and gets its own message in the UI. CLI side: move the System32 guard into unsloth_cli/_system_dir_guard.py and run it before the command modules import, since commands.studio resolves STUDIO_HOME at import time. The commands the desktop itself issues take no path from the user, so they move to a safe folder and carry on; everything else keeps the hard error, since relocating a command would silently rebase the relative paths its caller typed. This half fixes anyone whose installed desktop build predates the change, without waiting for a desktop release. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Only trust a Windows directory that holds System32, and tell an absent profile from an absent install Two review findings on the working-directory work. WINDIR is an ordinary variable, so pointing it at the user's own profile made windows_roots() treat that profile as a Windows installation: an ordinary project folder underneath it then looked like a system folder, the fallback rejected the home for being "inside the Windows directory", and the backend could not start anywhere on that machine. Candidates are now checked rather than trusted, and a directory only counts if it actually contains System32. With nothing on the machine looking like Windows, the check falls back to SystemRoot or the default, never to the settable value. Same fix on both sides, since the CLI guard reads the same variables. The managed install lives under the user's profile, so a profile that is not mounted yet makes find_unsloth_binary() return None and preflight reported NotInstalled before the working-directory check could run. That is the exact case the check was added for, and it was sending those users to reinstall. Check whether the home is reachable before turning a failed lookup into "not installed", and report it as its own state with no binary path and no repair offered. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Tighten the comments added by this PR * Keep the preflight message choice testable The branch that tells an unreachable profile from an outdated install sat inline in use-tauri-backend.ts, which pulls in React and the Tauri APIs and so cannot be imported from a test. Move the choice into its own module and drive it directly, so a new backend reason cannot silently land in the stale-install bucket again. * Pin relative path overrides before relocating out of a system folder Studio resolves UNSLOTH_STUDIO_HOME and the cache overrides with Path.resolve(), which anchors a relative value to the working directory. So `UNSLOTH_STUDIO_HOME=.\custom unsloth studio update` from System32 moved first and then resolved the override against the new directory, silently targeting a folder the caller never named. That is the one thing this guard is supposed not to do to caller-supplied paths. Absolutise the relative overrides against the original directory before the move, so they keep meaning what they meant. A ~ value is left alone, since expanduser does not consult the working directory, and an environment that cannot be pinned is one we refuse to move underneath. Covers the Studio home pair plus the cache and llama.cpp overrides, which resolve the same way: UNSLOTH_LLAMA_CPP_PATH, UNSLOTH_COMPILE_LOCATION, HF_HOME, HF_HUB_CACHE, HUGGINGFACE_HUB_CACHE, HF_XET_CACHE. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Pin every relative path override and withhold repair on an unreachable profile - pin_relative_overrides() missed the Studio documents, projects and sandbox roots and the sd.cpp/whisper.cpp/llama.cpp engine paths, so a relative value would have been retargeted by the move, and the remaining single-path cache overrides are pinned for the same reason - an owned or ownerless-spawned backend that is stale no longer offers auto repair when the managed profile is unreachable: the repair runs through the same profile and stops a backend that still answers * Pin relative overrides on the desktop side too, and align the home checks - the desktop moved a CLI child out of a system folder without rewriting the relative path overrides it inherited, so the same install placed state in a different folder depending on whether the desktop or the CLI guard did the move; both layers now anchor those values to the directory being left, with a test that fails if the two lists drift apart - pin the diffusion cache dirs, OLLAMA_MODELS, DG_VISUAL_BIN and UNSLOTH_DG_SHIM, which are resolved against the working directory as well - home_dir_available() accepted a home the working directory resolver then rejected, so a SYSTEM account was offered an install that cannot start; both now go through usable_home_dir() * Report an unreachable profile as the reason a stale backend cannot repair Withholding auto repair was not enough: the result still carried the backend's own reason, so the frontend advised running the update, which needs the same profile the probe could not reach. Both stale paths now report working_directory_unavailable, and the backend's reason goes to the log. * Resolve drive-relative overrides through the OS before moving A value such as HF_HOME=D:cache names the current directory on drive D, so joining it to the folder being left hands it straight back and the move retargets it. Both layers now ask the OS to resolve it first, GetFullPathNameW through ntpath.abspath and std::path::absolute, which is what tracks each drive's own directory. The CLI guard refuses to move at all if that resolution fails, rather than moving and silently changing where the value points. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Pin the stable-diffusion binary overrides and the rest of the cache family SD_CLI_PATH and SD_SERVER_PATH are the highest-priority binary locations for the sd.cpp engine, so a relative value pointed somewhere else after the move. Added alongside the llama.cpp and whisper.cpp equivalents, together with the HF and XDG names that belong to the same families as the ones already pinned. * Pin the GPU SDK roots, and do not relocate into a missing or shared profile - CUDA_PATH, HIP_PATH, HIP_PATH_57 and ROCM_PATH are joined with bin/ for DLL discovery, so a relative value pointed elsewhere after the move - a profile that has not mounted yet still has a writable parent, so makedirs built an empty second profile that would shadow the real one when it arrives; the guard now requires the home to exist, as the Rust resolver does - the public profile is refused whichever variable named it: allow_public only kept PUBLIC out of the candidate list, so a USERPROFILE or ~ that resolves there still put one account's state in a folder shared by every account * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Tighten the comments added since the last pass * Cover running the working-directory fix more than once Both halves run repeatedly over state an earlier run touched: the desktop resolves the directory on every spawn, the guard runs in every CLI process, and a child's environment reaches its own grandchildren. Two tests pin the fixpoint, one for a command configured twice and one for a value that has already been anchored. * Pin STUDIO_LOCAL_REPO, and blame a lost profile for the probe that failed - "studio update" is one of the commands that relocates, and it resolves a relative STUDIO_LOCAL_REPO against the working directory, so the move retargeted the checkout the user meant to install from - a profile can drop between the working directory check and the probes that follow, which reported cli_unusable or desktop_capability_probe_failed and offered a repair needing that same profile; both now ask again and report the profile when it is what went missing * Narrow the desktop marker, and anchor the values a move could still retarget Five independent reviews of the branch agreed on the same three gaps: - the marker is inherited by the backend and everything below it, so treating it as authorisation for any "studio" subcommand let a marked descendant relocate "studio run --model .\local.gguf", rebasing a path the caller chose. It now authorises only invocations that carry no path, which is what the desktop actually runs - a value like "\cache" is rooted to the drive of the current directory, not to a drive, so a profile on another drive moved it. Root-relative values now go through the OS with the drive-relative ones, and the extended prefix is matched case-insensitively, since the object manager accepts \\?\unc\ too - four more single-path overrides are pinned, and two path lists are anchored entry by entry, so one relative entry cannot change what a whole search or allowlist means The desktop also relocated a child out of any folder under the Windows tree, while the CLI only ever refused System32 and SysWOW64. It now uses the same definition, so a child running from somewhere like C:\Windows\Temp keeps the directory it had. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Offer the roaming-profile cause only where roaming profiles exist A platform-isolation audit found this PR's working-directory-unavailable message asserting a Windows cause on every platform: Unsloth cannot reach your user folder, so it has nowhere to run from. This usually means a network or roaming profile is not available yet. The Rust half of this PR is deliberately not #[cfg]-gated; isolation comes from windows_roots() returning empty off Windows, which does hold for every env-var rewrite. But home_dir_available() is called ungated from preflight/managed.rs, so ManagedProbe::Unavailable{working_directory_unavailable} is reachable on Linux and macOS, where the same symptom means an unmounted home or a permissions problem. A roaming profile is a Windows concept and naming it there sends the reader looking for something that is not present. The symptom and the remedy are unchanged everywhere. Only the CAUSE moves behind a platform check, so it is offered where it applies instead of asserted everywhere. Test covers both directions: Windows still gets the sentence, Linux and macOS do not, and both keep the symptom and "Reconnect and try again". studio/frontend/tests/backend-preflight-message.test.ts 4 passed * Do not let the interpreter decide which folder a value names The Windows cross-platform CI caught this: ntpath.isabs answered True for a leading separator until Python 3.13 and False after it, so on 3.12 a root-relative "\cache" was treated as fully qualified and left to move with the working directory, which is the retargeting the pinning exists to prevent. Both halves now spell the test out, a drive plus a separator or a UNC share, so the same value names the same folder on every interpreter and on the Linux runner that tests the Rust half. * Run the update smoke tests when the CLI guard changes The guard runs on every CLI invocation, "unsloth studio update" included, but neither update workflow listed it, so the two suites that install Unsloth and then update it twice never ran for this change. * Do not rewrite an override the working directory never resolved Three of the pinned names are read as something other than a path by the code that consumes them, so anchoring one changed its meaning instead of preserving it. MLX_HOSTFILE holds either a filename or the host list itself, huggingface_hub expands %VAR% in HF_HOME and its neighbours after the guard has run, and the pre-quant allowlist ignores a bare on/off token precisely so that there is no allow-all mode: anchoring the "1" would have turned it into a real allowlisted directory. All three are now left alone, in the CLI guard and in the desktop twin. Also in this pass: - studio --frontend=.\dist carries a path inside the option token, which the marker gate missed because it only looked for a bare argument after the subcommand. A marked child running it is refused rather than moved. - \Windows\System32\config\systemprofile names SYSTEM's profile without a drive, so it compared equal to no drive-qualified Windows root and was accepted as a home. The drive-less spelling of each Windows root is compared too. - The child is only told where to run when that differs from where the parent already is. Reopening an inherited directory by name can fail if an ancestor turned unreadable after launch, where inheriting the open handle would have worked, so the no-move case stays exactly as it was. - An override the OS declines to resolve now refuses the whole move on the desktop side rather than being dropped, which is what the CLI guard already did: moving with that value still relative would retarget it. - The tests that read the ambient environment take the crate-wide env lock; XDG_DATA_HOME is one of the pinned names now, and the test that swaps it was documented as the only reader. * Scope the non-path exemptions, and anchor the import roots too Three follow-ups on the pinning: The exemptions for inline JSON, %VAR% / $VAR and bare on/off tokens now name the variables whose reader proves them, rather than applying to every pinned name. A directory really called [llama] or %data% is legal on Windows and UNSLOTH_LLAMA_CPP_PATH is read as exactly that, so the blanket form left it unpinned and the move retargeted it. PYTHONPATH joins the anchored search lists, and the guard anchors the relative entries this interpreter is already carrying in sys.path: those are resolved on every import, not at startup, so a move would let whatever sits in ~/.unsloth shadow a managed import. sys.path is best effort, unlike the environment: an import root is not worth refusing the move over, and refusing is how the login start broke to begin with. PATH is deliberately left out, being mostly other people's absolute entries. Preflight asks for the whole managed context, not just the directory. An override the OS declines to resolve fails the same spawn, and a probe that returned false for it was read as a broken CLI, which started an automatic repair that needed the same context and failed the same way. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Expand a cache override before deciding, and read the real invocation Three more from the review: huggingface_hub expands %VAR% in HF_HOME and its neighbours, but Studio's own hf_cache_settings._canonical() does not, so leaving such a value as written sent the two readers to different folders once the process moved. The guard now expands those names before deciding, writes the expanded form back when expanding is what made it name a folder, and anchors it when it does not. A name the machine does not set stays as written, which is what expandvars does too. The desktop got the same, with a small %VAR% expander since std has none. The updater removes PYTHONPATH on Windows, and pinning an inherited relative one put it straight back. The removal now happens after the managed context is applied: -I only covers the first interpreter, and the update's PowerShell and setup descendants start further Python processes that do not clear it. The Typer callback classified sys.argv even when the app was called as a library, so a host whose own argv looked like a desktop command could move the process out from under the caller's relative paths. It reads the invocation Click is running instead, and where Click keeps the tail to itself the invocation is refused rather than relocated. The console script is unaffected: it is classified at import, from the real argv. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Take the whole invocation, and the spellings that follow the process Four more from the review: os.path.expandvars takes %NAME%, $NAME and ${NAME} on a Windows path; the desktop expander only took the first, so a value written $LOCALAPPDATA\hf was anchored under System32 by the desktop while the CLI guard would have kept it. All three forms now, with a test that walks each one. The Typer group records the tokens it was handed. Click keeps the tail on the child context, so the callback saw only the subcommand name and refused a library `studio --api-only` that it should have relocated. Reading the recorded list classifies the invocation in full, whether it arrives through app(args = [...]) or a runner. HF_TOKEN_PATH is pinned like the caches beside it: huggingface_hub reads the credential file from there, and a relative value would follow the child and lose access to gated repos while everything still looked healthy. PYTHONPATH has two spellings that follow the process rather than the caller: an empty component means the working directory itself, and `~` is never expanded there, so Python reads ~\plugins as an ordinary relative folder. Both are anchored to the directory being left, in the environment, in sys.path and in the desktop twin. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Only anchor a sys.path entry that is a directory, and name the setting Two findings from a fresh review round. sys.path holds other people's strings as well as folders. setuptools registers a relative sentinel for an editable namespace install and its own path hook accepts that sentinel by exact equality, and a relative .zip keeps the spelling its already imported packages hold in their loaders; rewriting either breaks the import the pinning was meant to protect. Both were reproduced: an editable namespace stopped importing after the rewrite, and a package loaded from a relative archive lost its submodules. Only an entry that is a directory right now is anchored, plus the empty entry, which is the working directory by definition. A value that cannot be pinned now says so. Windows caps an environment variable at 32767 characters, so a long enough list can stop fitting once every entry names its folder in full, and a drive with no current directory of its own cannot be resolved either. Both were reported as "check that the user profile for this account exists and is writable", which sends the reader looking in the wrong place. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Expand exactly as ntpath does, and say which context failed The desktop expander read $NAME as far as the first non-word character, where ntpath counts a hyphen as part of the name. With CACHE and CACHE-ROOT both set, HF_HOME=$CACHE-ROOT\hf resolved to C:\right\hf in the CLI and C:\wrong-ROOT\hf in the desktop: one install, two folders. It now mirrors ntpath's pattern outright, including the single-quoted run that is copied through unexpanded, %% and $$ standing for one character, and anything unterminated staying as written. The guard's own tests call os.path.expandvars against the environment they describe rather than a stand-in that only knew %NAME%. Preflight tells its two context failures apart. A probe that could not be configured returns that instead of false, so a context which recovers between the failed apply and the check afterwards can no longer make an untested CLI look broken and start a repair. And an override the OS cannot resolve is reported as path_setting_unresolvable rather than as an unreachable user folder: the profile is fine, the value is not, and the frontend now says so instead of advising a reconnect. * Never panic on the spawn path, and leave a host's state as it was Five more from the review. The backend spawn unwrapped the managed context, so a drive that went between the preflight check and the spawn took the desktop down with it. It reports through the same diagnostics path as every other start failure now. UNSLOTH_STUDIO_HOME and STUDIO_HOME are removed for every managed child, because Tauri uses the legacy root whatever the environment says. Trying to resolve them could only invent a failure for a value the child never sees, so they are skipped when pinning and removed by the context helper itself rather than only at the call sites. A child was moved with its relative overrides untouched when the original directory could not be read at all. That silently retargets each of them at the new directory, so it is refused unless there is nothing relative left to preserve. The Typer callback no longer relocates. It runs after the command modules are imported, and commands.studio resolves STUDIO_HOME at import time, so a host that reached that point cannot be moved without leaving the cached root behind. The console script is unaffected: it is checked before any command module loads. The environment and sys.path are put back if the move does not happen. Inside a host process both belong to the caller, and a chdir that fails after pinning left them rewritten as though it had succeeded. * Write a ~ value out, and pin the uv cache Two more from the review round. A `~` value was left alone on the grounds that expanduser does not consult the working directory. That is true of expanduser and false of the readers: llama_cpp.py hands UNSLOTH_LLAMA_CPP_PATH and LLAMA_SERVER_PATH straight to Path(), as the whisper and stable diffusion overrides do with theirs, so `~\llama.cpp` was an ordinary relative path for them and followed the child to the new directory. It is written out now, on both sides, which is what the caller meant and what the readers that do call expanduser would have computed for themselves. That also covers the PYTHONPATH case more honestly than anchoring it did: `~` names the profile rather than a folder called "~" beside the old directory. UV_CACHE_DIR joins the pinned names. uv reads it as written, Studio treats a non-blank value as authoritative, and `unsloth studio update` runs uv through setup.ps1, so a relative one moved the install cache. The guard's tests and the simulations now call the real expanduser against the environment each case describes; the stand-in returned the profile for every input, which is what hid this. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Judge a list entry by entry, and keep the checkout out of the children Three more from the review. The lost-directory check read a whole path list as one value, so PYTHONPATH=C:\vendor;plugins looked qualified because of the drive at the front while `plugins` still depended on the directory that was gone. Each entry is judged on its own now, with the empty PYTHONPATH component counting as the directory itself. MLX_IBV_DEVICES is pinned beside MLX_HOSTFILE and exempted from anchoring the same way. `_json_rank_count_from_env` reads the two identically: either the device list inline as JSON, or a filename. STUDIO_LOCAL_REPO is read by the update and installer path alone (install_python_stack.py), so a stale drive-relative value was failing preflight, backend startup, capability probes and auth provisioning over a setting none of them look at. It stays pinned for the update child and is dropped for the rest. * Anchor an import root that is really there, and read /var/cache as absolute Two more from the review. The lost-directory check judged every value by Windows rules, and it is the one part of the pinning that runs off Windows: a desktop on Linux or macOS whose launch directory was deleted read XDG_CACHE_HOME=/var/cache as relative and failed every managed spawn with path_setting_unresolvable over a value that depends on no directory at all. The native spelling counts there too now. sys.path entries are anchored when they name something on disk rather than only a directory. Skipping every non-directory kept setuptools' editable sentinel safe, which is what it was for, but it also left a relative importable archive behind: os.environ is not sys.path, so after the move the next import from that archive looked for it beside the new working directory. A folder or an archive is anchored; a string that names nothing on disk is still left exactly as written. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Read a POSIX path list with POSIX rules, separator included The lost-directory fallback is the one part of the pinning that also runs off Windows, and absoluteness there was already fixed. The separator was not: the list was still split on ';', so "/opt/vendor:plugins" stayed a single entry, started with '/', and passed as absolute. The relative entry behind it was never seen, and the child got it resolved against the wrong directory. Deciding both from a parameter rather than cfg! is what makes this testable. The existing entry-by-entry test feeds a Windows list and runs on every platform, so a compile-time cfg would either break it off Windows or leave the POSIX path untested. Now each test names the rules it means, and the POSIX one also pins that a Windows-shaped value stays Windows-judged. Supersedes the narrower POSIX test added alongside the absoluteness fix: same fixture, and the new one additionally covers the separator and the Windows-stays-Windows direction. * Normalize overrides before refusing a lost-directory move When the launch directory is gone, the pin scan judged each override exactly as written, without the expansion and non-path exemptions the moving path applies. HF_HOME=%LOCALAPPDATA%\hf, an inline JSON MLX_HOSTFILE and UNSLOTH_ALLOW_LOCAL_PREQUANT_PATH=1 were all read as relative, so preflight and every managed spawn failed with path_setting_unresolvable over values that never depended on a directory. The same normalization now runs before the check. * Shorten the comments added by this branch Same intent, fewer lines: the reasons that are not obvious from the code stay, the restatements of it go. No code or behaviour change. * Pin the model paths llama-server reads for itself llama-server takes LLAMA_ARG_MODEL, LLAMA_ARG_MMPROJ and the two draft-model spellings straight from the environment and resolves a relative one against its own working directory, and Studio reads them back when it sizes a launch (llama_cpp.py). A managed child moved out of System32 with one of those still relative would look for the model or the projector beneath ~/.unsloth. Both mirrored lists now carry them. The URL and HF-repo spellings stay out: they name no local file. * Settle an expansion before anchoring it, and restore the real sys.path Two defects found by a fresh round of idempotency review. A value whose expansion needs a second pass, LOCALAPPDATA holding %USERPROFILE% and HF_HOME holding %LOCALAPPDATA%, was anchored while still half expanded, so the reader that expands saw a folder name with a second drive in the middle of it. Expansion now runs to a fixpoint, and a value that never settles, HF_HOME holding itself, is left exactly as written rather than anchored or grown. Both layers, with a test each. The console script passes no list, so the guard pinned the real sys.path with no snapshot to put back: a chdir that then failed left the process carrying import roots it never agreed to, while the environment was restored. The snapshot is now taken from the list actually being pinned. The existing rollback test passed its own list and so missed the one path production takes; the harness can now leave it out. * Never let a lost directory fail a spawn, and let the installer build its profile Two success-to-failure changes found by the same review round. A process whose working directory has been deleted or unmounted can still spawn children from the handle it holds. Pinning cannot anchor anything to a directory it cannot name, and the answer was to refuse, which took the capability probe, the backend start, the auth provision and the update down over a setting the command may never read. The pins still report what a move would lose; that report now decides whether the child moves rather than whether it runs, so it stays where it is, exactly as it did before this file learned about working directories. The installer shared the managed resolver, which requires the home to exist so that a child never builds an empty folder shadowing a roaming profile that has not mounted yet. install.ps1 and install.sh detect a SYSTEM profile themselves, and before they shared the resolver a home that did not exist was simply created along with ~/.unsloth, so they get that policy back. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Pin the pinning decision as a table * Expand a value the way its reader does, and never leave the process elsewhere Four findings from a fresh round of review, all in code this branch added. Expansion now runs exactly one pass, which is what every reader runs, and the result is written back only when expanding it again would change nothing. The fixpoint loop that was here consumed an escaped %%NAME%% on the first pass and then expanded what the escape was protecting, and a nested reference came out half expanded. Both are now left exactly as written, alongside the self-referencing case they were added for. Python never expands ~ in PYTHONPATH, so neither does the pinning: `~\plugins` is anchored as the relative folder the interpreter actually reads rather than turned into a profile folder it was never reading, which also stops an inert entry from becoming an importable one. Click reads `-f.\dist` as a value exactly as it reads `--frontend=.\dist`, so an invocation carrying one is refused rather than rebased under the new folder. A chdir can succeed and still land somewhere the guard refuses, through a junction or a profile inside the Windows tree. The process now goes back where it started, so the values written for the move are put back with it instead of being restored under a directory nobody chose. * Resolve a tilde the way ntpath does, and pin two more paths More from the same review round, all cross-layer. The two layers expanded a tilde and a variable in opposite orders, so a value that names a folder through both reached different folders. Rust now does the tilde first and the variables second, as the CLI guard does. Rust also resolved ~someone-else as a sibling of this profile unconditionally. ntpath declines to guess unless the profile is named after the current user, because C:\Users\me.DOMAIN is not me's sibling, and now so does this. AMDGPU_ASIC_ID_TABLE_PATH and VLLM_CACHE_ROOT are read straight from the environment as file paths, by import_fixes.py and by Studio when the caller set one themselves, so both lists carry them. The update no longer pins PYTHONPATH on Windows, where build_update_command drops it anyway: pinning it could only refuse an update over a value the child never receives. * Never refuse an update over a setting it drops, and name the one that blocks Two review items, both about what a failure costs. STUDIO_LOCAL_REPO is now pinned best effort in both layers. A bare `unsloth studio update` drops it before anything reads it, and `update --local` is refused rather than relocated, so a stale drive-relative value could only refuse the one update form the System32 fallback exists for. It is still written out whenever it can be, so a reader that appears later still finds the folder the caller meant; every other setting still stops the move, because something does read those. A context failure now carries the setting that caused it, as `path_setting_unresolvable:HF_HOME`, and the window names it. "One of Unsloth's folder settings" is not something anyone can act on, and every pin failure already names the setting it could not preserve. The name only, never the value, since this reaches the screen. * Refuse the move when one pass leaves the folder up to the caller Leaving an unsettled expansion as written and moving anyway takes the value with the process: the reader resolves what one pass gives it against whatever directory it is standing in, so a nested %NESTED%, an escaped %%NAME%% or a self-reference quietly followed the child to ~/.unsloth. One pass is still what the reader does, so it is still what decides. If that result names a folder on its own, C:\\cache\\%UNSET%\\assets, the value means the same thing from anywhere and is left alone. If it does not, the setting cannot be preserved across a move and the move is refused, naming it. Both layers, a test each. * Read a path list the way the host does, and report what will not fit Four more from review, all in the moving path. The list loop split and rejoined on ';' and judged absoluteness by Windows rules, while the lost-directory branch had already learned the native ones. A POSIX PYTHONPATH of plugins:/opt/vendor was therefore one entry, and both import roots left with it. Both loops now read the host's separator and the host's idea of absolute. A pinned list that no longer fits in a Windows variable is now reported as the setting that did not fit, rather than discovered by CreateProcess, where the window would have offered a repair that hits the same wall. The tilde now follows USERPROFILE, which is what ntpath.expanduser answers and what the CLI guard uses. dirs::home_dir() reads the known folder, which a portable or overridden environment moves, and the two layers have to name the same folder. GGML_BACKEND_PATH joins both lists: llama_cpp.py preserves it into the llama.cpp child, which resolves it against wherever it is standing. * Expand a POSIX value the POSIX way, and check a scalar against the limit too The lost-directory branch is the one part of the pinning that runs off Windows, and it still read %HOME%/hf as an expandable reference. posixpath.expandvars leaves that literal and expands $HOME instead, so a value that depends on where the process is standing was read as one that does not, and the child moved out from under it. Expansion now follows the host: $NAME and ${NAME} off Windows, %NAME% and the rest on it. The oversized check covered the joined list but not a scalar, which crosses the same limit once it names its folder in full. Same check, same message, both layers. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: danielhanchen <unslothshared@gmail.com> |
||
|
Daniel Han
|
18dbedb889
|
studio: harden the launcher-refresh installer fetch (#8542)
* studio: ship install.sh / install.ps1 in the wheel so the launcher refresh stops fetching one `unsloth studio update` refreshes the desktop launcher by re-running the installer with --shortcuts-only. It looked for install.sh / install.ps1 in a checkout or at _PACKAGE_ROOT, which is site-packages for a wheel, found neither, and fell back to downloading https://unsloth.ai/install.sh and piping it into `bash -s` (or writing install.ps1 to a tempfile and running it with -ExecutionPolicy Bypass). That is the normal path for every PyPI install, so a routine update ran code from a second trust anchor: whoever can tamper with that response gets execution as the updating user, on top of the PyPI channel the package already came from. Rather than drop the fallback and lose launcher refreshes for PyPI users, ship the installers with the distribution. pyproject data-files puts them under <data>/share/unsloth, which pip and uv both resolve to the Studio venv root, so the local exec path now covers wheel installs and the network fallback is gone. - _installer_script_candidates(): checkout, _PACKAGE_ROOT, <data>/share/unsloth, sys.prefix, USER_BASE - both fetch-and-exec blocks replaced by a skip message that names the reinstall command - Windows wheel installs now take the existing `& 'path' --shortcuts-only` branch that checkouts already used, instead of the wrapper that regexed `Install-UnslothStudio @args` out of a downloaded script Verified: wheel and sdist both carry the installers, pip and uv place them at <venv>/share/unsloth, and `bash <venv>/share/unsloth/install.sh --shortcuts-only` regenerates the launcher and exits 0. Editable installs still resolve the repo root. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * studio: fetch the launcher installer from raw.githubusercontent, keep the bundled copy as fallback Follow-up to the previous commit, which pinned the launcher refresh to the installer that shipped with the wheel. That gave up hotfixing install.sh between releases, which is worth keeping: a launcher bug should be fixable without cutting a version. So the refresh fetches main again, with the parts of the old path that were actually dangerous removed. The fetch now goes to raw.githubusercontent.com/unslothai/unsloth/main directly. https://unsloth.ai/install.sh was only ever a Cloudflare 301 to that exact URL, so the bytes always came from the repo, but the hop put the unsloth.ai DNS and CDN control plane in the path of code execution on every updating machine. Going to the origin removes that without changing what runs. Also: - redirects off raw.githubusercontent.com, or to plain http, are refused rather than followed - a response that is not an installer (captive portal page, HTML error body, truncated transfer) is not executed. This is not a security control, anyone choosing the response can satisfy it, it is there because piping a hotel wifi login page into bash is a boring way to break a machine - the response is size-capped rather than read unbounded - the fetch falls back to the installer bundled under <data>/share/unsloth when the network is unavailable, so an offline update still refreshes the launcher instead of silently skipping it - UNSLOTH_NO_REMOTE_INSTALLER=1 pins the refresh to the bundled copy - a source checkout still outranks the network, so `update --local` tests its own installer rather than main's The Windows fetched path writes the script to a BOM tempfile and runs it with -File and the arguments appended, so install.ps1's own `Install-UnslothStudio @args` at EOF receives them. That deletes the regex that used to strip that line for the stdin path. Verified live on Linux: fetch returns 258755 bytes that validate and regenerate the launcher; UNSLOTH_NO_REMOTE_INSTALLER=1 regenerates it from the bundled copy; with neither, the refresh skips and prints the reinstall command. * studio: apply the installer host guard to the starting URL, not just redirects https://unsloth.ai/install.sh 301s to raw.githubusercontent.com, so a redirect-only check would have fetched straight through it if the constant were ever pointed back at the website. _is_allowed_installer_url now gates the first request with the same rule the redirect handler applies, and the redirect handler calls it too. Found by running the fetch against the unsloth.ai URL on an installed wheel: it succeeded rather than being refused. * studio: fall back to the bundled installer on more of the ways a fetch can go wrong Three from review, all real: A fetched installer that runs but exits nonzero consumed the fallback. If main is temporarily broken, or incompatible with the installed release, the launcher was left unrefreshed even though a release-matched copy was sitting on disk. Both fetched runners now report the exit code and return False so the bundled copy runs. HTTP framing errors escaped _fetch_installer. IncompleteRead from a truncated chunked transfer, and BadStatusLine / LineTooLong from a malformed proxy, are HTTPException and neither URLError, OSError nor ValueError, so they propagated out of an update that had otherwise succeeded instead of falling back. I had assumed IncompleteRead was covered by the ValueError clause; it is not on this Python. The bundled lookup missed the managed venv. A pip-installed CLI can drive an update into STUDIO_HOME/unsloth_studio (_studio_deps.running_outside_managed_venv), where setup writes the new data files; searching only the running interpreter's prefixes would have run the foreign CLI's older bundled installer, or none at all. Three tests added, one per case. * studio: tighten the comments around the launcher-refresh installer lookup * studio: let the managed venv lead the bundled installer lookup Follow-up to the previous commit, which added STUDIO_HOME/unsloth_studio to the bundled roots but appended it. A pip-installed CLI updating into the managed venv would then still find its OWN bundled installer first, at sysconfig/sys.prefix, and run that older copy rather than the release-matched one setup had just written into the target venv. Appending covered the case where the foreign CLI ships no installer at all, and missed the case where it ships an out-of-date one. The managed root now leads. Running inside the managed venv it is sys.prefix again and the existing dedup drops the repeat, so this only reorders the case it is for. * studio: tighten the two comments added after the last comment pass * studio: keep the launcher refresh on unsloth.ai and drop the bundled installer unsloth.ai and unslothai/unsloth are trusted, so the launcher refresh goes back to fetching https://unsloth.ai/install.sh and install.ps1, and nothing is shipped in the wheel. Fetching is the point: a launcher fix reaches users without waiting for a release, which shipping a copy would have given up. Reverted from the earlier approach here: - pyproject no longer ships install.sh / install.ps1 as data-files - no <data>/share/unsloth, sys.prefix, USER_BASE or managed-venv lookup, and no local fallback: a fetch that does not land skips the refresh, as it did before - no UNSLOTH_NO_REMOTE_INSTALLER, which only existed to pin to the bundled copy - site and sysconfig imports are unused again and go with it Kept, because none of it depends on distrusting the source and each one is a way an update could misbehave regardless: - a truncated or malformed response (IncompleteRead, BadStatusLine, LineTooLong) is an HTTPException, neither URLError nor OSError, so it used to escape and abort an update that had already succeeded. It is caught and the refresh is skipped. - the response is size-capped rather than read unbounded - a body that is not an installer, a captive-portal login page or an HTTP error page, is not piped into bash - https and the unsloth.ai -> raw.githubusercontent redirect chain are the only thing followed, checked on the first request as well as on redirects - a source checkout still outranks the network, so `update --local` tests its own installer rather than the published one Verified live: the fetch through unsloth.ai returns 260933 bytes that pass the shape check and regenerate the launcher; a fetch that 404s skips cleanly instead of raising. * studio: reject a truncated installer instead of piping half a script into bash Found by simulating the fetch against a local server that declares a Content-Length and then sends a third of it. response.read(amt) returns what arrived and does not compare it against the declared length; only a further read() does, raising IncompleteRead. The bounded read added here for the size cap therefore disabled the completeness check that the previous unbounded read() got for free. The markers this code looks for sit early in install.sh, so a body cut off a third of the way in still passed _looks_like_installer and was piped into bash as a half-written script: read(amt) -> 86251 of 258755 bytes, no exception, markers present read(amt)+read() -> IncompleteRead, caught, refresh skipped read() (pre-PR) -> IncompleteRead, uncaught, traceback So the bounded read was worse than what it replaced, not just different. The follow-up read() restores the check while keeping the cap, and returns b"" on the normal path. Two tests: a truncated prefix that still satisfies the marker check is refused, and a complete body still fetches. * studio: skip, do not abort, when the Windows temp script cannot be created tempfile.mkstemp() sat outside the try, and the write inside it was not guarded, so a full disk, a read-only or missing %TEMP%, or antivirus holding the handle raised OSError straight out of `unsloth studio update` after the package update had already completed. Same class as the HTTP framing escape fixed earlier: a best-effort launcher refresh must not turn a successful update into a traceback. Both are now reported and skipped, and the write failure still unlinks the file it created. Two tests cover it. Also corrects the docstring: the pre-PR fetched path already used a BOM tempfile with -File. What changed is that the args go after the path so the installer's own `Install-UnslothStudio @args` receives them, which is what removed the regex that used to rewrite that line. * studio: stop the shape check from rejecting a legitimate future installer Two ways the check could have disabled every wheel-based launcher refresh, silently and permanently, until new Python shipped. `<#` opens PowerShell comment-based help, an entirely ordinary way for install.ps1 to begin, and the HTML guard rejected any body starting with `<`. It now matches the markup it means (doctype, html, head, body, xml) and explicitly allows `<#`. The markers pinned internal names: create_studio_shortcuts is a shell function and Install-UnslothStudio a wrapper, both renameable without changing the command-line contract. Only `--shortcuts-only` is required now, which is the flag this code passes, so an installer lacking it cannot serve the request regardless. The size floor drops to 512 bytes for the same reason: a smaller legitimate wrapper should not be refused. Three tests: a help-block script passes, real HTML and XML error bodies are still refused, and a renamed internal helper does not break the check. * studio: close the fake handle's descriptor in the Windows tempfile test The stand-in handle took the descriptor from the patched os.fdopen and never closed it. Windows will not unlink a file that still has an open descriptor, so the cleanup assertion failed there for a reason unrelated to the code under test. The handle now owns and closes the descriptor, and the test asserts the descriptor is closed so a leak fails on Linux too. * studio: keep the refresh fallbacks the refactor dropped Two behaviour differences against main that had nothing to do with hardening. A local installer that cannot be launched no longer ends the refresh. The old candidate loop caught the exec OSError and carried on to the next candidate and then to the network; the helpers now report that with a return value so the caller falls through the same way. The fetch no longer discards a site-wide urllib opener. It used to call urlopen, which honours install_opener, so a machine whose proxy auth or corporate CA lives in a site handler reached the network through it. Validating the redirect chain needs our own opener, so the installed handlers are carried across instead of dropped. * studio: stop the installer opener touching global urllib state Carrying the site-installed handlers into the fetch opener was wrong twice over. OpenerDirector.add_handler assigns handler.parent, so sharing them repointed the installed opener's own handlers at this one and broke every later urlopen in the process, and copying them instead does not deliver the intended benefit either because the default HTTPSHandler in the new opener already answers first. The opener is private again. Proxy environment variables and the system trust store still work. A machine whose proxy auth or CA lives in a programmatically installed opener skips the refresh, which the update survives. * studio: try every on-disk installer before reaching for the network The bool fall-through restored the network fallback but not the rest of what the pre-refactor loop did: it selected only the first candidate that existed, so a second one was dropped when the first could not be launched. An update --local with a distinct checkout could therefore run the published installer while a usable one sat on disk. Candidates are collected rather than reduced to the first, and the caller stops at whichever one actually launches. * studio: tighten the comments added by the launcher-refresh change --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> |
||
|
Daniel Han
|
098a6a0957
|
Studio: honor a request's enable_tools: false instead of overriding it (#8547)
* Studio: honor a request's enable_tools: false instead of overriding it The process-wide tool policy was an override, not a default. unsloth studio run installed set_tool_policy(True) at startup, and _effective_enable_tools returned that value whenever it was non-None, so the request's own enable_tools field was never read. The Studio UI sends its tool pills as an explicit request field, and expresses 'every pill off' by omitting enable_tools entirely. Against a True override that omission read as 'tools on', and with enabled_tools also absent the route selected ALL_TOOLS, so a chat with every tool switched off still advertised web_search, python, terminal and render_html. Thread-title generation, which posts to /v1/chat/completions with no tool fields, picked them up the same way. Only unsloth studio run installed the policy, so unsloth studio, the desktop app and Colab behaved correctly and the two commands disagreed on the same UI. Split the policy into two slots. The override still comes from an explicit --enable-tools/--disable-tools and still beats the request. The new default is what an omitted enable_tools falls back to, installed as True by every launcher, so tools stay on for every bind including --secure. A request that says enable_tools: false now turns them off. Frontend sends the off state explicitly rather than by omission, in the local chat, external provider and token-count paths, and pins enable_tools: false on title generation so a 24-token summarisation never carries tool schemas. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Studio: do not let the tools-on default answer a request that stated its intent The safetensors/MLX path resolves _sf_tools_on straight from _effective_enable_tools, without the two withdrawals the GGUF router applies. So the launcher default introduced here reached requests that had already expressed their own tool intent: - tool_choice: "none" with enable_tools omitted resolved to tools on, and with no enabled_tools allowlist that selected every built-in, python and terminal included, turning a standard opt-out into server-side execution. - A client tools catalog with enable_tools omitted made _sf_client_tools false, so the request left the client-tool passthrough for Unsloth's own loop and the caller got built-ins instead of calls for its own functions. The GGUF router avoids both with _client_disabled_tool_calls and _explicit_studio_tool_loop_requested. Draw the same line on the safetensors gate: the default only answers a request that said nothing, so tool_choice: "none", a client catalog, or tool-result history withdraws it. An explicit enable_tools/mcp_enabled ask, and a CLI --enable-tools or --disable-tools, are unchanged. Also read the resolved _sf_tools_on in the _sf_client_tools gate rather than recomputing _effective_enable_tools, which would have hidden the withdrawal. * Studio: let a response_format contract withdraw the tools-on default too _takes_tool_passthrough already ends with _extract_response_format(payload) is not None, so on the GGUF router a structured-output request keeps the passthrough and never enters the server tool loop. The safetensors withdrawal missed it, so a request supplying response_format while omitting enable_tools still resolved _sf_tools_on to true and could select and run every built-in. response_format is not a declared field on ChatCompletionRequest; the model is extra=allow and OpenAI-SDK clients spread extra_body at the top level, so read it through _extract_response_format rather than an attribute. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Studio: resolve the tool policy before protocol selection, and scope the default to unsloth studio run Two fixes for the same root cause: the tools-on default reaching code built around an omitted enable_tools meaning no tools. _sf_server_tool_intent read the raw policy while the withdrawal ran ~40 lines later, so a tool_choice: "none" or response_format request classified the response protocol on the template's tool_use branch and then generated on the plain one. On a model whose reasoning markers live only in the tool template the extractor starts in the wrong mode and can return the answer as reasoning_content. Resolve _sf_cli_policy / _sf_tools_on / _sf_mcp_allowed once, above the classification, and derive the intent from the resolved value. The default is also no longer installed by run_server. It belongs to unsloth studio run, the launcher that has always forced tools on, and which installs it itself. Installing it in _apply_cli_tool_policy extended it to unsloth studio, the desktop app and Colab, where paths that assume an omitted enable_tools means no tools started seeing it: n > 1 is rejected by the tool loop though the plain path implements it, max_tool_calls_per_message: 0 still advertises schemas and the nudge, and the pre-switch passthrough guard does not recognise tool-result history so it 400s a non-streaming continuation. Those paths predate this PR and are unchanged on unsloth studio run; scoping the default keeps them that way everywhere else. unsloth studio run still defaults tools on for every bind, --secure included, and a request's enable_tools: false is still honored. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Studio: correct the tool-policy help and docstrings for the scoped default Scoping the tools-on default to unsloth studio run left three places claiming it applies everywhere. The --help for plain unsloth studio and for a direct run.py launch both said 'Default: on for every bind', which is now the opposite of what those launchers do, and the tool_policy module said 'Launchers install True'. A --help line about a tool-execution default is worth keeping exact. unsloth studio run's own help is unchanged, since it is the launcher that does default them on. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> |
||
|
Daniel Han
|
5714530f27
|
Route spoofed Strix Halo GPUs to the AMD per-gfx index (#8480)
* Route spoofed Strix Halo GPUs to the AMD per-gfx index (#7331) HSA_OVERRIDE_GFX_VERSION=11.0.0 is the widely circulated Strix Halo workaround, and ROCr applies it in userland, so rocminfo hands the spoofed gfx1100 to every consumer. The installer believed it: the correct gfx1151 inferred from the product name in /proc/cpuinfo was discarded, the Strix reroute intersected {gfx1151, gfx1150, gfx1152} against ["gfx1100"] and got nothing, and torch came from download.pytorch.org/whl/rocm6.3 as 2.9.1+rocm6.3. The first real allocation then ran gfx1100 kernels on gfx1151 silicon and segfaulted. The runtime-visible arch still outranks the product name everywhere it did before. The correction fires only when HSA_OVERRIDE_GFX_VERSION is set, the probe saw exactly one device, that device's arch differs from an inferred RDNA 3.5 APU arch, and a source the override cannot reach agrees with the product name: KFD topology sysfs first, since amdkfd writes gfx_target_version from the kernel's own table, then rocminfo re-run with the variable stripped, and only if neither can answer, the variable statically naming the arch that was reported. A mixed Strix APU plus discrete AMD GPU host stays on today's path. Two devices in the probe declines outright, and so does a kernel that sees a second GPU the spoofed probe had collapsed away, so the existing precedence test needed no change. install.sh carries the same three helpers and the same decision, and the tests execute both copies over the same eight host shapes rather than grepping for them. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Require corroboration before undoing an HSA_OVERRIDE_GFX_VERSION spoof Three problems with the correction as it stood. install.sh never fired. The Strix reroute passes `_gfx_all` in raw, and that is `rocminfo | grep -oE 'gfx[1-9][0-9a-z]{2,3}'` output, which repeats the token once per Name / ISA line. A single GPU therefore arrives as two or three lines, the device count read 2 or 3, and the helper returned early on every real host, #7331's own included. Count DISTINCT tokens instead. The Python side already splits on agent headers and was unaffected, so the two paths disagreed on exactly the host the fix exists for: `studio update` repaired the machine and a fresh `curl | sh` install re-broke it. The static fallback could fire when it should not. "The variable names the arch that was reported, so assume a spoof" is indistinguishable from a host telling the truth: a real RX 7900 XTX in a Ryzen AI Max chassis reports gfx1100, infers gfx1151 from the CPU product name (the inference reads /proc/cpuinfo and never looks at the GPU), and presents the identical fingerprint the moment its owner has the override set for an unrelated reason. Same for a correct HSA_OVERRIDE_GFX_VERSION=10.3.0 on an RDNA2 card. Both were rerouted to Strix wheels whenever KFD sysfs was unreadable. Drop the fallback: corroboration from the kernel or from an unspoofed re-probe is now required, and a re-probe that still answers the probed arch is read as evidence FOR the probe rather than as a failure to disprove. Rerouting a working machine to the wrong wheels is worse than the bug being fixed. The Python re-probe left the visible-device masks in place while install.sh cleared them, so a mask pinned to the dGPU hid the second GPU whose presence is the only thing that vetoes the correction on a mixed host. Clear all three in both, and add the arch the variable names as a required precondition rather than a sufficient one: ROCr can only rename an agent to the target the variable names, so any other reading is real silicon. Tests: the parity check now builds each side's probe input the way its own call site does instead of handing both a pre-shaped list, which is what let the install.sh defect through. Over the resulting matrix the previous code diverges on 91 of 3360 shapes with install.sh correcting none of them; it is clean now. Adds the 7900-XTX-in-a-Ryzen-AI-Max cases, executes the shell KFD parser against a fabricated topology tree rather than grepping it, and passes create = True where a helper is patched so a run against an older tree fails on the assertion instead of on AttributeError. * Say why the spoof check declined, and count arches without wc The correction printed "Checking whether the ISA is being spoofed." and then, on every path that decides there is no spoof, said nothing further. That is the CORRECT outcome for a real gfx1100 card in a Ryzen AI Max chassis, and it is the outcome a user is most likely to see, so silence reads as a failure. Report the conclusion in both implementations, naming the source that declined to corroborate and the arch being kept. Also count distinct probe tokens inside the existing awk instead of piping to `wc -l | tr -d`. wc was the one tool the change added to install.sh's dependency set (0 pre-existing uses); this drops it along with two process spawns per call. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Clear a corroborated HSA_OVERRIDE_GFX_VERSION spoof before Studio launches * Clear a contradicting HSA_OVERRIDE_GFX_VERSION at every Studio launch install.sh unsets the corroborated spoof for the one launch it performs itself, but that unset dies with the installer: studio/setup.sh runs install_python_stack.py as a child, so the `unsloth studio update` repair, the generated launch-studio.sh and a hand-typed `unsloth studio` all still start with the variable set. Clear it at the CLI chokepoint the exec, the Windows Popen and the in-process paths all pass through, keyed on the install rather than on a hardware probe: AMD's per-gfx index ships rocm_sdk_libraries_<arch> beside single-arch wheels, so an override naming a different arch is provably asking for kernels this install does not contain. A generic multi-arch index brings no such distribution and is left alone, since there the override is often the only thing making the GPU usable. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Read the active ROCm family, clear the spoof on every launch entry point * Pin the amd-smi re-probe parity between install.sh and the Python stack * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Keep the override when --no-torch installs no per-gfx wheels Clearing HSA_OVERRIDE_GFX_VERSION is only sound because native per-gfx wheels are going in on that branch. The guard checked only that the spoof had been corroborated, not that anything was actually being installed. --no-torch, and the Intel Mac auto-detection that sets the same SKIP_TORCH, reach the reroute and then install no torch at all. Clearing there left the host with the generic wheels it already had and no override, which is strictly worse than either alone: on a spoofed Strix host the override was the only source of usable kernels. The block's own comment already stated the invariant (native wheels are going in); it just did not enforce it. It does now. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Let torch's own requirements decide whether the rocm metadata is live The launch-time arbiter read the bare rocm meta-package's Requires-Dist to name the family the install carries kernels for. That is authoritative only while torch actually resolves through it. Switching from AMD's per-gfx index to a generic pytorch.org ROCm one does not uninstall it: the generic wheels vendor their own ROCm libraries and depend on no meta-package, so rocm is orphaned outright, and pip has no autoremove. The stale metadata then named the OLD family and cleared an override that the generic wheels may be the only reason the GPU works at all, which is the opposite of what this code is for. torch's Requires-Dist is now the first discriminator. An unknown shape answers no, so a tree this cannot read is left alone rather than arbitrated on a guess. The venv fixture grows the same dependency edge, since one that never carried it could not tell a live meta-package from an orphaned one. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Condense comments in the HSA_OVERRIDE_GFX_VERSION spoof detection --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: danielhanchen <danielhanchen@users.noreply.github.com> |
||
|
Daniel Han
|
a867496a28
|
Studio: launch a DFlash speculative drafter automatically (#8338)
* Studio: launch a DFlash drafter automatically Studio has recognised dflash-*.gguf since #7811, but only to hide it from the quant picker. Nothing ever launched it, so a model that ships a DFlash sidecar fell through to no speculative decoding at all. Add DFlash as the third launchable drafter kind beside MTP and DSpark: a _is_dflash_drafter_path predicate, local and Hub discovery, a supports_dflash capability parsed from llama-server --help, and the --model-draft / --spec-type draft-dflash emission. Unlike DSpark it is on under Auto, since the published sidecar is 1.52 GiB and ships in the model's own GGUF repo rather than being an ~11 GB opt-in fetch. DSpark keeps first refusal when a repo somehow ships both, matching llama.cpp's own downloader. Discovery confirms general.architecture = dflash in the header rather than pairing on the filename: the published sidecar is dflash-kquant.gguf, which names no model family, so the DSpark pairing rule would reject the one file this exists to find. The dflash/ directory is still not a drafter marker, and DFlash is still excluded from companion reclaim, both because the name doubles as a family a publisher puts on real weights. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Harden the DFlash drafter fallback, pairing and dedupe Four fixes from review of the auto-launch path. Strip user-supplied DFlash args on the drafterless retry. The gate that enters the retry counts a DFlash request, but the cleanup only recognised MTP and DSpark. llama.cpp accumulates speculative types, so prepending --spec-default while the DFlash group survived relaunched the drafter that had just failed, and a main model that loads fine without it was lost instead of recovered. Skip a DFlash sidecar that names another weight in the same folder. _drafter_matches_weight is False both for a sidecar naming no family and for one naming a different family, so ranking put them in one bucket and precision could float the foreign one to the top: loading model B beside dflash-model-A-Q8_0.gguf and dflash-kquant.gguf launched model A's drafter. Both files carry a real dflash header, so the architecture check behind the ranking cannot catch it. The decision is made against the weights actually present in the folder rather than by guessing which stems are precision tokens, which keeps the published unpaired sidecar eligible. Stand the Auto DFlash fetch down once DSpark has resolved. DSpark takes first refusal in the promotion, so for a repo shipping both kinds the DFlash sidecar could never launch and the fetch spent bandwidth and cache on a file that would not be used. An explicit dflash request still fetches. Keep Auto deduplicated after a failed DFlash drafter. _speculative_type is reset to "default" by a successful drafterless retry while the launch still records the resolved sidecar, so the next Apply compared the intent's empty MTP path against it and reloaded a healthy server. _spec_drafter_kind survives the fallback and now decides the comparison. test_mtp_drafter_companion.py, test_native_gguf_companion.py, test_llama_cpp_mtp_detection.py and test_resolve_quant_gguf.py: 489 passed, including two new tests for the foreign-sidecar case and for the paired sidecar still winning. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Keep DFlash discovery, the training guard and the hints in step Discovery now accepts the dflash- prefix only. The shared companion predicates recognise DFlash by that prefix, so a <model>-dflash.gguf accepted by discovery was also a selectable Q8_0 main model in the quant picker, and choosing that variant handed llama-server the drafter as the target. Teaching the predicate the suffix instead would hide a real model whose name merely ends in DFlash, which is the case #7811 exists to protect, so detection gives the form up rather than the picker giving up a model. No published sidecar uses it; the shipped one is dflash-kquant.gguf. The same mismatch exists for MTP on main and is left alone here. The training VRAM guard now sizes a drafter named through llama_extra_args. Discovery never fills gguf_dflash_file for a file outside the model directory, but load_model still passes that path to llama-server, so a load could be admitted beside a training run while nothing was charged for the sidecar it makes resident. The Speculative Decoding hint said Auto picks DSpark or else MTP / ngram and that everything but DSpark leaves output unchanged. Auto now picks DFlash too, and like DSpark it is not bit-identical on quantized targets. The Draft Tokens hint gained the DFlash default, which shares the MTP branch at 2 on GPU and 3 on CPU/Mac. 514 passed across the drafter, companion, detection, quant-resolution and picker suites, including two new tests pinning the suffix form out of discovery and the prefix form still in. * Pair the remote DFlash sidecars with the selected weight detect_dflash_file already refuses a sidecar named after a NEIGHBOURING weight, so a folder holding two families cannot attach a foreign drafter locally. The download picker and the offline cache reuse still ranked every dflash-*.gguf by precision and name alone, never comparing a candidate against the weight being loaded, so in a repo hosting more than one family dflash-model-A-Q8_0.gguf outranked the generic dflash-kquant.gguf and model B downloaded and launched model A's drafter. The pairing rule now lives in one place, dflash_repo_preference_key, built on the same _drafter_names_other_weight predicate the local scan uses: a sidecar naming this weight's family first (most specific stem first, as detect_mtp_file does), then one naming no weight present here, then one naming a neighbour. The last is demoted rather than dropped, so a repo whose only sidecar looks foreign still has a fallback. Deciding against the weights actually present is what keeps the published unpaired sidecar eligible: dflash-kquant.gguf has a precision token for a stem, not a family name, so "the stem is non-empty" cannot stand in for "this names another model". Nothing changes for a repo with one sidecar, and with no weight in hand the order is precision only, as before. Tests cover a multi-family repo picking the generic sidecar, the same repo picking the specific one for its own weight, the shipped Muse-Glimmer layout still resolving, and the cached path agreeing with the download path. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Validate DFlash candidates and size the extras drafter once Three fixes found in review of the DFlash drafter work. detect_dflash_file read a candidate's GGUF header before asking the caller's accept callback about it, so a dflash-*.gguf symlink in a directory reached through a native grant had its out-of-lease target opened before the grant check ran, and no later rejection takes a read back. The loop now resolves the launch path, runs accept, and only then parses the header and applies the architecture check. accept still receives the resolved launch path, and callers that pass no accept see the same candidates in the same order as before. The training admission guard charged the llama_extra_args --model-draft sidecar on top of the local one discovery had already found, so a 1.5 GiB drafter was billed as 3 GiB and the guard could refuse an inference load that fits. The effective draft path is now sized exactly once, with identity taken from the resolved path so a symlink or another spelling of the same file dedupes too. That same charge also satisfied the local-weights early return on its own. Loading a remote GGUF repo has no local main weight, so a local --model-draft made the guard return the drafter alone and skip the listing that prices the target model, which could admit a load that then exhausts VRAM next to a running training job. The local branch now fires only when a local weight is actually present, and the drafter is added to whichever branch produces the estimate, including the remote one. Regression tests for all three. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Validate remote DFlash files by header and stop charging unused DFlash bytes Two fixes to the DFlash sidecar paths. Remote and cached DFlash candidates are now confirmed by their GGUF header, not by their filename. _pick_dflash and _cached_repo_dflash_drafter selected with _is_dflash_drafter_path, a dflash- prefix test, while the local scan in detect_dflash_file also required general.architecture == dflash. A remote repo holding an ordinary weight whose basename starts with dflash- therefore had that full weight downloaded and handed to llama-server as --model-draft, which falls back at startup after the bytes are already spent. The architecture rule moves into is_dflash_architecture in model_config, beside the naming rules and shared by every path, the way dflash_repo_preference_key already is. The header is only readable once the file is on disk, so the download validates after the fetch and falls through to the next candidate instead of returning None; the prefix-only naming rule is unchanged. The training coexistence guard no longer charges DFlash bytes a load under Auto will never fetch. _remote_gguf_companion_bytes added the preferred DSpark and the preferred DFlash sidecar whenever the repo listed both, but the loader stands down on the DFlash fetch once DSpark resolves under Auto, so those bytes are never resident and the guard could 409 a load that fits. The new dspark_first flag mirrors that selection. Where the choice is genuinely unknown the deliberate over-estimate stands, and an explicitly forced DFlash still pays for its sidecar. Regression tests cover the fetch falling through an impostor to the real sidecar, an all-impostor repo recording a permanent absence, the snapshot reuse and offline cache lookups applying the same rule, and the Auto guard charging DSpark only when a repo publishes both kinds. * Gate the DFlash stand-down and the guard's sizing on what the load actually does The Auto DFlash fetch stood down whenever _download_dspark answered with a path, but that call deliberately reports an already-cached DSpark sidecar even on a binary with no usable --spec-type draft-dspark (so the route's reuse check does not reload the same server on every Apply). The promotion refuses such a path, so on a DFlash-capable binary a repo shipping both companions suppressed the DFlash fetch for a sidecar that can never launch and the load ended up with no drafter at all. The capability gate now lives in _dspark_wins_auto, shared by the fetch and the promotion so the two cannot disagree. _remote_gguf_companion_bytes still ranked DFlash candidates with the name-only dflash_preference_key while the loader moved to the family-aware dflash_repo_preference_key, so in a multi-family repo the guard could price a different, smaller sidecar than the one that lands. The selected weight name is threaded down and the guard now sorts with the downloader's key over the neighbouring weights from the same listing. * Apply the load's boundaries to drafter discovery, and size Auto's one drafter ModelConfig.from_identifier ran the local companion scan with no way for the caller to say what was in bounds, so a native-grant load read the header of a dflash-*.gguf symlinked out of the granted directory. The validated rescan on the load route rejected it afterwards, which does not take a read back. The boundary now travels into the scan, for all three drafter kinds, so the two passes cannot disagree about what is in bounds. Remote DFlash discovery matched the basename in any nested directory, but the local contract is root level only: a quants/dflash-*.gguf is an ordinary weight detect_dflash_file would never offer, and the header can only be read once the bytes are here, so the whole weight downloaded before the rejection. Checked through a separate predicate so the prefix-only naming rule the other callers share stays exactly as it is. A split companion is only usable as a whole set, since llama-server resolves the sibling shards from the first one's directory. Fetching just the picked shard left a drafter whose header reads fine and which the server cannot open, so the load fell back to no speculation with nothing to show for the download. The companion download now resolves its shards with the same helper the main-model download uses, and neither reuse path reports a half set as a cache hit. The remote sizing charged the first-ranked DFlash candidate, but a rejected candidate falls through to the next name in the ranking, which can be a larger file; headers are unreadable from a listing, so the bound now covers every candidate the fallback can reach. And under Auto the guard charged the MTP drafter on top of the DFlash sidecar that replaces it. Auto launches exactly one drafter, in a fixed order, so dspark_first now expresses the whole promotion: DSpark alone when the repo publishes one, otherwise the larger of the DFlash bound and the MTP drafter, since every DFlash candidate can still be turned away on its header and the load then keeps the MTP one. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Budget a split DFlash sidecar as a set, and reject half a cached one The remote sizing bounded the DFlash fetch with the largest candidate the post-fetch fallback could land on, but each entry is one shard, while _download_companion_gguf fetches the whole shard set the picked file belongs to and llama-server keeps every shard resident. A sidecar published as two 1 GiB shards was budgeted at 1 GiB, and under-charging is the direction that waves a load through and then exhausts VRAM beside a running training job. The candidates are grouped into their sets with _gguf_extra_shards, the same helper the download resolves shards with, and the bound is the largest set total. _cached_repo_dflash_drafter's offline fallback accepted a candidate on is_file plus its header, so a snapshot holding shard 1 alone was handed back as the drafter with no fetch left to complete the set. The header reads fine, then llama-server cannot open the siblings it resolves from that directory and the load falls back to no speculation. Same _drafter_split_is_complete rule the snapshot reuse already applies, and skipped rather than fatal like the header check, since another snapshot may hold the complete copy. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Move drafter naming, ranking and DFlash discovery into a drafters package Pure structural move, no behaviour change. The shared primitives, the ranking keys and the DFlash detector were spread through model_config alongside unrelated model handling, and the same rules are reached from four different paths, so they now live in one package. model_config re-exports every moved name, so callers and tests that import them from there keep working. The package deliberately does not import model_config at module import time. The GGUF split and quant naming helpers stay where they are, since non-drafter code shares them, and are imported per call instead. * Give the guard's DFlash bound a name and a home The bound was fifteen lines of generator plus the comment explaining why it is a max over shard sets rather than the best-ranked candidate, inline in the middle of a function that also sizes mmproj, MTP and DSpark. It is pure arithmetic over a listing, so it moves to drafters.budget with the reasoning attached to it. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Studio: fix the DFlash lint gate, and carry over what #6747 got right Five changes on top of the DFlash drafter work. Lint gate. The compatibility shim re-exporting the moved drafter helpers from model_config tripped scripts/verify_import_hoist.py, whose __all__ exemption is scoped to package __init__.py and which ships a reexport_in_ordinary_module_is_still_blocked self-test. The shim is gone: the module imports only what it still calls, and every other call site imports from utils.models.drafters directly. dspark_preference_key stays reachable from model_config as a delegating def, because repointing routes/inference.py's pre-existing function-local import is the verifier's TARGET-CHANGED case and its relocation exemption only covers module-level imports. Download plan. preferred_dflash_sibling in hub/utils/gguf_plan.py, and the sidecar as an expected file on every GgufVariantPlan. The sidecar was fetched but the hub manifest never knew about it, so download progress under-counted by ~1.5 GiB. Ranked with dflash_repo_preference_key, so the plan and the loader cannot disagree, and per variant, so a multi-family repo does not hand variant B the drafter named after variant A. Capability-regained retry. A load that stood down because llama-server could not run the drafter told the user to update, then deduped the reload the update was meant to repair. spec_binary_fallback_can_retry re-reads the binary, asking about the capability the drafter kind actually needs rather than the reason code, since every kind records the same binary_no_mtp. Transient fetch retry. _download_companion_gguf gained on_transient_failure, so a listing that never answered or a download that dropped is worth one more Apply. Permanent Hub errors, a full or unwritable cache, offline mode and cancellation are unaffected, and a header rejection still falls through to the next candidate rather than counting as transient. The probe cache key moved from (path, int(mtime)) to (path, st_mtime_ns, st_size), so an update landing in the same second as the probe is not answered with the old build's capabilities. CLI. unsloth_cli/_inference.py passes gguf_dflash_file into the GGUF load, so the managed CLI path engages DFlash instead of silently running without it. No vision gate, now measured rather than argued. Muse-Glimmer-30B UD-Q4_K_XL with mmproj-kquant and dflash-kquant, llama.cpp b10342, one B200, n_max=2, greedy, on a prompt carrying ~545 image tokens: 92.1 to 114.2 tok/s at 0.646 acceptance, greedy output byte-identical to the drafter-free run, no load failure. The comment at the Auto promotion site cited llama.cpp #22673, which is an MTP result, for a DFlash decision; it now cites the measurement. Also fixes test_from_identifier_never_reads_a_sidecar_outside_the_boundary, which patched is_dflash_architecture on the re-exporting module rather than the one detect_dflash_file resolves it in, so its reads == [] assertion held whether or not the lease boundary worked. * Tighten the DFlash comments for PR #8338 * Apply ruff-format kwarg spacing for PR #8338 * Fix the DFlash download plan and two stale-state reloads for PR #8338 Five review items, all reproduced first. The download plan promised the wrong files. A split sidecar contributed only its first shard, so the variant read complete while the loader's completeness check then refused the companion; it now carries the whole shard family. The pairing weight came from the listing's first sibling while plan_from_expected_files keeps the lexicographically first family, so a two-family variant key planned the discarded family's sidecar; both now use the kept family. And a root-level dflash- prefix is one real weights carry, which a listing cannot tell apart from a drafter, so a 54 GB model was planned as a companion to a 15 GB variant; a candidate is now bounded by the weights it would draft for, since a drafter is a few layers of its target and cannot outweigh it. The training coexistence guard charged the Auto DFlash sidecar even when extra args owned --spec-type, which stops the loader's promotion, so a chat load could be refused with 409 for bytes nothing would open. Extra args asking for draft-dflash keep the charge. The diffusion early-return cleared the speculative fallback state but not the DFlash retry flag, and discovery runs before the metadata read that classifies the model, so a transient sidecar failure tore down a healthy diffusion server on every Apply. Each fix has a regression test that fails without it. * Carry the DFlash plan bounds into the runtime paths for PR #8338 Four review items from the second round, each reproduced first. The budget still charged a forced dflash mode when extra args owned --spec-type. _build_speculative_flags returns before any mode branch in that case, so neither the forced mode nor the Auto promotion reaches the sidecar; only extra args asking for draft-dflash themselves still pay. The runtime picker had no size bound, so a root-level ordinary weight carrying the dflash- prefix downloaded in full before its header could be read, which is exactly what the download plan now refuses. It applies the same bound, sized from the repo listing, and an unavailable size leaves the candidate eligible as before. A permanent listing error records no answer at all, so _dflash_sidecar_absent stayed False and the drafter_not_found arm relaunched a healthy drafter-free server on every Apply. DFlash asks through _dflash_retry_needed instead, which is set only for the failures worth another attempt. A listing holding part of a split companion returned its first shard as usable, contradicting the complete-set checks on snapshot and cache reuse and handing llama-server a set it cannot open. The filename carries the set size, so the listing is now checked before the download. Each fix has a regression test that fails without it. * Make the DFlash size and split rules agree across plan, fetch and guard for PR #8338 Six review items from the third round, each reproduced first. Five are places the previous round's rules had not reached. dflash_plan_files now filters candidate families before ranking rather than after, so a half-published split set or an oversized ordinary weight at the top of the order steps aside for a usable sidecar behind it instead of taking the plan down with it. It also applies the split-completeness rule the runtime got last round, since planning a set the listing only half carries reports the download complete and then loses DFlash. The runtime size bound compared the picked shard rather than its whole set, so a split ordinary weight whose halves each sit under the target still downloaded in full. It sums the family now, through a shared helper. The training coexistence guard took the maximum over every root candidate with no size bound at all, charging gigabytes for files the fetch itself refuses. dflash_budget_bytes takes the target size and drops them. The incomplete-split rejection added last round lands after outcome["listed"] is set, so DSpark read a settled answer as retryable and relaunched a healthy server on every Apply. It records absence explicitly. SpeculativeType omitted dflash, so Typer rejected --speculative-type dflash before any of the new loading code ran and the mode was reachable only through Auto. Each fix has a regression test that fails without it. * Price DSpark by shard set and share one split-listing rule for PR #8338 Four review items from the fourth round, two of them under-charges that could admit a load beside a running training job and then exhaust VRAM. The guard priced a remote DSpark sidecar as the single file the ranking picked, while llama-server maps every shard of a split set, so a two-shard sidecar was budgeted at roughly half its resident weight. DSpark candidates are grouped into shard families now and the selected family's total is charged, matching what DFlash already did. Auto granted DSpark first refusal on the strength of the listing alone. Since the fetch now refuses an incomplete split set, the load falls through to DFlash, which can be the larger of the two, and the guard had already returned the DSpark figure. Only a complete set settles it. The runtime DFlash picker filtered incomplete families after ranking rather than before, so a half-published set at the top of the order returned a shard, _download_companion_gguf refused it, and the loop ended instead of reaching the complete sidecar behind it. Extras owning --spec-type with their own --model-draft charged the discovered sidecar as well, though _build_speculative_flags returns before that one is emitted. Only the drafter that launches is charged now. Extras without --spec-type still charge both, since Studio emits its own and which lands is genuinely unknown. The listing completeness rule was about to have three copies, so it moved into utils.models.drafters as split_listing_is_complete and the plan, the fetch and the guard all call it. Each fix has a regression test that fails without it. * Tighten the DFlash review-round comments for PR #8338 Comments and docstrings only, no code change: verified with comment_tools.py check and the prepush gate's comment-only mode. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> |
||
|
Daniel Han
|
7116d5e6c0
|
Document why resumed Hermes one-shots re-add --yolo and --accept-hooks (#8431) | ||
|
Daniel Han
|
428a14f145
|
Scope the Studio DNS-pinning opt-out to proxied fetches (#8420)
* Scope the Studio DNS-pinning opt-out to proxied fetches * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Tighten the comments on the proxy-scoped fetch path * Decide proxy routing on the host urllib tests, and hold the opener to it * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Match ProxyHandler's lowercasing when testing the proxy scheme key * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Trim the routing comment --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> |
||
|
Daniel Han
|
88c54835a7
|
CLI: stop verify-install describing an unrelated venv around an editable checkout (#8308)
`unsloth studio verify-install` resolves which prefix owns
studio/install_manifest.py by walking up from the module file to the first
pyvenv.cfg. After `./install.sh --local` that module lives in the repo, so the
first pyvenv.cfg above it is whatever venv the clone happens to sit inside,
which is frequently unrelated to the managed install.
It then walked that venv's site-packages and reported every managed dependency
as missing, so a healthy install printed
Unsloth Studio install is incomplete (studio_install_incomplete).
missing packages: matplotlib, nest_asyncio, datasets, huggingface-hub, ...
repair with: unsloth studio update
and exited 1, even though all of those import fine in the managed venv.
setup.sh / setup.ps1 gate their "already up to date" fast path on that exit
code, so every install re-ran the full dependency pass, and the desktop
preflight reads the same reason string.
A prefix now only owns the module when the module actually lives in that
prefix's site-packages. An editable checkout resolves to no owner, which falls
back to sys.prefix: the venv the CLI is running in, which is the managed one.
|
||
|
Daniel Han
|
c82836d0ad
|
Read PSModulePath under the key Windows actually exports (#8304)
* Read PSModulePath under the key Windows actually exports os.environ upper-cases keys on Windows, so dict(os.environ) carries PSMODULEPATH and a plain dict is case-sensitive. _profile_probe_env read "PSModulePath", always got the empty default, and wrote back a second key differing only by case, so the probe child lost the caller's module entries instead of having them reordered behind Windows PowerShell's own. Resolve the key case-insensitively and update it in place. This is what has failed parity (windows-latest) on every branch since #8161: the five PSModulePath tests read the same missing key out of the returned dict. * Tighten the comments on the module-path key lookup |
||
|
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> |
||
Lee Jackson
|
5f5f1e3550
|
Count a RECORD path claim before the row is filtered out (#8181)
* Count a path claim before the row is filtered out * Tighten the comments on the reordered claim count --------- Co-authored-by: danielhanchen <danielhanchen@gmail.com> |
||
|
Daniel Han
|
c2c16b7293
|
Studio: exempt shared top-level trees from the integrity check for our own wheels too (#8179) | ||
Nilay
|
bece6d2ab4
|
Preflight the Codex GGUF check before loading the model (#7873)
* Preflight Codex GGUF check before the model load * Skip the Codex GGUF preflight for remote studio targets * Preflight owner-less shorthands and ignore auxiliary GGUFs * Filter unambiguous big-endian GGUFs in the Codex preflight * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Honor HF offline mode and skip Codex preflight for unverified loopback servers * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Stub the hub listing in fake_studio so codex tests stay offline * Defer bare names to attached servers and gate the hint probe to hub ids * Ask the attached server for GGUF variants and guard malformed HF_ENDPOINT * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Skip the attach variants probe for direct .gguf files * Probe shorthand candidates on attach and gate preflight to auto-starts * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Resolve gguf-variants with load-path parity and defend the attach probe * Run the Codex preload gate only when a load is imminent * Keep sibling quants and vision for a direct gguf in a marked directory * Settle live raw attach answers, complete direct-file variants, filter drafters * Track the existence-first local branch in the picker contract test * Refuse auxiliary direct ggufs, round-trip loose quants, gate the requested variant * Refuse companion paths, casefold variant match, keep local answers local, mark torn splits * Gate direct-path variants, mark empty and mis-indexed splits partial * Judge direct variants with parent context and require non-empty split siblings * Accept exactly the direct-file variant labels the server resolves * Mirror the variant fallback label and fail incomplete loopback direct files * Resolve symlinked shards, ask the server for foreign direct variants, match local answers strictly * Accept the bpw-stripped label when resolving a loose gguf variant * Report local resolution to the gate, settle explicit-path empties, accept full stems * Carry local resolution through the legacy route and label rows with the load extractor * Accept bpw-stripped labels at the gate and refuse big-endian direct files * Ignore torn local rows for variant matching and refuse broken direct symlinks * Scope readiness to the file the load uses and align label acceptance * Resolve advertised labels, prefer whole variantless picks, filter be rows by the loader * Probe remote direct paths, align split grammar, filter local be rows by the loader * Keep quant tokens to the basename and judge legacy local answers the same * Mirror basename tokens, settle local empties, and probe unpickable local shapes * Follow symlinked shard sets and let a named sibling answer for refused paths * Judge readiness by the load split grammar and keep hub-shaped gguf ids remote * Answer the pre-load gate with the load resolver instead of mirroring it * List every resolver-accepted label and let the probe judge direct variants * Follow split symlinks through the load, flag cleanable rows, and defer non-local raw answers * Bind completeness and aliases to the resolver's chosen file, one walk per row * Fail a direct gguf path this process can see is missing * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Offer the relative fallback label and ask the server about nested drafter folders * Ask the server about drafter folders and let the resident model answer first * Apply pinned ruff formatting * Resolve relative bases, compare resident quants exactly, trust loadable on empty answers * Run the preload gate when a run knob changes the load intent * Follow upstream: a direct file loads itself whatever quant was asked for * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Report a missing resolved path as not loadable * Refuse companions with any variant, defer foreign path syntax, keep symlink aliases * Confirm the loaded identifier before treating a direct path as resident * Tighten the comments added by this change * Cover the Windows side of the path syntax guard for PR #7873 * Check direct-file readiness even when a variant is supplied for PR #7873 * Judge the completion scan's big-endian filter with the loader label for PR #7873 * Normalize a local identifier before probing it for PR #7873 * Keep a read error unknown instead of a definite absence for PR #7873 * Confirm the server is this machine before judging its filesystem for PR #7873 * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Keep a locked direct file unanswered instead of authoritative for PR #7873 * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Judge WSL drive paths with the loader's normalizer for PR #7873 * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Probe unjudgeable spellings and honor negative verdicts for PR #7873 * Tighten comments for PR #7873 --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: danielhanchen <danielhanchen@gmail.com> |
||
|
Daniel Han
|
b741de5f36
|
Studio: say what a failed launcher move-aside costs (#8109)
* Studio: say what a failed launcher move-aside costs The Windows update transaction frees Scripts\unsloth.exe before setup so the installer can publish a replacement. When os.replace cannot free it, because antivirus or an ACL is holding the file, the update continues with a warning. Continuing is right: an antivirus hold must not make the environment unupdatable. But the consequence was invisible. uv only self-replaces its own executable, so it cannot replace a launcher it could not move, and the pip fallback strips --upgrade-package and finds the bare unsloth requirement already satisfied. Setup then exits 0 with unsloth still at its old version, the update reports success, and the desktop keeps finding the backend stale and retrying the repair. Name the cost and what to do about it, so a partial update is not mistaken for a complete one. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Scope the move-aside test to the move, not every os.replace Patching os.replace wholesale also broke _atomic_copy's backup, so the test was asserting on a compound failure rather than the one it names. Fail only the .update-stale rename, and assert the backup warning is absent to keep it that way. * Correct the comment and pin the backup in the move-aside test Windows does allow renaming a running image, so saying uv cannot replace the launcher stated an OS limit where the truth is an implementation one: uv self-replaces only its own exe and otherwise deletes outright, so its uninstall is what fails here. The test now samples the backup during setup, where it still exists, to pin that only the move aside failed. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> |
||
Etherl
|
e0a2bd8317
|
Studio: preserve the Windows launcher during updates (#8092)
* Fix Windows Studio launcher updates * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fix the remaining damaged-file reports and launcher recovery for PR #8092 The report this PR targets listed three damaged files. Preserving the launcher fixes one; the other two still fail the update on their own, since _fail_if_install_damaged exits 1 on any finding. Both are produced by our own installer, so the update they fail is the update meant to repair them. einx and torchao both ship a top-level test/conftest.py, and install_python_stack.py force-reinstalls torchao every update, so pip deletes the file and the pinned torchao does not ship it. package-lock.json is rewritten in place by setup.ps1 and setup.sh, which run npm install inside the installed tree; under legacy-peer-deps npm dedupes hoisted entries and the file shrinks below its recorded size, reproduced exactly as 28473 to 27225. Drop both classes while reading RECORD rather than when reporting, so a filtered row also stays out of the ownership tally and the limit budget and cannot crowd out a real finding. Mirrored into the sidecar scanner, whose docstring asks for the two predicates to be kept in sync. Also three fixes to the transaction itself: - Recover from the hardlinked bin/unsloth.exe shim, which survives the old updater's .deleteme unlink. - Warn instead of exiting when the launcher is missing or invalid. An install already broken by the old updater has neither launcher nor .deleteme, and exiting before setup stopped exactly those users from updating. validate_launcher still judges the result. - Gate recovery on validity rather than existence, and treat a failed backup as a missing safety net rather than a fatal error. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Resolve the launcher from the managed venv and keep a good backup Three follow-ups from review. The transaction resolved Scripts from sys.executable, but setup.ps1 installs into STUDIO_HOME/unsloth_studio (setup.ps1:3411). When a pip-installed or checkout CLI drives the update, those differ, so it backed up and --version validated the caller's launcher while the one actually being replaced went unprotected. Resolve the managed venv the same way _studio_deps._managed_root does for the damage scan. __enter__ overwrote the transaction backup unconditionally, guarded only by the two-byte MZ check. A backup outlives __enter__ only when a previous run died before validating, so it holds the last launcher known to run; overwriting it with a PE-shaped but unvalidated canonical file destroyed the only recovery copy. Write a backup only when there is no usable one already. package-lock.json was skipped outright, which dropped its existence check too. npm rewrites it in place but never deletes it, so keep the row and drop only its recorded size. Also add scripts/ to the shared non-runtime roots: unsloth_zoo ships a top-level scripts/, the same squatted-namespace shape as einx's test/, and it has no __init__.py so nothing imports it. * Move the launcher aside for setup, and restore it if nothing replaces it I rejected this on the strength of setup.ps1:4386-4391, which says renaming the running launcher "only ever failed (WinError 32)". That is not right. A probe on windows-latest builds a real console-script package, runs it, and renames the live launcher: the rename succeeds and a replacement can then be written at the freed path. RESULT idle-launcher: RENAME SUCCEEDED RESULT running-launcher: RENAME SUCCEEDED RESULT publish-replacement: WROTE a new launcher at the canonical path main moved the launcher aside before setup (studio.py:3182) and this branch had removed it, so uv could no longer replace Scripts\unsloth.exe. uv only self-replaces its own executable and deletes a third-party console script outright, and the pip fallback then no-ops on the already-satisfied bare unsloth, so the upgrade was silently skipped. Move it aside again, but keep what this branch was written for: when setup publishes no launcher, validate_launcher restores it rather than leaving the venv with none. That was the original bug, where the old updater renamed the launcher away and then deleted its own .deleteme. Restore prefers the backup over the moved-aside copy: the backup is the last launcher known to run, the moved-aside one is only this run's unvalidated canonical file. The mocked harness cannot reproduce a sharing violation, so the tests pin the invariant (the canonical path is free during setup, a recoverable copy always exists) while the CI probe covers the Windows semantics. * Tell a missing launcher from a broken one, and retry restores Two follow-ups, both from the restore path added in 724b274d4. Restoring before the health check could not tell setup publishing nothing from setup publishing something unusable. A zero-byte or non-PE replacement was quietly swapped for the previous launcher, which then passed --version, so the update reported success and deleted its own recovery copies. Sample whether setup published anything before any restore: nothing published and a good restore is the no-op update this transaction exists for, while a launcher setup did write and that cannot run stays a failure even though the previous one goes back. _restore_backup also picked the first candidate passing the two-byte header check and stopped there. Backups are taken after only that check, so an interrupted run can leave a PE-shaped but non-runnable one, and preferring it stranded the working launcher this run had moved aside. Split restoration: _restore_from puts one candidate back, and _restore_runnable walks the candidates until one actually runs. * Restore a runnable launcher on exceptional exit, narrow the exemption __exit__ restored the first PE-shaped candidate, so an interrupted run's non-runnable backup was installed over the working launcher this run had moved aside, and it could undo a restore validate_launcher had just made. It now uses _restore_runnable, which leaves an already-working launcher alone, walks the candidates until one passes --version, and falls back to the best candidate rather than whichever was tried last. The shared-namespace exemption was also too broad. I justified it on the grounds that tests/ and scripts/ ship no __init__.py, which is wrong: PEP 420 makes them importable, and this repo does 'from scripts import ...' itself. Restrict it to distributions Unsloth does not ship, so einx and torchao squatting on a top-level test/ is exempt while our own top-level trees stay checked. * Keep all four recovery copies as runtime candidates _recovery_candidates only offered the backup and the moved-aside copy, so when an interrupted run left a PE-shaped but non-runnable backup and the legacy .deleteme or the PATH shim was still good, the bad backup was accepted on its header alone and the good copy was never reached. The update then failed every time with the broken bytes canonical. All four are candidates now, deduplicated by normalised path, and _restore_runnable walks them until one passes --version. * Move the update lock out of the replaceable venv Resolving Scripts from the managed venv put the lock inside $VenvDir, and setup.ps1:3748 removes that whole directory to rebuild a stale torch. Windows refuses a recursive delete while a handle inside it is open, so an external CLI holding the lock for the whole setup run failed the repair with "Could not remove stale venv". Keep it under the Studio home instead, which is stable and is the right grain anyway: it is what names the managed venv. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: danielhanchen <danielhanchen@gmail.com> |
||
|
Etherl
|
d495a09bf0
|
Guard Windows Studio installs against active runtimes (#7764)
* Fix Studio installer runtime race * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Close remaining Studio installer races * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Keep Windows installer tests Windows-only * Stabilize Windows process guard test * Coordinate terminal Studio launches * Guard all managed Studio launches * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Coordinate custom Studio roots * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Handle direct GGUF settings rows * Apply repository formatter * Close remaining Studio update races * Handle Windows runtime gate CI edge cases * Verify the updater parent shim by image * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Allow verified updater shim chains * Stabilize Windows process guard test * Handle Windows console-script redirectors * Close remaining Windows updater guard gaps * Handle spaced and repeated Windows update shells * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Resolve Tauri root aliases before validation * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fix custom-root locks and updater ancestry * Align Windows runtime identity checks * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Scope desktop fallback to the current user * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Preserve drive-root mutex identity * Use ordinal semantics in Studio idle scans * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Guard standalone Studio setup mutations * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Restore the setup gate handoff independently * Version the Windows installer native helper * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Use ordinal path comparison in Studio runtime scan * Tighten Studio runtime gate test comments * Exempt the venv Python redirector in the Studio runtime gate Windows venv Scripts\python.exe is a redirector that runs base Python as a child, so `unsloth studio setup` runs as unsloth.exe -> python.exe -> us. The ancestor walk only exempted the unsloth.exe shims and stopped at the redirector, so the installer flagged its own launcher and every Windows install failed with "The managed Studio environment is in use by unsloth.exe". Carry one redirector as pending and exempt it only when a shim sits directly above it, so a managed backend that spawns an update still blocks. Also stop gating the protected shim paths on exists(), so a shim renamed out of the way mid-update is still recognised. Drop "Studio" from the two runtime-lock messages so process.rs satisfies the desktop branding contract. * Close the redirector exemption when the updater is the managed image Only a base interpreter runs under a venv redirector. If our own executable is inside the managed root there is no redirector above us, so a managed parent is a real consumer and must keep blocking. Adds the regression to the redirector test. * Key the redirector exemption on sys.executable, not on a shim above it The Tauri updater runs `<venv>\Scripts\python.exe -I -c ... studio update` directly, so its chain is tauri.exe -> redirector -> base Python with no unsloth.exe in it. Requiring a shim above the redirector made every desktop update block on its own launcher. A venv redirector starts base Python as a child and waits, so when we are the base image and sys.executable still names the managed interpreter, our direct parent is that launcher. Exempt exactly that hop; ancestors above it must still be shims, and a managed image at depth two or more keeps blocking. * Stop the x86 guard test racing its own probe The 32-bit leg fired one scan against a probe that lives about five seconds, while a WOW64 shell start plus the Add-Type compile regularly costs more than that, so it read an empty list on a Windows runner. Give the probe a long life and retry like the 64-bit sibling already does. The assertion is unchanged. * Tighten comments in the Studio runtime gate changes --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: danielhanchen <michaelhan2050@gmail.com> |
||
|
Nilay
|
e5c55e3577
|
Validate an explicit GGUF variant before unloading the resident model (#7862) | ||
oobabooga
|
2e593b3f85
|
Studio: add opt-in DSpark speculative decoding (#7968)
* Add opt-in DSpark speculative decoding * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fix lint blockers, retry-guard test and DSpark broken-build gate for PR #7968 Three fixes for the failing CI on this branch: - Drop the unused `Literal` import from unsloth_cli/commands/chat.py and inference.py. The options are typed with `SpeculativeType`, so the added import was a leftover and tripped the import-hoist blocker. - Make test_the_startup_retry_drops_the_mtp_the_extras_and_the_env_carry whitespace insensitive. The guard now names both drafters, so formatting wrapped the call and the literal substring assertion no longer matched. It asserts on both `_extra_args_requests_mtp` and `_extra_args_requests_dspark` now, so the DSpark half is covered too. - Gate DSpark on the whole broken build window instead of one tag. The reshape regression is ggml-org/llama.cpp#26531 and the fix is #26577, so every prebuilt based on b10259 through b10268 aborts on a DSpark load, not only b10265-mix-89aa77b. Matching the base build number keeps source builds unaffected, since those carry no install marker. * Probe DSpark support before downloading the sidecar The ~11 GB DSpark sidecar was fetched at llama_cpp.py:8664 while the first supports_dspark consumer sat ~480 lines later, so a binary that cannot run draft-dspark paid for the whole download and then fell back without ever opening the file. probe_server_capabilities is already called just above for supports_kv_unified, so the answer is in scope and cached and the check costs nothing. This is the default path right now, not an edge case: the shipped unslothai/llama.cpp prebuilt b10265-mix-89aa77b sits inside the known-broken b10259..b10268 window, so supports_dspark is False on a standard install. Also swaps the order of the first two DSpark fallbacks. Now that the fetch is gated on the same answer, a gated binary leaves no sidecar, and checking the drafter first reported "no matching dspark-*.gguf sidecar was found" and told the user to place a file that was never the problem, while re-loading on every Apply through the drafter_not_found dedup branch. Adds three regression tests, all of which fail without this change. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Enforce fixed fit for pass-through DSpark, size split sidecars, correct the hint Three fixes from the latest review round. Extras that own --spec-type return from _build_speculative_flags before _speculative_type is set, so the --fit strip keyed only on that field never fired for a pass-through DSpark launch and a user --fit on survived. DSpark's layout cannot be reshaped, so that aborts the load. The strip now also reads the accumulated spec types, which covers both the flag and the env. The training coexistence estimate sized the drafter with a bare stat(), while the main weight beside it already used the split-aware helper. Discovery hands back shard 1, so a split sidecar was counted at one shard and the guard could admit a load that evicts the training run it exists to protect. The Speculative Decoding hint promised "no accuracy hit" unconditionally, which DSpark does not meet: on a quantized target its greedy output can differ from a non speculative run (ggml-org/llama.cpp#25618). Measured here on DeepSeek-V4-Flash-0731 UD-Q4_K_XL, where the same greedy conversation produced 10570 tokens without a drafter and 14687 with one. The claim now stays with Auto, and DSpark carries its own caveat. Both backend fixes have regression tests that fail without them. * Pin fit off for pass-through DSpark, keep cached sidecars visible, reclaim them on delete * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Allow DSpark under --fit on: fitting only skips the sidecar reserve * Gate the training-guard DSpark estimate on binary support, fix the picker contract marker * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Default Auto to DSpark when a sidecar is available, and stop the two reload loops it exposed * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Charge no drafter when forced DSpark is gated off by the binary * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Gate remote Auto DSpark sizing, retry only a failed sidecar fetch, refresh the Auto hint * Apply ruff kwarg-spacing formatting --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: danielhanchen <unslothai@gmail.com> |
||
|
Long Yixing
|
4a4509c156
|
feat(studio): add settings-managed remote access (#7875)
* refactor(studio): make cloudflare tunnels runtime-managed * feat(studio): add public access controls * fix(studio): preserve public stop responses * feat(studio): show public access settings * fix(studio): report settings-managed public links * fix(studio): respect launch tunnel ownership * fix(studio): apply public runtime trust policy * refactor(studio): rename public access to remote access * feat(studio): serve web ui through desktop remote access * feat(studio): set the remote password from desktop The desktop app signs in with a local secret and never sees the seeded administrator password, which is only printed to a terminal. Remote access refuses to open a tunnel while that seeded password is still in place, so a desktop user had no way forward: the change-password flow needs a current password they do not have, and the General tab hides its password row on desktop. Add a Remote password row to the Remote access section, desktop only. While the seeded password is pending it sets the first password through a new POST /api/auth/desktop-initial-password; once a password exists the same row changes it through the existing endpoint. The new route accepts only a desktop-issued JWT (web sessions and API keys are refused), applies only while the seeded credential is unchanged, and binds its write to the credential version it read so a concurrent web change or CLI reset is not overwritten. Both password routes now keep the local desktop credential valid and mint desktop-flagged tokens when the caller is the desktop app, so auto-auth stays passwordless and the session survives the change it made. Browser callers keep their existing behavior: the credential is revoked and ordinary tokens are issued. Remote access status reports password_pending on its own, independent of block precedence, so the row is correct even when another block hides the reason. Re-reading status after either operation clears the pending-password block and lets the tunnel start; remote browsers then sign in as unsloth with the new password. The action reuses the account password dialog in an initial-password mode rather than adding a component, and adds no file. * feat(studio): redesign remote access settings as a status card The remote access controls rendered as flat settings rows, so the feature read as hidden and the tunnel URL was squeezed into a description cell. Rebuild remote-access-section.tsx as a bordered card: - header with globe icon, title, live status dot plus state text and the start/stop action together - online state gets a dedicated URL panel with a copy button, a QR button that opens a scannable code for phones (react-qr-code, svg, zero deps), and a note that the URL plus remote password grants sign in - block reasons and tunnel errors render as their own strip, destructive styling on error - shine sweep on the status text while starting or stopping - password and auto start rows move below a divider inside the card Polling and start/stop/auto-start logic are unchanged. * fix(studio): stop tunnel dns wait from stalling on negative caches Starting remote access took 45-48s while a manual tunnel came online in about 8s. _wait_for_dns queried DoH immediately after the edge connection registered, before the fresh trycloudflare hostname propagated, and the resolver negative-cached the NXDOMAIN (trycloudflare.com publishes a 1800s SOA negative TTL; 1.1.1.1 was observed serving the stale negative for about 40s while the loop re-asked it every 2s). - delay the first lookup 3s so it cannot seed those negative caches - ask two independent resolvers per round (cloudflare and google DoH) - cap the dns wait at 20s so it can never starve the health probe, which is what actually gates advertising the URL Start to online now measures about 12s end to end against a live backend. The consecutive-error bailout for blocked DoH is unchanged. * fix(studio): drop stale stopping status after a finished stop A stop worker outlives its completed stop by up to about a second while draining stop responses. During that window remote_access_status kept reporting state stopping and forced can_start false, so a quick stop-then-start flashed "Stopping" in the UI and the start request could 409 as operation_in_progress. Once the tunnel controller reports off with no pending stop handle the stop is done, so a still-alive stop worker no longer masks the idle state or a newly admitted start. Unconfirmed terminations (stop_pending) keep reporting stopping so retry stays visible. * fix(studio): resolve the tunnel hostname through one resolver Asking a second DoH resolver in the same round cannot rescue the poll: both queries land milliseconds apart, so the round that seeds one resolver's negative cache seeds the other's too. It also disclosed the freshly issued hostname to a third party on nearly every start, where the provider that handed the name out already knows it. The hold-off and the wait cap stay, which is where the measured speedup comes from. The cap trades one case away: a hostname that only propagates after 20s now meets the OS resolver while it is still missing, and because the tunnel has registered by then a failed probe ends the attempt instead of retrying over http2. In exchange every start whose record exists by 20s spends the rest of the deadline on probe attempts rather than a single one at 45s. * fix(studio): report a stop only while its teardown is outstanding A stop worker outlives the teardown it performed, so reporting stopping until that thread exits hid the tunnel already being off and refused a start that would have succeeded. The worker's recorded admission sits behind the control token once its teardown advanced the generation, but a start advances the generation the same way, so only that advance together with an off tunnel and no pending stop shows the teardown actually happened. * fix(studio): keep the remote access card legible The status text goes back to full opacity. The shimmer needs partly transparent glyphs, which put the transitional states below the contrast that same text had before, and a viewer who asked for reduced motion paid that cost with no sweep to show for it. The status dot already marks the transition. The auto-start switch's accessible name matches its visible label again after the rename, and the block reason no longer borrows the error colour when a failed tunnel is not what it is reporting. Moving the URL panel and the block-reason strip out of the section body keeps the section at the complexity it had before the card, and SettingsRow drops labelAccessory along with the row that used it. * perf(studio): verify a new tunnel at the edge instead of through DNS Cloudflare routes quick tunnels by TLS SNI, so the edge serves a tunnel as soon as its connection registers, before the hostname resolves anywhere. Probing an edge address with the tunnel's name in SNI and Host takes that hostname's DNS off the startup path, along with the wait that existed only to keep an early lookup from caching the miss. Measured over four fresh tunnels each: verification after registration falls from 10.97s to 6.61s on average, and a whole start from 22.9s to 16.8s. It also removes the case behind the slowest starts, where one DoH query sent before the record propagates is negative-cached, blinding the poll until its own cap expires and pushing a start past 30s. The hostname path stays as the fallback for networks that block direct addresses or intercept TLS. It is entered as soon as the edge looks unreachable rather than merely unready: an error 1033 page is an answer, a failed connection is not. * fix(studio): bound the edge probe and dial distinct frontends Verifying a quick tunnel at Cloudflare's edge shipped with two defects. The wait checked the clock only after trying every address, so a full pass always ran: verify_public_url with a 0.05s timeout still took 12s, against 1.65s before the edge probe existed, and a pass of two real TLS handshakes overshot the 15s cap to about 19s, taken out of the hostname fallback's share of the same deadline. The clock is now read before every attempt, and no attempt is given more time than the deadline leaves. The addresses were also not distinct. macOS reports the A records again as IPv4-mapped under AF_INET6, so taking one per family dialled a single frontend twice, as 104.16.230.132 and ::ffff:104.16.230.132. That doubled the cost of a pass, halved the poll rate, and let two dependent samples of one address exhaust the unreachable counter. Deduplicating by frontend yields 104.16.230.132 and 104.16.231.132 here. An intercepting proxy answering with its own page is an answer rather than an unreachable edge, so it resets that counter exactly as error 1033 does and the wait ends at the cap instead. The comments claimed interception left through the early exit; they now describe what it does. * fix(studio): clear the python floor, read encoding and locale parity gates The tunnel test annotated a helper with `str | None`, which evaluates on the 3.9 floor that pyproject declares and pushed the studio union ratchet from 35 files to 36. Six new test reads took the platform default encoding, so they break on Windows the moment those files gain a non-ASCII byte. The remote password dialog's six new strings existed only in en, which strict locale parity rejects; they are now translated into every overlay. * Fix settings-managed tunnels dying on Linux, and stop-during-retry orphans for PR #7875 PR_SET_PDEATHSIG is a parent-THREAD death signal, not parent-process: the kernel fires it when the thread that forked the child exits. start_remote_access forks cloudflared from a short-lived worker thread, so the connector was SIGTERMed about a second after it came online, every time. Reproduced 3/3 on a real install: the URL is logged, then state goes to error with "cloudflared exited". Auto-start shares the same worker, so it was affected too. Only Linux and WSL are hit. macOS arms no per-thread signal and Windows uses a process-wide Job Object, so both were fine, as was the launch path, which forks from the main thread. Changes: - process_lifetime: spawn_on_lifetime_thread() performs the fork on one process-lifetime daemon thread, so PDEATHSIG means "die with the parent process" again. Non-Linux spawns directly. Falls back to an inline spawn when no helper thread can be obtained, so it can never block. - cloudflare_tunnel: spawn through that helper. The abort branch now tears the connector down instead of returning the URL of a process it just detached, which could leave a live public tunnel no later stop_studio_tunnel() could reach. Restore _tunnel_state to "starting" after attempt 1's teardown so the http2 retry no longer advertises "stopping", which made stop_studio_tunnel() early-return and stop nothing, and made the Settings Stop route a no-op. - run.py: an explicit --cloudflare/--no-cloudflare/--secure on this invocation now beats an inherited _UNSLOTH_CLOUDFLARE_INTENT, so a stale export, Docker ENV or systemd Environment= cannot re-enable a tunnel the user opted out of. The marker still softens the compatibility --no-cloudflare into "unset". - Add tests for remote-access-state.ts, which had no coverage. No bugs found there; the logic is correct, it simply was not exercised. Verified: 701 passed in the focused backend slice (the one failure is a pre-existing missing optional dep, and fails on main too), 347 frontend tests, typecheck, biome and 80 simulation tests including a real-child-process tunnel lifecycle and Chromium/Firefox/WebKit CORS runs. * Surface the unstoppable-connector error, bound the Stop wait, unlatch polling for PR #7875 Follow-ups from reviewing the remote access flow on Linux. The tunnel lifetime fix landed already; these are the smaller things around it. - remote_access_status collapsed "cloudflared could not be stopped" into the generic "Cloudflare tunnel failed". That is the one error the user can act on: the connector's exit was never confirmed, so it still holds the runtime slot and Start stays disabled. Pass it through like the other known messages. - The Stop worker waited on a live start worker with no deadline, polling at 100 Hz. A start that never claims settings ownership (foreign owner, or one that bailed on admission) deferred the user's Stop for the full probe deadline, up to about 170s. Bound it at 5s and poll at 50 Hz. - setPollEnabled(false) had no path back to true. It fires when you stop a tunnel from a browser connected through that tunnel, which is correct, but the section then stayed dark until it unmounted. Resume on any later action or on a password-change refresh, since both prove the origin is reachable. - test_change_password_policy called change_password positionally, so the new is_desktop parameter defaulted to the Depends object, which is truthy. Harmless today because the assertions fire earlier, but it silently exercises the preserve-desktop-secret branch. Pass False explicitly. Verified: 230 passed in the focused backend slice, 347 frontend tests, typecheck, build, locale parity, and no new biome warnings. Re-checked on a real install: auto-start brought a tunnel up at boot and it stayed online, and Stop through that tunnel returned 200 in 0.07s. The one backend failure (test_health_response_reports_desktop_capability_fields) is a pre-existing environment gap and fails on main too. * Correct streaming_supported, shorten the CORS preflight window, guard the spawner across fork for PR #7875 Findings from simulating the remote access paths across platforms, browsers and upgrade scenarios. Three small fixes, each measured rather than reasoned. - streaming_supported was a hardcoded True. Every tunnel Studio opens is a Cloudflare Quick Tunnel, and Cloudflare documents that Quick Tunnels do not support Server-Sent Events. Measured against a real tunnel: a local SSE endpoint delivers 6 events over a 5.0s spread, and the same endpoint through the tunnel answers 200 with text/event-stream and then delivers nothing at all before the read times out. Chunked non-SSE streaming was blackholed the same way, so no transport change works around it. The field now reports whether a tunnel is currently carrying the traffic, so it is true locally and false while a tunnel is live. Nothing branches on it yet, which is exactly why it should be right before something does. - The CORS middleware inherited Starlette's 600s Access-Control-Max-Age. is_allowed_origin closes the instant the tunnel URL clears, but a preflight the browser already cached does not. Measured across four engines: after remote access was stopped, WebKit reused its cached preflight and the state-changing POST still reached the server, while Chromium, Firefox and Edge re-preflighted and were refused. Pinning max_age to 60 keeps preflight caching useful and makes revocation nearly as immediate as every other trust signal here. - spawn_on_lifetime_thread keeps a module-level lock and a helper thread. A fork child inherits the lock in whatever state it was in, and a spawner whose thread does not exist in the child. Reproduced: forking while the lock is held deadlocks the child permanently. Not reachable today, since the backend only uses the spawn start method and the one fork start method lives in the training worker, which never imports the tunnel. Registering an at-fork reset is three lines and closes the class of bug rather than the instance. Also verified and unchanged: no schema change in any store, so an old studio.db with no remote_access_auto_start row reads as false and fail-closed on corrupt or non-boolean values; the argument parser is byte-identical to main, so no launch flag default moved; the PR touches no hardware, GPU or desktop files, and all twelve OS x accelerator combinations produce an identical remote-access status. Host and Origin are forbidden request headers in all four engines, so no page script can forge the Cloudflare provenance gate. Verified: 95 simulation cases, 41 adversarial cases, 109 in the directly affected backend slice, 347 frontend tests, typecheck, build and locale parity. On a real install: auto-start brought a tunnel up at boot and it stayed online, Stop through that tunnel returned 200 in 0.09s, and the live status now reports max-age 60 and streaming_supported false. * Keep the desktop backend up without a dist, and settle self-origin Stop on off for PR #7875 The desktop spawns "studio --api-only" with no --frontend, and install.sh --tauri skips the frontend build outright. The new desktop-owned branch made _serve_frontend true for that launch, so an unresolvable studio/frontend/dist raised SystemExit before TAURI_PORT was emitted and the app never got a backend. The SPA only backs the optional remote web UI there, so warn and carry on API-only; a web UI launch still aborts loudly. A Stop sent from the tunnel's own origin is answered with a terminal off, then polling restarts in perform()'s finally and the first poll can still land inside the ~50ms teardown drain, where the backend reports "stopping". That overwrote the terminal off, cloudflared then exited, and the card latched on a permanent "Stopping" for a tunnel that was already down. Teardown frames no longer overwrite it, while a poll that can still stop the connector proves teardown was abandoned and takes the card back over. --------- Co-authored-by: Maheswar Kumar <110882203+mahiatlinux@users.noreply.github.com> Co-authored-by: danielhanchen <michaelhan2050@gmail.com> |
||
|
Daniel Han
|
6d0ac7c865
|
Studio: do not let the default tool policy reject every plain Anthropic message (#8023)
* Do not let the default tool policy reject every plain Anthropic message
`unsloth studio run` resolves the server-side tool policy to on unless --disable-tools
(resolve_tool_policy: "tools default on for every bind",
|
||
benj
|
a59ca88086
|
Fix studio server crash when launched without a console (Windows) (#7932)
* Fix studio server crash when launched without a console (Windows)
A hidden Windows subprocess (CREATE_NO_WINDOW, e.g. `unsloth studio` launched
from install.ps1 or a launcher) has no console, so the interpreter leaves
sys.stdout / sys.stderr as None. `_setup_server_disk_logging` wraps them in a
_TeeStream, and write()/flush()/close() delegated straight to the wrapped
stream, crashing on the first print with:
AttributeError: 'NoneType' object has no attribute 'write'
Make the tee tolerate a None wrapped stream: write() no-ops (still logging to
the session file), flush()/close() no-op, and __getattr__ raises AttributeError
instead of crashing. `_harden_console_close` also early-returns on None.
Adds an AST regression test (pinned the same way as the other studio tests
because importing run.py needs the full studio venv).
Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
* Normalize missing std streams so the console-less launch actually starts
The _TeeStream guards land too late to fix the reported crash. run.py imports
loggers (and so structlog) at line 109, and structlog binds `from sys import
stdout` at its own import time, so a None stdout is captured permanently before
run_server() begins. The first logger.info() then dies with "cannot create weak
reference to 'NoneType' object", 36 lines before _setup_server_disk_logging()
installs the tee. Even past that, uvicorn.Config() probes sys.stdout.isatty()
and LOGGING_CONFIG leaves use_colors as None, so startup aborts anyway. And
UNSLOTH_STUDIO_NO_FILE_LOG=1 skips the tee entirely, so the guards never apply.
Point the missing streams at the null device at the top of run.py instead, before
the loggers import. A null-device stream answers encoding/buffer/fileno/isatty/
reconfigure like a real one, so structlog, uvicorn and the __main__ failure
handler all work and nothing downstream needs its own None check. Streams that
already exist are left untouched by identity, so this is a no-op on Linux, macOS,
Colab (ipykernel OutStream), Tauri (piped stdout) and pytest capture.
Also pass our std handles to the backend on Windows: without them
CREATE_NO_WINDOW gives the child its own hidden console, so `unsloth studio > log`
captures nothing. This mirrors what the setup.ps1 call already does.
Tests: the AST helper accepted any `if ... is None` anywhere in the method, so it
passed on a _TeeStream that still raised AttributeError on the first write. It now
requires an early-exit guard naming the wrapped stream, plus the ordering
constraint above. Real behavioural coverage goes in
studio/backend/tests/test_server_disk_logging.py, which runs on Python 3.10-3.13
rather than tests/studio/ on 3.12 only.
Verified on Python 3.9-3.14 x structlog {24.1.0, 26.1.0} x uvicorn {0.51.0,
0.52.1}, and on windows-latest / macos-14 / ubuntu-latest. On real Windows,
DETACHED_PROCESS and pythonw.exe are what leave the streams as None;
CREATE_NO_WINDOW alone gives the child a hidden console with valid handles, so
the comments now say that instead.
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* Tighten the comments added by this PR
Comment and docstring text only, no code change. Keeps the load-bearing facts:
the ordering constraint against the loggers import, why the launcher hands its
std handles to the child, and why _guards_target is strict.
* Let the guard check see through a local alias
_guards_target only matched a guard that named the attribute directly, so the
idiomatic `stream = self._stream; if stream is None: return` refactor failed the
test even though it is correct. Track assignments whose value is the target and
accept a guard on either name. Still rejects a guard on the wrong object, a
guard with no early exit, and an alias of a different attribute.
* Correct two rationales in the comments
Omitting stdin from the Popen call does not withhold it: subprocess fills
hStdInput from GetStdHandle(STD_INPUT_HANDLE) whenever any other handle is
passed, so the old comment described a guarantee the code does not make.
State what the sys.__stdout__ assignment buys instead of asserting a generic
fallback: rich reads sys.__stdout__.fileno() at import and otherwise settles on
fds 0/1/2, which a process with no std handles does not have.
---------
Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
Co-authored-by: Daniel Han <unslothai@gmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
|
||
|
oobabooga
|
957b994299
|
Studio: simplify the GGUF loading backend (#7663) | ||
|
Daniel Han
|
5b61173042
|
Windows: move the installer out of System32 instead of failing in it (#7744)
* Windows: move the installer out of System32 instead of failing in it An elevated PowerShell opens in C:\Windows\System32, so `irm https://unsloth.ai/install.ps1 | iex` installs from there. install.ps1 never changed directory, so the failure only surfaced once `unsloth studio setup` ran, minutes later after PyTorch had downloaded, as a bare "unsloth studio setup failed (exit code 1)" followed by a full rollback. install.ps1 now relocates out of %SystemRoot% before any install work, and the CLI guard it would otherwise trip names the folder and prints the commands to fix it. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Inherit the real environment for the pwsh subprocess tests pwsh needs PATH and SystemRoot on Windows, and $HOME comes from HOMEDRIVE/HOMEPATH there, so the no-safe-directory case has to override that pair too. * Quote the recovery path and run the guard tests on Windows CI cd C:\Users\Jane Doe reaches Set-Location as two arguments and fails with "A positional parameter cannot be found that accepts argument 'Doe'", so the one line the message hands out to escape System32 did not run for any profile path containing a space. PowerShell now gets a single-quoted path (verbatim, with an embedded apostrophe doubled for C:\Users\O'Brien) and cmd a double-quoted one, which it needs anyway once command extensions are off. The new tests also only ran on Linux: tests/ is auto-discovered by the ubuntu-only repo-cpu job, and the cross-platform parity workflow names its files explicitly. Added the guard tests there so they run on windows-latest and macos-latest, which is where they are meaningful. * Pin the llama.cpp path by PowerShell location and skip a SYSTEM-account home IsPathRooted calls C:llama.cpp and \llama.cpp rooted, but both still move when Set-Location does, so the guard skipped rebasing exactly the paths that needed it. Resolving through the session PSPath resolver covers every partially qualified form. Not GetFullPath: that resolves against [Environment]::CurrentDirectory, which Set-Location does not update, so it would anchor to a different directory again. The CLI message had the same shape of bug: SYSTEM's USERPROFILE is C:\Windows\System32\config\systemprofile, so the recovery command sent the user straight back into the folder the guard rejects. It now takes the first home outside the Windows tree and prints no path at all rather than one that fails again, which is how install.ps1 already filters its own candidates. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Stop naming USERPROFILE in the branch that just rejected it The no-safe-directory branch printed "cd $env:USERPROFILE", but it is only reached once USERPROFILE, HOME, PUBLIC and TEMP have all been rejected, so on the account that actually lands there (a service or SYSTEM, whose profile is C:\Windows\System32\config\systemprofile) it pointed straight back into the tree the guard refuses. It now says nothing outside the Windows directory was usable and to run the installer from a normal user account, which is also where Studio would install. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Refuse a Studio root that stays inside the Windows tree after relocating Relocating the working directory does not move $StudioHome, which was resolved from USERPROFILE hundreds of lines earlier. On a service or SYSTEM account that is C:\Windows\System32\config\systemprofile\.unsloth\studio, so the venv and every model download still landed in the system tree while the installer reported having escaped it. Before this guard existed that case failed loudly at studio setup, so continuing quietly would be a regression. Rebasing the root instead would orphan the install, since the runtime resolvers recompute it from USERPROFILE, so the installer now stops and says to run from a normal user account. * Put a path boundary on the system-root test and trigger the matrix on the CLI The StudioHome check compared against the bare root, so an absolute UNSLOTH_STUDIO_HOME=C:\WindowsStudio, which is supported, read as a descendant of C:\Windows and aborted the install after the custom root had already been created and validated. The three containment tests are now one Test-UnderSystemRoot helper that matches the root itself or the root plus a separator, so siblings pass. The parity matrix also only triggered on the installer scripts and its test files, so a later edit to the guard in unsloth_cli/__init__.py would have run the Ubuntu backend job alone and merged without the Windows leg that covers it. * Tighten the comments on the new guard Comments, docstrings and whitespace only. The reasons that are not recoverable from the code stay: why the relocation is not restored, why GetFullPath is the wrong resolver for --with-llama-cpp-dir, why StudioHome is refused rather than rebased, why the containment prefix carries a separator, and why the pwsh tests pin HOMEDRIVE/HOMEPATH and expanduser. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> |
||
|
Daniel Han
|
9fe5dd9eb4
|
Fix unsloth run on Windows: the venv entry point is unsloth.exe (#7718)
* Fix `unsloth run` on Windows: the venv entry point is unsloth.exe The re-exec into the studio venv resolved studio_python.parent / "unsloth". Windows installs that console script as unsloth.exe, so is_file() was always false and every Windows run aborted with "Unsloth venv missing 'unsloth' entry point" on an install that was fine. Pick the name per platform, as _studio_shim_path already does. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> |
||
|
Daniel Han
|
f706214383
|
studio update: fail instead of reporting success on a damaged install (#7691)
* studio update: fail instead of reporting success on a damaged install
pip considers a distribution with intact metadata already satisfied, so an
update reinstalls nothing when a package's FILES are damaged. It printed
"Unsloth Studio Installed", exited 0, and Studio then died at boot with
`cannot import name 'Depends' from 'fastapi'`. That is the shape behind the
advice to re-run the full installer, and it is only actionable if the update
says so instead of claiming success.
Reproduced by emptying fastapi/__init__.py in a real install: before this,
update exited 0; now it exits 1, names the file, and points at the installer.
A full install repairs it, which the message says.
A missing-package check cannot catch this, because the damaged module still
imports at top level. Comparing each RECORD entry against the filesystem does,
imports nothing, and takes under a second on an install with torch (44k files
in 1.5s).
Two things the check deliberately does not do, both from real installs:
- it does not flag a file LARGER than recorded, which means two
distributions claim one path (descript-audio-codec ships a top-level
tests/__init__.py) rather than damage
- it does not read Distribution.files, which drops entries whose file is
gone and so can never report a deletion; RECORD is parsed directly
Zero findings on four independent healthy installs, including two updated from
older tags. A CLI living outside the venv it is updating skips the check rather
than describe the wrong tree, and --no-verify turns it off.
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* Cap deleted-file findings and skip the check on system Python
Two review findings on the damaged-install check.
The finding cap was not applied on the deletion branch: the `continue`
after appending "is missing" skipped the `len(found) >= limit` test, so
only truncated files were ever capped. A wiped package takes the deletion
branch, and torch alone has ~11.8k RECORD entries, each of which the
desktop updater streams out as its own IPC event. The existing cap test
used truncated files and so could not see this.
`running_outside_managed_venv` also returned False on a plain system
Python. On Colab there is no Unsloth venv -- studio/setup.sh installs the
backend into the system interpreter on purpose -- so the check ran over
distro dist-packages, whose RECORDs legitimately list files the distro
never installed (PEP 627). Reproduced on Ubuntu system Python, where it
reports an apt-owned `markdown-it-py: ../scripts/markdown-it is missing`
and would have aborted the update. It now returns True when sys.prefix
has no pyvenv.cfg.
Both regression tests fail against the unfixed source.
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* Carry a custom Studio root into the reinstall command
The check's only actionable output pointed at the wrong installation.
A custom root is inferred from sys.prefix when the user runs the shim
directly, but the shim is a bare symlink (install.sh:4367) and
_ensure_studio_env_exported only sets os.environ for this process, so the
shell that runs the printed command sees no UNSLOTH_STUDIO_HOME. Both
installers then fall back to ~/.unsloth/studio and build a fresh install
while the damaged one stays broken.
The assignment goes before `sh` rather than before `curl`, matching the
form install.sh documents at line 14, and the root is quoted so paths
with spaces survive. The default root keeps the plain command.
* Stop pinning one CPython's Distribution.files behaviour
The test asserted that files() drops entries whose file is gone. That
filter is not in every interpreter this project supports (>= 3.9), and on
the ones without it the entry is listed instead, so the assertion fails
there. The unsloth_cli suite happens to run only on 3.12 in CI, so this
was a trap for anyone running it locally on an older interpreter rather
than a live failure.
Asserts the property that actually matters on every version: files()
either drops the entry or lists one whose locate() does not resolve, so
it cannot report the deletion either way, which is why the detector
parses RECORD. Checked the new form is falsifiable, not vacuous.
* Say what to do when the reinstall will not clear the damage
install_python_stack installs the current requirement sets and prunes
nothing, and the installer never recreates the venv, so a package left
over from an older release, or added by hand, survives the reinstall the
message recommends and reports the same damage forever. The only way out
was --no-verify, which the message offered without explaining why the
first suggestion had not worked.
Scoping the scan to Studio's dependency closure was the other option and
is the more dangerous one: the closure would have to be derived from the
requirement files, anything installed outside them would fall out of
scope, and a miss there means real damage passes silently, which is the
exact failure this check exists to catch. Over-blocking with an accurate
message is the safe direction.
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* Quote the interpreter and drop deps in the direct repair command
Two problems with the fallback command added in the previous commit.
sys.executable was printed unquoted, so a custom root containing spaces
split into several shell tokens and the command would not run. Custom
roots with spaces are supported, and the reinstall line right above it
already quotes STUDIO_HOME. Windows gets the call operator, since a
quoted string in command position is otherwise parsed as a string.
It also lacked --no-deps. pip would resolve the damaged package's
dependency graph, and --force-reinstall can then replace pinned runtime
packages, swapping the installed CUDA or ROCm torch build while repairing
an unrelated orphan. install_python_stack.py pairs the two flags in its
own targeted repairs for the same reason.
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* Scan only this interpreter's site-packages, and exempt shared paths both ways
Two review findings on the detector itself.
distributions() searches every sys.path entry, not just sys.prefix, so a
damaged distribution reachable only through an inherited PYTHONPATH
failed every update while sitting outside the installation. Neither
printed repair command can reach it there, so unlike an orphan inside the
venv it was a permanent failure loop with only --no-verify as a way out.
Reproduced, then confirmed fixed. The scan now takes purelib and platlib,
and falls back to the old unrestricted behaviour if those cannot be
resolved, since over-scanning beats scanning nothing.
The collision exemption was also one-directional. Two distributions
claiming one path means whichever copy landed is the one on disk, so its
size says nothing about either RECORD -- but only the larger-than-recorded
case was excluded, and a collision that overwrote with a shorter file was
reported as corruption. Sizes are now compared in a second pass, once
multiply-owned paths are known. A singly owned short file is still
damage, with a test pinning that so the exemption cannot widen.
Real venv with torch: 0 findings, 0.92s.
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* Close four holes in the damage scan
Reproduced all four before fixing; the first is a regression from the
previous commit.
Skipping multiply-owned paths outright also skipped their existence
check. Multiple ownership makes the recorded sizes ambiguous, but it
cannot explain a missing file, so only the size comparison is skipped now
and a deleted shared file is reported once per owner.
Rows with a blank size field were discarded before the file was located.
The field is optional and real wheels leave it blank, so deleting such a
file went unreported. The row is kept with an unknown size and only the
shrinkage comparison is skipped.
A directory standing in for a recorded module passed as healthy, because
an empty directory is commonly 4096 bytes on POSIX and so cleared the
shrinkage test. Entries must now resolve to a regular file. RECORD
directory entries (a trailing slash) are skipped outright.
The printed repair command used a bare package name, and --force-reinstall
reinstalls even when a package is current, so it would have upgraded the
orphan rather than repairing it. --no-deps does not prevent that. It now
shows <package>==<installed version>.
A real venv with torch still reports 0 findings in 0.95s, so the two new
rules add no false positives.
* Describe --verify as the file scan it is
The help said it checks that the backend still imports. It imports
nothing: it compares RECORD entries against the filesystem, so same-size
corruption, a file edited longer, or an intact but incompatible package
all pass. That is a weaker guarantee than the wording promised, and the
wording is what a user decides --no-verify against.
* Keep a no-torch install in no-torch mode when repairing it
install.sh and install.ps1 derive SKIP_TORCH from their own flag or
UNSLOTH_NO_TORCH and then pass that value into setup, and the env tier
beats the recorded mode by design (install_python_stack.py:2257-2276), so
a plain reinstall over a GGUF-only install resolves as UNSLOTH_NO_TORCH
=false and pulls the whole PyTorch stack. Ordinary `studio update` stays
in no-torch mode precisely because it injects no env var. The printed
command now carries UNSLOTH_NO_TORCH, in the form each installer
documents, when the record says so.
Added only on an explicit True: recorded_no_torch() returns None when
nothing recorded the mode, and its contract is that None is not False.
Guessing either way is harmful, so an unknown mode keeps the plain
command.
No root argument, which a first attempt got wrong. The manifest and
marker live in the venv, not the install root, and recorded_no_torch
defaults to Path(sys.prefix); passing STUDIO_HOME looked one directory
too high, read None, and would never have fired. Verified against the
real install: the install-root path returns None, the venv returns the
recorded False. The test now asserts the root so it cannot regress.
---------
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
|
||
|
Daniel Han
|
7869f3f491
|
studio update --local: refuse site-packages as the repo root (#7693)
* studio update --local: refuse site-packages as the repo root
The second consecutive `unsloth studio update --local` failed on Windows:
ERROR: file:///C:/Users/.../unsloth_studio/Lib/site-packages does not appear
to be a Python project: neither 'setup.py' nor 'pyproject.toml' found.
[FAILED] Python dependency installation failed (exit code 1)
STUDIO_LOCAL_REPO was derived as Path(__file__).parents[2], which is the repo
root only while the CLI runs from a checkout. The first update replaces the
editable install with a normal one, so on the second run parents[2] is
site-packages and uv is asked to install site-packages as a project. The
existing comment already noted the path 'may be inside site-packages'; nothing
checked it.
An explicitly set STUDIO_LOCAL_REPO is now honoured, which makes repeat local
updates work, and a root with no pyproject.toml is refused with the two ways
forward instead of uv's error. Found by a Windows update matrix on staging;
the first update passes there and only the second failed.
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* Absolutise an explicit STUDIO_LOCAL_REPO before checking it
A relative override passed the guard and then failed anyway. The check runs
against update()'s cwd, but the value is handed downstream verbatim, and
studio/setup.sh does `cd "$SCRIPT_DIR"` (line 1031) before it calls
install_python_stack (line 1105). So `STUDIO_LOCAL_REPO=.` re-resolved against
studio/, which has neither pyproject.toml nor setup.py, and uv produced exactly
the error this guard exists to replace.
Also strip and expanduser the value: a tilde path is the natural way to name a
checkout and was never expanded, and a whitespace-padded value rejected a valid
one. The sibling reader at studio.py:2744 already strips.
Three tests added; all three fail without the change.
* Run the override checkout's setup script, and print a PowerShell-valid hint
Two review findings on the STUDIO_LOCAL_REPO escape hatch this PR adds.
The override chose which repo gets installed but not which setup script
ran: _run_setup_script resolved setup.sh/setup.ps1 from _PACKAGE_ROOT.
Those scripts build the frontend under their own SCRIPT_DIR, and the
editable install of the override removes the installed tree they built
into. studio/frontend/dist is gitignored, so pointing at a fresh checkout
left Studio with no frontend and `unsloth studio run` exiting 1. The
checkout's own script now takes precedence; with no override the
resolution order is unchanged.
The guard also printed only `STUDIO_LOCAL_REPO=/path/to/unsloth ...`,
which is POSIX prefix-assignment syntax. PowerShell parses that as a
command name, so the sole recovery instruction did not work on the
platform the guard exists for. Windows now gets the $env: form.
All three regression tests fail against the unfixed source.
* Refuse a checkout with no setup script instead of falling back
Falling back to the installed copy's setup script is the behaviour the
override exists to stop: that script builds its own frontend, the
editable install then removes the tree it built into, and the selected
checkout is left without one. A sparse checkout is an unusable local
source, so say so rather than quietly running somebody else's script.
The message names the missing path; the no-override resolution order is
unchanged.
---------
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
|
||
|
Lee Jackson
|
92106ea1fb
|
Fix multiline prompts through Windows npm shims (#7567)
* Fix multiline prompts through Windows npm shims * Fix npm shim review edge cases * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Support all standard Windows npm shims * Resolve Node without PATHEXT script shadowing --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Wasim Yousef Said <wasimysdev@gmail.com> |
||
|
Lee Jackson
|
b5b52cfa48
|
Run OpenClaw in the launched project workspace (#7568)
* Run OpenClaw in the launched project workspace * Fix OpenClaw recipe workspace resolution * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Wasim Yousef Said <wasimysdev@gmail.com> |
||
|
Daniel Han
|
d1127c09af
|
Studio: keep a slow model load alive through a proxy timeout (#7635)
* Studio: keep a slow model load alive through a proxy timeout
Loading a large GGUF behind --secure failed in the browser with "Request
failed" while the server returned 200. Cloudflare quick tunnels drop a
request whose origin has sent no body bytes for ~100s; over one session
12 requests passed that mark and every one of them succeeded server-side:
6 loads (102.6-327.1s), 1 unload (160.9s) and 5 chat completions.
The chat ones stream, so bytes flow from the first token and they survive
(one ran 2254.9s). Load and unload send nothing until they finish, so
only those get killed.
Measured against a real quick tunnel before picking this design:
no body for 150s -> 524
status + headers at t=0, no body -> 524, so headers alone are NOT enough
one byte at t=90s, then silence -> killed ~125s later; the client sees
a 200 with an EMPTY body
one space every 20s -> survives, body intact
So the timer wants body bytes and resets on each one, and the padding has
to be continuous. /load and /unload now hand off to _tunnel_safe_json: a
call finishing inside 15s keeps today's contract exactly, HTTPException
and status code included, and only a slower one commits a 200 and emits a
space every 20s until its payload is ready. Leading whitespace is legal
JSON, so all six loadModel callers parse the body unchanged.
The cost is that a failure discovered after the status is committed can
only travel in-band, as _deferred_error, which parseJsonOrThrow now
raises on. Everything that fails before the 15s mark still gets a real
status code, which covers argument validation, an unknown identifier and
the download-manager and sidecar 409s.
A client disconnect does not cancel the work, so an abandoned load still
leaves the model resident, as it does today.
Also rename the load log's `fit` field. It is use_fit, i.e. whether
--fit on is passed, so `fit: False` meant "fits, GPUs pinned, --fit off"
-- the opposite of how it reads.
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* Tighten the comments added by this PR
Comment, docstring and whitespace only. Keeps every measured number (the ~100s
quick-tunnel idle cutoff, the 100-330s load window, the 160s teardown, the 20s
pad interval, and the probe results including the empty-body outcome at t=90s),
the headers-are-not-enough finding that motivates streaming rather than an early
flush, and the list of failures that still get a real status code.
Also corrects one claim rather than shortening it: the fixture comment said it
preserved the ordering the real values encode, but the defaults are
AFTER=15.0 < EVERY=20.0 while the fixture sets 0.05 > 0.02, so it inverts that
ordering. It now points at the test that does guard the real defaults.
* Keep non-browser load clients blocking and able to see a late failure
Padding a slow /load commits its 200 before the work finishes. That was handled
for the browser and nowhere else, so two classes of client were left broken.
In-process callers got a StreamingResponse whose body nobody drains, so they
resumed while the load was still running. routes/preview.py awaited the route
and then immediately started a preview chat against the previous model, or none.
Everything the route did apart from the padding now lives in load_model_gated,
the route is the only caller of _tunnel_safe_json, and preview awaits the gated
coroutine, so it blocks until the model is really resident and sees the real
exception. Both sidecar checks, the lifecycle gate and the active-generation
callback move across unchanged. /unload needs no equivalent: nothing calls it
in-process, and _unload_model_impl already is the named awaitable, so its
docstring just says so.
Python clients treated any 200 as success, so a late OOM or llama-server startup
failure read as a successful load: unsloth_cli/commands/studio.py returned the
envelope as the load result and start.py recorded it as a success. _inference.py
was worse, closing the response at the headers and generating mid-load.
raise_for_deferred_error and read_json_checking_deferred_error go in
unsloth_cli/_inference.py, the CLI's shared HTTP module, and raise
urllib.error.HTTPError carrying the deferred status and detail. That is the class
all three sites already handle for a plain HTTP failure, and .read() yields the
same {"detail": ...} shape, so their existing except blocks, messages and exit
codes keep working. Keys match the backend and the TypeScript client:
status_code and detail, defaulting to 500.
The fast path is untouched: under the keepalive delay the route still returns the
plain payload and still raises HTTPException with its real status code.
18 tests, each confirmed to fail before the fix. The preview ones are the clearest:
pre-fix the chat starts with zero loads finished.
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* Take the GGUF teardown off the event loop, and reject a truncated padded body
Three review findings, all correct, and the first two meant the padding was dead
code on the two slowest paths it exists for.
LlamaCppBackend.unload_model is synchronous, and two call sites ran it bare on the
event loop: replacing a loaded GGUF with a non-GGUF model, and the already-loaded
manual unload. A 600 GB teardown measures 160s, and while it holds the loop
_tunnel_safe_json's delay timer cannot fire and its keepalive generator cannot
yield, so zero bytes go out and the proxy still kills the request. Both now go
through asyncio.to_thread, matching the in-flight-cancel branch that already did.
An AST test fences both functions against a new bare call, since the fix makes the
call an attribute rather than a call node and would otherwise be easy to undo.
Moving them off-loop lets other handlers run during the teardown. Everything that
could interleave destructively is already excluded: concurrent load and unload by
the process-wide lifecycle gate held across the new await, new inference by the
keep-warm middleware taking the same gate, idle auto-unload likewise, and every
other holder of the backend lock only ever runs inside a thread already. What is
newly observable is GET /status, which is not gated, reporting the model as loaded
for up to the 5s terminate window while the server dies, then flipping. The
reverse is unreachable: _kill_process clears _process before the identifier, so
is_loaded can never be true with a cleared identifier.
A truncated padded response was read as success. On the web side .json() throws,
the catch makes the body null, the 200 is already committed and no deferred error
is present, so it returned null as a completed load or unload. This PR's own
tunnel probe documents exactly that case. On the Python side _http_json's
`or "{}"` turned a blank body into {}, which read as a finished load. Both clients
now require a non-empty object from those two routes and say the operation did not
report completion otherwise.
That strictness is scoped to the two padded routes rather than the shared JSON
helper. They are the only routes that commit a 200 before the work is done, so
they are the only ones where an empty body means "may or may not have happened"
instead of "no payload by design"; the codebase has six 204 routes and five
callers that discard the value, which a blanket null error would break.
17 tests, each confirmed to fail first. The teardown ones use a synchronous fake
that blocks until it has seen pad bytes, so pre-fix the log shows the pad message
only after the teardown returns, and the unload path never becomes a stream at all.
* Tighten the comments the review rounds added
Comment, docstring and whitespace only: 46 comment lines out, 46 file lines out.
The first pass ran when this was only the tunnel-safe change, so the prose added
by the two rounds of review fixes had never been through one. The tunnel test
file carries most of the reduction at 18 lines.
Every non-derivable fact survives, grep-checked: the ~100s idle cutoff, the
100-330s load window, the 160s 600 GB teardown, the 20s pad interval, the probe
result that a byte at t=90s then silence yields a 200 with an empty body,
headers-alone still 524ing, leading whitespace being legal JSON, the failures
that keep a real status code, the client-disconnect guarantee, why in-process
callers await load_model_gated, why the deferred error is an HTTPError
specifically, why the teardowns must be off-loop, and why the truncated-body
strictness is scoped to the two padded routes.
One wording fix rather than a shortening: an empty dict was described as
"counts", which was ambiguous about which way. The check is
`isinstance(body, dict) and body`, so {} is rejected, and the comment now says so.
Left alone: everything from the first pass, and test_preview_routes.py, whose
diff is pure load_model -> load_model_gated renames with no new prose.
---------
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
|
||
|
Nilay
|
a31a8ff352
|
Don't disarm Ctrl+C in agents launched by unsloth start (#7630) | ||
|
Daniel Han
|
1564ce4585
|
Fix two pre-existing Backend CI failures blocking every open PR (#7639)
* Read the backend source as utf-8 in the model-picker contract test _read_backend landed in #7385 without an encoding, so it falls back to the platform default and breaks on Windows the moment a backend file it reads gains a non-ASCII byte. tests/test_source_read_encoding.py guards exactly this and is currently failing on main, which turns Repo tests (CPU) red on every open PR whose CI runs the merge commit. The sibling _read helper directly above already passes encoding = "utf-8". * Strip colour from the unsloth_cli CLI test output Three test_start.py cases assert on plain substrings of result.output, e.g. "Invalid value for '--gpu-memory-mode'". Typer renders parameter errors through Rich, which emits ANSI escapes as soon as FORCE_COLOR is set, so the substring is split across escape sequences and the assertion fails even though the message is present. They pass locally and fail in CI purely because the runner exports FORCE_COLOR; click, typer and rich are the same versions in both. Setting NO_COLOR alone does not help, FORCE_COLOR still wins, so the autouse fixture removes FORCE_COLOR and CLICOLOR_FORCE as well. #7598 only recently started running unsloth_cli/tests in Backend CI, which is why this surfaced now. Verified: the whole suite is 748 passed with and without FORCE_COLOR=1. |
||
|
oobabooga
|
cf96288820
|
unsloth start: harden coding-agent installation (#7523)
* unsloth start: harden coding-agent installation * Never let the managed-Node probe break an agent launch * unsloth start: narrow managed Node probe fallback * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Keep npm lookup compatible with single-arg which stubs and skip POSIX-only tests on Windows Cross-platform runs surfaced two problems in the npm work. `_npm_executable` called `shutil.which("npm", path = ...)` on the common path, which broke callers and tests that stub `shutil.which` with a single positional argument. It now keeps the plain `shutil.which("npm")` lookup and only walks PATH entry by entry after that first hit is rejected as a WSL Windows shim, which is the only case that needs the wider search. Seven tests added in this branch assume POSIX: the managed node layout uses a bare `npm` rather than `npm.cmd`, WSL shim rejection is a no-op when os.name is "nt", and the npm hint is PowerShell quoted there. Marked them skipif os.name == "nt", matching the convention already used in this file. unsloth_cli/tests/test_start.py: 381 passed. * Resolve the npm directory with dirname so a patched os.name cannot pick the wrong Path flavour _install_command used Path(npm).parent. pathlib chooses PosixPath or WindowsPath from os.name at call time, so a caller that overrides os.name (the install tests set it to posix) builds the flavour the host cannot instantiate, and the call raised NotImplementedError on Windows. os.path.dirname gives the same answer without consulting os.name. An empty result now leaves PATH alone rather than prepending the current directory. * Reclaim ephemeral agent homes left behind by abnormal exits Moving ephemeral homes out of the system temp directory and under Studio's auth tree removed the only thing that ever cleaned them up: the OS. When the wrapper is killed by SIGKILL, the console closes or the machine crashes, the context manager's finally never runs, and nothing prunes <STUDIO_HOME>/auth/agents/.tmp, so interrupted sessions accumulate there indefinitely. Only the Windows codex path had reclamation. `_temporary_agent_config` now goes through the same locked session helper that path already used, so every agent gets the scavenge on launch, the advisory lock that keeps a live session from being swept, and the heartbeat that anchors the stale window to wrapper death rather than session start. `_reclaim_stale_ephemeral_sessions` and `_short_ephemeral_session` take the prefix to glob and create, which is the only part that was codex specific. An age-only sweep was not enough on its own: without the live marker a session still running after the stale window would be deleted underneath itself. unsloth_cli/tests/test_start.py: 382 passed. * Route Codex subagent homes through the short Windows parent `_ephemeral_session_parent` matched the agent name exactly, so only `codex` reached the short `~/.unsloth/.tmp/u-codex-*` root. The subagent path is created as `codex-subagent`, and it also nests CODEX_HOME one level deeper under `<home>/parent`, so it needed the short root more than a plain launch, not less. On Windows with an 8 character mkdtemp suffix the resulting CODEX_HOME was 86 characters against 248 for the git limit, where current main was 69 and a plain codex launch is 45. Codex checks out its curated plugins below CODEX_HOME, which is what exceeded the limit in the first place. Both names now take the short root and the same `u-codex-` prefix, so one scavenger pass covers both, and the subagent home lands at 52. `_session_config` now derives its prefix from `_ephemeral_session_prefix` for both branches rather than rebuilding it inline. unsloth_cli/tests/test_start.py: 385 passed. * Fall back to the system temp dir when the Studio auth tree is unwritable Routing every ephemeral home under <STUDIO_HOME>/auth/agents/.tmp made a writable local auth tree a hard requirement for any non-persistent launch. Attaching to a remote or already-running Studio with an explicit key does not need one, and the key cache already tolerates that: _remember_key wraps its write in except OSError. Such launches used tempfile.mkdtemp() before this branch and now died with PermissionError before the agent started. _temporary_agent_config now falls back to the system temp dir when the root cannot be created, which is where these homes lived previously. Reclamation is lost on that path, but the OS prunes it, so nothing accumulates. unsloth_cli/tests/test_start.py: 386 passed. * Give agent probes the augmented PATH and cover an existing unwritable temp root Two gaps in the previous two commits. _which_with_install_dirs restores PATH before returning, so a shim it resolved through Studio's managed Node could not find that node when the probe actually ran it. That broke `opencode debug config` on the --as-subagent path and made the claude and codex version probes report an unsupported build, which changes the flags the agent is launched with. Probes now build their environment with _probe_env, which augments PATH for the child without leaving it set in this process. The unwritable-root fallback only covered mkdir. When the temp root already exists but cannot be written, mkdir(exist_ok = True) succeeds and the failure moves to .cleanup.lock inside _short_ephemeral_session. The whole setup is now inside the guard, so either failure falls back to the system temp dir. unsloth_cli/tests/test_start.py: 388 passed. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Keep the managed Node on PATH when the home directory is unavailable _augment_path_with_install_dirs returned as soon as Path.home() raised, and the managed Node lookup sits after that return, so a container running under a bare UID never got the managed Node on PATH. An npm agent shim installed there still resolved, then failed because `env node` could not find a node. The same environment is the one the install hint was already taught to handle. Only the user install dirs need a home, so a missing one now leaves them out rather than skipping the rest. When there is nothing to add, PATH is still left untouched, as before. unsloth_cli/tests/test_start.py: 390 passed. * Tighten the comments added on this branch Compress the explanations to the fewest lines that still carry the reason: which failure each guard is for, and which environment reproduces it. No code changes. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Lee Jackson <130007945+Imagineer99@users.noreply.github.com> Co-authored-by: danielhanchen <unslothai@gmail.com> |
||
|
Nilay
|
ceef4123e6
|
Studio: Stop every running Unsloth server, not just the last one recorded (#7577)
* Stop every running Unsloth server, and refuse to start a second on a taken port * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Check the fallback range, guard PID reuse, and keep writing studio.pid * Signal each server once when its PID is recorded in more than one file * Confirm a recorded PID is a Studio server before signalling it * Pin PID records to process start time and check every listener on a port * Keep every recorded start time per PID and accept in-process Studio servers * Match the blocking listener address and stop trusting unverifiable PID records * Never delete a PID record that cannot be verified * Detect our own server from our own records instead of a psutil listener scan * Match a pre-upgrade studio.pid to the blocked port before falling back * Never signal PID 0 or 1, and verify a per-port record before trusting it * Stop unverifiable records instead of skipping them, and record every bind address * Drop the command-line guess, fix Windows liveness, and free the PID record last * Studio: harden the per-port PID records against the cases that lose a server Follow-up on the per-port PID files. Each item below is a case where the new code either lost a server the old code could still stop, or stopped something that was not ours. All were reproduced against real Studio servers. studio/backend/run.py - Write the per-port record and the legacy studio.pid independently. They shared one try, so a studio root that could not take a new directory entry left the server recorded nowhere at all and unstoppable from the CLI; the old code still recorded it in studio.pid, which is an overwrite of an existing path and can still succeed. _remove_pid_file now also checks studio.pid when the per-port write failed. - Write the record through a temp file and os.replace. `stop` reads these concurrently and treats a truncated read as a corrupt record. - A failed Windows tasklist probe now means "alive", matching the CLI. Treating it as dead pruned a live server's record and let the next launch fall back past it, which is the orphan this work exists to fix. - Guard the unlink in _own_studio_on_port. Pruning is a courtesy and must not abort startup. - Extract _resolve_port so the requested-port abort is reachable from a test. Deleting that abort previously left the whole suite green. - Keep the plain fallback for api-only callers. The desktop app hardcodes 8888 and documents its reliance on the 8888-8908 range, and it reports a non-zero backend exit to the user as "Server stopped unexpectedly". It reads the bound port back from TAURI_PORT, as `studio run` does from app.state.server_port, so a fallback there is harmless and both servers are still recorded and stoppable. The interactive path prints the requested port, so it still aborts. - isdigit() is not enough to gate int(): a superscript two passes it and the ValueError escaped into every caller of _read_pid_record. unsloth_cli/commands/studio.py - An untimed record no longer cancels a timed one for the same PID. Every current server writes both a timed per-port record and an untimed studio.pid, so the start-time check was inert exactly where it mattered, and after a crash plus a PID reuse `stop` sent SIGTERM to whatever unrelated process had inherited the PID. - Distinguish an unreadable record from an invalid one. A root-owned record, or one caught mid-write, still belongs to a live server, and deleting it stranded that server. - Route every PID-file removal through _unlink_quietly. One undeletable record raised PermissionError and left the remaining live servers running. - Same isdigit()/int() guard as the backend. Tests - The requested-port abort, the recorded bind address, and the api-only fallback are now covered; all three previously survived deletion. - tests/studio/test_studio_pid_file_contract.py pins run.py's filename scheme to the CLI's glob and keeps studio.pid parseable by an older CLI. It lives under tests/studio because unsloth_cli/tests is not run by any workflow. - test_cli_studio_stop_windows.py now checks _signal_stop as well as stop. The kill moved into _signal_stop, so the os.kill(pid, 0) guard passed vacuously. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Studio: let a caller that follows the port keep the fallback, and never take studio.pid from a live server Two problems with keying the own-server abort on api_only. `unsloth studio run` is not the bare-banner path: it stores `app = run_server(...)` and reads `app.state.server_port` back, then uses it for the health wait, the model load and the printed base URL. Gating on api_only aborted it, so starting a second model while the first was up stopped working, where before it landed on the next port and printed the right URL. Replace the proxy with an explicit abort_if_own_studio, defaulting to the old api_only behaviour so the exec'd `run.py` path is unchanged, and have `studio run` opt out. The api_only exemption also reopened the orphan from the other side. _write_pid_file overwrote studio.pid unconditionally, and a pre-upgrade server is recorded there and nowhere else, so an exempt launch falling back past one erased its only record. Take the file over only when it is free, already ours, or held by a dead PID. Also resync _pid_is_studio_backend with the CLI copy: an untimed record next to a timed one carried no information but cancelled the start-time check, which is what let a reused PID be treated as ours. Tests: 51 backend, 26 CLI, 9 under tests/studio. Real Studio servers still abort the bare same-port relaunch, still fall back past a foreign listener, and one `unsloth studio stop` still stops every server in all five scenarios. * Studio: hand over the legacy PID pointer, and fail stop on unreadable records Two follow-ups from review of the previous commit. Only one backend owns studio.pid at a time. When that server exited it deleted the file, so an older CLI, which reads nothing else, could no longer stop a sibling that was still serving. _remove_pid_file now hands the pointer to a live sibling instead of dropping it. _pid_file_entries skipped records it could not read, for instance one written by a server started under sudo. When that was the only record, stop printed "No running Unsloth server found" and exited 0 while the server kept serving. Unreadable records are now reported and make stop exit 1, so a partial stop is never mistaken for a complete one. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Daniel Han <unslothai@gmail.com> |
||
|
Nilay
|
52609fb890
|
Studio: reset-password rotates the credential in place instead of deleting auth.db (#7573)
* reset-password: rotate the admin credential in place instead of deleting auth.db * reset-password: fix the CI callers and error handling for the in-place rotation * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * reset-password: narrow the CI change to the jobs that read .bootstrap_password * reset-password: stop over-claiming what the reset revokes and when it takes effect * auth: bind token issuance to the credential version that was verified * auth: bind credential-creating writes to the version the request authenticated with * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * auth: bind the change-password and workflow-key writes to their own credential version * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * auth: read the credential version inside the transaction that validated it * data-recipe: answer 401 when a reset revokes the credential mid job start * Fix lint blocker and Windows path assertion for PR #7573 Drop the now-unused validate_api_key import from studio/backend/auth/authentication.py. Every call site moved to validate_api_key_with_credential, so the Source lint job's import-hoist gate flagged it as a blocker. The wrapper itself stays in storage.py; test_api_key_expiry.py still exercises it. Make test_run_reexec_forwards_resolved_frontend_on_public_launch compare against str(Path(...)) instead of a POSIX literal. _find_frontend_dist returns a Path, so on Windows the forwarded value is \fake\studio\frontend\dist and the assertion could never pass there. Pre-existing, surfaced by running unsloth_cli/tests on Windows. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Daniel Han <danielhanchen@gmail.com> |
||
|
Daniel Han
|
5cebc46124
|
Make the unsloth_cli studio tests pass in isolation (#7599)
* Make the unsloth_cli studio tests pass in isolation Six tests in test_studio_run_parallel_flag.py and one in test_studio_secure_flag.py only passed in a full-directory run. All of them reach the in-venv branch of run(), which does `from state.tool_policy import set_tool_policy`. That module lives under studio/backend, so it only imports once something has put that directory on sys.path, and nothing in either file does. They were relying on test_start.py, which calls ensure_studio_backend_path() and leaks the sys.path entry, or on test_studio_cloudflare_flag.py, which stubs the module. Add a stub_tool_policy_state fixture in a new conftest and use it in the seven, so the state comes from the test rather than from whatever ran first. Every file in unsloth_cli/tests now passes on its own, and the suite is stable across four pytest-randomly seeds. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> |
||
|
Daniel Han
|
0ed26297ed
|
Run unsloth_cli/tests in Backend CI (#7598)
unsloth_cli/tests had no CI at all. unsloth_cli/** was a paths trigger and a ruff target, so the Backend CI job already fired on CLI changes but never ran these 673 tests, which cover the studio launcher, the pre-exposure gate and the auth secret writers. Four had been failing on main unnoticed. Two were stale rather than broken code: - test_studio_default_exposes_parallel_option pinned the plain --parallel default to 1, but #7455 deliberately moved _PARALLEL_DEFAULT_PLAIN to 4 so a new chat does not queue behind the previous one. Assert against the constant so the two cannot drift again. - test_reexec_forwards_api_only expected --secure --api-only to re-exec. The pre-exposure gate now refuses that combination, because api-only serves no login page and the bootstrap deadline does not apply, so a seeded password could never be changed. Drop the case and assert the refusal instead. Two only passed when a built frontend dist happened to be present, which it is not in a fresh clone or on a runner. Both reach a public-launch path where the missing-dist gate exits first, so they never got to the backend check and the run_server call they are about. Stub _find_frontend_dist the way their siblings already do. Own step rather than folding into the tests/ discovery: pyproject's testpaths is tests/, and this suite needs no PYTHONPATH or CUDA spoof, importing neither unsloth nor torch. Its deps are already installed by the job (pydantic and uvicorn, which brings click, via studio.txt; pyyaml explicitly). |
||
|
Nilay
|
7348a20497
|
Studio: Write auth secret files with a trailing newline (#7576)
* Write auth secret files with a trailing newline * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Pin LF in the auth secret writers and migrate legacy files Both writers used text mode, so on Windows the trailing newline became CRLF. The Windows Studio smoke jobs run under bash and read the file with OLD=$(cat ...), which strips the LF but leaves the CR attached, so the credential goes into the login body as "<secret>\r" and the request fails. Write bytes in the backend and pin newline in the CLI so the file is "<secret>\n" on every platform. generate_bootstrap_password() also returned early on an existing file, so upgraded installs kept the original problem; it now rewrites anything that isn't already exactly "<secret>\n", best-effort so a read-only auth dir cannot fail startup. The raw test assertions used read_text(), which decodes CRLF back to "\n" and would have stayed green on Windows. They read bytes now. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Run the newline migration on the path upgrades actually take ensure_default_admin() short-circuits to _load_bootstrap_password() once the admin row exists, so the normalisation added in the previous commit sat on generate_bootstrap_password(), which only fresh installs reach. An upgraded install kept its newline-less file. Both readers now share _read_persisted_bootstrap_password(). Make the write atomic while it is here: it can now rewrite a live file, and a partial write would destroy the only plaintext copy of the recovery credential. Same mkstemp plus os.replace shape the CLI writer already uses. Tests cover the upgrade path through ensure_default_admin(), a well-formed file not being rewritten on every start, a failing migration not blocking startup, and the atomic replace. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Normalise the bootstrap file in place so a cleared credential stays cleared The rename-based rewrite could recreate the file: if a password change ran clear_bootstrap_password(), or the CLI cleanup deleted it, between the read and the write, os.replace put the revoked plaintext back on disk, where a later auth.db reset would re-seed it. Open the existing file without O_CREAT instead, so a deleted file cannot be resurrected, and re-check the contents through that descriptor so an in-place truncation or a rotated credential is not overwritten either. That gives up the atomic rename, so the in-place path is restricted to trailing-whitespace fixes. Every partial state is then the secret plus leftover whitespace, which still strips to the same credential. Files with leading whitespace are left alone; every reader strips, so they keep working. Creation still goes through the atomic writer. * Open the bootstrap file in binary mode and finish the write Three defects in the in-place normalisation, all on the Windows upgrade path. os.open does not add O_BINARY on Windows and CPython never changes the CRT default of _O_TEXT, so the descriptor was in text mode: os.write turned the LF straight back into CRLF and ftruncate then cut the LF off, leaving "<secret>\r". That is the bug this PR exists to fix, reintroduced by the migration itself, and it is a fixed point that never converges. os.read translates in reverse too, so a genuinely CRLF file failed verification and was silently skipped. os.write may return having written fewer bytes than asked; ftruncate would then NUL-extend the credential so it no longer matched the hash in auth.db. os.fchmod only reached Windows in 3.13 and AttributeError is not OSError, so on 3.9 to 3.12 it escaped both handlers and aborted the first start after upgrade. * Make the bootstrap normalisation append-only clear_bootstrap_password() falls back to truncating the file through its own descriptor when the unlink fails, which is what happens on Windows while this one is open. That truncation could land after the equality check and before the write, so the rewrite put the revoked plaintext back. Append a single LF instead, and only to a file that is exactly the credential. An append cannot restore a revoked secret: over a cleared file the result is a lone newline, which strips to empty and reads back as no bootstrap password. Releases before the newline wrote the password with no terminator at all, so that is the only shape in the wild; anything else is left alone and keeps working because every reader strips. Never truncating also removes the short-write NUL-fill hazard entirely, so the write loop is gone. O_BINARY stays: without it Windows would turn the appended LF into CRLF. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fix a typo in a bootstrap normalisation test name * Tighten the bootstrap newline comments --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Daniel Han <danielhanchen@gmail.com> Co-authored-by: danielhanchen <unslothai@gmail.com> |
||
Kirelos Namroud
|
150b5ba25a
|
feat(studio): adjustable llama-server parallel slots from the web UI (#7447)
* feat(studio): share the llama-server --parallel bounds as PARALLEL_MIN/MAX The per-load parallel-slots field needs the same 1..64 range the CLI flag validates, but models/inference.py cannot import run.py (run.py builds the app that imports routes that import models). Promote the bounds into this dependency-free module, which already owns the -np/--parallel semantics, and record the deliberate mirrors that cannot import it (run.py, the unsloth CLI, the web UI). The denylist entry stays: the first-class field is now the single write path for the slot count, so a pass-through would still desync the committed bookkeeping from llama-server. * feat(studio): note the per-load override in the --parallel help text --parallel is now the server-wide default that a per-load n_parallel (the Studio Parallel Slots run setting) can override, not the definitive slot count. Point at the new control so a user does not conclude a restart is the only way to change slots, and record the shared PARALLEL_MIN/MAX mirror alongside the existing CLI one. * feat(studio): add n_parallel to LoadRequest and echo the slot counts LoadRequest.n_parallel (optional, PARALLEL_MIN..PARALLEL_MAX) lets a load pick its own llama-server --parallel count; omitted, the server-wide launch default applies. ValidateModelRequest carries it too so the training-coexistence estimate sizes the KV cache like the follow-up load rather than passing on a smaller footprint. LoadResponse and InferenceStatusResponse gain both requested_parallel_slots (what the load was invoked with) and parallel_slots (what llama-server actually runs after the fitter's slot reduction), so a client can tell an honored request from a reduced one. Both are None where --parallel has no meaning: non-GGUF loads and the diffusion runner. * feat(studio): record the requested parallel-slot count on the backend The auto GPU-memory fit may launch fewer slots than requested to keep the model fully on GPU, so the committed effective count cannot answer "is the live server what this request asked for?". Store the invoked count separately (mirroring the _requested_n_ctx pattern) from the pre-reduction pending kwargs, expose it as requested_parallel_slots, and have _already_in_target_state compare requested-vs-requested: comparing against the effective count would reload -- and re-reduce -- forever on an identical Apply. The comparison sits in the non-diffusion branch, since the diffusion runner ignores --parallel entirely. The requested value shares the effective count's lifecycle, so every unload/kill path clears it and a stale count cannot poison the next load's dedupe. * feat(studio): honor a per-load parallel-slot count in /load and /validate Resolve the slot count once per load -- the request field if set, else the server-wide launch default -- and feed it to every consumer that must agree: the training-coexistence guard, the llama-server load kwargs, and the reload dedupe. Without the dedupe comparison a changed slot count would be swallowed as already_loaded; it compares requested-vs-requested and skips the diffusion runner, which ignores --parallel. app.state.llama_parallel_slots is deliberately never written: it stays the launch intent and the admission-queue fallback, so one load's override cannot leak into later loads. /validate resolves the same way so its estimate cannot undercount what the load then allocates. Both /load returns and /status echo the counts through one helper, which reports None for diffusion -- its load never commits a count, so echoing the reset placeholder would fabricate an "invoked with 1 slot". * feat(studio): accept nParallel in the chat-preset load config ChatPresetLoadConfig is extra="forbid", so a preset carrying the new parallel slots knob would 422 the whole settings sync without this field. Bounds come from the shared PARALLEL_MIN/MAX rather than literals, so a future range change cannot start rejecting presets the UI still allows. * test(studio): cover the per-load parallel-slots knob Pins the behaviors a regression would silently break: the requested-vs-effective dedupe (comparing against the reduced count would reload forever), the diffusion skip and its None echo, the requested count's reset lifecycle, and its commit from the pre-reduction pending kwargs. Also pins the three bounds mirrors that cannot import PARALLEL_MIN/MAX (run.py, the unsloth CLI, the web UI) plus the preset model that can, so a range change cannot leave one of them clamping or rejecting at the old limit. * test(studio): refresh the --parallel denylist comments for the UI knob The pinned rationale said the typer flag owns the slot count and pointed users at a Studio restart. Parallel Slots / LoadRequest.n_parallel is now the other managed writer, and the 1..64 guard is the shared PARALLEL_MIN/MAX -- a reader following the old comments would conclude the UI control does not exist. * feat(studio): note the per-load override in the CLI --parallel help Both the plain-serve and `unsloth studio run` flags now describe a server-wide default the Studio Parallel Slots run setting can override per load, matching the backend help text. * feat(studio): remember a per-model Parallel Slots override nParallel joins the per-model config with the same null-means-follow-the-default convention as the other knobs: null keeps the server-wide --parallel count, so a blank control never pins a number and isDefaultConfig still deletes an otherwise-untouched config instead of storing it. The value is re-clamped to N_PARALLEL_MIN/MAX on every localStorage read and write (the store is user-editable), and listing it in STORED_CONFIG_FIELDS keeps it from being dropped as an unknown key. Legacy blobs predate the knob, so their migration carries null. No schema-version bump: an additive optional field, like the GPU fields before it. * feat(studio): bridge nParallel between the per-model config and the store The config->store, store->config and equality helpers all need the new field: without the equality arm a slots-only edit reads as unchanged, so Apply is dropped and the dirty state never lights up. * feat(studio): track the parallel-slot override in the chat runtime store nParallel holds the editable override and loadedNParallel the value the last successful load sent, which the failed-switch rollback re-sends. Both are per-model: they clear on unload and on a model switch, unlike the standing preferences (GPU memory mode, speculative type) that survive one. There is deliberately no backend-echo field for the control: the echo is the resolved count, so adopting it would pin a blank "follow the server default" input to an explicit number. * feat(studio): type n_parallel and the slot-count echoes The load request gains the optional per-load slot count, and both the load response and the status payload gain requested_parallel_slots (invoked) and parallel_slots (actually running after the fitter's reduction). Keys stay snake_case: the payload is serialized as-is, with no case conversion. * feat(studio): forward n_parallel to the validate preflight validateModel builds its own body rather than forwarding the load payload, so the slot count has to be listed explicitly. Slots scale the KV estimate, and the preflight exists to refuse a load the training guard would then 409 -- an unforwarded count would validate a smaller footprint than the load allocates. * feat(studio): include nParallel in the active model's config The sidebar assembles the active model's config from individually subscribed store fields; an unsubscribed field would leave the form showing a stale value after any external change. * feat(studio): add the Parallel Slots control to the run settings A numeric input in the GGUF advanced section, blank meaning "follow the server default". It clamps on change like the Draft Tokens field rather than using NumericValueInput, so there is no blur-draft to lose when the user types a value and immediately clicks Load. hasNonDefaultAdvanced counts it too, so a remembered override reopens the advanced section instead of hiding the setting that is actually in effect. * feat(studio): key the sidebar config form on nParallel too The signature drives the remount that re-seeds the form; without the new field an externally changed slot count would leave the sidebar showing the old one. * feat(studio): send the Parallel Slots override on load performLoad snapshots the slot count at click time (staged run-settings config first, else the store) and sends it on both the validate preflight and the load, so the two size the same footprint. A cross-model switch re-baselines it like the other per-model knobs -- the previous model's count must not follow onto the next one -- and the failed-switch rollback re-sends the previous model's value so a rescue reload cannot silently drop to the server default. The success path keeps the click-time value rather than the response echo: the echo is the count the fitter resolved, so adopting it would turn a blank "follow the server default" control into an explicit pin. Slots are GGUF-only, so a transformers load sends and records null instead of a phantom override. * feat(studio): carry the slot override through the compare-pane load The compare pane builds its own load request, so it needs the field explicitly or a pane with a remembered override would load at the server default. Its validate preflight sends the same count, matching the comment above it that promises validation is sized exactly as the load below. GGUF-gated on both calls, and the store adopts the pane's own click-time value rather than the resolved echo, mirroring the single-model path. * feat(studio): honor the remembered slot override on startup auto-load The auto-load path reads the per-model config and forwards every other remembered knob, so a remembered Parallel Slots value was the one setting lost on the "load last used model" path: llama-server came back at the server-wide default with the control showing blank, and the first manual Apply afterwards then forced a needless reload because the counts disagreed. * feat(studio): seed the slot baseline from the status echo Only the rollback baseline is seeded, never the editable control: the echo is the resolved count, so adopting it would pin a blank "follow the server default" input to a number. Without the seed, loadedNParallel stayed null after a tab reload or a second tab adopting the running model, and a failed switch then rolled the previous model back at the server default while every other knob was restored. * feat(studio): capture Parallel Slots in chat presets The knob joins the preset load config end to end: captured from the store, re-clamped when read back (persisted presets are untrusted input), applied on switch, and summarized in the preset chip. Its default is null, so coalesceDefaultLoadKnobs keeps a default-only preset empty rather than persisting a no-op override. * feat(studio): re-derive the preset state when Parallel Slots changes Both preset memos snapshot the store through capturePresetLoadConfig, so without the new dependency a slots-only edit left the unsaved-changes flag and the load summary showing the previous value. * test(studio): pin the Parallel Slots wiring end to end Source-contract coverage for the hops a refactor can silently drop: the three /load builders (interactive, compare pane, startup auto-load) and their validate preflights, per-model persistence and clamping, the UI row, and the status seed -- including the negative assertion that hydration seeds only the rollback baseline, never the control, so the resolved echo cannot pin a blank "server default" input. * test(studio): pin nParallel in the preset load config Covers capture, clamped read-back and apply on the frontend, plus the backend field itself: ChatPresetLoadConfig is extra="forbid", so a missing or drifted field 422s every settings sync that carries a preset. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fall back to one slot when llama-server lacks --kv-unified for PR #7447 Without --kv-unified an explicit --parallel N makes llama-server give each slot -c/N, so on a build without the flag choosing N slots silently shrinks every context window for a feature that build cannot serve. Clamp to one slot and log why, placed after the requested count is captured so the echo still reports it and before the KV estimates so the fit matches what actually launches. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Clear the slot control on load paths that never send it, and size the training guard for diffusion Four review findings on the per-load Parallel Slots knob. The editable nParallel control means "follow the server default" when null, so any success path that does not send a slot count has to clear it. Three paths kept a value staged for a different model: - chat-adapter.ts, cached non-GGUF auto-load: the interactive and compare builders already clear both fields for a non-GGUF response, this third one did not. The field never renders for a non-GGUF target, so the stale count was invisible and unclearable from the UI yet still persisted, and it flips isDefaultConfig so a user with no overrides silently gets a stored entry. - chat-adapter.ts, fresh-model fallback: its request omits n_parallel but its success state resynced every other knob and left the slots alone, so a staged edit survived against a server running the default and the next Apply reloaded at a count that load never sent. - apply-inference-status-to-store.ts: on a model change underneath the tab every sibling knob adopts the new model's status, but nParallel updated only its baseline, so the previous model's explicit count followed onto the new model and saving or reloading there pinned it. Clear the control and keep seeding the baseline for the rollback. The training-coexistence guard sized a diffusion GGUF with the requested slot count. _estimate_kv_cache_bytes scales the SWA cache with slots (swa_limit = swa * slots + ubatch), but load_model hands a diffusion target to _start_diffusion_server before the slot plumbing, so that runner is always single-slot. At the new default of 4 this inflated the estimate and could 409 a load that fits. An unclassified GGUF keeps the requested count. Backend base KV depends on -c alone, not on --parallel, which is why only the SWA term is affected: llama.cpp PR 14363 and discussion 4130. Tests: three training-guard cases in test_parallel_slots_per_load.py and one source contract in test_model_picker_contracts.py, each mutation-checked. 174 passed across the backend slot/admission/training suites, 56 across the frontend contract suites. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Keep the slot control when re-adopting the running model, and never record slots for a diffusion load Two follow-ups from the latest review round. The first is a regression from c796393. That commit cleared the slot control whenever hydratingExistingModel was set, to stop model A's count following onto model B. But that flag is also set on the resident-model adopt path: when the store checkpoint is an external provider id and the user re-picks the still loaded local model, applyActiveModelStatusToStore is called with the external id as previousCheckpoint, so the flag is unconditionally true. The clear then wiped the config applyPerModelConfigToRuntime had restored two lines earlier, and it was the only knob that did, because the siblings re-adopt the status echo while this one cleared. Gate the clear on the tab's own baseline no longer matching the running count: a genuine A to B swap still clears, re-adopting the same model keeps its value. The second revises an earlier call of mine. I rejected the diffusion phantom as cosmetic because the backend ignores the value on every send. The sharpened report is right and my rejection was wrong: capturePresetLoadConfig records nParallel with no model gate, a Preset carries no model id, and applying one writes nParallel for whatever model is current. So a count recorded against a diffusion model, which the backend never applied, rides a saved preset onto a text GGUF and becomes a real override the user never chose. Record slots only when the load actually committed them, on all three load builders. Tests: two source contracts in test_model_picker_contracts.py, both mutation checked. Frontend typecheck clean, 58 passed across the contract and preset suites. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Clear the slot baseline when status reports a model without slots Hydrating from a GGUF to a slotless model left loadedNParallel at the previous model's count: the seed only runs when the echo is non-null, and the control clear added earlier touches nParallel alone. The stale baseline is what a failed-switch rollback re-sends, and preset capture reads it, so it could claim slots for a model that never used them. Clear it when status describes a model that cannot have slots. /status omits the echo entirely for non-GGUF and sends an explicit null for the diffusion runner, so keying on is_gguf === false or an explicit null covers both while an absent field on a GGUF, which is how an older backend reports one, still leaves the baseline alone. Test mutation checked; frontend typecheck clean against a fresh npm ci. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Distinguish a same-model re-adopt from a model swap, and size the training guard at the slots that launch for PR #7447 * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Keep the blank slot control across a failed-switch rollback for PR #7447 * Restore a remembered slot override when hydrating a fresh store for PR #7447 * Tighten comments for PR #7447 * Restore a remembered slot override on a model switch too for PR #7447 * Tighten comments and docstrings for PR #7447 * Take the rollback slot intent from the picker's pre-switch snapshot for PR #7447 * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: danielhanchen <danielhanchen@gmail.com> Co-authored-by: danielhanchen <unslothai@gmail.com> |
||
|
Daniel Han
|
c608649552
|
feat(studio): run chats in parallel in the Chat tab (#7455)
* feat(studio): run chats in parallel in the Chat tab New Chat used to cancel whatever the current conversation was generating. It now leaves it running, like switching to the Train or Export tab: the sidebar shows which chats are still going, and Stop is per conversation. Plain `unsloth studio` launched llama-server with one decode slot, so the admission queue serialised every chat regardless of what the UI did. Both entry points now default to the same slot count as `unsloth studio run`. A model swap still ends every running chat, since they all decode on one llama-server. /load and /unload now refuse with 409 and name those chats unless the caller passes force_cancel_active, and the UI asks first. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix(studio): scope the composer tool badge to its own conversation The green "Running Python: ..." badge above the composer read a single global store value, so one chat's tool call showed above every other chat's composer, including a brand-new empty one. Its elapsed counter also restarted at 0 on every thread switch, and a run ending anywhere cleared the badge everywhere. Key the status by thread and store the moment it started, so each conversation shows only its own tool call and the counter resumes rather than restarts. Also adds a test that every conversation gets its own tool sandbox directory, which parallel tool calls depend on. * Fix stalled tool calls while awaiting approval for PR #7455 Three problems, all from the approval prompt behaving as though only one chat could ever run. Arguments were not streamed for a gated call, so the chat stayed blank for as long as the model took to write the payload, which for a large file is minutes. Nothing runs before the decision either way, and the code is what is being approved, so python and terminal now stream their card while gated. render_html stays suppressed: its card renders the payload. The status read "Running ..." with a climbing timer while the call had not started. It now reports that it is waiting for approval, then switches to running once allowed. The admission lease was held across the wait, so four unanswered prompts held all four decode slots and no other chat could start while llama-server sat idle. A parked run keeps its lease but no longer counts against capacity. Measured with four prompts left open: every gated call streamed its code, none reported running, and a fresh chat answered in 0.4s where it previously waited 290s and never did. * Fix duplicated and truncated tool cards for PR #7455 A gated tool call rendered two cards: the provisional one that streams the arguments, plus a second one keyed by the approval id. Only the second ever got its tool_end, so the first spun "Running" for the rest of the chat. Reuse the open part when the approval prompt arrives. The terminal card also showed nothing but a 60-char trigger label, so a long heredoc read as no progress at all. It now renders the command the same way the Python card renders its script, and neither is capped at 10k chars. Both cells moved inside the collapsible, so one chevron hides the code with the output and Copy / Download exist only while the card is open. A card parked on the prompt says so instead of counting up "Running". * Fix review findings on the parallel-chat gate for PR #7455 Backend: - /unload rechecks active generations under the lifecycle gate, like /load, and lets its 409 through the catch-all instead of rewriting it as a 500. - /load gates only once _load_model_impl has decided this is a real reload, so an Apply on the already-loaded model no longer refuses, and the retry it asks for no longer cancels every chat before returning already_loaded. - The direct /v1/responses stream registers in the cancel registry, so a non-forced unload can no longer tear llama-server down under it. - run_server defaults to the same slot count as the CLI. colab.py calls it without the argument, so Colab was still serialising every chat. Frontend: - Cancelling a backgrounded chat aborts its own request rather than only posting a cancel id, which is the only thing that ends an external-provider or audio run. - The model-swap dialog counts local runs only, and falls back to the backend when this tab's map is empty, so a reload or a second tab still gets asked. - Context usage and the diffusion canvas are scoped to the chat that produced them; a compare row reads activity from its member threads. Tests: - The extracted-source cancel harnesses supply the active-generations module, which the tracked-cancel class now depends on. * Fix the swap confirmation scope and cancel timing for PR #7455 A forced load cancelled every chat before the model identifier, GPU selection, training coexistence and download checks had run, so a load that then failed those checks stopped the chats and replaced nothing. The refusal still happens early, but the destructive cancel now sits immediately before the teardown it is paying for, and rechecks under the gate like /unload does. The swap dialog only reconciled with the backend when this tab looked idle, so one local chat was enough to hide a second tab's runs. Confirming then sent force_cancel_active, which cancels every backend run, including the ones the dialog never mentioned. The backend snapshot is now merged in every time, so the dialog names what will actually stop. External-provider runs are never registered there, so the union stays local-only. Also drops the active-generations docstring claim about restoring sidebar spinners, which nothing consumes. * Defer destructive cancels and track every local stream for PR #7455 /unload cancelled the running chats before it had resolved that it unloads anything. A stale model_path, which a second tab produces routinely, killed every chat and then no-opped, leaving the resident model up. It now refuses early and cancels only at each teardown, matching /load. The swap dialog also stopped every chat locally the moment the user confirmed, which threw away the two-phase backend behaviour: a load that then failed identifier resolution, GPU validation or the training guard had already truncated the replies. The backend now owns the cancel. Three local streams decoded on llama-server without registering, so a non-forced unload counted zero generations and tore the server down mid response: /v1/completions streaming, and the plain and server-tool Anthropic streams, the first of which is the default /v1/messages path. Note this makes a non-forced load return 409 during those runs rather than draining quietly, the same trade the /v1/responses fix made. The safetensors tool loop still announced a gated call as running while it waited on a human; only the GGUF loop had been fixed. A source-level parity test now pins both. Also drops stopAllChatThreads, which has no callers left. * Studio: close three load/unload gate races found in review Re-check the in-flight load guard after the stop-running-chats confirm. The confirm always GETs active-generations before its zero-running early-out, so the guard no longer sits atomically ahead of the reservation and two picks in that window both reached performLoad over the same refs. ejectModel had the same shape and gets the same re-check. Reject a sidecar swap immediately before the forced cancel in both load branches. The previous check was back at the top of preflight, so an install reserving during identifier resolution, the tier probe, the training guard or the download check made the post-drain recheck 409 a load whose chats had already been stopped. Enter the Anthropic passthrough's cancel tracker inside its body generator. It was entered eagerly and returned through _sse_streaming_response, which sets no unstarted_cleanup, so a response whose body never started left the run registered forever and 409'd every later non-forced load and unload. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Trim comments across the files this PR touches Tightens the comments and doc blocks in the backend, CLI, tests and frontend files changed by this PR: collapses multi-line explanations to a single line where they still read clearly, and drops the ones the code already says. No code changes, verified by an AST comparison against the previous commit. * Studio: defer the destructive cancel and close two gate gaps Move the forced cancel behind every check that can still reject a swap. The drain now runs first with the runs it is about to cancel discounted, so it waits only for inference the cancel cannot end, then the sidecar check decides, then the cancel fires, then a second drain lets those runs unwind before teardown. A sidecar install reserving during the drain no longer 409s a load whose chats have already been stopped. Track the non-streaming /v1/completions proxy. It was the last local decode path missing from active_generations, so an unload, which runs no drain, tore llama-server down under it and force_cancel_active could not signal it. It now uses the same tracked cancel event and dedicated client as the OpenAI pass-through. Skip the client's preliminary unload while chats are generating and let /load evict at its own post-preflight point instead. Forwarding force_cancel_active there truncated replies before identifier resolution, the GPU and training guards and the download check had run. Keep per-thread context usage so returning to a chat whose background run finished restores its bar instead of leaving it blank until the next turn. Make the running-flag clear run-specific. Every run without a resolved thread id shares the "__default" key, so concurrent compare panes could clear each other's flag and strand a live stop handle. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Studio: register the embeddings proxy with the swap gate /v1/embeddings proxied straight through the pooled client with no tracked cancel event, so it never appeared in active_generations. /unload runs no idle drain, so a concurrent non-forced unload counted zero generations and killed llama-server mid-request, and force_cancel_active had no event to signal. Mirrors the completions proxy: tracked event, dedicated unpooled client closed by a cancel/disconnect watcher, unregister in a nested finally so a close failure cannot leave a phantom generation behind. * Trim comments on the newest changes in this PR Comments only, no code changes: shorten the ones added by the load-gate ordering, embeddings and per-thread usage work down to the same density as the rest of the diff. * Studio: register the legacy generate stream with the swap gate /generate/stream built a cancel event but never entered the tracker, so it was invisible to active_generations. Being in the keep-warm middleware's inference suffixes only covers /load, which drains; /unload does not, so a non-forced unload passed the 409 gate and then blocked on the standard backend's generation lock, and a forced swap had no event to signal. Registered inside the body generator under a nested finally so a teardown failure cannot skip the unregister. The AST contract test asserted the cleanup finally by overwriting its flag per Try node, so a nested try made the last one win. Accumulate instead, which is what the existence claim meant. * Studio: three more swap-gate gaps found in review Register /audio/generate with the gate. TTS holds the model for the whole request and /unload runs no drain, so unregistered a non-forced swap counted zero generations and tore the model down mid-generation; the orchestrator path only waits 15s for the generation lock, which real TTS exceeds. No cancel keys: no backend takes a cancel_event for audio, so the event has no observer and a forced swap still cannot interrupt audio already in flight. Thread the tracked cancel event into the /v1/responses admission wait. It was the only admission caller passing None, so a queued run could not be reached by cancel_all() and a plain /inference/cancel could not stop it at all. Same omission fixed at the upstream send there and on /v1/completions. Let an unforced unload of a stale model path reach the no-op check. Before this PR that request returned 200 and did nothing; the new gate refused it with 409 for a request that reaches no teardown branch. Gate both refusal passes on the disjunction of the route's own teardown conditions, including not is_loaded, so a mid-load GGUF still refuses. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Studio: register the remaining non-streaming decode paths stream defaults to false on all three of these, so they are the ordinary shape of their routes, and each holds a local backend for the whole request. /unload runs no idle drain, so with no registry entry a non-forced swap counted zero generations and tore the backend down mid-request instead of returning 409, and a forced one had no event to signal. Non-streaming /v1/messages: all three helpers ran with an empty registry, since only the streaming siblings were tracked. Registered at the call site because the pass-through takes no cancel_event of its own, and with no cancel keys, matching those siblings. Non-streaming standard chat and audio-input chat: the trackers in this route sit inside their `if payload.stream:` arms, so neither else branch was covered. The GGUF sibling already registers its own non-streaming branch. Each exit is in a finally on the branch's existing try, so the except arms are covered too: a leaked entry 409s every later swap until restart. * Studio: tighten the swap-gate comments Comment-only pass over the newest swap-gate registrations: collapse the multi-line rationales in /unload, the legacy generate stream, audio generation and the non-streaming chat branches, and the matching test preambles, to the shortest form that still carries the reason. No code changes. * Studio: stop the reselect dialog promising a stop that never happens Picking an external provider leaves the local model resident and stops the status poll mirroring it, so reselecting that model showed the stop-chats dialog, and /load then answered already_loaded ahead of its cancel hook. Confirmed with the live backend: the same pick with force_cancel_active set still returned already_loaded and the chat kept streaming. Not stopping those chats is right, since the load never interrupts them, so remove the prompt rather than honour it. Blanket-skipping is unsafe, because the same id and variant with one sampling setting changed is a real reload and 409s, so the branch only fires when a status fetch confirms the resident checkpoint and variant match, and then adopts it without calling /load. Redact native model paths from the active-generations response. Registering /generate/stream recorded backend.active_model_name verbatim, which is an absolute path for a native local model, and this route is the only place that serialises it. Redacting at the response covers every tracker rather than the one that surfaced it. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Studio: keep hydrated context usage in the per-thread map The history loader restores a saved conversation's usage through setContextUsage only, and it runs once per mount, so switching away and back left the bar blank for a hydrated chat even after the per-thread map landed. setContextUsage now writes the value through to the visible thread's own entry and clears that entry when passed null, which covers both hydration call sites and any future writer. * Studio: unblock load cancellation and share unresolved thread keys Run the two stop-loading fast paths ahead of the unload route's pre-gate refusal. _unload_may_evict returns True for exactly the model being cancelled, so the refusal was blocking the branch that cancels a load which has replaced nothing and can interrupt no chat. The client made that unrecoverable: cancelLoading sends the unload without force, drops the result, and its abort never reaches /load, which takes no signal, so the load ran on and could later cancel those chats and swap the model. Nothing else is exempted; an unload that would tear down a serving model matches neither fast path and still 409s. The comment claiming the client lets that 409 surface is corrected, since it discards it. Hold every owner behind a shared thread key. Runs with no resolved thread id share "__default" (concurrent compare panes, since startCompare clears activeThreadId), so a single owner slot let a second run replace the first's token and then delete the shared entry while it was still generating, and the server-cancel map lost the older handle the same way. Both now hold a list, the running and local flags survive until the last owner clears, and stopChatThread stops every handle under the key. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Studio: carry a confirmed swap into the sidecar install, key restored usage by thread Picking a model that needs a newer transformers while chats generate raised the "stop N chats" prompt, but the answer never reached the install that runs before the load: /install-latest-transformers refused on those same chats and took no force flag, so Retry hit the same 409 and nothing in the flow stopped them. Carry force_cancel_active through the consent dialog into the installer. Only the pre-gate fast path is skipped: the recheck under the lifecycle gate still has to pass, so an unconfirmed caller is refused as before. The cancel runs last inside the gate, after every check that can still reject the install, and the drain behind it is bounded since it holds the gate and the sidecar reservation. Also key restored context usage by the thread the loader read. history.load() captures remoteId before two awaited round trips, so a switch inside that window filed one thread's usage under another and setActiveThreadId kept re-applying it. Preserve sibling owners when a run key is cleared without an owner: the image rejection gate now uses its own token, and the reducer leaves owned runs alone. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Studio: bound the post-cancel drains, and make cancellation reach the paths that ignored it A forced swap cancels the chats it interrupts, then waits for them to unwind. That wait had no deadline while holding the lifecycle gate, and TTS on the subprocess backend observes no cancel event at all, so one audio generation could pin every load, unload and new request for its whole duration. Bound both post-cancel drains. Pre-cancel drains stay unbounded: the swap can still be refused there, so shortening them would weaken what they protect. /unload had the opposite problem and no drain at all, cancelling and tearing down on the next line, which turned a clean stream end into a dropped connection. Give it the same bounded wait, gated on the cancel having cancelled something so an idle Eject pays nothing. Make the cancel actually land where it can. GGUF TTS now takes a cancel_event and a watcher closes its client to break the blocking POST. The Anthropic non-streaming pass-through did the same thing the completions and embeddings paths used to: register with the gate, then run both POSTs on the pooled client that cannot be closed. It now uses a per-request client like they do. Also: park and unpark the admission queue the reservation actually holds, since queues are keyed by base_url and a reload mints a new port; key tool output by remoteId on both sides, so the first turn of a New Chat stops writing under one key and reading another; and give tool status a run owner, so a finishing run cannot blank the badge a concurrent one is still showing. Clamp --parallel to 1 on a llama-server without --kv-unified. The new default of 4 would otherwise split -c four ways on such a build, quartering the context window for a feature it cannot serve. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Studio: scope a chat's Stop to its own generation, and clear the way before a confirmed install Safetensors generation is serialized on _gen_lock and the worker has a single cancel event, so a chat still queued on that lock owns no generation. Its Stop handler called reset_generation_state() anyway, which set the shared event and ended whichever conversation was actually running. Parallel chats is what makes that reachable. _generate_inner now records its cancel_event as the current holder once it takes the lock, and reset_generation_state drops a reset from anyone else. Every route call site passes its own request event. A reset with no event stays global, so unload and model switch cannot leave a generation alive, and a reset while nothing runs still resets, so an error path before generation is not a no-op. The other two backends take the argument too, or the standard one raises TypeError on every cancel. The sidecar install had the mirror of the /load ordering problem: it cancelled the chats first and drained second, so an unrelated counted request the cancel cannot reach (a count_tokens, say) was still there for the recheck, which then refused an install that had already stopped every chat for nothing. Drain the unreachable remainder first, discounting the registered chats, then cancel. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Studio: close the windows the previous round's fixes left open Three follow-ups, two of them holes in the fixes just before them. The worker claim went in after _send_cmd, so the command was already running unclaimed and a queued chat's Stop in that window still reset it. Claim first, with the send inside the same try, so a failed send releases it too. Tool status kept one entry per key with an owner. That stops a foreign clear but not an overwrite: under the shared unresolved-thread key the second run replaced the first's entry, and its own clear then removed the only one while the first tool was still running. Keep per-run entries and render the newest. /unload gated its drain on having cancelled something, so a request that passed the keep-warm middleware but had not reached its tracker yet was invisible to it and the teardown landed on an already-admitted request. Drain on the middleware count instead, which covers that window as well as the cancelled runs, then re-cancel whatever registered while waiting. Bounded, not a refusal: an unload is deliberate, and on expiry it proceeds exactly as before. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Trim the parallel-chats comments to their reasons Compress the multi-line rationales added by this branch into shorter forms and drop restatements of the code below them. The reasons behind the drain bounds, the deferred cancel, the per-request generation ownership and the thread-scoped tool and usage keys are kept, just said in fewer lines. * Studio: own the worker per generation, and make a resumed chat requeue for its slot Ownership was a single lock holder, so dispatched runs (compare mode bypasses _gen_lock by design) never claimed it and the guard fell straight through to the global reset: a Stop on one of them ended its siblings. Track the generations actually running instead, claimed before the send and released in the same finally on both paths. A reset still proceeds when nothing is running, so an error path ahead of generation is not swallowed. park() hands the freed slot to a waiter, so a chat resuming from a tool approval could take it back while that waiter was still decoding, putting two holders on a one-slot server and sending the resumed tool loop past the admission limit. unpark_async waits for room; the plain unpark stays for a holder tearing down, which will not decode again. Audio only observed its cancel event on a forced swap. An explicit Stop just aborts the fetch, and this route has no cancel id, so llama-server ran on to the request timeout after the chat reported it stopped. Watch the disconnect. Also read tool status by remoteId, matching the key the adapter writes and the fix already made for tool output, and stop an unresolved run from writing its usage into whichever conversation the user moved to. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Studio: let only the generation the worker is running speak for it, and hold a slot for a resuming chat The ownership list recorded admission, but the subprocess runs generations one at a time, so a dispatched request queued behind another counted as an owner and its Stop signalled the shared cancel event, ending the request that was actually running. Keep admission for release bookkeeping and gate ownership on execution instead, promoted when the worker first answers that request. Nothing executing still permits a reset, so an error path ahead of generation is not swallowed. The worker has one cancel event and no per-request cancellation, so this decides who may pull the lever rather than making the lever per-request. A resuming chat also polled for a slot it could never see: release() grants to the next waiter under the same lock, so later arrivals overtook an approved chat indefinitely. A pending unpark now reserves the next slot and they queue behind it. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Studio: cover the prefill window, and keep a first turn's tool output readable Gating worker ownership on execution left the interval between the send and the first response uncovered: nothing is executing then, and the empty case admitted anyone, so a queued chat's Stop still ended the one in prefill. Split the empty case. Nothing claimed at all still permits a reset, so an error path ahead of generation is not swallowed; claimed but unanswered resolves to the oldest claim, which is what a FIFO command queue is working on. Putting both sides of the tool-output scope on remoteId left the first turn of a New Chat writing under the unresolved scope for its whole life while the readers recomputed the moment the autosave assigned an id, so the card blanked mid-run. The readers now fall back to the unresolved scope, which only an unpersisted first turn can occupy. * Studio: order the parked approvals, and tie a worker claim to its enqueue The reservation added for admission fairness was a bare count, so every approved holder counted against every other: park two chats, approve both, and once the last decoder released, nothing could ever satisfy the check again. That is a deadlock where the problem it fixed was only unfairness. Make it a FIFO ticket so a pending unpark blocks the ones behind it and no others. _owns_worker reads claim order to decide which request the worker is prefilling, which only holds if claiming and enqueuing cannot interleave. Hold one lock across both on the dispatched and the locked path. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Studio: follow a first turn's run when its thread gets an id, and key the denoising canvas per chat A run started before its thread existed filed every handle under "__default". Nothing moved them once autosave assigned the real id, so the sidebar row showed no spinner and Stop could not reach the generation, which kept holding a slot. adoptDefaultThreadRun re-keys the run maps onto the real id from the thread adapter's initialize(), where the id first exists; anything already filed under that id wins, since that is a later run. The adapter captures its key once at run start, so it now resolves the live key per use through runKeyForOwner, looking its own serverCancel up in the owner map. Without that the migrated entries are stranded and the spinner never clears. The denoising canvas was one global slot, so two diffusion chats overwrote each other and the ownership tag then hid the visible preview until that thread emitted again. It is now activeDiffusionCanvasByThreadId, written and cleared per thread, and the frame no longer carries a threadId of its own. The bubble reads threadListItem.remoteId, dropping the dead threadListItem.id arm: the writer tags unstable_threadId, which is exactly remoteId. Two existing backend tests needed the same treatment. _bare_orchestrator skips __init__, so it now sets the claim bookkeeping the worker ownership check reads. The Anthropic passthrough gate test anchored on comment prose that a rewrap had broken; it anchors on the code instead. * Studio: hand the worker over cleanly between generations, and stop unresolved runs sharing each other's state Worker ownership moved off the consumer and onto the dispatcher. Consumers read their mailbox whenever they get around to it, so a request whose gen_done had been routed still owned the worker while the next one ran, and a late Stop for it cancelled that one. The dispatcher is the only place responses arrive in the order the worker produced them: it now retires a request at its terminal response and promotes the next one, and answering a request makes it the sole executor, since the subprocess runs one generation at a time. reserve()'s immediate path ignored the unpark tickets that _grant_waiters_locked already honours, so a request arriving between a slot freeing and an approved chat's next poll took it, repeatedly. It applies the same reservation now. Three places let concurrent first turns share state through the "__default" key. Nothing links a run filed there to the id its thread later receives, so rather than guess, each now declines when the key is ambiguous: adoption only re-keys a lone run, the composer badge only claims a lone status, and the tool-output fallback only applies to a thread that is still running. That leaves two concurrent first turns where they were before adoption existed instead of handing one thread the other's handles. A first turn's usage was never filed, because its key stayed null for the whole run while autosave moved activeThreadId to the real id, so the context bar went blank after the first reply. It resolves the adopted key like the cleanup handles do. Cancelling a forced load left the UI with no model: the previous one stays resident until /load's teardown, and the cancel path cleared the checkpoint without rolling back. It now resyncs from the backend, which is right whether or not the load got that far. The sidecar install drain is weighted 1:4 rather than halved, total unchanged. Only the second half benefits from patience, and cutting it short refused installs whose chats had already been stopped for nothing. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Studio: give a first turn its real thread id before the run starts A first turn filed every run handle under a shared unresolved key because assistant-ui binds unstable_threadId before the thread is persisted. Two of them overlapping there is unresolvable afterwards, and the last round's migration could only decline rather than guess, which left neither sidebar row showing its run. The id is available earlier than I claimed. append() already tracks threadListItem.initialize() by the user message id, and createPersistedRunAdapter already awaits that promise before invoking the adapter, so the thread is persisted by the time the run begins. It was only being discarded: the tracked promise resolved to void. It now resolves to the assigned id, and the wrapper hands it to the adapter when assistant-ui had none. An id that is already set is never replaced, since that would move a running chat's handles out from under the row watching them. The existing unresolved-key guards stay as a safety net but should no longer carry weight. The sidebar counted running thread ids rather than rows, so one compare conversation read as two chats. It folds ids into rows through the same threadIds the row spinner uses, and still counts a running id that matches no row. _TrackedCancel always registered kind="chat", so an embeddings or raw completions request appeared in the model-swap prompt as an unnamed conversation and confirming cancelled it while calling it a chat. The non-conversation routes now pass their own kind, and the prompt says "requests" whenever the snapshot is not all chats. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Studio: withhold the shared worker cancel from a request the worker has left Moving ownership to the dispatcher fixed reset_generation_state, but the token loop signals the shared worker event directly and did not carry the same rule. A dispatched consumer runs with mark_started off and can still be draining tokens buffered before its gen_done was routed, so stopping it there ended whichever request the worker had started next. It now signals only when _owns_worker agrees, the same predicate reset_generation_state uses. The local drain and return are unconditional, since those touch nothing but this stream. The remaining _cancel_generation callers are deliberately global: subprocess shutdown, the pre-load kill and unload_model. * Studio: add the AGPL-3.0 header to the first-turn identity test * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Studio: stop the dispatcher and a _gen_lock stream fighting over the response queue Nothing stopped the dispatcher starting under a _gen_lock generation, so once compare was opened while an ordinary chat was still streaming, both consumed _resp_queue and whichever response the dispatcher took without a mailbox was dropped, gen_done included. That chat truncated or hung. This PR is what makes it reachable, since navigating into compare no longer ends the chat behind it. Delaying the dispatcher would serialise compare behind whatever chat happens to be streaming, so the direct readers get a mailbox instead. _direct_reader returns a reader, a cancel drain and a release, and files the mailbox under _direct_mailboxes rather than _mailboxes, which means "compare requests are in flight" to the unload and distributed paths and must not count an ordinary chat. Both directions close. The dispatcher finds the direct reader's mailbox instead of dropping. And this reader can already be blocked on the queue when a compare request's dispatcher starts, so a response that is not ours goes to its own mailbox rather than being consumed, which would have corrupted the chat and hung the pane. All three _gen_lock readers use it, and the cancel drain goes through it too. The sidebar's return target still picked a raw pane id while the count grouped by row, and /chat addresses compare with `compare`, not `thread`. It resolves through the same items now, so a running compare row returns to its pair. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Studio: keep worker ownership honest across audio, API traffic and a replaced worker The audio-input send got a mailbox last round but stayed unclaimed, so a compare request queued behind it looked like the oldest owner and stopping that queued request signalled the shared event into the audio chat. It claims under the send lock and releases in the finally, like _generate_inner. Ownership is keyed on cancel-event identity with nothing tying it to a worker generation, so a consumer still blocked on its mailbox when the process was replaced stayed recorded as the executor, and a generation on the fresh worker could not be stopped. _shutdown_subprocess clears that state once the process is confirmed dead, mailboxes included: nothing routes to them again, and a stale one reads as compare activity to the unload path. Not on the survived-SIGKILL path, which keeps its handle on purpose. The four public /v1/messages trackers were registering as chats. The distinction is a Studio thread, not the protocol, and those branches already say "No thread_id: public API surface" while the Studio path passes payload.thread_id separately. They carry their own kind now, so the swap prompt stops calling an external request a chat. The swap confirmation still counted raw pane ids, so a compare conversation asked to stop two chats and listed its title twice. It folds panes onto pairId and lowers the count by what it collapsed, leaving a first turn the backend can count but not name. Deep Research set runningByThreadId but registered no server-cancel handle, and that map is how Stop, archive and delete reach a thread that is no longer active. Leaving the outgoing thread running is this PR's doing, so the run was left unreachable while its supervisor kept working against a conversation the user could delete. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Studio: tighten the parallel-chats comments * Studio: replay a Deep Research stop that arrived before the run existed The handle is registered before createResearchRun resolves because the thread can be stopped while that request is in flight, but it had no id to act on and dropped the stop. The supervisor then followed a run the user had already stopped, archived or deleted. It latches instead: a stop with no id yet sets a flag, and the adapter replays it against the id the moment creation returns rather than starting to follow. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Studio: fix worker ownership on a raced reroute, and the stop-chats prompt Four review findings on the parallel-chats work, all reproduced first. - _direct_reader hands a foreign response to its own mailbox, but skipped the ownership move the dispatcher makes. A _gen_lock reader already blocked on resp_queue can beat the compare dispatcher to that request's first response, and the compare consumer opts out of marking, so nothing promoted it: the direct request stayed the recorded executor, its late reset cancelled the compare generation, and the compare chat's own Stop was ignored. - A chat stopped while queued on _gen_lock was still claimed and sent once the lock freed. Cancellation is only checked on a token, so a long prefill, or a generation reaching gen_done without one, occupied the worker after Stop. Same hole in the audio-input path, which shares the lock. - The stop-chats prompt counted generation handles, not conversations. One chat holds several while a tool continuation registers its next leg before the previous unwinds, so it offered to stop two chats and listed one title. - Ejecting a model confirms through that dialog, which told the user "Unloading the model reloads the model" and offered "Stop and reload". Confirming calls /unload and leaves nothing loaded. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Studio: name the TTS run's thread so the stop prompt counts it once The audio branch registers its run locally under the thread key but sent no thread_id, so the backend tracker filed the same generation under no thread. The stop-chats prompt then had a named local run and an unnamed backend one and, since e8e7594 started adding unnamed entries to the named ones, counted a single TTS chat as two requests. The backend already reads payload.thread_id, so sending it lines both registries up on the same run. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> |
||
|
Lee Jackson
|
3230a10a9c
|
Fix Windows Codex temporary home path (#7519)
* Fix Windows Codex temporary home path * Fix Codex ephemeral session cleanup * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Harden Codex temp home reclamation --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> |
||
|
Lee Jackson
|
31699f9c04
|
Default coding-agent servers to reasoning off (#7521)
* Default coding agent servers to reasoning off * Fix reasoning startup compatibility and attach warning |
||
|
Daniel Han
|
1781770bee
|
Studio: detect an interrupted dependency install instead of launching a backend that cannot import (#7492)
Some checks are pending
Unsloth GGUF CI / JSON, images (push) Waiting to run
Mac Studio UI CI / Chat UI Tests (push) Waiting to run
Unsloth Update CI / Unsloth Updating Tests (push) Waiting to run
Windows Unsloth API CI / Unsloth API & Auth Tests (push) Waiting to run
Windows Unsloth GGUF CI / OpenAI, Anthropic API tests (push) Waiting to run
Windows Unsloth GGUF CI / Tool calling Tests (push) Waiting to run
Windows Unsloth GGUF CI / JSON, images (push) Waiting to run
Windows Unsloth GGUF CI / Unsloth install + inference without Visual Studio (push) Waiting to run
Windows Unsloth GGUF CI / GPU prebuilt resolves without Visual Studio (push) Waiting to run
Unsloth GGUF CI / Tool calling Tests (push) Waiting to run
Mac Studio API CI / Unsloth API & Auth Tests (push) Waiting to run
Mac Studio GGUF CI / OpenAI, Anthropic API tests (push) Waiting to run
Mac Studio GGUF CI / Tool calling Tests (push) Waiting to run
Mac Studio GGUF CI / JSON, images (push) Waiting to run
Mac Studio Install Matrix CI / Install + load (macos-14) (push) Waiting to run
Mac Studio Install Matrix CI / Install + load (macos-15) (push) Waiting to run
Mac Studio Install Matrix CI / Install + load (macos-26) (push) Waiting to run
Mac Studio Install Matrix CI / Install + load (macos-15-intel) (push) Waiting to run
Mac Studio Install Matrix CI / Install + load (macos-26-intel) (push) Waiting to run
Mac Studio Update CI / Unsloth Updating Tests (push) Waiting to run
Unsloth Tauri CI / Tauri Linux debug build (no codesign) (push) Waiting to run
Unsloth UI CI / Chat UI Tests (push) Waiting to run
Windows Unsloth GGUF CI / VC++ runtime detect + install round-trip (windows-latest) (push) Waiting to run
Windows Unsloth UI CI / Chat UI Tests (push) Waiting to run
Windows Unsloth Update CI / Unsloth Updating Tests (push) Waiting to run
Wheel CI / Wheel build + content sanity + import smoke (push) Waiting to run
Windows Unsloth GGUF CI / setup.ps1 unit tests (VS 2026 / CMake guard) (push) Waiting to run
Windows Unsloth GGUF CI / real-VS detection (VS 2022) (push) Waiting to run
Windows Unsloth GGUF CI / real-VS detection (VS 2026) (push) Waiting to run
Windows Unsloth GGUF CI / VC++ runtime detect + install round-trip (windows-2025-vs2026) (push) Waiting to run
* Studio: detect an interrupted dependency install instead of launching a backend that cannot import An installer killed part-way leaves a venv with a working CLI but without studio.txt's dependencies. Nothing recorded that, so three separate places all reported it healthy: - the desktop preflight probed only `unsloth -h` (typer + rich) and a hardcoded desktop-capabilities dict, neither of which touches studio.backend, so it returned ManagedReady and spawned a backend that died on `import structlog`; - setup.sh's fast path compared the installed unsloth version against PyPI, which matches on a half-built venv because unsloth is installed early, so `unsloth studio update` printed "up to date" and repaired nothing; - start_managed_repair calls that update and then re-checks with the same blind probes, so Repair reported success without fixing anything. install_python_stack.py now clears a completion manifest before the dependency pass and writes it only after the final step. `unsloth studio verify-install` and desktop-capabilities' new studio_install_ok field read it, the preflight turns a false answer into ManagedStale so auto-repair runs, and setup.sh / setup.ps1 gain an escape hatch next to the existing anyio one. Separately, the wheel ships studio/ and studio.backend* but declared none of their dependencies, so `unsloth train`, `export`, `chat`, `inference` and `studio` all ended in a rich traceback after a plain pip install. structlog is the only hard module-level import that chain reaches once starlette's annotation-only import moves under TYPE_CHECKING, so it becomes a core dependency and the rest of the server stack becomes a [studio] extra mirroring studio.txt. The CLI import sites now report missing dependencies as a sentence with two remedies. Fixes #4701, #5260, #7147 * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Match the trimmed comments merged on the pip branch * Put the install manifest in the preflight fingerprint for PR #7492 The capability cache keyed the venv on pyvenv.cfg, uv.lock, requirements.txt, the interpreter and site-packages/unsloth_cli/commands/studio.py, none of which a repair touches when it only reinstalls studio.txt. So an entry cached while the install was healthy stayed valid after the manifest was dropped, and the probe returned Ready on exactly the half-built venv this is meant to catch. * Address the review findings on PR #7492 Fail the install when the completion manifest cannot be written, instead of exiting 0 without the record every later check requires, which is a repair loop by construction. Compare the version of the package the manifest names, so `studio update --package X` does not read as a permanent version change. Read the manifest from the venv that owns it when the CLI runs outside the managed venv, and drop the dependency verdict in that case: the walk ran against the wrong interpreter and says nothing about that venv. Name the import that actually failed. `unsloth train` reaches torch through the same guard, and the studio extra does not carry it, so recommending that extra alone left the command failing in the same place. * Declare click, which typer stopped providing, for PR #7492 unsloth_cli/commands/start.py imports click at module scope and unsloth_cli/__init__.py imports that module, so every unsloth command needs it. typer carried click through 0.19 and dropped it in 0.27, and the declared floor is typer>=0.12.0, so a fresh resolve gets no click. On the published wheel it still arrives because huggingface_hub requires click<9,>=8.4.2, which is luck rather than a declaration. A wheel built from this branch's dependency list has neither, and every command dies at import. Verified: before, `unsloth --help` on a fresh venv raised ModuleNotFoundError for click; after, it exits 0. The drift test now covers it. * Keep a running backend from the previous app version manageable The manageability bump gated two unrelated things through one constant. For the managed CLI probe 2 is right: a CLI reporting 1 cannot answer studio_install_ok. For a RUNNING backend it is wrong, because a process already started cannot change what it reports, so bumping studio/backend/main.py in lockstep does not help one the previous app version spawned. That backend is proven ours by root id and ownership token, but lifecycle_control_block_reason returned Unmanageable, and that branch never calls adopt_verified_backend. has_owned_backend() stays false, so Repair falls into block_external_conflict, which finds the same process and refuses: the app could no longer stop a backend it owns the token for. The same regression in backend.rs turned a terminal-launched same-root server from AttachedReady into ExternalConflict. Split the constant: DESKTOP_BACKEND_MANAGEABILITY_VERSION = 1 for the two live-backend probes, DESKTOP_MANAGEABILITY_VERSION = 2 for the CLI probe. Every real gate (protocol, auth, ownership, desktop-login, MIN_DESKTOP_BACKEND_VERSION) is untouched, so an old backend still reaches OwnedStale, adopt, stop, repair. Also stop the installer when the stale manifest cannot be removed. Windows raises on a read-only or locked file, and the pass would then run behind a marker that still names this version and these digests, so a run killed part-way would verify as complete. * Answer for the managed venv, not the one the CLI happens to run in The guard matched ModuleNotFoundError.name, an import name, against missing_requirements(), which returns distribution names. So a missing PyJWT printed 'pip install jwt', and jwt, docx and fitz are each a real but unrelated PyPI project (fitz is a neuroimaging workflow tool), so following the advice installed the wrong package and left the backend just as broken. Map the import to its distribution before deciding, and never offer the import itself. install_state() verified the caller's own prefix. The wheel ships studio/, so a CLI installed outside the managed venv always finds its own copy of the helper first, and a healthy managed install reported studio_install_incomplete with a missing list copied from the wrong venv. Selecting the root is not enough: _installed_version() reads the running interpreter and req_root defaults to the caller's studio.txt, so both checks still answered for the wrong venv. Hand verify_install() that venv's own metadata, enumerated through Distribution.discover(context = ...path), which does not fall back to sys.path. The candidate order is untouched, so shadowed-tree detection is unchanged. setup.ps1 replaces pip, torch and triton before install_python_stack.py runs, so the manifest it drops is not dropped before the first mutation. A run killed in between kept a marker that still verifies while torch was half-replaced; drop it at the top of the dependency pass instead. setup.sh is unaffected, the stack is the first thing its pass runs, and a test now pins both. pip uninstall rewrites nothing that was fingerprinted, and cache_matches re-reads the cached studio_install_ok rather than re-checking, so a venv that lost a studio.txt package kept being served the healthy verdict. Fold a sorted hash of the installed dist-info names into the marker hash. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * A missing manifest helper is a torn install, not an old one studio/install_manifest.py ships in the same wheel as _studio_deps.py, so nothing legitimately has one without the other: a CLI predating both never reaches this code, and the desktop already calls such a CLI stale on desktop_manageability_version. Returning ok=true there reported a healthy install for a tree the package update had half replaced, and the preflight then launched a backend whose own run.py could be just as absent. Report it incomplete so repair runs. * Tighten comments across the install-detection changes * Validate Studio dependency readiness --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Lee Jackson <130007945+Imagineer99@users.noreply.github.com> Co-authored-by: Wasim Yousef Said <wasimysdev@gmail.com> |
||
|
oobabooga
|
0b34377778
|
Studio: Expose GPU memory mode in unsloth run and unsloth start (#7421)
* Add CLI GPU memory mode selection * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Preserve manual GPU layer overrides --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Lee Jackson <130007945+Imagineer99@users.noreply.github.com> |
||
|
Daniel Han
|
7a9749eb4f
|
unsloth start: keep the local subagent unattended and out of plan mode (#7437)
* unsloth start: keep the local subagent unattended and out of plan mode The local subagent child could stall waiting on a permission prompt, and a parent session in plan mode could still reach the editing agent. - Drop human-blocking tools from the child so it runs unattended. The read-only child also drops the file writers. - Emit a PreToolUse hook that reads permission_mode itself and denies the editing agent under plan mode, so routing holds when the model ignores SKILL.md. Fails open, and is skipped under the WSL bridge where a Windows interpreter path is not runnable in the distro. * Make the read-only subagent actually read-only, and drop stale WSL gates From the first review of this branch, which drove the real code against a fake HOME holding a pre-existing Claude install and diffed the tree before and after. No config, agent, MCP server or CLAUDE.md of the user's was touched in either arm, and the session dir is removed on exit, Ctrl-C and exception. Three real findings came out of it: - The read-only child could still write. Plan mode routes Bash through a safety classifier served by the same local model, so a small model saying yes is what authorised the write; a child spawned with read_only created a file. Denying Bash there makes the label true, at the cost of shell exploration while planning. Read, Grep and Glob still cover the search it needs. - A persisted plugin dir kept a plan_gate.py from an earlier Windows run, so a later WSL run shipped a hooks.json naming an interpreter the distro cannot execute. Hook errors do not block, so this only ever wasted a spawn, but it accumulated and the branch had no test. - The comment claimed the read-only child keeps ExitPlanMode "as Claude does under plan mode". A --print child is never offered the plan or prompt tools at all, so most of both deny lists is inert today. Kept as a guard against a version that starts offering them, but the comment now says so. Also covers "auto" in the gate's non-plan modes, which is a real permission_mode and the one the child's own Bash classifier runs under. * Stop the gate failing closed, and bound a wedged child Second review of this branch, driving real claude 2.1.219 against a mock endpoint rather than reading. The gate could fail closed. If plan_gate.py went missing the interpreter exited 2, which Claude treats as a blocking hook error, so the editing tool was denied in every mode rather than just plan. Running the script through runpy instead of handing its path to the interpreter turns that into an ordinary traceback, which is exit 1 and allows. Verified both exit codes directly. The hook also had no timeout, so a hung one stalled the parent for as long as it hung, measured past 400s. Bounded at 10s. The real stall this branch is named for was untouched: run_local_agent polled communicate() forever, so a local server that accepts and never answers left the child and the parent blocked indefinitely, measured past 400s. Added a wall-clock deadline that kills the child and says the server looks wedged. UNSLOTH_CLAUDE_SUBAGENT_TIMEOUT overrides it, 0 restores the old behaviour. Also corrected the plan-mode comment. Claude already refuses the editing tool in plan mode on its own, since it advertises readOnlyHint false; what the hook adds is a reason naming the read-only tool to call instead. The WSL comment had the direction backwards: the gate is the Linux path, not the Windows one. Tests: the hook command's quoting and its behaviour with the gate deleted, both previously unguarded, plus the timeout and its env override. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Keep the gate path out of the shell string Codex review. The hook command is run by a shell, and the gate path was interpolated into it, so a session-config root containing shell metacharacters expanded before Python saw it. Verified on both: sh expands $(..), backticks and $VAR; cmd expands %VAR%. In every case the path no longer resolves, the gate exits 1, and because that intentionally fails open the routing message silently stops appearing. The path now travels as base64, whose alphabet has no metacharacter in either shell. Parametrised over all four hostile forms, and the old interpolation makes those tests fail. One correction to the report: it says the editing agent becomes callable in plan mode. It does not. Claude refuses that tool by itself, since it advertises readOnlyHint false, which was checked earlier by deleting the hook entirely. What a mangled path costs is the reason naming the read-only agent to call instead, not the block. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Lee Jackson <130007945+Imagineer99@users.noreply.github.com> |
||
|
Daniel Han
|
1daaa5cbb4
|
Let a decode failure degrade instead of escaping a fail-closed helper (#7487)
* Let a decode failure degrade instead of escaping a fail-closed helper Pinning utf-8 makes a read that used to return mojibake on Windows raise instead. 33 of those reads sit under a handler catching OSError or json.JSONDecodeError but not UnicodeDecodeError, which subclasses ValueError, so a corrupt file would now escape a helper written to return a default. Adds UnicodeDecodeError to those tuples only. * Treat an undecodable install lock as stale instead of retrying forever |
||
|
Daniel Han
|
3fd948eb95
|
Pin utf-8 on shipping-code text I/O instead of the operator locale (#7486)
* Pin utf-8 on shipping-code text I/O instead of the operator locale 113 read_text/write_text/open call sites across unsloth, studio and unsloth_cli let locale.getencoding() decide the encoding. That is utf-8 on the Linux and macOS runners and cp1252 on a stock Windows install, so the same file decodes differently for a Windows user and silently produces mojibake or raises UnicodeDecodeError. Adds tests/test_runtime_text_encoding.py to keep it that way. It resolves openers through each file's own imports rather than a fixed list of module names, so an aliased tarfile.open or a local from PIL.Image import open is not asked for an encoding it does not take. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Scan tracked files only and resolve the unbound Path calling forms * Honour PEP 263 when scanning sources and migrate a legacy JSONL before appending * Scope guard imports lexically and only migrate a legacy file when it round-trips * Leave a legacy JSONL untouched and resolve path aliases in the foreign-opener check * Tighten comments --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> |
||
Leo Borcherding
|
1dd2fc4583
|
tests: read checked-in files as UTF-8 instead of the platform default (#7438)
* tests: read checked-in files as UTF-8 instead of the platform default Path.read_text() with no encoding uses locale.getpreferredencoding(), which is UTF-8 on the Linux runners and cp1252 on a stock Windows install. Nine module-level reads of checked-in source files were relying on that default. studio/backend/routes/inference.py carries the DeepSeek tool-call token regexes, so it holds U+FF5C and U+2581. Under cp1252 that read raised UnicodeDecodeError on byte 0x81 at position 97806, and because the reads run at import time it took test_cancel_atomicity.py and test_cancel_id_wiring.py out at collection, not as failures. Green on CI, permanently broken for a Windows contributor running the suite locally. Adds a guard: at module scope there is no tmp_path fixture, so a bare read_text()/write_text()/open() there is always touching a checked-in file. That makes the rule mechanical enough to enforce with no allowlist, while staying quiet about temp-dir I/O inside test bodies where the platform default is harmless. The repo already spells this correctly in 464 other places; this only stops the stragglers coming back. * tests: cover import-time helper reads and keep the guard py3.9-safe Follows up on the Codex review: - add `from __future__ import annotations`, since `str | None` in `_offender` is evaluated at import on Python 3.9 and pyproject declares requires-python ">=3.9,<3.15". - widen the guard from module scope to import time. Class bodies and the bodies of module-level helpers called from an executing statement run during collection too, so `CODE = _extract_mixed_precision_code()` was the same hazard as an inline read. `if __name__ == "__main__":` blocks are skipped: pytest never executes them. - scan studio/backend/tests/ as well as tests/. Both trees are collected on Windows by separate CI jobs, and the offender that started this, test_tool_xml_strip.py reading routes/inference.py, lives there. Widening it surfaced seven more import-time reads of checked-in sources; all now name utf-8. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Harden the import-time encoding guard for PR #7438 Close the detector gaps raised in review, all of which I reproduced against the actual AST before changing anything. False negatives (the guard let a real hazard through): - _is_main_guard ignored the comparison operator, so if __name__ != "__main__" counted as script-only even though its body runs at import. - The else arm of a main guard was discarded with the rest of the If node. - Decorators and argument defaults on a module-level def were skipped with the body, though both are evaluated when the def executes. - Path.open() in text mode was invisible; only builtin open() was matched. - encoding = None and encoding = "locale" both re-select the platform default, but the keyword merely being present counted as pinned. False positives (the guard would have blocked a compliant contributor): - A non-literal mode fell through to the "r" default, so open(p, mode) was flagged even when mode is "rb", where adding encoding= is a ValueError and there is no edit that satisfies the rule. - Same for open(*args) and a **kwargs splat, which hide the mode and can hide an encoding. - Lambda bodies and comprehension elements were walked even though neither runs at definition. Verified: still reports the same 22 offenders on unpatched main, green on this branch and on the tree merged with latest main (557 files), and an adversarial corpus of 33 cases now scores zero false positives and zero false negatives. Also corrected two docstring claims: neither collecting job runs on Windows, and the read is governed by locale.getencoding(). * Walk eager comprehensions and treat io.open as the builtin Two regressions from the previous commit, both reproduced against the AST before changing anything. Lumping list, set and dict comprehensions in with generator expressions was wrong. Only a genexp is lazy; the other three run their element expression, their filters and their nested iterators immediately, so CONTENTS = [p.read_text() for p in PATHS] at module scope is an import-time read the guard was silently missing. Comprehensions are now walked in full and only the genexp keeps the outermost-iterable-only treatment. io was also in the not-a-path-opener list, but io.open is the builtin, with the same mode position and the same platform default. io.open(CHECKED_IN_FILE) is exactly the hazard this guard exists for, so it is matched now, with binary modes and a pinned encoding still exempt. tarfile.open and fitz.open stay exempt since neither has an encoding to name. Verified: 13 targeted cases covering all five eager comprehension forms and io.open in text, binary and pinned shapes all classify correctly; still 22 offenders on unpatched main; green on this branch and on the tree merged with latest main. * Close three more walker gaps in the import-time guard All three reproduced against the AST first. A generator expression handed straight to a call is consumed there, so DATA = "".join(p.read_text() for p in paths) runs its element at import. Only an unconsumed genexp bound to a name stays lazy, so the walker now follows the consumed ones in full and keeps the outermost-iterable-only treatment for the rest. if "__main__" == __name__ is an equivalent and accepted spelling of the main guard, but requiring __name__ on the left meant its body was treated as import-time code. That is a false positive on a block pytest never runs, so both operand orders are recognised now. The helper table was built from module-level defs only, so a def in a class body invoked while the class is constructed was never followed, contradicting the walker's stated coverage of class bodies. Helpers are now collected from the module body and from class bodies at any nesting. Verified: 15 targeted cases including all three fixes and the earlier ones still classify correctly; still 22 offenders on unpatched main; green on this branch and on the tree merged with latest main. * Handle positional read_text encodings, lazy generators and nested helpers * Guard reads reached from test bodies, unbound Path calls and __file__ paths * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Follow derived paths, skip lazy generator helpers, cover compressed openers * Guard the CLI tests, helper parameters and unbound Path arguments * Discover test roots and follow literal, in-place and tuple-derived paths * Identify module openers by import, unwrap starred paths, pin subprocess snippets * Resolve import origins, seed helper locals, follow named generators and parametrize * Scope imports lexically, list tracked test files, bind unpacked names * Resolve aliased openers, keyword-only params, destructured targets, next() * Pin the encoding on subprocess snippets, workflow lint and CLI output for PR #7438 * Harden the CLI encoding guard against detached streams for PR #7438 * Tighten the encoding guard's path and scope analysis for PR #7438 * Resolve path provenance more precisely and keep POSIX stream encodings for PR #7438 * Resolve qualified path classes and scope conditional imports for PR #7438 * Scope CLI stream setup to the entry point and align two encoding pairs for PR #7438 --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: danielhanchen <danielhanchen@gmail.com> |