Commit graph

4 commits

Author SHA1 Message Date
Daniel Han
187144d4e7
Reduce and tighten code comments and docstrings repo-wide (#6095)
Trim and tighten code comments and docstrings across the repository. Comment-only: every changed file verified code-identical to main via AST/token comparison.
2026-06-08 23:09:51 -07:00
Daniel Han
8292e699e4
Studio: make code comments and docstrings more succinct (#6029)
Trim and tighten code comments and docstrings across studio/ Python. Comment-only: every changed file verified code-identical to main via AST/token comparison.
2026-06-08 23:07:28 -07:00
Daniel Han
3ce187da02
Formatting: ruff line-length 100, kwarg-spacing passes, drop blank after short local imports (#6079)
Raise ruff line-length to 100 and extend the local pre-commit format pipeline (def-signature magic-comma normalization, short multi-line assert collapse, kwarg '=' spacing, blank-line-after-short-import removal, adjacent string-literal / f-string+plain merge, redundant-pass pruning). Every transform re-checks the file AST and is dropped if it would differ; the whole-repo reformat is verified AST-identical per file and idempotent.
2026-06-08 04:24:13 -07:00
Michael Han
3ff6204aa7
studio: load cached GGUF models when fully offline (#5505)
* studio: load cached GGUF models when fully offline

When huggingface.co is unreachable, GGUF model loads fail in three distinct
places even though the bits are already in ~/.cache/huggingface/hub. Each
failure has a different surface symptom:

1. list_gguf_variants() raises straight through HTTPException(500), so the
   variant dropdown shows 'Failed to list GGUF variants'.

2. detect_gguf_model_remote() silently returns None after retries fail. The
   caller then treats a GGUF-only repo as non-GGUF and routes it through the
   transformers/MLX path. On Apple Silicon this surfaces as 'Unsloth currently
   only works on NVIDIA, AMD and Intel GPUs.'

3. _download_gguf() loses list_repo_files() to the network and falls back to a
   filename heuristic ('{repo}-{variant}.gguf'). When the repo name does not
   echo the filenames (e.g. repo 'Qwen3.6-27B-MTP-GGUF' contains a file
   'Qwen3.6-27B-UD-Q4_K_XL.gguf' with no MTP), hf_hub_download cannot find
   that invented filename in the cache and aborts.

Fix in three layers:

- list_gguf_variants / detect_gguf_model_remote: honor HF_HUB_OFFLINE and
  fall back to scanning the local HF cache snapshot when the API throws.
  detect_gguf_model_remote still keeps its retry loop for transient flakes;
  the cache fallback only kicks in after every attempt fails.

- _download_gguf: when list_repo_files() fails, look up variant -> real
  filename inside the cached snapshot before resorting to the heuristic.

- llama_cpp.load_model / inference worker startup: when DNS for
  huggingface.co fails (2s probe), set HF_HUB_OFFLINE=1 for the process so
  every hf_hub_download call below resolves from cache instantly instead of
  spending ~25s on five exponential retries.

Online behavior is unchanged: the API is tried first and only used to fail
over. The cache scan is a strict subset of what list_local_gguf_variants
already does today for local paths.

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* studio: tighten inline comments on offline GGUF fallback

* studio: address review feedback on offline GGUF fallback

Fixes from the review pass on #5505:

* ruff F823 (lint CI red): the late `import os` at the bottom of
  LlamaCppBackend.load_model made `os` a function-local name, so my
  new `os.environ` reference at the top of the same method was a
  use-before-bind. Surfaces at runtime as
  'cannot access local variable os where it is not associated with a value'
  and is why the Mac/Windows Studio API jobs were failing too. The
  env-var mutation has been moved into a module-level contextmanager,
  so load_model no longer touches `os` directly.

* Codex P1: cache variant match now uses the relative path, not the
  basename. Layouts like `BF16/foo.gguf` (variant token only in
  parent dir) were silently skipped, falling through to the bogus
  `{repo}-{variant}.gguf` heuristic and failing offline loads of
  models stored under quant-named subdirs.

* Codex P1: HF_HUB_OFFLINE no longer persists past one model load.
  llama_cpp.load_model now uses a contextmanager that probes DNS,
  sets HF_HUB_OFFLINE/TRANSFORMERS_OFFLINE only when DNS is dead,
  and pops them in finally (preserving any prior user setting of
  TRANSFORMERS_OFFLINE). Pre-existing user-set HF_HUB_OFFLINE is
  respected as a no-op. worker.py keeps the startup probe because the
  orchestrator spawns a fresh worker per load -- comment updated to
  make that lifecycle explicit, and a warning is now logged.

* Gemini: cache-dir lookup centralized in `_iter_hf_cache_snapshots`.
  Three near-identical copies (in list/detect helpers and the
  llama_cpp offline scan) now go through one helper.

* Gemini: `huggingface_hub.utils.is_offline_mode` does not exist in
  1.x (verified locally); `huggingface_hub.constants.HF_HUB_OFFLINE`
  is snapshot-at-import-time and does not reflect runtime mutations.
  Manual env-var parsing kept.

* socket probe now saves and restores the prior default timeout
  instead of unconditionally setting None on exit, so it composes
  with caller code that already configured a timeout.

* worker.py probe now logs a warning when offline mode is auto-enabled
  so debugging the case isn't blind.

* studio: regression tests for offline GGUF cache fallback

Lock in the offline fallback path from #5505 so future refactors can't
silently regress either bug. 26 tests, 0.55 s, no network/GPU/subprocess.

Covers:

* _iter_hf_cache_snapshots: missing cache, missing repo, missing
  snapshots/, newest-mtime ordering, case-insensitive repo match.
* _list_gguf_variants_from_hf_cache and the list_gguf_variants
  online/offline-env/API-exception/reraise paths.
* _detect_gguf_from_hf_cache and detect_gguf_model_remote 3x-fail
  fallback. Pre-existing RepositoryNotFoundError early-return preserved.
* Codex P1 #1 regression: BF16/foo.gguf (quant only in subdir name)
  must resolve via _detect_gguf_from_hf_cache, which now matches the
  snapshot-relative path rather than the basename.
* _probe_dns_dead: returns True/False, restores prior socket timeout.
* Codex P1 #2 regression: _hf_offline_if_dns_dead sets env only inside
  the block, restores on exit (including on exception), re-probes DNS
  on the next call so a transient hiccup cannot lock the long-lived
  LlamaCppBackend singleton offline. Honors a user-set HF_HUB_OFFLINE
  as a no-op. Preserves a user-set TRANSFORMERS_OFFLINE across exit.

Follows the existing studio backend test stub pattern (loggers /
structlog / httpx stubs + backend dir on sys.path).

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* studio: extend offline cache fallback to _download_mmproj and quant label

Two follow-up fixes from the review pass on #5505:

* _download_mmproj() now mirrors _download_gguf()'s offline path:
  when list_repo_files() fails, scan the local HF cache snapshot for
  any GGUF whose basename starts with mmproj-. Without this, offline
  vision GGUF loads succeed at the main weight (the existing PR fix)
  but the mmproj returns None and llama-server starts without vision
  support. Same _iter_hf_cache_snapshots helper, F16 preference and
  fallback to the first match are preserved.

* _extract_quant_label() now considers parent directory segments when
  the basename has no quant token. Layouts like BF16/foo.gguf are
  already documented in this file and are returned by the new
  snapshot-relative-path filter in _download_gguf; before this fix
  their variant label collapsed to "foo" (the last hyphen segment of
  the basename). Regex is the same; the search just walks parent
  segments innermost-first if the basename misses.

Tests (studio/backend/tests/test_offline_gguf_cache_fallback.py):

* TestExtractQuantLabelSubdir: basename quant unchanged, quant-only-
  in-parent, UD- prefix in parent, deeper nesting picks the
  innermost matching segment.
* TestDownloadMmprojOfflineCacheFallback: cache fallback returns the
  mmproj when list_repo_files fails, F16 preference holds when both
  variants are in cache, no-mmproj cache returns None.
* httpx stub now prefers the real package when installed (the CI
  install list already includes it) and falls back to the stub only
  when httpx is genuinely missing. Newer huggingface_hub imports
  HTTPError/Response/Request at module load, so the previous
  fixed-set stub broke when those names were added upstream.

26 existing cases plus 7 new = 33 pass in 0.74s.

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Fix/adjust offline cache + DNS probe per PR #5505 review

Four review findings tightened, with regression tests:

- list_local_gguf_variants subdir collapse (P1 codex 10:08): pass the
  snapshot-relative path to _extract_quant_label so BF16/foo.gguf and
  Q4_K_M/foo.gguf produce distinct labels instead of folding to the same
  basename pseudo-quant.
- list_gguf_variants cache fallback (P2 codex 12:10): surface
  RepositoryNotFoundError / GatedRepoError / RevisionNotFoundError /
  EntryNotFoundError to the caller instead of masking with stale cache,
  matching detect_gguf_model_remote.
- _detect_gguf_from_hf_cache mmproj (P2 codex 12:10): exclude mmproj
  files from the candidate list so a partial cache with only a vision
  projector cannot route the projector as the main model.
- _probe_dns_dead global timeout (P2 codex 13:06): run the gethostbyname
  on a daemon thread with join timeout so concurrent sockets in the same
  interpreter never inherit a process-wide socket.setdefaulttimeout
  mutation. Same shape applied in worker.py's startup probe.

* Make llama-server health check tolerant of warmup races

Two layered fixes for the Windows GGUF smoke CI Tool calling Tests
flake that exit-22'd on a single httpx.ReadError during llama-server
warmup. The 'windows-latest -> windows-2025-vs2026' image rollout is
hitting main with the identical symptom.

A. _wait_for_health: catch httpx.ReadError, RemoteProtocolError,
   WriteError alongside ConnectError and TimeoutException. A TCP RST
   mid-read while llama-server is still binding the port (WinError
   10054) is a 'still warming up' signal, not fatal. The existing
   _process.poll() check still wins for real crashes.

B. _drain_stdout + spawn: tee llama-server stdout/stderr to a
   per-launch log file at ~/.unsloth/studio/logs/llama-server/
   <port>.log. Any future subprocess crash leaves a forensic trace
   on disk even when Studio's traceback only captures the symptom
   (ReadError) and not the cause. Best-effort: a logging-side OSError
   never blocks the load.

Regression coverage: TestWaitForHealthRetriesOnReadError pins the
retry behaviour for the three new exception types and verifies that a
real process exit still short-circuits the loop.

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* ci(windows): retry inference/load + collect llama-server logs

Composite fix for the Tool calling Tests flake that exit-22'd on a
single httpx.ReadError during llama-server warm-up. The
windows-latest -> windows-2025-vs2026 runner image rollout has been
hitting main with the identical symptom.

- All three jobs (openai-anthropic, tool-calling, json-images) now
  retry POST /api/inference/load up to 3 times with 10s backoff and
  preserve the response body for post-mortem. One transient 500 no
  longer fails the whole job.
- A new "Collect llama-server logs" step copies the per-launch
  llama-server stdout teed by Studio under ~/.unsloth/studio/logs/
  llama-server/ into the workspace, and the upload-artifact step
  now includes logs/llama-server/*.log so any future subprocess
  crash leaves a forensic trace.

---------

Co-authored-by: shimmyshimmer <shimmyshimmer@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Daniel Han <danielhanchen@gmail.com>
2026-05-17 21:25:39 -07:00