[wip] support save_hyperparameters after mutation

[tbenst]

What does this PR do?

Here's a WIP pull request that closes #3494 and closes #3522.

I had hoped this would be a simple one line fix, like getattr(self, x) if hasattr(self,x) else init_args[x] and it nearly was, but upon running tests I found that pytorch-lightning supports a diverse array of hparams, including omega_conf and Namespace, which greatly complicates things.

Please look at the two new tests, test_hparams_inplace_mutation and test_hparams_after_mutation for understanding the intent.

This is my first pull request so greatly appreciate any help or feedback!!

Status

6 tests fail, including one that I wrote!

============================= short test summary info ==============================
FAILED tests/models/test_hparams.py::test_namespace_hparams[SaveHparamsModel] - A...
FAILED tests/models/test_hparams.py::test_dict_hparams[SaveHparamsModel] - Attrib...
FAILED tests/models/test_hparams.py::test_omega_conf_hparams[SaveHparamsModel] - ...
FAILED tests/models/test_hparams.py::test_single_config_models_fail[AnotherArgModel-config0]
FAILED tests/models/test_hparams.py::test_single_config_models_fail[OtherArgsModel-config1]
FAILED tests/models/test_hparams.py::test_hparams_after_mutation - AttributeError...

The test I wrote that fails is surprising to me and appears to be a case of Black Magic, as only the final assert fails.

Before submitting

• Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
• Did you read the contributor guideline, Pull Request section?
• Did you make sure your PR does only one thing, instead of bundling different changes together? Otherwise, we ask you to create a separate PR for every change.
• Did you make sure to update the documentation with your changes?
• Did you write any new necessary tests?
• Did you verify new and existing tests pass locally with your changes?
• If you made a notable change (that affects users), did you update the CHANGELOG?

PR review

@edenlightning @Borda

[pep8speaks]

Hello @tbenst! Thanks for updating this PR.

• In the file pytorch_lightning/core/lightning.py:

Line 1343:44: E231 missing whitespace after ':'
Line 1346:24: E231 missing whitespace after ','
Line 1347:39: W291 trailing whitespace
Line 1404:75: W291 trailing whitespace
Line 1406:1: W293 blank line contains whitespace
Line 1410:1: W293 blank line contains whitespace
Line 1419:1: W293 blank line contains whitespace

• In the file tests/models/test_hparams.py:

Line 545:1: E303 too many blank lines (3)
Line 555:1: E302 expected 2 blank lines, found 1
Line 566:1: E302 expected 2 blank lines, found 1

Comment last updated at 2020-10-07 09:53:12 UTC

[mergify]

This pull request is now in conflict... :(

[mergify]

This pull request is now in conflict... :(

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. If you need further help see our docs: https://pytorch-lightning.readthedocs.io/en/latest/CONTRIBUTING.html#pull-request or ask the assistance of a core contributor here or on Slack. Thank you for your contributions.

[Borda]

@tbenst is it ready to review/finish it?

[awaelchli]

@tbenst
Could you check my comment here #4316 (comment)
regarding reproducibility? I think it is ok if we add the possibility to call the save function on updated values, however, the user should be aware of the side effects it can cause.

[Borda]

@tbenst there won't be any more releases in 1.1.x so I have changed your destination branch to feat 1.2
pls, there is no need to close this PR, just rebase on the target branch, or maybe easier just cherry-pick the commit (@kaushikb11 will help you if needed)

[tchaton]

Dear @tbenst,

Would you mind resolve the conflicts ?

Best,
T.C

[Borda]

@tbenst is this still WIP or ready to review?

[tbenst]

Sorry for my long delay. Finally back working on a project using pytorch lightning. I'm happy to update this PR.

I've updated to latest version, but I'm having trouble getting the auto_lr_find to work at all:

• previously, could follow trainer.fit(model, dataloader) pattern. Now, it seems have to specify def train_loader(self). This is not noted in the documentation
• it seems that using a vanilla PyTorch Dataloader in conjunction with auto_lr_find is broken due to https://github.com/PyTorchLightning/pytorch-lightning/blob/8ba6304c73c0fd030c50d5de52cbaddfed161613/pytorch_lightning/utilities/model_helpers.py#L42. DataLoaders do not have a __code__ attribute.

error trace

---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
~/code/babelfish/notebooks/lstm_denoise.py in 
     261                     #  default_root_dir=save_dir
     262                      )
---> 263 trainer.tune(model)
     264 # trainer.fit(model, torch.utils.data.DataLoader(dataset, batch_size=None))
     265 trainer.fit(model, dataloader)

~/opt/anaconda3/envs/bf/lib/python3.8/site-packages/pytorch_lightning/trainer/trainer.py in tune(self, model, train_dataloader, val_dataloaders, datamodule)
    813 
    814         """
--> 815         self.tuner.tune(model, train_dataloader, val_dataloaders, datamodule)
    816 
    817     def call_setup_hook(self, model):

~/opt/anaconda3/envs/bf/lib/python3.8/site-packages/pytorch_lightning/tuner/tuning.py in tune(self, model, train_dataloader, val_dataloaders, datamodule)
     35     def tune(self, model, train_dataloader, val_dataloaders, datamodule):
     36         # setup data, etc...
---> 37         self.trainer.train_loop.setup_fit(model, train_dataloader, val_dataloaders, datamodule)
     38 
     39         # hook

~/opt/anaconda3/envs/bf/lib/python3.8/site-packages/pytorch_lightning/trainer/training_loop.py in setup_fit(self, model, train_dataloader, val_dataloaders, datamodule)
    110 
    111         # check that model is configured correctly
--> 112         self.trainer.config_validator.verify_loop_configurations(model)
    113 
    114     def setup_training(self, model: LightningModule):

~/opt/anaconda3/envs/bf/lib/python3.8/site-packages/pytorch_lightning/trainer/configuration_validator.py in verify_loop_configurations(self, model)
     33         """
     34         if not self.trainer.testing:
---> 35             self.__verify_train_loop_configuration(model)
     36             self.__verify_eval_loop_configuration(model, 'validation')
     37         else:

~/opt/anaconda3/envs/bf/lib/python3.8/site-packages/pytorch_lightning/trainer/configuration_validator.py in __verify_train_loop_configuration(self, model)
     53         # verify model has a train dataloader
     54         # -----------------------------------
---> 55         has_train_dataloader = is_overridden('train_dataloader', model)
     56         if not has_train_dataloader:
     57             raise MisconfigurationException(

~/opt/anaconda3/envs/bf/lib/python3.8/site-packages/pytorch_lightning/utilities/model_utils.py in is_overridden(method_name, model)
     40         is_overridden = instance_attr.patch_loader_code != str(super_attr.__code__)
     41     else:
---> 42         is_overridden = instance_attr.__code__ is not super_attr.__code__
     43     return is_overridden

AttributeError: 'DataLoader' object has no attribute '__code__'

Edit: realize it may not be clear that the original motivation for this PR is that auto_lr_find is a good example of how hparams might be modified post-init & yet we want to save the mutated params.

[codecov]

Codecov Report

Merging #3622 (8ba6304) into master (9c415d2) will increase coverage by 6%.
The diff coverage is n/a.

❗ Current head 8ba6304 differs from pull request most recent head 8ce4d2f. Consider uploading reports for the commit 8ce4d2f to get more accurate results

@@           Coverage Diff            @@
##           master   #3622     +/-   ##
========================================
+ Coverage      86%     92%     +6%     
========================================
  Files         113     199     +86     
  Lines        8551   12957   +4406     
========================================
+ Hits         7361   11963   +4602     
+ Misses       1190     994    -196     

[carmocca]

Hi @tbenst! Thanks for looking into this PR again.

I have not been able to reproduce any of those issues using our bug report model: https://github.com/PyTorchLightning/pytorch-lightning/blob/master/pl_examples/bug_report_model.py

Please, give it a try and open separate issues for those if you can reproduce. It's easier for us to track it that way.

[cowwoc]

What is the status of this PR?

[carmocca]

Closing this PR as it has become stale. We would agree to merge this if somebody wants to take over the authorship. Thank you for your time!