-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
ENH Make PEFT configs forward compatible #2038
ENH Make PEFT configs forward compatible #2038
Conversation
Right now, loading a PEFT config saved with a more recent PEFT version than is currently installed will lead to errors when new arguments are added to the config in the newer PEFT version. The current workaround is for users to manually edit the adapter_config.json to remove those entries. With this PR, PEFT will make an attempt at removing these unknown keys by inspecting the signature. The user will be warned about these removed keys. This should generally be a safe measure because we will generally not introduce new config settings that change the default behavior. However, if a non-default is used, this could lead to wrong results. This is mentioned in the warning. While working on the tests, I also converted the unittest.TestCase to a normal pytest test in order to be able to use pytest fixtures. I also plan on adding the PEFT version to the adapter_config.json in the future. This will allow us to better handle compatibility issues in the future. As adding that new key to all PEFT configs could cause a lot of disruption, I want to get this PR in first to ensure forward compatibility. Note that this new mechanism will not help anyone using a PEFT version <= 0.12.0, so this will be a slow transition.
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. |
Let's wait for #1996 to be merged first, than update this branch and make the two changes work together nicely. |
This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread. |
Not stale, still waiting for that PR, should be done soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat! Fully agree that this is a better plan.
Right now, loading a PEFT config saved with a more recent PEFT version than is currently installed will lead to errors when new arguments are added to the config in the newer PEFT version. The current workaround is for users to manually edit the adapter_config.json to remove those entries. With this PR, PEFT will make an attempt at removing these unknown keys by inspecting the signature. The user will be warned about these removed keys. This should generally be a safe measure because we will generally not introduce new config settings that change the default behavior. However, if a non-default is used, this could lead to wrong results. This is mentioned in the warning. While working on the tests, I also converted the unittest.TestCase to a normal pytest test in order to be able to use pytest fixtures. I also plan on adding the PEFT version to the adapter_config.json in the future. This will allow us to better handle compatibility issues in the future. As adding that new key to all PEFT configs could cause a lot of disruption, I want to get this PR in first to ensure forward compatibility. Note that this new mechanism will not help anyone using a PEFT version < 0.14.0, so this will be a slow transition.
Right now, loading a PEFT config saved with a more recent PEFT version than is currently installed will lead to errors when new arguments are added to the config in the newer PEFT version. The current workaround is for users to manually edit the
adapter_config.json
to remove those entries.To give an example, if a new LoRA option called "foobar" is added to PEFT v0.14.0, the saved
adapter_config.json
will from that point on always contain an entry for "foobar", even if by default, this value does nothing and the default value is being used. When a user of a lower version of PEFT wants to load that checkpoint, they'll get an error, even though the checkpoint would work. The only workarounds are upgrading PEFT (which is not always desired) or manually deleting "foobar", which is annoying.With this PR, PEFT will make an attempt at removing these unknown keys by inspecting the signature. The user will be warned about these removed keys. This should generally be a safe measure because we will generally not introduce new config settings that change the default behavior. However, if a non-default is used, this could lead to wrong results. For this reason, users are urged in the warning to upgrade PEFT.
While working on the tests, I also converted the
unittest.TestCase
to a normal pytest test in order to be able to use pytest fixtures.I also plan on adding the PEFT version to the
adapter_config.json
in the future. This will allow us to better handle compatibility issues in the future. As adding that new key to all PEFT configs could cause a lot of disruption, I want to get this PR in first to ensure forward compatibility.Note that this new mechanism will not help anyone using a PEFT version <= 0.13.0, so this will be a slow transition.