Fix born-broken FakeModel fixture in mlx finetune_last_n_layers test#739
Merged
Conversation
test_get_peft_model_passes_finetune_last_n_layers_through has failed since it was introduced in #669: the trainable parameter summary that get_peft_model prints (added in #634) calls model.trainable_parameters() and model.parameters(), which the synthetic FakeModel never stubbed. CI never executed the test body (collect-only plus exclusion list), so the failure stayed hidden. Give the fixture the two methods returning empty trees, matching the fixtures in test_mlx_save_lora_adapters_filter, so the summary computes 0 of 0 params and the num_layers assertions are exercised as intended.
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the FakeModel class in tests/test_mlx_finetune_last_n_layers.py to include mock parameters and trainable_parameters methods. This change ensures that the trainable-parameter summary at the end of get_peft_model can walk these methods without raising errors or failing assertions. There are no review comments to evaluate, and I have no feedback to provide.
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.
danielhanchen
added a commit
that referenced
this pull request
Jun 11, 2026
…gate (#755) test_mlx_finetune_last_n_layers was born broken in #669 and stayed invisible until #739 because no CI job executed it: the version matrix only collects, the macOS MLX job runs the shim smoke test alone, and the zoo-specific CPU list does not include it. Add a small hard-gate step in repo-tests-cpu running it together with test_training_utils_use_cache (the use_cache disable/restore contract from #715). Both files are CPU-pure and run in under a second, and the job already installs the deps they need.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
test_get_peft_model_passes_finetune_last_n_layers_through has been failing since it was introduced in #669. The trainable parameter summary at the end of get_peft_model (unsloth_zoo/mlx/loader.py, added in #634) calls model.trainable_parameters() and model.parameters(), which the synthetic FakeModel in this test never stubbed, so every run dies with AttributeError before the function returns. Verified by running the test at 049a14d in a detached worktree: it fails identically there, so this was never a regression from later commits.
It went unnoticed because CI never executes this test body: consolidated-tests-ci.yml only does pytest --collect-only for this file, and mlx-ci.yml runs just the shim smoke test on macOS.
Fix is test-only: give FakeModel the two methods returning empty trees, matching the fixture convention already used throughout test_mlx_save_lora_adapters_filter.py. With empty trees the summary computes 0 of 0 params (the existing total > 0 guard avoids division by zero) and the test reaches its num_layers assertions as intended.
No library code changes, so there is no impact on any runtime path (CUDA, ROCm, XPU, MLX) or on transformers, TRL or vLLM version compatibility.
Full suite on this branch: 1134 passed, 51 skipped, 0 failed.