Studio: never pick a macOS AppleDouble sidecar as a GGUF - #8919
Conversation
On volumes that cannot store Mac file metadata -- exFAT, FAT32, many network shares -- macOS writes a ~4 KB AppleDouble file named `._<name>.gguf` beside every model. Studio accepted any name ending in `.gguf` and took the first match in sorted order, and `._` sorts ahead of letters, so the sidecar won. llama-server then opened 4 KB of Finder metadata, found no GGUF header, and exited. The failure classifier had no branch for that, so it fell through to its catch-all and blamed the file or the user's memory -- which is why a 1-3B model on a 16 GB machine reported an out-of-memory-shaped error. The same selection bug picked `._mmproj-*` for vision models and every `._` part of a split model. Three changes, matching the three ways this surfaced: - Filename rule: `._` prefixed names are no longer GGUFs. The canonical rule lives in `hub.utils.gguf.is_gguf_filename`; `utils.models.model_config` and `core.inference.llama_cpp` carry documented mirrors because core deliberately does not import from hub. Every scanner, variant resolver, companion picker and cache-reuse path now goes through one of the three. - Header check: the metadata reader records whether the file actually starts with `GGUF`, so an invalid file is refused before llama-server is spawned. A definite non-GGUF header is kept distinct from an unreadable one, so a truncated but valid GGUF still falls through to llama-server as before. - Failure message: `invalid magic characters ... expected 'GGUF'` is now classified ahead of the generic invalid-file/OOM fallback, and names Finder metadata explicitly when the path is AppleDouble. Memory is never mentioned. Fixes unslothai#8566
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e50dd737e9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b398867769
ℹ️ 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".
b398867 to
e51f3a1
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! 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". |
e51f3a1 to
59ae87b
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59ae87b708
ℹ️ 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".
59ae87b to
d586aea
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d586aea8f8
ℹ️ 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".
d586aea to
8f09175
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. 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". |
|
Reviewed this against #8566 and it holds up well. The design is right, and I want to record the checks rather than just say "looks good". VerifiedReproduced the bug on On this branch both return the real file, and an end-to-end run over a directory containing actual AppleDouble bytes ( The predicate is the part I was most prepared to argue with, and it is correct. Missing, empty and 2-byte files all return False rather than raising, so an unreadable candidate is not mistaken for metadata. Splitting the work between the magic-checking predicate for local paths and the name-only Full One thing worth fixing
The function-level It matters because any job that runs "the PR's changed test files together" hits it. I ran exactly that across ubuntu-latest, macos-14 and windows-latest and got the same two failures on all three. Importing For completeness, the third failure in that batch, Smaller suggestionThe new classifier branch is a real improvement over the memory catch-all. The wording ends on "Re-download the model, or pick a different file", which is right for a generic non-GGUF but is not the remedy for the case that prompted this. For the exFAT reporter the fix is Not blocking
Malicious scan clean, static and both LLM passes. Nice piece of work, and thanks for chasing the AppleDouble angle. Happy to merge once the test-isolation point is sorted. |
On a volume without native extended attributes -- exFAT, FAT, most SMB and NFS shares -- macOS keeps a file's xattrs and resource fork in a companion beside it, named `._<name>` and holding AppleDouble data with magic `0x00051607`. The companion carries the described file's extension, so `model-Q4_K_M.gguf` gets `._model-Q4_K_M.gguf` and `train.jsonl` gets `._train.jsonl`. It answers every name-shaped question the way the real file does, and because `.` sorts ahead of alphanumerics, first-match-wins selection prefers it. `pathlib.Path.glob` matches these names where the older `glob.glob` hid them, which is why some call sites were affected and others were not. The reported symptom is a GGUF model that will not load from a cache on exFAT, but the same defect reaches datasets, images, video, PDFs, `.py` sources and Arrow caches. Training runs died at data load; LoRA training was refused because of a file the user cannot see; `repo_ships_transformers_weights` accepted `._consolidated.safetensors` by extension and the ignore-pattern builder then stripped every `consolidated*` file, so a Mistral-style repo downloaded "successfully" with no weights on disk; `expected_files_from_snapshot_dir` baked companions into the completion contract, so a finished download read as partial forever once macOS cleaned them up; and the RAG folder scan embedded AppleDouble bytes as retrievable chunks cited in answers. This replaces the filename rule this branch carried earlier, which treated every `._`-prefixed name as not-a-GGUF. That rule refuses a file for its name alone, and a user is free to name a real model `._model.gguf` -- on a volume with native xattrs nothing else would ever create that name, and the file is a perfectly good GGUF. Only the bytes can tell the two apart. `utils/appledouble.py` holds the format-agnostic primitives. A path is metadata only when its name carries the `._` prefix *and* its first four bytes are the AppleDouble magic. The prefix alone is never enough: a user's own `._model.gguf` is a real model and must stay loadable. Only `._` names are ever opened, so a scan costs one extra read per companion rather than one per file. Remote listings carry names with no bytes to read, so they are settled by pairing instead: a `._x` is dropped only when its subject `x` is present in the same listing, which leaves a sole candidate usable whatever it is called. GGUF passes a subject key so a split quant's shards count as one family. Where consumers shared a walk, the filter went into the walk rather than into each caller: `cached_repo_files` for the cache inventory and model routes, `_safe_dataset_image_path` for the dataset image serve, caption and delete endpoints, and `dataset_files_in_dir` for local dataset resolution across GPU, MLX and embedding training. One error message changes with it. llama.cpp prints `invalid magic characters: '<4 bytes>', expected 'GGUF'` when the file it opened is not a GGUF, formatting the bytes it found with `%c`, so a binary header arrives as unprintable characters. Nothing in the classifier matched that line, so it fell through to the generic "check that the GGUF file is valid and you have enough memory" -- which sent people to free memory that was never short, over a file that was never a model. That failure is now named. The message says the file is not a GGUF and gives the model path for context without claiming that path is the bad file: llama-server reports this while opening the vision projector and the drafter too, and only the main model's path reaches the classifier. The rule sits after the loader diagnostics, since a dyld failure explains why the wrong bytes were reached and stays the more specific answer when both appear. Fixes unslothai#8566
8f09175 to
c7ec308
Compare
|
Thanks — the reproduction on Test isolationFixed, but not where you suggested, because the first suggestion made it worse and I want to show the measurement rather than assert it. Hoisting to module scope turns the two test failures into a whole-module collection error: Reaching the helpers without The cause is That makes this pre-existing and repo-wide rather than specific to my file — 163 files under So I fixed the cause: the stub now installs only when the real import genuinely fails. Reverting the conftest to bare Classifier wordingTaken as you framed it. The generic text is unchanged and a targeted line replaces only the closing remedy when the model path starts with
Phrased about the volume rather than the file on purpose, since llama-server may have choked on the projector or the drafter while the main model merely happens to sit on the same disk — The two I am leaving
Head is now |
|
Confirmed this still hits is_gguf_filename in studio/backend/hub/utils/gguf.py, where an AppleDouble sidecar passes the extension test and sorts ahead of the real quant, and that the invalid-magic output has no classifier branch today. Will get this reviewed. |
|
@Lyxot could you take a look at this one? |
|
Both points are addressed on the current head ( Test isolation. Reproduced exactly at Same invocation at head: The fix is in Classifier wording. That is what head does. The branch keeps "Re-download the model, or pick a different file." and substitutes the For the record on regressions: All four Codex items are correct catches and all four are fixed at head; I reproduced each one before agreeing to it. Separately, every org workflow on this PR is sitting at |
* Keep path_utils off PEP 604, which the 3.9 floor gate rejects `Core` has been failing on main at 'python floor compatibility (HARD GATE)', all three HF/TRL cells: 36 studio files now evaluate PEP 604 unions on the floor, up from 35 The 36th is studio/backend/utils/paths/path_utils.py, from #8919. That change is right; it just added subject_key: Callable[[str], object] | None = None to a module with no `from __future__ import annotations`, so the annotation is evaluated at import and `X | None` is a TypeError on the declared 3.9 floor. Uses Optional[...], which the module already imports, rather than adding the future import. Five other modules import this one, and deferring every annotation in it is a wider change than the one union needs -- the ratchet's own docstring warns that converting a file wants Studio booted, because FastAPI resolves annotations when it builds endpoints. Verified on main: offenders back to 35 against a debt of 35, the whole test_python39_compatibility.py suite passes (8), and the AppleDouble guards the original change added still pass (9). * [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>
…ard them (unslothai#9353) * Put back the AppleDouble GGUF filters unslothai#9074 reverted, and guard them Backend CI's 3.13 leg has six failures on main beyond the two in unslothai#9348. Three separate causes, all of them in the tests or in a merge resolution rather than in anything a PR meant to change. 1. llama_cpp.py lost every line of unslothai#8919 ------------------------------------------------------------------------ unslothai#8919, "never pick a macOS AppleDouble sidecar as a GGUF", touched 49 files. 18b97f8 ("keep and search the turns rolling context evicts", unslothai#9074) reverted all five of its hunks in core/inference/llama_cpp.py and nothing else. That is the signature of a branch cut before unslothai#8919 landed and merged whole-file: unslothai#9074 is a rolling-context PR, its diff carries no replacement for any of this, and it did not revert the tests, which is the only reason CI said anything at all. Checked the rest of unslothai#8919 line by line against main: of the 49 files it changed, llama_cpp.py is the only one that lost anything. All five hunks are restored here, and the file now contains every line unslothai#8919 added. Four of the five are the selection sites, and they are the half that was silent: _gguf_snapshot_files a local walk now skips the companion on its bytes _pick_mmproj "._mmproj-F16.gguf" satisfied the F16 preference and sorted ahead of the real adapter _pick_dspark every GGUF under dspark/ qualifies, so a sidecar ranked equal to its sibling and sorted first; also back at module level, where unslothai#8919 put it because it is handed a live repo listing as well as a snapshot the HF list_repo_files a repo listing has no bytes to read The fifth is the one CI caught: the "invalid magic characters" branch in _classify_llama_start_failure. Without it a user who points llama-server at a "._model.gguf" sidecar gets "Check that the GGUF file is valid and you have enough memory" and goes off to free memory they already have, which is issue unslothai#8566 exactly. Restored verbatim at its original anchor, below the dyld branch so test_a_dyld_failure_still_outranks_it keeps holding. Five new tests in test_appledouble_guards.py cover the four selection sites behaviourally, including that _pick_dspark is reachable from module scope, since nesting it back inside the method is how it was reverted. Each also pins that a file a user genuinely named "._something" still resolves: nothing may be refused for its name alone. Mutation-tested by restoring llama_cpp.py to its current main state: 7 failed, all five new guards plus the two that were already red. 2. The refactor guard baseline ------------------------------------------------------------------------ unslothai#9074 added RAG_SEARCH_TOOLS to core/inference/tool_call_parser, which is one of the two strict modules the guard runs with additions_matter, so a new public name there is a deliberate re-baseline by design. The symbol is correct: three modules import it, and test_conversation_recall_injection.py already pins its value. Re-baselined through the tool's own `snapshot`, then trimmed to just this entry. The full snapshot also absorbed 52 unreviewed new names in core.inference.llama_cpp, 3 in safetensors_agentic and 151 lines of patch_targets churn. Those are additive drift the guard tolerates on purpose, so recording them fixes nothing and pins symbols nobody looked at. Mutation-tested: an added throwaway public symbol still turns both tests red, so the strict-addition behaviour survived the re-baseline. 3. The research opt-out payload ------------------------------------------------------------------------ 28b8880 ("compact a chat by resetting the epoch", unslothai#9162) added tools_withheld to the generation kwargs. test_the_opt_out_changes_nothing_a_default_install_does compared the two payloads whole, which was right when every kwarg was model-facing. tools_withheld is not. Its only consumer is _can_reset_epoch, which picks a compaction strategy; it never reaches the prompt, the sampling params or the tool catalogue. And it has to differ: without the opt-out a compacted thread can still re-admit search_conversation through the checkpoint-repair branch, so resetting the epoch is safe, while with the opt-out that repair is closed on this turn and every identical turn after it, so a reset would strand the epoch behind a tool that never arrives. Forcing the two equal is what would break Deep Research, which sends a real thread_id. The 17 model-facing fields are identical and neither side carries a `tools` key, so the asymmetry this test exists to catch is not present. Rather than just excluding the key, it is now pinned in both directions plus an explicit no-tool-catalogue assertion, so the file catches more than before. Mutation-tested: pinning tools_withheld = False in routes/inference.py fails the new assertion for both parameters. Verification ------------------------------------------------------------------------ test_appledouble_guards.py, test_llama_cpp_start_failure_classification.py, test_refactor_guard.py, test_research_internal_call_tool_gate.py and hub/tests/test_model_services.py: 508 passed, from 6 failed. Wider sweep over tests/test_llama*.py and the hub model services: no collateral. * [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>

Fixes #8566 — the GGUF-picker half that was left open after #8574.
Motivation
On volumes that cannot store Mac file metadata (exFAT, FAT32, many network shares), macOS writes a ~4 KB AppleDouble file beside every file, named
._<name>and holding magic0x00051607. The companion carries the described file's extension, somodel-Q4_K_M.ggufgets._model-Q4_K_M.gguf. Studio accepted any name ending in.ggufand took the first match in sorted order, and.sorts ahead of alphanumerics, so the sidecar won every time.llama-server then opened 4 KB of Finder metadata, found no GGUF header, and exited. The failure classifier had no branch for that output, so it fell through to its catch-all and blamed the file or the user's memory — which is why a 1–3B model on a 16 GB machine reported an out-of-memory-shaped error. The same selection bug picked
._mmproj-*for vision models, and every._part of a split model.The companion answers every name-shaped question the way the real file does, so nothing about this is specific to GGUF.
pathlib.Path.globmatches these names where the olderglob.globhid them, which is why some call sites were affected and others were not:repo_ships_transformers_weightsaccepted._consolidated.safetensorsby extension, and the ignore-pattern builder then stripped everyconsolidated*file — a Mistral-style repo downloaded "successfully" with no weights on disk;expected_files_from_snapshot_dirbaked companions into the completion contract, so a finished download read as partial forever once macOS cleaned them up;Changes
._-prefixed name as not-a-GGUF. That refuses a file for its name alone, and a user is free to name a real model._model.gguf— on a volume with native xattrs nothing else would ever create that name, and the file is a perfectly good GGUF. The rule is now: a path is Finder metadata when its name carries the._prefix and its first four bytes are the AppleDouble magic. Only._names are ever opened, so a scan costs one extra read per companion rather than one per file. The predicates live inutils.paths.path_utils, beside the other path questions.._xis dropped only when its subjectxis present in the same listing and the same directory; a sole candidate survives whatever it is called. GGUF passes a subject key so a split quant's shards count as one family.cached_repo_filesfor the cache inventory and the model routes,_safe_dataset_image_pathfor the dataset image serve, caption and delete endpoints, anddataset_files_in_dirfor local dataset resolution across GPU, MLX and embedding training.invalid magic characters ... expected 'GGUF'is now classified ahead of the generic invalid-file/OOM fallback. llama.cpp formats the four bytes it found with%c, so a binary header arrives as unprintable characters and nothing matched the line. The message gives the model path for context without claiming that path is the bad file: llama-server reports this while opening the vision projector and the drafter too, and only the main model's path reaches the classifier. Memory is never mentioned.Deliberately out of scope: size- and count-only cosmetics that break nothing — a few KB on a VRAM estimate, a progress bar reading 99.9%, a "last downloaded" sort order.
Tests
test_appledouble_guards.pycovers what every consumer shares rather than one case per call site: the predicates, the shadow pairing including a split quant's shard family, the cache walk that follows a snapshot entry to the blob it links to, the dataset walk, and the two selections a sidecar used to win outright — a drafter budget priced off a shared basename, andconsolidated*weights hidden by their companion. The classification path is covered in the suite that already owns it: the failure is reported as not-a-GGUF, it does not name an ordinary main model as the bad file, and a dyld failure still outranks it.The one-line guards at individual call sites — upload block quota, dataset image gate, immediate weight file, RAG folder scan, remote code scan, delete cleanup, companion family names, named dataset file, and the cached-files walk in
routes/models— are not each covered by a test here. Each was verified during development by reverting the guard and re-running the suite. They are visible in review, but CI will not notice if a later edit drops one; flagging that rather than leaving it implied.Run on macOS from
studio/backend:hub/tests/: 554 passed, 1 pre-existing failure (a case-insensitive-filesystem assumption)6f443b5c, same invocation, run sequentially: 288 failures on each side, identical sets — the branch introduces none and fixes none. This environment carries a large pre-existing failure count (macOS host, blocked network), so a raw pass/fail number proves nothing; the set comparison is the instrument.