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

Enable --cfg=job --resolve with ${hydra:...} resolver #1696

Merged
merged 12 commits into from
Jul 17, 2021
40 changes: 18 additions & 22 deletions hydra/_internal/hydra.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,12 @@
from collections import defaultdict
from typing import Any, Callable, DefaultDict, List, Optional, Sequence, Type, Union

from omegaconf import Container, DictConfig, OmegaConf, flag_override, open_dict
from omegaconf import Container, DictConfig, OmegaConf, flag_override, read_write

from hydra._internal.utils import get_column_widths, run_and_report
from hydra.core.config_loader import ConfigLoader
from hydra.core.config_search_path import ConfigSearchPath
from hydra.core.hydra_config import HydraConfig
from hydra.core.plugins import Plugins
from hydra.core.utils import (
JobReturn,
Expand Down Expand Up @@ -147,25 +148,13 @@ def get_sanitized_hydra_cfg(src_cfg: DictConfig) -> DictConfig:
for key in list(cfg.keys()):
if key != "hydra":
del cfg[key]
with open_dict(cfg.hydra):
with flag_override(cfg.hydra, ["struct", "readonly"], [False, False]):
Jasha10 marked this conversation as resolved.
Show resolved Hide resolved
del cfg.hydra["hydra_help"]
del cfg.hydra["help"]
return cfg

def _get_cfg(
self,
config_name: Optional[str],
overrides: List[str],
cfg_type: str,
with_log_configuration: bool,
) -> DictConfig:
def get_sanitized_cfg(self, cfg: DictConfig, cfg_type: str) -> DictConfig:
assert cfg_type in ["job", "hydra", "all"]
cfg = self.compose_config(
config_name=config_name,
overrides=overrides,
run_mode=RunMode.RUN,
with_log_configuration=with_log_configuration,
)
if cfg_type == "job":
with flag_override(cfg, ["struct", "readonly"], [False, False]):
del cfg["hydra"]
Expand All @@ -181,12 +170,13 @@ def show_cfg(
package: Optional[str],
resolve: bool = False,
) -> None:
cfg = self._get_cfg(
cfg = self.compose_config(
config_name=config_name,
overrides=overrides,
cfg_type=cfg_type,
run_mode=RunMode.RUN,
with_log_configuration=False,
)
HydraConfig.instance().set_config(cfg)
if package == "_global_":
package = None

Expand All @@ -204,7 +194,9 @@ def show_cfg(
if package is not None:
print(f"# @package {package}")
if resolve:
OmegaConf.resolve(ret)
with read_write(cfg.hydra):
OmegaConf.resolve(ret)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems unrelated. What is the motivation for this? (Is there a missing test?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The HydraConfig.set_config method sets a readonly flag on cfg.hydra, so OmegaConf.resolve fails unless the flag is turned off.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could unset the readonly flag in show_cfg and show_help.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 57452d5. I only needed to unset the flag in show_cfg. In show_help, a readonly flag is no problem because we are using OmegaConf.to_yaml(..., resolve=...) instead of OmegaConf.resolve(...).

cfg = self.get_sanitized_cfg(cfg, cfg_type)
sys.stdout.write(OmegaConf.to_yaml(ret))

@staticmethod
Expand Down Expand Up @@ -345,6 +337,7 @@ def app_help(
run_mode=RunMode.RUN,
with_log_configuration=True,
)
HydraConfig.instance().set_config(cfg)
help_cfg = cfg.hydra.help
clean_cfg = copy.deepcopy(cfg)

Expand Down Expand Up @@ -400,12 +393,14 @@ def _print_search_path(

box: List[List[str]] = [["Provider", "Search path"]]

cfg = self._get_cfg(
cfg = self.compose_config(
config_name=config_name,
overrides=overrides,
cfg_type="hydra",
run_mode=RunMode.RUN,
with_log_configuration=False,
)
HydraConfig.instance().set_config(cfg)
cfg = self.get_sanitized_cfg(cfg, cfg_type="hydra")

sources = cfg.hydra.runtime.config_sources

Expand Down Expand Up @@ -472,13 +467,14 @@ def _print_config_info(
self._print_defaults_list(config_name=config_name, overrides=overrides)

cfg = run_and_report(
lambda: self._get_cfg(
lambda: self.compose_config(
config_name=config_name,
overrides=overrides,
cfg_type="all",
run_mode=RunMode.RUN,
with_log_configuration=False,
)
)
HydraConfig.instance().set_config(cfg)
self._log_header(header="Config", filler="*")
with flag_override(cfg, ["struct", "readonly"], [False, False]):
del cfg["hydra"]
Expand Down
1 change: 1 addition & 0 deletions news/1681.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix the --cfg=job --resolve flags so that the ${hydra:...} resolver now prints properly
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes --help as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I updated the news fragment in 751653f.

1 change: 1 addition & 0 deletions tests/test_apps/interpolation_to_hydra_config/config.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
a: ${hydra:job.name}
13 changes: 13 additions & 0 deletions tests/test_apps/interpolation_to_hydra_config/my_app.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved
from omegaconf import DictConfig

import hydra


@hydra.main(config_path=".", config_name="config")
def my_app(cfg: DictConfig) -> None:
pass
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you reuse simple_app here?
You can place the config next to it or somewhere else and override the used config like:

python tests/test_apps/simple_app/my_app.py  --config-name=config --config-dir=tests/test_apps/app_with_cfg

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 1d7bdd2.



if __name__ == "__main__":
my_app()
34 changes: 31 additions & 3 deletions tests/test_hydra.py
Original file line number Diff line number Diff line change
Expand Up @@ -459,9 +459,10 @@ def test_cfg_with_package(


@mark.parametrize(
"resolve,expected",
"script,resolve,expected",
[
param(
"tests/test_apps/simple_interpolation/my_app.py",
False,
dedent(
"""\
Expand All @@ -472,6 +473,7 @@ def test_cfg_with_package(
id="cfg",
),
param(
"tests/test_apps/simple_interpolation/my_app.py",
True,
dedent(
"""\
Expand All @@ -481,11 +483,23 @@ def test_cfg_with_package(
),
id="resolve",
),
param(
"tests/test_apps/interpolation_to_hydra_config/my_app.py",
True,
dedent(
"""\
a: my_app
"""
),
id="resolve_hydra_config",
),
],
)
def test_cfg_resolve_interpolation(tmpdir: Path, resolve: bool, expected: str) -> None:
def test_cfg_resolve_interpolation(
tmpdir: Path, script: str, resolve: bool, expected: str
) -> None:
cmd = [
"tests/test_apps/simple_interpolation/my_app.py",
script,
"hydra.run.dir=" + str(tmpdir),
"--cfg=job",
Jasha10 marked this conversation as resolved.
Show resolved Hide resolved
]
Expand Down Expand Up @@ -617,6 +631,20 @@ def test_sweep_complex_defaults(
),
id="overriding_help_template:$CONFIG,interp,yes_resolve",
),
param(
"examples/tutorials/basic/your_first_hydra_app/2_config_file/my_app.py",
["--help", "--resolve"],
["hydra.help.template=$CONFIG", "db.user=${hydra:job.name}"],
dedent(
"""\
db:
driver: mysql
user: my_app
password: secret
"""
),
id="overriding_help_template:$CONFIG,resolve_interp_to_hydra_config",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generally speaking, avoid testing with the tutorial apps except in tests/test_examples. (I know the code is not being consistent about it).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made the change in 27052cf.

),
param(
"examples/tutorials/basic/your_first_hydra_app/2_config_file/my_app.py",
["--help"],
Expand Down