Fix Inconsistent Missing Keys Warning for Adapter Weights in PEFT #2084

yaswanth19 · 2024-09-21T12:02:40Z

Refer #1932 for more context.

The main logic is we would have a specific prefix of adapter related keys and if such prefix is present in any of the missing keys then raise a warning else the adapter is loaded succesfully.

yaswanth19 · 2024-09-21T12:12:48Z

Hi @BenjaminBossan

I think this logic should cover most of the cases. I haven't added prompt learning techniques here as they don't have a concrete prefix. Please let me know if I am missing any edge cases.

BenjaminBossan

Thanks a lot for this PR. I made a few suggestion, please check.

On top of those, I would also like to see a test being added. LMK if you feel up to that task and if you need support with it.

src/peft/peft_model.py

yaswanth19 · 2024-09-23T13:49:06Z

Yes @BenjaminBossan, Let me know which test cases should be handled. I will add those.

BenjaminBossan · 2024-09-23T15:57:59Z

Let me know which test cases should be handled. I will add those.

I checked our existing tests and I think it would actually be fine if we extend one of our existing tests and check that the missing_keys are empty. Take a look at this one:

peft/tests/testing_common.py

Lines 520 to 536 in f4cf170

    
           def _test_load_multiple_adapters(self, model_id, config_cls, config_kwargs): 
        
               # just ensure that this works and raises no error 
        
               model = self.transformers_class.from_pretrained(model_id) 
        
               config = config_cls( 
        
                   base_model_name_or_path=model_id, 
        
                   **config_kwargs, 
        
               ) 
        
               model = get_peft_model(model, config) 
        
               with tempfile.TemporaryDirectory() as tmp_dirname: 
        
                   model.save_pretrained(tmp_dirname) 
        
                   del model 
        
                   model = self.transformers_class.from_pretrained(model_id).to(self.torch_device) 
        
                   model = PeftModel.from_pretrained(model, tmp_dirname, torch_device=self.torch_device) 
        
                   model.load_adapter(tmp_dirname, adapter_name="other") 
        
                   model.load_adapter(tmp_dirname, adapter_name="yet-another")

There we're just loading two adapters and do no further checks (the test is just that there is no error). We could assign the load_result and assert that there are no missing keys for both of them. WDYT?

Apart from that, I just noticed that after your last pushes, when I go to https://github.com/huggingface/peft/pull/2084/files, for some reason I see 27 changed files (even though it only says 1 file on this page). Not sure what's going on. Maybe it's a GitHub bug, but it makes it super hard to review the PR. I would suggest to wait a day and see if it goes away by itself. If not, maybe you need to rebase on the latest main or if that doesn't help, create a fresh PR. Again, let's wait a day and only try those options if there is still the same error then.

BenjaminBossan · 2024-09-24T09:51:05Z

Hmm, I still see 27 changed files :-/ Could you please try re-basing? Otherwise, we may need a new PR with just the relevant changes.

yaswanth19 · 2024-09-24T14:20:36Z

Rebased the branch correctly and added test cases. Please review the PR.

BenjaminBossan

Thanks for the updates. The rebase appears to have fixed the issue with the GH diff, nice. I have a suggestion to simplify the code a bit. Moreover, could you please run make style on the PR?

src/peft/peft_model.py

yaswanth19 · 2024-09-24T14:52:30Z

Done with changes.

HuggingFaceDocBuilderDev · 2024-09-24T16:56:43Z

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.

BenjaminBossan · 2024-09-25T09:32:37Z

As you can see, the CI is failing and the reason is that the test does not pass when checking VB-LoRA. The reason for that is that the vector bank in VB-LoRA is shared among all layers, so there isn't an individual one for each layer. I don't think there is an easy way to account for that in the code changes you made. My suggestion is therefore to simply skip the test for VB-LoRA, adding a comment explaining why. Could you please adjust the test accordingly?

To check locally that the error is fixed, just run pytest tests/ -k test_load_multiple_adapters -v.

yaswanth19 · 2024-09-25T11:15:35Z

Done, Skipping the VBLORA test case and added appropriate comment.

BenjaminBossan

Great, thanks for this PR, this should hopefully reduce confusion about missing keys in the future. Nice work.

yaswanth19 · 2024-09-25T13:42:46Z

Yup Thanks @BenjaminBossan for guiding me, and I would be happy to help with any other issues or feature requests 🤗

BenjaminBossan · 2024-09-25T13:53:11Z

Sounds good, thanks! You can watch the PEFT issues for potential contributions, especially when there is the contributions-welcome tag.

After merging huggingface#2084, we now clean up the missing_keys when loading a PEFT adapter to remove all but the relevant keys (the fact that base model keys are missing is expected when loading a PEFT adapter). Since the presence of missing_keys now really means that something might have gone wrong during loading, we can now warn the user if they call PeftModel.from_pretrained. Note that load_adapter still does not warn, as here we return the load_result and users can already check, but for from_pretrained, they don't have that possibility.

After merging #2084, we now clean up the missing_keys when loading a PEFT adapter to remove all but the relevant keys (the fact that base model keys are missing is expected when loading a PEFT adapter). Since the presence of missing_keys now really means that something might have gone wrong during loading, we can now warn the user if they call PeftModel.from_pretrained. Note that load_adapter still does not warn, as here we return the load_result and users can already check, but for from_pretrained, they don't have that possibility.

After merging huggingface#2084, we now clean up the missing_keys when loading a PEFT adapter to remove all but the relevant keys (the fact that base model keys are missing is expected when loading a PEFT adapter). Since the presence of missing_keys now really means that something might have gone wrong during loading, we can now warn the user if they call PeftModel.from_pretrained. Note that load_adapter still does not warn, as here we return the load_result and users can already check, but for from_pretrained, they don't have that possibility.

…ace#2084) When loading a PEFT adapter, a lot of missing keys are reported, because the base model weights are not loaded. However, this is totally fine. Therefore, those missing keys can be safely ignored. When using from_pretrrained, the missing keys won't be returned to the user, thus there is no room for confusion. But when using load_adapter, the missing keys (and unexpected keys) are returned and can cause confusion. With this PR, the missing keys are filtered to remove keys that are unrelated to the adapter. A small gap is VB-LoRA which reports missing keys because the vector bank parameters are actually only loaded once and then shared.

After merging huggingface#2084, we now clean up the missing_keys when loading a PEFT adapter to remove all but the relevant keys (the fact that base model keys are missing is expected when loading a PEFT adapter). Since the presence of missing_keys now really means that something might have gone wrong during loading, we can now warn the user if they call PeftModel.from_pretrained. Note that load_adapter still does not warn, as here we return the load_result and users can already check, but for from_pretrained, they don't have that possibility.

BenjaminBossan requested changes Sep 23, 2024

View reviewed changes

src/peft/peft_model.py Outdated Show resolved Hide resolved

src/peft/peft_model.py Outdated Show resolved Hide resolved

src/peft/peft_model.py Outdated Show resolved Hide resolved

yaswanth19 closed this Sep 23, 2024

yaswanth19 reopened this Sep 23, 2024

yaswanth19 added 4 commits September 24, 2024 19:24

added logic to filter missing keys

9c2e8fa

🔧 Added peft_type_tp_prefix_mapping

f9aff6d

removed warnings from laod_adapter

ba2ab8c

added logic to filter missing keys

55c1c58

yaswanth19 force-pushed the filter-missing-keys branch from bc5237c to 55c1c58 Compare September 24, 2024 13:58

yaswanth19 added 2 commits September 24, 2024 19:47

added check on adapter name

68f342a

Added test cases for filtering missing key logic

2077376

BenjaminBossan requested changes Sep 24, 2024

View reviewed changes

src/peft/peft_model.py Outdated Show resolved Hide resolved

Minor refactoring

22c7b13

Modified test case

31536ac

formatting fix

53395ef

BenjaminBossan approved these changes Sep 25, 2024

View reviewed changes

BenjaminBossan merged commit ccc3501 into huggingface:main Sep 25, 2024

BenjaminBossan mentioned this pull request Sep 30, 2024

ENH: Warn when from_pretrained misses PEFT keys #2118

Merged

Fix Inconsistent Missing Keys Warning for Adapter Weights in PEFT #2084

Fix Inconsistent Missing Keys Warning for Adapter Weights in PEFT #2084

Uh oh!

Conversation

yaswanth19 commented Sep 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yaswanth19 commented Sep 21, 2024

Uh oh!

BenjaminBossan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yaswanth19 commented Sep 23, 2024

Uh oh!

BenjaminBossan commented Sep 23, 2024

Uh oh!

BenjaminBossan commented Sep 24, 2024

Uh oh!

yaswanth19 commented Sep 24, 2024

Uh oh!

BenjaminBossan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yaswanth19 commented Sep 24, 2024

Uh oh!

HuggingFaceDocBuilderDev commented Sep 24, 2024

Uh oh!

BenjaminBossan commented Sep 25, 2024

Uh oh!

yaswanth19 commented Sep 25, 2024

Uh oh!

BenjaminBossan left a comment

Choose a reason for hiding this comment

Uh oh!

yaswanth19 commented Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BenjaminBossan commented Sep 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yaswanth19 commented Sep 21, 2024 •

edited

Loading

yaswanth19 commented Sep 25, 2024 •

edited

Loading