FIX Allow same layer adapters on different devices #1742
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Resolves 1639 The issue is that so far, we made the assumption in PEFT that all adapter weights of a specific layer are on the same device. There can be cases where it is useful to have adapters on different devices. E.g. when a user loads a lot of LoRA adapters and wants to offload those not currently in use to CPU, they would not currently be able to do so. With this PR, we add this possibility. To achieve this, when we update an adapter layer with a new adapter, we only move that specific adapter to the device of the base layer, will not touching the other loaded adapters. While working on this, I discovered a small bug in VeRA when adding multiple adapters, which is now also fixed. This PR has the potential to lead to unforeseen issues, so careful review is required. After merging this, let's keep it out of releases for a while to ensure it doesn't break anything.
|
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. |
4 tasks
As it's not a unittest
pacman100
approved these changes
May 20, 2024
There was a problem hiding this comment.
Thank you @BenjaminBossan for refactoring out the logic for setting the device of the adapters without disturbing the devices of the prior added adapters! 🚀 Very useful for offloading when working with a vast numbers of adapters.
younesbelkada
approved these changes
May 21, 2024
|
Thanks for the reviews. I'll wait a bit in case @iuliaturc has time to check if this PR fixes the initial issue. If we don't hear back, I'll merge in a few days. We should let this PR "simmer" for a bit since there is a small probability that this will break some edge case we haven't thought of. |
|
Thanks so much for the PR! I left a comment here. TL;DR is that, indeed, only one LoRA seems to be loaded at a time, but the fix doesn't seem to address the original problem (that latency keeps creeping up the more calls we make). |
|
Thanks for the confirmation. |
BenjaminBossan
added a commit
to BenjaminBossan/peft
that referenced
this pull request
May 24, 2024
PR huggingface#1742 introduced the feature that adapters of the same layer can be on different devices. A new method was introduced that is responsible for moving the parameters related to a specific adapter in a consistent way. In BOFT, however, one parameter was overlooked, boft_P. This parameter is not stored inside a ParameterDict or ModuleDict, hence it was not moved. The reason is (presumably) that this parameter is shared between all BOFT adapters, as it's always identical. However, this clashes with having different adapters on different devices. To solve this, the parameter is now moved on the fly to the correct device during the forward pass.
BenjaminBossan
added a commit
that referenced
this pull request
May 27, 2024
PR #1742 introduced the feature that adapters of the same layer can be on different devices. A new method was introduced that is responsible for moving the parameters related to a specific adapter in a consistent way. In BOFT, however, one parameter was overlooked, boft_P. This parameter is not stored inside a ParameterDict or ModuleDict, hence it was not moved. The reason is (presumably) that this parameter is shared between all BOFT adapters, as it's always identical. However, this clashes with having different adapters on different devices. To solve this, the parameter is now moved on the fly to the correct device during the forward pass.
BenjaminBossan
added a commit
to BenjaminBossan/peft
that referenced
this pull request
Jun 12, 2024
Locally, multiple AdaLoRA-related tests are failing. We did not catch this in the nightly run because the tests were missing the necessary pytest marker. The issue is related to the change in huggingface#1742, which made it possible to load different adapters on different devices. Although that PR itself was sound, the issue is that for AdaLoRA, one of its parameters, ranknum, was not listed in the other_param_names and was thus not moved to the correct device. This oversight is now fixed and the GPU tests are now passing locally for me. This PR also adds the missing pytest marker to the test class that was missing it, so that these errors should be caught by our nightly CI in the future.
BenjaminBossan
added a commit
that referenced
this pull request
Jun 13, 2024
Locally, multiple AdaLoRA-related tests are failing. We did not catch this in the nightly run because the tests were missing the necessary pytest marker. The issue is related to the change in #1742, which made it possible to load different adapters on different devices. Although that PR itself was sound, the issue is that for AdaLoRA, one of its parameters, ranknum, was not listed in the other_param_names and was thus not moved to the correct device. This oversight is now fixed and the GPU tests are now passing locally for me. This PR also adds the missing pytest marker to the test class that was missing it, so that these errors should be caught by our nightly CI in the future.
8 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #1639
The issue is that so far, we made the assumption in PEFT that all adapter weights of a specific layer are on the same device. There can be cases where it is useful to have adapters on different devices. E.g. when a user loads a lot of LoRA adapters and wants to offload those not currently in use to CPU, they would not currently be able to do so.
With this PR, we add this possibility. To achieve this, when we update an adapter layer with a new adapter, we only move that specific adapter to the device of the base layer, will not touching the other loaded adapters.
While working on this, I discovered a small bug in VeRA when adding multiple adapters, which is now also fixed.
This PR has the potential to lead to unforeseen issues, so careful review is required. After merging this, let's keep it out of releases for a while to ensure it doesn't break anything.