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

Support OmegaConf in Lightning #1883

Closed
wants to merge 4 commits into from
Closed

Conversation

Darktex
Copy link

@Darktex Darktex commented May 19, 2020

Lightning hparams were recently changed to support only dicts and ArgParse namespaces. This diff (re)introduces compatibility with OmegaConf DictConfig, which is the type of config that Hydra uses.

What does this PR do?

Fixes #1880.

@pep8speaks
Copy link

pep8speaks commented May 19, 2020

Hello @Darktex! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-05-22 15:00:06 UTC

@mergify mergify bot requested a review from a team May 19, 2020 02:26
Copy link
Contributor

@tullie tullie left a comment

Choose a reason for hiding this comment

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

lgtm just fix the formatting comment

pytorch_lightning/trainer/training_io.py Show resolved Hide resolved
@mergify mergify bot requested a review from a team May 19, 2020 02:33
@yukw777
Copy link
Contributor

yukw777 commented May 19, 2020

How does this PR play with this discussion: #1871?

@williamFalcon
Copy link
Contributor

let’s add support for this now while we work out a good long-term solution. Thanks for the PR!

@mergify mergify bot requested a review from a team May 19, 2020 05:47
Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

need to add loading from DictConfig

@mergify mergify bot requested a review from a team May 19, 2020 06:08
@tullie
Copy link
Contributor

tullie commented May 19, 2020

@Borda Can you explain your comment a bit more? I don't understand the issue

@Borda
Copy link
Member

Borda commented May 20, 2020

@Borda Can you explain your comment a bit more? I don't understand the issue

https://github.com/PyTorchLightning/pytorch-lightning/blob/9b629637b890a1eb9a0d9011bfa10322e0d89da0/pytorch_lightning/core/lightning.py#L1613-L1617

@ImMrMa
Copy link

ImMrMa commented May 20, 2020

@Borda Can you explain your comment a bit more? I don't understand the issue

https://github.com/PyTorchLightning/pytorch-lightning/blob/9b629637b890a1eb9a0d9011bfa10322e0d89da0/pytorch_lightning/core/lightning.py#L1613-L1617

save the DictConfig to hparms.yaml and load it need to do!

@mergify
Copy link
Contributor

mergify bot commented May 21, 2020

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

@Darktex
Copy link
Author

Darktex commented May 21, 2020

I'm editing this from the WebUI so the experience is... uh... improvable? :D But hopefully this should do the trick

@Darktex
Copy link
Author

Darktex commented May 21, 2020

I could add a test for this here https://github.com/PyTorchLightning/pytorch-lightning/blob/master/tests/trainer/test_trainer.py#L29

but it would require to add omegaconf as a dep. What do you guys think?

@codecov
Copy link

codecov bot commented May 21, 2020

Codecov Report

Merging #1883 into master will increase coverage by 0%.
The diff coverage is 52%.

@@          Coverage Diff           @@
##           master   #1883   +/-   ##
======================================
  Coverage      88%     88%           
======================================
  Files          74      74           
  Lines        4603    4608    +5     
======================================
+ Hits         4062    4069    +7     
+ Misses        541     539    -2     

@williamFalcon
Copy link
Contributor

we can add as a test dep but let’s keep it out of the main requirements. we already do this with the loggers as well

Lightning hparams were recently changed to support only dicts and ArgParse namespaces. This diff (re)introduces compatibility with OmegaConf DictConfig, which is the type of config that Hydra uses.
@mergify
Copy link
Contributor

mergify bot commented May 24, 2020

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

@Borda Borda added discussion In a discussion stage feature Is an improvement or enhancement labels May 26, 2020
@Borda
Copy link
Member

Borda commented May 26, 2020

@Darktex how is it going, mind finish it? it seems that Will was asking for some tests...

@Borda Borda added the waiting on author Waiting on user action, correction, or update label May 26, 2020
@Darktex
Copy link
Author

Darktex commented May 26, 2020

Looks like we don't need this anymore after #1896 landed

@williamFalcon
Copy link
Contributor

@Darktex i assume this means omegaconf is now supported after we generalized the init?

@williamFalcon
Copy link
Contributor

williamFalcon commented May 31, 2020

Fixed in #2025.
Reworked with new hparams.
@Borda is working on supporting the DictConfig for saving in hparams

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion In a discussion stage feature Is an improvement or enhancement waiting on author Waiting on user action, correction, or update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support OmegaConf dicts in hparams
8 participants