PR #697 cross-OS validation (do not merge)#156
Conversation
Prefer HF vision aliases when inverting mlx-vlm sanitizers, while keeping same-name tensor rewrites as a fallback. This preserves Gemma3 patch embedding layout fixes and avoids stopping early on Qwen-family MLX vision_tower names.
Use loaded VLM model instances when replaying mlx-vlm sanitizers for GGUF tensor rewrites, while keeping the config-derived class pipeline as a fallback. This covers models whose top-level sanitizer delegates through submodules, such as GLM-OCR.
This reverts commit d8cdf89. Also removes the related regression coverage and later helper hardening now that bug-4 is out of scope.
…n jobs Sweep .github/workflows/* (mlx-ci.yml is broken on PR head -- imports old flat unsloth_zoo.mlx_loader path; subpackage migration moved it to unsloth_zoo.mlx.loader). Add three focused per-OS jobs covering the tests/test_mlx_save_export_regressions.py suite that ships with PR unslothai#697: - pr697-macos.yml -- macos-14 Apple Silicon, .[mlx] extras + subpackage import smoke + 23 save/export regressions + shim smoke (highest-signal job). - pr697-ubuntu.yml -- ubuntu-latest, CPU torch + [core] + mlx_simulation shim, full pytest plus --collect-only. - pr697-windows.yml -- windows-latest, shim path, Windows-native pathlib coverage for the new save/export helpers. Staging branch only. Do not merge.
![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 comprehensive regression tests and several improvements to the MLX save and GGUF export pipelines. Key changes include fixing path resolution for package-layout converters in llama_cpp.py, adding robust VLM processor repair mechanisms in loader.py, and implementing MLX VLM tensor rewriting for GGUF compatibility in utils.py. The reviewer feedback focuses on ensuring cross-platform encoding safety by explicitly specifying encoding="utf-8" when reading and writing JSON configuration and index files in utils.py.
| config_path = path / "config.json" | ||
| if not config_path.exists(): | ||
| return 0 | ||
| with open(config_path, "r") as f: |
There was a problem hiding this comment.
On Windows, opening files without specifying encoding="utf-8" can lead to UnicodeDecodeError if the file contains non-ASCII characters (which is common in model configs). Please specify encoding="utf-8" when reading the config file.
| with open(config_path, "r") as f: | |
| with open(config_path, "r", encoding="utf-8") as f: |
| sanitize_steps = _build_mlx_vlm_sanitize_pipelines(config, model=model) | ||
| if not sanitize_steps: | ||
| if config_changed: | ||
| with open(config_path, "w") as f: |
There was a problem hiding this comment.
On Windows, writing files without specifying encoding="utf-8" can lead to encoding mismatches or UnicodeEncodeError if the config contains non-ASCII characters. Please specify encoding="utf-8" when writing the config file.
| with open(config_path, "w") as f: | |
| with open(config_path, "w", encoding="utf-8") as f: |
|
|
||
| index_path = path / "model.safetensors.index.json" | ||
| if rewritten and index_path.exists(): | ||
| with open(index_path, "r") as f: |
There was a problem hiding this comment.
On Windows, opening files without specifying encoding="utf-8" can lead to UnicodeDecodeError if the index file contains non-ASCII characters. Please specify encoding="utf-8" when reading the index file.
| with open(index_path, "r") as f: | |
| with open(index_path, "r", encoding="utf-8") as f: |
| ) | ||
| weight_map[new_name] = shard | ||
| index_data["weight_map"] = dict(sorted(weight_map.items())) | ||
| with open(index_path, "w") as f: |
There was a problem hiding this comment.
On Windows, writing files without specifying encoding="utf-8" can lead to encoding mismatches or UnicodeEncodeError if the index contains non-ASCII characters. Please specify encoding="utf-8" when writing the index file.
| with open(index_path, "w") as f: | |
| with open(index_path, "w", encoding="utf-8") as f: |
| json.dump(index_data, f, indent=4) | ||
|
|
||
| if config_changed: | ||
| with open(config_path, "w") as f: |
There was a problem hiding this comment.
On Windows, writing files without specifying encoding="utf-8" can lead to encoding mismatches or UnicodeEncodeError if the config contains non-ASCII characters. Please specify encoding="utf-8" when writing the config file.
| with open(config_path, "w") as f: | |
| with open(config_path, "w", encoding="utf-8") as f: |
pyproject gates torch off on darwin+arm64 (mlx is the native path), but tests/mlx_simulation/__init__.py unconditionally imports torch. The autouse _install_mlx_torch_shim fixture in test_mlx_save_export_regressions.py runs on every test in the file, so torch is required on macos-14 too.
|
All three OS jobs green on commit c9fc1960:
Closing per staging-fork convention; never merged. |
3 participants
Staging CI runs for unslothai/unsloth-zoo#697 (`Lyxot:fix/mlx-save-gguf-export-parity` -- 11 MLX save / GGUF export parity fixes + 23 regressions in `tests/test_mlx_save_export_regressions.py`).
No Apple Silicon hardware locally, so this branch validates the PR on real `macos-14` GitHub Actions runners (the highest-signal target) plus `ubuntu-latest` and `windows-latest` via the `tests/mlx_simulation/` torch-spoof shim.
Branches
Jobs
Sweep rationale
`.github/workflows/mlx-ci.yml` on PR head still imports the pre-migration flat module path (`unsloth_zoo.mlx_loader`); PR unslothai#697 lives in the subpackage `unsloth_zoo.mlx.loader`. Left as-is the workflow would hard-fail unrelated to PR unslothai#697's diff, so the staging branch sweeps every existing workflow and substitutes the three jobs above. Precedent: PR #154 on this fork.
Lifecycle
Throwaway. Close (don't merge) once `macos-14` + `ubuntu-latest` are green and PR unslothai#697 upstream lands.