fix: save_pretrained unbound local active_adapters crash(#289)#374
fix: save_pretrained unbound local active_adapters crash(#289)#374umran666 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces two independent changes: it sets _hf_peft_config_loaded = False on merged models to prevent crashes during save_pretrained, and it adds safety checks to handle cases where self.model is None during model resets (introducing a persistent self.dtype attribute). The reviewer notes that according to the repository style guide, these semantically independent changes must be split into separate pull requests.
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.
| # If a prior model load was interrupted/cancelled mid-process, self.model will be None. | ||
| current_model = None | ||
| if self.model is not None: | ||
| current_model = getattr(self.model.config, "name_or_path", None) |
There was a problem hiding this comment.
According to the Repository Style Guide (Rules 9 and 10), a pull request must implement only one change, and semantically independent changes must be split into separate PRs.
The changes here to handle self.model is None in reset_model (and the associated introduction of self.dtype) are semantically independent of the fix for the save_pretrained crash (which is resolved by setting _hf_peft_config_loaded = False in get_merged_model). Please split these changes into a separate pull request.
References
- Pull requests should implement one change, and one change only. PRs containing multiple semantically independent changes must be split into multiple PRs. (link)
4c35dc1 to
3a506b1
Compare
|
I've never seen such a crash. Under which conditions does this happen? |
|
Check issue #289 you get any idea |
It happens when saving a merged model. |
I already commented in that issue that I don't understand what the issue is. How can I reproduce this? |
To reproduce: What's going on: merge_and_unload() removes all the LoRA layers from the model, but it doesn't clear the The fix just sets that flag to False right after merging so save_pretrained() takes the normal save path instead of the PEFT one. Same error reported in #289. |
I've done that dozens if not hundreds of times, and I've never seen a crash. I almost always choose "merge". And all models use LoRAs in Heretic, it's the only ablation method we support. This is not it. |
|
Ah, figured it out. It only happens if the model path you load is already a LoRA folder (with adapter_config.json) instead of a raw base model. When you pass a LoRA folder to Then If you always load raw base models, the flag is False so you never see it. |
|
That doesn't make sense. Heretic doesn't and cannot support loading LoRAs, how would that work? We need to do inference on the model we load in order to abliterate it. We can't do that with a LoRA, the bulk of the model is missing. |
|
When you do from_pretrained on a lora directory, it pulls the "base model" mentioned in the lora peft configuration (config.json) |
|
Really? But we're using |
It actually works because Hugging Face |
|
Yeah, I still don't get how this works when we load the model through Transformers, even though LoRA functionality is provided by PEFT. Which LoRA model did you test this with? |
This problem is a general one in the PEFT and Transformers integration and is unrelated to the model that was used. As soon as you load any directory with Since Heretic invokes |
|
Ok, the explanation makes sense. I will merge this after the 1.4 release because I can't test this comprehensively right now. |
After merging adapters, the base model retains an internal
_hf_peft_config_loadedflag. When you callsave_pretrained(),transformerstries to find active adapters that are no longer there, causing anUnboundLocalErrorcrash.Setting
_hf_peft_config_loaded = Falseon the merged model immediately after the merge prevents the crash and allows the model to save normally.