Support storing hparams as a dict

[neggert]

🚀 Feature

Right now, we assume model.hparams is an argparse.Namespace. We've had a number of requests to support hparams as a simple dict. Let's do it.

[festeh]

It would also be great to support nested dictionaries. Especially for hydra users, because this library favors modular configs.

[awaelchli]

#943 Started also here with suggestion, but not sure what is the right way to do it.

[tstumm]

What about having a set_hparams(hparams: Union[Dict, argparse.Namespace]) function in LightningModule (or having it as an mandatory argument to the constructor), s.t. the user only needs to call either the function in the modules constructor OR calling it implicitly using a super.__init__(hparams call. It could handle arbitrary conversion logic, such that the internally used hparams object can be used the same way everywhere.

[awaelchli]

I like your idea of the mandatory hparams argument to the constructor. If the superclass takes care of storing the hparams, it even gives the user more flexibility, for example he/she can rename it to self.args or something. It would also simplify the logic in the load method (e.g. LightningModule.load_from_checkpoint).

[awaelchli]

There is still the question of how we support multiple types of hparam objects and instantiate the user's LightningModule in the load_from_checkpoint method.

[festeh]

On the other hand, adding a mandatory argument will break a lot of existing code. What if instead use a
private method _set_hparams(self) which is called after constructor has finished and looks for self.hparams? Yes, user will not able to change the hparams name to something else, but I consider it normal.

[tstumm]

@awaelchli You can just type-check them and then convert it to some common type, e.g., a dict.
Or we just constraint the type of hparams to dict and expect that every user handles conversion on their own.
@festeh That would bring in a lot of additional complexity, as one then needs to either 1) manually call that method (so you need to change your code anyway) or 2) the method is called by some instrinsic step in the Pytorch-Lightning training/loading procedure. Breaking backwards compatibility is not necessarily a bad thing.

[festeh]

@tstumm I meant the second option, this method may be called by Trainer internally after the model is passed to it. Then it can perform the steps suggested by you (type-checking and converting), like this

def _set_hparams(self):
    hparams = getattr(self, "hparams", None)
    if hparams is not None:
        if isinstance(hparams, dict):
            ...
        elif isinstance(hparams, Namespace):
            ...

I should say we need a really good reason to introduce a change that will break a lot of code.

[Borda]

hmmm, so what about having hparams as property which will be Namespace and setter set_hparams which will accept both Namespace and dict and store it in _hparams in the original form...

[awaelchli]

What if the hparams changes after setter is called? It is mutable, the user can change the values anytime. It should probably only be converted when checkpoint is saved.
And when restoring, we basically can't restore the user's original type... we don't know what to pass in to constructor. that's the big problem I see.

[awaelchli]

actually, in the PR #1029 of @Borda the conversion is done in the property, not the setter. This could work. I think it's a good idea.