diffusers icon indicating copy to clipboard operation
diffusers copied to clipboard

[Core] Set `weights_only=True` nicely when we use `torch.load()`.

Open sayakpaul opened this issue 1 year ago • 8 comments

What does this PR do?

See internal discussion: https://pytorch.slack.com/archives/CN963QVJB/p1722612785082449.

Can propagate this PR to examples/ and scripts/ after I have an approval.

sayakpaul avatar Sep 18 '24 02:09 sayakpaul

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.

@yiyixuxu WDYT?

sayakpaul avatar Sep 18 '24 02:09 sayakpaul

can we wrap the torch_load + weights_only into a function and use it everywhere?

yiyixuxu avatar Sep 18 '24 19:09 yiyixuxu

actually, we already have one, no? https://github.com/huggingface/diffusers/blob/5d476f57c58c3cf7f39e764236c93c267fe83ca1/src/diffusers/models/model_loading_utils.py#L98

yiyixuxu avatar Sep 18 '24 19:09 yiyixuxu

That could work! Will update the PR.

sayakpaul avatar Sep 19 '24 02:09 sayakpaul

just want to bring that there some users that even though it's not recommended and risky, still want to load checkpoints with the weights_only=False option. For example: https://github.com/huggingface/diffusers/issues/8874 https://github.com/huggingface/diffusers/issues/9154

IMO it should be the default but users should be able to change it, I don't have a strong opinion about this just bringing it up to attention.

asomoza avatar Sep 19 '24 15:09 asomoza

I think better to wait for Dhruv to come back then.

sayakpaul avatar Sep 20 '24 08:09 sayakpaul

@DN6 cc I think this is relevant.

sayakpaul avatar Oct 18 '24 05:10 sayakpaul

Hey, any update on this, fyi the BC-breaking default flip is coming in torch 2.6 https://dev-discuss.pytorch.org/t/bc-breaking-change-torch-load-is-being-flipped-to-use-weights-only-true-by-default-in-the-nightlies-after-137602/2573 want to make sure everything works smoothly

Is there anything I can do to help push this along? cc @sayakpaul @DN6 @yiyixuxu

mikaylagawarecki avatar Nov 05 '24 18:11 mikaylagawarecki

hi @mikaylagawarecki I think we've flipped the default long time ago https://github.com/huggingface/diffusers/pull/7393

cc @DN6 to confirm

yiyixuxu avatar Nov 05 '24 20:11 yiyixuxu

Hi. Yes, we flipped it because of the security issue. We don't allow setting weights_only=True when loading Diffusers models anymore. https://github.com/huggingface/diffusers/blob/76b7d86a9a5c0c2186efa09c4a67b5f5666ac9e3/src/diffusers/models/model_loading_utils.py#L144

Users would have to either convert to safetensors or deserialise those checkpoints on their own

DN6 avatar Nov 06 '24 05:11 DN6