Pytorch-lightning: Add support for hierarchical dict

[S-aiueo32]

🚀 Feature

Motivation

Since v0.7.0, LightningModule accepts dict hparams, however, still TensorBoardLogger raises an error with hierarchical dict. Considering the compatibility of the other package, especially Hydra #807, hierarchical dict should be accepted by any loggers.

Pitch

• Flatten hierarchical dict before hparam logging

Alternatives

The function _convert_params in loggers/base.py will be changed like:

def _convert_params(self, params: Union[Dict[str, Any], Namespace]) -> Dict[str, Any]:
    # in case converting from namespace
    if isinstance(params, Namespace):
        params = vars(params)

    # added part
    params = flatten_dict(params)  # convert {'a': {'b': 'c'}} -> {'a/b': 'c'}

    if params is None:
        params = {}

    return params

[Borda]

that sounds good! we have already started some work related to params, see #1130 #1128
but still, these flatten dict are also welcome, would you mind to send a PR?
cc: @PyTorchLightning/core-contributors

[luiscape]

Given that incompatibility with TensorBoardLogger, it does make to flatten these dicts, I think. Just a minor comment: I would suggest that instead of

params = flatten_dict(params)  # convert {'a': {'b': 'c'}} -> {'a/b': 'c'}

The flattened dict would use . to indicate hierarchy:

params = flatten_dict(params)  # convert {'a': {'b': 'c'}} -> {'a.b': 'c'}

Now, is there an use-case for de-serializing these flattened dicts back into hierarchical ones?

[S-aiueo32]

Thank you for replying, @Borda. I'll send PR after some discussions here.

[S-aiueo32]

I saw the PRs #1130 #1128, then I feel there are some conflicts between the PRs.
For example, if non-primitive types are converted to str by #1130, #1128 does not have any effects.
The integrative discussion around hparams is needed I think.

[S-aiueo32]

@luiscape Thank you for the suggestions.

The flattened dict would use . to indicate hierarchy

Do you have any reasons to use . as the delimiter? I think the optimal delimiter is logger-dependent.

Now, is there an use-case for de-serializing these flattened dicts back into hierarchical ones?

I don't have any idea because the flattening only has the meaning of parsing for logging.
I assume the original hierarchical dict is stored as a member of LightningModule, so I think the users don't need it.

[S-aiueo32]

PR is ready here!
After merging of #1130, I'll send it.

[Borda]

I saw the PRs #1130 #1128, then I feel there are some conflicts between the PRs.
For example, if non-primitive types are converted to str by #1130, #1128 does not have any effects.

Could you pls comment on these PRs thow...

The integrative discussion around hparams is needed I think.

Could you open an issue and raise/start such discussion (or jsut continue in the Hydra one?)

[S-aiueo32]

Could you pls comment on these PRs thow...

I commented in #1128.

Could you open an issue and raise/start such discussion (or jsut continue in the Hydra one?)

Yeah, I'm going to open issues.

[luiscape]

@S-aiueo32 I suggested using . as separators to indicate hierarchy just because of style, really. I think that's more intuitive given the use of . in the package / module convention of Python.

If there's further functionality added by using / let's do that instead.

[jeremyjordan]

Could we implement this in such as way to enable the use of writer.add_scalars() for the Tensorboard version? When you use this method it plots the values on a single chart (versus grouping charts with individual metrics when you organize tags like parent/child).

eg.

versus

[S-aiueo32]

@luiscape
Hmm, honestly I think either . or / are fine.
We should use / if we work along with TensorBoard manner, however, it doesn't have any functionality in the hparam section. 😅
Anyway, the delimiter can be controlled by the argument in my current branch head.

[S-aiueo32]

@jeremyjordan
My simple answer is NO. The latter is the correct way in TensorBoard fashion.
To implement the former, we need multiple event files logging the metrics with the same tags, which is hacky and cluttered.