Support **DictConfig hparam serialization

[romesco]

What does this PR do?

Context: Passing an unpacked DictConfig to __init__() and then calling save_hyperparameters()

Bug: save_hyperparameters() deals with the unpacked DictConfig as a dict which might still contain values of type DictConfig. Upon serialization during saving, this errors out because it is handled with yaml.dump().

Fix: Enables OmegaConf to serialize the object as originally intended by casting back to a DictConfig.

Fixes: #2844

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

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?

Always.

[pep8speaks]

Hello @romesco! Thanks for updating this PR.

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

Comment last updated at 2020-08-11 10:03:26 UTC

[romesco]

Let me update this to actual casting, I was using the OmegaConf API, but this is likely creating a new object which is definitely not memory efficient...

[romesco]

For a bit more clarification, this is fixing the case where the LightningModule is defined like the following example:

class LightningTemplateModel(LightningModule):
    def __init__(self,
                 in_features: int = 28 * 28,
                 hidden_dim: int = 1000,
                 out_features: int = 10,
                 drop_prob: float = 0.2,
                 batch_size: int = 2,
                 learning_rate: float = 0.001 * 8,
                 optimizer_name: str = 'adam',
                 data_root: str = './datasets',
                 ds = None,
                 **kwargs
                 ):
        # init superclass
        super().__init__()
        self.save_hyperparameters()

followed by this constructor call:

model = LightningTemplateModel(**cfg.model, **cfg.data)

where cfg is a DictConfig from omegaconf.

I am unpacking the cfg before using it. I have a slight preference for this over passing the DictConfig directly because it respects the default keyword args. If something is missing from the cfg, the default in keyword args fills it in. It seems that the best practices interface for a LightningModule will list out all the arguments with defaults like above as it is way more explicit and easy to use. Although one could define a set of defaults in the DictConfig format, I doubt a majority of users who want to exchange portable models will do that. So far it seems either people include defaults in the add_model_specific_args() or now as a part of the __init__() signature.

With that in mind, I would prefer to unpack, but that leads me to the issue where save_hyperparameters() sees the args as their own items which might be DictConfigs themselves [this happens when OmegaConf creates a heirarchical DictConfig I guess]. That's what this fix addresses. There might be a better way to do this, and I'm open to suggestions from others of course!

[omry]

I don't understand what is an 'unpacked DictConfig'. What are you actually passing in as hparams?

Looking at the fix, it does not make much sense to me.

[Borda]

I don't understand what is an 'unpacked DictConfig'. What are you actually passing in as hparams?

like this LightningTemplateModel(**cfg.model, **cfg.data)

Looking at the fix, it does not make much sense to me.

and what you dont like?

[omry]

I see, so you a dict that have some DictConfig objects in it?
now it makes more sense.

[omry]

lgtm

[justusschock]

almost good, just some minor changes

[codecov]

Codecov Report

Merging #2519 into master will increase coverage by 1%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #2519    +/-   ##
=======================================
+ Coverage      89%     90%    +1%     
=======================================
  Files          80      80            
  Lines        7531    7425   -106     
=======================================
- Hits         6738    6709    -29     
+ Misses        793     716    -77     

[romesco]

Made all the requested changes. All (non docs) CI tests are passing!

[omry]

I think you want OmegaConf = None here, otherwise you are accessing an uninitialized variable.

[romesco]

ah you're right, forgot after removing Container to replace this

[mergify]

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

[romesco]

@Borda is this actually [blocked by #2639]?

if anything, it seems like it's blocking #2639 😅

Either way, enabling this enhancement is IMO much more minor than that PR which definitely still needs more discussion. =]