FEAT: Make safe serialization the default one #1088
Conversation
|
|
||
| # check if `adapter_config.json` is present | ||
| self.assertTrue(os.path.exists(os.path.join(tmp_dirname, "adapter_config.json"))) | ||
|
|
||
| # check if `pytorch_model.bin` is not present | ||
| self.assertFalse(os.path.exists(os.path.join(tmp_dirname, "pytorch_model.bin"))) | ||
| # check if `model.safetensors` is not present |
There was a problem hiding this comment.
This change in necessary as now in transformers we don't save anymore pytorch-model.bin but the safetensors model
|
The documentation is not available anymore as the PR was closed or merged. |
There was a problem hiding this comment.
No full review yet, but some general questions:
- Do we want to do a deprecation/future warning cycle before making safetensors the default? How was it done in transformers?
- Do we want to run
_test_save_pretrainedtwice, once with safetensors, once with pytorch?
|
Thanks @BenjaminBossan !
Yes this is what I did in the PR, I kept |
Oh I see. I wonder if I think the new test is not quite a regression test, as regression testing would require the model to be persisted with one PEFT version and loaded with another PEFT version. I think we should really try to advance #995, which adds regression tests which would also help cover this case. |
| @@ -287,7 +287,10 @@ def _test_save_pretrained(self, model_id, config_cls, config_kwargs): | |||
| model = model.to(self.torch_device) | |||
|
|
|||
| with tempfile.TemporaryDirectory() as tmp_dirname: | |||
| model.save_pretrained(tmp_dirname) | |||
| if safe_serialization: | |||
There was a problem hiding this comment.
I prefer to not pass safe_serialization=safe_serialization in save_pretrained to test the native behaviour, let me know if this makes sense
There was a problem hiding this comment.
You mean the default behavior? I think it's okay either way.
There was a problem hiding this comment.
yes sorry I meant the default behaviour
|
We are post the patch release now and merged this branch with main, this PR is ready for another review! |
|
The documentation is not available anymore as the PR was closed or merged. |
There was a problem hiding this comment.
Thank you @younesbelkada for making safetensors the default format for saving the adapter weights 🔐! This makes it safe by avoiding the risk of random code execution cases when using pickled format.
There was a problem hiding this comment.
Thanks for making the switch and adjusting the tests. I think we can merge now, although I would imagine some users will be surprised that their code suddenly produces safetensors. We should put a big note about this on the release notes once we publish the next version.
What does this PR do?
As per title, to be in line with the current efforts in the OSS ecosystem let's also make in PEFT safe serialization the default behaviour
Adapted the tests and added some regression tests!
cc @BenjaminBossan @pacman100