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 target_modules type in config.from_pretrained #1046

Conversation

BenjaminBossan
Copy link
Member

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. Added a unit test for the aforementioned bug.
  3. Use parameterized instead of looping through the classes. Note that this makes the diff look much bigger than it actually is, the tests are actually unchanged, just unindented by one level because the loop is removed.

Notes

This fix is based on my comment here:

#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 this:

config = ...
config.target_modules = ["a", "b", "c"]

Using __setattr__, we wouldn't need to rely on __post_init__. However, I would propose to save the heavy guns for when absolutely necessary.

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.
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.

This looks super clean ! Thanks a lot for fixing target_modules type in from_pretrained, adding corresponding tests and extending the config tests for all config classes!

@BenjaminBossan BenjaminBossan merged commit 1d0535e into huggingface:main Oct 24, 2023
11 checks passed
@BenjaminBossan BenjaminBossan deleted the fix-config-from-pretrained-post-init branch October 24, 2023 10:06
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.

add_weighted_adapter() is unusable, throws error: "Invalid type <class 'list'> found in target_modules"
2 participants