ceos/unsloth
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Tighten comments across the install readiness changes

Browse source
This commit is contained in:
Daniel Han 2026-07-27 17:54:28 +00:00
commit 508d3720c4
7 changed files with 65 additions and 100 deletions
Download patch file Download diff file Expand all files Collapse all files

7
studio/src-tauri/src/commands.rs
View file

@ -19,8 +19,7 @@ async fn managed_install_ready_after_repair() -> bool {
}
/// The update can install the newer CLI and only then reach a rejected
/// environment value. No reinstall changes that, so report it instead of
/// running the bundled installer over a healthy tree.
/// environment value. No reinstall changes that, so report it instead.
fn unrepairable_after_update(reason: Option<&str>) -> Option<String> {
match reason {
Some(crate::preflight::STUDIO_RUNTIME_STARTUP_FAILED) => Some(
@ -793,9 +792,7 @@ mod tests {
}
#[test]
fn rejected_settings_stop_repair_instead_of_reinstalling() {
// The update can install the newer CLI and only then reach the bad
// value, so this arm is the one that would otherwise reinstall over a
// healthy tree and fail again the same way.
// This arm is the one that would otherwise reinstall over a healthy tree.
let msg =
super::unrepairable_after_update(Some(crate::preflight::STUDIO_RUNTIME_STARTUP_FAILED))
.expect("a rejected setting must stop repair");

10
studio/src-tauri/src/desktop_backend_owner.rs
View file

@ -265,9 +265,8 @@ impl BackendOwnerState {
self.write()
}
/// Only while the file is still ours. Cleanup after a backend exits can run
/// for seconds terminating descendants, and a start in that window has
/// already written the next backend's metadata to this same path.
/// Only while the file is still ours. Cleanup after an exit can take seconds,
/// and a start in that window already wrote the next metadata to this path.
pub(crate) fn remove(self) {
if let Ok(Some(on_disk)) = read_metadata(&self.path) {
if on_disk.token_sha256 != self.metadata.token_sha256 {
@ -278,8 +277,7 @@ impl BackendOwnerState {
return;
}
}
// Unreadable counts as ours: a file nothing can parse would otherwise
// outlive every backend and fail each later ownership check.
// Unreadable counts as ours, or it would outlive every backend.
remove_metadata_file(&self.path);
}
@ -994,8 +992,6 @@ mod tests {
#[test]
fn a_replaced_owner_file_survives_the_previous_backend_cleanup() {
// Cleanup after an exit can take seconds; a start in that window has
// already written the next backend's metadata to the same path.
let path = temp_metadata_path("replaced-owner");
let exited = BackendOwnerState::from_metadata(path.clone(), metadata(1, Some(8888)));
let mut current = metadata(2, Some(8899));

24
studio/src-tauri/src/install.rs
View file

@ -83,11 +83,9 @@ fn clear_install_in_progress_marker() -> Result<(), String> {
}
/// Clear only where the script provably never touched the venv: it failed to
/// spawn, or it exited asking for elevation, which install.sh decides (:1931)
/// before it creates the venv (:2120). Once the script is running, any failure
/// can leave pip part-way through replacing a package, and the runtime probe
/// does not reach transitive dependencies, so the marker is the only signal
/// left. Never fatal: losing it costs a fast path, not correctness.
/// spawn, or it asked for elevation, which install.sh decides (:1931) before it
/// creates the venv (:2120). Later on any failure can leave pip part-way
/// through, and the marker is the only signal. Never fatal: it costs a fast path.
fn clear_install_marker_best_effort() {
if let Err(msg) = clear_install_in_progress_marker() {
warn!("[install] {}", msg);
@ -193,7 +191,7 @@ fn emit_complete(app: &AppHandle) {
// ── Spawn ──
/// A start rejected because one is already running. Distinguished from a real
/// A start rejected because one is already running, kept apart from a real
/// spawn failure so the loser of the race leaves the winner's marker alone.
const INSTALL_ALREADY_RUNNING: &str = "Installation is already running.";
@ -518,9 +516,8 @@ fn run_install_with_event_mode(
let (stdout, stderr) = match spawn_script(&script, &args, &state) {
Ok(handles) => handles,
Err(msg) => {
// Nothing ran, so nothing is half-installed. Unless another
// installer is already inside the venv, in which case the marker
// is its own and clearing it here throws away its recovery signal.
// Nothing ran, so nothing is half-installed. Unless another installer
// owns the marker, where clearing it drops its recovery signal.
if msg != INSTALL_ALREADY_RUNNING {
clear_install_marker_best_effort();
}
@ -587,8 +584,7 @@ fn run_install_with_event_mode(
let _ = app.emit(event_mode.needs_elevation_event(), &packages);
Err("NEEDS_ELEVATION".to_string())
} else {
// Keep the marker: the script ran, so pip may have replaced or
// removed packages before it failed.
// Keep the marker: the script ran, so pip may be part-way through.
let msg = format!("Installer exited with code {}", code);
diagnostics::finish_attempt(
&diagnostics,
@ -651,8 +647,7 @@ pub fn record_pending_elevation_canceled(
let Some(attempt) = attempt else {
return false;
};
// The elevation exit left the marker in place for the resumed run that is
// now not happening.
// The resumed run the elevation exit left the marker for is not happening.
clear_install_marker_best_effort();
diagnostics::finish_attempt(
diagnostics,
@ -926,8 +921,7 @@ fn finish_elevation_failure(
exit_status: Option<String>,
message: String,
) {
// Terminal, like a cancelled prompt: the run the code 2 exit left the
// marker for is not happening.
// Terminal, like a cancelled prompt: the resumed run is not happening.
clear_install_marker_best_effort();
if let Some(attempt) = attempt {
diagnostics::finish_attempt(

25
studio/src-tauri/src/preflight.rs
View file

@ -24,7 +24,7 @@ use managed::probe_managed_bin;
#[cfg(test)]
use version::{backend_version_compatible, MIN_DESKTOP_BACKEND_VERSION};
/// The managed backend refused to start on a value it inherited from the
/// The managed backend refused to start on a value inherited from the
/// environment rather than from the install.
pub(crate) const STUDIO_RUNTIME_STARTUP_FAILED: &str = "studio_runtime_startup_failed";
@ -33,7 +33,7 @@ fn release_auto_repair() -> bool {
}
/// Reinstalling cannot change a rejected environment value, so repairing over
/// one only replaces a healthy install and fails the same way afterwards.
/// one only replaces a healthy install and fails the same way.
fn stale_reason_is_repairable(reason: &str) -> bool {
reason != STUDIO_RUNTIME_STARTUP_FAILED
}
@ -404,8 +404,6 @@ mod tests {
#[test]
fn rejected_backend_settings_do_not_auto_repair() {
// No reinstall can change an inherited environment value, so repairing
// would replace a healthy install and fail again the same way.
assert!(!stale_reason_is_repairable(STUDIO_RUNTIME_STARTUP_FAILED));
for reason in [
"studio_runtime_missing_dependency",
@ -685,8 +683,7 @@ exit 1
#[tokio::test]
async fn a_hung_cli_is_stale_without_running_the_legacy_fallback() {
// An older CLI rejects the unknown command at once, so only a broken one
// reaches the timeout. Retrying it with two more 10s probes would treble
// the wait before repair on exactly the installs that need it soonest.
// reaches the timeout, and two more 10s probes would treble the wait.
let _cache_guard = MANAGED_CAPABILITY_CACHE_TEST_LOCK.lock().await;
let _cache_home = ManagedCapabilityCacheHome::new("runtime-hang");
remove_managed_capability_cache();
@ -713,9 +710,8 @@ sleep 120
#[cfg(unix)]
#[tokio::test]
async fn managed_runtime_probe_survives_more_stdout_than_the_pipe_holds() {
// The probe imports the whole backend, so import-time output can exceed
// the 64 KiB pipe buffer. Waiting on exit before draining wedges the
// child mid-write and reports a healthy install as broken.
// The probe imports the whole backend, so output can exceed the 64 KiB
// pipe buffer, and waiting before draining wedges the child mid-write.
let _cache_guard = MANAGED_CAPABILITY_CACHE_TEST_LOCK.lock().await;
let _cache_home = ManagedCapabilityCacheHome::new("runtime-noisy");
remove_managed_capability_cache();
@ -753,10 +749,9 @@ exit 1
#[tokio::test]
async fn a_cli_predating_the_runtime_check_is_stale_for_its_own_reason() {
// Every published CLI satisfies MIN_DESKTOP_BACKEND_VERSION but has no
// `desktop-runtime-check`, so click exits 2 with an empty stdout. That
// is the CLI the interrupted installs shipped with, and -h passes
// without touching the backend, so launchable is not ready. Update it
// first, and keep the reason apart from a CLI that cannot answer.
// `desktop-runtime-check`, so click exits 2 with an empty stdout. It is
// what the interrupted installs shipped with, and -h passes without
// touching the backend, so keep it apart from a CLI that cannot answer.
let _cache_guard = MANAGED_CAPABILITY_CACHE_TEST_LOCK.lock().await;
let _cache_home = ManagedCapabilityCacheHome::new("runtime-check-absent");
remove_managed_capability_cache();
@ -810,8 +805,8 @@ exit 2
let _cache_home = ManagedCapabilityCacheHome::new("cache-hit");
remove_managed_capability_cache();
// Runtime readiness is checked on every probe, while the static
// desktop-capabilities probe is skipped once the cache is warm.
// Runtime readiness runs on every probe; the capability probe is skipped
// once the cache is warm.
let fake = fake_cli(
"cap-cache-hit",
r#"#!/bin/sh

33
studio/src-tauri/src/preflight/managed.rs
View file

@ -303,21 +303,19 @@ async fn run_cli_probe(bin: &Path, args: &[&str]) -> bool {
}
}
/// The probe could not answer: the binary would not run, or it exited without
/// a payload. Distinct from a CLI that is simply too old to know the command.
/// The probe could not answer: it would not run, or exited without a payload.
/// Distinct from a CLI that is simply too old to know the command.
const RUNTIME_PROBE_FAILED: &str = "studio_runtime_probe_failed";
/// A hung binary, kept out of the fallback below: an older CLI rejects the
/// unknown command at once, so only a broken one reaches the timeout, and
/// running two more probes on it would treble the wait before repair.
/// unknown command at once, so only a broken one reaches the timeout.
const RUNTIME_PROBE_TIMEOUT: &str = "studio_runtime_probe_timeout";
/// A CLI predating the runtime probe. Repairable, and the update installs one
/// that can answer, so the next preflight checks the venv for real.
/// A CLI predating the runtime probe. Repairable: the update installs one that
/// can answer, so the next preflight checks the venv for real.
const RUNTIME_CHECK_UNSUPPORTED: &str = "studio_runtime_check_unsupported";
/// True when the CLI is older than `desktop-runtime-check` yet still launches.
/// Such a CLI exits with a usage error and no JSON, which is indistinguishable
/// from a crashed probe; `--help` resolves the command without running it, and
/// the legacy launch probe keeps a genuinely unusable binary out of this path.
/// Its usage error is indistinguishable from a crashed probe, so `--help`
/// resolves the command without running it.
async fn predates_runtime_check(bin: &Path) -> bool {
!run_cli_probe(bin, &["studio", "desktop-runtime-check", "--help"]).await
&& run_cli_probe(bin, &["-h"]).await
@ -359,9 +357,8 @@ async fn probe_cli_runtime(bin: &Path) -> Result<(), String> {
return Err(RUNTIME_PROBE_FAILED.to_string());
};
// Drain while waiting: the backend import this probe runs can print more than
// the pipe buffer holds, and waiting first deadlocks on it, timing out a
// healthy install into repair.
// Drain while waiting: the backend import can print more than the pipe holds,
// and waiting first deadlocks, timing out a healthy install into repair.
let reader = tokio::spawn(async move {
let mut buffer = Vec::new();
let _ = stdout.read_to_end(&mut buffer).await;
@ -522,13 +519,11 @@ fn desktop_capability_ready(capability: &DesktopCapability) -> bool {
pub(super) async fn probe_managed_bin(bin: PathBuf) -> ManagedProbe {
let started = Instant::now();
// Runtime readiness is intentionally uncached. A matching capability
// fingerprint only proves protocol compatibility, not that Studio's backend
// imports are complete after an interrupted dependency transaction.
// Runtime readiness is intentionally uncached: a matching capability
// fingerprint proves protocol compatibility, not that the venv is complete.
if let Err(reason) = probe_cli_runtime(&bin).await {
// A CLI too old for this subcommand is the one the interrupted installs
// shipped with, and -h passes without touching the backend, so accepting
// it here leaves those users on the same crash. Update it, then ask.
// A CLI too old for this subcommand is what the interrupted installs
// shipped with, and -h passes without touching the backend.
let reason = if reason == RUNTIME_PROBE_FAILED && predates_runtime_check(&bin).await {
RUNTIME_CHECK_UNSUPPORTED.to_string()
} else {
@ -619,7 +614,7 @@ pub async fn managed_install_ready() -> bool {
}
/// Ready, or the reason it is not, so repair can tell an unrepairable cause
/// apart from a stale install rather than reinstalling over both.
/// apart from a stale install.
pub async fn managed_install_state() -> Result<(), Option<String>> {
match probe_managed_install().await {
ManagedProbe::Ready { .. } => Ok(()),

37
unsloth_cli/commands/studio.py
View file

@ -283,9 +283,8 @@ def _studio_requirement_roots(run_mod):
roots = []
for line in requirements.read_text(encoding = "utf-8").splitlines():
line = line.partition("#")[0].strip()
# pip flags (-r, --extra-index-url) are not requirements. Skipping them
# matters: an unparseable line would report the install broken, and
# repair reinstalls the same file, so the failure would never clear.
# pip flags (-r, --extra-index-url) are not requirements. Treating one
# as broken is permanent: repair reinstalls the same file.
if not line or line.startswith("-"):
continue
try:
@ -301,10 +300,9 @@ def _studio_requirement_roots(run_mod):
def _missing_studio_requirement(run_mod):
"""First requirement studio.txt needs that this venv cannot supply.
Walks dependencies too: starlette is imported by studio/backend/main.py but
reaches the venv only as a FastAPI dependency, and main is imported inside
run_server, so a direct-only check calls the install ready and the server
still dies on startup. Metadata only, no imports, ~80ms on a full venv.
Walks dependencies too: starlette reaches the venv only via FastAPI and is
imported inside run_server, so a direct-only check calls a doomed install
ready. Metadata only, no imports, ~80ms on a full venv.
"""
from importlib.metadata import PackageNotFoundError, distribution
from packaging.requirements import InvalidRequirement, Requirement
@ -313,21 +311,18 @@ def _missing_studio_requirement(run_mod):
seen = set()
def visit(requirement, is_root):
"""The name to report, or None; queues the requirement's dependencies."""
"""Missing name, or None; queues the requirement's dependencies."""
try:
installed = distribution(requirement.name)
except PackageNotFoundError:
return requirement.name
# Metadata outlives the package it describes: hatchling wheels (fastapi,
# typer) store .dist-info/METADATA as the first archive entry, so an
# unpack killed midway leaves a readable version for modules that never
# landed. RECORD is written last, so its absence marks that unpack.
# Metadata outlives the package: wheels store METADATA first, so a killed
# unpack leaves a version behind. RECORD is last, so its absence marks it.
if installed.files is None:
return requirement.name
# Only studio.txt pins are enforced. install_python_stack installs torch
# and friends with --no-deps, so a transitive bound can read unsatisfied
# in a venv that works, and repair reinstalls studio.txt either way.
# prereleases=True: a prerelease satisfying a floor is not a broken install.
# Only studio.txt pins are enforced: install_python_stack uses --no-deps,
# so transitive bounds read unsatisfied in venvs that work. A prerelease
# satisfying a floor is not a broken install either.
if (
is_root
and requirement.specifier
@ -345,9 +340,8 @@ def _missing_studio_requirement(run_mod):
pending.append(parsed)
return None
# Every root is checked before the walk starts. Reached as a dependency
# first, a root would mark itself seen and never meet its own pin: datasets
# asks for huggingface-hub>=0.25,<2 and studio.txt pins it to ==0.36.2.
# Roots are seen up front: reached as a dependency first, a root would never
# meet its own pin (datasets wants huggingface-hub<2, studio.txt ==0.36.2).
roots = _studio_requirement_roots(run_mod)
seen.update(_canonical_distribution_name(root.name) for root in roots)
for requirement in roots:
@ -2927,9 +2921,8 @@ def desktop_runtime_check(
"reason": "missing_dependency",
"module": exc.name,
}
# SystemExit is not an Exception. run.py raises it for rejected settings such
# as UNSLOTH_CPU_THREADS, and letting it escape would emit no payload at all,
# so the desktop app would reinstall over an environment value instead.
# SystemExit is not an Exception. run.py raises it for rejected settings like
# UNSLOTH_CPU_THREADS; escaping emits no payload, so the app reinstalls.
except SystemExit as exc:
payload = {
"runtime_ready": False,

29
unsloth_cli/tests/test_studio_runtime_readiness.py
View file

@ -65,8 +65,7 @@ def test_desktop_runtime_check_catches_later_startup_dependency(monkeypatch, cap
def test_desktop_runtime_check_ignores_pip_flag_lines(monkeypatch, capsys, tmp_path):
"""An unparseable line must not fail the check: repair reinstalls the same
file, so the install would be declared broken forever."""
"""Repair reinstalls the same file, so an unparseable line breaks it forever."""
studio = importlib.import_module("unsloth_cli.commands.studio")
backend = tmp_path / "backend"
requirements = backend / "requirements"
@ -126,9 +125,8 @@ def test_desktop_runtime_check_rejects_version_mismatch(monkeypatch, capsys, tmp
def test_desktop_runtime_check_rejects_metadata_without_an_unpacked_package(
monkeypatch, capsys, tmp_path
):
"""fastapi's wheel stores .dist-info/METADATA as its first archive entry, so
an interrupted unpack leaves a readable version for modules that never
landed. RECORD is written last, so its absence marks the unfinished unpack."""
"""fastapi's wheel stores METADATA first, so an interrupted unpack leaves a
readable version behind. RECORD is last, so its absence marks the unpack."""
studio = importlib.import_module("unsloth_cli.commands.studio")
backend = tmp_path / "backend"
requirements = backend / "requirements"
@ -166,9 +164,8 @@ def _fake_distributions(monkeypatch, installed):
def test_desktop_runtime_check_rejects_a_missing_transitive_dependency(
monkeypatch, capsys, tmp_path
):
"""starlette reaches the venv only as a FastAPI dependency, and the backend
imports it from main.py, which run.py imports inside run_server. A
direct-only check calls the install ready and the server dies on start."""
"""starlette reaches the venv only as a FastAPI dependency, so a direct-only
check calls the install ready and the server dies on start."""
studio = importlib.import_module("unsloth_cli.commands.studio")
backend = tmp_path / "backend"
requirements = backend / "requirements"
@ -187,8 +184,7 @@ def test_desktop_runtime_check_rejects_a_missing_transitive_dependency(
def test_desktop_runtime_check_ignores_optional_and_circular_dependencies(
monkeypatch, capsys, tmp_path
):
"""No extra is requested, so an extras-only dependency is not missing, and a
dependency cycle must terminate rather than walk forever."""
"""Extras-only dependencies are not missing, and a cycle must terminate."""
studio = importlib.import_module("unsloth_cli.commands.studio")
backend = tmp_path / "backend"
requirements = backend / "requirements"
@ -200,8 +196,8 @@ def test_desktop_runtime_check_ignores_optional_and_circular_dependencies(
monkeypatch,
{
"fastapi": ("0.140.5", ['uvicorn; extra == "standard"', "starlette"]),
# Transitive bounds are not enforced: install_python_stack installs
# with --no-deps, so an unmet one can describe a working venv.
# Transitive bounds are not enforced: --no-deps installs leave unmet
# ones on venvs that work.
"starlette": ("0.1", ["fastapi>=99"]),
},
)
@ -212,9 +208,8 @@ def test_desktop_runtime_check_ignores_optional_and_circular_dependencies(
def test_desktop_runtime_check_reports_a_rejected_setting_instead_of_exiting(monkeypatch, capsys):
"""run.py raises SystemExit for values such as UNSLOTH_CPU_THREADS=invalid.
Escaping without a payload makes the desktop app reinstall over an
environment value no install can change."""
"""run.py raises SystemExit for values like UNSLOTH_CPU_THREADS=invalid, and
with no payload the app reinstalls over a value no install can change."""
studio = importlib.import_module("unsloth_cli.commands.studio")
def _rejected_setting():
@ -233,8 +228,8 @@ def test_desktop_runtime_check_reports_a_rejected_setting_instead_of_exiting(mon
def test_a_root_pin_is_checked_even_when_a_dependency_names_it_first(monkeypatch, capsys, tmp_path):
"""datasets asks for huggingface-hub>=0.25,<2 and studio.txt pins ==0.36.2.
Reached as a dependency first, the pin would never get to decide."""
"""datasets wants huggingface-hub<2, studio.txt pins ==0.36.2. Reached as a
dependency first, the pin would never get to decide."""
studio = importlib.import_module("unsloth_cli.commands.studio")
backend = tmp_path / "backend"
requirements = backend / "requirements"