Feat(mlx): GGUF multi-quant - #976
Conversation
…d_gguf
The CUDA path documents quantization_method as accepting a list so one
export can emit several GGUFs (unsloth/save.py:1323, normalized 1342-1352).
On MLX that argument reached a dict lookup keyed by the whole value:
quant_type = quant_map.get(quantization_method, quantization_method)
so a list raised TypeError: unhashable type: 'list'. The crash is reachable
from the user-facing API: model.save_pretrained_gguf is bound in loader.py
and forwards quantization_method untouched.
Resolve the argument to an ordered list of llama.cpp types instead, and
quantize each target off the single shared intermediate. The merge and the
convert_hf_to_gguf pass are the expensive steps and already ran once, so
additional targets only cost their own llama-quantize pass.
Behaviour notes:
- Targets follow the CUDA rule (save.py:1528): everything that is not the
intermediate itself gets a llama-quantize pass. Full-precision targets are
not exempt, since llama-quantize emits f16/bf16/f32 too - exempting them
would silently drop the f16 in ["f16", "q4_k_m"].
- The pre-existing single-target contract is kept unchanged: a lone
full-precision request is satisfied by whatever convert_hf_to_gguf emitted,
with no quantize pass, even when first_conversion names another dtype.
- Duplicates are deduplicated (preserving order); output filenames derive
from the quant type alone, so a repeat would re-quantize onto the same
path. This deviates from CUDA, which does not deduplicate.
- The intermediate is removed only once something was quantized off it and
it is not itself a requested type, matching CUDA's quants_created /
want_full_precision pair. When no quantize pass runs the converted file IS
the export and always stays.
- Scalar strings keep their exact previous semantics.
Quant names are still passed through to llama-quantize unvalidated, as
before; an upfront allowlist would duplicate ALLOWED_QUANTS from the
unsloth repo and is left as a follow-up.
tests/test_mlx_save_export_edge_cases.py was only ever collected, and as the surrounding comment notes, `pytest tests/ --collect-only` proves only that a file imports. The MLX GGUF export path therefore had no executed coverage, the same gap that let test_mlx_finetune_last_n_layers stay broken from unslothai#669 until unslothai#739. The file meets this gate's bar: deterministic, CPU-pure, and well under a second for the whole module. It fakes llama.cpp end to end, so no binary, no network and no weights are involved.
![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: beade423b0
ℹ️ 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".
| legacy_single_full_precision = ( | ||
| len(quant_types) == 1 and quant_types[0] in _GGUF_FULL_PRECISION_TYPES | ||
| ) |
There was a problem hiding this comment.
When the caller passes a list/tuple that deduplicates to one full-precision target plus an explicit different first_conversion, this condition treats it as the legacy scalar path and skips llama-quantize. For example, quantization_method=["f16"] with first_conversion="bf16" leaves only Model.BF16.gguf, even though the new list-form contract says the requested targets should be emitted; keep the legacy exemption only for an original scalar argument, not for singleton lists.
Useful? React with 👍 / 👎.
| if len(quant_types) == 1 and quant_types[0] in _GGUF_FULL_PRECISION_TYPES: | ||
| first_conversion = quant_types[0] | ||
| else: | ||
| # k-quants and q8_0 go through a bf16 intermediate, then llama-quantize | ||
| first_conversion = "bf16" |
There was a problem hiding this comment.
For a list containing only full-precision targets, this fallback still chooses a bf16 intermediate whenever there is more than one deduplicated target. A request like quantization_method=["f32", "f16"] therefore converts the model to *.BF16.gguf first and then creates the requested *.F32.gguf from already-rounded bf16 data, unlike the scalar "f32" path that converts directly to f32; choose a requested full-precision target (preferably f32 when present) as the intermediate when there are no k-quant/q8 targets.
Useful? React with 👍 / 👎.
| # and it is not itself a requested type (e.g. ["bf16", "q4_k_m"]). When | ||
| # nothing was quantized it IS the export, so it always stays - including | ||
| # a full-precision target reached through an explicit first_conversion. | ||
| if quantized_any and first_conversion not in quant_types: |
There was a problem hiding this comment.
When a caller passes through a llama.cpp-style case variant such as "BF16" in a list, the converter writes the same *.BF16.gguf path as the bf16 intermediate, but this case-sensitive membership check decides the intermediate was not requested and deletes it after any quantize pass. For quantization_method=["BF16", "q4_k_m"], the requested BF16 artifact is removed; compare normalized quant names or the actual output path before deleting.
Useful? React with 👍 / 👎.
…i-quant # Conflicts: # .github/workflows/consolidated-tests-ci.yml
GGUF output paths are {base}.{name.upper()}.gguf, so "BF16", " bf16 " and
"bf16" all name the same file on disk. The export then compares quant names
as plain strings in three places - which intermediate to convert to, which
targets still need a llama-quantize pass, and whether the intermediate is
scratch or a requested artifact - so an unfolded name made those three
disagree about which file is which.
quantization_method=["BF16", "q4_k_m"] ran llama-quantize with input and
output both pointing at *.BF16.gguf, then deleted that requested artifact as
if it were scratch. first_conversion is the other half of the same spec and
had the same defect, so it is folded identically; a non-string
first_conversion also used to surface as AttributeError: 'list' object has no
attribute 'upper' AFTER the LoRA merge and convert, and is now rejected
before them like a bad quantization_method already was.
llama.cpp's own entry point already normalizes this way: check_quantization_type
(unsloth_zoo/llama_cpp.py:2174) lowercases before validating, which is why the
converter accepts "BF16" while this module's comparisons did not.
The full-precision exemption in Step 6 keyed off the deduplicated length alone, so quantization_method=["f16"] with first_conversion="bf16" took the pre-PR scalar path, skipped llama-quantize entirely and left only Model.BF16.gguf - dropping the single target the caller asked for. Upstream has no full-precision exemption: it quantizes every requested method that is not the intermediate (unslothai/unsloth unsloth/save.py:2067-2070). The exemption here exists only to preserve pre-PR behaviour of the string form, so it is now gated on the caller having actually passed a string. The list form follows upstream. _normalize_gguf_quantization_methods returns that flag rather than the use site re-testing the raw argument, so the "was this a sequence" predicate stays in one place.
A multi-target export fell back to a bf16 intermediate whenever more than one type was requested, so quantization_method=["f32", "f16"] converted the model to *.BF16.gguf and then built the requested *.F32.gguf out of bf16-rounded data - f32-shaped, but carrying only 8 mantissa bits of real signal. The scalar "f32" path converted straight to f32, so the list form was strictly worse than the single form it was meant to generalize. bf16, f16 and f32 are all exactly representable in f32 (bf16 is literally the high 16 bits of an IEEE f32), so an f32 intermediate can never round the weights the other targets are quantized from - k-quant and q8_0 outputs are unchanged by this. Deliberately limited to f32. f16 and bf16 are not orderable against each other - bf16 has the wider exponent, f16 the longer mantissa - so promoting a requested f16 to the intermediate would clip the range of a bf16 checkpoint. A requested f16 without an f32 still goes through bf16, and that non-change is pinned by a test.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a42f112d46
ℹ️ 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".
| # Step 6: Quantize every requested target off the shared intermediate. | ||
| # The merge and convert above are the expensive steps and already ran | ||
| # once, so extra targets only cost their own llama-quantize pass. | ||
| base_gguf = f"{output_base}.{first_conversion.upper()}.gguf" |
There was a problem hiding this comment.
When a multi-target request includes f32 plus any quantized target for a model whose f32 GGUF crosses the default --split-max-size (50GB), convert_to_gguf writes and returns sharded files instead of {output_base}.F32.gguf (see unsloth_zoo/llama_cpp.py:2567-2607), but this code ignores that return value and still passes the unsplit filename to llama-quantize. The new f32 promotion makes this reachable even for ~13B-class models where a BF16 intermediate would have fit, so requests like ["f32", "q4_k_m"] fail at the quantize step despite successfully producing the F32 shards.
Useful? React with 👍 / 👎.
save_pretrained_gguf built the llama-quantize input as
f"{output_base}.{first_conversion.upper()}.gguf" and ignored what
convert_to_gguf returned. That name only exists when the intermediate fits
under --split-max-size (50GB by default); above it convert_hf_to_gguf writes
SHARD_NAME_FORMAT files ({base}-00001-of-000NN.gguf) and the plain name is
never created, so llama-quantize was handed a path that does not exist and the
export failed after the merge and convert had already succeeded. The same
assumption also left the shards behind when the intermediate was scratch,
since the cleanup only unlinked the unsplit name.
Promoting the intermediate to f32 whenever f32 is a requested target (a42f112)
made this reachable a model class earlier: a ~13B f32 GGUF crosses 50GB where
its bf16 form would not, so ["f32", "q4_k_m"] hit it on hardware where the old
bf16 path was fine.
_resolve_gguf_intermediate prefers the paths convert_to_gguf returned and falls
back to scanning the output directory, and quantizing reads the first shard -
llama-quantize's loader picks up split.count from it and loads the rest
(llama.cpp src/llama-model-loader.cpp, llama_get_list_splits), writing an
unsplit output because --keep-split is not passed.
|
Both Codex points are right. The singleton-list one was already fixed in The sharded intermediate one is fixed in Not verified on real weights: reproducing the split needs a ~13B model at f32, over 50GB on disk and well past this machine's 11.84GB Metal working-set ceiling for the merge, so the shard behaviour is exercised through the converter stub and checked against llama.cpp's loader source rather than an actual sharded GGUF. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
Confirmed the MLX path still keys quant_map on the raw argument at unsloth_zoo/mlx/utils.py:14654, so the documented list form of quantization_method never worked there. Will get this reviewed. |
2 participants
save_pretrained_ggufcrashes on the list form ofquantization_methodthat the CUDA pathdocuments, so an MLX user cannot produce more than one GGUF per export.
The bug
CUDA states the contract in the signature itself (
unsloth/save.py:1323):and normalizes it to a list at
save.py:1342-1352. On MLX the argument reached a dict lookup keyedby the whole value:
A list is unhashable, so this raises:
It is reachable from the user-facing API, not just the internal helper:
model.save_pretrained_ggufis bound in
loader.pyand forwardsquantization_methoduntouched.Beyond the crash, the single-target restriction is wasteful. The LoRA merge and the
convert_hf_to_ggufpass are the expensive steps, and both are independent of the target quant —producing q4_k_m and q8_0 meant running the whole pipeline twice.
What changed
quantization_methodresolves to an ordered list of llama.cpp types, and each target is quantizedoff the single shared intermediate. Extra targets now cost only their own
llama-quantizepass.Scalar strings keep their exact previous semantics.
push_to_hub_ggufinherits the behaviour andalready uploads every
*.ggufit finds.Deliberate decisions
save.py:1528): everything that is not the intermediate itselfgets a
llama-quantizepass. Full-precision targets are not exempt —llama-quantizeemitsf16/bf16/f32 too, so exempting them would silently drop the
f16in["f16", "q4_k_m"].satisfied by whatever
convert_hf_to_ggufemitted, with no quantize pass, even whenfirst_conversionnames a different dtype. Only the list form is new behaviour.quants_created/want_full_precisionpair. It is removed only once something was quantized off it and it is notitself a requested type, so
["bf16", "q4_k_m"]keeps the bf16 file. When no quantize pass runs theconverted file is the export and always stays. The previous code deleted unconditionally whenever a
quantize ran, which a naive loop would have turned into deleting a requested artifact.
alone, so a repeated entry would re-run
llama-quantizeonto a path it just wrote. CUDA does notdeduplicate; this is a small intentional divergence.
mean duplicating
ALLOWED_QUANTSfrom theunslothrepo into this one, where the two copies woulddrift. Left as a follow-up rather than smuggled into this PR. Argument-shape errors (non-string
entries, empty list) are still rejected before the expensive merge.
Tests
Twelve cases in
tests/test_mlx_save_export_edge_cases.py, covering: the list form producing everyquant from one merge and one convert; tuple input; per-element alias resolution; deduplication;
full-precision targets that are not the intermediate (
["f16","q4_k_m"]and["bf16","f16"]);intermediate retained when also requested; intermediate retained when nothing was quantized; explicit
first_conversionwith a list; scalar-string regression; and forwarding through bothmodel.save_pretrained_ggufandpush_to_hub_gguf.The bug was reproduced on the parent commit before fixing — the new list test fails there with
TypeError: unhashable type: 'list'atutils.py:12488.Across the 789-test MLX + GGUF suite the failing set is byte-identical to the base commit; the only
delta is the twelve added tests. The existing GGUF assertions are untouched — the shared scaffold
gained an accumulating
quantize_callslist alongside the pre-existing single-callquantize_kwargs.CI
Second commit adds the file to the behavioral-gate job. It was previously only collected, and as that
step's own comment says,
pytest tests/ --collect-onlyproves only that a file imports — the samegap that let
test_mlx_finetune_last_n_layersstay broken from #669 until #739. The file isdeterministic, CPU-pure, fakes llama.cpp end to end (no binary, no network, no weights) and runs in
well under a second.
I fixed one stale number while pulling this up — the Tests section said "ten added tests" from before
the CUDA-rule fix added two more.