From 508d3720c4dbea7e5b71f27c87780c065c3885be Mon Sep 17 00:00:00 2001 From: Daniel Han Date: Mon, 27 Jul 2026 17:54:28 +0000 Subject: [PATCH] Tighten comments across the install readiness changes --- studio/src-tauri/src/commands.rs | 7 +--- studio/src-tauri/src/desktop_backend_owner.rs | 10 ++--- studio/src-tauri/src/install.rs | 24 +++++------- studio/src-tauri/src/preflight.rs | 25 +++++-------- studio/src-tauri/src/preflight/managed.rs | 33 +++++++---------- unsloth_cli/commands/studio.py | 37 ++++++++----------- .../tests/test_studio_runtime_readiness.py | 29 ++++++--------- 7 files changed, 65 insertions(+), 100 deletions(-) diff --git a/studio/src-tauri/src/commands.rs b/studio/src-tauri/src/commands.rs index 813d8c6ec8..ddacbee71e 100644 --- a/studio/src-tauri/src/commands.rs +++ b/studio/src-tauri/src/commands.rs @@ -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 { 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"); diff --git a/studio/src-tauri/src/desktop_backend_owner.rs b/studio/src-tauri/src/desktop_backend_owner.rs index 95b606b111..5e0c0425e6 100644 --- a/studio/src-tauri/src/desktop_backend_owner.rs +++ b/studio/src-tauri/src/desktop_backend_owner.rs @@ -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)); diff --git a/studio/src-tauri/src/install.rs b/studio/src-tauri/src/install.rs index fc149aba0f..74e560a921 100644 --- a/studio/src-tauri/src/install.rs +++ b/studio/src-tauri/src/install.rs @@ -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, 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( diff --git a/studio/src-tauri/src/preflight.rs b/studio/src-tauri/src/preflight.rs index 3032bb4e63..b9046eaa26 100644 --- a/studio/src-tauri/src/preflight.rs +++ b/studio/src-tauri/src/preflight.rs @@ -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 diff --git a/studio/src-tauri/src/preflight/managed.rs b/studio/src-tauri/src/preflight/managed.rs index 4a6e552fcd..ba38523688 100644 --- a/studio/src-tauri/src/preflight/managed.rs +++ b/studio/src-tauri/src/preflight/managed.rs @@ -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> { match probe_managed_install().await { ManagedProbe::Ready { .. } => Ok(()), diff --git a/unsloth_cli/commands/studio.py b/unsloth_cli/commands/studio.py index 6baea99f74..43c6634865 100644 --- a/unsloth_cli/commands/studio.py +++ b/unsloth_cli/commands/studio.py @@ -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, diff --git a/unsloth_cli/tests/test_studio_runtime_readiness.py b/unsloth_cli/tests/test_studio_runtime_readiness.py index 80b51a875e..454638258b 100644 --- a/unsloth_cli/tests/test_studio_runtime_readiness.py +++ b/unsloth_cli/tests/test_studio_runtime_readiness.py @@ -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"