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

Mismatch between docstring and code regarding when on_load_checkpoint hook is called #3993

Closed
hbredin opened this issue Oct 8, 2020 · 3 comments · Fixed by #3996
Closed
Labels
bug Something isn't working docs Documentation related help wanted Open to be worked on

Comments

@hbredin
Copy link
Contributor

hbredin commented Oct 8, 2020

🐛 Bug

The docstring of on_load_checkpoint hook says that it is called before trying to load_state_dict:
https://github.com/PyTorchLightning/pytorch-lightning/blob/cea5f1f53876399dfaa0d37accdc527af7ca39af/pytorch_lightning/core/saving.py#L203-L206

However, in LightningModule.load_from_checkpoint, it is called after load_state_dict:
https://github.com/PyTorchLightning/pytorch-lightning/blob/cea5f1f53876399dfaa0d37accdc527af7ca39af/pytorch_lightning/core/saving.py#L195-L199

Additional context

Related discussion on Slack: https://pytorch-lightning.slack.com/archives/CQXV8BRH9/p1602168345184000

I think the docstring is correct and the call to on_load_checkpoint should be moved right before load_state_dict to give the model a chance to call setup.

@hbredin hbredin added bug Something isn't working help wanted Open to be worked on labels Oct 8, 2020
@Borda
Copy link
Member

Borda commented Oct 8, 2020

@hbredin mind sending a PR to fix it... 🐰

@hbredin
Copy link
Contributor Author

hbredin commented Oct 8, 2020

I can do that. Should I fix the docstring or the code?
I'd go with the code.

@Borda Borda added the docs Documentation related label Oct 8, 2020
hbredin added a commit to hbredin/pytorch-lightning that referenced this issue Oct 8, 2020
williamFalcon pushed a commit that referenced this issue Oct 9, 2020
@NumesSanguis
Copy link
Contributor

I need this code change as well! I'm doing transfer learning and I want to support both loading the original model with the original weights, and modify it for a new task.
on_load_checkpoint would allow me to redo the modifications I've done for transfer learning, so the state_dict of the transferred model can be properly restored.

At present I need to add non-network code to the model to handle this logic, which is ugly and prone to bugs.

This would allow to have the same model to redo the modifications I've made for transfer learning,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working docs Documentation related help wanted Open to be worked on
Projects
None yet
3 participants