mirror of
https://github.com/unslothai/unsloth.git
synced 2026-08-21 23:03:58 +00:00
7 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Daniel Han
|
827d25931b
|
Stop 19 test files racing on one PowerShell startup cache (#9371)
* Stop 19 test files racing on one PowerShell startup cache
Backend CI run 32341628757 on `
|
||
|
Daniel Han
|
6c124b6dd8
|
Run the repo CPU test suite on all four runner cores (#9019)
* Fix a 500 on dictation Unload before any backend is resident
stt_unload passes expected_model positionally:
_, unload_stt = _stt_lifecycle()
failed = await asyncio.to_thread(unload_stt, engines, model)
_stt_lifecycle() returns the orchestrator's unload_stt_model when a backend is
resident and stt_registry.unload when one is not. Only the first takes
expected_model positionally; on the registry it sits behind a `*`:
def unload(engines = None, *, wait = True, expected_model = None)
def unload_stt_model(self, engines = None, expected_model = None)
So with nothing loaded yet, which is what a fresh process is, Unload raises
TypeError and the route answers 500. Reproduced outside pytest:
peek_inference_backend() fresh process: None
stt_unload -> TypeError: unload() takes from 0 to 1 positional arguments
but 2 were given
Pass it by keyword, which both callables accept.
Found while profiling CI, not by the tests, and the tests are why: this file's
two unload tests passed only because an earlier test in the full suite had left
a backend resident. Standalone they failed, and in their own file they failed;
they went green only inside the full serial run. That order dependence hid a
live bug. The new test drives the no-backend path directly, so neither the
signature nor the call site can drift back.
before: 2 failed, 53 passed (file alone)
after: 56 passed
* Run the repo CPU test suite on all four runner cores
Backend CI is the most expensive workflow in the repo at 126.7 runner-minutes
per push. Install is cached at ~1.2 minutes, so almost all of it is pytest.
ubuntu-latest has 4 vCPUs and this suite is CPU-bound and GPU-free, so it was
running on one core of four.
On a real runner the step goes 17.75 minutes to 7.67, and the failure set is
unchanged: the same single pre-existing failure, serial and parallel, on the
same tree in the same staging repo. Locally, on the dependency shape the
workflow installs, 806.1s to 219.7s with an identical 2 failed / 8066 passed /
178 skipped / 40 subtests result.
The three small pytest steps in this job stay serial: the hardware-spoof step
exists because those files mutate hardware.py module globals, and all three
already measure under 0.1 minutes.
The four-interpreter matrix is deliberately left alone. It is the bigger prize
at ~79 runner-minutes, but measured under -n 4 on a real runner it fails tests
serial does not, in more than one way and not the same way twice, so it needs
its order dependencies found and fixed first rather than a flag.
* Pin the two unload signatures against the route's one call site
The bug fixed in the previous commit exists because _stt_lifecycle returns two
different callables and the route has a single call site:
def unload(engines = None, *, wait = True, expected_model = None) # stt_registry
def unload_stt_model(self, engines = None, expected_model = None) # orchestrator
A call that suits one is a TypeError on the other, and which one runs depends on
whether a backend happens to be resident, so the broken half only appears on a
fresh process. That will recur the next time either signature is edited.
This binds BOTH real signatures against the arguments the route actually passes.
It does not demand they be identical, only that one call site can serve both.
Checked against two mutations:
registry grows a keyword-only param -> passes (benign, and it should)
registry drops expected_model -> fails, "got an unexpected keyword
argument 'expected_model'"
Paired with the test beside it: that one covers the call site (route -> registry
on a cold process), this one covers the two callees staying compatible.
57 passed.
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* Give each node harness run its own script file
Reported on this PR, and reproduced before changing anything. Every _run in
test_chat_preset_builtin_invariants.py wrote the same TEMP/run.mts and then
executed "node run.mts" from TEMP. Under pytest-xdist's default load
distribution two of this file's nine tests can land on different workers, so one
worker executes the script the other just wrote over the top of it.
Pinned to four cores, running the file at -n 4:
before: 5 failed / 6 failed / 5 failed / 5 failed / 6 failed (of 9)
after: 9 passed, six runs out of six
Serial is unchanged at 9 passed either way, which is why the full-suite runs
that back this PR did not show it: at ~8000 tests across four workers these nine
rarely collide, so it is a latent flake rather than a reliable failure. That is
worse, not better, and enabling -n 4 is what would have started rolling the dice.
A unique NAME rather than a per-call directory, which is what
tests/studio/_node_harness.py::run_harness does for every other node harness
here. Those scripts reach the frontend sources by a relative path counted from
TEMP, so an extra directory level breaks every import with ERR_MODULE_NOT_FOUND
(tried it: 9 failed). register.mjs and loader.mjs stay shared because their
contents are fixed, so a concurrent rewrite writes identical bytes.
Swept the other five fixed TEMP roots under tests/studio: all of them already
allocate per call through tempfile.mkdtemp, directly or via run_harness. This
file was the only one rolling its own.
* Shorten the harness comment
* Stop two more shared-state races the parallel repo job would expose
Both reported on this PR, both real, both confirmed by reading the code.
1. tests/studio/test_chat_preset_builtin_invariants.py still rewrote the shared
register.mjs and loader.mjs on every _run. Only run.mts became unique in the
previous commit. write_text truncates before it writes, so a worker rewriting
one of those while another worker's node process is importing it can hand that
process an empty or partial module. The contents are constant, which is why I
wrongly called a concurrent rewrite harmless: identical bytes still arrive
after a truncation. They are now written through a temp file and os.replace,
so every reader sees one whole version.
2. tests/python/test_no_torch_filtering.py::TestRealRequirementsFiltering had an
autouse fixture that snapshotted the requirements directory and, at teardown,
deleted every filtered file that had appeared since. Those files land in the
REAL requirements directory, not tmp_path, so under xdist one test's teardown
removes a file another worker is still reading. It now records the paths it
creates and removes only those.
test_chat_preset_builtin_invariants.py: 9 passed serial, 9 passed on four -n 4 runs
test_no_torch_filtering.py: 65 passed serial, 65 passed on four -n 4 runs
no filtered files left behind
* Run the event-loop latency tests outside the parallel invocation
Reported on this PR. tests/studio/load_freeze asserts upper bounds on real
elapsed time -- a /health burst under 250 ms while a 600 ms blocking probe runs,
and a 100-request burst under 350 ms -- and those bounds ARE the contract, so
they cannot be loosened without the tests ceasing to test anything. A pytest
worker descheduled by the other three inflates them.
Measured before changing anything, under 3x CPU oversubscription (8 spinners
pinned to the same 4 cores): 8 ms against the 250 ms bound, 38 ms against the
350 ms one, 3 runs, no failures. So the margin is wide on this box. It is not
wide enough to leave alone: a runner core is several times slower, which puts
the 350 ms bound within reach of an unlucky schedule. The directory is now
ignored from the -n 4 run and runs in its own serial step, 20s against the
~10 minutes -n 4 saves on this job.
The ignore and the step are two edits held together by nothing, and losing the
step is silent -- the job stays green while 23 tests stop running. New guard
tests/studio/test_backend_ci_parallel_isolation.py fails if an isolated path is
missing from the parallel run's ignores, or ignored with no serial step running
it, for load_freeze and for the three hardware-spoof files that were already in
this shape. Deleting either half of the load_freeze change fails it; a third
test pins the command scan so it cannot pass by matching nothing.
load_freeze: 23 passed in 20.14s serial
new guard: 9 passed, and 1 failed under each of the two mutations
test_ci_shell_suite_coverage.py + test_xpu_spoof_pipeline.py: 58 passed
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* Raise the matrix hang limit past the suite it was cancelling
Not a review item; found while triaging why every PR in this stack carries a red
check. The Backend CI matrix limit was 30 minutes, set when the step measured
14.1 to 15.0. It now measures 22.5 to 29.2, and it is cancelling: across the six
open PRs on this stack, five had exactly one leg cancelled at 30m9s while its
siblings passed at 25 to 29 minutes. Which leg loses is luck, so the red says
nothing about the PR, and a cancelled leg burns the full 30 minutes for no
signal.
45, which is what the Windows UI job already uses. This is a hang guard, not a
budget, and it costs nothing until a job needs it. The real fix is running this
matrix on all four cores, worth ~79 runner-minutes a push, and it stays blocked
on the order dependencies this stack is working through one at a time.
* Give each PowerShell probe its own script path
Reported on this PR, real, and reproduced directly. _run_capturing_bytes wrote to
tests/python/_{stem}_probe_{shape}.ps1, a name several tests share, so under
xdist one case unlinks the script another has written before that one's pwsh
child opens it. pwsh is installed on ubuntu-latest, so these do not skip there.
Reverting to the shared name and running -n 4 four times: 1 failed each time.
With a uuid in the name:
35 passed, 8 skipped serial
35 passed, 8 skipped on each of four -n 4 runs
no probe scripts left behind
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* Stop the pin scanner reading a generated filtered file as a second pin source
Reported on this PR and reproduced. test_no_torch_filtering.py exercises
install_python_stack._filter_requirements, which writes
`.{stem}-filtered-XXXX.txt` BESIDE the source on purpose, so relative -r/-c
includes still resolve. test_diffusers_pin.py::test_only_the_pin_file_names_diffusers
scans REQ_ROOT.rglob("*.txt"), so under xdist it can see one worker's copy of
diffusers-pin.txt and report it as a second source of the pin.
Calling _filter_requirements on diffusers-pin.txt and then running that test
fails exactly as described. It is not only an xdist problem: the function passes
delete=False, so the file also survives a real install on any machine.
The scan now skips dotfiles. That matches what it means -- no CHECKED-IN
requirements file other than the pin may name diffusers -- rather than narrowing
it: a real second source still fails it (verified with a probe file), and the
production write path is untouched, since writing beside the source is the
behaviour that makes includes resolve.
5 passed with a filtered file present, 1 failed with a real second source
* Exempt only the generated filter files, not every dotfile
Reported on this PR and right: a checked-in hidden requirements file such as
.constraints.txt is a real place the pin could be overridden from, and the
blanket dotfile skip took it out of the scan.
Matched by the shape _filter_requirements actually writes now: a dot, the source
stem, "-filtered-", and NamedTemporaryFile's 8-character suffix.
generated filtered file present: 5 passed
checked-in .constraints.txt naming diffusers: 1 failed
---------
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
|
||
|
Daniel Han
|
9c69529705
|
Windows: stop compiling C# for colour on hosts that already render it (#8767)
* Windows: stop compiling C# for colour on hosts that already render it Enable-StudioVirtualTerminal is called unconditionally by install.ps1 and studio/setup.ps1, and it reaches Add-Type, which runs the C# compiler and drops a source file in %TEMP% on every install. An ANY.RUN submission of the shipped 0.1.701-beta Windows build captured that as two csc.exe processes and a "Suspicious source code drop". Under Windows Terminal there is nothing to enable: it always renders VT. Ask for that case first and skip the compile. All three conjuncts are load-bearing. WT_SESSION is inherited, so the desktop app's console-less spawn carries it into a pipe, and without the redirect check the Studio log panel would fill with escape sequences. $Host.UI.SupportsVirtualTerminal reports what the host CAN render, not whether this output buffer has ENABLE_VIRTUAL_TERMINAL_PROCESSING set, so it cannot carry the decision alone either. Nothing else moves. Outside this one function both scripts are identical to main line for line, and $script:StudioVtOk is the only value the function feeds, so the same verdict means the same bytes. The other compile stays. UnslothStudioFinalPathV2 feeds Get-StudioRuntimePathHash, which Python derives the same mutex name from byte for byte, so a managed fast path differing on case or an 8.3 name would let two installers each believe they hold the install lock. Guards: test_installer_av_shapes.py fails if the compile moves back ahead of the host check or loses a conjunct, and test_windows_setup_output_encoding.py runs this function beside the one it replaces on a real Windows host, with WT_SESSION forced set and forced empty, asserting the same verdict and the same banner bytes. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Decide the redirected case without the compiler, not the Windows Terminal one Review caught that the WT_SESSION test was unsound. WT_SESSION is inherited, so a run launched from Windows Terminal into a NEW legacy console, which is what an elevated install gets, carries it with stdout not redirected and a buffer that has no ENABLE_VIRTUAL_TERMINAL_PROCESSING. SupportsVirtualTerminal reports host capability rather than the state of that buffer, so the branch would have claimed VT and printed literal escape sequences. There is no sound way to learn the current buffer's mode without GetConsoleMode, which is the compile. So decide the other direction instead: a redirected stdout is not a console, GetConsoleMode fails on a non-console handle, and the compiled path could then only return $false. Return it directly. This is provably identical rather than probably identical, and it covers the case that was actually measured: install.rs spawns install.ps1 with a pipe, so the desktop install is exactly where the compile was happening. Also drops the env plumbing from _run_console_less. It is lru_cached, so a dict argument would have raised TypeError before PowerShell was ever spawned, and the Windows parity job would have failed rather than proving anything. The parity case no longer needs it: the console-less probe IS the redirected case, so the early return is the branch under test rather than a bystander. * Reconstruct the exact merge-base function in the VT parity test The regex stripped only the guard and left the four comments above it behind, so the reconstructed predecessor was merge-base code plus comments rather than the merge-base function. Comments do not execute, so the comparison was still measuring the right thing, but a test that says it compares against the real predecessor should do that. Verified both files now reconstruct byte for byte. Also drops a stale WT_SESSION reference from an assertion message, left over from the design this PR replaced. * Tighten the comments this PR adds --------- Co-authored-by: danielhanchen <danielhanchen@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> |
||
|
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
|
||
Wasim Yousef Said
|
f567ae8f39
|
Studio: skip redundant packaged frontend rebuilds (#8326)
* Desktop: skip frontend rebuild during updates * Tests: tolerate rustfmt in updater UTF-8 contract * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Studio: use packaged frontend for PyPI installs * Studio: keep the packaged frontend skip off source checkouts STUDIO_LOCAL_INSTALL records where the Python package came from, not which tree setup runs out of. An editable overlay separates the two: with UNSLOTH_CI_SOURCE_OVERLAY, or in a venv left editable by an earlier --local run, the mode stays 0 while SCRIPT_DIR is a checkout whose dist is a stale build artifact rather than a release one. The skip then serves that stale dist and a source change silently never reaches the browser, which is the outcome the overlay legs of clean-machine-install-ci exist to catch. A wheel ships no top-level files, so a pyproject.toml next to studio/ marks the tree as source. Require its absence before trusting the packaged dist; site-packages installs are unaffected and still skip. Also check the Tauri branch before the packaged one in setup.ps1 so a desktop update reports the same reason it reports on POSIX. Covered by new cases in tests/sh/test_packaged_frontend_skip.sh and tests/studio/test_node_decision.ps1. --------- 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
|
07df95079e
|
Studio: route every Windows installer line through the UTF-8 stdout sink (#8148)
* Studio: route every Windows setup line through the UTF-8 stdout sink The desktop setup log rendered "?? Unsloth Studio Setup" over a rule of replacement characters. Tauri spawns Windows PowerShell 5.1 with CREATE_NO_WINDOW (install.rs), so the [Console]::OutputEncoding setter throws and both entry scripts rebind [Console]::Out to a UTF-8 writer. step/substep already write only through that writer when stdout is redirected, so they came out right. Every other line did not: Write-Host is written by 5.1's console host with its own writer on the OEM code page, and U+1F9A5 has no OEM form while U+2500 becomes a bare 0xC4, which from_utf8_lossy turns into U+FFFD. The banner and the footer are not steps, so they kept arriving as mojibake, and install.ps1 had neither the IsOutputRedirected probe nor a mirror at all. Add Write-StudioLine above the first write in studio/setup.ps1 and install.ps1: console handle when redirected, Write-Host when interactive, since it is the only writer that colorizes. Rewrite 164 call sites in setup.ps1 and 155 in install.ps1 onto it, including install.ps1's own step/substep. Write-Host now survives only inside helpers that have already ruled out the redirected sink, and the launcher script install.ps1 generates keeps its own, since it runs as a separate process. No behaviour change for an interactive console user: same text, same colors, same single record per line. test_windows_setup_output_encoding.py gains byte-level coverage that the real banner and footer, sliced out of setup.ps1, survive both launch shapes as valid UTF-8 exactly once, plus a source contract that runs on Linux and names any file:line that reaches for Write-Host outside the allow-list. Studio.Setup.Output.Tests.ps1 covers Write-StudioLine in both modes and pins install.ps1's copy to setup.ps1's. Harnesses that splice these scripts apart now stub or dot-source Write-StudioLine: two PowerShell harnesses, one Python harness, and the VC++ redist leg of studio-windows-inference-smoke. pytest tests/python tests/test_installer_*.py: 1077 passed (2 pre-existing sandbox failures unrelated to this change). All 16 tests/studio harnesses and 57 Pester cases pass. Both scripts parse clean. * CI: spawn install.ps1 as a child process so its lines reach install.log * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Stub the output sink in the llama.cpp backend PowerShell harness * Guard the console-less spawn on a Windows runner The byte-level cases in this file run with a console attached, and a GitHub runner gives a CREATE_NO_WINDOW child one, so the UTF-8 setter succeeds there and every version of these scripts emits a clean banner. Those cases cannot tell this fix from what preceded it. Add cases that call FreeConsole() in the child first, which is the state install.rs's own comment assumes CREATE_NO_WINDOW produces. There Write-Host has no screen buffer to query, throws, and takes the script down: 2 bytes of stdout and exit 1 rather than the banner. The probe is assembled entirely out of text sliced from the script under test and spawned with install.rs's own interpreter, flags and creation flags. No Windows job ran this file, so its byte-level half was only ever exercised under pwsh 7 on the Linux Backend CI leg, which is UTF-8 by default. Add it to the cross-platform parity matrix, which already has a windows-latest row and already triggers on install.ps1 and studio/setup.ps1. * Report skips in the parity step A platform-gated case that stopped running on the row it exists for still reports green with -q alone. * Slice the error preference too It is what turns the Write-Host throw into a dead script rather than a skipped line, so restating it would be assuming the result. * Say what the comments actually mean * Make the console-less cases fail on a lost banner, not just a mangled one * Stub the output sink in every harness that splices these scripts The Write-Host rewrite left four spliced-source harnesses reaching Write-StudioLine without defining it. An undefined command is a terminating error, so each one either aborted or was swallowed by the harness's own catch, and the test kept passing while no longer testing anything. - test_windows_python_venv_hardening.py, partial-rollback case: the five-line split-move warning was lost. The assertion that "both halves are named" only stayed green because $existing is a prefix of the rollback dir, so it matched the dir= line instead. Pin it to the warning text. - test_path_probe_access_denied.ps1, ownership guard: the catch scored the command-not-found as the intended failure and never reached Exit-SetupFailure. Pin the check to the EXIT-SETUP message. - test_windows_installer_concurrency_guard.py: the decision block prints before Exit-InstallFailure, so on Windows the active case aborted at exit 1 and never produced RESULT:blocked. - Studio.Setup.Vs2026.Tests.ps1: on a host without cmake, Ensure-BuildToolsForLlamaSourceBuild hits the sink first and the no-op case fails on the throw. Also stub the three remaining harnesses that splice sink-calling helpers but do not reach the sink on the paths they exercise today, so the next case added to them cannot reintroduce this. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> |
||
|
Daniel Han
|
d8effae0d5
|
Studio: fix the Windows desktop setup log mojibake and double-printed steps (#8083)
* Studio: fix the Windows desktop setup log mojibake and double-printed steps
The desktop 'Getting things ready...' log rendered as:
?? Unsloth Studio Setup
<52 replacement chars>
gpu
none (chat-only / GGUF)
gpu none (chat-only / GGUF)
Encoding. studio/setup.ps1 never set [Console]::OutputEncoding, so Windows
PowerShell 5.1 encoded redirected output with the OEM code page while the
desktop app decodes the pipe as UTF-8 (String::from_utf8_lossy in
src-tauri/src/install.rs). That corrupts two different ways: the sloth U+1F9A5
has no OEM representation so PowerShell substitutes one '?' per UTF-16
surrogate, and the rule U+2500 does have one, so it becomes a bare 0xC4 byte
that is invalid UTF-8 and surfaces as U+FFFD. Both entry scripts now set the
console encoding, $OutputEncoding, PYTHONUTF8 and PYTHONIOENCODING before the
first write, and Refresh-Environment can no longer reload the two Python vars
back over ours mid-run. The patch is ASCII-only: these files are UTF-8 without
a BOM and 5.1 parses those as ANSI.
Duplication. step/substep wrote through Write-Host AND a console-handle mirror.
The mirror's comment assumed Write-Host does not survive the process chain; it
does, because the CLI spawns setup.ps1 as -Command "& '...' *>&1"
(unsloth_cli/commands/studio.py), which merges the Information stream into
stdout deliberately. The sink is now resolved once and exactly one is used:
redirected writes to the console handle, interactive writes to Write-Host.
Splitting. step composed one logical line from two Write-Host calls using
-NoNewline, and a redirected consumer turns each Information record boundary
into a line break. Both scripts now emit one composed record; install.ps1 needs
this most, having no mirror to fall back on.
Rust children on Windows get PYTHONUTF8/PYTHONIOENCODING too, since install.rs,
update.rs and process.rs all decode their output as UTF-8. The readers stay
lossy on purpose -- strict decoding would turn display corruption into an
installation failure.
Tests: a Pester suite auto-discovered by the existing pester job, and a pytest
byte-level probe that runs real PowerShell in both the -File and -Command
launch shapes and asserts on raw bytes. Verified to fail against the unfixed
tree (9 Pester and 13 pytest failures) rather than merely passing.
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* Bind a UTF-8 writer with no console, and pass -X utf8 to the isolated child
Two holes in the previous commit, both on the exact path the desktop app takes.
[Console]::OutputEncoding P/Invokes SetConsoleOutputCP, which needs a console
handle. Under CREATE_NO_WINDOW there is none, so it throws, and it drops the
cached writer BEFORE throwing while assigning OutputEncoding only after. Console
.Out therefore rebuilt on the old code page. Swallowing the exception was not
enough once redirected step/substep use Console.Out as their only sink, so the
catch path now binds an explicit UTF-8 StreamWriter over OpenStandardOutput.
build_update_command launches Python with -I, which implies -E, so that process
ignores every PYTHON* variable and PYTHONUTF8/PYTHONIOENCODING never reached it.
Pass -X utf8 as a switch instead. The env vars stay for its descendants.
https://docs.python.org/3/using/cmdline.html#cmdoption-I
* Bind the UTF-8 writer to stderr as well when there is no console
The no-console fallback repaired Console.Out only. Tauri pipes stderr through
the same lossy UTF-8 decode (install.rs) and emits it to the same UI log, and
InstallFailureContext builds the user-facing failure message from those lines,
so a PowerShell error carrying a non-ASCII path still arrived as U+FFFD.
install.ps1 also writes its Clear-TauriInstallError markers there.
* Tighten the comments added by this PR
Comments only, no code change. Verified with the PowerShell AST tokenizer for
both .ps1 files and the Pester suite (token streams identical with Comment and
NewLine excluded), comment_tools.py for the Python test, and a code-only diff
for update.rs.
* Update the Windows command assertion for the added UTF-8 flags
windows_update_command_uses_python_not_replaceable_console_stub asserts the
exact argument vector, so adding -X utf8 broke it. The Windows cargo test job
in studio-tauri-smoke.yml runs it; the Linux job skips it under cfg(windows),
and cargo check type-checks tests without running them, so neither the org
Linux run nor the staging check caught it.
---------
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
|