fix loading pretrained config #1041
Closed
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
There was a problem hiding this comment.
Thanks for catching this bug. I wonder if another solution would not be better: Instead of calling __post_init__ twice (once implicitly via the constructor, once explicitly), can we update the class_kwargs using the loaded_attributes before calling config = config_cls(**class_kwargs) and have the same effect?
4 tasks
BenjaminBossan
added a commit
to BenjaminBossan/peft
that referenced
this pull request
Oct 23, 2023
Fixes huggingface#1045, supersedes huggingface#1041 Description When loading a config from a file, we currently set the loaded attributes on the config directly. However, this sidesteps the __post_init__ call, which is required to convert the target_modules to a set. This PR fixes this by avoiding to set attributes on the config class directly, instead of going through __init__. Other changes While working on this, I did a slight refactor of the config tests. 1. All config classes are included now (some where missing before). 2. Use parameterized instead of looping through the classes. 3. Added a unit test for the aforementioned bug. Notes This fix is based on my comment here: huggingface#1041 (review) Normally, I'd just wait for Sourab to reply, but since he's off for a few days, I created a separate PR. Another way we could achieve this is to override __setattr__ on the config class which explicitly converts target_modules to a set. This would cover the case where a user does something like: config = ... config.target_modules = ["a", "b", "c"] Then we don't need to rely on __post_init__. However, I would propose to save the heavy guns for when absolutely necessary.
BenjaminBossan
added a commit
that referenced
this pull request
Oct 24, 2023
Fixes #1045, supersedes #1041 Description When loading a config from a file, we currently set the loaded attributes on the config directly. However, this sidesteps the __post_init__ call, which is required to convert the target_modules to a set. This PR fixes this by avoiding to set attributes on the config class directly, instead of going through __init__. Other changes While working on this, I did a slight refactor of the config tests. 1. All config classes are included now (some where missing before). 2. Use parameterized instead of looping through the classes. 3. Added a unit test for the aforementioned bug.
|
@pacman100 This PR should be superseded by the change here: I'll leave this PR open for now in case you want to double-check. |
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.
What does this PR do?