Commit graph

3 commits

Author SHA1 Message Date
Daniel Han
22e6d64493
Studio: scale export GGUF size estimates from the real model size (#6418)
* Studio: scale export GGUF size estimates from the real model size

The Export page showed hardcoded, model-independent GGUF quant size
labels (Q8_0 ~8.2 GB, BF16 ~14.2 GB, ...) calibrated for an ~8B model.
For a 35B MoE model like Qwen3.6-35B-A3B (67 GiB bf16, Q8 ~34 GiB) the
picker wrongly reported Q8 ~8.2 GB. Only the displayed estimate was
wrong; the actual export via save_pretrained_gguf was always correct.

Add GET /api/models/export-size, which returns a model's MoE-aware
fp16/bf16-equivalent size and total params using the existing
estimate_fp16_model_size_bytes (safetensors -> config -> local -> vllm).
The result is memoized and degrades to nulls so a size hint can never
break the Export page.

The Export picker now scales each quant from that size
(bytes ~= fp16_bytes * bits_per_weight / 16, GiB units to match the
model selector), and renders no size when it is unknown rather than a
misleading fixed number. The Est. size summary in the page and dialog
is restored now that the value comes from the backend.

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

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

* Studio export-size: address review feedback

- Run the size estimate off the event loop with asyncio.to_thread so a slow
  Hugging Face request cannot stall other API or SSE endpoints.
- Cache only successful estimates; a transient failure (offline, gated before
  credentials) is no longer pinned as unavailable until restart.
- Forward the HF token so private and gated models can be sized, and refetch
  when the token changes.
- Clamp the size formatter index so sub-1-byte values cannot pick an
  out-of-range unit.

* Studio export-size: address second review pass

- Send the HF token in an X-HF-Token header instead of the query string, so
  it never lands in URLs, logs, or browser history.
- Key the estimate cache by model id only (the fp16 size is token independent),
  so HF tokens are never retained in the cache.
- Restrict local-path sizing to known Studio roots (outputs/exports/cache/home)
  so an authenticated caller cannot trigger a scan of an arbitrary directory.

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

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

* Studio export-size: fix CI (import-hoist + isolated-load test stubs)

- Import ExportSizeResponse from models.models in routes/models.py instead of
  re-exporting it through models/__init__.py, so the import-hoist lint does not
  flag a newly added but un-loaded re-export (models/__init__.py is unchanged).
- Add Header and ExportSizeResponse to the stubbed fastapi / models.models in
  test_export_absolute_paths.py, which loads routes/models.py in isolation.

* Studio: validate export-size local path before filesystem access

CodeQL flagged the export-size local-path guard as path injection: the
user-provided model path was resolved and stat-ed before it was checked
for containment under a Studio data root. Decide containment by lexical
normalization (normpath/abspath/expanduser, no filesystem access) and
only touch the filesystem once the path is proven to sit under a trusted
root, so an unvalidated value never reaches a filesystem call. Add a
direct containment unit test (under-root, root itself, missing, /etc,
and '..' traversal).

* Studio: trim export-size comments to be more concise

Shorten docstrings and comments on the export-size endpoint, helpers, tests,
and frontend size utilities; drop comments that just restate the code. Verified
code-identical (comments only) via AST/TS-compiler check. No behavior change.

* Studio: harden export-size local-path handling

Address review feedback on the export-size endpoint's local sizing:
- Resolve symlinks and re-verify containment in _is_sizable_local_path so a
  symlink inside a Studio root can't point the sizer outside it.
- Re-validate the resolved LoRA base before sizing, so a crafted adapter
  whose base_model points outside the roots can't redirect the scan.
- Skip nested checkpoint-*/global_step* snapshots when summing local weight
  sizes so a run dir's intermediate checkpoints don't inflate the estimate.
- Size the checkpoint directory for full fine-tune checkpoint exports (whose
  base may be a local/custom path), keeping base-model sizing for adapters.

Adds tests for the adapter-base escape, symlink escape, and nested-checkpoint
exclusion.

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

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

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: danielhanchen <michaelhan2050@gmail.com>
2026-06-18 05:44:17 -07:00
Wasim Yousef Said
048f34e8f2
Fix GGUF variant file selection (#6342)
* Fix GGUF variant resolution

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

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

* Address GGUF variant review feedback

* Harden GGUF endian filtering

* Address GGUF endian review comments

* Mirror GGUF endian filter in local resolver

* Fix GGUF route import test stub

* Apply GGUF endian filtering across load paths

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

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

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
2026-06-16 12:42:58 +02:00
Anmol Mishra
554c289538
fix: respect absolute export paths to prevent cross-drive copy failures (WinError 112) (#6088)
* fix: allow absolute save_directory in export paths to prevent cross-drive copy failures

The GGUF export pipeline (and all other export flows) forced every
save_directory through resolve_export_dir(), which always resolved
the path under exports_root() — typically ~/.unsloth/studio/exports/
on the system drive (C: on Windows).

When a user selected an output directory on a different drive (E:):
1. The absolute path was rejected at the Pydantic validator level.
2. Even if it got through, resolve_export_dir would re-resolve it
   under C:\Users\.unsloth\studio\exports\.
3. After GGUF conversion completed on E:, the relocation step would
   try to move/copy the finished files to C:, causing:
   - WinError 17 (cross-drive move failure when shutil.move falls
     through to a cross-filesystem copy)
   - WinError 112 (disk full on C:)

Fix both layers:
- _validate_save_directory: accept absolute paths (they represent an
  explicit user choice of output location).
- resolve_export_dir, resolve_output_dir, resolve_tensorboard_dir:
  return absolute paths as-is instead of forcing them under the
  default root. Keep the existing safety checks (null bytes, '..'
  segments) and fall through to resolve_under_root for relative paths.

Fixes: https://github.com/unslothai/unsloth/issues/6082

* refactor: centralize user path validation into _resolve_user_path helper

Addresses code review feedback: the null-byte, '..', and absolute-path
checks were duplicated across resolve_output_dir, resolve_export_dir,
and resolve_tensorboard_dir. Extract a single _resolve_user_path helper
that all three delegate to.

No behavioral change — pure consolidation.

* fix: address code review — contain destructive cleanup and scope absolute paths

Address all review feedback from gemini-code-assist:

1. P1: destructive subdirectory cleanup (export_gguf)
   The flattening loop in export_gguf previously rmtree'd every
   subdirectory under abs_save_dir. When targeting an existing user
   directory on a different drive (#6082), this could nuke unrelated
   subdirectories. Now snapshot existing subdirectories before the
   export and only clean up dirs created during this run.

2. P2: keep scan/read endpoints contained
   Only resolve_export_dir accepts absolute paths (export is a write
   path where user picks location). Reverted resolve_output_dir and
   resolve_tensorboard_dir to use resolve_under_root directly — these
   are used by scan/read/training endpoints that must stay contained
   under their respective roots.

3. Centralization feedback
   Removed the _resolve_user_path helper since it's no longer needed
   with the narrowed scope. resolve_export_dir has the absolute path
   logic inline with a clear docstring.

* fix: skip pre-existing subdirs in GGUF flatten loop and clean stale export intermediates

Two issues caught in code review (chatgpt-codex-connector):

1. The flattening loop moved ALL .gguf files from ALL subdirectories
   into abs_save_dir, including pre-existing unrelated user subdirs.
   Now skip pre-existing subdirs entirely unless they are known
   export-owned intermediates (model/, model_gguf/).

2. After a failed export, known export-owned subdirectories (model/,
   model_gguf/) were snapshotted as pre-existing on retry and never
   cleaned up. These are now always cleaned up regardless, since they
   are known intermediates created by the export pipeline.

* fix: separate write vs read export paths, guard same-dir rmtree

Three issues caught in code review (chatgpt-codex-connector):

1. P1: scan endpoint containment
   resolve_export_dir was changed to accept absolute paths, but it's
   also used by scan/read endpoints (routes/models.py) that must stay
   contained under exports_root(). Split into:
   - resolve_export_dir: contained, used by scans
   - resolve_export_write_dir: accepts absolute paths, used by export
     backend only

2. P1: same-directory rmtree
   When a non-PEFT checkpoint's gguf_dir resolves to the same path as
   abs_save_dir (user selected the checkpoint's gguf output as their
   export directory), shutil.rmtree(gguf_dir) would delete the user's
   chosen output directory. Now skip relocation when both paths resolve
   to the same location.

3. P1: pre-existing subdir flatten loop
   Reverted _EXPORT_OWNED_SUBDIRS logic — 'model/' and 'model_gguf/'
   are common directory names in shared model folders and don't prove
   export ownership. Now only clean up subdirs that didn't exist before
   the export started.

* fix: remove dead _EXPORT_OWNED_SUBDIRS and fix _export_details for absolute paths

Two fixes from review comments:

1. Remove unused _EXPORT_OWNED_SUBDIRS declaration (leftover from
   previous iteration that was intentionally removed).

2. _export_details now returns the full absolute path when the export
   target is outside exports_root(), instead of truncating to basename.
   Users who export to E:\ can now see the full destination path in
   the success dialog.

* fix: use unique tmp dir for GGUF intermediates to avoid overwriting user dirs

When exporting to an absolute destination that already contains a
model/ subdirectory (e.g. a shared models folder), the hard-coded
model_save_path would overwrite files in that unrelated directory.

Use _tmp_model_<uuid> as the intermediate path instead, so user
directories are never touched. The tmp dir is created as a new subdir
of abs_save_dir and cleaned up by the flatten loop after GGUF files
are relocated.

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

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

* Fix GGUF local export paths for PR #6088

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

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

* Address GGUF export follow-ups for PR #6088

* Clean GGUF temp dirs on export failure for PR #6088

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

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

* Fix/adjust export path tests for PR #6088

* Fix/adjust export path review findings for PR #6088

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

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

* Fix/adjust home export path handling for PR #6088

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: wasimysaid <wasimysdev@gmail.com>
2026-06-12 12:52:57 +02:00