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

DON'T MERGE This is for debugging only #1135

Conversation

BenjaminBossan
Copy link
Member

@BenjaminBossan BenjaminBossan commented Nov 15, 2023

It appears that for LoRA, merging the first adapter works but merging the second adapter does not work.

@younesbelkada While debugging #1132, I wanted to isolate the error and found that it can already be reproduced on the main branch, without the changes from your PR.

I added two tests, one where the first adapter is merged, one where the second adapter is merged. The first test passes as expected, the second one fails for LoRA (it works for IA³):

┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
┃  File                          ┃  Function                                                                                                             ┃  Function Line  ┃  Error Line  ┃  Error                          ┃
┡━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┩
│  tests/test_decoder_models.py  │  PeftDecoderModelTester.test_merge_layers_multi_2_01_test_hf_internal_testing_tiny_random_OPTForCausalLM_lora         │  173            │  620         │  in _test_merge_layers_multi_2  │
│  tests/test_decoder_models.py  │  PeftDecoderModelTester.test_merge_layers_multi_2_07_test_hf_internal_testing_tiny_random_GPTNeoXForCausalLM_lora     │  173            │  620         │  in _test_merge_layers_multi_2  │
│  tests/test_decoder_models.py  │  PeftDecoderModelTester.test_merge_layers_multi_2_13_test_hf_internal_testing_tiny_random_GPT2LMHeadModel_lora        │  173            │  620         │  in _test_merge_layers_multi_2  │
│  tests/test_decoder_models.py  │  PeftDecoderModelTester.test_merge_layers_multi_2_19_test_hf_internal_testing_tiny_random_BloomForCausalLM_lora       │  173            │  620         │  in _test_merge_layers_multi_2  │
│  tests/test_decoder_models.py  │  PeftDecoderModelTester.test_merge_layers_multi_2_25_test_hf_internal_testing_tiny_random_gpt_neo_lora                │  173            │  620         │  in _test_merge_layers_multi_2  │
│  tests/test_decoder_models.py  │  PeftDecoderModelTester.test_merge_layers_multi_2_31_test_hf_internal_testing_tiny_random_GPTJForCausalLM_lora        │  173            │  620         │  in _test_merge_layers_multi_2  │
│  tests/test_decoder_models.py  │  PeftDecoderModelTester.test_merge_layers_multi_2_37_test_hf_internal_testing_tiny_random_GPTBigCodeForCausalLM_lora  │  173            │  620         │  in _test_merge_layers_multi_2  │
│  tests/test_decoder_models.py  │  PeftDecoderModelTester.test_merge_layers_multi_2_43_test_HuggingFaceM4_tiny_random_LlamaForCausalLM_lora             │  173            │  620         │  in _test_merge_layers_multi_2  │
└────────────────────────────────┴───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┴─────────────────┴──────────────┴─────────────────────────────────┘

PS: There is a small bug in IA³ where the active adapter attribute is not set after calling set_adapter, but that's not really related to the discussed issue.

EDIT: Also tested this with merge_adapter() instead of merge_and_unload, but that doesn't change the outcome.

It appears that for LoRA, merging the first adapter works but merging
the second adapter does not work.
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Nov 15, 2023

The documentation is not available anymore as the PR was closed or merged.

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.

2 participants