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.

Sign up for GitHub

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

Jump to bottom

[tests] add multiple active adapters tests #961

Merged
merged 20 commits into from
Sep 29, 2023
Merged

[tests] add multiple active adapters tests #961

merged 20 commits into from
Sep 29, 2023

Conversation

pacman100
Copy link
Contributor

What does this PR do?

  1. Add multiple active adapters tests

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Sep 26, 2023 •
edited
Loading

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

@pacman100
add util function for proper naming of tests
64479bb
@pacman100
fix bugs
00f7ba9 
1. fix `add_weighted_adapter` when working with adapters targeting different layers
2. fix `ia3` model and layer to handle adapters targeting different layers
3. fix the multiple active adapter tests
@pacman100 pacman100 changed the title add multiple active adapters tests [tests] add multiple active adapters tests Sep 26, 2023
@pacman100 pacman100 marked this pull request as ready for review September 26, 2023 10:56
BenjaminBossan
BenjaminBossan reviewed Sep 26, 2023
View reviewed changes
Copy link
Member

@BenjaminBossan BenjaminBossan left a comment

Choose a reason for hiding this comment

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

Fantastic, thanks so much for adding the tests while also fixing some remaining issues. I have a couple of comments, but nothing fundamental, please take a look.

src/peft/tuners/ia3/model.py Show resolved Hide resolved
src/peft/tuners/lora/model.py Outdated Show resolved Hide resolved
src/peft/tuners/lora/model.py Outdated Show resolved Hide resolved
src/peft/tuners/lora/model.py Show resolved Hide resolved
src/peft/tuners/lora/model.py Outdated Show resolved Hide resolved
tests/test_custom_models.py Outdated Show resolved Hide resolved
tests/test_custom_models.py Show resolved Hide resolved
tests/test_custom_models.py Show resolved Hide resolved
tests/test_custom_models.py Show resolved Hide resolved
tests/test_custom_models.py Show resolved Hide resolved
pacman100 and others added 5 commits September 26, 2023 22:52
@pacman100
fix bug
f62446f
@pacman100
address comments
8b3e5b3
@pacman100 @BenjaminBossan
address comments
7e26c07 
Co-Authored-By: Benjamin Bossan <BenjaminBossan@users.noreply.github.com>
@pacman100
fix tests
72aada4
@pacman100
remove unused code
bb47d24
@BenjaminBossan
Copy link
Member

@pacman100 There is still one test failing, could you please investigate? I restarted CI and it failed again, so it's probably not just flakiness.

BenjaminBossan
BenjaminBossan approved these changes Sep 28, 2023
View reviewed changes
Copy link
Member

@BenjaminBossan BenjaminBossan left a comment

Choose a reason for hiding this comment

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

Thanks, looks very good!

@pacman100 pacman100 merged commit cacee95 into main Sep 29, 2023
11 checks passed
@pacman100 pacman100 deleted the smangrul/add-multiple-active-adapters-tests branch February 20, 2024 05:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenjaminBossan BenjaminBossan BenjaminBossan approved these changes

@younesbelkada younesbelkada Awaiting requested review from younesbelkada

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pacman100 @HuggingFaceDocBuilderDev @BenjaminBossan