fix: Enable LoRAs#308
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to save or upload LoRA adapters independently and refactors model card generation into a centralized get_model_card function. However, several critical issues were identified: a git merge conflict marker was accidentally left in src/heretic/utils.py, the get_model_card function references a non-existent use_ara attribute which will cause a runtime error, and the function's return type annotation is incorrectly marked as None instead of ModelCard | None.
9270fd7 to
5419a20
Compare
|
Previously the locally saved LoRA would have the default autogenerated README Now the local file looks like which has the same info as the hf_hub uploaded version (just ran & put in https://huggingface.co/anrp/hereticuploadlora ) |
|
Resolves #152 |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors model card generation into a new utility module and introduces an option to save LoRA adapters without merging. The changes include moving README generation logic to src/heretic/model_card_utils.py and updating the saving and uploading workflows to automatically generate and store model cards. Review feedback highlights a potential crash when loading remote model cards and suggests improving the readability of the parameter table generation logic.
|
Thanks, but please split out the model card changes into a separate PR. We're very close to the 1.3 release now, and #303 is making further changes that affect the model card. This part is too risky to merge so late in the release cycle. |
This reverts commit 025ab3a.
|
Moved readme bit to #314, this is just a pure revert now |
|
Thanks! I was finally able to merge this after GitHub had issues with pull requests for several days. |
|
I just tested this: https://huggingface.co/p-e-w/gemma-4-E2B-it-heretic-LoRA The safetensors file is empty 😠 😠 😠 Looks like the bug isn't fixed after all. |
|
It is possible the adapter is actually empty (save it to disk and see?) that looks like valid-but-empty? |
|
Interestingly, it did work with Qwen3.5: https://huggingface.co/p-e-w/Qwen3.5-4B-heretic-LoRA |
No, that isn't possible, because otherwise the model wouldn't be abliterated. All abliteration happens through the adapter. If the model is modified at all (and it is, which you can see from the refusal reduction) then the adapter cannot be empty. |
|
There is probably a bug in the way PEFT identifies modules, which would explain why it works for one model but not the other, even though the Heretic configuration is identical. |
|
I think it might be something about your system? I just ran this and uploaded (directly) to https://huggingface.co/anrp/gemma-4-E2B-it-heretic-tryupload/tree/main and the adapter is reasonably-sized. ??? OTOH I am using an out of date uv that doesn't understand the exclude-newer directive, so it may be that a package needs to be updated. |
|
I spun up a fresh server for this test, installed dependencies with pip, and did the complete run with all parameters set to their defaults, with GPU acceleration. It may be that GPU operations are the problem, or that there is some hardware-specific behavior, but given that it clearly still doesn't work reliably, I see no option other than to revert this PR. |
|
Makes sense to revert it while figuring it out. I'd like to reproduce what's happening to you though, could you just write out the steps that you ran to get that failure mode (specifically, not uv, using pip, which host OS, ...)? I always use uv to run this which (basically) disallows me to mess with packages, so it's very weird. (FYI: I care about LoRAs being outputted so much because it's +few% overhead to have both the gold standard base model & the abliterated model serving at the same time with vllm, so very easy to compare with no switching overhead) |
Thanks to our new reproducibility system, I can indeed tell you exactly what I did: https://huggingface.co/p-e-w/gemma-4-E2B-it-heretic/blob/main/reproduce/README.md (This was exported from the same installation.) 😄 |
|
I did, and... it works. https://huggingface.co/anrp/gemma-4-E2B-it-heretic-trylora2/tree/main Don't know what to say anymore? |
|
I think I may have figured out what's going on. Try the following:
I suspect that merging the adapter with the base model empties it somehow. |
|
There's the clue. #321 fixes that flow (lora-after-save-merged gave an empty adapter even going to disk.) |
* fix: Reset model after saving merged model The adapter is lost and writes 0-byte adapters if you save an adapter after saving the merged model. * Revert "Revert "Revert "fix: disable LoRA export for now" (#308)" (#319)" This reverts commit 216c089. * Add comment as to why resetting model is needed
No description provided.