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

FIX: explicitly set weights_only to avoid FutureWarning #193

Merged
merged 6 commits into from
Jul 25, 2024

Conversation

scott-huberty
Copy link
Contributor

@scott-huberty scott-huberty commented Jul 25, 2024

Closes #192

If I'm understanding the Torch 2.4 changelog correctly, you just need to explicitly pass a value to weights_only to suppress the warning. Since the default is already False, I am just explicitly setting it here, so this should be backward compatible

I'll double check that the CI installs 2.4 so we can make sure this fix works.

Maintainer, please confirm the following before merging:

  • All comments resolved
  • All CIs are happy
  • Updated a changelog entry and contributor name in whats_new.rst is updated
  • PR description includes phrase "closes <#issue-number>"

- If I'm understanding the Torch 2.4 changelog correctly, you just need to explicitly pass a value to weights_only. Since the default is alraedy False, I am just explicitly setting it here so this should be backward compatible
Copy link
Member

@mscheltienne mscheltienne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @scott-huberty
We are loading a file we distribute, thus I think it's safe to keep loading with weights_only=False.
I pushed 2 commits which should get the CIs green.

@mscheltienne mscheltienne enabled auto-merge (squash) July 25, 2024 15:34
@mscheltienne mscheltienne disabled auto-merge July 25, 2024 15:36
@mscheltienne
Copy link
Member

On the other hand.. weights_only=True does not fail the tests. I haven't played with torch in a long time, @adam2392 do you have an opinion on which one should be set?

@adam2392
Copy link
Member

https://pytorch.org/docs/stable/generated/torch.load.html

Seems to suggest if we set to True we introduce security issues. I don't see any issue with just keeping it as default of false.

@scott-huberty
Copy link
Contributor Author

https://pytorch.org/docs/stable/generated/torch.load.html

I think it's the other way around. weights_only=True is safer because it restricts the deserialization to only tensors, primitive types, and dictionaries etc. whereas with weights_only=False, pickle could deserialize arbitrary/malicious python objects.

But I think they made/kept the default weights_only=False for backwards compatibility reasons:

pytorch/pytorch#87443

@adam2392
Copy link
Member

Sorry yeah oops mixed it up.

But yeah I don't see any reason we can't just use weights_only as True. Is there any issue with this? I don't think we load any non PyTorch primitives.

@mscheltienne
Copy link
Member

But yeah I don't see any reason we can't just use weights_only as True. Is there any issue with this? I don't think we load any non PyTorch primitives.

Well that's the part I don't know and am not familiar with.. but the unit tests seem to agree since they come back green locally with weights_only=True.

@scott-huberty
Copy link
Contributor Author

Agreed that ideally weights_only=True is preferred/safer but I also wasn't sure if that would break your code so went with False. I'm OK with changing it to True if you all are.

@mscheltienne
Copy link
Member

Let's go with True, IIRC we have tests which compare a forward pass in the model against numerical value outputted by the same forward pas in MATLAB, they would have failed if True was messing up something.

Thanks @scott-huberty I'll cut a release tomorrow to get your CIs green.

@mscheltienne mscheltienne merged commit 3cfc1ab into mne-tools:main Jul 25, 2024
17 checks passed
@scott-huberty
Copy link
Contributor Author

Thanks @scott-huberty I'll cut a release tomorrow to get your CIs green.

Thx @mscheltienne ! No rush in the meantime I'm just going make use of filterwarnings ; )

@mscheltienne
Copy link
Member

@scott-huberty 0.7 is out ;) Thanks!

@scott-huberty scott-huberty deleted the torch_load branch July 26, 2024 16:26
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.

FutureWarning in latest pytorch release
3 participants