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 hierarchical dict #1152

Merged

Conversation

S-aiueo32
Copy link
Contributor

@S-aiueo32 S-aiueo32 commented Mar 15, 2020

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?

Fixes #1144.

The main change of this PR is adding the function _flatten_dict to LightningLoggerBase.
This function flattens hierarchical dictionaries and nested Namespace objects:

# hierarchical dict
_flatten_dict({'a': {'b': 'c'}})  # => {'a/b': 'c'}

# nested Namespace
namespace = Namespace(foo=Namespace(bar='buzz'))
namespace = vars(namespace)  # this conversion for top-level is done in `_convert_params`
_flatten_dict(namespace )  # => {'foo/bar': 'buzz'}

Considering the compatibility to all loggers, _flatten_dict is called in the log_hyperparams function of each logger class that needs flattening:

def log_hyperparams(self, params: Union[Dict[str, Any], Namespace]) -> None:
    params = self._convert_params(params)
    params = self._flatten_dict(params)

In other words, this function is not called by the loggers as represented by WanbLogger, which supports hierarchical dictionaries by default, to keep them the hierarchy.

Note1: WanbLogger supports hierarchical dictionaries, but not nested Namespace. It does not raise an error, however, the output yaml file on the UI is displayed as:

foo:
  desc: null
  value: Namespace(bar='buzz')

Note2: TRAINS also supports hierarchical dictionaries, but the UI destroys the hierarchy as done by _flatten_dict. To add support for nested Namespace, TrainsLogger calls _flatten_dict.

@codecov
Copy link

codecov bot commented Mar 15, 2020

Codecov Report

Merging #1152 into master will increase coverage by <1%.
The diff coverage is 95%.

@@           Coverage Diff           @@
##           master   #1152    +/-   ##
=======================================
+ Coverage      93%     93%   +<1%     
=======================================
  Files          61      61            
  Lines        2672    2691    +19     
=======================================
+ Hits         2472    2491    +19     
  Misses        200     200

@awaelchli
Copy link
Contributor

very nice!

@Borda Borda added the feature Is an improvement or enhancement label Mar 16, 2020
@Borda Borda added this to the 0.7.2 milestone Mar 16, 2020
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.

LGTM 🚀

pytorch_lightning/loggers/base.py Outdated Show resolved Hide resolved
pytorch_lightning/loggers/base.py Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@Borda Borda added the ready PRs ready to be merged label Mar 18, 2020
@williamFalcon williamFalcon merged commit 01b8991 into Lightning-AI:master Mar 19, 2020
tullie pushed a commit to tullie/pytorch-lightning that referenced this pull request Apr 3, 2020
* Add support for hierarchical dict

* Support nested Namespace

* Add docstring

* Migrate hparam flattening to each logger

* Modify URLs in CHANGELOG

* typo

* Simplify the conditional branch about Namespace

Co-Authored-By: Jirka Borovec <[email protected]>

* Update CHANGELOG.md

Co-Authored-By: Jirka Borovec <[email protected]>

* added examples section to docstring

* renamed _dict -> input_dict

Co-authored-by: Jirka Borovec <[email protected]>
@KeAWang
Copy link

KeAWang commented Jun 7, 2020

Is there a reason that wandb.py wasn't changed to flatten dict?

@awaelchli
Copy link
Contributor

probably got forgotten, is already done, or not necessary. Did you check, is it necessary to do it @KeAWang ?

@awaelchli
Copy link
Contributor

awaelchli commented Jun 7, 2020

See here: https://docs.wandb.com/library/config

You can send us a nested dictionary in config, and we'll flatten the names using dots in our backend.

They have thought of it already :)

@Borda Borda modified the milestones: v0.7., v0.7.x Apr 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Is an improvement or enhancement ready PRs ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for hierarchical dict
6 participants