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.

Sign up for GitHub

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

Jump to bottom

hparams as dict #1029

Merged
merged 33 commits into from Mar 4, 2020
Merged

hparams as dict #1029

merged 33 commits into from Mar 4, 2020

Conversation

Borda
Copy link
Member

@Borda Borda commented Mar 3, 2020

What does this PR do?

Fixes #977

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

@Borda Borda added the feature Is an improvement or enhancement label Mar 3, 2020
@pep8speaks
Copy link

pep8speaks commented Mar 3, 2020 •
edited

Hello @Borda! Thanks for updating this PR.

Line 129:101: E501 line too long (101 > 100 characters)

Comment last updated at 2020-03-04 14:26:43 UTC

@williamFalcon williamFalcon added this to the 0.7.0 milestone Mar 3, 2020
@Borda Borda marked this pull request as ready for review March 3, 2020 18:39
@Borda Borda requested a review from ethanwharris March 3, 2020 18:39
yukw777
yukw777 reviewed Mar 3, 2020
View reviewed changes
Copy link
Contributor

@yukw777 yukw777 left a comment

Choose a reason for hiding this comment

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

This is awesome! Maybe we shouldn't have fixed the doc around this haha.

pytorch_lightning/loggers/neptune.py Outdated Show resolved Hide resolved
pytorch_lightning/loggers/tensorboard.py Outdated Show resolved Hide resolved
pytorch_lightning/loggers/test_tube.py Outdated Show resolved Hide resolved
pytorch_lightning/loggers/wandb.py Outdated Show resolved Hide resolved
tests/models/base.py Outdated Show resolved Hide resolved
@Borda
Copy link
Member Author

Borda commented Mar 3, 2020

This is awesome! Maybe we shouldn't have fixed the doc around this haha.

well, unless we define the dict type like Dict[str, Any] it does not matter :]

neggert
neggert reviewed Mar 3, 2020
View reviewed changes
tests/models/base.py Outdated Show resolved Hide resolved
@awaelchli
Copy link
Member

awaelchli commented Mar 3, 2020 •
edited

Correct me if I'm wrong. This does NOT solve #977. The discussion there is not over. As far as I can see this PR does not address the issue we have with restoring the hparams. This pr also adds other changes unrelated to hparams, why?

@Borda Borda changed the title hparams as dict hparams as dict [wip] Mar 3, 2020
awaelchli
awaelchli reviewed Mar 3, 2020
View reviewed changes
pytorch_lightning/core/lightning.py Outdated
hparams = Namespace(**self._hparams)
return hparams

def set_hparams(self, params: Union[Dict[str, Any], Namespace]):
Copy link
Member

Choose a reason for hiding this comment

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

setters in python should be done with @hparams.setter decorator, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

can you in decorator define types?

Copy link
Member

Choose a reason for hiding this comment

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

@hparams.setter
def hparams(self, params: Union[Dict[str, Any], Namespace]):
    self._hparams = params

This should work. However, it does not enforce the types. The user can still pass in whatever it wants.

@Borda
Copy link
Member Author

Borda commented Mar 3, 2020

Correct me if I'm wrong. This does NOT solve #977. The discussion there is not over. As far as I can see this PR does not address the issue we have with restoring the hparams. This pr also adds other changes unrelated to hparams, why?

yeah, sorry, made is WIP, I have added somment to the original thread, could you check it pls...

@awaelchli awaelchli mentioned this pull request Mar 3, 2020
Closed
@awaelchli
Copy link
Member

awaelchli commented Mar 3, 2020 •
edited

@Borda I see a small edge case (maybe).

def __init__(params):
    self.myparams = params  # the user does not set self.params
    # later in code ...
    self.myparams['batch_size']  # the user works with a dict. Not allowed!

user trains model ...
user wants to load the checkpoint now:
model = MyLightningModule.load_from_checkpoint(...) # this will pass in an empty Namespace to the module, because the checkpoint contains empty hparams Namespace
The model will not work. I guess this problem already existed before, but I think there was a warning when hparams was not set.

@williamFalcon
Copy link
Contributor

great job guys! let's wrap this up in the next few hours so we can do the release. this is the last PR holding this up

@Borda
Copy link
Member Author

Borda commented Mar 3, 2020

@awaelchli I see a solution to have self._hparams as dict always and property self.hparams as Namespace so it does not break anything and if you need you may do

self.hparams.batch_size
self._hparams['batch_size']

@awaelchli
Copy link
Member

awaelchli commented Mar 3, 2020 •
edited

Yes, that's one way. It would also mean from now on self.hparams is always going to be a Namespace, and it would break exising code bases where they had a dict or something else. Is this acceptable?

@Borda
Copy link
Member Author

Borda commented Mar 4, 2020

@williamFalcon @awaelchli in checkpoint the hparams are always as dict or am I missing something?
https://github.com/PyTorchLightning/pytorch-lightning/blob/f6934e5f14589ef3d35cfdae7e3c74eb3020637b/pytorch_lightning/trainer/training_io.py#L240-L241

@awaelchli
Copy link
Member

awaelchli commented Mar 4, 2020 •
edited

@williamFalcon Yes there should probably a save and load test with dict.
However, the user can always break the system, for example when accessing the arg before setting sefl.hparams = arg it could lead to typeerror.

@Borda
Copy link
Member Author

Borda commented Mar 4, 2020

FYI pytorch/vision#1938

@williamFalcon
Copy link
Contributor

this code forces a load as namespace. if the user model defined a dictionary then it would break no? because the user would access hparams[“arg”] but since we always force a namespace that would crash?

@awaelchli
Copy link
Member

@williamFalcon if you want that behaviour I see no way around pickeling the whole hparams object or saving the type with it as I suggested in #943 . Otherwise at load time we have no way of telling the type the user chose.

williamFalcon
williamFalcon reviewed Mar 4, 2020
View reviewed changes
pytorch_lightning/trainer/training_io.py Outdated
checkpoint['hparams'] = vars(model.hparams)
is_namespace = isinstance(model.hparams, Namespace)
checkpoint['hparams'] = vars(model.hparams) if is_namespace else model.hparams
checkpont['hparams_type'] = 'namespaces' if is_namespace else 'dict'
Copy link
Contributor

Choose a reason for hiding this comment

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

@awaelchli @Borda fixed it... now need to fix load

Copy link
Member

Choose a reason for hiding this comment

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

how does that address your concern about hparams[".." ] access? load_from_checkpoint will pass a Namespace to Lightningmodule.

Copy link
Member

Choose a reason for hiding this comment

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

ok nvm :)

Copy link
Member Author

Choose a reason for hiding this comment

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

well I would rather stay with the fixed interface - hparams as Namespace property and setter

@williamFalcon williamFalcon changed the title hparams as dict hparams as dict [blocked by 1041] Mar 4, 2020
@williamFalcon williamFalcon merged commit e586ed4 into Lightning-AI:master Mar 4, 2020
@Borda Borda changed the title hparams as dict [blocked by 1041] hparams as dict Mar 4, 2020
@Borda Borda deleted the hparams branch March 4, 2020 14:38
@awaelchli awaelchli mentioned this pull request Mar 5, 2020
Closed
tullie pushed a commit to tullie/pytorch-lightning that referenced this pull request Apr 3, 2020
@Borda @williamFalcon @tullie
hparams as dict [blocked by 1041] (Lightning-AI#1029)
a2ebb11 
* hparams as dict

* hparams as dict

* fixing

* fixing

* fixing

* fixing

* typing

* typing

* chnagelog

* update set hparams

* use setter

* simplify

* chnagelog

* imports

* pylint

* typing

* Update training_io.py

* Update training_io.py

* Update lightning.py

* Update test_trainer.py

* Update __init__.py

* Update base.py

* Update utils.py

* Update test_trainer.py

* Update training_io.py

* Update test_trainer.py

* Update test_trainer.py

* Update test_trainer.py

* Update test_trainer.py

* Update callback_config.py

* Update callback_config.py

* Update test_trainer.py

Co-authored-by: William Falcon <waf2107@columbia.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neggert neggert neggert left review comments

@yukw777 yukw777 yukw777 left review comments

@williamFalcon williamFalcon williamFalcon left review comments

@awaelchli awaelchli awaelchli left review comments

@ethanwharris ethanwharris Awaiting requested review from ethanwharris

Assignees
No one assigned
Labels
feature Is an improvement or enhancement
Projects
None yet
Milestone
pl:0.7
Development

Successfully merging this pull request may close these issues.

Support storing hparams as a dict
6 participants
@Borda @pep8speaks @awaelchli @williamFalcon @neggert @yukw777