Add support for logging hydra's hparams to TensorBoard.

[lkhphuc]

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 to update the docs?
• Did you write any new necessary tests?
• If you made a notable change (that affects users), did you update the CHANGELOG?

What does this PR do?

As discussed in #807, currently I can incorporate Hydra with Pytorch-Lightning easily. The only thing that's missing is hparams logging to Tensorboard.

Usage becomes:

• in conf/config.yaml

data:
  - path: my_dataset/
model:
  - pretrained: albert-based-uncased
  - num_layers: 2
  - dropout: 0.5
optim:
  - lr: 3e-4
  - momentum: 0.99
  - scheduler: CosineLR

# Group all pytorch-lightning trainer args under this
trainer:
  - gpus: 1
  - val_percent_check: 0.1
  .... 

• in train.py

@hydra.main(config_file="conf/config.yaml")
def main(cfg: DictConfig):
    module = MyModule(hparams=cfg)
    trainer = Trainer(**OmegaConf.to_container(cfg.trainer, resolve=True))
    trainer.fit(module)

For this PR I think no document is needed, but if needed I can write up some minimal examples to use Hydra with Lightning.

[codecov]

Codecov Report

Merging #1417 into master will decrease coverage by <1%.
The diff coverage is 71%.

@@           Coverage Diff           @@
##           master   #1417    +/-   ##
=======================================
- Coverage      92%     92%   -<1%     
=======================================
  Files          63      63            
  Lines        3384    3389     +5     
=======================================
+ Hits         3104    3107     +3     
- Misses        280     282     +2

[pep8speaks]

Hello @lkhphuc! Thanks for updating this PR.

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

Comment last updated at 2020-04-22 13:49:19 UTC

[Borda]

changelog need to be rebased on new release #1419

[lkhphuc]

Will do when it's merged.

[mergify]

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

[omry]

trainer = Trainer(**OmegaConf.to_container(cfg.trainer, resolve=True))

I recommend against converting the OmegaConf object to dict unless there is absolutely no choice.
There are a few features offered by OmegaConf that are lost when converting to primitive containers.

For example, interpolation and mutation validation (The second will become a much bigger thing in the upcoming OmegaConf 2.0 and Hydra 1.0, with the introduction of Structured configs).

[lkhphuc]

If there's a way to use Hydra without the ugly ** keywords arguments expansion I would definitely use it, but as I tried up to now this is the only way that works.

There are a few features offered by OmegaConf that are lost when converting to primitive containers.

As these are only trainer's flag and handled by Lightning, I don't see any lost benefit for user at all. User can still use all the features provided by Hydra when pass in the config to their own Module.

[omry]

If there's a way to use Hydra without the ugly ** keywords arguments expansion I would definitely use it, but as I tried up to now this is the only way that works.

Have you tried this?

trainer = Trainer(**cfg.trainer)

There are a few features offered by OmegaConf that are lost when converting to primitive containers.

As these are only trainer's flag and handled by Lightning, I don't see any lost benefit for user at all. User can still use all the features provided by Hydra when pass in the config to their own Module.

This is only partially true: you will get inferior error reporting for code or configuration errors.

[Borda]

@lkhphuc how is it going, do you need my help to finish it?
btw, I think it is linked to #1271

[omry]

Please pin to 0.11.3 for now.
The next version (1.0.0) will break some compatibility and it would be good to test it before you allow it.

One of the most important changes is that the supported Python versions are changing from >= 2.7 to >=3.6.

I will have a release candidate for Hydra 1.0 in a few weeks, at that point, after a test - you can relax the restriction.

[lkhphuc]

Hi @Borda , I rebased on master and pinned the hydra version for now as suggested.

This is a very simple PR, instead of passing a Namespace to Module's hparams, you can pass a DictConfig and everything from Lightining still works fine (this PR fixes the last problem of Tensorboard logging hyper-parameters).

This is mostly for my own use case so I'm not sure if it's aligned with Lightning roadmap in the future or not, just think this patch might help.
If Lightning + Hydra is planed for future deep integration and this patch might complicated things then feel free to close this one.

[Borda]

@williamFalcon what do you say about adding this dependence?

[mergify]

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

[Borda]

@williamFalcon can you give your opinion on adding this extra dependency?

[omry]

I don't think it makes sense.

1. OmegaConf dependency is enough for this, there is absolutely no reason to depend on Hydra here.
2. This can probably also be done in userland, converting the config object to a primitive before passing it in. (OmegaConf.to_container(params, resolve=True))

[omry]

As I said earlier, I would be happy to chat with the PytorchLightning leads about what a proper integration might look like.

[Borda]

@williamFalcon ^^

[williamFalcon]

hi @omry ping me on lightning slack or pt slack?

[omry]

hi @omry ping me on lightning slack or pt slack?

sent you a direct message in the lightning slack.

[reactivetype]

Adding hydra as dependency may add tech debt later. I suggest to improve support for Omegaconf as alternative to argparse's Namespace. I think PL does support this already to some extent.

[williamFalcon]

yeah, no hydra dep. I need to think about it this week but we should make it easy to use hydra.