bugfix: Resolve interpolation bug with Hydra #5406

tchaton · 2021-01-07T15:43:20Z

What does this PR do?

Resolves DictConfig in save_hyperparameters function.

Fixes #5384 <- this links related issue to this PR

Before submitting

Was this discussed/approved via a GitHub issue? (not for typos and docs)
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?
Did you make sure to update the documentation with your changes? (if necessary)
Did you write any new necessary tests? (not for typos and docs)
Did you verify new and existing tests pass locally with your changes?
Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

Is this pull request ready for review? (if not, please submit in draft mode)
Check that all items from Before submitting are resolved
Make sure the title is self-explanatory and the description concisely explains the PR
Add labels and milestones (and optionally projects) to the PR so it can be classified
Check that target branch and milestone match!

Did you have fun?

Make sure you had fun coding 🙃

tests/models/test_hparams.py

Borda · 2021-01-07T15:48:33Z

tests/models/conf/config.yaml

@@ -0,0 +1,17 @@
+# Copyright The PyTorch Lightning team.


hall it rather go to tests/data/conf? (not important)

Simpler as hydra need relative path.

There are multiple initialize methods you can use that might make more sense to your use case.
Take a look at the docs [here[(https://hydra.cc/docs/experimental/compose_api#initialization-methods).

Borda · 2021-01-07T15:50:07Z

pytorch_lightning/utilities/parsing.py

    # only collect variables that appear in the signature
    local_args = {k: local_vars[k] for k in init_parameters.keys()}
    local_args.update(local_args.get(kwargs_var, {}))
    local_args = {k: v for k, v in local_args.items() if k not in exclude_argnames}
+    local_args = apply_to_collection(local_args, DictConfig, resolve_dict_config)


does this convert everything to Hydra even you pass pure dict?

It will be triggered only on DictConfig.

so you say whenever you have Hydra installed, you will convert any input to DictConfig, correct?

No, it will convert a DictConfig with interpolation to a resolved one.

…PyTorchLightning/pytorch-lightning into bugfix/5384_hydra_interpolation

codecov · 2021-01-07T18:46:23Z

Codecov Report

Merging #5406 (3427b50) into master (a053d75) will increase coverage by 0%.
The diff coverage is 95%.

@@          Coverage Diff           @@
##           master   #5406   +/-   ##
======================================
  Coverage      93%     93%           
======================================
  Files         134     135    +1     
  Lines        9996   10011   +15     
======================================
+ Hits         9313    9328   +15     
  Misses        683     683

omry · 2021-01-07T19:17:54Z

pytorch_lightning/utilities/parsing.py

+    if OMEGACONF_AVAILABLE:
+        local_args = apply_to_collection(local_args, DictConfig, resolve_dict_config)


if apply_to_collection is recursive, this is exponentially expensive because OmegaConf.to_container is also recursive.

can you explain why you need resolve all interpolations eagerly? there are subtle implications to doing it which can be surprising.

The interpolation is broken, and need to be resolved.

apply_to_collection is recursive. However, it will stop and apply resolve_dict_config as soon as it encounter a DictConfig object and return. In the example, it will apply it directly on cfg as it is the only element in local_args.
If you have a better solution, please share the idea or make a PR 👍

What is subtle implications to doing it which can be surprising. ?

The interpolation is broken, and need to be resolved.

Why is it broken? OmegaConf containers has a reference to their parent. The interpolation should work:

from omegaconf import * cfg = OmegaConf.create({"foo": {"bar": "${zoo}"}, "zoo": 20}) assert cfg.foo.bar == 20 foo = cfg.foo assert foo.bar == 20

Something was probably done along the way to break it.
I think it's better to identify what is the root cause before fixing this.

If you have a better fix, please submit a PR.

You seem a bit defensive, If you don't need my advice you are welcome to ignore it.

Hey @omry

ahaha I apologise, I didn't mean to sound defensive and didn't express myself properly.
I meant The interpolation feature is broken, and we need to resolve it on the Lightning side. We are doing something wrong ! 👍

Your advices are always welcome !!!

And yes, you are entirely right. @SeanNaren spotted the root cause of the problem.
We were off in our loop. Surprising our tests didn't catch this ...

Just pushed @SeanNaren fix and test pass.

Accepted, I think it was just a small misunderstanding.

When I asked "Why is it broken?" I did not think you meant that it's broken in OmegaConf, but rather that it's not working correctly. My question was more along the lines of "Why is it not working correctly? there has to be something going on here".

Anyway, water under the bridge. Great to see you improving Hydra support in PL!

SeanNaren · 2021-01-08T12:51:32Z

As @omry said this flow works:

from omegaconf import *

cfg = OmegaConf.create({"foo": {"bar": "${zoo}"}, "zoo": 20})

assert cfg.foo.bar == 20
foo = cfg.foo
assert foo.bar == 20

OmegaConf.save(cfg, 'output', resolve=True)

So there is no issue with Hydra, I think this is the issue:

https://github.com/PyTorchLightning/pytorch-lightning/blob/master/pytorch_lightning/core/saving.py#L371

for v in hparams.values():
            if OmegaConf.is_config(v):
                with fs.open(config_yaml, "w", encoding="utf-8") as fp:
                    OmegaConf.save(OmegaConf.create(hparams), fp, resolve=True)

This should be:

for v in hparams.values():
            if OmegaConf.is_config(v):
                with fs.open(config_yaml, "w", encoding="utf-8") as fp:
                    OmegaConf.save(OmegaConf.create(v), fp, resolve=True) #hparams should be v

EDIT: let's keep the test though, this is a crucial test to have for saving!

SeanNaren

Nice LGTM

tests/models/test_hparams.py

carmocca

Not an expert on Hydra but LGTM 👍

pytorch_lightning/core/saving.py

omry · 2021-01-08T20:48:45Z

pytorch_lightning/core/saving.py

        for v in hparams.values():
            if OmegaConf.is_config(v):
                with fs.open(config_yaml, "w", encoding="utf-8") as fp:
-                    OmegaConf.save(OmegaConf.create(hparams), fp, resolve=True)
+                    OmegaConf.save(OmegaConf.create(v), fp, resolve=True)
                return


This snippet does not make a whole lot of sense to me.
It seems to be iterating the hparams, and saving the first value that is an OmegConf config, and ignore the rest?

@Borda Any idea ?

omry · 2021-01-08T20:51:00Z

tests/models/conf/config.yaml

@@ -0,0 +1,17 @@
+# Copyright The PyTorch Lightning team.


There are multiple initialize methods you can use that might make more sense to your use case.
Take a look at the docs [here[(https://hydra.cc/docs/experimental/compose_api#initialization-methods).

omry · 2021-01-08T20:52:37Z

tests/models/test_hparams.py

+@pytest.mark.skipif(not HYDRA_AVAILABLE, reason="Hydra is not available")
+def test_model_save_hyper_parameters_interpolation_with_hydra(tmpdir):
+
+    initialize(config_path="conf")


initialize() is changing the global state.

Use this to avoid that.

with initialize(): ...

Please go over the relevant page, there have been many changes in Hydra 1.0 you may not be aware of yet.

Thanks Omry. I will adress those comments tomorrow 😁

Hey Omry. How can I save / load a collection of normal dict, list and DictConfig, ListConfig ?

I have tried to save without interpolation and reload with it, but I didn't manage to make it work ? Would you mind having a look ?

Input:

args_0 = compose(config_name="config") # As expected, it works for this one. args_1 = {"cfg": compose(config_name="config")} args_2 = [compose(config_name="config")] kwarg_1 = {"cfg": [compose(config_name="config")]} model = TestHydraModel(args_0, args_1, args_2, kwarg_1=kwarg_1)

For saving:

if OMEGACONF_AVAILABLE: with fs.open(config_yaml, "w", encoding="utf-8") as fp: OmegaConf.save(hparams, fp)

args_0: log: ${training.log} training: log: Something args_1: cfg: log: ${training.log} training: log: Something args_2: - log: ${training.log} training: log: Something kwarg_1: cfg: - log: ${training.log} training: log: Something

to reload (This won't work as it breaks the original structure of the DictConfigs)

if OMEGACONF_AVAILABLE: with fs.open(config_yaml, "r") as fp: hparams = yaml.full_load(fp) for k, v in hparams.items(): hparams[k] = OmegaConf.create(v) return hparams

How should I save and load to preserve interpolation ? Or should I resolve everything so I don't have pb.

Current solution for saving

if OMEGACONF_AVAILABLE: def resolve_dict_config(data): return OmegaConf.to_container(data, resolve=True) hparams = apply_to_collection(hparams, DictConfig, resolve_dict_config) with fs.open(config_yaml, "w", encoding="utf-8") as fp: OmegaConf.save(hparams, fp) return

args_0: log: Something training: log: Something args_1: cfg: log: Something training: log: Something args_2: - log: Something training: log: Something kwarg_1: cfg: - log: Something training: log: Something

…PyTorchLightning/pytorch-lightning into bugfix/5384_hydra_interpolation

pep8speaks · 2021-01-09T13:11:15Z

Hello @tchaton! Thanks for updating this PR.

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

Comment last updated at 2021-01-09 13:12:48 UTC

SeanNaren · 2021-01-09T13:55:29Z

pytorch_lightning/core/saving.py

-            if OmegaConf.is_config(v):
-                with fs.open(config_yaml, "w", encoding="utf-8") as fp:
-                    OmegaConf.save(OmegaConf.create(hparams), fp, resolve=True)
+        # deepcopy: hparams from user shouldn't be resolved


I'm a bit lost as to what the logic here is. Let's say I have multiple DictConfigs stored in hparams like below

{ 'arg1': DictConfig(...), 'arg2': DictConfig(...) }

With the logic you're proposing, we're going to resolve all DictConfig's eagerly (so that all interpolation's are resolved) and then save the hparams file.

The previous logic would only save the config if it found a DictConfig object, or in my logic the first conf that it finds, which is definitely unsuitable.

So to be clear, the issue is that hparams is a dictionary of DictConfigs and there is no save function that auto resolves them?

Just realised the PR got merged, but if we could come up with a solution that doesn't require a try/catch + recursive search this would be preferred I think (cc @tchaton @omry)

Any reason why the try catch was needed?

@omry It was auto-merge, but let s come with a better solution.

omry · 2021-01-09T17:57:44Z

@tchaton
I am guessing that the reason you have a plain dict containing OmegaConf containers is that at some point someone used **cfg on the OmegaConf container.

Here is a snippet showing the behavior:

from omegaconf import *

cfg = OmegaConf.create(
    {
        "a": {"b": 10},
        "aa": {
            "bb": "${a.b}",
        },
    }
)


def foo(**kwargs):
    print(type(kwargs))
    for v in kwargs.values():
        print(type(v))

    OmegaConf.save(kwargs, "/tmp/foo.yaml")


foo(**cfg)

cfg2 = OmegaConf.load("/tmp/foo.yaml")
print("cfg1", cfg)
print("cfg2", cfg2)
assert cfg == cfg2
assert cfg.aa.bb == cfg2.aa.bb == 10

print(OmegaConf.to_yaml(cfg2, resolve=True))

Output:

<class 'dict'>
<class 'omegaconf.dictconfig.DictConfig'>
<class 'omegaconf.dictconfig.DictConfig'>
cfg1 {'a': {'b': 10}, 'aa': {'bb': '${a.b}'}}
cfg2 {'a': {'b': 10}, 'aa': {'bb': '${a.b}'}}
a:
  b: 10
aa:
  bb: 10

OmegaConf.save can operate on plain dicts. (internally it converts them to a DictConfig prior to converting them to yaml

Unless there is a compelling reason, I wouldn't resolve when saving. You are changing the semantics of the config.
A Hydra user might be changing a,b above , for instance via the command line - and expect both it and aa.bb to reflect the change.
This would work before you resolve, but not if you load the resolved config.

I think it's reasonable to expect users that have interpolations in the configs to also use OmegaConf/Hydra to load the config later.

If you still want to resolve as you save, just add resolve=True to the save call as before.

omry · 2021-01-09T18:18:06Z

tests/models/test_hparams.py

+        args_0 = compose(config_name="config")
+        args_1 = {"cfg": compose(config_name="config")}
+        args_2 = [compose(config_name="config")]
+        kwarg_1 = {"cfg": [compose(config_name="config")]}
+        model = TestHydraModel(args_0, args_1, args_2, kwarg_1=kwarg_1)


Is this trying to emulate the following?

cfg = compose(config_name="config") model = TestHydraModel(**cfg)

The function call itself does this:

class MyModel(pl.LightningModule): def __init__(self, cfg_1: DictConfig, cfg_2: Dictconfig): self.save_hyperparameters() # Autosaves your parameters for loading your model in the future # after training your model MyModel.load_from_checkpoint('model.ckpt') # load's from checkpoint, uses the saved input args

In the test he's emulating the above I think!

I think the logic flow that is currently implemented is:

Loop through the hparams dictionary

If DictConfig, resolve interpolation and create new dict container

After the loop, save all the resolved DictConfigs

There is some weirdness however. I'm hoping we can do something a bit cleaner:

If OmegaConf is available, we rely on OmegaConf entirely to save the hparams dictionary without resolving anything. @omry we should expect OmegaConf.save to work with primitive types/anything that can be pickle-able right? How about having two different DictConfigs within the dictionary structure?

If not, default save functionality

If OmegaConf is available, we rely on OmegaConf entirely to save the hparams dictionary without resolving anything. @omry we should expect OmegaConf.save to work with primitive types/anything that can be pickle-able right?

No, OmegaConf does not support arbitrary objects in the config.

How about having two different DictConfigs within the dictionary structure?

That should be fine.

Borda · 2021-01-11T21:27:31Z

pytorch_lightning/utilities/__init__.py

@@ -23,36 +23,16 @@

 from pytorch_lightning.utilities.apply_func import move_data_to_device
 from pytorch_lightning.utilities.distributed import AllGatherGrad, rank_zero_info, rank_zero_only, rank_zero_warn
+from pytorch_lightning.utilities.package_utils import _module_available


well, this was moved already to #5256 along with all COnstants so we can import them anywhere without cyclic imports...

* resolve bug * Apply suggestions from code review * resolve package import * resolve import * update on comments * update on comments * hacky fix * update * exit * update * to_container * typo * resolve import * update * resolve pep8 Co-authored-by: Jirka Borovec <[email protected]> Co-authored-by: Sean Naren <[email protected]> (cherry picked from commit bb5031b)

resolve bug

e57a876

tchaton requested review from awaelchli, Borda, justusschock, SeanNaren and williamFalcon as code owners January 7, 2021 15:43

tchaton self-assigned this Jan 7, 2021

tchaton added this to the 1.1.x milestone Jan 7, 2021

tchaton added the feature Is an improvement or enhancement label Jan 7, 2021

tchaton mentioned this pull request Jan 7, 2021

Value interpolation with hydra composition #5384

Closed

Borda reviewed Jan 7, 2021

View reviewed changes

Borda and others added 4 commits January 7, 2021 16:50

Apply suggestions from code review

3f2e9d7

resolve package import

a861180

Merge branch 'bugfix/5384_hydra_interpolation' of https://github.com/…

551ea0f

…PyTorchLightning/pytorch-lightning into bugfix/5384_hydra_interpolation

resolve import

3891278

omry reviewed Jan 7, 2021

View reviewed changes

update on comments

1c3e077

SeanNaren approved these changes Jan 8, 2021

View reviewed changes

Merge branch 'master' into bugfix/5384_hydra_interpolation

4a92ddf

carmocca reviewed Jan 8, 2021

View reviewed changes

tests/models/test_hparams.py Outdated Show resolved Hide resolved

carmocca reviewed Jan 8, 2021

View reviewed changes

tests/models/test_hparams.py Outdated Show resolved Hide resolved

carmocca reviewed Jan 8, 2021

View reviewed changes

tests/models/test_hparams.py Show resolved Hide resolved

carmocca approved these changes Jan 8, 2021

View reviewed changes

update on comments

ce2b131

teddykoker approved these changes Jan 8, 2021

View reviewed changes

pytorch_lightning/core/saving.py Outdated Show resolved Hide resolved

omry reviewed Jan 8, 2021

View reviewed changes

tchaton added 2 commits January 9, 2021 12:13

hacky fix

03efd24

update

4aedc6d

tchaton added 8 commits January 9, 2021 12:30

exit

5a33c3f

update

e942045

Merge branch 'master' into bugfix/5384_hydra_interpolation

5f13c09

to_container

1b1d694

Merge branch 'bugfix/5384_hydra_interpolation' of https://github.com/…

7178564

…PyTorchLightning/pytorch-lightning into bugfix/5384_hydra_interpolation

typo

2045be6

resolve import

cf297f1

update

2aced4d

tchaton added the bug Something isn't working label Jan 9, 2021

resolve pep8

3427b50

tchaton enabled auto-merge (squash) January 9, 2021 13:14

SeanNaren reviewed Jan 9, 2021

View reviewed changes

tchaton merged commit bb5031b into master Jan 9, 2021

tchaton deleted the bugfix/5384_hydra_interpolation branch January 9, 2021 13:55

omry reviewed Jan 9, 2021

View reviewed changes

Borda reviewed Jan 11, 2021

View reviewed changes

		if OMEGACONF_AVAILABLE:
		local_args = apply_to_collection(local_args, DictConfig, resolve_dict_config)

bugfix: Resolve interpolation bug with Hydra #5406

bugfix: Resolve interpolation bug with Hydra #5406

Conversation

tchaton commented Jan 7, 2021 • edited Loading

What does this PR do?

Before submitting

PR review

Did you have fun?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 7, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

tchaton Jan 8, 2021 • edited Loading

Choose a reason for hiding this comment

tchaton Jan 8, 2021 • edited Loading

Choose a reason for hiding this comment

tchaton Jan 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaton Jan 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeanNaren commented Jan 8, 2021 • edited Loading

SeanNaren left a comment

Choose a reason for hiding this comment

carmocca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaton Jan 9, 2021 • edited Loading

Choose a reason for hiding this comment

tchaton Jan 9, 2021 • edited Loading

Choose a reason for hiding this comment

pep8speaks commented Jan 9, 2021 • edited Loading

Comment last updated at 2021-01-09 13:12:48 UTC

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omry commented Jan 9, 2021 • edited Loading

Choose a reason for hiding this comment

SeanNaren Jan 11, 2021 • edited Loading

Choose a reason for hiding this comment

SeanNaren Jan 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaton commented Jan 7, 2021 •

edited

Loading

codecov bot commented Jan 7, 2021 •

edited

Loading

tchaton Jan 8, 2021 •

edited

Loading

tchaton Jan 8, 2021 •

edited

Loading

tchaton Jan 8, 2021 •

edited

Loading

tchaton Jan 8, 2021 •

edited

Loading

SeanNaren commented Jan 8, 2021 •

edited

Loading

tchaton Jan 9, 2021 •

edited

Loading

tchaton Jan 9, 2021 •

edited

Loading

pep8speaks commented Jan 9, 2021 •

edited

Loading

omry commented Jan 9, 2021 •

edited

Loading

SeanNaren Jan 11, 2021 •

edited

Loading

SeanNaren Jan 11, 2021 •

edited

Loading