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 unpickling issues with CUDA sub-attributes #431

Merged
merged 4 commits into from
Feb 5, 2019

Conversation

ottonemo
Copy link
Member

@ottonemo ottonemo commented Jan 31, 2019

Solves issues with, for example,

  • criterion__weight = <cuda tensor>
  • optimizer__state
  • module__some_weird_parameter = <cuda_tensor>

Before, criterion was never considered a CUDA dependent
attribute which is also fixed.

Solves issues with, for example,
* `criterion__weight = <cuda tensor>`
* `optimizer__state`
* `module__some_weird_parameter = <cuda_tensor>`

Before `criterion` was never considered a CUDA dependent
attribute, this is also fixed.
@ottonemo
Copy link
Member Author

ottonemo commented Feb 2, 2019

I think there was also a bug where net.cuda_dependent_attributes_ was overwritten with the torch byte array of said attributes when unpickling. This is also fixed by introducing the intermediate state variable __cuda_dependent_attributes__.

@BenjaminBossan
Copy link
Collaborator

LGTM. Could you quickly comment on why that part is no longer required:

https://github.com/dnouri/skorch/blob/master/skorch/net.py#L1422

(or was it ever?)

@ottonemo
Copy link
Member Author

ottonemo commented Feb 2, 2019

I don't think it was ever needed. Stumbled upon it a while ago and used this occasion to get rid of it.

There's one problem though, what do we do about old pickled files? These will fail to load with this change since they are missing the __cuda_dependent_attributes__ state variable. We could load the old way and issue a warning that this fail will fail to load with the next release?

This is going to be deprecated with the next non-bugfix release.
@ottonemo
Copy link
Member Author

ottonemo commented Feb 2, 2019

I've added a compatibility check so that we can direct users to this version of skorch if they have problems loading their old pickle files. In future versions this fallback can be removed.

@BenjaminBossan
Copy link
Collaborator

This looks good, just one minor thing: Could you mention in the CHANGES.md that the old pickle format is deprecated and users should re-pickle?

@ottonemo
Copy link
Member Author

ottonemo commented Feb 5, 2019

@BenjaminBossan done, please review again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants