[staging CI] unslothai/unsloth#7757 - #672
Closed
danielhanchen wants to merge 6 commits into
Closed
Conversation
…le install tree Follow-up to unslothai#7735, which routed the prebuilt llama.cpp probes through three-state path probing but left two gaps. Phase 4 read $LlamaServerBin with a bare Test-Path under "Stop". A forced compile, a pinned PR or a custom llama source skips Phase 3.4 entirely, so on those routes this was the first probe inside the tree and a denied build\ aborted with the raw "Test-Path : Access is denied" the merged PR set out to remove. It now probes three-state, and the CMakeCache.txt read below it is guarded too: a listed file can still deny the read, which the probe cannot see. The probe is skipped for a linked UNSLOTH_LOCAL_LLAMA_CPP_DIR, where it would read through the junction into the user's own checkout, and the denial reports -OwnershipUnverified under a custom home, where nothing on this route has proven the tree is ours. The whisper.cpp phase promises failure is never fatal, but under a custom UNSLOTH_STUDIO_HOME an unreadable tree exited the whole run, taking llama.cpp inference down with it. Assert-StudioOwnedOrAbsent gains a -NonFatal mode that hands the denial back instead; an unowned tree still stops. The check stays behind the installer-exists gate it used to sit inside, so a tree without install_whisper_prebuilt.py remains the no-op it was. Backend: _is_runnable let Path.is_file() propagate EACCES. Now that setup leaves a denied whisper.cpp in place, that turned into a 500 out of /api/inference/audio/stt/status, the one endpoint reporting both dictation engines, so the setup message promising Transformers dictation still works was not true. It reads as engine-unavailable instead.
Mutation testing found six ways to reintroduce the bugs this branch fixes while the tests stayed green. Assert-StudioOwnedOrAbsent: the -NonFatal returns were counted, not ordered. Moving one below its Exit-PathAccessDenied makes it dead code and the whisper phase fatal again; hoisting one above the custom-home gate reports a fresh install as unreadable. Each return is now pinned immediately above the exit it pre-empts, with no unpaired return allowed. The whisper denial branch had no assertion scoped to its own body. Both phrases it was checked for already occur elsewhere in the phase, so the branch could be turned back into an Exit-SetupFailure and stay green. The branch is now sliced out and checked for step/Yellow, both phrases, and the absence of any exit. The installer gate was checked for presence, not for being a conjunct, so -or-joining or negating it reopened the installer-less tree the test is named for. The denial subject was unpinned, so it could name llama-server.exe and tell the user to move aside one file instead of the tree. Slice terminators are now asserted through one helper: an unasserted terminator does not fail, it silently widens the window to end-of-file and makes everything inside it near-vacuous. The whisper binary probe test gated its only behavioural case on geteuid() == 0, which silently drops it in any root container. It probes for a real denial instead. Two pre-existing exact counts in the ownership guard tests become floors: this branch consumed the last of their headroom, so the next legitimate route added there would break two tests that say nothing about it.
for more information, see https://pre-commit.ci
The control probed a marker file that did not exist. Windows reports a missing child of a denied directory as absent rather than throwing, so the control read as "this host cannot deny" and failed the suite on windows-latest while passing under chmod on Linux. It now probes a file that exists inside the locked tree, matching the control the suite already uses. That difference also splits the routes by platform for a tree with no ownership marker, which is the fresh custom-home case: Linux catches it on the marker probe, Windows has to catch it on the adoptable-state read. Added a case that accepts either route and rejects anything but Denied, so the Windows one is exercised for the first time.
Staging CI on windows-latest caught this. Get-StudioAdoptableState decided "denied" only from probes of two marker files inside the tree, but Windows reports a MISSING child of an unreadable directory as absent rather than throwing. A denied tree holding neither marker therefore returned "No", and Assert-StudioOwnedOrAbsent fell through to "path is not an Unsloth-owned install" and exited: the wrong cause, and fatal, on the only platform any of this runs on. It also defeated the whisper -NonFatal path, since an unowned tree is still fatal by design. Listing the directory itself distinguishes "no markers here" from "cannot look", so that is the fallback when neither probe reported a denial. A readable tree with no markers still returns "No" as before, and the catch swallows anything that is not a denial because this helper must not throw. This also corrects the message a denied custom-home llama.cpp tree produced on Windows, which reported the same wrong cause. chmod 000 blocks the child probes outright, so it never reached the new code. chmod 111 allows stat of a named child while forbidding a listing, which is exactly the Windows shape, so the test now covers both and the negative control fires only on the 111 case.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Disposable CI run for unslothai#7757. Do not merge; closed after CI.