PR5727 validation: targeted CI for MLX base export fix#143
PR5727 validation: targeted CI for MLX base export fix#143danielhanchen wants to merge 3 commits into
Conversation
Apply the 2-line fix from unslothai#5727 (add save_method="merged_16bit" to the two MLX save_pretrained_merged calls in studio/backend/core/export/export.py:export_base_model) and add three targeted workflows on this staging fork to validate the change cross-platform without burning credits on the main repo. Workflows added: - pr5727-mlx-base-export.yml (macos-14): loads gemma-3-270m-it as a non-PEFT model and runs tests/studio/run_pr5727_non_peft_base_export.py which (a) confirms the pre-PR error reproduces, (b) confirms the fix succeeds, (c) drives ExportBackend.export_base_model end-to-end. - pr5727-linux-import.yml (ubuntu-latest): AST + py_compile + ExportBackend import + tests/studio/test_export_output_path_contract.py. - pr5727-windows-import.yml (windows-latest): AST + py_compile + ExportBackend import. The rest of .github/workflows/* is stripped on this branch to keep CI fast and well below the 5-concurrent-Windows-runner cap; only the three PR5727 workflows trigger on push to this branch. Tests added: - tests/studio/run_pr5727_non_peft_base_export.py
![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 fixes a bug in MLX base model exports by explicitly setting the save method to "merged_16bit" and adds a comprehensive smoke test for this scenario. It also refines the installation process by ensuring pydantic and its core dependencies are correctly resolved before applying no-dependency requirements. Additionally, library glob patterns for prebuilt binaries were updated for better maintainability. Feedback suggests removing the new minimum length requirement for current passwords in the authentication form to avoid locking out users with legacy credentials. It was also recommended to move the global weight_decay change in the RL trainer to a separate PR as it is out of scope.
| (currentPassword.length < 8 || | ||
| newPassword.length < 8 || | ||
| newPassword !== confirmPassword || | ||
| currentPassword === newPassword); |
There was a problem hiding this comment.
Enforcing a minimum length of 8 characters on the currentPassword field is unnecessary and potentially problematic. While security policies should be strictly enforced for new passwords, the frontend should not block a user from attempting to authenticate or change their password if their existing one (e.g., a legacy password or a specific bootstrap credential) happens to be shorter. The backend is the appropriate place to validate the correctness of the current password, and the UI already enforces the length for the input field via minLength.
(newPassword.length < 8 ||
newPassword !== confirmPassword ||
currentPassword === newPassword);
| if (currentPassword.length < 8) { | ||
| setError( | ||
| currentPassword | ||
| ? "Current password must be at least 8 characters." | ||
| : "Unable to initialize setup. Reload the page and try again.", | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
As noted above, enforcing a length check on the current password during submission can lock out users with legacy credentials. It is safer to only check for the presence of the password before proceeding with the change request.
if (!currentPassword) {
setError("Unable to initialize setup. Reload the page and try again.");
return;
}
| "weight_decay": 0.01, | ||
| # LoRA decays A and B toward 0 so effective W = W_init + (alpha/r) * B @ A is pulled toward W_init, not 0 as in full FT. | ||
| # 0.001 keeps a small Frobenius prior |A|_F^2 + |B|_F^2 without measurably dragging the merged adapter back to base. | ||
| "weight_decay": 0.001, |
There was a problem hiding this comment.
This change reduces the default weight_decay from 0.01 to 0.001 for RL trainers. While the provided reasoning in the comments is sound for LoRA training (regularizing toward the base model rather than zero), this appears to be a global hyperparameter change that is unrelated to the primary objective of this PR ("MLX base export fix"). It would be better to track such tuning changes in a separate PR to maintain a clear commit history and avoid unintended side effects on other training workflows.
…poof; fix MLX phase 3 sandbox
CI failures from round 1:
* macOS (real MLX): phase_1 + phase_2 PASSED -- pre-PR error reproduced
verbatim and save_method="merged_16bit" succeeded. phase_3 failed
because resolve_export_dir() rejects paths outside
$UNSLOTH_STUDIO_HOME/exports/. Fix: pin UNSLOTH_STUDIO_HOME to a
workdir subdir and pass a relative export name "pr5727_phase3".
* Linux/Windows: unsloth_zoo.device_type.get_device_type() raises
NotImplementedError at import time because no torch accelerator is
visible. Fix: replace `python -c` with `pytest` so tests/conftest.py
auto-applies the existing CUDA spoof. Added a focused pytest module
tests/studio/test_pr5727_export_kwargs.py that asserts both
save_method="merged_16bit" kwargs land inside the _IS_MLX branch
of export_base_model AND imports ExportBackend cleanly.
Also drop the pull_request trigger from the MLX workflow so it only
fires once per push (was firing twice per round previously).
Round 2 Linux/Windows results: 10/11 pass on Linux, 4/5 on Windows. The only failure is test_export_backend_imports, which fails at the unsloth import chain on bitsandbytes (Linux) or triton (Windows) -- deps unrelated to this PR. Since the change is gated on _IS_MLX = False on those platforms and the patched lines never execute there, AST parse + py_compile + kwarg-presence + AST-walk-of-_IS_MLX-branch (all already passing) provide complete signal. The full ExportBackend import only adds value on macOS Apple Silicon, where the real MLX workflow already drives ExportBackend.export_base_model end-to-end via run_pr5727_non_peft_base_export.py. Mark test_export_backend_imports as skip-on-non-Apple-arm so the cross-platform job stays meaningful without needing the full unsloth dep stack.
2 participants
Throwaway PR on the staging fork to exercise unslothai#5727 end-to-end on real macOS Apple Silicon and to confirm no regression on Linux + Windows imports. Three workflows on this branch only:
pr5727-mlx-base-export.yml(macos-14): runstests/studio/run_pr5727_non_peft_base_export.pywhich loadsunsloth/gemma-3-270m-itas a non-PEFT model, confirms the pre-PR error reproduces withoutsave_method, confirmssave_method="merged_16bit"succeeds, and drivesExportBackend.export_base_modelend-to-end.pr5727-linux-import.yml(ubuntu-latest): AST + py_compile + ExportBackend import +test_export_output_path_contract.py.pr5727-windows-import.yml(windows-latest): AST + py_compile + ExportBackend import.All other
.github/workflows/*are stripped on this branch to keep CI fast and below the 5-concurrent-Windows cap. Not for merge.