Fix case-variant model matching and GGUF cache reuse in unsloth start#6900
Conversation
Imagineer99
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces case-insensitive matching and fallback mechanisms for cached Hugging Face repository snapshots and GGUF files, allowing offline reuse of older or differently-cased cached models. It also adds corresponding unit tests. The review feedback highlights a critical incorrect import path in llama_cpp.py that would cause cache resolution to fail silently, a potential OSError crash during snapshot directory sorting in model_config.py, and an opportunity to improve local path detection in start.py by checking file existence directly instead of relying on string prefixes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
6fa6366 to
e61fd5e
Compare
for more information, see https://pre-commit.ci
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e207acdf32
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…erly When a cached main shard was reused from an older snapshot, the extra shards were resolved independently and could come from a different snapshot dir (or a fresh download into the current ref), leaving llama.cpp unable to load a multi-shard GGUF whose pieces are split across directories. Only reuse a cached main shard when every sibling shard sits in the same snapshot; otherwise fetch the whole set together so they stay co-located. Also patch huggingface_hub.constants.HF_HUB_CACHE (not just the HF_HUB_CACHE env var) in the two cache tests that seeded a temp cache: the snapshot lookup reads the module constant, so the env-only override let the real cache leak in and skip an asserted download.
|
Pushed a small revision (e86112e):
Verified with the offline GGUF cache suite (66 passed, including a new split-across-snapshots regression test) and an exhaustive |
When listing GGUF variants from the local HF cache, a newer snapshot may contain only a companion file (for example a vision projector fetched on demand) while the actual quant files live in an older snapshot. The prior scan returned the first snapshot whose vision flag was set, yielding an empty variant list and hiding the real quants. Keep scanning older snapshots for actual variants and carry the vision flag across snapshots. Also record the disk-space fallback variant's size in expected_sizes so the later cache-reuse probe can size-verify the fallback main shard instead of only checking for its existence.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a2743861b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if not (isinstance(cached_path, str) and os.path.exists(cached_path)): | ||
| cached_path = _cached_hf_snapshot_file( | ||
| hf_repo, | ||
| p.path, | ||
| expected_size = p.size, |
There was a problem hiding this comment.
When a split GGUF has shard files cached in different snapshot directories, this new snapshot fallback makes the disk preflight count each shard as already cached, so total_download_bytes can become 0 and the smaller-variant fallback is skipped. Later in the same _download_gguf flow, the co-location check rejects that split cache and downloads the full shard set anyway, so low-disk users can hit a download failure instead of falling back even though the preflight claimed nothing needed to be downloaded. The preflight should apply the same co-location requirement before subtracting split-shard bytes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. The disk-space preflight now applies the same co-location check (_snapshot_has_all_shards) before counting split-shard bytes as cached: if a split GGUF's shards are not co-located in one snapshot the whole set is treated as needing download, so the preflight no longer reads 0 bytes, skips the smaller-variant fallback, and then fails the full download on a low-disk machine.
| if resolved_hf_repo != hf_repo: | ||
| logger.info( | ||
| "Using cached repo_id casing '%s' for requested '%s'", | ||
| resolved_hf_repo, | ||
| hf_repo, | ||
| ) | ||
| hf_repo = resolved_hf_repo |
There was a problem hiding this comment.
This casing normalization is local to _download_gguf, but the load_model path still calls _download_mmproj and _download_mtp with the original hf_repo string after the main GGUF returns. In the offline/cache-only case where the requested casing differs from the cached canonical repo, the main file can now resolve from the canonical cache while companion resolution only picks a target from that cache and then calls hf_hub_download under the original cache key, so vision projectors or MTP drafters are returned as None even though they are cached. The resolved repo id needs to be shared with the companion downloads or they need the same snapshot-file reuse.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. load_model now resolves the requested repo id to its cached canonical casing once, up front (via a shared _resolve_repo_id_casing helper), and passes that to the main GGUF and both companions (_download_mmproj / _download_mtp), so an offline case-variant request no longer loads the main file from the canonical cache dir while the vision projector / MTP drafter miss it under the requested casing.
| try: | ||
| if Path(os.path.expanduser(text)).exists(): | ||
| return False | ||
| except OSError: | ||
| return False | ||
| return True |
There was a problem hiding this comment.
When Studio reports a loaded model whose id is a relative local directory like Models/Foo, running unsloth start --model models/foo from a different working directory makes this exists() probe fail, so both strings are treated as Hub IDs and _resolve_model attaches to the already-loaded local model instead of asking Studio to load the requested Hub repo or distinct path. The backend can classify an existing one-slash relative path as local and expose it unchanged, so the CLI should not rely on the client cwd to decide whether case-insensitive matching is safe.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid observation: the exists() probe is relative to the client's cwd, so a loaded model whose id is a bare relative local path could be misclassified from a different cwd. The robust fix is backend-side (Studio classifying and exposing the loaded model's local-vs-hub nature), since the client cannot reliably resolve a server-relative path from its own cwd. That is out of scope for this client-side matching change; the current heuristic is best-effort and correct for the common cases (absolute local paths and hub ids). Tracking it as a backend follow-up rather than changing the client guess.
…ation Two fixes to the case-variant GGUF cache reuse: - Resolve the requested repo id to its cached canonical casing once in load_model, up front, and pass it to the main GGUF and its companions (mmproj / MTP drafter). Previously only _download_gguf resolved the casing internally, so a case-variant request loaded the main file from the canonical cache dir while the companions kept the requested casing and missed the cached vision projector / drafter offline. Extracted the resolution into a shared _resolve_repo_id_casing helper. - Apply the split-shard co-location check in the disk-space preflight. When a split GGUF's shards are cached across different snapshots the whole set is refetched later, so counting them as cached made the preflight read 0 bytes to download, skip the smaller-variant fallback, and then fail the full download on a low-disk machine.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c6fcebb9b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if main_cached is None or not _snapshot_has_all_shards( | ||
| main_cached, gguf_filename, gguf_extra_shards, expected_sizes | ||
| ): | ||
| split_needs_refetch = True |
There was a problem hiding this comment.
When a newer cache snapshot has only the first shard but an older snapshot has the complete split GGUF, _cached_hf_snapshot_file() returns that newer partial main shard first. This check then immediately marks the split as needing a refetch instead of looking for an older co-located shard set, so offline or low-disk users with a complete older snapshot can still be forced into a download or smaller-variant fallback even though the requested model is already cached.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fb24ba8. Split reuse now scans snapshots for one that holds the whole set co-located (_cached_colocated_split_main) instead of taking the newest snapshot's first shard, so a newer partial snapshot no longer shadows an older complete one and an already-cached split model is reused rather than refetched.
| local_path = _cached_hf_snapshot_file( | ||
| hf_repo, | ||
| gguf_filename, | ||
| expected_size = expected_sizes.get(gguf_filename), | ||
| ) |
There was a problem hiding this comment.
When the Hub is reachable but the current revision's GGUF is not cached, this shortcut can return any older snapshot with the same filename and at least the current size, bypassing hf_hub_download and its revision/etag check. If a repo updates a GGUF in place while keeping the same path and similar size, online loads keep serving the stale cached weights instead of fetching the current file; restrict this fallback to offline/current-ref repair or verify the snapshot revision before returning it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid: online, reusing a same-name snapshot from a different revision can serve stale weights past hf_hub_download's etag check. The naive fix (reuse only when offline) regresses the intended online reuse for case-variant repo dirs and current-ref-partial repair (covered by test_download_reuses_older_snapshot_when_current_ref_snapshot_is_partial and the lowercase-shadow test), so the correct fix is to verify the snapshot's revision matches the current ref before reusing it rather than gating on offline. Leaving that as a follow-up so the legitimate reuse paths are preserved.
| if not (_is_hub_model_id(actual) and _is_hub_model_id(requested)): | ||
| return False | ||
| return str(actual).casefold() == str(requested).casefold() |
There was a problem hiding this comment.
For a loaded local model reported as a relative path such as models/Foo, running unsloth start --model models/foo from a different cwd or on a case-sensitive filesystem where that path does not exist makes both strings pass _is_hub_model_id(). The new matcher then casefolds them and attaches to models/Foo instead of treating the differently-cased local path as a mismatch, which can connect the agent to the wrong local model.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The exact-match short-circuit already handles same-string matches; casefolding is applied only to hub ids, which are case-insensitive on the Hub, so folding them is correct. The residual case is a loaded model reported as a bare relative local path that does not exist from the client's cwd, which is fundamentally ambiguous client-side (org/name and dir/file are indistinguishable). The robust fix is server-side: Studio classifying and exposing the loaded model as local vs hub. The client heuristic already rejects existing local paths, path-shaped prefixes, and drive letters, which covers the supported configurations, so not changing the client guess here.
| gguf_filename = fallback_file | ||
| # Record the fallback's size so the later cache-reuse | ||
| # probe can size-verify it; without this the fallback | ||
| # main shard is only checked for existence. | ||
| expected_sizes[fallback_file] = fallback_size |
There was a problem hiding this comment.
When the low-disk fallback selects a split GGUF, _find_smallest_fitting_variant() returns the total size of all shards, but this stores that total as the expected size for the first shard. A fully cached first shard is therefore rejected unless it is as large as the whole split model, so cached split fallbacks are unnecessarily re-downloaded instead of reused.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fb24ba8. The disk-space fallback now records its size in expected_sizes only for a single-file fallback. _find_smallest_fitting_variant returns the whole variant size, so storing it as the first shard's expected size rejected a valid cached first shard of a split fallback; split fallbacks fall back to an existence check.
- When reusing a cached split GGUF, scan snapshots for one that holds the whole set co-located instead of taking the newest snapshot's first shard. A newer snapshot with only the first shard no longer shadows an older complete snapshot, so an already-cached split model is reused rather than refetched (which would fail offline). - The disk-space fallback records its size in expected_sizes only for a single-file fallback. _find_smallest_fitting_variant returns the whole variant size, so using it as the first shard's expected size rejected a valid cached first shard of a split fallback and forced a re-download.
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb24ba8949
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if main_cached is None or not _snapshot_has_all_shards( | ||
| main_cached, gguf_filename, gguf_extra_shards, expected_sizes | ||
| ): | ||
| split_needs_refetch = True |
There was a problem hiding this comment.
When a split GGUF has a newer cache snapshot containing only the first shard and an older snapshot containing the complete shard set, _cached_hf_snapshot_file returns the newer first shard, so this branch marks split_needs_refetch and counts zero cached bytes. On low-disk/offline loads the preflight can then fall back or raise before the later _cached_colocated_split_main scan gets a chance to reuse the complete older snapshot, so a fully cached split model can fail to load unnecessarily. Use the co-located snapshot scan here before deciding the split must be refetched.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b2b1009. The preflight now uses the same co-located snapshot scan as the download path (_cached_colocated_split_main) instead of the newest-snapshot probe, so a newer snapshot with only the first shard no longer masks an older complete one and needlessly trips the disk fallback for a fully cached split model.
| local_path = _cached_hf_snapshot_file( | ||
| hf_repo, | ||
| gguf_filename, | ||
| expected_size = expected_sizes.get(gguf_filename), | ||
| ) |
There was a problem hiding this comment.
In online non-force loads this returns the first matching snapshot file and skips hf_hub_download, so a repo whose GGUF was updated under the same filename can keep serving an older cached commit whenever the old file size is at least the current size. Before this change the download helper checked the Hub ref and would resolve the current file; use the snapshot shortcut only when offline or after confirming the snapshot matches the current revision.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid concern. The cross-snapshot reuse is a deliberate resilience choice (offline and case-variant repo dirs, current-ref-partial repair), and gating it on offline regresses those legitimate paths (test_download_reuses_older_snapshot_when_current_ref_snapshot_is_partial and the lowercase-shadow test). The freshness fix is to verify the reused snapshot's revision matches the current ref before serving it online, which is a design decision about the reuse mechanism itself (resilience vs freshness). Leaving that to a focused change so the resilience paths are preserved; note try_to_load_from_cache is revision-safe if the online path should prefer it.
There was a problem hiding this comment.
Same fix in e0c17e2: the snapshot reuse path is now only taken when HF_HUB_OFFLINE=1. In the online case we always go through hf_hub_download, which validates against the current revision, so a stale cached snapshot cannot shadow an updated file.
| None | ||
| if requested and load_has_overrides | ||
| else next((m for m in models if m["id"] == requested), None) | ||
| else next((m for m in models if _model_id_matches(m.get("id"), requested)), None) |
There was a problem hiding this comment.
When /v1/models contains a cached-but-unloaded catalog entry whose id only differs from the request by case, this new case-insensitive match treats it as ready and never calls /api/inference/load. Studio marks those catalog entries with loaded: false, and Codex then preflights the currently active backend rather than this requested entry, so a cached GGUF can be rejected or the agent can be pointed at a model that is not resident unless auto-switch happens to be enabled. Require the match to be loaded before using it to skip the load endpoint.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b2b1009. _resolve_model now only attaches to a /v1/models entry whose loaded is not False, so a cached-but-unloaded catalog entry that only case-differs no longer skips /api/inference/load and leaves the agent pointed at a non-resident model. Covered by test_resolve_model_loads_when_catalog_hit_is_not_loaded.
… catalog hit - The disk-space preflight now uses the same co-located snapshot scan as the download path (_cached_colocated_split_main) instead of the newest-snapshot probe, so a newer snapshot holding only the first shard no longer masks an older complete one and trips the smaller-variant fallback for a fully cached split model. - _resolve_model only attaches to a /v1/models entry that is actually loaded (loaded != False). /v1/models also lists cached-but-unloaded catalog entries, and matching one by case skipped /api/inference/load and left the agent pointed at a model that is not resident.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2b1009344
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| local_path = _cached_colocated_split_main( | ||
| hf_repo, gguf_filename, gguf_extra_shards, expected_sizes | ||
| ) | ||
| else: | ||
| local_path = _cached_hf_snapshot_file( |
There was a problem hiding this comment.
When force is false, this new cache probe can return any local snapshot that has the requested GGUF file before calling hf_hub_download_with_xet_fallback. In an online load where the Hub repo has moved since the cached snapshot, especially if the replacement has the same or a smaller size, the loader never asks Hugging Face to resolve the current main revision and silently serves stale weights until the user force-downloads or clears the cache. This fallback should only be used when the current-revision download/cache lookup is unavailable, e.g. offline or after confirming the current snapshot does not contain the file.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same deliberate resilience-vs-freshness tradeoff noted above. The cross-snapshot reuse is intentional for offline, case-variant repo dirs, and current-ref-partial repair (regressing those breaks test_download_reuses_older_snapshot_when_current_ref_snapshot_is_partial and the lowercase-shadow test). Restricting it to revision-verified reuse online is a design decision about the reuse mechanism itself, so leaving it to a focused change; try_to_load_from_cache is revision-safe if the online path should prefer freshness over the resilience fallback.
There was a problem hiding this comment.
Addressed in e0c17e2: cross-snapshot and case-variant GGUF reuse is now gated on HF_HUB_OFFLINE. Online, hf_hub_download fetches the current revision (and resumes a partial download), so an in-place GGUF update is picked up rather than shadowed by an older same-name blob. The older-snapshot reuse now only runs offline, as a resilience fallback. The two reuse regression tests are marked as the offline scenarios they represent and a new online test asserts a fresh fetch.
Reusing a same-name blob from an older or case-variant snapshot bypasses the Hub revision/etag check, so a repo that updates a GGUF in place could serve stale weights online. Gate the cross-snapshot and case-variant reuse (both the disk-space preflight accounting and the download path) on HF_HUB_OFFLINE. Online, hf_hub_download fetches the current revision and resumes a partial download, so the reuse is unnecessary there; offline it remains the resilience fallback. Marked the two reuse regression tests as the offline scenarios they represent and added an online test asserting a fresh fetch.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0c17e2520
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return True | ||
| if not (_is_hub_model_id(actual) and _is_hub_model_id(requested)): | ||
| return False | ||
| return str(actual).casefold() == str(requested).casefold() |
There was a problem hiding this comment.
When the requested/loaded model is a relative path that is not present in the CLI process's current filesystem (for example a server-side path such as models/Llama/Foo.gguf, or a remote Studio), _is_hub_model_id treats it as a hub id just because it contains /. This case-insensitive comparison then lets _resolve_model attach to an already loaded models/llama/foo.gguf instead of calling /api/inference/load for the distinct path; on case-sensitive filesystems that can point the agent at the wrong model. Please restrict the casefold path to syntactically valid HF repo ids rather than arbitrary slash-containing strings.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3fd3246: _is_hub_model_id now requires a syntactically valid hub id (a single namespace/name over the HF charset), so a server-side relative path like models/Llama/Foo.gguf is no longer casefold-matched against a differently cased path. This is host independent, unlike the local-existence probe which cannot see a path that only exists on the server. Added a regression assertion.
| # offline. Online, fall through to hf_hub_download so its revision/etag check | ||
| # fetches the current file (and resumes a partial) instead of serving a stale | ||
| # same-name blob from an older revision. | ||
| if not force and os.environ.get("HF_HUB_OFFLINE") == "1": |
There was a problem hiding this comment.
This new offline-only cache reuse is skipped unless HF_HUB_OFFLINE is exactly "1", but Studio already treats values like true, yes, and on as offline in _env_offline. With HF_HUB_OFFLINE=true, the earlier Hub calls can still be offline, but this branch falls through to hf_hub_download instead of _cached_hf_snapshot_file, so the cross-snapshot/case-variant cached GGUF that this change is meant to reuse can still fail to load offline. Please use the same truthy offline parsing here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3fd3246: both offline gates now use the same truthy parse as the rest of the backend (_env_offline: 1/true/yes/on) via a _hf_env_offline helper, so HF_HUB_OFFLINE=true routes through the cached-snapshot reuse instead of falling through to hf_hub_download. Added a regression test with HF_HUB_OFFLINE=true.
| # all resolve from the same cache entry. Otherwise a case-variant | ||
| # request resolves the main file from the canonical cache dir while the | ||
| # companions keep the requested casing and miss the cached files. | ||
| _resolved_repo = _resolve_repo_id_casing(hf_repo) |
There was a problem hiding this comment.
When the requested casing already has any HF cache directory (for example a partial lower-case snapshot), resolve_cached_repo_id_case returns that exact spelling, so this new normalization does not switch companions to the canonical cache. _download_gguf can still load the main file because it now scans all case-variant snapshots directly, but _download_companion_gguf only picks the filename from those snapshots and then calls hf_hub_download with this still-partial repo id, which misses the canonical mmproj/MTP file offline and silently drops the companion. Please either return the cached companion path directly or choose the repo casing from the snapshot that contains the files.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3fd3246: _download_companion_gguf now returns the cached snapshot path directly when offline, resolving the companion via _cached_hf_snapshot_file (which scans every case-variant snapshot) rather than calling hf_hub_download with a possibly-partial repo casing. Added a test where the mmproj lives only in the canonical-cased snapshot while a partial lower-case dir shadows the casing.
Three follow-ups on the case-variant GGUF cache path: - Honor every truthy HF_HUB_OFFLINE spelling (1/true/yes/on), not just "1", when gating the cross-snapshot and case-variant cache reuse. With HF_HUB_OFFLINE=true the Hub calls are already offline, so the reuse must trigger or the cached GGUF fails to load; route both the preflight accounting and the download path through the same offline parse the rest of the backend uses. - Resolve mmproj/MTP companions from the actual cached snapshot when offline. resolve_cached_repo_id_case can keep a partial lower-case spelling when any dir exists under the requested casing, so an hf_hub_download on that casing misses the canonical companion; scan every case-variant snapshot and return the cached path. - Restrict the case-insensitive model-id match to syntactically valid hub ids (a single namespace/name over the HF charset). A server-side relative path such as models/Llama/Foo.gguf is no longer treated as a hub id, so it cannot casefold-match a differently cased path on a case-sensitive filesystem. This is host independent, unlike the local-existence probe which cannot see a server path.
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3fd32462c9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| parts = text.split("/") | ||
| if len(parts) != 2: | ||
| return False | ||
| if any(part in ("", ".", "..") or not _HF_REPO_ID_SEGMENT_RE.match(part) for part in parts): | ||
| return False |
There was a problem hiding this comment.
When unsloth start talks to a remote Studio, this treats any non-existent two-segment string as a Hub repo id, so a loaded server-side relative local model such as Models/Foo will match a request for models/foo and skip /api/inference/load. On a case-sensitive server those are different paths, and the CLI can attach to the already-loaded model instead of loading/failing the requested path; the local Path.exists() probe cannot detect paths that only exist on the Studio host.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c4b53d2: casefold matching is now gated on is_loopback_url(base). The local Path.exists() probe is only authoritative for a Studio on this host, so a remote Studio now uses exact-match only; a case-mismatched request falls through to /api/inference/load, whose already-loaded dedup resolves the correct model instead of the CLI casefold-attaching to a distinct server-side path like Models/Foo. Added a remote-base regression test asserting the load endpoint is consulted.
A two-segment string like Models/Foo is indistinguishable from a hub id, and the local Path.exists() probe in _is_hub_model_id cannot see a path that exists only on a remote Studio host. So against a remote server, casefolding could attach to a distinct server-side path (Models/Foo vs models/foo) on a case-sensitive filesystem. Gate the case-insensitive match on is_loopback_url(base): only a local Studio, where the existence probe is authoritative, casefolds. For a remote Studio the match is exact and a case-mismatched request falls through to /api/inference/load, whose already-loaded dedup resolves it correctly.
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df6b5a57d9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if Path(os.path.expanduser(text)).exists(): | ||
| return False |
There was a problem hiding this comment.
When a bare --model names a two-segment local path on a loopback Studio that was started from a different working directory, this existence check runs in the CLI process cwd, not the server's cwd. A loaded server-relative path such as Models/Foo.gguf can therefore be treated as a Hub id if it does not exist under the CLI cwd, so _model_id_matches casefolds it against models/foo.gguf and skips /api/inference/load even though those are distinct files on a case-sensitive filesystem.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, and it holds even on loopback because the server can be started from a different cwd than the CLI. The robust fix is to classify _is_hub_model_id purely by syntax and drop the Path.exists() probe (it runs in the CLI cwd, not the server's). I verified this locally (tests pass); since maintainer edits are off on this branch now, here is the change to apply:
_MODEL_FILE_SUFFIXES = (".gguf", ".ggml", ".safetensors", ".bin", ".pt", ".pth", ".onnx")
# ... in _is_hub_model_id, replace the try/Path.exists() block with:
# A weight-file path (Models/Foo.gguf) is not a repo id; a repo id names a repo.
if text.lower().endswith(_MODEL_FILE_SUFFIXES):
return False
return True
So Models/Foo.gguf (two-segment, .gguf) is rejected by syntax, models/Llama/Foo.gguf by the extra segment, and a real id like unsloth/gemma-3-4b-it-GGUF (no weight-file suffix) still classifies as a hub id. This is host independent, so it no longer depends on where the CLI or the server was launched.
3 participants
Summary
unsloth startafter Studio canonicalizes model casingRoot Cause
unsloth startcompared requested model IDs to Studio's reported/v1/modelsIDs case-sensitively, so a request likeunsloth/gemma-4-e2b-it-ggufcould fail after Studio reported the canonicalunsloth/gemma-4-E2B-it-GGUF. The GGUF loader also trusted the current HF cache ref / first case-variant cache dir too narrowly, which could miss a valid cached GGUF snapshot and trigger unnecessary downloads.Testing
python -m pytest studio/backend/tests/test_offline_gguf_cache_fallback.py -qpython -m pytest unsloth_cli/tests/test_start.py::test_connect_codex_matches_requested_model_case_insensitively unsloth_cli/tests/test_start.py::test_resolve_model_matches_loaded_canonical_case_after_load -qunsloth start codex --model unsloth/gemma-4-E2B-it-GGUF:UD-Q4_K_XL --no-launch --yolo