Skip to content
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

Fix unwrapping peft models #948

Merged
merged 2 commits into from
Nov 5, 2023

Conversation

kkteru
Copy link
Contributor

@kkteru kkteru commented Nov 2, 2023

Fixes issues discussed here.

cc @younesbelkada

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.

Makes sense, thanks!

@younesbelkada
Copy link
Contributor

Hi @kkteru
Can you merge this branch with main branch to make sure tests pass?

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

@kkteru
Copy link
Contributor Author

kkteru commented Nov 4, 2023

Interesting, somehow the test_sft_trainer.py tests failed on latest main branch (without any changes from this branch) in my local. Investigating further.

@younesbelkada
Copy link
Contributor

@kkteru what are the failed tests? might be unrelated to your changes I think - you can just merge your branch with upstream main and see here if the tests pass

@kkteru kkteru force-pushed the fix-unwrapping-peft-models branch from b9c7662 to 7c12444 Compare November 4, 2023 22:41
@kkteru
Copy link
Contributor Author

kkteru commented Nov 4, 2023

These ones fail both on the main branch and this branch (merged with main) on my system. I just rebased on top of latest main and pushed. I dont see the tests running here?

=================================================================== short test summary info ====================================================================
FAILED tests/test_sft_trainer.py::SFTTrainerTester::test_sft_trainer - AssertionError: False is not true
FAILED tests/test_sft_trainer.py::SFTTrainerTester::test_sft_trainer_infinite_with_model - AssertionError: False is not true
FAILED tests/test_sft_trainer.py::SFTTrainerTester::test_sft_trainer_infinite_with_model_epochs - AssertionError: False is not true
FAILED tests/test_sft_trainer.py::SFTTrainerTester::test_sft_trainer_with_model - AssertionError: False is not true
FAILED tests/test_sft_trainer.py::SFTTrainerTester::test_sft_trainer_with_model_num_train_epochs - AssertionError: False is not true

@younesbelkada
Copy link
Contributor

The tests were failing because you had an old version of transformers, installing the 4.35.0 will fix your isses! As you can see, CI is green now :D

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.

Awesome clean up !

@younesbelkada younesbelkada merged commit 6ff0fac into huggingface:main Nov 5, 2023
8 checks passed
lapp0 pushed a commit to lapp0/trl that referenced this pull request May 10, 2024
* First unwrap the model and then process the input embeddings

* Changed base_model to base_model.model to stay consistent with peft model abstractions
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.

3 participants