Skip to content

Conversation

@ArthurZucker
Copy link
Collaborator

@ArthurZucker ArthurZucker commented Aug 24, 2023

What does this PR do?

Simplifies the logic when loading a PEFT model: the _adapter_model_path is used instead of maybe_has_adapter_model_path and has_adapter_config

@ArthurZucker ArthurZucker requested a review from sgugger August 24, 2023 12:02
@ArthurZucker ArthurZucker marked this pull request as ready for review August 24, 2023 12:02
Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks simpler indeed!

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Aug 24, 2023

The documentation is not available anymore as the PR was closed or merged.

Copy link
Contributor

@younesbelkada younesbelkada left a 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

@ArthurZucker
Copy link
Collaborator Author

Thanks! Can confirm that the tests are now green! @younesbelkada some are really fast do you not want to remove the slow mention for them?

Copy link
Contributor

@younesbelkada younesbelkada left a 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 ArthurZucker merged commit fecf085 into huggingface:main Aug 24, 2023
@younesbelkada
Copy link
Contributor

younesbelkada commented Aug 24, 2023

@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

parambharat pushed a commit to parambharat/transformers that referenced this pull request Sep 26, 2023
* refactor complicated from pretrained for peft

* nits

* more nits

* Update src/transformers/modeling_utils.py

Co-authored-by: Sylvain Gugger <[email protected]>

* make tests happy

* fixup after merge

---------

Co-authored-by: Sylvain Gugger <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants