FileNotFoundError when using SentenceTransformerTrainingArguments(load_best_model_at_end=True) and Peft
Author: GTimothee
Pull request: no
Labels: name: bug

### System Info

I used google colab default environment, with last version of transformers and sentence-transformers

### Who can help?

@muellerzr 

### Information

- [ ] The official example scripts
- [X] My own modified scripts

### Tasks

- [ ] An officially supported task in the `examples` folder (such as GLUE/SQuAD, ...)
- [ ] My own task or dataset (give details below)

### Reproduction

Here is the example code as a gist: [gist](https://gist.github.com/GTimothee/cb3551ba6eb14b04f7a06d63ea4616f9)

Just open the gist in a colab notebook and run it

### Expected behavior

This is a follow up about another bug found in sentence-transformers. The sentence-transformers library just integrated peft using transformers.integrations. The bug is that when using SentenceTransformerTrainingArguments(load_best_model_at_end=True) there is a FileNotFoundError as we try to load a classical checkpoint file (pth) but we saved an adapter instead. When looking into the load_best_model function, it just uses the function from the transformers.trainer.Trainer. So we need to modify the transformers library to solve the problem. 

The issue is that there is a function in transformers that checks if the model is a PeftMixedModel or not. If not, it is not considered a peft model and the trainer tries to load the model as usual. The problem is our model is a PeftAdapterMixin so it is not recognized as a peft model. 

See also: https://github.com/UKPLab/sentence-transformers/issues/3056 

In my opinion, we need to add to the check a 2-step check 1) is it a PeftAdapterMixin and 2) has it adapters loaded? Maybe it is only one part of the solution though, and we need a special loading snippet in the transformers.trainer.Trainer._load_best_model directly.

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

Please note that issues that do not follow the [contributing guidelines](https://github.com/huggingface/transformers/blob/main/CONTRIBUTING.md) are likely to be ignored.

cc @muellerzr @sunmarc - seems like a bad interaction between PEFT and Trainer's assumptions

Thanks for the report @GTimothee, maybe interesting to you too @BenjaminBossan. Would you like to submit a PR to fix it @GTimothee ? 

Thanks @GTimothee for the report and @SunMarc for the ping. Indeed, the analysis is correct. For the fix, it would probably be sufficient to adjust `_is_peft_model` to check additionally check if `any(isinstance(module, BaseTunerLayer) for module in model.modules())` (where `BaseTunerLayer` comes from `peft.tuners.tuners_utils`). I haven't verified this though.

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.

Please note that issues that do not follow the [contributing guidelines](https://github.com/huggingface/transformers/blob/main/CONTRIBUTING.md) are likely to be ignored.

Sorry for the late answer, I would be happy to give it a try if you reopen the issue @SunMarc @BenjaminBossan 

Thanks @GTimothee, feel free to ping me if you have questions.

@BenjaminBossan your solution work for making is_peft_model return True, but I am wondering if it will not create side effects. 

For example I now get this error: 
```
----> 4 trainer = SentenceTransformerTrainer(
      5     model=model,
      6     args=args,

3 frames

[/usr/local/lib/python3.11/dist-packages/sentence_transformers/trainer.py](https://localhost:8080/#) in __init__(self, model, args, train_dataset, eval_dataset, loss, evaluator, data_collator, tokenizer, model_init, compute_metrics, callbacks, optimizers, preprocess_logits_for_metrics)
    240             )
    241 
--> 242         super().__init__(**super_kwargs)
    243         # If the eval_dataset is "dummy", then we set it back to None
    244         if self.eval_dataset == "dummy":

[/usr/local/lib/python3.11/dist-packages/transformers/utils/deprecation.py](https://localhost:8080/#) in wrapped_func(*args, **kwargs)
    170                 warnings.warn(message, FutureWarning, stacklevel=2)
    171 
--> 172             return func(*args, **kwargs)
    173 
    174         return wrapped_func

[/usr/local/lib/python3.11/dist-packages/transformers/trainer.py](https://localhost:8080/#) in __init__(self, model, args, data_collator, train_dataset, eval_dataset, processing_class, model_init, compute_loss_func, compute_metrics, callbacks, optimizers, optimizer_cls_and_kwargs, preprocess_logits_for_metrics)
    630             unwrapped_model.forward
    631             if not _is_peft_model(unwrapped_model)
--> 632             else unwrapped_model.get_base_model().forward
    633         )
    634         forward_params = inspect.signature(model_forward).parameters

[/usr/local/lib/python3.11/dist-packages/torch/nn/modules/module.py](https://localhost:8080/#) in __getattr__(self, name)
   1926             if name in modules:
   1927                 return modules[name]
-> 1928         raise AttributeError(
   1929             f"'{type(self).__name__}' object has no attribute '{name}'"
   1930         )

AttributeError: 'SentenceTransformer' object has no attribute 'get_base_model'
```

Indeed, _is_peft_model is used in other places of the Trainer class, and in many places we try to access base_model or use get_base_model, but it does not exist in sentence-transformers' PeftAdapterMixin (https://github.com/UKPLab/sentence-transformers/blob/master/sentence_transformers/peft_mixin.py#L22) . So it would require to add additional code everywhere (There are 13 uses of _is_peft_model in Trainer) to check that the model has get_base_model or base_model attribute.

Do you think we should just check if the model is of instance PeftAdapterMixin for now and maybe make an update of the PeftAdapterMixin class on sentence-transformers side in a second time, to make it more compatible with transformers?

Otherwise I can just add an additional ```if hasattr(self.model, "get_base_model") or hasattr(self.model, "base_model")``` here and there :)

@BenjaminBossan up :)

Thanks for investigating @GTimothee. Could you please post a small reproducer?

Regarding the solution, I agree that it would be annoying to add this extra check in so many places. I wonder if we could make SentenceTransformer pass the `base_model`/`get_base_model` call to the underlying PEFT model. Pinging @tomaarsen.

> maybe make an update of the PeftAdapterMixin class on sentence-transformers side

Very open to this, my goal with ST is to avoid changes in Transformers where possible. I think ST is a pretty reasonable place for this, but I agree with @BenjaminBossan that a reproducer would be quite nice to help us out.

- Tom Aarsen

@BenjaminBossan @tomaarsen 

Here is a notebook you can run in colab that will reproduce the error: https://gist.github.com/GTimothee/4be39baadc2c4f4bdbcade557417d1fc If you run it as is it will display my new error (after fixing the filenotfounderror). 

If you want to reproduce the filenotfounderror, just remove the setup part where I install my fork of transformers and install transformers directly. Edit: apparently I already gave this gist for this issue : https://gist.github.com/GTimothee/cb3551ba6eb14b04f7a06d63ea4616f9

What version of `peft` are you using? On my first run, I didn't have issues with the `SentenceTransformerTrainer` cell, but I haven't looked in a lot of detail.

About reproducing the filenotfounderror: 
- You are right @tomaarsen load_and_save_embed_model.ipynb seems to work I just tried it, sorry about that. It was not a good example it appears.
- Here is a link to what should be a working example: https://colab.research.google.com/drive/1OLnK9J-gZt5S0v7Yuw08jLD7t_BS-jGw?usp=sharing (I saw there is a bug in the display of the gists, so sharing the notebook directly).
- Note the  !pip install --upgrade sentence-transformers 
- Note the !pip install --upgrade transformers
- peft version seems to be 0.14.0

About the second error (using my fork of transformers and the fix that comes with it) here is the link to the notebook: https://colab.research.google.com/drive/1FfCUN1zwn0a7jG1WnDadXlmM4EFIL7wW?usp=sharing

Hope that helps.



friendly ping @tomaarsen @BenjaminBossan 

@GTimothee I can confirm that this code breaks with the changes on your branch. When I add a `get_base_model` method on `SentenceTransformer` like so, it works (there is an error later, but that might be a version thing):

```python
class MySentenceTranformer(SentenceTransformer):
    def get_base_model(self):
        return self[0].auto_model
```

This is probably not the right way to solve the issue, @tomaarsen should know more.

I can confirm that adding `get_base_model` resolves the immediate issue, and I'm not opposed to applying this fix to Sentence Transformers, but more issues will be encountered later on. For example, here: https://github.com/huggingface/transformers/blob/dc06e7cecd5dc98681566e5201481b42583c4382/src/transformers/trainer.py#L2958-L2964

@BenjaminBossan Huh, the expectation here is that `model.active_adapters` or `model.active_adapter` are properties, whereas in Sentence Transformers they are methods. I just checked in Transformers as well, and they're also methods there? https://github.com/huggingface/transformers/blob/dc06e7cecd5dc98681566e5201481b42583c4382/src/transformers/integrations/peft.py#L410-L447

Is this a bug, or is `model` a PeftModel instead of a `PreTrainedModel` perhaps?

- Tom Aarsen


> the expectation here is that `model.active_adapters` or `model.active_adapter` are properties, whereas in Sentence Transformers they are methods. I just checked in Transformers as well, and they're also methods there?

Yes, the is the error I alluded to above. Unfortunately, somewhere along the way, the transformers integration was implemented with a slightly different interface than PEFT, which has led to some confusion in the past. In `PeftModel` instances, we have [properties](https://github.com/huggingface/peft/blob/main/src/peft/peft_model.py#L152-L153), whereas the transformers integration uses methods. I assume that Sentence Transformer uses the latter, but maybe it's best to account for both?

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.

Please note that issues that do not follow the [contributing guidelines](https://github.com/huggingface/transformers/blob/main/CONTRIBUTING.md) are likely to be ignored.