mirror of
https://github.com/unslothai/unsloth.git
synced 2026-08-17 04:43:52 +00:00
Windows: do not abort setup on an unreadable llama.cpp install (#7735)
* Windows: do not abort setup on an unreadable llama.cpp install Test-Path raises UnauthorizedAccessException instead of returning $false when an ACL denies the probe. setup.ps1 runs under $ErrorActionPreference = "Stop", so the bare probe of UNSLOTH_PREBUILT_INFO.json in the llama.cpp prebuilt phase killed setup with a raw "Test-Path : Access is denied" and exit code 1. The desktop app had nothing but [TAURI:ERROR_DEFAULT] to fall back on, so it showed "unsloth studio setup failed (exit code 1)". ~/.unsloth/llama.cpp sits beside the app, not inside it, so reinstalling reused the unreadable folder and hit the same line again, including a reinstall to a different drive. Add Get-PathState (Present / Absent / Denied) plus Test-PathQuiet, route the probes that read inside install trees we do not own through them, and report a denied llama.cpp install through Exit-SetupFailure so the reason and the recovery steps reach the desktop UI. Reported in unsloth-test/unsloth-test#9 * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Stop on every denied path, and split the recovery commands Review follow-ups: - Assert-StudioOwnedOrAbsent treated a denied root as absent and returned, so the caller could go on to replace a tree it cannot read. Probe the root three-state and stop on Denied, still gated on $StudioHomeIsCustom so default-home behaviour is unchanged. - The source-build .git probe treated a denied checkout as "no checkout" and cloned a replacement. The swap that follows recursively removes the original and moves the temp tree over it under "Continue" and unchecked, so a denied child could leave a half-deleted install. Stop instead. This path already treated denied as absent before the previous commit (that probe runs under "Continue", so it printed an error and took the false branch), so the hazard is older than this branch, but it is in scope for the same reason. - Probe $LlamaCppDir itself three-state, so an unreadable parent is reported rather than dying on the bare probe under "Stop". - takeown and icacls were printed joined by "then", which is not a PowerShell separator: takeown would swallow the rest as arguments and icacls would never run. Print them on separate lines. Fold the repeated guidance into Exit-PathAccessDenied so all five denial routes report the same thing. * Harden the denial reporting path, found by simulation Ran the real decision blocks against simulated filesystems (denied file, denied parent, traverse-only and list-only dirs, symlinks, dangling links, wildcard and unicode paths, 3000 random paths) plus PSScriptAnalyzer's 5.1/6.2/7.0 syntax check. Two things came out of it: - Get-PathDenialDetail threw a parameter-binding exception on an empty path. It runs while a failure is being reported, so it would have replaced the actionable message with a raw binding error at exactly the wrong moment. Null and empty are now accepted and return no detail. - The link-target lookup used an empty catch, which PSScriptAnalyzer flags and which hid the intent. It assigns $null explicitly now. Both are covered by new checks. Also promoted the strongest invariant from the simulation into the suite: Get-PathState must agree with a bare Test-Path on every probe that did not throw, and Denied may only appear where the old probe threw, so no path that worked before can take a different branch now. Verified: PSUseCompatibleSyntax reports nothing for 5.1, 6.2 and 7.0; the Python contract tests pass on 3.10 through 3.13 in separate uv venvs; the tauri install:: unit tests pass (17), which is the code that prefers the [TAURI:ERROR] line over the generic exit-code message. * Trigger the Windows PowerShell tests when they change studio-windows-inference-smoke.yml runs six PowerShell unit tests out of tests/studio, but its pull_request paths filter matched none of them, and no other workflow runs them. A PR touching only one of those tests never ran it. Five predate this branch; the sixth is the ACL test added here. Scope the filter to tests/studio/*.ps1 rather than tests/studio/**, so a python-only change under that directory does not pull in the GGUF smoke jobs. This matches what the other two workflows already do: parity-ci lists its .ps1 test outright and update-smoke uses a scoped glob. Guard it in test_ci_shell_suite_coverage.py, which exists for this exact failure (tests/sh had the same hole): every tests/*.ps1 a workflow invokes must be matched by that workflow's paths filter, and must exist. The GitHub glob matcher it needs has its own table-driven test, since a wrong matcher would make the guard pass on everything. Verified by reverting the one-line filter change: the guard then names all six unrun tests. * Make the Windows PowerShell test step fail when a test fails Verifying the path-filter fix turned up a second hole in the same step. A `shell: pwsh` step inherits only the LAST command's exit code, and this step ran five tests as five bare commands, so only the last one could fail the build. test_resolve_cuda_toolkit.ps1 has been printing FAIL exits non-zero (scenario 2, forced source build) FAIL exits non-zero (scenario 6, no toolkit, forced) 2 check(s) FAILED on every Windows run, exiting 1, and the job reported success. Confirmed on main (run 30723608191, shac67410a7), so it predates this branch, and it reproduces locally. The cause is in the test, not the installer. Resolve-CudaToolkit -RequireOrExit leaves through Exit-SetupFailure, which the child harness never stubbed, so under ErrorActionPreference=Continue the call was an ignored command-not-found, the child fell through and exited 0. The harness already injects the real Resolve-CudaToolkit and Write-CudaDriverToolkitMismatch by AST, so inject the real Exit-SetupFailure the same way. That test now passes 25/25. With it green, add the exit-code checks after each invocation, matching what studio-windows-update-smoke.yml already does, and guard the pattern in test_ci_shell_suite_coverage.py: any step running more than one PowerShell test must check $LASTEXITCODE after each. Verified by reverting each piece: dropping one check makes the guard name that test, and dropping the Exit-SetupFailure injection brings both scenario failures straight back. * Stop on a denied --with-llama-cpp-dir instead of reinstalling over it When UNSLOTH_LOCAL_LLAMA_CPP_DIR points at the canonical $LlamaCppDir and the llama-server.exe there is ACL-denied, Test-PathQuiet collapsed Denied to $false, $LocalLlamaServerFound stayed false, and the canonical branch reported "nothing built there yet; running the normal install". The prebuilt installer then moves that tree aside and replaces it, which is exactly what the branch's own comment says it exists to prevent. The old bare probe stopped first, by throwing under "Stop". Probe the candidates three-state and stop on Denied. Same for the directory probe itself, which reported an unreadable dir as "does not exist" and sent the user after the wrong problem. The generic message could not be reused as-is here: it tells the user to delete the folder because Unsloth reinstalls it, which is true of the managed cache and wrong for a build they pointed us at. Exit-PathAccessDenied takes -UserSupplied, which swaps that advice for restoring access or repointing UNSLOTH_LOCAL_LLAMA_CPP_DIR, and keeps the takeown/icacls lines. Verified by driving the real block through every state: a readable build is still reused, a genuinely empty canonical dir still falls through to the normal install, a missing dir still reports "does not exist", and a denied build now stops with exit 1 instead of being replaced. Reverting the probe puts the fall-through back, and the user-supplied path never prints "delete or rename" or "managed cache". * Carry the denial through three more probes Three review points, all reproduced before fixing: - The canonical --with-llama-cpp-dir override still got the managed advice ("delete it, Unsloth reinstalls it"). The override means the user asked to reuse whatever is in that tree, so deleting it is wrong wherever it sits. Both candidate denials now pass -UserSupplied, which collapses the branch to one call. - Phase 1b's git prerequisite scan probes the same candidate binaries with a bare Test-Path under "Stop", thousands of lines before the Phase 4 guards, so a denied override terminated the run with the raw error this change exists to replace. Reproduced, then guarded. - Test-StudioOwnedAdoptable collapsed a denied prebuilt marker to $false, so Assert-StudioOwnedOrAbsent called an Unsloth tree an unrelated directory and told the user to move it aside. Get-StudioAdoptableState returns Yes/No/Denied and the guard reports the denial first; Test-StudioOwnedAdoptable stays as the boolean view for the cosmetic cleanup gate. A denied file under a readable directory is a Windows-ACL-only state: POSIX keeps a mode-000 file stat-able, and a symlink into a denied directory still answers Test-Path. The local run injects that one state at the lowest seam and lets the real functions run; the Windows leg of test_path_probe_access_denied.ps1 builds it for real with icacls and skips elsewhere with the reason. test_setup_ps1_adopts_existing_whisper_prebuilt_marker sliced between two function names and the marker scan moved, so its anchor now points at Get-StudioAdoptableState. Its assertion is unchanged. Reverting each fix individually puts the original behaviour back: the ownership misdiagnosis, the raw "Access to the path ... is denied" from Phase 1b, and the delete-your-own-build advice. * Close the remaining denial gaps and the stale probe anchors for PR #7735 - tests/sh/test_with_llama_cpp_dir_flag.sh anchored the literal 'if ($ResolvedLocal -eq $LlamaCppDir) {', which2a61343hoisted into $LocalIsCanonical. Re-pin it to the comparison, not the branch. - The junction path deleted and replaced $LlamaCppDir behind a bare Test-Path, so a denied destination still threw raw under a default home. Probe it three-state, and treat Denied as surviving removal. - Get-Content on the prebuilt metadata still globbed while the probes gating it went literal, so a path holding [ or ] passed the probe and threw into the catch. Make both reads literal, with a test. - Get-PathDenialDetail could throw on a non-filesystem provider item whose .Attributes has no -band overload, replacing the failure being reported. - Win32Exception keeps E_FAIL in HResult and the code in NativeErrorCode, so the HRESULT check never matched it. Fix the comment and the check. - test_windows_git_gate.py ran the layout scan in a child that never defined Get-PathState, sode486ffhad it silently report nothing built. Inject the real helpers, as test_resolve_cuda_toolkit.ps1 does. - Relax the exact Exit-PathAccessDenied count to a floor and key the -UserSupplied rule on the path reported rather than on position. * Stop advising deletion of a tree whose ownership cannot be read The ownership guard stops precisely because it could not read the marker, so it cannot claim the folder is ours either. It was still emitting the managed-cache text, telling the user to delete it. Eleven lines below, the readable-but-unowned branch says 'move it aside' instead, so we were being gentler when we could prove the tree was not ours than when we could not read it at all. New -OwnershipUnverified wording for those three stops. Also from re-reading the previous commit: - The reparse-point unlink above the junction path ran before the new three-state probe, and a link reports Present, so the probe could not cover it. A denied unlink still terminated on the raw .Delete() throw. - That junction destination probe had no test at all; reverting it left the suite green, since the count floor cannot see a swap. Pinned by name like every other route. - The re-pinned shell anchor accepted an assignment that nothing consumed. Pin the branch that uses it too. - Get-PathDenialDetail now checks the item type rather than the attribute type, which also covers a provider item with no Attributes at all. * Check both destructive steps of the temp-dir swap The guard above the clone path probes only .git, but its own comment names any unreadable child as the risk. A forced source build over a non-git install with a denied child elsewhere reads Absent, clones into a temp dir, and reaches the swap. Both steps there are non-terminating under Continue and neither was checked. Reproduced: Remove-Item partially fails, the original survives, and Move-Item then moves the temp dir INSIDE it, so the new binary lands at llama.cpp/llama.cpp.build.<pid>/llama-server.exe while $LlamaServerBin points at llama.cpp/build/bin/Release. Setup carries on reporting success with no usable server and a half-deleted install. Check the removal before moving, so the stop happens while the temp build is still whole, and check the move afterwards. Denied routes through Exit-PathAccessDenied; anything else surviving exits 3 like the other blocked-replacement paths. * [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>
This commit is contained in:
parent
a0a7fc110d
commit
ebfefcf84e
9 changed files with 937 additions and 27 deletions
|
|
@ -36,11 +36,30 @@ def _git_gate_block() -> str:
|
|||
raise AssertionError("Unclosed git gate block in setup.ps1")
|
||||
|
||||
|
||||
def _function(name: str) -> str:
|
||||
"""Inject the real helper the block calls; an undefined one is a silent no-op
|
||||
under Continue, which would let the layout scan always report 'nothing built'."""
|
||||
source = SETUP_PS1.read_text(encoding = "utf-8")
|
||||
start = source.index(f"function {name} {{")
|
||||
depth = 0
|
||||
for index in range(source.index("{", start), len(source)):
|
||||
if source[index] == "{":
|
||||
depth += 1
|
||||
elif source[index] == "}":
|
||||
depth -= 1
|
||||
if depth == 0:
|
||||
return source[start : index + 1]
|
||||
raise AssertionError(f"Unclosed function {name} in setup.ps1")
|
||||
|
||||
|
||||
def _script() -> str:
|
||||
return f"""
|
||||
$DefaultLlamaPrForce = "0"
|
||||
$DefaultLlamaSource = "https://github.com/ggml-org/llama.cpp"
|
||||
$DefaultLlamaTag = "latest"
|
||||
{_function("Test-AccessDeniedError")}
|
||||
{_function("Get-PathState")}
|
||||
function Exit-PathAccessDenied {{ param($Path, $Label, [switch]$UserSupplied) throw "denied: $Path" }}
|
||||
{_git_gate_block()}
|
||||
Write-Output $gitNeeded
|
||||
"""
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue