-
Notifications
You must be signed in to change notification settings - Fork 27.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[from_pretrained
] Simpler code for peft
#25726
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks simpler indeed!
Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
The documentation is not available anymore as the PR was closed or merged. |
…to model-utils-nits
…ansformers into model-utils-nits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot @ArthurZucker for attempting to refactor that part, sadly all PEFT tests are failing with your changes, can you please make sure they pass before merging?
RUN_SLOW=1 pytest tests/peft_integration/test_peft_integration.py
Also make sure to rebase with main in order to run the tests
I suspect the reason is because you need to override the pretrained_model_name_or_path
instance with _adapter_model_path
and store the current adapter id in a new variable adapter_model_id
in the previous changes
Thanks! Can confirm that the tests are now green! @younesbelkada some are really fast do you not want to remove the |
…to model-utils-nits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! All of them should be using tiny models so they are fast, I don't know if we want to add PEFT as a dependency to our CIs so maybe let's keep it as it is for now, what do you think?
@ArthurZucker sorry the tests are still failing, probably something wrong happened during the merge, can you double check? 🙏 I can also have a look if you want Edit: just made #25733 |
* refactor complicated from pretrained for peft * nits * more nits * Update src/transformers/modeling_utils.py Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com> * make tests happy * fixup after merge --------- Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
* refactor complicated from pretrained for peft * nits * more nits * Update src/transformers/modeling_utils.py Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com> * make tests happy * fixup after merge --------- Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
* refactor complicated from pretrained for peft * nits * more nits * Update src/transformers/modeling_utils.py Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com> * make tests happy * fixup after merge --------- Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
What does this PR do?
Simplifies the logic when loading a PEFT model: the
_adapter_model_path
is used instead ofmaybe_has_adapter_model_path
andhas_adapter_config