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

[PEFT] Support low_cpu_mem_usage option for PEFT loading adapters #33725

Merged
merged 2 commits into from
Oct 3, 2024
Merged

[PEFT] Support low_cpu_mem_usage option for PEFT loading adapters #33725

merged 2 commits into from
Oct 3, 2024

Conversation

BenjaminBossan
Copy link
Member

What does this PR do?

PEFT added support for low_cpu_mem_usage=True when loading adapters in huggingface/peft#1961. This feature is now available when installing PEFT v0.13.0. With this PR, this option is also supported when loading PEFT adapters directly into transformers models.

Additionally, with this PR, huggingface/diffusers#9510 will be unblocked, which implements this option in diffusers.

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

@HuggingFaceDocBuilderDev
Copy link

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@sayakpaul
Copy link
Member

@ArthurZucker could you give this a look?

@sayakpaul sayakpaul mentioned this pull request Sep 27, 2024
Merged
ArthurZucker
ArthurZucker approved these changes Sep 27, 2024
View reviewed changes
src/transformers/integrations/peft.py
adapter_kwargs (`Dict[str, Any]`, *optional*):
Additional keyword arguments passed along to the `from_pretrained` method of the adapter config and
`find_adapter_config_file` method.
"""
check_peft_version(min_version=MIN_PEFT_VERSION)

# peft only supports low_cpu_mem_usage starting from v0.13.0
peft_load_kwargs = {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

LGTM, not sure we need all the kwargs as you are not taking them from the kwargs of this function 😉

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if I get you, do you mean this should be merged with adapter_kwargs? That wouldn't work, as these are kwargs used for a different purpose.

Or that using a dict for a single argument is overkill?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or that using a dict for a single argument is overkill?
this 🤗

Copy link
Member Author

Choose a reason for hiding this comment

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

I found this more readable and extensible than having:

if low_cpu_mem_usage:
    inject_adapter_in_model(peft_config, self, adapter_name, low_cpu_mem_usage=low_cpu_mem_usage)
else:
    inject_adapter_in_model(peft_config, self, adapter_name)

(same with the set_peft_model_state_dict call)

If you want me to change it to this instead or have an alternative idea, let me know and I'll change it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

no worries

@ArthurZucker
Copy link
Collaborator

not sure what's going on with the tests, rebasing should help !

@BenjaminBossan
Copy link
Member Author

@ArthurZucker I still get fetch_tests - Failed from setup_and_quality. Any idea what could be the cause? I ran ruff on the touched files.

@BenjaminBossan @ArthurZucker
[PEFT] Support low_cpu_mem_usage for PEFT loading
4214349 
PEFT added support for low_cpu_mem_usage=True when loading adapters in
huggingface/peft#1961. This feature is now
available when installing PEFT v0.13.0. With this PR, this option is
also supported when loading PEFT adapters directly into transformers
models.

Additionally, with this PR,
huggingface/diffusers#9510 will be unblocked,
which implements this option in diffusers.
@BenjaminBossan @ArthurZucker
Fix typo
5236a47
@ArthurZucker ArthurZucker force-pushed the enh-peft-support-low-cpu-mem-usage branch from 45394a5 to 5236a47 Compare October 3, 2024 12:57
@ArthurZucker
Copy link
Collaborator

I had to force push as the rebasing you did went a bit wrong! Should be good now! 🤗

@BenjaminBossan
Copy link
Member Author

I had to force push as the rebasing you did went a bit wrong! Should be good now! 🤗

Oops, sorry about that. Anything that needs to be done from my side for this PR?

@ArthurZucker
Copy link
Collaborator

Nope, I was just waiting for the cis!

@ArthurZucker ArthurZucker merged commit 6500f78 into huggingface:main Oct 3, 2024
22 of 24 checks passed
@BenjaminBossan BenjaminBossan deleted the enh-peft-support-low-cpu-mem-usage branch October 10, 2024 14:21
NielsRogge pushed a commit to NielsRogge/transformers that referenced this pull request Oct 21, 2024
@BenjaminBossan @NielsRogge
[PEFT] Support low_cpu_mem_usage option for PEFT loading adapters (hu…
459e402 
…ggingface#33725)

* [PEFT] Support low_cpu_mem_usage for PEFT loading

PEFT added support for low_cpu_mem_usage=True when loading adapters in
huggingface/peft#1961. This feature is now
available when installing PEFT v0.13.0. With this PR, this option is
also supported when loading PEFT adapters directly into transformers
models.

Additionally, with this PR,
huggingface/diffusers#9510 will be unblocked,
which implements this option in diffusers.

* Fix typo
BernardZach pushed a commit to BernardZach/transformers that referenced this pull request Dec 5, 2024
@BenjaminBossan @BernardZach
[PEFT] Support low_cpu_mem_usage option for PEFT loading adapters (hu…
98d9708 
…ggingface#33725)

* [PEFT] Support low_cpu_mem_usage for PEFT loading

PEFT added support for low_cpu_mem_usage=True when loading adapters in
huggingface/peft#1961. This feature is now
available when installing PEFT v0.13.0. With this PR, this option is
also supported when loading PEFT adapters directly into transformers
models.

Additionally, with this PR,
huggingface/diffusers#9510 will be unblocked,
which implements this option in diffusers.

* Fix typo
BernardZach pushed a commit to innovationcore/transformers that referenced this pull request Dec 6, 2024
@BenjaminBossan @BernardZach
[PEFT] Support low_cpu_mem_usage option for PEFT loading adapters (hu…
58ee07d 
…ggingface#33725)

* [PEFT] Support low_cpu_mem_usage for PEFT loading

PEFT added support for low_cpu_mem_usage=True when loading adapters in
huggingface/peft#1961. This feature is now
available when installing PEFT v0.13.0. With this PR, this option is
also supported when loading PEFT adapters directly into transformers
models.

Additionally, with this PR,
huggingface/diffusers#9510 will be unblocked,
which implements this option in diffusers.

* Fix typo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ArthurZucker ArthurZucker ArthurZucker approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@BenjaminBossan @HuggingFaceDocBuilderDev @sayakpaul @ArthurZucker