From 4664dfb2261bef60a0bf7ea8c43a435b3f8d83de Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Fri, 30 Oct 2020 17:32:37 -0700 Subject: [PATCH 01/91] --info defaults to show defaults list information --- hydra/_internal/hydra.py | 105 ++++++++++++++++++++++++++++++++------- hydra/_internal/utils.py | 17 +++++-- tests/test_hydra.py | 4 +- 3 files changed, 103 insertions(+), 23 deletions(-) diff --git a/hydra/_internal/hydra.py b/hydra/_internal/hydra.py index 8c8211ef3a9..0df57c32fb5 100644 --- a/hydra/_internal/hydra.py +++ b/hydra/_internal/hydra.py @@ -31,6 +31,7 @@ from hydra.types import RunMode, TaskFunction from .config_loader_impl import ConfigLoaderImpl +from .defaults_list import expand_defaults_list from .utils import create_automatic_config_search_path log: Optional[logging.Logger] = None @@ -486,6 +487,67 @@ def _print_plugins_profiling_info(self, top_n: int) -> None: self._log_footer(header=header, filler="-") + def _print_defaults_list(self, config_name: Optional[str], overrides: List[str]): + run_mode = RunMode.RUN + ret = self.config_loader.compute_input_defaults_list( + config_name=config_name, + overrides=overrides, + run_mode=RunMode.RUN, + from_shell=True, + ) + + defaults = expand_defaults_list( + defaults=ret.input_defaults, + skip_missing=run_mode == RunMode.RUN, + ignore_config_load_failures=True, + repo=self.config_loader.repository, # type: ignore + ) + + box: List[List[str]] = [ + [ + "Config group", + "Config name", + "Package", + "Parent", + "Skip reason", + ] + ] + for d in defaults: + row = [ + d.config_group, + d.config_name, + d.package, + d.parent, + d.skip_load_reason if d.skip_load else "", + ] + row = [x if x is not None else "" for x in row] + box.append(row) + padding = get_column_widths(box) + del box[0] + log.debug("") + self._log_header("Defaults List", filler="*") + header = "| {} | {} | {} | {} | {} | ".format( + "Config group".ljust(padding[0]), + "Config name".ljust(padding[1]), + "Package".ljust(padding[2]), + "Parent".ljust(padding[3]), + "Skip reason".ljust(padding[4]), + ) + self._log_header(header=header, filler="-") + + for row in box: + log.debug( + "| {} | {} | {} | {} | {} |".format( + row[0].ljust(padding[0]), + row[1].ljust(padding[1]), + row[2].ljust(padding[2]), + row[3].ljust(padding[3]), + row[4].ljust(padding[4]), + ) + ) + + self._log_footer(header=header, filler="-") + def _print_debug_info(self, cfg: DictConfig) -> None: assert log is not None if log.isEnabledFor(logging.DEBUG): @@ -528,29 +590,36 @@ def compose_config( self._print_debug_info(cfg) return cfg - def show_info(self, config_name: Optional[str], overrides: List[str]) -> None: + def show_info( + self, info: str, config_name: Optional[str], overrides: List[str] + ) -> None: from .. import __version__ simple_stdout_log_config(level=logging.DEBUG) global log log = logging.getLogger(__name__) self._log_header(f"Hydra {__version__}", filler="=") - self._print_plugins() - self._print_search_path() - self._print_plugins_profiling_info(top_n=10) - - cfg = run_and_report( - lambda: self._get_cfg( - config_name=config_name, - overrides=overrides, - cfg_type="all", - with_log_configuration=False, + + if info == "all": + self._print_plugins() + self._print_search_path() + self._print_plugins_profiling_info(top_n=10) + self._print_defaults_list(config_name, overrides) + + cfg = run_and_report( + lambda: self._get_cfg( + config_name=config_name, + overrides=overrides, + cfg_type="all", + with_log_configuration=False, + ) ) - ) - self._print_composition_trace(cfg) + self._print_composition_trace(cfg) + log.debug("\n") + self._log_header(header="Config", filler="*") + with open_dict(cfg): + del cfg["hydra"] + print(OmegaConf.to_yaml(cfg)) - log.debug("\n") - self._log_header(header="Config", filler="*") - with open_dict(cfg): - del cfg["hydra"] - print(OmegaConf.to_yaml(cfg)) + elif info == "defaults": + self._print_defaults_list(config_name, overrides) diff --git a/hydra/_internal/utils.py b/hydra/_internal/utils.py index aa50aee6d3d..7cd98f19b5d 100644 --- a/hydra/_internal/utils.py +++ b/hydra/_internal/utils.py @@ -348,7 +348,11 @@ def add_conf_dir() -> None: has_show_cfg = args.cfg is not None num_commands = ( - args.run + has_show_cfg + args.multirun + args.shell_completion + args.info + args.run + + has_show_cfg + + args.multirun + + args.shell_completion + + (args.info is not None) ) if num_commands > 1: raise ValueError( @@ -388,7 +392,9 @@ def add_conf_dir() -> None: ) ) elif args.info: - hydra.show_info(config_name=config_name, overrides=args.overrides) + hydra.show_info( + args.info, config_name=config_name, overrides=args.overrides + ) else: sys.stderr.write("Command not specified\n") sys.exit(1) @@ -491,7 +497,12 @@ def __repr__(self) -> str: ) parser.add_argument( - "--info", "-i", action="store_true", help="Print Hydra information" + "--info", + "-i", + const="all", + nargs="?", + action="store", + help="Print Hydra information [all|defaults]", ) return parser diff --git a/tests/test_hydra.py b/tests/test_hydra.py index 2dcd9c4736f..b526f35d0db 100644 --- a/tests/test_hydra.py +++ b/tests/test_hydra.py @@ -605,7 +605,7 @@ def test_sweep_complex_defaults( The config_path is relative to the Python file declaring @hydra.main() --config-name,-cn : Overrides the config_name specified in hydra.main() --config-dir,-cd : Adds an additional config dir to the config search path ---info,-i : Print Hydra information +--info,-i : Print Hydra information [all|defaults] Overrides : Any key=value arguments to override config values (use dots for.nested=overrides) """, id="overriding_help_template:$FLAGS_HELP", @@ -660,7 +660,7 @@ def test_sweep_complex_defaults( The config_path is relative to the Python file declaring @hydra.main() --config-name,-cn : Overrides the config_name specified in hydra.main() --config-dir,-cd : Adds an additional config dir to the config search path ---info,-i : Print Hydra information +--info,-i : Print Hydra information [all|defaults] Overrides : Any key=value arguments to override config values (use dots for.nested=overrides) """, id="overriding_hydra_help_template:$FLAGS_HELP", From 36a53ba1a64fee7fbc271d5d24adf8449292fb97 Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Mon, 23 Nov 2020 10:46:48 -0800 Subject: [PATCH 02/91] testing loading of raw defaults list from configs --- .../core_plugins/file_config_source.py | 10 +- .../importlib_resources_config_source.py | 10 +- .../core_plugins/structured_config_source.py | 12 +- hydra/core/NewDefaultElement.py | 20 +++ hydra/plugins/config_source.py | 115 +++++++++++++----- tests/test_data/default_lists/empty.yaml | 0 .../test_data/default_lists/group1/file1.yaml | 0 .../default_lists/group1/group2/file1.yaml | 0 .../non_config_group_default.yaml | 2 + tests/test_data/default_lists/one_item.yaml | 2 + tests/test_data/default_lists/optional.yaml | 3 + .../test_data/default_lists/self_leading.yaml | 3 + .../default_lists/self_trailing.yaml | 3 + tests/test_defaults_list2.py | 74 +++++++++++ 14 files changed, 209 insertions(+), 45 deletions(-) create mode 100644 hydra/core/NewDefaultElement.py create mode 100644 tests/test_data/default_lists/empty.yaml create mode 100644 tests/test_data/default_lists/group1/file1.yaml create mode 100644 tests/test_data/default_lists/group1/group2/file1.yaml create mode 100644 tests/test_data/default_lists/non_config_group_default.yaml create mode 100644 tests/test_data/default_lists/one_item.yaml create mode 100644 tests/test_data/default_lists/optional.yaml create mode 100644 tests/test_data/default_lists/self_leading.yaml create mode 100644 tests/test_data/default_lists/self_trailing.yaml create mode 100644 tests/test_defaults_list2.py diff --git a/hydra/_internal/core_plugins/file_config_source.py b/hydra/_internal/core_plugins/file_config_source.py index a74f9f843dd..f91fa218ebd 100644 --- a/hydra/_internal/core_plugins/file_config_source.py +++ b/hydra/_internal/core_plugins/file_config_source.py @@ -40,15 +40,17 @@ def load_config( ) f.seek(0) cfg = OmegaConf.load(f) - defaults_list = self._extract_defaults_list( - config_path=config_path, cfg=cfg - ) + + raw_defaults_list = self._extract_raw_defaults_list(cfg=cfg) return ConfigResult( config=self._embed_config(cfg, header["package"]), path=f"{self.scheme()}://{self.path}", provider=self.provider, header=header, - defaults_list=defaults_list, + defaults_list=self._create_defaults_list( + config_path=config_path, defaults=raw_defaults_list + ), + new_defaults_list=self._create_new_defaults_list(raw_defaults_list), ) def available(self) -> bool: diff --git a/hydra/_internal/core_plugins/importlib_resources_config_source.py b/hydra/_internal/core_plugins/importlib_resources_config_source.py index 95e07d8506e..1af2bbd69c6 100644 --- a/hydra/_internal/core_plugins/importlib_resources_config_source.py +++ b/hydra/_internal/core_plugins/importlib_resources_config_source.py @@ -51,15 +51,17 @@ def load_config( ) f.seek(0) cfg = OmegaConf.load(f) - defaults_list = self._extract_defaults_list( - config_path=config_path, cfg=cfg - ) + + raw_defaults_list = self._extract_raw_defaults_list(cfg=cfg) return ConfigResult( config=self._embed_config(cfg, header["package"]), path=f"{self.scheme()}://{self.path}", provider=self.provider, header=header, - defaults_list=defaults_list, + defaults_list=self._create_defaults_list( + config_path=config_path, defaults=raw_defaults_list + ), + new_defaults_list=self._create_new_defaults_list(raw_defaults_list), ) def available(self) -> bool: diff --git a/hydra/_internal/core_plugins/structured_config_source.py b/hydra/_internal/core_plugins/structured_config_source.py index a75813f374b..0c019a48613 100644 --- a/hydra/_internal/core_plugins/structured_config_source.py +++ b/hydra/_internal/core_plugins/structured_config_source.py @@ -50,17 +50,17 @@ def load_config( is_primary_config=is_primary_config, package_override=package_override, ) - defaults_list = self._extract_defaults_list( - config_path=config_path, cfg=ret.node - ) - cfg = self._embed_config(ret.node, header["package"]) + raw_defaults_list = self._extract_raw_defaults_list(cfg=ret.node) return ConfigResult( - config=cfg, + config=self._embed_config(ret.node, header["package"]), path=f"{self.scheme()}://{self.path}", provider=provider, header=header, - defaults_list=defaults_list, + defaults_list=self._create_defaults_list( + config_path=config_path, defaults=raw_defaults_list + ), + new_defaults_list=self._create_new_defaults_list(raw_defaults_list), ) def available(self) -> bool: diff --git a/hydra/core/NewDefaultElement.py b/hydra/core/NewDefaultElement.py new file mode 100644 index 00000000000..806ce599235 --- /dev/null +++ b/hydra/core/NewDefaultElement.py @@ -0,0 +1,20 @@ +from dataclasses import dataclass +from typing import Optional + + +@dataclass +class InputDefaultElement: + # config group name if present + group: Optional[str] = None + # config file name + name: Optional[str] = None + optional: bool = False + package: Optional[str] = None + + +@dataclass +class ProcessedDefaultElement(InputDefaultElement): + parent: Optional[str] = None + + addressing_key: Optional[str] = None + result_package: Optional[str] = None diff --git a/hydra/plugins/config_source.py b/hydra/plugins/config_source.py index 944b5e22f9c..44f29cf4cc2 100644 --- a/hydra/plugins/config_source.py +++ b/hydra/plugins/config_source.py @@ -1,4 +1,6 @@ # Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved +import copy + from textwrap import dedent import warnings @@ -10,6 +12,7 @@ from typing import List, Optional, Dict, Tuple, MutableSequence from hydra.core import DefaultElement +from hydra.core.NewDefaultElement import InputDefaultElement from hydra.errors import HydraException from omegaconf import ( Container, @@ -31,6 +34,7 @@ class ConfigResult: config: Container header: Dict[str, str] defaults_list: List[DefaultElement] + new_defaults_list: List[InputDefaultElement] is_schema_source: bool = False @@ -241,40 +245,41 @@ def _get_header_dict(config_text: str) -> Dict[str, str]: return res + @staticmethod + def _split_group( + group_with_package: str, + ) -> Tuple[str, Optional[str], Optional[str]]: + idx = group_with_package.find("@") + if idx == -1: + # group + group = group_with_package + package = None + else: + # group@package + group = group_with_package[0:idx] + package = group_with_package[idx + 1 :] + + package2 = None + if package is not None: + # if we have a package, break it down if it's a rename + idx = package.find(":") + if idx != -1: + package2 = package[idx + 1 :] + package = package[0:idx] + + if package == "": + package = None + + if package2 == "": + package2 = None + + return group, package, package2 + @staticmethod def _create_defaults_list( config_path: Optional[str], defaults: ListConfig, ) -> List[DefaultElement]: - def _split_group( - group_with_package: str, - ) -> Tuple[str, Optional[str], Optional[str]]: - idx = group_with_package.find("@") - if idx == -1: - # group - group = group_with_package - package = None - else: - # group@package - group = group_with_package[0:idx] - package = group_with_package[idx + 1 :] - - package2 = None - if package is not None: - # if we have a package, break it down if it's a rename - idx = package.find(":") - if idx != -1: - package2 = package[idx + 1 :] - package = package[0:idx] - - if package == "": - package = None - - if package2 == "": - package2 = None - - return group, package, package2 - if not isinstance(defaults, MutableSequence): raise ValueError( dedent( @@ -289,7 +294,7 @@ def _split_group( """ ) ) - + defaults = copy.deepcopy(defaults) res: List[DefaultElement] = [] for item in defaults: if isinstance(item, DictConfig): @@ -302,7 +307,7 @@ def _split_group( if len(keys) == 0: raise ValueError(f"Missing group name in {item}") key = keys[0] - config_group, package, package2 = _split_group(key) + config_group, package, package2 = ConfigSource._split_group(key) node = item._get_node(key) assert node is not None config_name = node._value() @@ -374,3 +379,51 @@ def _extract_defaults_list( ) else: return [] + + @staticmethod + def _create_new_defaults_list( + defaults: ListConfig, + ) -> List[InputDefaultElement]: + res: List[DefaultElement] = [] + for item in defaults: + if isinstance(item, DictConfig): + optional = False + if "optional" in item: + optional = item.pop("optional") + keys = list(item.keys()) + if len(keys) > 1: + raise ValueError(f"Too many keys in default item {item}") + if len(keys) == 0: + raise ValueError(f"Missing group name in {item}") + key = keys[0] + config_group, package, _package2 = ConfigSource._split_group(key) + node = item._get_node(key) + assert node is not None + config_name = node._value() + + default = InputDefaultElement( + group=config_group, + name=config_name, + package=package, + optional=optional, + ) + elif isinstance(item, str): + path, package, _package2 = ConfigSource._split_group(item) + default = InputDefaultElement(name=path, package=package) + else: + raise ValueError( + f"Unsupported type in defaults : {type(item).__name__}" + ) + res.append(default) + return res + + @staticmethod + def _extract_raw_defaults_list(cfg: Container) -> List[InputDefaultElement]: + if not OmegaConf.is_dict(cfg): + return [] + + with read_write(cfg): + with open_dict(cfg): + defaults = cfg.pop("defaults", []) + + return defaults diff --git a/tests/test_data/default_lists/empty.yaml b/tests/test_data/default_lists/empty.yaml new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/test_data/default_lists/group1/file1.yaml b/tests/test_data/default_lists/group1/file1.yaml new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/test_data/default_lists/group1/group2/file1.yaml b/tests/test_data/default_lists/group1/group2/file1.yaml new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/test_data/default_lists/non_config_group_default.yaml b/tests/test_data/default_lists/non_config_group_default.yaml new file mode 100644 index 00000000000..004af35e1ed --- /dev/null +++ b/tests/test_data/default_lists/non_config_group_default.yaml @@ -0,0 +1,2 @@ +defaults: + - some_config diff --git a/tests/test_data/default_lists/one_item.yaml b/tests/test_data/default_lists/one_item.yaml new file mode 100644 index 00000000000..7264a581cdb --- /dev/null +++ b/tests/test_data/default_lists/one_item.yaml @@ -0,0 +1,2 @@ +defaults: + - group1: file1 \ No newline at end of file diff --git a/tests/test_data/default_lists/optional.yaml b/tests/test_data/default_lists/optional.yaml new file mode 100644 index 00000000000..1d4d7814837 --- /dev/null +++ b/tests/test_data/default_lists/optional.yaml @@ -0,0 +1,3 @@ +defaults: + - group1: file1 + optional: true diff --git a/tests/test_data/default_lists/self_leading.yaml b/tests/test_data/default_lists/self_leading.yaml new file mode 100644 index 00000000000..5bcdb8b06b3 --- /dev/null +++ b/tests/test_data/default_lists/self_leading.yaml @@ -0,0 +1,3 @@ +defaults: + - _self_ + - group1: file1 \ No newline at end of file diff --git a/tests/test_data/default_lists/self_trailing.yaml b/tests/test_data/default_lists/self_trailing.yaml new file mode 100644 index 00000000000..af036bd724f --- /dev/null +++ b/tests/test_data/default_lists/self_trailing.yaml @@ -0,0 +1,3 @@ +defaults: + - group1: file1 + - _self_ diff --git a/tests/test_defaults_list2.py b/tests/test_defaults_list2.py new file mode 100644 index 00000000000..dcffabec7fa --- /dev/null +++ b/tests/test_defaults_list2.py @@ -0,0 +1,74 @@ +# Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved +from pytest import mark, param +from typing import List + +from hydra._internal.config_repository import ConfigRepository +from hydra._internal.config_search_path_impl import ConfigSearchPathImpl +from hydra.core.NewDefaultElement import InputDefaultElement +from hydra.core.plugins import Plugins +from hydra.test_utils.test_utils import chdir_hydra_root + +chdir_hydra_root() + +# registers config source plugins +Plugins.instance() + + +def create_repo(): + csp = ConfigSearchPathImpl() + csp.append(provider="test", path="file://tests/test_data/default_lists") + return ConfigRepository(config_search_path=csp) + + +@mark.parametrize( + "config_path,expected_list", + [ + param("empty", [], id="empty"), + param( + "one_item", + [InputDefaultElement(group="group1", name="file1")], + id="one_item", + ), + param( + "one_item", + [InputDefaultElement(group="group1", name="file1")], + id="one_item", + ), + param( + "self_leading", + [ + InputDefaultElement(name="_self_"), + InputDefaultElement(group="group1", name="file1"), + ], + id="self_leading", + ), + param( + "self_trailing", + [ + InputDefaultElement(group="group1", name="file1"), + InputDefaultElement(name="_self_"), + ], + id="self_trailing", + ), + param( + "optional", + [ + InputDefaultElement(group="group1", name="file1", optional=True), + ], + id="optional", + ), + param( + "non_config_group_default", + [ + InputDefaultElement(name="some_config"), + ], + id="non_config_group_default", + ), + ], +) +def test_loaded_defaults_list( + config_path: str, expected_list: List[InputDefaultElement] +): + repo = create_repo() + result = repo.load_config(config_path=config_path, is_primary_config=True) + assert result.new_defaults_list == expected_list From 82a3ee75c8edec148d9951f61b0add347dab571e Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Mon, 23 Nov 2020 13:30:50 -0800 Subject: [PATCH 03/91] remove package header manipulation in example config source --- .../example_configsource_plugin/example_configsource_plugin.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/examples/plugins/example_configsource_plugin/hydra_plugins/example_configsource_plugin/example_configsource_plugin.py b/examples/plugins/example_configsource_plugin/hydra_plugins/example_configsource_plugin/example_configsource_plugin.py index ac21e612d58..442019d8ce9 100644 --- a/examples/plugins/example_configsource_plugin/hydra_plugins/example_configsource_plugin/example_configsource_plugin.py +++ b/examples/plugins/example_configsource_plugin/hydra_plugins/example_configsource_plugin/example_configsource_plugin.py @@ -74,9 +74,6 @@ def load_config( if normalized_config_path in self.headers else {} ) - if "package" not in header: - header["package"] = "" - self._update_package_in_header( header=header, normalized_config_path=normalized_config_path, From a198217ff0b36a2f98e2ee4eaf82f8031aa61f0f Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Mon, 23 Nov 2020 13:32:36 -0800 Subject: [PATCH 04/91] some basic basic defaults tree are passing --- hydra/_internal/new_defaults_list.py | 169 ++++++++++++++++++ hydra/core/NewDefaultElement.py | 11 +- hydra/plugins/config_source.py | 58 +++--- .../non_config_group_default.yaml | 2 +- tests/test_defaults_list2.py | 169 ++++++++++++++++-- 5 files changed, 360 insertions(+), 49 deletions(-) create mode 100644 hydra/_internal/new_defaults_list.py diff --git a/hydra/_internal/new_defaults_list.py b/hydra/_internal/new_defaults_list.py new file mode 100644 index 00000000000..765c7747de0 --- /dev/null +++ b/hydra/_internal/new_defaults_list.py @@ -0,0 +1,169 @@ +from dataclasses import dataclass +from textwrap import dedent +from typing import Dict, List, Optional, Tuple, Union + +from hydra import MissingConfigException +from hydra._internal.config_repository import IConfigRepository +from hydra.core.NewDefaultElement import InputDefault +from hydra.core.object_type import ObjectType +from hydra.core.override_parser.types import Override + + +@dataclass +class DefaultsTreeNode: + parent: InputDefault + children: Optional[List[Union["DefaultsTreeNode", InputDefault]]] = None + + +@dataclass +class ResultDefault: + config_path: Optional[str] = None + parent: Optional[str] = None + addressing_key: Optional[str] = None + result_package: Optional[str] = None + is_self: bool = False + + +@dataclass +class GroupOverrides: + map: Dict[str, str] + + def get_choice_for(self, default: InputDefault) -> str: + if default.group is not None: + key = default.group # TODO: use package if present + if key in self.map: + return self.map[key] + else: + return default.name + else: + return default.name + + +@dataclass +class DefaultsList: + defaults: List[ResultDefault] + config_overrides: List[Override] + + +def _split_overrides( + repo: IConfigRepository, + overrides: List[Override], +) -> Tuple[List[Override], List[Override]]: + # TODO + return overrides, [] + + +def _create_group_overrides(default_overrides: List[Override]) -> GroupOverrides: + group_overrides = GroupOverrides(map={}) + for override in default_overrides: + value = override.value() + assert isinstance(value, str) + group_overrides.map[override.key_or_group] = value + + return group_overrides + + +def load_config_defaults_list( + default: InputDefault, group_overrides: Dict[str, str] +) -> List[InputDefault]: + ... + + +def _create_defaults_tree( + repo: IConfigRepository, + root: DefaultsTreeNode, + is_primary_config: bool, + group_overrides: GroupOverrides, +) -> DefaultsTreeNode: + assert root.children is None + + choice = group_overrides.get_choice_for(root.parent) + if root.parent.group is not None: + path = f"{root.parent.group}/{choice}" + else: + path = choice + + loaded = repo.load_config(config_path=path, is_primary_config=is_primary_config) + + if loaded is None: + missing_config_error(repo, root.parent) + else: + children = [] + for d in loaded.new_defaults_list: + if d.is_self(): + children.append(d) + else: + new_root = DefaultsTreeNode(parent=d) + subtree = _create_defaults_tree( + repo=repo, + root=new_root, + is_primary_config=False, + group_overrides=group_overrides, + ) + if subtree.children is None: + children.append(d) + else: + children.append(subtree) + + if len(children) > 0: + root.children = children + return root + + +def _create_defaults_list( + repo: IConfigRepository, + config_name: str, + default_overrides: List[Override], +) -> List[ResultDefault]: + group_overrides = _create_group_overrides(default_overrides) + primary = InputDefault(name=config_name) + root = DefaultsTreeNode(parent=primary) + defaults_tree = _create_defaults_tree( + repo=repo, + root=root, + group_overrides=group_overrides, + is_primary_config=True, + ) + # TODO: convert tree to list with DFS + # TODO: fail if duplicate items exists + return [] + + +def create_defaults_list( + repo: IConfigRepository, + config_name: str, + overrides: List[Override], +) -> DefaultsList: + default_overrides, config_overrides = _split_overrides(repo, overrides) + defaults = _create_defaults_list(repo, config_name, default_overrides) + ret = DefaultsList(defaults=defaults, config_overrides=config_overrides) + return ret + + +def missing_config_error(repo: IConfigRepository, element: InputDefault) -> None: + options = None + if element.name is not None: + options = repo.get_group_options(element.group, ObjectType.CONFIG) + opt_list = "\n".join(["\t" + x for x in options]) + msg = ( + f"Could not find '{element.name}' in the config group '{element.group}'" + f"\nAvailable options:\n{opt_list}\n" + ) + else: + msg = dedent( + f"""\ + Could not load {element.config_path()}. + """ + ) + + descs = [] + for src in repo.get_sources(): + descs.append(f"\t{repr(src)}") + lines = "\n".join(descs) + msg += "\nConfig search path:" + f"\n{lines}" + + raise MissingConfigException( + missing_cfg_file=element.config_path(), + message=msg, + options=options, + ) diff --git a/hydra/core/NewDefaultElement.py b/hydra/core/NewDefaultElement.py index 806ce599235..625411e049e 100644 --- a/hydra/core/NewDefaultElement.py +++ b/hydra/core/NewDefaultElement.py @@ -3,7 +3,7 @@ @dataclass -class InputDefaultElement: +class InputDefault: # config group name if present group: Optional[str] = None # config file name @@ -11,10 +11,5 @@ class InputDefaultElement: optional: bool = False package: Optional[str] = None - -@dataclass -class ProcessedDefaultElement(InputDefaultElement): - parent: Optional[str] = None - - addressing_key: Optional[str] = None - result_package: Optional[str] = None + def is_self(self) -> bool: + return self.name == "_self_" diff --git a/hydra/plugins/config_source.py b/hydra/plugins/config_source.py index 44f29cf4cc2..2371214ef4c 100644 --- a/hydra/plugins/config_source.py +++ b/hydra/plugins/config_source.py @@ -12,7 +12,7 @@ from typing import List, Optional, Dict, Tuple, MutableSequence from hydra.core import DefaultElement -from hydra.core.NewDefaultElement import InputDefaultElement +from hydra.core.NewDefaultElement import InputDefault from hydra.errors import HydraException from omegaconf import ( Container, @@ -34,7 +34,7 @@ class ConfigResult: config: Container header: Dict[str, str] defaults_list: List[DefaultElement] - new_defaults_list: List[InputDefaultElement] + new_defaults_list: List[InputDefault] is_schema_source: bool = False @@ -177,32 +177,38 @@ def _update_package_in_header( ) -> None: config_without_ext = normalized_config_path[0 : -len(".yaml")] + if "package" in header: + # keep a backup of the original package header + # TODO: clean up manipulation of pacakge header in config sources + header["orig_package"] = header["package"] + package = ConfigSource._resolve_package( config_without_ext=config_without_ext, header=header, package_override=package_override, ) - if is_primary_config: - if "package" not in header: - header["package"] = "_global_" - else: - if package != "": - raise HydraException( - f"Primary config '{config_without_ext}' must be " - f"in the _global_ package; effective package : '{package}'" - ) - else: - if "package" not in header: - # Loading a config group option. - # Hydra 1.0: default to _global_ and warn. - # Hydra 1.1: default will change to _package_ and the warning will be removed. - header["package"] = "_global_" - msg = ( - f"\nMissing @package directive {normalized_config_path} in {self.full_path()}.\n" - f"See https://hydra.cc/docs/next/upgrades/0.11_to_1.0/adding_a_package_directive" - ) - warnings.warn(message=msg, category=UserWarning) + # TODO: cleanup + # if is_primary_config: + # if "package" not in header: + # header["package"] = "_global_" + # else: + # if package != "": + # raise HydraException( + # f"Primary config '{config_without_ext}' must be " + # f"in the _global_ package; effective package : '{package}'" + # ) + # else: + # if "package" not in header: + # # Loading a config group option. + # # Hydra 1.0: default to _global_ and warn. + # # Hydra 1.1: default will change to _package_ and the warning will be removed. + # header["package"] = "_global_" + # msg = ( + # f"\nMissing @package directive {normalized_config_path} in {self.full_path()}.\n" + # f"See https://hydra.cc/docs/next/upgrades/0.11_to_1.0/adding_a_package_directive" + # ) + # warnings.warn(message=msg, category=UserWarning) header["package"] = package @@ -383,7 +389,7 @@ def _extract_defaults_list( @staticmethod def _create_new_defaults_list( defaults: ListConfig, - ) -> List[InputDefaultElement]: + ) -> List[InputDefault]: res: List[DefaultElement] = [] for item in defaults: if isinstance(item, DictConfig): @@ -401,7 +407,7 @@ def _create_new_defaults_list( assert node is not None config_name = node._value() - default = InputDefaultElement( + default = InputDefault( group=config_group, name=config_name, package=package, @@ -409,7 +415,7 @@ def _create_new_defaults_list( ) elif isinstance(item, str): path, package, _package2 = ConfigSource._split_group(item) - default = InputDefaultElement(name=path, package=package) + default = InputDefault(name=path, package=package) else: raise ValueError( f"Unsupported type in defaults : {type(item).__name__}" @@ -418,7 +424,7 @@ def _create_new_defaults_list( return res @staticmethod - def _extract_raw_defaults_list(cfg: Container) -> List[InputDefaultElement]: + def _extract_raw_defaults_list(cfg: Container) -> List[InputDefault]: if not OmegaConf.is_dict(cfg): return [] diff --git a/tests/test_data/default_lists/non_config_group_default.yaml b/tests/test_data/default_lists/non_config_group_default.yaml index 004af35e1ed..02480b8a6d6 100644 --- a/tests/test_data/default_lists/non_config_group_default.yaml +++ b/tests/test_data/default_lists/non_config_group_default.yaml @@ -1,2 +1,2 @@ defaults: - - some_config + - empty diff --git a/tests/test_defaults_list2.py b/tests/test_defaults_list2.py index dcffabec7fa..2c010dd7e73 100644 --- a/tests/test_defaults_list2.py +++ b/tests/test_defaults_list2.py @@ -2,9 +2,17 @@ from pytest import mark, param from typing import List -from hydra._internal.config_repository import ConfigRepository +from hydra._internal.config_repository import ConfigRepository, IConfigRepository from hydra._internal.config_search_path_impl import ConfigSearchPathImpl -from hydra.core.NewDefaultElement import InputDefaultElement +from hydra._internal.new_defaults_list import ( + create_defaults_list, + ResultDefault, + DefaultsTreeNode, + _create_defaults_tree, + _create_group_overrides, +) +from hydra.core.NewDefaultElement import InputDefault +from hydra.core.override_parser.overrides_parser import OverridesParser from hydra.core.plugins import Plugins from hydra.test_utils.test_utils import chdir_hydra_root @@ -14,7 +22,14 @@ Plugins.instance() -def create_repo(): +# TODO: +# - test with simple config group overrides +# - test with config group overrides overriding config groups @pkg +# - test handling missing configs mentioned in defaults list (with and without optional) +# - test overriding configs in absolute location + + +def create_repo() -> IConfigRepository: csp = ConfigSearchPathImpl() csp.append(provider="test", path="file://tests/test_data/default_lists") return ConfigRepository(config_search_path=csp) @@ -26,49 +41,175 @@ def create_repo(): param("empty", [], id="empty"), param( "one_item", - [InputDefaultElement(group="group1", name="file1")], + [InputDefault(group="group1", name="file1")], id="one_item", ), param( "one_item", - [InputDefaultElement(group="group1", name="file1")], + [InputDefault(group="group1", name="file1")], id="one_item", ), param( "self_leading", [ - InputDefaultElement(name="_self_"), - InputDefaultElement(group="group1", name="file1"), + InputDefault(name="_self_"), + InputDefault(group="group1", name="file1"), ], id="self_leading", ), param( "self_trailing", [ - InputDefaultElement(group="group1", name="file1"), - InputDefaultElement(name="_self_"), + InputDefault(group="group1", name="file1"), + InputDefault(name="_self_"), ], id="self_trailing", ), param( "optional", [ - InputDefaultElement(group="group1", name="file1", optional=True), + InputDefault(group="group1", name="file1", optional=True), ], id="optional", ), param( "non_config_group_default", [ - InputDefaultElement(name="some_config"), + InputDefault(name="some_config"), ], id="non_config_group_default", ), ], ) -def test_loaded_defaults_list( - config_path: str, expected_list: List[InputDefaultElement] -): +def test_loaded_defaults_list(config_path: str, expected_list: List[InputDefault]): repo = create_repo() result = repo.load_config(config_path=config_path, is_primary_config=True) assert result.new_defaults_list == expected_list + + +def _test_defaults_list_impl( + config_name: str, + overrides: List[str], + expected: List[ResultDefault], +) -> None: + parser = OverridesParser.create() + repo = create_repo() + result = create_defaults_list( + repo=repo, + config_name=config_name, + overrides=parser.parse_overrides(overrides=overrides), + ) + + assert result.defaults == expected + + +@mark.parametrize( + "config_name, overrides, expected", + [ + param("empty", [], [], id="empty"), + ], +) +def test_simple_cases( + config_name: str, + overrides: List[str], + expected: List[ResultDefault], +) -> None: + _test_defaults_list_impl( + config_name=config_name, overrides=overrides, expected=expected + ) + + +def _test_defaults_tree_impl( + config_name: str, + overrides: List[str], + expected: DefaultsTreeNode, +) -> None: + parser = OverridesParser.create() + repo = create_repo() + parent = InputDefault(name=config_name) + root = DefaultsTreeNode(parent=parent) + group_overrides = _create_group_overrides( + parser.parse_overrides(overrides=overrides) + ) + result = _create_defaults_tree( + repo=repo, + root=root, + group_overrides=group_overrides, + is_primary_config=True, + ) + + assert result == expected + + +@mark.parametrize( + "config_name, overrides, expected", + [ + param( + "empty", + [], + DefaultsTreeNode(parent=InputDefault(name="empty")), + id="empty", + ), + param( + "non_config_group_default", + [], + DefaultsTreeNode( + parent=InputDefault(name="non_config_group_default"), + children=[InputDefault(name="empty")], + ), + id="non_config_group_default", + ), + param( + "one_item", + [], + DefaultsTreeNode( + parent=InputDefault(name="one_item"), + children=[InputDefault(group="group1", name="file1")], + ), + id="one_item", + ), + param( + "optional", + [], + DefaultsTreeNode( + parent=InputDefault(name="optional"), + children=[ + InputDefault(group="group1", name="file1", optional=True), + ], + ), + id="optional", + ), + param( + "self_leading", + [], + DefaultsTreeNode( + parent=InputDefault(name="self_leading"), + children=[ + InputDefault(name="_self_"), + InputDefault(group="group1", name="file1"), + ], + ), + id="self_leading", + ), + param( + "self_trailing", + [], + DefaultsTreeNode( + parent=InputDefault(name="self_trailing"), + children=[ + InputDefault(group="group1", name="file1"), + InputDefault(name="_self_"), + ], + ), + id="self_trailing", + ), + ], +) +def test_simple_defaults_tree_cases( + config_name: str, + overrides: List[str], + expected: DefaultsTreeNode, +) -> None: + _test_defaults_tree_impl( + config_name=config_name, overrides=overrides, expected=expected + ) From 1f3c8ee4224d7c7ec48a81a2dd036132ee36df3d Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Mon, 23 Nov 2020 16:17:12 -0800 Subject: [PATCH 05/91] initial nested defaults test passed --- hydra/_internal/new_defaults_list.py | 43 +++++++++---- hydra/core/NewDefaultElement.py | 51 ++++++++++++++- hydra/plugins/config_source.py | 6 +- .../default_lists/group1/config_item.yaml | 2 + .../default_lists/group1/group_item.yaml | 2 + .../default_lists/include_nested_group.yaml | 2 + tests/test_defaults_list2.py | 63 ++++++++++++------- 7 files changed, 130 insertions(+), 39 deletions(-) create mode 100644 tests/test_data/default_lists/group1/config_item.yaml create mode 100644 tests/test_data/default_lists/group1/group_item.yaml create mode 100644 tests/test_data/default_lists/include_nested_group.yaml diff --git a/hydra/_internal/new_defaults_list.py b/hydra/_internal/new_defaults_list.py index 765c7747de0..f909363f6dd 100644 --- a/hydra/_internal/new_defaults_list.py +++ b/hydra/_internal/new_defaults_list.py @@ -4,7 +4,7 @@ from hydra import MissingConfigException from hydra._internal.config_repository import IConfigRepository -from hydra.core.NewDefaultElement import InputDefault +from hydra.core.NewDefaultElement import ConfigDefault, GroupDefault, InputDefault from hydra.core.object_type import ObjectType from hydra.core.override_parser.types import Override @@ -28,15 +28,23 @@ class ResultDefault: class GroupOverrides: map: Dict[str, str] + def is_overridden(self, default: InputDefault) -> bool: + if isinstance(default, GroupDefault): + key = default.group # TODO: use package if present + return key in self.map + + return False + def get_choice_for(self, default: InputDefault) -> str: - if default.group is not None: + if isinstance(default, GroupDefault): key = default.group # TODO: use package if present if key in self.map: return self.map[key] else: return default.name else: - return default.name + assert isinstance(default, ConfigDefault) + return default.path @dataclass @@ -77,11 +85,19 @@ def _create_defaults_tree( ) -> DefaultsTreeNode: assert root.children is None - choice = group_overrides.get_choice_for(root.parent) - if root.parent.group is not None: - path = f"{root.parent.group}/{choice}" + parent = root.parent + if isinstance(parent, GroupDefault): + if group_overrides.is_overridden(parent): + override_name = group_overrides.get_choice_for(parent) + parent.name = override_name + parent.config_name_overridden = True + path = parent.get_config_path() else: - path = choice + assert isinstance(parent, ConfigDefault) + path = parent.path + + if is_primary_config: + root.parent.parent_base_dir = "" loaded = repo.load_config(config_path=path, is_primary_config=is_primary_config) @@ -91,9 +107,12 @@ def _create_defaults_tree( children = [] for d in loaded.new_defaults_list: if d.is_self(): + d.parent_base_dir = root.parent.parent_base_dir children.append(d) else: new_root = DefaultsTreeNode(parent=d) + d.parent_base_dir = parent.get_group_path() + new_root.parent_base_dir = d.get_group_path() subtree = _create_defaults_tree( repo=repo, root=new_root, @@ -116,7 +135,7 @@ def _create_defaults_list( default_overrides: List[Override], ) -> List[ResultDefault]: group_overrides = _create_group_overrides(default_overrides) - primary = InputDefault(name=config_name) + primary = ConfigDefault(path=config_name) root = DefaultsTreeNode(parent=primary) defaults_tree = _create_defaults_tree( repo=repo, @@ -142,17 +161,17 @@ def create_defaults_list( def missing_config_error(repo: IConfigRepository, element: InputDefault) -> None: options = None - if element.name is not None: + if isinstance(element, GroupDefault) is not None: options = repo.get_group_options(element.group, ObjectType.CONFIG) opt_list = "\n".join(["\t" + x for x in options]) msg = ( - f"Could not find '{element.name}' in the config group '{element.group}'" + f"Could not find '{element.name}' in the config group '{element.get_group_path()}'" f"\nAvailable options:\n{opt_list}\n" ) else: msg = dedent( f"""\ - Could not load {element.config_path()}. + Could not load {element.get_config_path()}. """ ) @@ -163,7 +182,7 @@ def missing_config_error(repo: IConfigRepository, element: InputDefault) -> None msg += "\nConfig search path:" + f"\n{lines}" raise MissingConfigException( - missing_cfg_file=element.config_path(), + missing_cfg_file=element.get_config_path(), message=msg, options=options, ) diff --git a/hydra/core/NewDefaultElement.py b/hydra/core/NewDefaultElement.py index 625411e049e..86ec571dc64 100644 --- a/hydra/core/NewDefaultElement.py +++ b/hydra/core/NewDefaultElement.py @@ -1,9 +1,41 @@ -from dataclasses import dataclass +from dataclasses import dataclass, field from typing import Optional @dataclass class InputDefault: + def is_self(self) -> bool: + raise NotImplementedError() + + def get_group_path(self) -> str: + raise NotImplementedError() + + def get_config_path(self) -> str: + raise NotImplementedError() + + +@dataclass +class ConfigDefault(InputDefault): + path: str + package: Optional[str] = None + parent_base_dir: Optional[str] = field(default=None, compare=False, repr=False) + + def is_self(self) -> bool: + return self.path == "_self_" or self.path.endswith("/_self_") + + def get_group_path(self) -> str: + idx = self.path.rfind("/") + if idx == -1: + return "" + else: + return self.path[0:idx] + + def get_config_path(self) -> str: + return self.path + + +@dataclass +class GroupDefault(InputDefault): # config group name if present group: Optional[str] = None # config file name @@ -11,5 +43,22 @@ class InputDefault: optional: bool = False package: Optional[str] = None + parent_base_dir: Optional[str] = field(default=None, compare=False, repr=False) + config_name_overridden: bool = field(default=False, compare=False, repr=False) + def is_self(self) -> bool: return self.name == "_self_" + + def get_group_path(self) -> str: + assert self.parent_base_dir is not None + assert self.group is not None + if self.parent_base_dir == "": + return self.group + else: + return f"{self.parent_base_dir}/{self.group}" + + def get_config_path(self) -> str: + group_path = self.get_group_path() + assert group_path != "" + + return f"{group_path}/{self.name}" diff --git a/hydra/plugins/config_source.py b/hydra/plugins/config_source.py index 2371214ef4c..eb28dab5733 100644 --- a/hydra/plugins/config_source.py +++ b/hydra/plugins/config_source.py @@ -12,7 +12,7 @@ from typing import List, Optional, Dict, Tuple, MutableSequence from hydra.core import DefaultElement -from hydra.core.NewDefaultElement import InputDefault +from hydra.core.NewDefaultElement import InputDefault, GroupDefault, ConfigDefault from hydra.errors import HydraException from omegaconf import ( Container, @@ -407,7 +407,7 @@ def _create_new_defaults_list( assert node is not None config_name = node._value() - default = InputDefault( + default = GroupDefault( group=config_group, name=config_name, package=package, @@ -415,7 +415,7 @@ def _create_new_defaults_list( ) elif isinstance(item, str): path, package, _package2 = ConfigSource._split_group(item) - default = InputDefault(name=path, package=package) + default = ConfigDefault(path=path, package=package) else: raise ValueError( f"Unsupported type in defaults : {type(item).__name__}" diff --git a/tests/test_data/default_lists/group1/config_item.yaml b/tests/test_data/default_lists/group1/config_item.yaml new file mode 100644 index 00000000000..2c5857d7d64 --- /dev/null +++ b/tests/test_data/default_lists/group1/config_item.yaml @@ -0,0 +1,2 @@ +defaults: + - group2/file1 \ No newline at end of file diff --git a/tests/test_data/default_lists/group1/group_item.yaml b/tests/test_data/default_lists/group1/group_item.yaml new file mode 100644 index 00000000000..55a0f77f44d --- /dev/null +++ b/tests/test_data/default_lists/group1/group_item.yaml @@ -0,0 +1,2 @@ +defaults: + - group2: file1 \ No newline at end of file diff --git a/tests/test_data/default_lists/include_nested_group.yaml b/tests/test_data/default_lists/include_nested_group.yaml new file mode 100644 index 00000000000..fbdf3398002 --- /dev/null +++ b/tests/test_data/default_lists/include_nested_group.yaml @@ -0,0 +1,2 @@ +defaults: + - group1: group_item \ No newline at end of file diff --git a/tests/test_defaults_list2.py b/tests/test_defaults_list2.py index 2c010dd7e73..0b239486c2e 100644 --- a/tests/test_defaults_list2.py +++ b/tests/test_defaults_list2.py @@ -11,7 +11,7 @@ _create_defaults_tree, _create_group_overrides, ) -from hydra.core.NewDefaultElement import InputDefault +from hydra.core.NewDefaultElement import InputDefault, GroupDefault, ConfigDefault from hydra.core.override_parser.overrides_parser import OverridesParser from hydra.core.plugins import Plugins from hydra.test_utils.test_utils import chdir_hydra_root @@ -41,41 +41,41 @@ def create_repo() -> IConfigRepository: param("empty", [], id="empty"), param( "one_item", - [InputDefault(group="group1", name="file1")], + [GroupDefault(group="group1", name="file1")], id="one_item", ), param( "one_item", - [InputDefault(group="group1", name="file1")], + [GroupDefault(group="group1", name="file1")], id="one_item", ), param( "self_leading", [ - InputDefault(name="_self_"), - InputDefault(group="group1", name="file1"), + ConfigDefault(path="_self_"), + GroupDefault(group="group1", name="file1"), ], id="self_leading", ), param( "self_trailing", [ - InputDefault(group="group1", name="file1"), - InputDefault(name="_self_"), + GroupDefault(group="group1", name="file1"), + ConfigDefault(path="_self_"), ], id="self_trailing", ), param( "optional", [ - InputDefault(group="group1", name="file1", optional=True), + GroupDefault(group="group1", name="file1", optional=True), ], id="optional", ), param( "non_config_group_default", [ - InputDefault(name="some_config"), + ConfigDefault(path="empty"), ], id="non_config_group_default", ), @@ -126,7 +126,7 @@ def _test_defaults_tree_impl( ) -> None: parser = OverridesParser.create() repo = create_repo() - parent = InputDefault(name=config_name) + parent = ConfigDefault(path=config_name) root = DefaultsTreeNode(parent=parent) group_overrides = _create_group_overrides( parser.parse_overrides(overrides=overrides) @@ -147,15 +147,15 @@ def _test_defaults_tree_impl( param( "empty", [], - DefaultsTreeNode(parent=InputDefault(name="empty")), + DefaultsTreeNode(parent=ConfigDefault(path="empty")), id="empty", ), param( "non_config_group_default", [], DefaultsTreeNode( - parent=InputDefault(name="non_config_group_default"), - children=[InputDefault(name="empty")], + parent=ConfigDefault(path="non_config_group_default"), + children=[ConfigDefault(path="empty")], ), id="non_config_group_default", ), @@ -163,8 +163,8 @@ def _test_defaults_tree_impl( "one_item", [], DefaultsTreeNode( - parent=InputDefault(name="one_item"), - children=[InputDefault(group="group1", name="file1")], + parent=ConfigDefault(path="one_item"), + children=[GroupDefault(group="group1", name="file1")], ), id="one_item", ), @@ -172,9 +172,9 @@ def _test_defaults_tree_impl( "optional", [], DefaultsTreeNode( - parent=InputDefault(name="optional"), + parent=ConfigDefault(path="optional"), children=[ - InputDefault(group="group1", name="file1", optional=True), + GroupDefault(group="group1", name="file1", optional=True), ], ), id="optional", @@ -183,10 +183,10 @@ def _test_defaults_tree_impl( "self_leading", [], DefaultsTreeNode( - parent=InputDefault(name="self_leading"), + parent=ConfigDefault(path="self_leading"), children=[ - InputDefault(name="_self_"), - InputDefault(group="group1", name="file1"), + ConfigDefault(path="_self_"), + GroupDefault(group="group1", name="file1"), ], ), id="self_leading", @@ -195,14 +195,28 @@ def _test_defaults_tree_impl( "self_trailing", [], DefaultsTreeNode( - parent=InputDefault(name="self_trailing"), + parent=ConfigDefault(path="self_trailing"), children=[ - InputDefault(group="group1", name="file1"), - InputDefault(name="_self_"), + GroupDefault(group="group1", name="file1"), + ConfigDefault(path="_self_"), ], ), id="self_trailing", ), + param( + "include_nested_group", + [], + DefaultsTreeNode( + parent=ConfigDefault(path="include_nested_group"), + children=[ + DefaultsTreeNode( + parent=GroupDefault(group="group1", name="group_item"), + children=[GroupDefault(group="group2", name="file1")], + ) + ], + ), + id="include_nested_group", + ), ], ) def test_simple_defaults_tree_cases( @@ -213,3 +227,6 @@ def test_simple_defaults_tree_cases( _test_defaults_tree_impl( config_name=config_name, overrides=overrides, expected=expected ) + + +# TODO: test config_path() From e194f1b2e2ca6b0396f3686b45fd5762d2b86c21 Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Mon, 23 Nov 2020 16:35:26 -0800 Subject: [PATCH 06/91] added tests for get_config_path and get_group_path --- hydra/core/NewDefaultElement.py | 22 ++++++++++++++++++--- tests/test_defaults_list2.py | 35 ++++++++++++++++++++++++++++++++- 2 files changed, 53 insertions(+), 4 deletions(-) diff --git a/hydra/core/NewDefaultElement.py b/hydra/core/NewDefaultElement.py index 86ec571dc64..077025e7432 100644 --- a/hydra/core/NewDefaultElement.py +++ b/hydra/core/NewDefaultElement.py @@ -24,14 +24,27 @@ def is_self(self) -> bool: return self.path == "_self_" or self.path.endswith("/_self_") def get_group_path(self) -> str: + assert self.parent_base_dir is not None idx = self.path.rfind("/") if idx == -1: - return "" + group = "" + else: + group = self.path[0:idx] + + if self.parent_base_dir == "": + return group else: - return self.path[0:idx] + if group == "": + return f"{self.parent_base_dir}" + else: + return f"{self.parent_base_dir}/{group}" def get_config_path(self) -> str: - return self.path + assert self.parent_base_dir is not None + if self.parent_base_dir == "": + return self.path + else: + return f"{self.parent_base_dir}/{self.path}" @dataclass @@ -46,6 +59,9 @@ class GroupDefault(InputDefault): parent_base_dir: Optional[str] = field(default=None, compare=False, repr=False) config_name_overridden: bool = field(default=False, compare=False, repr=False) + def __post_init__(self): + assert self.group is not None and self.group != "" + def is_self(self) -> bool: return self.name == "_self_" diff --git a/tests/test_defaults_list2.py b/tests/test_defaults_list2.py index 0b239486c2e..963f147486d 100644 --- a/tests/test_defaults_list2.py +++ b/tests/test_defaults_list2.py @@ -229,4 +229,37 @@ def test_simple_defaults_tree_cases( ) -# TODO: test config_path() +@mark.parametrize( + "default,expected_group_path,expected_config_path", + [ + param( + ConfigDefault(path="bar", parent_base_dir=""), + "", + "bar", + id="config_default:empty_basedir", + ), + param( + ConfigDefault(path="bar", parent_base_dir="foo"), + "foo", + "foo/bar", + id="config_default:with_parent_basedir", + ), + param( + GroupDefault(group="foo", name="bar", parent_base_dir=""), + "foo", + "foo/bar", + id="group_default:empty_basedir", + ), + param( + GroupDefault(group="foo", name="bar", parent_base_dir="zoo"), + "zoo/foo", + "zoo/foo/bar", + id="group_default:with_parent_basedir", + ), + ], +) +def test_get_paths( + default: InputDefault, expected_group_path, expected_config_path +) -> None: + assert default.get_group_path() == expected_group_path + assert default.get_config_path() == expected_config_path From b1713a7cc8e2fc5d5d95526d7676864ac2d95cc8 Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Tue, 24 Nov 2020 11:01:56 -0800 Subject: [PATCH 07/91] append tests for defaults tree --- hydra/_internal/new_defaults_list.py | 90 +++++---- tests/defaults_list/__init__.py | 40 ++++ .../test_basic.py} | 130 +------------ tests/defaults_list/test_defaults_tree.py | 181 ++++++++++++++++++ ...group_default.yaml => config_default.yaml} | 0 .../{one_item.yaml => group_default.yaml} | 0 6 files changed, 277 insertions(+), 164 deletions(-) create mode 100644 tests/defaults_list/__init__.py rename tests/{test_defaults_list2.py => defaults_list/test_basic.py} (50%) create mode 100644 tests/defaults_list/test_defaults_tree.py rename tests/test_data/default_lists/{non_config_group_default.yaml => config_default.yaml} (100%) rename tests/test_data/default_lists/{one_item.yaml => group_default.yaml} (100%) diff --git a/hydra/_internal/new_defaults_list.py b/hydra/_internal/new_defaults_list.py index f909363f6dd..3a1ad97241b 100644 --- a/hydra/_internal/new_defaults_list.py +++ b/hydra/_internal/new_defaults_list.py @@ -1,6 +1,7 @@ +import copy from dataclasses import dataclass from textwrap import dedent -from typing import Dict, List, Optional, Tuple, Union +from typing import Dict, List, Optional, Union from hydra import MissingConfigException from hydra._internal.config_repository import IConfigRepository @@ -25,21 +26,45 @@ class ResultDefault: @dataclass -class GroupOverrides: - map: Dict[str, str] +class Overrides: + override_choices: Dict[str, str] + append_group_defaults: List[GroupDefault] + config_overrides: List[Override] + + def __init__(self, repo: IConfigRepository, overrides_list: List[Override]) -> None: + self.override_choices = {} + self.append_group_defaults = [] + self.config_overrides = [] + + for override in overrides_list: + is_group = repo.group_exists(override.key_or_group) + value = override.value() + if not is_group: + self.config_overrides.append(override) + else: + if not isinstance(value, str): + raise ValueError( + f"Config group override must be a string : {override}" + ) + if override.is_add(): + self.append_group_defaults.append( + GroupDefault(group=override.key_or_group, name=value) + ) + else: + self.override_choices[override.key_or_group] = value def is_overridden(self, default: InputDefault) -> bool: if isinstance(default, GroupDefault): key = default.group # TODO: use package if present - return key in self.map + return key in self.override_choices return False def get_choice_for(self, default: InputDefault) -> str: if isinstance(default, GroupDefault): key = default.group # TODO: use package if present - if key in self.map: - return self.map[key] + if key in self.override_choices: + return self.override_choices[key] else: return default.name else: @@ -53,24 +78,6 @@ class DefaultsList: config_overrides: List[Override] -def _split_overrides( - repo: IConfigRepository, - overrides: List[Override], -) -> Tuple[List[Override], List[Override]]: - # TODO - return overrides, [] - - -def _create_group_overrides(default_overrides: List[Override]) -> GroupOverrides: - group_overrides = GroupOverrides(map={}) - for override in default_overrides: - value = override.value() - assert isinstance(value, str) - group_overrides.map[override.key_or_group] = value - - return group_overrides - - def load_config_defaults_list( default: InputDefault, group_overrides: Dict[str, str] ) -> List[InputDefault]: @@ -81,14 +88,17 @@ def _create_defaults_tree( repo: IConfigRepository, root: DefaultsTreeNode, is_primary_config: bool, - group_overrides: GroupOverrides, + overrides: Overrides, ) -> DefaultsTreeNode: assert root.children is None + if is_primary_config: + root.parent.parent_base_dir = "" + parent = root.parent if isinstance(parent, GroupDefault): - if group_overrides.is_overridden(parent): - override_name = group_overrides.get_choice_for(parent) + if overrides.is_overridden(parent): + override_name = overrides.get_choice_for(parent) parent.name = override_name parent.config_name_overridden = True path = parent.get_config_path() @@ -96,16 +106,19 @@ def _create_defaults_tree( assert isinstance(parent, ConfigDefault) path = parent.path - if is_primary_config: - root.parent.parent_base_dir = "" - loaded = repo.load_config(config_path=path, is_primary_config=is_primary_config) if loaded is None: missing_config_error(repo, root.parent) else: + + defaults_list = copy.deepcopy(loaded.new_defaults_list) + if is_primary_config: + for d in overrides.append_group_defaults: + defaults_list.append(d) + children = [] - for d in loaded.new_defaults_list: + for d in defaults_list: if d.is_self(): d.parent_base_dir = root.parent.parent_base_dir children.append(d) @@ -117,7 +130,7 @@ def _create_defaults_tree( repo=repo, root=new_root, is_primary_config=False, - group_overrides=group_overrides, + overrides=overrides, ) if subtree.children is None: children.append(d) @@ -132,15 +145,14 @@ def _create_defaults_tree( def _create_defaults_list( repo: IConfigRepository, config_name: str, - default_overrides: List[Override], + overrides: Overrides, ) -> List[ResultDefault]: - group_overrides = _create_group_overrides(default_overrides) primary = ConfigDefault(path=config_name) root = DefaultsTreeNode(parent=primary) defaults_tree = _create_defaults_tree( repo=repo, root=root, - group_overrides=group_overrides, + overrides=overrides, is_primary_config=True, ) # TODO: convert tree to list with DFS @@ -151,11 +163,11 @@ def _create_defaults_list( def create_defaults_list( repo: IConfigRepository, config_name: str, - overrides: List[Override], + overrides_list: List[Override], ) -> DefaultsList: - default_overrides, config_overrides = _split_overrides(repo, overrides) - defaults = _create_defaults_list(repo, config_name, default_overrides) - ret = DefaultsList(defaults=defaults, config_overrides=config_overrides) + overrides = Overrides(repo=repo, overrides_list=overrides_list) + defaults = _create_defaults_list(repo, config_name, overrides) + ret = DefaultsList(defaults=defaults, config_overrides=overrides.config_overrides) return ret diff --git a/tests/defaults_list/__init__.py b/tests/defaults_list/__init__.py new file mode 100644 index 00000000000..75ab7b75427 --- /dev/null +++ b/tests/defaults_list/__init__.py @@ -0,0 +1,40 @@ +# Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved +from typing import List + +from hydra._internal.config_repository import IConfigRepository, ConfigRepository +from hydra._internal.config_search_path_impl import ConfigSearchPathImpl +from hydra._internal.new_defaults_list import ( + DefaultsTreeNode, + _create_defaults_tree, + Overrides, +) +from hydra.core.NewDefaultElement import ConfigDefault +from hydra.core.override_parser.overrides_parser import OverridesParser + + +def create_repo() -> IConfigRepository: + csp = ConfigSearchPathImpl() + csp.append(provider="test", path="file://tests/test_data/default_lists") + return ConfigRepository(config_search_path=csp) + + +def _test_defaults_tree_impl( + config_name: str, + overrides_list: List[str], + expected: DefaultsTreeNode, +) -> None: + parser = OverridesParser.create() + repo = create_repo() + parent = ConfigDefault(path=config_name) + root = DefaultsTreeNode(parent=parent) + overrides = Overrides( + repo=repo, overrides_list=parser.parse_overrides(overrides=overrides_list) + ) + result = _create_defaults_tree( + repo=repo, + root=root, + overrides=overrides, + is_primary_config=True, + ) + + assert result == expected diff --git a/tests/test_defaults_list2.py b/tests/defaults_list/test_basic.py similarity index 50% rename from tests/test_defaults_list2.py rename to tests/defaults_list/test_basic.py index 963f147486d..1a0e06a5901 100644 --- a/tests/test_defaults_list2.py +++ b/tests/defaults_list/test_basic.py @@ -2,19 +2,15 @@ from pytest import mark, param from typing import List -from hydra._internal.config_repository import ConfigRepository, IConfigRepository -from hydra._internal.config_search_path_impl import ConfigSearchPathImpl from hydra._internal.new_defaults_list import ( create_defaults_list, ResultDefault, - DefaultsTreeNode, - _create_defaults_tree, - _create_group_overrides, ) from hydra.core.NewDefaultElement import InputDefault, GroupDefault, ConfigDefault from hydra.core.override_parser.overrides_parser import OverridesParser from hydra.core.plugins import Plugins from hydra.test_utils.test_utils import chdir_hydra_root +from tests.defaults_list import create_repo chdir_hydra_root() @@ -29,23 +25,17 @@ # - test overriding configs in absolute location -def create_repo() -> IConfigRepository: - csp = ConfigSearchPathImpl() - csp.append(provider="test", path="file://tests/test_data/default_lists") - return ConfigRepository(config_search_path=csp) - - @mark.parametrize( "config_path,expected_list", [ param("empty", [], id="empty"), param( - "one_item", + "group_default", [GroupDefault(group="group1", name="file1")], id="one_item", ), param( - "one_item", + "group_default", [GroupDefault(group="group1", name="file1")], id="one_item", ), @@ -73,7 +63,7 @@ def create_repo() -> IConfigRepository: id="optional", ), param( - "non_config_group_default", + "config_default", [ ConfigDefault(path="empty"), ], @@ -97,7 +87,7 @@ def _test_defaults_list_impl( result = create_defaults_list( repo=repo, config_name=config_name, - overrides=parser.parse_overrides(overrides=overrides), + overrides_list=parser.parse_overrides(overrides=overrides), ) assert result.defaults == expected @@ -119,116 +109,6 @@ def test_simple_cases( ) -def _test_defaults_tree_impl( - config_name: str, - overrides: List[str], - expected: DefaultsTreeNode, -) -> None: - parser = OverridesParser.create() - repo = create_repo() - parent = ConfigDefault(path=config_name) - root = DefaultsTreeNode(parent=parent) - group_overrides = _create_group_overrides( - parser.parse_overrides(overrides=overrides) - ) - result = _create_defaults_tree( - repo=repo, - root=root, - group_overrides=group_overrides, - is_primary_config=True, - ) - - assert result == expected - - -@mark.parametrize( - "config_name, overrides, expected", - [ - param( - "empty", - [], - DefaultsTreeNode(parent=ConfigDefault(path="empty")), - id="empty", - ), - param( - "non_config_group_default", - [], - DefaultsTreeNode( - parent=ConfigDefault(path="non_config_group_default"), - children=[ConfigDefault(path="empty")], - ), - id="non_config_group_default", - ), - param( - "one_item", - [], - DefaultsTreeNode( - parent=ConfigDefault(path="one_item"), - children=[GroupDefault(group="group1", name="file1")], - ), - id="one_item", - ), - param( - "optional", - [], - DefaultsTreeNode( - parent=ConfigDefault(path="optional"), - children=[ - GroupDefault(group="group1", name="file1", optional=True), - ], - ), - id="optional", - ), - param( - "self_leading", - [], - DefaultsTreeNode( - parent=ConfigDefault(path="self_leading"), - children=[ - ConfigDefault(path="_self_"), - GroupDefault(group="group1", name="file1"), - ], - ), - id="self_leading", - ), - param( - "self_trailing", - [], - DefaultsTreeNode( - parent=ConfigDefault(path="self_trailing"), - children=[ - GroupDefault(group="group1", name="file1"), - ConfigDefault(path="_self_"), - ], - ), - id="self_trailing", - ), - param( - "include_nested_group", - [], - DefaultsTreeNode( - parent=ConfigDefault(path="include_nested_group"), - children=[ - DefaultsTreeNode( - parent=GroupDefault(group="group1", name="group_item"), - children=[GroupDefault(group="group2", name="file1")], - ) - ], - ), - id="include_nested_group", - ), - ], -) -def test_simple_defaults_tree_cases( - config_name: str, - overrides: List[str], - expected: DefaultsTreeNode, -) -> None: - _test_defaults_tree_impl( - config_name=config_name, overrides=overrides, expected=expected - ) - - @mark.parametrize( "default,expected_group_path,expected_config_path", [ diff --git a/tests/defaults_list/test_defaults_tree.py b/tests/defaults_list/test_defaults_tree.py new file mode 100644 index 00000000000..295ea550da0 --- /dev/null +++ b/tests/defaults_list/test_defaults_tree.py @@ -0,0 +1,181 @@ +# Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved +from pytest import mark, param +from typing import List + +from hydra._internal.new_defaults_list import ( + DefaultsTreeNode, +) +from hydra.core.NewDefaultElement import GroupDefault, ConfigDefault +from hydra.core.plugins import Plugins +from hydra.test_utils.test_utils import chdir_hydra_root +from tests.defaults_list import _test_defaults_tree_impl + +chdir_hydra_root() + +# registers config source plugins +Plugins.instance() + + +@mark.parametrize( + "config_name, overrides, expected", + [ + param( + "empty", + [], + DefaultsTreeNode(parent=ConfigDefault(path="empty")), + id="empty", + ), + param( + "config_default", + [], + DefaultsTreeNode( + parent=ConfigDefault(path="config_default"), + children=[ConfigDefault(path="empty")], + ), + id="config_default", + ), + param( + "group_default", + [], + DefaultsTreeNode( + parent=ConfigDefault(path="group_default"), + children=[GroupDefault(group="group1", name="file1")], + ), + id="group_default", + ), + param( + "optional", + [], + DefaultsTreeNode( + parent=ConfigDefault(path="optional"), + children=[ + GroupDefault(group="group1", name="file1", optional=True), + ], + ), + id="optional", + ), + param( + "self_leading", + [], + DefaultsTreeNode( + parent=ConfigDefault(path="self_leading"), + children=[ + ConfigDefault(path="_self_"), + GroupDefault(group="group1", name="file1"), + ], + ), + id="self_leading", + ), + param( + "self_trailing", + [], + DefaultsTreeNode( + parent=ConfigDefault(path="self_trailing"), + children=[ + GroupDefault(group="group1", name="file1"), + ConfigDefault(path="_self_"), + ], + ), + id="self_trailing", + ), + param( + "include_nested_group", + [], + DefaultsTreeNode( + parent=ConfigDefault(path="include_nested_group"), + children=[ + DefaultsTreeNode( + parent=GroupDefault(group="group1", name="group_item"), + children=[GroupDefault(group="group2", name="file1")], + ) + ], + ), + id="include_nested_group", + ), + ], +) +def test_simple_defaults_tree_cases( + config_name: str, + overrides: List[str], + expected: DefaultsTreeNode, +) -> None: + _test_defaults_tree_impl( + config_name=config_name, overrides_list=overrides, expected=expected + ) + + +@mark.parametrize( + "config_name, overrides, expected", + [ + param( + "empty", + ["+group1=file1"], + DefaultsTreeNode( + parent=ConfigDefault(path="empty"), + children=[GroupDefault(group="group1", name="file1")], + ), + id="empty:append", + ), + param( + "config_default", + ["+group1=file1"], + DefaultsTreeNode( + parent=ConfigDefault(path="config_default"), + children=[ + ConfigDefault(path="empty"), + GroupDefault(group="group1", name="file1"), + ], + ), + id="config_default:append", + ), + param( + "group_default", + ["+group1=file1"], + DefaultsTreeNode( + parent=ConfigDefault(path="group_default"), + children=[ + # config tree allow for duplicate items + GroupDefault(group="group1", name="file1"), + GroupDefault(group="group1", name="file1"), + ], + ), + id="group_default:append", + ), + param( + "self_trailing", + ["+group1=file1"], + DefaultsTreeNode( + parent=ConfigDefault(path="self_trailing"), + children=[ + GroupDefault(group="group1", name="file1"), + ConfigDefault(path="_self_"), + GroupDefault(group="group1", name="file1"), + ], + ), + id="self_trailing:append", + ), + param( + "include_nested_group", + ["+group1=file1"], + DefaultsTreeNode( + parent=ConfigDefault(path="include_nested_group"), + children=[ + DefaultsTreeNode( + parent=GroupDefault(group="group1", name="group_item"), + children=[GroupDefault(group="group2", name="file1")], + ), + GroupDefault(group="group1", name="file1"), + ], + ), + id="include_nested_group:append", + ), + ], +) +def test_tree_with_append_override( + config_name: str, + overrides: List[str], + expected: DefaultsTreeNode, +) -> None: + _test_defaults_tree_impl( + config_name=config_name, overrides_list=overrides, expected=expected + ) diff --git a/tests/test_data/default_lists/non_config_group_default.yaml b/tests/test_data/default_lists/config_default.yaml similarity index 100% rename from tests/test_data/default_lists/non_config_group_default.yaml rename to tests/test_data/default_lists/config_default.yaml diff --git a/tests/test_data/default_lists/one_item.yaml b/tests/test_data/default_lists/group_default.yaml similarity index 100% rename from tests/test_data/default_lists/one_item.yaml rename to tests/test_data/default_lists/group_default.yaml From 4d794e4b1e2b90bc5159e7ea6af8b81b6ba029d8 Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Tue, 24 Nov 2020 11:22:05 -0800 Subject: [PATCH 08/91] simple group override tests for defaults tree --- hydra/_internal/new_defaults_list.py | 24 ++--- tests/defaults_list/__init__.py | 9 +- tests/defaults_list/test_defaults_tree.py | 95 +++++++++++++++---- .../test_data/default_lists/group1/file2.yaml | 0 .../default_lists/group1/group2/file2.yaml | 0 .../{group_item.yaml => group_item1.yaml} | 0 .../default_lists/group1/group_item2.yaml | 2 + .../default_lists/include_nested_group.yaml | 2 +- 8 files changed, 95 insertions(+), 37 deletions(-) create mode 100644 tests/test_data/default_lists/group1/file2.yaml create mode 100644 tests/test_data/default_lists/group1/group2/file2.yaml rename tests/test_data/default_lists/group1/{group_item.yaml => group_item1.yaml} (100%) create mode 100644 tests/test_data/default_lists/group1/group_item2.yaml diff --git a/hydra/_internal/new_defaults_list.py b/hydra/_internal/new_defaults_list.py index 3a1ad97241b..c1ad037e280 100644 --- a/hydra/_internal/new_defaults_list.py +++ b/hydra/_internal/new_defaults_list.py @@ -12,7 +12,7 @@ @dataclass class DefaultsTreeNode: - parent: InputDefault + node: InputDefault children: Optional[List[Union["DefaultsTreeNode", InputDefault]]] = None @@ -55,14 +55,14 @@ def __init__(self, repo: IConfigRepository, overrides_list: List[Override]) -> N def is_overridden(self, default: InputDefault) -> bool: if isinstance(default, GroupDefault): - key = default.group # TODO: use package if present + key = default.get_group_path() # TODO: use package if present return key in self.override_choices return False def get_choice_for(self, default: InputDefault) -> str: if isinstance(default, GroupDefault): - key = default.group # TODO: use package if present + key = default.get_group_path() # TODO: use package if present if key in self.override_choices: return self.override_choices[key] else: @@ -78,12 +78,6 @@ class DefaultsList: config_overrides: List[Override] -def load_config_defaults_list( - default: InputDefault, group_overrides: Dict[str, str] -) -> List[InputDefault]: - ... - - def _create_defaults_tree( repo: IConfigRepository, root: DefaultsTreeNode, @@ -93,9 +87,9 @@ def _create_defaults_tree( assert root.children is None if is_primary_config: - root.parent.parent_base_dir = "" + root.node.parent_base_dir = "" - parent = root.parent + parent = root.node if isinstance(parent, GroupDefault): if overrides.is_overridden(parent): override_name = overrides.get_choice_for(parent) @@ -109,7 +103,7 @@ def _create_defaults_tree( loaded = repo.load_config(config_path=path, is_primary_config=is_primary_config) if loaded is None: - missing_config_error(repo, root.parent) + missing_config_error(repo, root.node) else: defaults_list = copy.deepcopy(loaded.new_defaults_list) @@ -120,10 +114,10 @@ def _create_defaults_tree( children = [] for d in defaults_list: if d.is_self(): - d.parent_base_dir = root.parent.parent_base_dir + d.parent_base_dir = root.node.parent_base_dir children.append(d) else: - new_root = DefaultsTreeNode(parent=d) + new_root = DefaultsTreeNode(node=d) d.parent_base_dir = parent.get_group_path() new_root.parent_base_dir = d.get_group_path() subtree = _create_defaults_tree( @@ -148,7 +142,7 @@ def _create_defaults_list( overrides: Overrides, ) -> List[ResultDefault]: primary = ConfigDefault(path=config_name) - root = DefaultsTreeNode(parent=primary) + root = DefaultsTreeNode(node=primary) defaults_tree = _create_defaults_tree( repo=repo, root=root, diff --git a/tests/defaults_list/__init__.py b/tests/defaults_list/__init__.py index 75ab7b75427..ad5ed1847d1 100644 --- a/tests/defaults_list/__init__.py +++ b/tests/defaults_list/__init__.py @@ -20,16 +20,15 @@ def create_repo() -> IConfigRepository: def _test_defaults_tree_impl( config_name: str, - overrides_list: List[str], + input_overrides: List[str], expected: DefaultsTreeNode, ) -> None: parser = OverridesParser.create() repo = create_repo() parent = ConfigDefault(path=config_name) - root = DefaultsTreeNode(parent=parent) - overrides = Overrides( - repo=repo, overrides_list=parser.parse_overrides(overrides=overrides_list) - ) + root = DefaultsTreeNode(node=parent) + overrides_list = parser.parse_overrides(overrides=input_overrides) + overrides = Overrides(repo=repo, overrides_list=overrides_list) result = _create_defaults_tree( repo=repo, root=root, diff --git a/tests/defaults_list/test_defaults_tree.py b/tests/defaults_list/test_defaults_tree.py index 295ea550da0..cd4809376af 100644 --- a/tests/defaults_list/test_defaults_tree.py +++ b/tests/defaults_list/test_defaults_tree.py @@ -22,14 +22,14 @@ param( "empty", [], - DefaultsTreeNode(parent=ConfigDefault(path="empty")), + DefaultsTreeNode(node=ConfigDefault(path="empty")), id="empty", ), param( "config_default", [], DefaultsTreeNode( - parent=ConfigDefault(path="config_default"), + node=ConfigDefault(path="config_default"), children=[ConfigDefault(path="empty")], ), id="config_default", @@ -38,7 +38,7 @@ "group_default", [], DefaultsTreeNode( - parent=ConfigDefault(path="group_default"), + node=ConfigDefault(path="group_default"), children=[GroupDefault(group="group1", name="file1")], ), id="group_default", @@ -47,7 +47,7 @@ "optional", [], DefaultsTreeNode( - parent=ConfigDefault(path="optional"), + node=ConfigDefault(path="optional"), children=[ GroupDefault(group="group1", name="file1", optional=True), ], @@ -58,7 +58,7 @@ "self_leading", [], DefaultsTreeNode( - parent=ConfigDefault(path="self_leading"), + node=ConfigDefault(path="self_leading"), children=[ ConfigDefault(path="_self_"), GroupDefault(group="group1", name="file1"), @@ -70,7 +70,7 @@ "self_trailing", [], DefaultsTreeNode( - parent=ConfigDefault(path="self_trailing"), + node=ConfigDefault(path="self_trailing"), children=[ GroupDefault(group="group1", name="file1"), ConfigDefault(path="_self_"), @@ -82,10 +82,10 @@ "include_nested_group", [], DefaultsTreeNode( - parent=ConfigDefault(path="include_nested_group"), + node=ConfigDefault(path="include_nested_group"), children=[ DefaultsTreeNode( - parent=GroupDefault(group="group1", name="group_item"), + node=GroupDefault(group="group1", name="group_item1"), children=[GroupDefault(group="group2", name="file1")], ) ], @@ -100,7 +100,7 @@ def test_simple_defaults_tree_cases( expected: DefaultsTreeNode, ) -> None: _test_defaults_tree_impl( - config_name=config_name, overrides_list=overrides, expected=expected + config_name=config_name, input_overrides=overrides, expected=expected ) @@ -111,7 +111,7 @@ def test_simple_defaults_tree_cases( "empty", ["+group1=file1"], DefaultsTreeNode( - parent=ConfigDefault(path="empty"), + node=ConfigDefault(path="empty"), children=[GroupDefault(group="group1", name="file1")], ), id="empty:append", @@ -120,7 +120,7 @@ def test_simple_defaults_tree_cases( "config_default", ["+group1=file1"], DefaultsTreeNode( - parent=ConfigDefault(path="config_default"), + node=ConfigDefault(path="config_default"), children=[ ConfigDefault(path="empty"), GroupDefault(group="group1", name="file1"), @@ -132,7 +132,7 @@ def test_simple_defaults_tree_cases( "group_default", ["+group1=file1"], DefaultsTreeNode( - parent=ConfigDefault(path="group_default"), + node=ConfigDefault(path="group_default"), children=[ # config tree allow for duplicate items GroupDefault(group="group1", name="file1"), @@ -145,7 +145,7 @@ def test_simple_defaults_tree_cases( "self_trailing", ["+group1=file1"], DefaultsTreeNode( - parent=ConfigDefault(path="self_trailing"), + node=ConfigDefault(path="self_trailing"), children=[ GroupDefault(group="group1", name="file1"), ConfigDefault(path="_self_"), @@ -158,10 +158,10 @@ def test_simple_defaults_tree_cases( "include_nested_group", ["+group1=file1"], DefaultsTreeNode( - parent=ConfigDefault(path="include_nested_group"), + node=ConfigDefault(path="include_nested_group"), children=[ DefaultsTreeNode( - parent=GroupDefault(group="group1", name="group_item"), + node=GroupDefault(group="group1", name="group_item1"), children=[GroupDefault(group="group2", name="file1")], ), GroupDefault(group="group1", name="file1"), @@ -177,5 +177,68 @@ def test_tree_with_append_override( expected: DefaultsTreeNode, ) -> None: _test_defaults_tree_impl( - config_name=config_name, overrides_list=overrides, expected=expected + config_name=config_name, input_overrides=overrides, expected=expected + ) + + +@mark.parametrize( + "config_name, overrides, expected", + [ + param( + "group_default", + ["group1=file2"], + DefaultsTreeNode( + node=ConfigDefault(path="group_default"), + children=[GroupDefault(group="group1", name="file2")], + ), + id="group_default:override", + ), + param( + "optional", + ["group1=file2"], + DefaultsTreeNode( + node=ConfigDefault(path="optional"), + children=[ + GroupDefault(group="group1", name="file2", optional=True), + ], + ), + id="optional:override", + ), + param( + "include_nested_group", + ["group1=group_item2"], + DefaultsTreeNode( + node=ConfigDefault(path="include_nested_group"), + children=[ + DefaultsTreeNode( + node=GroupDefault(group="group1", name="group_item2"), + children=[GroupDefault(group="group2", name="file2")], + ) + ], + ), + id="include_nested_group:override", + ), + param( + "include_nested_group", + ["group1/group2=file2"], + DefaultsTreeNode( + node=ConfigDefault(path="include_nested_group"), + children=[ + DefaultsTreeNode( + node=GroupDefault(group="group1", name="group_item1"), + children=[GroupDefault(group="group2", name="file2")], + ) + ], + ), + id="include_nested_group:override_nested", + ), + ], +) +def test_simple_group_override( + config_name: str, + overrides: List[str], + expected: DefaultsTreeNode, +) -> None: + _test_defaults_tree_impl( + config_name=config_name, input_overrides=overrides, expected=expected ) diff --git a/tests/test_data/default_lists/group1/file2.yaml b/tests/test_data/default_lists/group1/file2.yaml new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/test_data/default_lists/group1/group2/file2.yaml b/tests/test_data/default_lists/group1/group2/file2.yaml new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/test_data/default_lists/group1/group_item.yaml b/tests/test_data/default_lists/group1/group_item1.yaml similarity index 100% rename from tests/test_data/default_lists/group1/group_item.yaml rename to tests/test_data/default_lists/group1/group_item1.yaml diff --git a/tests/test_data/default_lists/group1/group_item2.yaml b/tests/test_data/default_lists/group1/group_item2.yaml new file mode 100644 index 00000000000..20067001d84 --- /dev/null +++ b/tests/test_data/default_lists/group1/group_item2.yaml @@ -0,0 +1,2 @@ +defaults: + - group2: file2 \ No newline at end of file diff --git a/tests/test_data/default_lists/include_nested_group.yaml b/tests/test_data/default_lists/include_nested_group.yaml index fbdf3398002..e67363dd0ea 100644 --- a/tests/test_data/default_lists/include_nested_group.yaml +++ b/tests/test_data/default_lists/include_nested_group.yaml @@ -1,2 +1,2 @@ defaults: - - group1: group_item \ No newline at end of file + - group1: group_item1 \ No newline at end of file From 06e52f240c999bc5e3d141eafb183af9ce7a6c00 Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Tue, 24 Nov 2020 18:40:47 -0800 Subject: [PATCH 09/91] tree_to_list dfs implementation --- hydra/_internal/new_defaults_list.py | 92 ++++++++++++++----- ...faultElement.py => new_default_element.py} | 31 ++++++- hydra/plugins/config_source.py | 2 +- tests/defaults_list/__init__.py | 2 +- tests/defaults_list/test_basic.py | 70 +++++++++++++- tests/defaults_list/test_defaults_tree.py | 53 ++++++++--- 6 files changed, 205 insertions(+), 45 deletions(-) rename hydra/core/{NewDefaultElement.py => new_default_element.py} (75%) diff --git a/hydra/_internal/new_defaults_list.py b/hydra/_internal/new_defaults_list.py index c1ad037e280..8631ad52c4c 100644 --- a/hydra/_internal/new_defaults_list.py +++ b/hydra/_internal/new_defaults_list.py @@ -1,28 +1,20 @@ import copy from dataclasses import dataclass from textwrap import dedent -from typing import Dict, List, Optional, Union +from typing import Dict, List from hydra import MissingConfigException from hydra._internal.config_repository import IConfigRepository -from hydra.core.NewDefaultElement import ConfigDefault, GroupDefault, InputDefault +from hydra.core.new_default_element import ( + ConfigDefault, + DefaultsTreeNode, + GroupDefault, + InputDefault, + ResultDefault, +) from hydra.core.object_type import ObjectType from hydra.core.override_parser.types import Override - - -@dataclass -class DefaultsTreeNode: - node: InputDefault - children: Optional[List[Union["DefaultsTreeNode", InputDefault]]] = None - - -@dataclass -class ResultDefault: - config_path: Optional[str] = None - parent: Optional[str] = None - addressing_key: Optional[str] = None - result_package: Optional[str] = None - is_self: bool = False +from hydra.errors import ConfigCompositionException @dataclass @@ -78,6 +70,21 @@ class DefaultsList: config_overrides: List[Override] +def _validate_self(containing_node: InputDefault, defaults: List[InputDefault]) -> None: + # check that self is present only once + has_self = False + for d in defaults: + if d.is_self(): + if has_self: + raise ConfigCompositionException( + f"Duplicate _self_ defined in {containing_node.get_config_path()}" + ) + has_self = True + + if not has_self: + defaults.insert(0, ConfigDefault(path="_self_")) + + def _create_defaults_tree( repo: IConfigRepository, root: DefaultsTreeNode, @@ -95,29 +102,29 @@ def _create_defaults_tree( override_name = overrides.get_choice_for(parent) parent.name = override_name parent.config_name_overridden = True - path = parent.get_config_path() - else: - assert isinstance(parent, ConfigDefault) - path = parent.path + + path = parent.get_config_path() loaded = repo.load_config(config_path=path, is_primary_config=is_primary_config) if loaded is None: missing_config_error(repo, root.node) else: - defaults_list = copy.deepcopy(loaded.new_defaults_list) if is_primary_config: for d in overrides.append_group_defaults: defaults_list.append(d) + if len(defaults_list) > 0: + _validate_self(containing_node=parent, defaults=defaults_list) + children = [] for d in defaults_list: if d.is_self(): d.parent_base_dir = root.node.parent_base_dir children.append(d) else: - new_root = DefaultsTreeNode(node=d) + new_root = DefaultsTreeNode(node=d, parent=root) d.parent_base_dir = parent.get_group_path() new_root.parent_base_dir = d.get_group_path() subtree = _create_defaults_tree( @@ -136,6 +143,40 @@ def _create_defaults_tree( return root +def _create_result_default(tree: DefaultsTreeNode, node: InputDefault) -> ResultDefault: + res = ResultDefault() + if node.is_self(): + res.config_path = tree.node.get_config_path() + res.is_self = True + pn = tree.parent_node() + cp = pn.get_config_path() if pn is not None else None + res.parent = cp + else: + res.config_path = node.get_config_path() + if tree is not None: + res.parent = tree.node.get_config_path() + return res + + +def _tree_to_list( + tree: DefaultsTreeNode, + output: List[ResultDefault], +): + node = tree.node + + if tree.children is None or len(tree.children) == 0: + rd = _create_result_default(tree=tree.parent, node=node) + output.append(rd) + else: + for child in tree.children: + if isinstance(child, InputDefault): + rd = _create_result_default(tree=tree, node=child) + output.append(rd) + else: + assert isinstance(child, DefaultsTreeNode) + _tree_to_list(tree=child, output=output) + + def _create_defaults_list( repo: IConfigRepository, config_name: str, @@ -149,9 +190,12 @@ def _create_defaults_list( overrides=overrides, is_primary_config=True, ) + + output = [] + _tree_to_list(tree=defaults_tree, output=output) # TODO: convert tree to list with DFS # TODO: fail if duplicate items exists - return [] + return output def create_defaults_list( diff --git a/hydra/core/NewDefaultElement.py b/hydra/core/new_default_element.py similarity index 75% rename from hydra/core/NewDefaultElement.py rename to hydra/core/new_default_element.py index 077025e7432..63efcdeb49e 100644 --- a/hydra/core/NewDefaultElement.py +++ b/hydra/core/new_default_element.py @@ -1,5 +1,14 @@ from dataclasses import dataclass, field -from typing import Optional +from typing import List, Optional, Union + + +@dataclass +class ResultDefault: + config_path: Optional[str] = None + parent: Optional[str] = None + # addressing_key: Optional[str] = None + # result_package: Optional[str] = None + is_self: bool = False @dataclass @@ -21,7 +30,7 @@ class ConfigDefault(InputDefault): parent_base_dir: Optional[str] = field(default=None, compare=False, repr=False) def is_self(self) -> bool: - return self.path == "_self_" or self.path.endswith("/_self_") + return self.path == "_self_" def get_group_path(self) -> str: assert self.parent_base_dir is not None @@ -78,3 +87,21 @@ def get_config_path(self) -> str: assert group_path != "" return f"{group_path}/{self.name}" + + +@dataclass +class DefaultsTreeNode: + node: InputDefault + children: Optional[List[Union["DefaultsTreeNode", InputDefault]]] = None + + parent: Optional["DefaultsTreeNode"] = field( + default=None, + repr=False, + compare=False, + ) + + def parent_node(self) -> Optional[InputDefault]: + if self.parent is None: + return None + else: + return self.parent.node diff --git a/hydra/plugins/config_source.py b/hydra/plugins/config_source.py index eb28dab5733..b79bdc5b7cf 100644 --- a/hydra/plugins/config_source.py +++ b/hydra/plugins/config_source.py @@ -12,7 +12,7 @@ from typing import List, Optional, Dict, Tuple, MutableSequence from hydra.core import DefaultElement -from hydra.core.NewDefaultElement import InputDefault, GroupDefault, ConfigDefault +from hydra.core.new_default_element import InputDefault, ConfigDefault, GroupDefault from hydra.errors import HydraException from omegaconf import ( Container, diff --git a/tests/defaults_list/__init__.py b/tests/defaults_list/__init__.py index ad5ed1847d1..e19da566e4d 100644 --- a/tests/defaults_list/__init__.py +++ b/tests/defaults_list/__init__.py @@ -8,7 +8,7 @@ _create_defaults_tree, Overrides, ) -from hydra.core.NewDefaultElement import ConfigDefault +from hydra.core.new_default_element import ConfigDefault from hydra.core.override_parser.overrides_parser import OverridesParser diff --git a/tests/defaults_list/test_basic.py b/tests/defaults_list/test_basic.py index 1a0e06a5901..eb73d0a1567 100644 --- a/tests/defaults_list/test_basic.py +++ b/tests/defaults_list/test_basic.py @@ -6,7 +6,7 @@ create_defaults_list, ResultDefault, ) -from hydra.core.NewDefaultElement import InputDefault, GroupDefault, ConfigDefault +from hydra.core.new_default_element import InputDefault, GroupDefault, ConfigDefault from hydra.core.override_parser.overrides_parser import OverridesParser from hydra.core.plugins import Plugins from hydra.test_utils.test_utils import chdir_hydra_root @@ -19,10 +19,12 @@ # TODO: -# - test with simple config group overrides +# - (Y) test with simple config group overrides # - test with config group overrides overriding config groups @pkg # - test handling missing configs mentioned in defaults list (with and without optional) # - test overriding configs in absolute location +# - test duplicate _self_ error +# - test duplicates in result config list @mark.parametrize( @@ -84,10 +86,11 @@ def _test_defaults_list_impl( ) -> None: parser = OverridesParser.create() repo = create_repo() + overrides_list = parser.parse_overrides(overrides=overrides) result = create_defaults_list( repo=repo, config_name=config_name, - overrides_list=parser.parse_overrides(overrides=overrides), + overrides_list=overrides_list, ) assert result.defaults == expected @@ -96,10 +99,67 @@ def _test_defaults_list_impl( @mark.parametrize( "config_name, overrides, expected", [ - param("empty", [], [], id="empty"), + param( + "empty", + [], + [ResultDefault(config_path="empty")], + id="empty", + ), + param( + "config_default", + [], + [ + ResultDefault(config_path="config_default", is_self=True), + ResultDefault(config_path="empty", parent="config_default"), + ], + id="config_default", + ), + param( + "group_default", + [], + [ + ResultDefault(config_path="group_default", is_self=True), + ResultDefault(config_path="group1/file1", parent="group_default"), + ], + id="group_default", + ), + param( + "self_leading", + [], + [ + ResultDefault(config_path="self_leading", is_self=True), + ResultDefault(config_path="group1/file1", parent="self_leading"), + ], + id="self_leading", + ), + param( + "self_trailing", + [], + [ + ResultDefault(config_path="group1/file1", parent="self_trailing"), + ResultDefault(config_path="self_trailing", is_self=True), + ], + id="self_trailing", + ), + param( + "include_nested_group", + [], + [ + ResultDefault(config_path="include_nested_group", is_self=True), + ResultDefault( + config_path="group1/group_item1", + parent="include_nested_group", + is_self=True, + ), + ResultDefault( + config_path="group1/group2/file1", parent="group1/group_item1" + ), + ], + id="include_nested_group", + ), ], ) -def test_simple_cases( +def test_simple_defaults_list_cases( config_name: str, overrides: List[str], expected: List[ResultDefault], diff --git a/tests/defaults_list/test_defaults_tree.py b/tests/defaults_list/test_defaults_tree.py index cd4809376af..f9f89b0bf46 100644 --- a/tests/defaults_list/test_defaults_tree.py +++ b/tests/defaults_list/test_defaults_tree.py @@ -5,7 +5,7 @@ from hydra._internal.new_defaults_list import ( DefaultsTreeNode, ) -from hydra.core.NewDefaultElement import GroupDefault, ConfigDefault +from hydra.core.new_default_element import GroupDefault, ConfigDefault from hydra.core.plugins import Plugins from hydra.test_utils.test_utils import chdir_hydra_root from tests.defaults_list import _test_defaults_tree_impl @@ -30,7 +30,7 @@ [], DefaultsTreeNode( node=ConfigDefault(path="config_default"), - children=[ConfigDefault(path="empty")], + children=[ConfigDefault(path="_self_"), ConfigDefault(path="empty")], ), id="config_default", ), @@ -39,7 +39,10 @@ [], DefaultsTreeNode( node=ConfigDefault(path="group_default"), - children=[GroupDefault(group="group1", name="file1")], + children=[ + ConfigDefault(path="_self_"), + GroupDefault(group="group1", name="file1"), + ], ), id="group_default", ), @@ -49,6 +52,7 @@ DefaultsTreeNode( node=ConfigDefault(path="optional"), children=[ + ConfigDefault(path="_self_"), GroupDefault(group="group1", name="file1", optional=True), ], ), @@ -84,10 +88,14 @@ DefaultsTreeNode( node=ConfigDefault(path="include_nested_group"), children=[ + ConfigDefault(path="_self_"), DefaultsTreeNode( node=GroupDefault(group="group1", name="group_item1"), - children=[GroupDefault(group="group2", name="file1")], - ) + children=[ + ConfigDefault(path="_self_"), + GroupDefault(group="group2", name="file1"), + ], + ), ], ), id="include_nested_group", @@ -112,7 +120,10 @@ def test_simple_defaults_tree_cases( ["+group1=file1"], DefaultsTreeNode( node=ConfigDefault(path="empty"), - children=[GroupDefault(group="group1", name="file1")], + children=[ + ConfigDefault(path="_self_"), + GroupDefault(group="group1", name="file1"), + ], ), id="empty:append", ), @@ -122,6 +133,7 @@ def test_simple_defaults_tree_cases( DefaultsTreeNode( node=ConfigDefault(path="config_default"), children=[ + ConfigDefault(path="_self_"), ConfigDefault(path="empty"), GroupDefault(group="group1", name="file1"), ], @@ -134,6 +146,7 @@ def test_simple_defaults_tree_cases( DefaultsTreeNode( node=ConfigDefault(path="group_default"), children=[ + ConfigDefault(path="_self_"), # config tree allow for duplicate items GroupDefault(group="group1", name="file1"), GroupDefault(group="group1", name="file1"), @@ -160,9 +173,13 @@ def test_simple_defaults_tree_cases( DefaultsTreeNode( node=ConfigDefault(path="include_nested_group"), children=[ + ConfigDefault(path="_self_"), DefaultsTreeNode( node=GroupDefault(group="group1", name="group_item1"), - children=[GroupDefault(group="group2", name="file1")], + children=[ + ConfigDefault(path="_self_"), + GroupDefault(group="group2", name="file1"), + ], ), GroupDefault(group="group1", name="file1"), ], @@ -189,7 +206,10 @@ def test_tree_with_append_override( ["group1=file2"], DefaultsTreeNode( node=ConfigDefault(path="group_default"), - children=[GroupDefault(group="group1", name="file2")], + children=[ + ConfigDefault(path="_self_"), + GroupDefault(group="group1", name="file2"), + ], ), id="group_default:override", ), @@ -199,6 +219,7 @@ def test_tree_with_append_override( DefaultsTreeNode( node=ConfigDefault(path="optional"), children=[ + ConfigDefault(path="_self_"), GroupDefault(group="group1", name="file2", optional=True), ], ), @@ -210,10 +231,14 @@ def test_tree_with_append_override( DefaultsTreeNode( node=ConfigDefault(path="include_nested_group"), children=[ + ConfigDefault(path="_self_"), DefaultsTreeNode( node=GroupDefault(group="group1", name="group_item2"), - children=[GroupDefault(group="group2", name="file2")], - ) + children=[ + ConfigDefault(path="_self_"), + GroupDefault(group="group2", name="file2"), + ], + ), ], ), id="include_nested_group:override", @@ -224,10 +249,14 @@ def test_tree_with_append_override( DefaultsTreeNode( node=ConfigDefault(path="include_nested_group"), children=[ + ConfigDefault(path="_self_"), DefaultsTreeNode( node=GroupDefault(group="group1", name="group_item1"), - children=[GroupDefault(group="group2", name="file2")], - ) + children=[ + ConfigDefault(path="_self_"), + GroupDefault(group="group2", name="file2"), + ], + ), ], ), id="include_nested_group:override_nested", From 7b245fae307b5089f351223815b7eef80b34b7ca Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Wed, 25 Nov 2020 15:58:36 -0800 Subject: [PATCH 10/91] Computing package in result defaults list --- hydra/_internal/new_defaults_list.py | 12 +- hydra/core/new_default_element.py | 54 +++++++- tests/defaults_list/test_basic.py | 151 +++++++++++++++++++--- tests/defaults_list/test_defaults_tree.py | 15 +++ 4 files changed, 211 insertions(+), 21 deletions(-) diff --git a/hydra/_internal/new_defaults_list.py b/hydra/_internal/new_defaults_list.py index 8631ad52c4c..57d2c815fbf 100644 --- a/hydra/_internal/new_defaults_list.py +++ b/hydra/_internal/new_defaults_list.py @@ -95,6 +95,7 @@ def _create_defaults_tree( if is_primary_config: root.node.parent_base_dir = "" + root.node.parent_package = "" parent = root.node if isinstance(parent, GroupDefault): @@ -122,10 +123,12 @@ def _create_defaults_tree( for d in defaults_list: if d.is_self(): d.parent_base_dir = root.node.parent_base_dir + d.parent_package = root.node.parent_package children.append(d) else: new_root = DefaultsTreeNode(node=d, parent=root) d.parent_base_dir = parent.get_group_path() + d.parent_package = parent.get_final_package() new_root.parent_base_dir = d.get_group_path() subtree = _create_defaults_tree( repo=repo, @@ -149,12 +152,17 @@ def _create_result_default(tree: DefaultsTreeNode, node: InputDefault) -> Result res.config_path = tree.node.get_config_path() res.is_self = True pn = tree.parent_node() - cp = pn.get_config_path() if pn is not None else None - res.parent = cp + if pn is not None: + cp = pn.get_config_path() + res.parent = cp + else: + res.parent = None + res.package = tree.node.get_final_package() else: res.config_path = node.get_config_path() if tree is not None: res.parent = tree.node.get_config_path() + res.package = node.get_final_package() return res diff --git a/hydra/core/new_default_element.py b/hydra/core/new_default_element.py index 63efcdeb49e..7f9c75b1b5e 100644 --- a/hydra/core/new_default_element.py +++ b/hydra/core/new_default_element.py @@ -7,7 +7,7 @@ class ResultDefault: config_path: Optional[str] = None parent: Optional[str] = None # addressing_key: Optional[str] = None - # result_package: Optional[str] = None + package: Optional[str] = None is_self: bool = False @@ -22,12 +22,48 @@ def get_group_path(self) -> str: def get_config_path(self) -> str: raise NotImplementedError() + def get_default_package(self) -> str: + return self.get_group_path().replace("/", ".") + + # def get_config_package_in_header(self) -> str: + # raise NotImplementedError() + + def get_final_package(self) -> str: + raise NotImplementedError() + + def _relative_group_path(self) -> str: + raise NotImplementedError() + + def _get_final_package( + self, + parent_package: Optional[str], + package: Optional[str], + ) -> str: + assert parent_package is not None + if package is None: + package = self._relative_group_path().replace("/", ".") + + if parent_package == "": + ret = package + else: + if package == "": + ret = parent_package + else: + ret = f"{parent_package}.{package}" + + lgi = ret.rfind("_global_") + if lgi == -1: + return ret + else: + return ret[lgi + len("_global_") + 1 :] + @dataclass class ConfigDefault(InputDefault): path: str package: Optional[str] = None parent_base_dir: Optional[str] = field(default=None, compare=False, repr=False) + parent_package: Optional[str] = field(default=None, compare=False, repr=False) def is_self(self) -> bool: return self.path == "_self_" @@ -55,6 +91,16 @@ def get_config_path(self) -> str: else: return f"{self.parent_base_dir}/{self.path}" + def get_final_package(self) -> str: + return self._get_final_package(self.parent_package, self.package) + + def _relative_group_path(self) -> str: + idx = self.path.rfind("/") + if idx == -1: + return "" + else: + return self.path[0:idx] + @dataclass class GroupDefault(InputDefault): @@ -88,6 +134,12 @@ def get_config_path(self) -> str: return f"{group_path}/{self.name}" + def get_final_package(self) -> str: + return self._get_final_package(self.parent_package, self.package) + + def _relative_group_path(self) -> str: + return self.group + @dataclass class DefaultsTreeNode: diff --git a/tests/defaults_list/test_basic.py b/tests/defaults_list/test_basic.py index eb73d0a1567..3dcadec129f 100644 --- a/tests/defaults_list/test_basic.py +++ b/tests/defaults_list/test_basic.py @@ -18,13 +18,17 @@ Plugins.instance() -# TODO: -# - (Y) test with simple config group overrides -# - test with config group overrides overriding config groups @pkg -# - test handling missing configs mentioned in defaults list (with and without optional) -# - test overriding configs in absolute location -# - test duplicate _self_ error -# - test duplicates in result config list +# TODO: (Y) Test with simple config group overrides +# TODO: (Y) Test computed package when there are no package overrides in package header +# TODO: test with config group overrides overriding config groups @pkg +# TODO: test with config header package override +# TODO: test with both config header and defaults list pkg override +# TODO: handle hydra overrides +# TODO: test handling missing configs mentioned in defaults list (with and without optional) +# TODO: test overriding configs in absolute location +# TODO: test duplicate _self_ error +# TODO: test duplicates in result config list +# TODO: Interpolation support @mark.parametrize( @@ -102,15 +106,15 @@ def _test_defaults_list_impl( param( "empty", [], - [ResultDefault(config_path="empty")], + [ResultDefault(config_path="empty", package="")], id="empty", ), param( "config_default", [], [ - ResultDefault(config_path="config_default", is_self=True), - ResultDefault(config_path="empty", parent="config_default"), + ResultDefault(config_path="config_default", package="", is_self=True), + ResultDefault(config_path="empty", package="", parent="config_default"), ], id="config_default", ), @@ -118,8 +122,10 @@ def _test_defaults_list_impl( "group_default", [], [ - ResultDefault(config_path="group_default", is_self=True), - ResultDefault(config_path="group1/file1", parent="group_default"), + ResultDefault(config_path="group_default", package="", is_self=True), + ResultDefault( + config_path="group1/file1", package="group1", parent="group_default" + ), ], id="group_default", ), @@ -127,8 +133,10 @@ def _test_defaults_list_impl( "self_leading", [], [ - ResultDefault(config_path="self_leading", is_self=True), - ResultDefault(config_path="group1/file1", parent="self_leading"), + ResultDefault(config_path="self_leading", package="", is_self=True), + ResultDefault( + config_path="group1/file1", package="group1", parent="self_leading" + ), ], id="self_leading", ), @@ -136,8 +144,10 @@ def _test_defaults_list_impl( "self_trailing", [], [ - ResultDefault(config_path="group1/file1", parent="self_trailing"), - ResultDefault(config_path="self_trailing", is_self=True), + ResultDefault( + config_path="group1/file1", package="group1", parent="self_trailing" + ), + ResultDefault(config_path="self_trailing", package="", is_self=True), ], id="self_trailing", ), @@ -145,14 +155,19 @@ def _test_defaults_list_impl( "include_nested_group", [], [ - ResultDefault(config_path="include_nested_group", is_self=True), + ResultDefault( + config_path="include_nested_group", package="", is_self=True + ), ResultDefault( config_path="group1/group_item1", parent="include_nested_group", + package="group1", is_self=True, ), ResultDefault( - config_path="group1/group2/file1", parent="group1/group_item1" + config_path="group1/group2/file1", + package="group1.group2", + parent="group1/group_item1", ), ], id="include_nested_group", @@ -203,3 +218,103 @@ def test_get_paths( ) -> None: assert default.get_group_path() == expected_group_path assert default.get_config_path() == expected_config_path + + +@mark.parametrize( + "default,expected", + [ + param( + ConfigDefault(path="bar", parent_base_dir=""), + "", + id="config_default", + ), + param( + ConfigDefault(path="foo/bar", parent_base_dir=""), + "foo", + id="config_default", + ), + param( + ConfigDefault(path="bar", parent_base_dir="foo"), + "foo", + id="config_default", + ), + param( + ConfigDefault(path="bar", parent_base_dir="a/b"), + "a.b", + id="config_default", + ), + param( + GroupDefault(group="a", name="a1", parent_base_dir=""), + "a", + id="group_default", + ), + param( + GroupDefault(group="a/b", name="a1", parent_base_dir=""), + "a.b", + id="group_default", + ), + param( + GroupDefault(group="a/b", name="a1", parent_base_dir="x"), + "x.a.b", + id="group_default", + ), + ], +) +def test_get_default_package(default: InputDefault, expected) -> None: + assert default.get_default_package() == expected + + +@mark.parametrize( + "default,expected", + [ + # empty parent package + param( + ConfigDefault(path="bar", parent_package=""), + "", + id="config_default:path=bar,parent_package=,package=", + ), + param( + ConfigDefault(path="group1/bar", parent_package=""), + "group1", + id="config_default:path=group1/bar,parent_package=, package=", + ), + param( + ConfigDefault(path="bar", parent_package="", package="pkg1"), + "pkg1", + id="config_default:path=bar,parent_package=, package=pkg1", + ), + param( + ConfigDefault(path="group1/bar", parent_package="", package="pkg1"), + "pkg1", + id="config_default:path=group1/bar,parent_package=,package=pkg1", + ), + # non empty parent package + param( + ConfigDefault(path="bar", parent_package="a", package="pkg1"), + "a.pkg1", + id="config_default:path=bar,parent_package=a, package=pkg1", + ), + # global package + param( + ConfigDefault(path="bar", parent_package="a", package="_global_.pkg1"), + "pkg1", + id="config_default:parent_package=a, package=_global_.pkg1", + ), + # global parent package + param( + ConfigDefault(path="bar", parent_package="_global_.foo", package="pkg1"), + "foo.pkg1", + id="config_default:parent_package=_global_.foo, package=pkg1", + ), + # both globals + param( + ConfigDefault( + path="bar", parent_package="_global_.foo", package="_global_.pkg1" + ), + "pkg1", + id="config_default:parent_package=_global_.foo, package=_global_.pkg1", + ), + ], +) +def test_get_final_package(default: InputDefault, expected) -> None: + assert default.get_final_package() == expected diff --git a/tests/defaults_list/test_defaults_tree.py b/tests/defaults_list/test_defaults_tree.py index f9f89b0bf46..4ec081f66bd 100644 --- a/tests/defaults_list/test_defaults_tree.py +++ b/tests/defaults_list/test_defaults_tree.py @@ -112,6 +112,21 @@ def test_simple_defaults_tree_cases( ) +# TODO: Test cases where the default lists has package overrides +# @mark.parametrize( +# "config_name, overrides, expected", +# [], +# ) +# def test_defaults_tree_with_package( +# config_name: str, +# overrides: List[str], +# expected: DefaultsTreeNode, +# ) -> None: +# _test_defaults_tree_impl( +# config_name=config_name, input_overrides=overrides, expected=expected +# ) + + @mark.parametrize( "config_name, overrides, expected", [ From b064f250981ebaf5851535e315584a9d6e97910e Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Thu, 26 Nov 2020 12:13:31 -0800 Subject: [PATCH 11/91] testing defaults tree with pacakge override in defaults list --- hydra/_internal/new_defaults_list.py | 3 +- tests/defaults_list/test_defaults_tree.py | 120 ++++++++++++++++-- .../default_lists/config_default_pkg1.yaml | 2 + .../group1/config_item_pkg2.yaml | 2 + .../group1/group_item1_pkg2.yaml | 2 + .../default_lists/group_default_pkg1.yaml | 2 + .../include_nested_config_item.yaml | 2 + .../include_nested_config_item_pkg2.yaml | 2 + .../include_nested_group_pkg2.yaml | 2 + .../default_lists/self_trailing_pkg1.yaml | 3 + 10 files changed, 126 insertions(+), 14 deletions(-) create mode 100644 tests/test_data/default_lists/config_default_pkg1.yaml create mode 100644 tests/test_data/default_lists/group1/config_item_pkg2.yaml create mode 100644 tests/test_data/default_lists/group1/group_item1_pkg2.yaml create mode 100644 tests/test_data/default_lists/group_default_pkg1.yaml create mode 100644 tests/test_data/default_lists/include_nested_config_item.yaml create mode 100644 tests/test_data/default_lists/include_nested_config_item_pkg2.yaml create mode 100644 tests/test_data/default_lists/include_nested_group_pkg2.yaml create mode 100644 tests/test_data/default_lists/self_trailing_pkg1.yaml diff --git a/hydra/_internal/new_defaults_list.py b/hydra/_internal/new_defaults_list.py index 57d2c815fbf..cfd9d998621 100644 --- a/hydra/_internal/new_defaults_list.py +++ b/hydra/_internal/new_defaults_list.py @@ -217,9 +217,10 @@ def create_defaults_list( return ret +# TODO: show parent config name in the error (where is my error?) def missing_config_error(repo: IConfigRepository, element: InputDefault) -> None: options = None - if isinstance(element, GroupDefault) is not None: + if isinstance(element, GroupDefault): options = repo.get_group_options(element.group, ObjectType.CONFIG) opt_list = "\n".join(["\t" + x for x in options]) msg = ( diff --git a/tests/defaults_list/test_defaults_tree.py b/tests/defaults_list/test_defaults_tree.py index 4ec081f66bd..32d89a6972e 100644 --- a/tests/defaults_list/test_defaults_tree.py +++ b/tests/defaults_list/test_defaults_tree.py @@ -15,6 +15,10 @@ # registers config source plugins Plugins.instance() +# TODO: test inclusion of nested config item (not group) +# TODO: Test cases where the default lists has package overrides +# defaults list packages to test : _global_, _global_.foo, _name_ + @mark.parametrize( "config_name, overrides, expected", @@ -100,6 +104,24 @@ ), id="include_nested_group", ), + param( + "include_nested_config_item", + [], + DefaultsTreeNode( + node=ConfigDefault(path="include_nested_config_item"), + children=[ + ConfigDefault(path="_self_"), + DefaultsTreeNode( + node=GroupDefault(group="group1", name="config_item"), + children=[ + ConfigDefault(path="_self_"), + ConfigDefault(path="group2/file1"), + ], + ), + ], + ), + id="include_nested_config_item", + ), ], ) def test_simple_defaults_tree_cases( @@ -112,19 +134,91 @@ def test_simple_defaults_tree_cases( ) -# TODO: Test cases where the default lists has package overrides -# @mark.parametrize( -# "config_name, overrides, expected", -# [], -# ) -# def test_defaults_tree_with_package( -# config_name: str, -# overrides: List[str], -# expected: DefaultsTreeNode, -# ) -> None: -# _test_defaults_tree_impl( -# config_name=config_name, input_overrides=overrides, expected=expected -# ) +@mark.parametrize( + "config_name, overrides, expected", + [ + param( + "config_default_pkg1", + [], + DefaultsTreeNode( + node=ConfigDefault(path="config_default_pkg1"), + children=[ + ConfigDefault(path="_self_"), + ConfigDefault(path="empty", package="pkg1"), + ], + ), + id="config_default_pkg1", + ), + param( + "group_default_pkg1", + [], + DefaultsTreeNode( + node=ConfigDefault(path="group_default_pkg1"), + children=[ + ConfigDefault(path="_self_"), + GroupDefault(group="group1", name="file1", package="pkg1"), + ], + ), + id="group_default_pkg1", + ), + param( + "self_trailing_pkg1", + [], + DefaultsTreeNode( + node=ConfigDefault(path="self_trailing_pkg1"), + children=[ + GroupDefault(group="group1", name="file1"), + ConfigDefault(path="_self_", package="pkg1"), + ], + ), + id="self_trailing_pkg1", + ), + param( + "include_nested_group_pkg2", + [], + DefaultsTreeNode( + node=ConfigDefault(path="include_nested_group_pkg2"), + children=[ + ConfigDefault(path="_self_"), + DefaultsTreeNode( + node=GroupDefault(group="group1", name="group_item1_pkg2"), + children=[ + ConfigDefault(path="_self_"), + GroupDefault(group="group2", name="file1", package="pkg2"), + ], + ), + ], + ), + id="include_nested_group_pkg2", + ), + param( + "include_nested_config_item_pkg2", + [], + DefaultsTreeNode( + node=ConfigDefault(path="include_nested_config_item_pkg2"), + children=[ + ConfigDefault(path="_self_"), + DefaultsTreeNode( + node=GroupDefault(group="group1", name="config_item_pkg2"), + children=[ + ConfigDefault(path="_self_"), + ConfigDefault(path="group2/file1", package="pkg2"), + ], + ), + ], + ), + id="include_nested_config_item_pkg2", + ), + ], +) +def test_defaults_tree_with_package_overrides( + config_name: str, + overrides: List[str], + expected: DefaultsTreeNode, +) -> None: + _test_defaults_tree_impl( + config_name=config_name, input_overrides=overrides, expected=expected + ) @mark.parametrize( diff --git a/tests/test_data/default_lists/config_default_pkg1.yaml b/tests/test_data/default_lists/config_default_pkg1.yaml new file mode 100644 index 00000000000..3d41e6c4e32 --- /dev/null +++ b/tests/test_data/default_lists/config_default_pkg1.yaml @@ -0,0 +1,2 @@ +defaults: + - empty@pkg1 diff --git a/tests/test_data/default_lists/group1/config_item_pkg2.yaml b/tests/test_data/default_lists/group1/config_item_pkg2.yaml new file mode 100644 index 00000000000..0635eddbd55 --- /dev/null +++ b/tests/test_data/default_lists/group1/config_item_pkg2.yaml @@ -0,0 +1,2 @@ +defaults: + - group2/file1@pkg2 \ No newline at end of file diff --git a/tests/test_data/default_lists/group1/group_item1_pkg2.yaml b/tests/test_data/default_lists/group1/group_item1_pkg2.yaml new file mode 100644 index 00000000000..621e89d6615 --- /dev/null +++ b/tests/test_data/default_lists/group1/group_item1_pkg2.yaml @@ -0,0 +1,2 @@ +defaults: + - group2@pkg2: file1 \ No newline at end of file diff --git a/tests/test_data/default_lists/group_default_pkg1.yaml b/tests/test_data/default_lists/group_default_pkg1.yaml new file mode 100644 index 00000000000..1f2fb2a24b0 --- /dev/null +++ b/tests/test_data/default_lists/group_default_pkg1.yaml @@ -0,0 +1,2 @@ +defaults: + - group1@pkg1: file1 \ No newline at end of file diff --git a/tests/test_data/default_lists/include_nested_config_item.yaml b/tests/test_data/default_lists/include_nested_config_item.yaml new file mode 100644 index 00000000000..c9cc4c53766 --- /dev/null +++ b/tests/test_data/default_lists/include_nested_config_item.yaml @@ -0,0 +1,2 @@ +defaults: + - group1: config_item \ No newline at end of file diff --git a/tests/test_data/default_lists/include_nested_config_item_pkg2.yaml b/tests/test_data/default_lists/include_nested_config_item_pkg2.yaml new file mode 100644 index 00000000000..c45e4ef8185 --- /dev/null +++ b/tests/test_data/default_lists/include_nested_config_item_pkg2.yaml @@ -0,0 +1,2 @@ +defaults: + - group1: config_item_pkg2 \ No newline at end of file diff --git a/tests/test_data/default_lists/include_nested_group_pkg2.yaml b/tests/test_data/default_lists/include_nested_group_pkg2.yaml new file mode 100644 index 00000000000..0a733da8efa --- /dev/null +++ b/tests/test_data/default_lists/include_nested_group_pkg2.yaml @@ -0,0 +1,2 @@ +defaults: + - group1: group_item1_pkg2 \ No newline at end of file diff --git a/tests/test_data/default_lists/self_trailing_pkg1.yaml b/tests/test_data/default_lists/self_trailing_pkg1.yaml new file mode 100644 index 00000000000..54e27a9172c --- /dev/null +++ b/tests/test_data/default_lists/self_trailing_pkg1.yaml @@ -0,0 +1,3 @@ +defaults: + - group1: file1 + - _self_@pkg1 From d8909d292dfdaefb943535fa146b2d74266d6e0a Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Thu, 26 Nov 2020 12:51:47 -0800 Subject: [PATCH 12/91] testing result list for default-list package override cases --- hydra/core/new_default_element.py | 4 + tests/defaults_list/__init__.py | 27 +- tests/defaults_list/test_basic.py | 255 ++++++++++++------ tests/defaults_list/test_defaults_tree.py | 189 +++++++------ ...railing_pkg1.yaml => error_self_pkg1.yaml} | 1 - 5 files changed, 293 insertions(+), 183 deletions(-) rename tests/test_data/default_lists/{self_trailing_pkg1.yaml => error_self_pkg1.yaml} (59%) diff --git a/hydra/core/new_default_element.py b/hydra/core/new_default_element.py index 7f9c75b1b5e..621fcb6fa93 100644 --- a/hydra/core/new_default_element.py +++ b/hydra/core/new_default_element.py @@ -65,6 +65,10 @@ class ConfigDefault(InputDefault): parent_base_dir: Optional[str] = field(default=None, compare=False, repr=False) parent_package: Optional[str] = field(default=None, compare=False, repr=False) + def __post_init__(self): + if self.is_self() and self.package is not None: + raise ValueError("_self_@PACKAGE is not supported") + def is_self(self) -> bool: return self.path == "_self_" diff --git a/tests/defaults_list/__init__.py b/tests/defaults_list/__init__.py index e19da566e4d..680673a2ce3 100644 --- a/tests/defaults_list/__init__.py +++ b/tests/defaults_list/__init__.py @@ -1,5 +1,5 @@ # Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved -from typing import List +from typing import List, Any from hydra._internal.config_repository import IConfigRepository, ConfigRepository from hydra._internal.config_search_path_impl import ConfigSearchPathImpl @@ -21,7 +21,7 @@ def create_repo() -> IConfigRepository: def _test_defaults_tree_impl( config_name: str, input_overrides: List[str], - expected: DefaultsTreeNode, + expected: Any, ) -> None: parser = OverridesParser.create() repo = create_repo() @@ -29,11 +29,20 @@ def _test_defaults_tree_impl( root = DefaultsTreeNode(node=parent) overrides_list = parser.parse_overrides(overrides=input_overrides) overrides = Overrides(repo=repo, overrides_list=overrides_list) - result = _create_defaults_tree( - repo=repo, - root=root, - overrides=overrides, - is_primary_config=True, - ) - assert result == expected + if isinstance(expected, DefaultsTreeNode): + result = _create_defaults_tree( + repo=repo, + root=root, + overrides=overrides, + is_primary_config=True, + ) + assert result == expected + else: + with expected: + _create_defaults_tree( + repo=repo, + root=root, + overrides=overrides, + is_primary_config=True, + ) diff --git a/tests/defaults_list/test_basic.py b/tests/defaults_list/test_basic.py index 3dcadec129f..45410b49c00 100644 --- a/tests/defaults_list/test_basic.py +++ b/tests/defaults_list/test_basic.py @@ -100,90 +100,6 @@ def _test_defaults_list_impl( assert result.defaults == expected -@mark.parametrize( - "config_name, overrides, expected", - [ - param( - "empty", - [], - [ResultDefault(config_path="empty", package="")], - id="empty", - ), - param( - "config_default", - [], - [ - ResultDefault(config_path="config_default", package="", is_self=True), - ResultDefault(config_path="empty", package="", parent="config_default"), - ], - id="config_default", - ), - param( - "group_default", - [], - [ - ResultDefault(config_path="group_default", package="", is_self=True), - ResultDefault( - config_path="group1/file1", package="group1", parent="group_default" - ), - ], - id="group_default", - ), - param( - "self_leading", - [], - [ - ResultDefault(config_path="self_leading", package="", is_self=True), - ResultDefault( - config_path="group1/file1", package="group1", parent="self_leading" - ), - ], - id="self_leading", - ), - param( - "self_trailing", - [], - [ - ResultDefault( - config_path="group1/file1", package="group1", parent="self_trailing" - ), - ResultDefault(config_path="self_trailing", package="", is_self=True), - ], - id="self_trailing", - ), - param( - "include_nested_group", - [], - [ - ResultDefault( - config_path="include_nested_group", package="", is_self=True - ), - ResultDefault( - config_path="group1/group_item1", - parent="include_nested_group", - package="group1", - is_self=True, - ), - ResultDefault( - config_path="group1/group2/file1", - package="group1.group2", - parent="group1/group_item1", - ), - ], - id="include_nested_group", - ), - ], -) -def test_simple_defaults_list_cases( - config_name: str, - overrides: List[str], - expected: List[ResultDefault], -) -> None: - _test_defaults_list_impl( - config_name=config_name, overrides=overrides, expected=expected - ) - - @mark.parametrize( "default,expected_group_path,expected_config_path", [ @@ -318,3 +234,174 @@ def test_get_default_package(default: InputDefault, expected) -> None: ) def test_get_final_package(default: InputDefault, expected) -> None: assert default.get_final_package() == expected + + +@mark.parametrize( + "config_name, overrides, expected", + [ + param( + "empty", + [], + [ResultDefault(config_path="empty", package="")], + id="empty", + ), + param( + "config_default", + [], + [ + ResultDefault(config_path="config_default", package="", is_self=True), + ResultDefault(config_path="empty", package="", parent="config_default"), + ], + id="config_default", + ), + param( + "group_default", + [], + [ + ResultDefault(config_path="group_default", package="", is_self=True), + ResultDefault( + config_path="group1/file1", package="group1", parent="group_default" + ), + ], + id="group_default", + ), + param( + "self_leading", + [], + [ + ResultDefault(config_path="self_leading", package="", is_self=True), + ResultDefault( + config_path="group1/file1", package="group1", parent="self_leading" + ), + ], + id="self_leading", + ), + param( + "self_trailing", + [], + [ + ResultDefault( + config_path="group1/file1", package="group1", parent="self_trailing" + ), + ResultDefault(config_path="self_trailing", package="", is_self=True), + ], + id="self_trailing", + ), + param( + "include_nested_group", + [], + [ + ResultDefault( + config_path="include_nested_group", package="", is_self=True + ), + ResultDefault( + config_path="group1/group_item1", + parent="include_nested_group", + package="group1", + is_self=True, + ), + ResultDefault( + config_path="group1/group2/file1", + package="group1.group2", + parent="group1/group_item1", + ), + ], + id="include_nested_group", + ), + ], +) +def test_simple_defaults_list_cases( + config_name: str, + overrides: List[str], + expected: List[ResultDefault], +) -> None: + _test_defaults_list_impl( + config_name=config_name, overrides=overrides, expected=expected + ) + + +@mark.parametrize( + "config_name, overrides, expected", + [ + param( + "config_default_pkg1", + [], + [ + ResultDefault( + config_path="config_default_pkg1", package="", is_self=True + ), + ResultDefault( + config_path="empty", package="pkg1", parent="config_default_pkg1" + ), + ], + id="config_default_pkg1", + ), + param( + "group_default_pkg1", + [], + [ + ResultDefault( + config_path="group_default_pkg1", package="", is_self=True + ), + ResultDefault( + config_path="group1/file1", + package="pkg1", + parent="group_default_pkg1", + ), + ], + id="group_default_pkg1", + ), + param( + "include_nested_group_pkg2", + [], + [ + ResultDefault( + config_path="include_nested_group_pkg2", package="", is_self=True + ), + ResultDefault( + config_path="group1/group_item1_pkg2", + parent="include_nested_group_pkg2", + package="group1", + is_self=True, + ), + ResultDefault( + config_path="group1/group2/file1", + package="group1.pkg2", + parent="group1/group_item1_pkg2", + ), + ], + id="include_nested_group_pkg2", + ), + param( + "include_nested_config_item_pkg2", + [], + [ + ResultDefault( + config_path="include_nested_config_item_pkg2", + package="", + is_self=True, + ), + ResultDefault( + config_path="group1/config_item_pkg2", + parent="include_nested_config_item_pkg2", + package="group1", + is_self=True, + ), + ResultDefault( + config_path="group1/group2/file1", + package="group1.pkg2", + parent="group1/config_item_pkg2", + ), + ], + id="include_nested_config_item_pkg2", + ), + ], +) +def test_override_package_in_defaults_list( + config_name: str, + overrides: List[str], + expected: List[ResultDefault], +) -> None: + _test_defaults_list_impl( + config_name=config_name, overrides=overrides, expected=expected + ) diff --git a/tests/defaults_list/test_defaults_tree.py b/tests/defaults_list/test_defaults_tree.py index 32d89a6972e..6ae8b2d5dad 100644 --- a/tests/defaults_list/test_defaults_tree.py +++ b/tests/defaults_list/test_defaults_tree.py @@ -1,6 +1,8 @@ # Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved -from pytest import mark, param -from typing import List +import re + +from pytest import mark, param, raises +from typing import List, Any from hydra._internal.new_defaults_list import ( DefaultsTreeNode, @@ -134,93 +136,6 @@ def test_simple_defaults_tree_cases( ) -@mark.parametrize( - "config_name, overrides, expected", - [ - param( - "config_default_pkg1", - [], - DefaultsTreeNode( - node=ConfigDefault(path="config_default_pkg1"), - children=[ - ConfigDefault(path="_self_"), - ConfigDefault(path="empty", package="pkg1"), - ], - ), - id="config_default_pkg1", - ), - param( - "group_default_pkg1", - [], - DefaultsTreeNode( - node=ConfigDefault(path="group_default_pkg1"), - children=[ - ConfigDefault(path="_self_"), - GroupDefault(group="group1", name="file1", package="pkg1"), - ], - ), - id="group_default_pkg1", - ), - param( - "self_trailing_pkg1", - [], - DefaultsTreeNode( - node=ConfigDefault(path="self_trailing_pkg1"), - children=[ - GroupDefault(group="group1", name="file1"), - ConfigDefault(path="_self_", package="pkg1"), - ], - ), - id="self_trailing_pkg1", - ), - param( - "include_nested_group_pkg2", - [], - DefaultsTreeNode( - node=ConfigDefault(path="include_nested_group_pkg2"), - children=[ - ConfigDefault(path="_self_"), - DefaultsTreeNode( - node=GroupDefault(group="group1", name="group_item1_pkg2"), - children=[ - ConfigDefault(path="_self_"), - GroupDefault(group="group2", name="file1", package="pkg2"), - ], - ), - ], - ), - id="include_nested_group_pkg2", - ), - param( - "include_nested_config_item_pkg2", - [], - DefaultsTreeNode( - node=ConfigDefault(path="include_nested_config_item_pkg2"), - children=[ - ConfigDefault(path="_self_"), - DefaultsTreeNode( - node=GroupDefault(group="group1", name="config_item_pkg2"), - children=[ - ConfigDefault(path="_self_"), - ConfigDefault(path="group2/file1", package="pkg2"), - ], - ), - ], - ), - id="include_nested_config_item_pkg2", - ), - ], -) -def test_defaults_tree_with_package_overrides( - config_name: str, - overrides: List[str], - expected: DefaultsTreeNode, -) -> None: - _test_defaults_tree_impl( - config_name=config_name, input_overrides=overrides, expected=expected - ) - - @mark.parametrize( "config_name, overrides, expected", [ @@ -380,3 +295,99 @@ def test_simple_group_override( _test_defaults_tree_impl( config_name=config_name, input_overrides=overrides, expected=expected ) + + +@mark.parametrize( + "config_name, overrides, expected", + [ + param( + "error_self_pkg1", + [], + raises(ValueError, match=re.escape("_self_@PACKAGE is not supported")), + id="error_self_pkg1", + ), + ], +) +def test_errors( + config_name: str, + overrides: List[str], + expected: Any, +) -> None: + _test_defaults_tree_impl( + config_name=config_name, input_overrides=overrides, expected=expected + ) + + +@mark.parametrize( + "config_name, overrides, expected", + [ + param( + "config_default_pkg1", + [], + DefaultsTreeNode( + node=ConfigDefault(path="config_default_pkg1"), + children=[ + ConfigDefault(path="_self_"), + ConfigDefault(path="empty", package="pkg1"), + ], + ), + id="config_default_pkg1", + ), + param( + "group_default_pkg1", + [], + DefaultsTreeNode( + node=ConfigDefault(path="group_default_pkg1"), + children=[ + ConfigDefault(path="_self_"), + GroupDefault(group="group1", name="file1", package="pkg1"), + ], + ), + id="group_default_pkg1", + ), + param( + "include_nested_group_pkg2", + [], + DefaultsTreeNode( + node=ConfigDefault(path="include_nested_group_pkg2"), + children=[ + ConfigDefault(path="_self_"), + DefaultsTreeNode( + node=GroupDefault(group="group1", name="group_item1_pkg2"), + children=[ + ConfigDefault(path="_self_"), + GroupDefault(group="group2", name="file1", package="pkg2"), + ], + ), + ], + ), + id="include_nested_group_pkg2", + ), + param( + "include_nested_config_item_pkg2", + [], + DefaultsTreeNode( + node=ConfigDefault(path="include_nested_config_item_pkg2"), + children=[ + ConfigDefault(path="_self_"), + DefaultsTreeNode( + node=GroupDefault(group="group1", name="config_item_pkg2"), + children=[ + ConfigDefault(path="_self_"), + ConfigDefault(path="group2/file1", package="pkg2"), + ], + ), + ], + ), + id="include_nested_config_item_pkg2", + ), + ], +) +def test_defaults_tree_with_package_overrides( + config_name: str, + overrides: List[str], + expected: DefaultsTreeNode, +) -> None: + _test_defaults_tree_impl( + config_name=config_name, input_overrides=overrides, expected=expected + ) diff --git a/tests/test_data/default_lists/self_trailing_pkg1.yaml b/tests/test_data/default_lists/error_self_pkg1.yaml similarity index 59% rename from tests/test_data/default_lists/self_trailing_pkg1.yaml rename to tests/test_data/default_lists/error_self_pkg1.yaml index 54e27a9172c..d09ef5878f6 100644 --- a/tests/test_data/default_lists/self_trailing_pkg1.yaml +++ b/tests/test_data/default_lists/error_self_pkg1.yaml @@ -1,3 +1,2 @@ defaults: - - group1: file1 - _self_@pkg1 From fe0c9ba92c413818d1a2ca7292fcaa75982619e7 Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Thu, 26 Nov 2020 14:04:46 -0800 Subject: [PATCH 13/91] Tests overriding group option with non default package --- hydra/_internal/new_defaults_list.py | 48 +++++---- hydra/core/new_default_element.py | 8 ++ tests/defaults_list/__init__.py | 2 + .../{test_basic.py => test_defaults_list.py} | 101 ++++++++++++++++-- tests/defaults_list/test_defaults_tree.py | 79 +++++++++++++- 5 files changed, 208 insertions(+), 30 deletions(-) rename tests/defaults_list/{test_basic.py => test_defaults_list.py} (80%) diff --git a/hydra/_internal/new_defaults_list.py b/hydra/_internal/new_defaults_list.py index cfd9d998621..83ba1597dfa 100644 --- a/hydra/_internal/new_defaults_list.py +++ b/hydra/_internal/new_defaults_list.py @@ -20,11 +20,14 @@ @dataclass class Overrides: override_choices: Dict[str, str] + override_used: Dict[str, bool] + append_group_defaults: List[GroupDefault] config_overrides: List[Override] def __init__(self, repo: IConfigRepository, overrides_list: List[Override]) -> None: self.override_choices = {} + self.override_used = {} self.append_group_defaults = [] self.config_overrides = [] @@ -40,28 +43,39 @@ def __init__(self, repo: IConfigRepository, overrides_list: List[Override]) -> N ) if override.is_add(): self.append_group_defaults.append( - GroupDefault(group=override.key_or_group, name=value) + GroupDefault( + group=override.key_or_group, + package=override.get_subject_package(), + name=value, + ) ) else: - self.override_choices[override.key_or_group] = value + key = override.get_key_element() + self.override_choices[key] = value + self.override_used[key] = False def is_overridden(self, default: InputDefault) -> bool: if isinstance(default, GroupDefault): - key = default.get_group_path() # TODO: use package if present - return key in self.override_choices + return default.get_override_key() in self.override_choices return False - def get_choice_for(self, default: InputDefault) -> str: - if isinstance(default, GroupDefault): - key = default.get_group_path() # TODO: use package if present - if key in self.override_choices: - return self.override_choices[key] - else: - return default.name - else: - assert isinstance(default, ConfigDefault) - return default.path + def override_default_option(self, default: GroupDefault) -> None: + key = default.get_override_key() + if key in self.override_choices: + default.name = self.override_choices[key] + default.config_name_overridden = True + self.override_used[key] = True + + def ensure_overrides_used(self): + for key, used in self.override_used.items(): + if not used: + msg = dedent( + f"""\ + Could not override '{key}'. No match in the defaults list. + To append to your default list use +{key}={self.override_choices[key]}""" + ) + raise ConfigCompositionException(msg) @dataclass @@ -100,9 +114,7 @@ def _create_defaults_tree( parent = root.node if isinstance(parent, GroupDefault): if overrides.is_overridden(parent): - override_name = overrides.get_choice_for(parent) - parent.name = override_name - parent.config_name_overridden = True + overrides.override_default_option(parent) path = parent.get_config_path() @@ -201,7 +213,6 @@ def _create_defaults_list( output = [] _tree_to_list(tree=defaults_tree, output=output) - # TODO: convert tree to list with DFS # TODO: fail if duplicate items exists return output @@ -213,6 +224,7 @@ def create_defaults_list( ) -> DefaultsList: overrides = Overrides(repo=repo, overrides_list=overrides_list) defaults = _create_defaults_list(repo, config_name, overrides) + overrides.ensure_overrides_used() ret = DefaultsList(defaults=defaults, config_overrides=overrides.config_overrides) return ret diff --git a/hydra/core/new_default_element.py b/hydra/core/new_default_element.py index 621fcb6fa93..ebf5a19fd34 100644 --- a/hydra/core/new_default_element.py +++ b/hydra/core/new_default_element.py @@ -57,6 +57,14 @@ def _get_final_package( else: return ret[lgi + len("_global_") + 1 :] + def get_override_key(self) -> str: + default_pkg = self.get_default_package() + final_pkg = self.get_final_package() + key = self.get_group_path() + if default_pkg != final_pkg: + key = f"{key}@{final_pkg}" + return key + @dataclass class ConfigDefault(InputDefault): diff --git a/tests/defaults_list/__init__.py b/tests/defaults_list/__init__.py index 680673a2ce3..aa114ebf04f 100644 --- a/tests/defaults_list/__init__.py +++ b/tests/defaults_list/__init__.py @@ -37,6 +37,7 @@ def _test_defaults_tree_impl( overrides=overrides, is_primary_config=True, ) + overrides.ensure_overrides_used() assert result == expected else: with expected: @@ -46,3 +47,4 @@ def _test_defaults_tree_impl( overrides=overrides, is_primary_config=True, ) + overrides.ensure_overrides_used() diff --git a/tests/defaults_list/test_basic.py b/tests/defaults_list/test_defaults_list.py similarity index 80% rename from tests/defaults_list/test_basic.py rename to tests/defaults_list/test_defaults_list.py index 45410b49c00..68ce59c0185 100644 --- a/tests/defaults_list/test_basic.py +++ b/tests/defaults_list/test_defaults_list.py @@ -1,6 +1,10 @@ # Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved -from pytest import mark, param -from typing import List +from textwrap import dedent + +import re + +from pytest import mark, param, raises +from typing import List, Any from hydra._internal.new_defaults_list import ( create_defaults_list, @@ -9,6 +13,7 @@ from hydra.core.new_default_element import InputDefault, GroupDefault, ConfigDefault from hydra.core.override_parser.overrides_parser import OverridesParser from hydra.core.plugins import Plugins +from hydra.errors import ConfigCompositionException from hydra.test_utils.test_utils import chdir_hydra_root from tests.defaults_list import create_repo @@ -20,7 +25,8 @@ # TODO: (Y) Test with simple config group overrides # TODO: (Y) Test computed package when there are no package overrides in package header -# TODO: test with config group overrides overriding config groups @pkg +# TODO: (Y) test with config group overrides overriding config groups @pkg +# TODO: (Y) test overriding config group choices with non-default packages # TODO: test with config header package override # TODO: test with both config header and defaults list pkg override # TODO: handle hydra overrides @@ -86,18 +92,25 @@ def test_loaded_defaults_list(config_path: str, expected_list: List[InputDefault def _test_defaults_list_impl( config_name: str, overrides: List[str], - expected: List[ResultDefault], + expected: Any, ) -> None: parser = OverridesParser.create() repo = create_repo() overrides_list = parser.parse_overrides(overrides=overrides) - result = create_defaults_list( - repo=repo, - config_name=config_name, - overrides_list=overrides_list, - ) - - assert result.defaults == expected + if isinstance(expected, list): + result = create_defaults_list( + repo=repo, + config_name=config_name, + overrides_list=overrides_list, + ) + assert result.defaults == expected + else: + with expected: + create_defaults_list( + repo=repo, + config_name=config_name, + overrides_list=overrides_list, + ) @mark.parametrize( @@ -405,3 +418,69 @@ def test_override_package_in_defaults_list( _test_defaults_list_impl( config_name=config_name, overrides=overrides, expected=expected ) + + +@mark.parametrize( + "config_name, overrides, expected", + [ + param( + "group_default_pkg1", + ["group1@pkg1=file2"], + [ + ResultDefault( + config_path="group_default_pkg1", package="", is_self=True + ), + ResultDefault( + config_path="group1/file2", + package="pkg1", + parent="group_default_pkg1", + ), + ], + id="option_override:group_default_pkg1", + ), + param( + "group_default_pkg1", + ["group1@wrong=file2"], + raises( + ConfigCompositionException, + match=re.escape( + dedent( + """\ + Could not override 'group1@wrong'. No match in the defaults list. + To append to your default list use +group1@wrong=file2""" + ) + ), + ), + id="option_override:group_default_pkg1:bad_package_in_override", + ), + param( + "include_nested_group_pkg2", + ["group1/group2@group1.pkg2=file2"], + [ + ResultDefault( + config_path="include_nested_group_pkg2", package="", is_self=True + ), + ResultDefault( + config_path="group1/group_item1_pkg2", + parent="include_nested_group_pkg2", + package="group1", + is_self=True, + ), + ResultDefault( + config_path="group1/group2/file2", + package="group1.pkg2", + parent="group1/group_item1_pkg2", + ), + ], + id="option_override:include_nested_group_pkg2", + ), + ], +) +def test_override_package_in_defaults_list__group_override( + config_name: str, + overrides: List[str], + expected: List[ResultDefault], +) -> None: + _test_defaults_list_impl( + config_name=config_name, overrides=overrides, expected=expected + ) diff --git a/tests/defaults_list/test_defaults_tree.py b/tests/defaults_list/test_defaults_tree.py index 6ae8b2d5dad..2dfea72a4b2 100644 --- a/tests/defaults_list/test_defaults_tree.py +++ b/tests/defaults_list/test_defaults_tree.py @@ -1,4 +1,6 @@ # Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved +from textwrap import dedent + import re from pytest import mark, param, raises @@ -9,6 +11,7 @@ ) from hydra.core.new_default_element import GroupDefault, ConfigDefault from hydra.core.plugins import Plugins +from hydra.errors import ConfigCompositionException from hydra.test_utils.test_utils import chdir_hydra_root from tests.defaults_list import _test_defaults_tree_impl @@ -17,7 +20,6 @@ # registers config source plugins Plugins.instance() -# TODO: test inclusion of nested config item (not group) # TODO: Test cases where the default lists has package overrides # defaults list packages to test : _global_, _global_.foo, _name_ @@ -391,3 +393,78 @@ def test_defaults_tree_with_package_overrides( _test_defaults_tree_impl( config_name=config_name, input_overrides=overrides, expected=expected ) + + +@mark.parametrize( + "config_name, overrides, expected", + [ + param( + "group_default_pkg1", + ["group1@pkg1=file2"], + DefaultsTreeNode( + node=ConfigDefault(path="group_default_pkg1"), + children=[ + ConfigDefault(path="_self_"), + GroupDefault(group="group1", name="file2", package="pkg1"), + ], + ), + id="option_override:group_default_pkg1", + ), + param( + "group_default_pkg1", + ["group1@wrong=file2"], + raises( + ConfigCompositionException, + match=re.escape( + dedent( + """\ + Could not override 'group1@wrong'. No match in the defaults list. + To append to your default list use +group1@wrong=file2""" + ) + ), + ), + id="option_override:group_default_pkg1:bad_package_in_override", + ), + param( + "include_nested_group_pkg2", + ["group1/group2@group1.pkg2=file2"], + DefaultsTreeNode( + node=ConfigDefault(path="include_nested_group_pkg2"), + children=[ + ConfigDefault(path="_self_"), + DefaultsTreeNode( + node=GroupDefault(group="group1", name="group_item1_pkg2"), + children=[ + ConfigDefault(path="_self_"), + GroupDefault(group="group2", name="file2", package="pkg2"), + ], + ), + ], + ), + id="option_override:include_nested_group_pkg2", + ), + param( + "include_nested_group_pkg2", + ["group1/group2=file2"], + raises( + ConfigCompositionException, + match=re.escape( + dedent( + """\ + Could not override 'group1/group2'. No match in the defaults list. + To append to your default list use +group1/group2=file2""" + ) + ), + ), + id="option_override:include_nested_group_pkg2:missing_package_in_override", + ), + ], +) +def test_defaults_tree_with_package_overrides__group_override( + config_name: str, + overrides: List[str], + expected: DefaultsTreeNode, +) -> None: + _test_defaults_tree_impl( + config_name=config_name, input_overrides=overrides, expected=expected + ) From 1b5bdca06a805cda017a3cc6ef8d7192809a7315 Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Thu, 26 Nov 2020 17:45:10 -0800 Subject: [PATCH 14/91] test include_nested_config_item_global --- tests/defaults_list/test_defaults_list.py | 23 +++++++++++++++++++ .../group1/config_item_global_.yaml | 2 ++ .../include_nested_config_item_global.yaml | 2 ++ 3 files changed, 27 insertions(+) create mode 100644 tests/test_data/default_lists/group1/config_item_global_.yaml create mode 100644 tests/test_data/default_lists/include_nested_config_item_global.yaml diff --git a/tests/defaults_list/test_defaults_list.py b/tests/defaults_list/test_defaults_list.py index 68ce59c0185..defb82a5331 100644 --- a/tests/defaults_list/test_defaults_list.py +++ b/tests/defaults_list/test_defaults_list.py @@ -408,6 +408,29 @@ def test_simple_defaults_list_cases( ], id="include_nested_config_item_pkg2", ), + param( + "include_nested_config_item_global", + [], + [ + ResultDefault( + config_path="include_nested_config_item_global", + package="", + is_self=True, + ), + ResultDefault( + config_path="group1/config_item_global_", + parent="include_nested_config_item_global", + package="group1", + is_self=True, + ), + ResultDefault( + config_path="group1/group2/file1", + package="", + parent="group1/config_item_global_", + ), + ], + id="include_nested_config_item_global", + ), ], ) def test_override_package_in_defaults_list( diff --git a/tests/test_data/default_lists/group1/config_item_global_.yaml b/tests/test_data/default_lists/group1/config_item_global_.yaml new file mode 100644 index 00000000000..dec38a8ec57 --- /dev/null +++ b/tests/test_data/default_lists/group1/config_item_global_.yaml @@ -0,0 +1,2 @@ +defaults: + - group2/file1@_global_ \ No newline at end of file diff --git a/tests/test_data/default_lists/include_nested_config_item_global.yaml b/tests/test_data/default_lists/include_nested_config_item_global.yaml new file mode 100644 index 00000000000..9a778a5d4ba --- /dev/null +++ b/tests/test_data/default_lists/include_nested_config_item_global.yaml @@ -0,0 +1,2 @@ +defaults: + - group1: config_item_global_ \ No newline at end of file From 38063b02c4b0ab057e0be36830e5dfabe18e31ff Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Thu, 26 Nov 2020 18:07:22 -0800 Subject: [PATCH 15/91] support key@ \(empty package\) in the overrides parser --- hydra/grammar/OverrideParser.g4 | 2 +- tests/test_overrides_parser.py | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/hydra/grammar/OverrideParser.g4 b/hydra/grammar/OverrideParser.g4 index 953c736cecc..c085ee2173e 100644 --- a/hydra/grammar/OverrideParser.g4 +++ b/hydra/grammar/OverrideParser.g4 @@ -23,7 +23,7 @@ key : ; packageOrGroup: package | ID (SLASH ID)+; // db, hydra/launcher -package: (ID | DOT_PATH); // db, hydra.launcher +package: ( | ID | DOT_PATH); // db, hydra.launcher, or the empty (for _global_ package) // Elements (that may be swept over). diff --git a/tests/test_overrides_parser.py b/tests/test_overrides_parser.py index d6e5f2e9b46..29717e4a7fe 100644 --- a/tests/test_overrides_parser.py +++ b/tests/test_overrides_parser.py @@ -520,6 +520,11 @@ def test_package_or_group(value: str, expected: Any) -> None: Key(key_or_group="package_or_group", pkg1=None, pkg2="pkg2"), id="package_or_group@:pkg2", ), + pytest.param( + "package_or_group@", + Key(key_or_group="package_or_group", pkg1=""), + id="package_or_group@", + ), ], ) def test_key(value: str, expected: Any) -> None: From cdfc3be336529fe4a86d9dd6e19199eab5d6da6c Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Thu, 26 Nov 2020 17:52:42 -0800 Subject: [PATCH 16/91] test include_nested_group_global_ --- tests/defaults_list/test_defaults_list.py | 47 +++++++++++++++++++ .../group1/group_item1_global_.yaml | 2 + .../include_nested_group_global_.yaml | 2 + 3 files changed, 51 insertions(+) create mode 100644 tests/test_data/default_lists/group1/group_item1_global_.yaml create mode 100644 tests/test_data/default_lists/include_nested_group_global_.yaml diff --git a/tests/defaults_list/test_defaults_list.py b/tests/defaults_list/test_defaults_list.py index defb82a5331..c6285f099fb 100644 --- a/tests/defaults_list/test_defaults_list.py +++ b/tests/defaults_list/test_defaults_list.py @@ -431,6 +431,29 @@ def test_simple_defaults_list_cases( ], id="include_nested_config_item_global", ), + param( + "include_nested_group_global_", + [], + [ + ResultDefault( + config_path="include_nested_group_global_", + package="", + is_self=True, + ), + ResultDefault( + config_path="group1/group_item1_global_", + parent="include_nested_group_global_", + package="group1", + is_self=True, + ), + ResultDefault( + config_path="group1/group2/file1", + package="", + parent="group1/group_item1_global_", + ), + ], + id="include_nested_config_item_global", + ), ], ) def test_override_package_in_defaults_list( @@ -497,6 +520,30 @@ def test_override_package_in_defaults_list( ], id="option_override:include_nested_group_pkg2", ), + # TODO: Enable support for empty package after @ in cli parser + param( + "include_nested_group_global_", + ["group1/group2@=file2"], + [ + ResultDefault( + config_path="include_nested_group_global_", + package="", + is_self=True, + ), + ResultDefault( + config_path="group1/group_item1_global_", + parent="include_nested_group_global_", + package="group1", + is_self=True, + ), + ResultDefault( + config_path="group1/group2/file2", + package="", + parent="group1/group_item1_global_", + ), + ], + id="option_override:include_nested_config_item_global", + ), ], ) def test_override_package_in_defaults_list__group_override( diff --git a/tests/test_data/default_lists/group1/group_item1_global_.yaml b/tests/test_data/default_lists/group1/group_item1_global_.yaml new file mode 100644 index 00000000000..14ee2683cb1 --- /dev/null +++ b/tests/test_data/default_lists/group1/group_item1_global_.yaml @@ -0,0 +1,2 @@ +defaults: + - group2@_global_: file1 \ No newline at end of file diff --git a/tests/test_data/default_lists/include_nested_group_global_.yaml b/tests/test_data/default_lists/include_nested_group_global_.yaml new file mode 100644 index 00000000000..5958e943985 --- /dev/null +++ b/tests/test_data/default_lists/include_nested_group_global_.yaml @@ -0,0 +1,2 @@ +defaults: + - group1: group_item1_global_ \ No newline at end of file From 70fda9562979481e8d20473878097a478e2af54d Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Fri, 27 Nov 2020 14:27:34 -0800 Subject: [PATCH 17/91] added test for _global_.foo package override --- tests/defaults_list/test_defaults_list.py | 66 ++++++++++++++++++- tests/defaults_list/test_defaults_tree.py | 3 - .../group1/group_item1_global_foo.yaml | 2 + .../include_nested_group_global_foo.yaml | 2 + 4 files changed, 69 insertions(+), 4 deletions(-) create mode 100644 tests/test_data/default_lists/group1/group_item1_global_foo.yaml create mode 100644 tests/test_data/default_lists/include_nested_group_global_foo.yaml diff --git a/tests/defaults_list/test_defaults_list.py b/tests/defaults_list/test_defaults_list.py index c6285f099fb..0a779228e8b 100644 --- a/tests/defaults_list/test_defaults_list.py +++ b/tests/defaults_list/test_defaults_list.py @@ -27,6 +27,10 @@ # TODO: (Y) Test computed package when there are no package overrides in package header # TODO: (Y) test with config group overrides overriding config groups @pkg # TODO: (Y) test overriding config group choices with non-default packages +# packages to test: +# _global_ +# _global_.foo +# _name_ # TODO: test with config header package override # TODO: test with both config header and defaults list pkg override # TODO: handle hydra overrides @@ -520,7 +524,6 @@ def test_override_package_in_defaults_list( ], id="option_override:include_nested_group_pkg2", ), - # TODO: Enable support for empty package after @ in cli parser param( "include_nested_group_global_", ["group1/group2@=file2"], @@ -554,3 +557,64 @@ def test_override_package_in_defaults_list__group_override( _test_defaults_list_impl( config_name=config_name, overrides=overrides, expected=expected ) + + +@mark.parametrize( + "config_name, overrides, expected", + [ + param( + "include_nested_group_global_foo", + [], + [ + ResultDefault( + config_path="include_nested_group_global_foo", + package="", + is_self=True, + ), + ResultDefault( + config_path="group1/group_item1_global_foo", + parent="include_nested_group_global_foo", + package="group1", + is_self=True, + ), + ResultDefault( + config_path="group1/group2/file1", + package="foo", + parent="group1/group_item1_global_foo", + ), + ], + id="include_nested_group_global_foo", + ), + param( + "include_nested_group_global_foo", + ["group1/group2@foo=file2"], + [ + ResultDefault( + config_path="include_nested_group_global_foo", + package="", + is_self=True, + ), + ResultDefault( + config_path="group1/group_item1_global_foo", + parent="include_nested_group_global_foo", + package="group1", + is_self=True, + ), + ResultDefault( + config_path="group1/group2/file2", + package="foo", + parent="group1/group_item1_global_foo", + ), + ], + id="include_nested_group_global_foo", + ), + ], +) +def test_include_nested_group_global_foo( + config_name: str, + overrides: List[str], + expected: List[ResultDefault], +) -> None: + _test_defaults_list_impl( + config_name=config_name, overrides=overrides, expected=expected + ) diff --git a/tests/defaults_list/test_defaults_tree.py b/tests/defaults_list/test_defaults_tree.py index 2dfea72a4b2..ea15a52f869 100644 --- a/tests/defaults_list/test_defaults_tree.py +++ b/tests/defaults_list/test_defaults_tree.py @@ -20,9 +20,6 @@ # registers config source plugins Plugins.instance() -# TODO: Test cases where the default lists has package overrides -# defaults list packages to test : _global_, _global_.foo, _name_ - @mark.parametrize( "config_name, overrides, expected", diff --git a/tests/test_data/default_lists/group1/group_item1_global_foo.yaml b/tests/test_data/default_lists/group1/group_item1_global_foo.yaml new file mode 100644 index 00000000000..a261477dc2a --- /dev/null +++ b/tests/test_data/default_lists/group1/group_item1_global_foo.yaml @@ -0,0 +1,2 @@ +defaults: + - group2@_global_.foo: file1 \ No newline at end of file diff --git a/tests/test_data/default_lists/include_nested_group_global_foo.yaml b/tests/test_data/default_lists/include_nested_group_global_foo.yaml new file mode 100644 index 00000000000..6cc297210ba --- /dev/null +++ b/tests/test_data/default_lists/include_nested_group_global_foo.yaml @@ -0,0 +1,2 @@ +defaults: + - group1: group_item1_global_foo \ No newline at end of file From fde719516b9a9cd1f79508018784f8af11d908e0 Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Fri, 27 Nov 2020 14:41:52 -0800 Subject: [PATCH 18/91] reorg tests a bit --- tests/defaults_list/test_defaults_list.py | 146 +++++++++++++--------- 1 file changed, 88 insertions(+), 58 deletions(-) diff --git a/tests/defaults_list/test_defaults_list.py b/tests/defaults_list/test_defaults_list.py index 0a779228e8b..f57d53442ae 100644 --- a/tests/defaults_list/test_defaults_list.py +++ b/tests/defaults_list/test_defaults_list.py @@ -28,8 +28,8 @@ # TODO: (Y) test with config group overrides overriding config groups @pkg # TODO: (Y) test overriding config group choices with non-default packages # packages to test: -# _global_ -# _global_.foo +# (Y) _global_ +# (Y) _global_.foo # _name_ # TODO: test with config header package override # TODO: test with both config header and defaults list pkg override @@ -353,42 +353,6 @@ def test_simple_defaults_list_cases( ], id="config_default_pkg1", ), - param( - "group_default_pkg1", - [], - [ - ResultDefault( - config_path="group_default_pkg1", package="", is_self=True - ), - ResultDefault( - config_path="group1/file1", - package="pkg1", - parent="group_default_pkg1", - ), - ], - id="group_default_pkg1", - ), - param( - "include_nested_group_pkg2", - [], - [ - ResultDefault( - config_path="include_nested_group_pkg2", package="", is_self=True - ), - ResultDefault( - config_path="group1/group_item1_pkg2", - parent="include_nested_group_pkg2", - package="group1", - is_self=True, - ), - ResultDefault( - config_path="group1/group2/file1", - package="group1.pkg2", - parent="group1/group_item1_pkg2", - ), - ], - id="include_nested_group_pkg2", - ), param( "include_nested_config_item_pkg2", [], @@ -435,32 +399,66 @@ def test_simple_defaults_list_cases( ], id="include_nested_config_item_global", ), + ], +) +def test_override_package_in_defaults_list( + config_name: str, + overrides: List[str], + expected: List[ResultDefault], +) -> None: + _test_defaults_list_impl( + config_name=config_name, overrides=overrides, expected=expected + ) + + +@mark.parametrize( + "config_name, overrides, expected", + [ param( - "include_nested_group_global_", + "include_nested_group_pkg2", [], [ ResultDefault( - config_path="include_nested_group_global_", - package="", - is_self=True, + config_path="include_nested_group_pkg2", package="", is_self=True ), ResultDefault( - config_path="group1/group_item1_global_", - parent="include_nested_group_global_", + config_path="group1/group_item1_pkg2", + parent="include_nested_group_pkg2", package="group1", is_self=True, ), ResultDefault( config_path="group1/group2/file1", - package="", - parent="group1/group_item1_global_", + package="group1.pkg2", + parent="group1/group_item1_pkg2", ), ], - id="include_nested_config_item_global", + id="include_nested_group_pkg2", + ), + param( + "include_nested_group_pkg2", + ["group1/group2@group1.pkg2=file2"], + [ + ResultDefault( + config_path="include_nested_group_pkg2", package="", is_self=True + ), + ResultDefault( + config_path="group1/group_item1_pkg2", + parent="include_nested_group_pkg2", + package="group1", + is_self=True, + ), + ResultDefault( + config_path="group1/group2/file2", + package="group1.pkg2", + parent="group1/group_item1_pkg2", + ), + ], + id="option_override:include_nested_group_pkg2", ), ], ) -def test_override_package_in_defaults_list( +def test_include_nested_group_pkg2( config_name: str, overrides: List[str], expected: List[ResultDefault], @@ -473,6 +471,21 @@ def test_override_package_in_defaults_list( @mark.parametrize( "config_name, overrides, expected", [ + param( + "group_default_pkg1", + [], + [ + ResultDefault( + config_path="group_default_pkg1", package="", is_self=True + ), + ResultDefault( + config_path="group1/file1", + package="pkg1", + parent="group_default_pkg1", + ), + ], + id="group_default_pkg1", + ), param( "group_default_pkg1", ["group1@pkg1=file2"], @@ -503,26 +516,43 @@ def test_override_package_in_defaults_list( ), id="option_override:group_default_pkg1:bad_package_in_override", ), + ], +) +def test_group_default_pkg1( + config_name: str, + overrides: List[str], + expected: List[ResultDefault], +) -> None: + _test_defaults_list_impl( + config_name=config_name, overrides=overrides, expected=expected + ) + + +@mark.parametrize( + "config_name, overrides, expected", + [ param( - "include_nested_group_pkg2", - ["group1/group2@group1.pkg2=file2"], + "include_nested_group_global_", + [], [ ResultDefault( - config_path="include_nested_group_pkg2", package="", is_self=True + config_path="include_nested_group_global_", + package="", + is_self=True, ), ResultDefault( - config_path="group1/group_item1_pkg2", - parent="include_nested_group_pkg2", + config_path="group1/group_item1_global_", + parent="include_nested_group_global_", package="group1", is_self=True, ), ResultDefault( - config_path="group1/group2/file2", - package="group1.pkg2", - parent="group1/group_item1_pkg2", + config_path="group1/group2/file1", + package="", + parent="group1/group_item1_global_", ), ], - id="option_override:include_nested_group_pkg2", + id="include_nested_config_item_global", ), param( "include_nested_group_global_", @@ -549,7 +579,7 @@ def test_override_package_in_defaults_list( ), ], ) -def test_override_package_in_defaults_list__group_override( +def test_include_nested_group_global( config_name: str, overrides: List[str], expected: List[ResultDefault], From d7713571ff9ebee0e34d0a68153f630b1a4c86a5 Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Fri, 27 Nov 2020 15:16:35 -0800 Subject: [PATCH 19/91] testing _name_ package override --- hydra/core/new_default_element.py | 24 ++++++- tests/defaults_list/test_defaults_list.py | 69 ++++++++++++++++++- .../group1/group_item1_name_.yaml | 2 + .../include_nested_group_name_.yaml | 2 + 4 files changed, 94 insertions(+), 3 deletions(-) create mode 100644 tests/test_data/default_lists/group1/group_item1_name_.yaml create mode 100644 tests/test_data/default_lists/include_nested_group_name_.yaml diff --git a/hydra/core/new_default_element.py b/hydra/core/new_default_element.py index ebf5a19fd34..f131a179299 100644 --- a/hydra/core/new_default_element.py +++ b/hydra/core/new_default_element.py @@ -34,15 +34,21 @@ def get_final_package(self) -> str: def _relative_group_path(self) -> str: raise NotImplementedError() + def get_name(self) -> str: + raise NotImplementedError() + def _get_final_package( self, parent_package: Optional[str], package: Optional[str], + name: str, ) -> str: assert parent_package is not None if package is None: package = self._relative_group_path().replace("/", ".") + package = package.replace("_name_", name) + if parent_package == "": ret = package else: @@ -96,6 +102,13 @@ def get_group_path(self) -> str: else: return f"{self.parent_base_dir}/{group}" + def get_name(self) -> str: + idx = self.path.rfind("/") + if idx == -1: + return self.path + else: + return self.path[idx + 1 :] + def get_config_path(self) -> str: assert self.parent_base_dir is not None if self.parent_base_dir == "": @@ -104,7 +117,9 @@ def get_config_path(self) -> str: return f"{self.parent_base_dir}/{self.path}" def get_final_package(self) -> str: - return self._get_final_package(self.parent_package, self.package) + return self._get_final_package( + self.parent_package, self.package, self.get_name() + ) def _relative_group_path(self) -> str: idx = self.path.rfind("/") @@ -146,8 +161,13 @@ def get_config_path(self) -> str: return f"{group_path}/{self.name}" + def get_name(self) -> str: + return self.name + def get_final_package(self) -> str: - return self._get_final_package(self.parent_package, self.package) + return self._get_final_package( + self.parent_package, self.package, self.get_name() + ) def _relative_group_path(self) -> str: return self.group diff --git a/tests/defaults_list/test_defaults_list.py b/tests/defaults_list/test_defaults_list.py index f57d53442ae..5c7df646e7d 100644 --- a/tests/defaults_list/test_defaults_list.py +++ b/tests/defaults_list/test_defaults_list.py @@ -30,15 +30,21 @@ # packages to test: # (Y) _global_ # (Y) _global_.foo -# _name_ +# (Y) _name_ # TODO: test with config header package override # TODO: test with both config header and defaults list pkg override # TODO: handle hydra overrides +# - reconsider support for overriding as before +# - implement an explicit overriding method for config groups # TODO: test handling missing configs mentioned in defaults list (with and without optional) # TODO: test overriding configs in absolute location # TODO: test duplicate _self_ error # TODO: test duplicates in result config list # TODO: Interpolation support +# TODO: package header: +# - consider making relative +# - consider deprecating completely +# TODO: update documentation @mark.parametrize( @@ -648,3 +654,64 @@ def test_include_nested_group_global_foo( _test_defaults_list_impl( config_name=config_name, overrides=overrides, expected=expected ) + + +@mark.parametrize( + "config_name, overrides, expected", + [ + param( + "include_nested_group_name_", + [], + [ + ResultDefault( + config_path="include_nested_group_name_", + package="", + is_self=True, + ), + ResultDefault( + config_path="group1/group_item1_name_", + parent="include_nested_group_name_", + package="group1", + is_self=True, + ), + ResultDefault( + config_path="group1/group2/file1", + package="group1.file1", + parent="group1/group_item1_name_", + ), + ], + id="include_nested_group_name_", + ), + param( + "include_nested_group_name_", + ["group1/group2@group1.file1=file2"], + [ + ResultDefault( + config_path="include_nested_group_name_", + package="", + is_self=True, + ), + ResultDefault( + config_path="group1/group_item1_name_", + parent="include_nested_group_name_", + package="group1", + is_self=True, + ), + ResultDefault( + config_path="group1/group2/file2", + package="group1.file2", + parent="group1/group_item1_name_", + ), + ], + id="include_nested_group_name_", + ), + ], +) +def test_include_nested_group_name_( + config_name: str, + overrides: List[str], + expected: List[ResultDefault], +) -> None: + _test_defaults_list_impl( + config_name=config_name, overrides=overrides, expected=expected + ) diff --git a/tests/test_data/default_lists/group1/group_item1_name_.yaml b/tests/test_data/default_lists/group1/group_item1_name_.yaml new file mode 100644 index 00000000000..52eeffb147c --- /dev/null +++ b/tests/test_data/default_lists/group1/group_item1_name_.yaml @@ -0,0 +1,2 @@ +defaults: + - group2@_name_: file1 \ No newline at end of file diff --git a/tests/test_data/default_lists/include_nested_group_name_.yaml b/tests/test_data/default_lists/include_nested_group_name_.yaml new file mode 100644 index 00000000000..7c18976215e --- /dev/null +++ b/tests/test_data/default_lists/include_nested_group_name_.yaml @@ -0,0 +1,2 @@ +defaults: + - group1: group_item1_name_ \ No newline at end of file From 6df6b991a12dc151053e72c6d8ff7fd7a18395b5 Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Fri, 27 Nov 2020 17:04:56 -0800 Subject: [PATCH 20/91] Testing primary config with a package header --- hydra/_internal/new_defaults_list.py | 13 +++ hydra/core/new_default_element.py | 23 +++-- tests/defaults_list/test_defaults_list.py | 90 ++++++++++++++----- .../test_data/default_lists/group1/file3.yaml | 0 .../default_lists/primary_pkg_header_foo.yaml | 5 ++ 5 files changed, 105 insertions(+), 26 deletions(-) create mode 100644 tests/test_data/default_lists/group1/file3.yaml create mode 100644 tests/test_data/default_lists/primary_pkg_header_foo.yaml diff --git a/hydra/_internal/new_defaults_list.py b/hydra/_internal/new_defaults_list.py index 83ba1597dfa..c7d5edc7130 100644 --- a/hydra/_internal/new_defaults_list.py +++ b/hydra/_internal/new_defaults_list.py @@ -99,6 +99,16 @@ def _validate_self(containing_node: InputDefault, defaults: List[InputDefault]) defaults.insert(0, ConfigDefault(path="_self_")) +def update_package_header( + repo: IConfigRepository, node: InputDefault, is_primary_config: bool +): + loaded = repo.load_config( + config_path=node.get_config_path(), is_primary_config=is_primary_config + ) + if loaded is not None and "orig_package" in loaded.header: + node.set_package_header(loaded.header["orig_package"]) + + def _create_defaults_tree( repo: IConfigRepository, root: DefaultsTreeNode, @@ -112,6 +122,9 @@ def _create_defaults_tree( root.node.parent_package = "" parent = root.node + + update_package_header(repo=repo, node=parent, is_primary_config=is_primary_config) + if isinstance(parent, GroupDefault): if overrides.is_overridden(parent): overrides.override_default_option(parent) diff --git a/hydra/core/new_default_element.py b/hydra/core/new_default_element.py index f131a179299..40257870920 100644 --- a/hydra/core/new_default_element.py +++ b/hydra/core/new_default_element.py @@ -6,7 +6,6 @@ class ResultDefault: config_path: Optional[str] = None parent: Optional[str] = None - # addressing_key: Optional[str] = None package: Optional[str] = None is_self: bool = False @@ -25,9 +24,6 @@ def get_config_path(self) -> str: def get_default_package(self) -> str: return self.get_group_path().replace("/", ".") - # def get_config_package_in_header(self) -> str: - # raise NotImplementedError() - def get_final_package(self) -> str: raise NotImplementedError() @@ -37,6 +33,20 @@ def _relative_group_path(self) -> str: def get_name(self) -> str: raise NotImplementedError() + def set_package_header(self, package_header: str) -> None: + assert self.__dict__["package_header"] is None + # TODO: is package header relative or absolute? + self.__dict__["package_header"] = package_header + + def get_package_header(self) -> Optional[str]: + return self.__dict__["package_header"] + + def get_package(self) -> str: + if self.__dict__["package"] is None: + return self.__dict__["package_header"] + else: + return self.__dict__["package"] + def _get_final_package( self, parent_package: Optional[str], @@ -76,8 +86,10 @@ def get_override_key(self) -> str: class ConfigDefault(InputDefault): path: str package: Optional[str] = None + parent_base_dir: Optional[str] = field(default=None, compare=False, repr=False) parent_package: Optional[str] = field(default=None, compare=False, repr=False) + package_header: Optional[str] = field(default=None, compare=False) def __post_init__(self): if self.is_self() and self.package is not None: @@ -118,7 +130,7 @@ def get_config_path(self) -> str: def get_final_package(self) -> str: return self._get_final_package( - self.parent_package, self.package, self.get_name() + self.parent_package, self.get_package(), self.get_name() ) def _relative_group_path(self) -> str: @@ -140,6 +152,7 @@ class GroupDefault(InputDefault): parent_base_dir: Optional[str] = field(default=None, compare=False, repr=False) config_name_overridden: bool = field(default=False, compare=False, repr=False) + package_header: Optional[str] = field(default=None, compare=False) def __post_init__(self): assert self.group is not None and self.group != "" diff --git a/tests/defaults_list/test_defaults_list.py b/tests/defaults_list/test_defaults_list.py index 5c7df646e7d..0c2f9d9fd28 100644 --- a/tests/defaults_list/test_defaults_list.py +++ b/tests/defaults_list/test_defaults_list.py @@ -334,9 +334,7 @@ def test_get_final_package(default: InputDefault, expected) -> None: ], ) def test_simple_defaults_list_cases( - config_name: str, - overrides: List[str], - expected: List[ResultDefault], + config_name: str, overrides: List[str], expected: List[ResultDefault] ) -> None: _test_defaults_list_impl( config_name=config_name, overrides=overrides, expected=expected @@ -408,9 +406,7 @@ def test_simple_defaults_list_cases( ], ) def test_override_package_in_defaults_list( - config_name: str, - overrides: List[str], - expected: List[ResultDefault], + config_name: str, overrides: List[str], expected: List[ResultDefault] ) -> None: _test_defaults_list_impl( config_name=config_name, overrides=overrides, expected=expected @@ -465,9 +461,7 @@ def test_override_package_in_defaults_list( ], ) def test_include_nested_group_pkg2( - config_name: str, - overrides: List[str], - expected: List[ResultDefault], + config_name: str, overrides: List[str], expected: List[ResultDefault] ) -> None: _test_defaults_list_impl( config_name=config_name, overrides=overrides, expected=expected @@ -525,9 +519,7 @@ def test_include_nested_group_pkg2( ], ) def test_group_default_pkg1( - config_name: str, - overrides: List[str], - expected: List[ResultDefault], + config_name: str, overrides: List[str], expected: List[ResultDefault] ) -> None: _test_defaults_list_impl( config_name=config_name, overrides=overrides, expected=expected @@ -586,9 +578,7 @@ def test_group_default_pkg1( ], ) def test_include_nested_group_global( - config_name: str, - overrides: List[str], - expected: List[ResultDefault], + config_name: str, overrides: List[str], expected: List[ResultDefault] ) -> None: _test_defaults_list_impl( config_name=config_name, overrides=overrides, expected=expected @@ -647,9 +637,7 @@ def test_include_nested_group_global( ], ) def test_include_nested_group_global_foo( - config_name: str, - overrides: List[str], - expected: List[ResultDefault], + config_name: str, overrides: List[str], expected: List[ResultDefault] ) -> None: _test_defaults_list_impl( config_name=config_name, overrides=overrides, expected=expected @@ -708,10 +696,70 @@ def test_include_nested_group_global_foo( ], ) def test_include_nested_group_name_( - config_name: str, - overrides: List[str], - expected: List[ResultDefault], + config_name: str, overrides: List[str], expected: List[ResultDefault] ) -> None: _test_defaults_list_impl( config_name=config_name, overrides=overrides, expected=expected ) + + +@mark.parametrize( + "config_name, overrides, expected", + [ + param( + "primary_pkg_header_foo", + [], + [ + ResultDefault( + config_path="primary_pkg_header_foo", + package="foo", + is_self=True, + ), + ResultDefault( + config_path="group1/file1", + package="foo.group1", + parent="primary_pkg_header_foo", + ), + ResultDefault( + config_path="group1/file1", + package="foo.pkg", + parent="primary_pkg_header_foo", + ), + ], + id="primary_pkg_header_foo", + ), + param( + "primary_pkg_header_foo", + ["group1@foo.group1=file2", "group1@foo.pkg=file3"], + [ + ResultDefault( + config_path="primary_pkg_header_foo", + package="foo", + is_self=True, + ), + ResultDefault( + config_path="group1/file2", + package="foo.group1", + parent="primary_pkg_header_foo", + ), + ResultDefault( + config_path="group1/file3", + package="foo.pkg", + parent="primary_pkg_header_foo", + ), + ], + id="primary_pkg_header_foo", + ), + ], +) +def test_primary_cfg_pkg_header_foo( + config_name: str, overrides: List[str], expected: List[ResultDefault] +): + _test_defaults_list_impl( + config_name=config_name, overrides=overrides, expected=expected + ) + + +# TODO: test the following package header cases: +# - package header in nested config (package header is absolute) +# - overriding config group with a package header, is it even possible given the circular dependency? diff --git a/tests/test_data/default_lists/group1/file3.yaml b/tests/test_data/default_lists/group1/file3.yaml new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/test_data/default_lists/primary_pkg_header_foo.yaml b/tests/test_data/default_lists/primary_pkg_header_foo.yaml new file mode 100644 index 00000000000..5cc08a5907c --- /dev/null +++ b/tests/test_data/default_lists/primary_pkg_header_foo.yaml @@ -0,0 +1,5 @@ +# @package foo + +defaults: + - group1: file1 + - group1@pkg: file1 \ No newline at end of file From 5458eca7194ff1d542f6d9d675e5fb5407a11c24 Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Fri, 27 Nov 2020 18:34:02 -0800 Subject: [PATCH 21/91] testing include_nested_group_pkg_header_foo --- hydra/_internal/new_defaults_list.py | 6 +- hydra/core/new_default_element.py | 2 +- tests/defaults_list/test_defaults_list.py | 59 +++++++++++++++++++ .../group1/group_item1_pkg_header_foo.yaml | 3 + .../include_nested_group_pkg_header_foo.yaml | 2 + 5 files changed, 70 insertions(+), 2 deletions(-) create mode 100644 tests/test_data/default_lists/group1/group_item1_pkg_header_foo.yaml create mode 100644 tests/test_data/default_lists/include_nested_group_pkg_header_foo.yaml diff --git a/hydra/_internal/new_defaults_list.py b/hydra/_internal/new_defaults_list.py index c7d5edc7130..12cbe6f4f5b 100644 --- a/hydra/_internal/new_defaults_list.py +++ b/hydra/_internal/new_defaults_list.py @@ -102,6 +102,9 @@ def _validate_self(containing_node: InputDefault, defaults: List[InputDefault]) def update_package_header( repo: IConfigRepository, node: InputDefault, is_primary_config: bool ): + # This loads the same config loaded in _create_defaults_tree + # To avoid loading it twice, the repo implementation is expected to cache + # loaded configs loaded = repo.load_config( config_path=node.get_config_path(), is_primary_config=is_primary_config ) @@ -132,6 +135,7 @@ def _create_defaults_tree( path = parent.get_config_path() loaded = repo.load_config(config_path=path, is_primary_config=is_primary_config) + # TODO: test case where the config option is overridden and the newly loaded config has a different package header. if loaded is None: missing_config_error(repo, root.node) @@ -148,7 +152,7 @@ def _create_defaults_tree( for d in defaults_list: if d.is_self(): d.parent_base_dir = root.node.parent_base_dir - d.parent_package = root.node.parent_package + d.parent_package = root.node.get_package() children.append(d) else: new_root = DefaultsTreeNode(node=d, parent=root) diff --git a/hydra/core/new_default_element.py b/hydra/core/new_default_element.py index 40257870920..0bacc1b7ef7 100644 --- a/hydra/core/new_default_element.py +++ b/hydra/core/new_default_element.py @@ -179,7 +179,7 @@ def get_name(self) -> str: def get_final_package(self) -> str: return self._get_final_package( - self.parent_package, self.package, self.get_name() + self.parent_package, self.get_package(), self.get_name() ) def _relative_group_path(self) -> str: diff --git a/tests/defaults_list/test_defaults_list.py b/tests/defaults_list/test_defaults_list.py index 0c2f9d9fd28..3bb9fb162ab 100644 --- a/tests/defaults_list/test_defaults_list.py +++ b/tests/defaults_list/test_defaults_list.py @@ -760,6 +760,65 @@ def test_primary_cfg_pkg_header_foo( ) +@mark.parametrize( + "config_name, overrides, expected", + [ + param( + "include_nested_group_pkg_header_foo", + [], + [ + ResultDefault( + config_path="include_nested_group_pkg_header_foo", + package="", + is_self=True, + ), + ResultDefault( + config_path="group1/group_item1_pkg_header_foo", + package="foo", + is_self=True, + parent="include_nested_group_pkg_header_foo", + ), + ResultDefault( + config_path="group1/group2/file1", + package="foo.group2", + parent="group1/group_item1_pkg_header_foo", + ), + ], + id="include_nested_group_pkg_header_foo", + ), + param( + "include_nested_group_pkg_header_foo", + ["group1/group2@foo.group2=file2"], + [ + ResultDefault( + config_path="include_nested_group_pkg_header_foo", + package="", + is_self=True, + ), + ResultDefault( + config_path="group1/group_item1_pkg_header_foo", + package="foo", + is_self=True, + parent="include_nested_group_pkg_header_foo", + ), + ResultDefault( + config_path="group1/group2/file2", + package="foo.group2", + parent="group1/group_item1_pkg_header_foo", + ), + ], + id="primary_pkg_header_foo", + ), + ], +) +def test_include_nested_group_pkg_header_foo( + config_name: str, overrides: List[str], expected: List[ResultDefault] +): + _test_defaults_list_impl( + config_name=config_name, overrides=overrides, expected=expected + ) + + # TODO: test the following package header cases: # - package header in nested config (package header is absolute) # - overriding config group with a package header, is it even possible given the circular dependency? diff --git a/tests/test_data/default_lists/group1/group_item1_pkg_header_foo.yaml b/tests/test_data/default_lists/group1/group_item1_pkg_header_foo.yaml new file mode 100644 index 00000000000..b40cd8e6956 --- /dev/null +++ b/tests/test_data/default_lists/group1/group_item1_pkg_header_foo.yaml @@ -0,0 +1,3 @@ +# @package foo +defaults: + - group2: file1 \ No newline at end of file diff --git a/tests/test_data/default_lists/include_nested_group_pkg_header_foo.yaml b/tests/test_data/default_lists/include_nested_group_pkg_header_foo.yaml new file mode 100644 index 00000000000..498d217c3b2 --- /dev/null +++ b/tests/test_data/default_lists/include_nested_group_pkg_header_foo.yaml @@ -0,0 +1,2 @@ +defaults: + - group1: group_item1_pkg_header_foo \ No newline at end of file From 12db5bd15d04054d574852894156de9b902ec02e Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Fri, 27 Nov 2020 18:45:13 -0800 Subject: [PATCH 22/91] testing include_nested_group_pkg_header_foo:override_first_level --- tests/defaults_list/test_defaults_list.py | 25 ++++++++++++++++++- .../group1/group_item2_pkg_header_foo.yaml | 3 +++ 2 files changed, 27 insertions(+), 1 deletion(-) create mode 100644 tests/test_data/default_lists/group1/group_item2_pkg_header_foo.yaml diff --git a/tests/defaults_list/test_defaults_list.py b/tests/defaults_list/test_defaults_list.py index 3bb9fb162ab..e1b6b5ad35c 100644 --- a/tests/defaults_list/test_defaults_list.py +++ b/tests/defaults_list/test_defaults_list.py @@ -807,7 +807,30 @@ def test_primary_cfg_pkg_header_foo( parent="group1/group_item1_pkg_header_foo", ), ], - id="primary_pkg_header_foo", + id="include_nested_group_pkg_header_foo:override_nested", + ), + param( + "include_nested_group_pkg_header_foo", + ["group1@foo=group_item2_pkg_header_foo"], + [ + ResultDefault( + config_path="include_nested_group_pkg_header_foo", + package="", + is_self=True, + ), + ResultDefault( + config_path="group1/group_item2_pkg_header_foo", + package="foo", + is_self=True, + parent="include_nested_group_pkg_header_foo", + ), + ResultDefault( + config_path="group1/group2/file2", + package="foo.group2", + parent="group1/group_item2_pkg_header_foo", + ), + ], + id="include_nested_group_pkg_header_foo:override_first_level", ), ], ) diff --git a/tests/test_data/default_lists/group1/group_item2_pkg_header_foo.yaml b/tests/test_data/default_lists/group1/group_item2_pkg_header_foo.yaml new file mode 100644 index 00000000000..4ecd3a82ccc --- /dev/null +++ b/tests/test_data/default_lists/group1/group_item2_pkg_header_foo.yaml @@ -0,0 +1,3 @@ +# @package foo +defaults: + - group2: file2 \ No newline at end of file From 4cbbe209953c6540a913ce84bd30bdef75360ad8 Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Fri, 27 Nov 2020 18:54:26 -0800 Subject: [PATCH 23/91] Test for include_nested_group_pkg_header_foo:override_first_level_with_package_header_change --- hydra/_internal/new_defaults_list.py | 7 +++++ tests/defaults_list/test_defaults_list.py | 28 +++++++++++++++++-- .../group1/group_item2_pkg_header_bar.yaml | 3 ++ 3 files changed, 36 insertions(+), 2 deletions(-) create mode 100644 tests/test_data/default_lists/group1/group_item2_pkg_header_bar.yaml diff --git a/hydra/_internal/new_defaults_list.py b/hydra/_internal/new_defaults_list.py index 12cbe6f4f5b..5445a4b96ec 100644 --- a/hydra/_internal/new_defaults_list.py +++ b/hydra/_internal/new_defaults_list.py @@ -56,6 +56,7 @@ def __init__(self, repo: IConfigRepository, overrides_list: List[Override]) -> N def is_overridden(self, default: InputDefault) -> bool: if isinstance(default, GroupDefault): + # TODO: collect overridable keys for help/useful error purposes return default.get_override_key() in self.override_choices return False @@ -131,6 +132,12 @@ def _create_defaults_tree( if isinstance(parent, GroupDefault): if overrides.is_overridden(parent): overrides.override_default_option(parent) + # clear package header and obtain updated one from overridden config + # (for the rare case it has changed) + parent.package_header = None + update_package_header( + repo=repo, node=parent, is_primary_config=is_primary_config + ) path = parent.get_config_path() diff --git a/tests/defaults_list/test_defaults_list.py b/tests/defaults_list/test_defaults_list.py index e1b6b5ad35c..7e60a27f0b8 100644 --- a/tests/defaults_list/test_defaults_list.py +++ b/tests/defaults_list/test_defaults_list.py @@ -832,6 +832,29 @@ def test_primary_cfg_pkg_header_foo( ], id="include_nested_group_pkg_header_foo:override_first_level", ), + param( + "include_nested_group_pkg_header_foo", + ["group1@foo=group_item2_pkg_header_bar"], + [ + ResultDefault( + config_path="include_nested_group_pkg_header_foo", + package="", + is_self=True, + ), + ResultDefault( + config_path="group1/group_item2_pkg_header_bar", + package="bar", + is_self=True, + parent="include_nested_group_pkg_header_foo", + ), + ResultDefault( + config_path="group1/group2/file2", + package="bar.group2", + parent="group1/group_item2_pkg_header_bar", + ), + ], + id="include_nested_group_pkg_header_foo:override_first_level_with_package_header_change", + ), ], ) def test_include_nested_group_pkg_header_foo( @@ -843,5 +866,6 @@ def test_include_nested_group_pkg_header_foo( # TODO: test the following package header cases: -# - package header in nested config (package header is absolute) -# - overriding config group with a package header, is it even possible given the circular dependency? +# - (Y) package header in nested config (package header is absolute) +# - (Y) overriding config group with a package header, is it even possible given the circular dependency? +# - Confirm package header is absolute (using a package header in a third level config) diff --git a/tests/test_data/default_lists/group1/group_item2_pkg_header_bar.yaml b/tests/test_data/default_lists/group1/group_item2_pkg_header_bar.yaml new file mode 100644 index 00000000000..8faa14bff3d --- /dev/null +++ b/tests/test_data/default_lists/group1/group_item2_pkg_header_bar.yaml @@ -0,0 +1,3 @@ +# @package bar +defaults: + - group2: file2 \ No newline at end of file From 0f3ddbc787ae5cbd3cbc0d5496224c2336275fe6 Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Sat, 28 Nov 2020 15:08:12 -0800 Subject: [PATCH 24/91] fixed pacakge header to be absolute + tests --- hydra/core/new_default_element.py | 8 ++- tests/defaults_list/test_defaults_list.py | 51 +++++++++++++++++-- .../group1/group2/file1_pkg_header_foo.yaml | 1 + .../group_item1_with_pkg_header_foo.yaml | 2 + 4 files changed, 56 insertions(+), 6 deletions(-) create mode 100644 tests/test_data/default_lists/group1/group2/file1_pkg_header_foo.yaml create mode 100644 tests/test_data/default_lists/group1/group_item1_with_pkg_header_foo.yaml diff --git a/hydra/core/new_default_element.py b/hydra/core/new_default_element.py index 0bacc1b7ef7..5d8b12d5a1d 100644 --- a/hydra/core/new_default_element.py +++ b/hydra/core/new_default_element.py @@ -35,7 +35,13 @@ def get_name(self) -> str: def set_package_header(self, package_header: str) -> None: assert self.__dict__["package_header"] is None - # TODO: is package header relative or absolute? + # package header is always interpreted as absolute. + # if it does not have a _global_ prefix, add it. + if package_header != "_global_" and not package_header.startswith("_global_."): + if package_header == "": + package_header = "_global_" + else: + package_header = f"_global_.{package_header}" self.__dict__["package_header"] = package_header def get_package_header(self) -> Optional[str]: diff --git a/tests/defaults_list/test_defaults_list.py b/tests/defaults_list/test_defaults_list.py index 7e60a27f0b8..edf1668abc3 100644 --- a/tests/defaults_list/test_defaults_list.py +++ b/tests/defaults_list/test_defaults_list.py @@ -31,7 +31,7 @@ # (Y) _global_ # (Y) _global_.foo # (Y) _name_ -# TODO: test with config header package override +# TODO: (Y) test with config header package override # TODO: test with both config header and defaults list pkg override # TODO: handle hydra overrides # - reconsider support for overriding as before @@ -44,6 +44,7 @@ # TODO: package header: # - consider making relative # - consider deprecating completely + # TODO: update documentation @@ -865,7 +866,47 @@ def test_include_nested_group_pkg_header_foo( ) -# TODO: test the following package header cases: -# - (Y) package header in nested config (package header is absolute) -# - (Y) overriding config group with a package header, is it even possible given the circular dependency? -# - Confirm package header is absolute (using a package header in a third level config) +@mark.parametrize( + "config_name, overrides, expected", + [ + param( + "empty", + ["+group1/group2=file1_pkg_header_foo"], + [ + ResultDefault(config_path="empty", package="", is_self=True), + ResultDefault( + config_path="group1/group2/file1_pkg_header_foo", + parent="empty", + package="foo", + ), + ], + id="included_from_overrides", + ), + param( + "empty", + ["+group1=group_item1_with_pkg_header_foo"], + [ + ResultDefault(config_path="empty", package="", is_self=True), + ResultDefault( + config_path="group1/group_item1_with_pkg_header_foo", + parent="empty", + package="group1", + is_self=True, + ), + ResultDefault( + config_path="group1/group2/file1_pkg_header_foo", + parent="group1/group_item1_with_pkg_header_foo", + package="foo", + is_self=False, + ), + ], + id="included_from_overrides", + ), + ], +) +def test_nested_package_header_is_absolute( + config_name: str, overrides: List[str], expected: List[ResultDefault] +): + _test_defaults_list_impl( + config_name=config_name, overrides=overrides, expected=expected + ) diff --git a/tests/test_data/default_lists/group1/group2/file1_pkg_header_foo.yaml b/tests/test_data/default_lists/group1/group2/file1_pkg_header_foo.yaml new file mode 100644 index 00000000000..73dd004e2b4 --- /dev/null +++ b/tests/test_data/default_lists/group1/group2/file1_pkg_header_foo.yaml @@ -0,0 +1 @@ +# @package foo \ No newline at end of file diff --git a/tests/test_data/default_lists/group1/group_item1_with_pkg_header_foo.yaml b/tests/test_data/default_lists/group1/group_item1_with_pkg_header_foo.yaml new file mode 100644 index 00000000000..1bc7d987cff --- /dev/null +++ b/tests/test_data/default_lists/group1/group_item1_with_pkg_header_foo.yaml @@ -0,0 +1,2 @@ +defaults: + - group2: file1_pkg_header_foo \ No newline at end of file From 571cd7ee2a871357f6ce8b7bed87a4d863effab6 Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Sat, 28 Nov 2020 15:36:33 -0800 Subject: [PATCH 25/91] test with both config header and defaults list pkg override --- tests/defaults_list/test_defaults_list.py | 95 ++++++++++++++++++- ...group_pkg_header_foo_override_pkg_bar.yaml | 2 + 2 files changed, 94 insertions(+), 3 deletions(-) create mode 100644 tests/test_data/default_lists/include_nested_group_pkg_header_foo_override_pkg_bar.yaml diff --git a/tests/defaults_list/test_defaults_list.py b/tests/defaults_list/test_defaults_list.py index edf1668abc3..be6f7e22cbc 100644 --- a/tests/defaults_list/test_defaults_list.py +++ b/tests/defaults_list/test_defaults_list.py @@ -32,10 +32,11 @@ # (Y) _global_.foo # (Y) _name_ # TODO: (Y) test with config header package override -# TODO: test with both config header and defaults list pkg override +# TODO: (Y) test with both config header and defaults list pkg override # TODO: handle hydra overrides -# - reconsider support for overriding as before -# - implement an explicit overriding method for config groups +# - (X) reconsider support for overriding as before +# - Support marked overrides in primary config only +# - Support marked override in all configs # TODO: test handling missing configs mentioned in defaults list (with and without optional) # TODO: test overriding configs in absolute location # TODO: test duplicate _self_ error @@ -910,3 +911,91 @@ def test_nested_package_header_is_absolute( _test_defaults_list_impl( config_name=config_name, overrides=overrides, expected=expected ) + + +@mark.parametrize( + "config_name, overrides, expected", + [ + param( + "include_nested_group_pkg_header_foo_override_pkg_bar", + [], + [ + ResultDefault( + config_path="include_nested_group_pkg_header_foo_override_pkg_bar", + parent=None, + package="", + is_self=True, + ), + ResultDefault( + config_path="group1/group_item1_pkg_header_foo", + parent="include_nested_group_pkg_header_foo_override_pkg_bar", + package="bar", + is_self=True, + ), + ResultDefault( + config_path="group1/group2/file1", + parent="group1/group_item1_pkg_header_foo", + package="bar.group2", + is_self=False, + ), + ], + id="include_nested_group_global_foo_override_pkg_bar", + ), + param( + "include_nested_group_pkg_header_foo_override_pkg_bar", + ["group1@bar=group_item2"], + [ + ResultDefault( + config_path="include_nested_group_pkg_header_foo_override_pkg_bar", + parent=None, + package="", + is_self=True, + ), + ResultDefault( + config_path="group1/group_item2", + parent="include_nested_group_pkg_header_foo_override_pkg_bar", + package="bar", + is_self=True, + ), + ResultDefault( + config_path="group1/group2/file2", + parent="group1/group_item2", + package="bar.group2", + is_self=False, + ), + ], + id="include_nested_group_global_foo_override_pkg_bar:override_group1", + ), + param( + "include_nested_group_pkg_header_foo_override_pkg_bar", + ["group1/group2@bar.group2=file2"], + [ + ResultDefault( + config_path="include_nested_group_pkg_header_foo_override_pkg_bar", + parent=None, + package="", + is_self=True, + ), + ResultDefault( + config_path="group1/group_item1_pkg_header_foo", + parent="include_nested_group_pkg_header_foo_override_pkg_bar", + package="bar", + is_self=True, + ), + ResultDefault( + config_path="group1/group2/file2", + parent="group1/group_item1_pkg_header_foo", + package="bar.group2", + is_self=False, + ), + ], + id="include_nested_group_global_foo_override_pkg_bar:override_group2", + ), + ], +) +def test_overriding_package_header_from_defaults_list( + config_name: str, overrides: List[str], expected: List[ResultDefault] +): + _test_defaults_list_impl( + config_name=config_name, overrides=overrides, expected=expected + ) diff --git a/tests/test_data/default_lists/include_nested_group_pkg_header_foo_override_pkg_bar.yaml b/tests/test_data/default_lists/include_nested_group_pkg_header_foo_override_pkg_bar.yaml new file mode 100644 index 00000000000..aee173f9518 --- /dev/null +++ b/tests/test_data/default_lists/include_nested_group_pkg_header_foo_override_pkg_bar.yaml @@ -0,0 +1,2 @@ +defaults: + - group1@bar: group_item1_pkg_header_foo \ No newline at end of file From 9225beb6e8f3cd729b8769e01a574e8e0905f23f Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Sun, 29 Nov 2020 11:49:25 -0800 Subject: [PATCH 26/91] Marked override in defaults list - same level --- hydra/_internal/new_defaults_list.py | 20 +++++++++++++- hydra/core/new_default_element.py | 2 ++ hydra/plugins/config_source.py | 12 ++++----- tests/defaults_list/test_defaults_tree.py | 27 +++++++++++++++++++ .../default_lists/override_same_level.yaml | 4 +++ 5 files changed, 58 insertions(+), 7 deletions(-) create mode 100644 tests/test_data/default_lists/override_same_level.yaml diff --git a/hydra/_internal/new_defaults_list.py b/hydra/_internal/new_defaults_list.py index 5445a4b96ec..5dd33f02d8f 100644 --- a/hydra/_internal/new_defaults_list.py +++ b/hydra/_internal/new_defaults_list.py @@ -54,6 +54,12 @@ def __init__(self, repo: IConfigRepository, overrides_list: List[Override]) -> N self.override_choices[key] = value self.override_used[key] = False + def add_override(self, default: GroupDefault) -> None: + assert default.override + key = default.get_override_key() + self.override_choices[key] = default.get_name() + self.override_used[key] = False + def is_overridden(self, default: InputDefault) -> bool: if isinstance(default, GroupDefault): # TODO: collect overridable keys for help/useful error purposes @@ -142,12 +148,12 @@ def _create_defaults_tree( path = parent.get_config_path() loaded = repo.load_config(config_path=path, is_primary_config=is_primary_config) - # TODO: test case where the config option is overridden and the newly loaded config has a different package header. if loaded is None: missing_config_error(repo, root.node) else: defaults_list = copy.deepcopy(loaded.new_defaults_list) + if is_primary_config: for d in overrides.append_group_defaults: defaults_list.append(d) @@ -155,6 +161,15 @@ def _create_defaults_tree( if len(defaults_list) > 0: _validate_self(containing_node=parent, defaults=defaults_list) + for d in defaults_list: + if d.is_self(): + continue + d.parent_base_dir = parent.get_group_path() + d.parent_package = parent.get_final_package() + + if isinstance(d, GroupDefault) and d.override: + overrides.add_override(d) + children = [] for d in defaults_list: if d.is_self(): @@ -162,9 +177,12 @@ def _create_defaults_tree( d.parent_package = root.node.get_package() children.append(d) else: + if isinstance(d, GroupDefault) and d.override: + continue new_root = DefaultsTreeNode(node=d, parent=root) d.parent_base_dir = parent.get_group_path() d.parent_package = parent.get_final_package() + new_root.parent_base_dir = d.get_group_path() subtree = _create_defaults_tree( repo=repo, diff --git a/hydra/core/new_default_element.py b/hydra/core/new_default_element.py index 5d8b12d5a1d..3935c232e8a 100644 --- a/hydra/core/new_default_element.py +++ b/hydra/core/new_default_element.py @@ -156,6 +156,8 @@ class GroupDefault(InputDefault): optional: bool = False package: Optional[str] = None + override: bool = False + parent_base_dir: Optional[str] = field(default=None, compare=False, repr=False) config_name_overridden: bool = field(default=False, compare=False, repr=False) package_header: Optional[str] = field(default=None, compare=False) diff --git a/hydra/plugins/config_source.py b/hydra/plugins/config_source.py index b79bdc5b7cf..f968ff9432d 100644 --- a/hydra/plugins/config_source.py +++ b/hydra/plugins/config_source.py @@ -304,9 +304,8 @@ def _create_defaults_list( res: List[DefaultElement] = [] for item in defaults: if isinstance(item, DictConfig): - optional = False - if "optional" in item: - optional = item.pop("optional") + optional = item.pop("optional", False) + item.pop("override", False) keys = list(item.keys()) if len(keys) > 1: raise ValueError(f"Too many keys in default item {item}") @@ -393,9 +392,9 @@ def _create_new_defaults_list( res: List[DefaultElement] = [] for item in defaults: if isinstance(item, DictConfig): - optional = False - if "optional" in item: - optional = item.pop("optional") + optional = item.pop("optional", False) + override = item.pop("override", False) + keys = list(item.keys()) if len(keys) > 1: raise ValueError(f"Too many keys in default item {item}") @@ -412,6 +411,7 @@ def _create_new_defaults_list( name=config_name, package=package, optional=optional, + override=override, ) elif isinstance(item, str): path, package, _package2 = ConfigSource._split_group(item) diff --git a/tests/defaults_list/test_defaults_tree.py b/tests/defaults_list/test_defaults_tree.py index ea15a52f869..8334fb590b6 100644 --- a/tests/defaults_list/test_defaults_tree.py +++ b/tests/defaults_list/test_defaults_tree.py @@ -465,3 +465,30 @@ def test_defaults_tree_with_package_overrides__group_override( _test_defaults_tree_impl( config_name=config_name, input_overrides=overrides, expected=expected ) + + +@mark.parametrize( + "config_name, overrides, expected", + [ + param( + "override_same_level", + [], + DefaultsTreeNode( + node=ConfigDefault(path="override_same_level"), + children=[ + ConfigDefault(path="_self_"), + GroupDefault(group="group1", name="file2"), + ], + ), + id="override_same_level", + ), + ], +) +def test_override_option_from_defaults_list( + config_name: str, + overrides: List[str], + expected: DefaultsTreeNode, +): + _test_defaults_tree_impl( + config_name=config_name, input_overrides=overrides, expected=expected + ) diff --git a/tests/test_data/default_lists/override_same_level.yaml b/tests/test_data/default_lists/override_same_level.yaml new file mode 100644 index 00000000000..add190de73c --- /dev/null +++ b/tests/test_data/default_lists/override_same_level.yaml @@ -0,0 +1,4 @@ +defaults: + - group1: file1 + - group1: file2 + override: true \ No newline at end of file From 7dfd9c5e2b262b706dc94b802ce487bebddcd356 Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Sun, 29 Nov 2020 15:14:11 -0800 Subject: [PATCH 27/91] Test for overriding nested config groups from parent --- tests/defaults_list/test_defaults_list.py | 8 +++- tests/defaults_list/test_defaults_tree.py | 42 +++++++++++++++++++ .../group1/override_same_level.yaml | 4 ++ .../include_override_same_level.yaml | 2 + .../override_nested_group_item.yaml | 4 ++ 5 files changed, 58 insertions(+), 2 deletions(-) create mode 100644 tests/test_data/default_lists/group1/override_same_level.yaml create mode 100644 tests/test_data/default_lists/include_override_same_level.yaml create mode 100644 tests/test_data/default_lists/override_nested_group_item.yaml diff --git a/tests/defaults_list/test_defaults_list.py b/tests/defaults_list/test_defaults_list.py index be6f7e22cbc..dfcea8d8589 100644 --- a/tests/defaults_list/test_defaults_list.py +++ b/tests/defaults_list/test_defaults_list.py @@ -33,10 +33,14 @@ # (Y) _name_ # TODO: (Y) test with config header package override # TODO: (Y) test with both config header and defaults list pkg override -# TODO: handle hydra overrides +# TODO: Support overriding config group values from the defaults list # - (X) reconsider support for overriding as before -# - Support marked overrides in primary config only +# - (Y) Support marked overrides in primary config only # - Support marked override in all configs +# - Test overriding of config groups with a specified package (@pkg) +# - Error handling for duplicate entries in result list +# - Error handling for entries that failed to override anything +# - Handle hydra overrides # TODO: test handling missing configs mentioned in defaults list (with and without optional) # TODO: test overriding configs in absolute location # TODO: test duplicate _self_ error diff --git a/tests/defaults_list/test_defaults_tree.py b/tests/defaults_list/test_defaults_tree.py index 8334fb590b6..56a8bb3c785 100644 --- a/tests/defaults_list/test_defaults_tree.py +++ b/tests/defaults_list/test_defaults_tree.py @@ -482,6 +482,48 @@ def test_defaults_tree_with_package_overrides__group_override( ), id="override_same_level", ), + param( + "include_override_same_level", + [], + DefaultsTreeNode( + node=ConfigDefault(path="include_override_same_level"), + children=[ + ConfigDefault(path="_self_"), + DefaultsTreeNode( + node=GroupDefault( + group="group1", + name="override_same_level", + ), + children=[ + ConfigDefault(path="_self_"), + GroupDefault( + group="group2", + name="file2", + ), + ], + ), + ], + ), + id="include_override_same_level", + ), + param( + "override_nested_group_item", + [], + DefaultsTreeNode( + node=ConfigDefault(path="override_nested_group_item"), + children=[ + ConfigDefault(path="_self_"), + DefaultsTreeNode( + node=GroupDefault(group="group1", name="group_item1"), + children=[ + ConfigDefault(path="_self_"), + GroupDefault(group="group2", name="file2"), + ], + ), + ], + ), + id="override_nested_group_item", + ), ], ) def test_override_option_from_defaults_list( diff --git a/tests/test_data/default_lists/group1/override_same_level.yaml b/tests/test_data/default_lists/group1/override_same_level.yaml new file mode 100644 index 00000000000..497e19d67a0 --- /dev/null +++ b/tests/test_data/default_lists/group1/override_same_level.yaml @@ -0,0 +1,4 @@ +defaults: + - group2: file1 + - group2: file2 + override: true \ No newline at end of file diff --git a/tests/test_data/default_lists/include_override_same_level.yaml b/tests/test_data/default_lists/include_override_same_level.yaml new file mode 100644 index 00000000000..06673b58089 --- /dev/null +++ b/tests/test_data/default_lists/include_override_same_level.yaml @@ -0,0 +1,2 @@ +defaults: + - group1: override_same_level \ No newline at end of file diff --git a/tests/test_data/default_lists/override_nested_group_item.yaml b/tests/test_data/default_lists/override_nested_group_item.yaml new file mode 100644 index 00000000000..eb07bb85a04 --- /dev/null +++ b/tests/test_data/default_lists/override_nested_group_item.yaml @@ -0,0 +1,4 @@ +defaults: + - group1: group_item1 + - group1/group2: file2 + override: true \ No newline at end of file From 00be420146c5f2342a76b3fa8a1c35bee9aa3503 Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Sun, 29 Nov 2020 17:53:11 -0800 Subject: [PATCH 28/91] testing external override of overrides in defaults list --- hydra/_internal/new_defaults_list.py | 10 ++-- tests/defaults_list/test_defaults_list.py | 7 +-- tests/defaults_list/test_defaults_tree.py | 54 +++++++++++++++++++ .../default_lists/group1/group2/file3.yaml | 0 4 files changed, 64 insertions(+), 7 deletions(-) create mode 100644 tests/test_data/default_lists/group1/group2/file3.yaml diff --git a/hydra/_internal/new_defaults_list.py b/hydra/_internal/new_defaults_list.py index 5dd33f02d8f..d6514dcea45 100644 --- a/hydra/_internal/new_defaults_list.py +++ b/hydra/_internal/new_defaults_list.py @@ -57,8 +57,9 @@ def __init__(self, repo: IConfigRepository, overrides_list: List[Override]) -> N def add_override(self, default: GroupDefault) -> None: assert default.override key = default.get_override_key() - self.override_choices[key] = default.get_name() - self.override_used[key] = False + if key not in self.override_choices: + self.override_choices[key] = default.get_name() + self.override_used[key] = False def is_overridden(self, default: InputDefault) -> bool: if isinstance(default, GroupDefault): @@ -275,10 +276,11 @@ def create_defaults_list( def missing_config_error(repo: IConfigRepository, element: InputDefault) -> None: options = None if isinstance(element, GroupDefault): - options = repo.get_group_options(element.group, ObjectType.CONFIG) + group = element.get_group_path() + options = repo.get_group_options(group, ObjectType.CONFIG) opt_list = "\n".join(["\t" + x for x in options]) msg = ( - f"Could not find '{element.name}' in the config group '{element.get_group_path()}'" + f"Could not find '{element.name}' in the config group '{group}'" f"\nAvailable options:\n{opt_list}\n" ) else: diff --git a/tests/defaults_list/test_defaults_list.py b/tests/defaults_list/test_defaults_list.py index dfcea8d8589..99c833e0e6b 100644 --- a/tests/defaults_list/test_defaults_list.py +++ b/tests/defaults_list/test_defaults_list.py @@ -36,9 +36,10 @@ # TODO: Support overriding config group values from the defaults list # - (X) reconsider support for overriding as before # - (Y) Support marked overrides in primary config only -# - Support marked override in all configs -# - Test overriding of config groups with a specified package (@pkg) -# - Error handling for duplicate entries in result list +# - (Y) Support marked override in all configs +# - (Y) Test overriding of config groups with a specified package (@pkg) +# - Overriding of config groups with a specified package (@pkg) when there are multiple choices from the same group +# - Error handling for duplicate entries in result list (suggest candidates) # - Error handling for entries that failed to override anything # - Handle hydra overrides # TODO: test handling missing configs mentioned in defaults list (with and without optional) diff --git a/tests/defaults_list/test_defaults_tree.py b/tests/defaults_list/test_defaults_tree.py index 56a8bb3c785..474880fd747 100644 --- a/tests/defaults_list/test_defaults_tree.py +++ b/tests/defaults_list/test_defaults_tree.py @@ -482,6 +482,18 @@ def test_defaults_tree_with_package_overrides__group_override( ), id="override_same_level", ), + param( + "override_same_level", + ["group1=file3"], + DefaultsTreeNode( + node=ConfigDefault(path="override_same_level"), + children=[ + ConfigDefault(path="_self_"), + GroupDefault(group="group1", name="file3"), + ], + ), + id="override_same_level:external_override", + ), param( "include_override_same_level", [], @@ -506,6 +518,30 @@ def test_defaults_tree_with_package_overrides__group_override( ), id="include_override_same_level", ), + param( + "include_override_same_level", + ["group1/group2=file3"], + DefaultsTreeNode( + node=ConfigDefault(path="include_override_same_level"), + children=[ + ConfigDefault(path="_self_"), + DefaultsTreeNode( + node=GroupDefault( + group="group1", + name="override_same_level", + ), + children=[ + ConfigDefault(path="_self_"), + GroupDefault( + group="group2", + name="file3", + ), + ], + ), + ], + ), + id="include_override_same_level:external_override", + ), param( "override_nested_group_item", [], @@ -524,6 +560,24 @@ def test_defaults_tree_with_package_overrides__group_override( ), id="override_nested_group_item", ), + param( + "override_nested_group_item", + ["group1/group2=file3"], + DefaultsTreeNode( + node=ConfigDefault(path="override_nested_group_item"), + children=[ + ConfigDefault(path="_self_"), + DefaultsTreeNode( + node=GroupDefault(group="group1", name="group_item1"), + children=[ + ConfigDefault(path="_self_"), + GroupDefault(group="group2", name="file3"), + ], + ), + ], + ), + id="override_nested_group_item:external_override", + ), ], ) def test_override_option_from_defaults_list( diff --git a/tests/test_data/default_lists/group1/group2/file3.yaml b/tests/test_data/default_lists/group1/group2/file3.yaml new file mode 100644 index 00000000000..e69de29bb2d From ea7c2e584fd9029992ab80da4a3fe14ea2ac995f Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Mon, 30 Nov 2020 11:16:41 -0800 Subject: [PATCH 29/91] testing two_group_defaults_different_pkgs --- hydra/core/new_default_element.py | 43 +++++++++++++++++- tests/defaults_list/test_defaults_list.py | 6 +-- tests/defaults_list/test_defaults_tree.py | 54 +++++++++++++++++++++++ 3 files changed, 98 insertions(+), 5 deletions(-) diff --git a/hydra/core/new_default_element.py b/hydra/core/new_default_element.py index 3935c232e8a..a195d449ae7 100644 --- a/hydra/core/new_default_element.py +++ b/hydra/core/new_default_element.py @@ -33,6 +33,12 @@ def _relative_group_path(self) -> str: def get_name(self) -> str: raise NotImplementedError() + def _get_attributes(self) -> List[str]: + raise NotImplementedError() + + def _get_flags(self) -> List[str]: + raise NotImplementedError() + def set_package_header(self, package_header: str) -> None: assert self.__dict__["package_header"] is None # package header is always interpreted as absolute. @@ -87,8 +93,29 @@ def get_override_key(self) -> str: key = f"{key}@{final_pkg}" return key + def __repr__(self) -> str: + attrs = [] + attr_names = self._get_attributes() + for attr in attr_names: + value = getattr(self, attr) + if value is not None: + attrs.append(f'{attr}="{value}"') -@dataclass + flags = [] + flag_names = self._get_flags() + for flag in flag_names: + value = getattr(self, flag) + if value: + flags.append(f"{flag}=True") + + ret = f"{','.join(attrs)}" + + if len(flags) > 0: + ret = f"{ret} ({','.join(flags)})" + return f"{type(self).__name__}({ret})" + + +@dataclass(repr=False) class ConfigDefault(InputDefault): path: str package: Optional[str] = None @@ -146,8 +173,14 @@ def _relative_group_path(self) -> str: else: return self.path[0:idx] + def _get_attributes(self) -> List[str]: + return ["path", "package"] -@dataclass + def _get_flags(self) -> List[str]: + return [] + + +@dataclass(repr=False) class GroupDefault(InputDefault): # config group name if present group: Optional[str] = None @@ -193,6 +226,12 @@ def get_final_package(self) -> str: def _relative_group_path(self) -> str: return self.group + def _get_attributes(self) -> List[str]: + return ["group", "name", "package"] + + def _get_flags(self) -> List[str]: + return ["optional", "override"] + @dataclass class DefaultsTreeNode: diff --git a/tests/defaults_list/test_defaults_list.py b/tests/defaults_list/test_defaults_list.py index 99c833e0e6b..5f9326e1057 100644 --- a/tests/defaults_list/test_defaults_list.py +++ b/tests/defaults_list/test_defaults_list.py @@ -38,19 +38,19 @@ # - (Y) Support marked overrides in primary config only # - (Y) Support marked override in all configs # - (Y) Test overriding of config groups with a specified package (@pkg) -# - Overriding of config groups with a specified package (@pkg) when there are multiple choices from the same group +# - (Y) Overriding of config groups with a specified package (@pkg) when there are multiple choices from same group # - Error handling for duplicate entries in result list (suggest candidates) # - Error handling for entries that failed to override anything # - Handle hydra overrides # TODO: test handling missing configs mentioned in defaults list (with and without optional) # TODO: test overriding configs in absolute location # TODO: test duplicate _self_ error -# TODO: test duplicates in result config list # TODO: Interpolation support # TODO: package header: # - consider making relative # - consider deprecating completely - +# TODO: Consider delete support +# TODO: Consider package rename support # TODO: update documentation diff --git a/tests/defaults_list/test_defaults_tree.py b/tests/defaults_list/test_defaults_tree.py index 474880fd747..15c7373cdd5 100644 --- a/tests/defaults_list/test_defaults_tree.py +++ b/tests/defaults_list/test_defaults_tree.py @@ -588,3 +588,57 @@ def test_override_option_from_defaults_list( _test_defaults_tree_impl( config_name=config_name, input_overrides=overrides, expected=expected ) + + +@mark.parametrize( + "config_name, overrides, expected", + [ + param( + "two_group_defaults_different_pkgs", + [], + DefaultsTreeNode( + node=ConfigDefault(path="two_group_defaults_different_pkgs"), + children=[ + ConfigDefault(path="_self_"), + GroupDefault(group="group1", name="file1", package="pkg1"), + GroupDefault(group="group1", name="file1", package="pkg2"), + ], + ), + id="two_group_defaults_different_pkgs", + ), + param( + "two_group_defaults_different_pkgs", + ["group1@pkg1=file2"], + DefaultsTreeNode( + node=ConfigDefault(path="two_group_defaults_different_pkgs"), + children=[ + ConfigDefault(path="_self_"), + GroupDefault(group="group1", name="file2", package="pkg1"), + GroupDefault(group="group1", name="file1", package="pkg2"), + ], + ), + id="two_group_defaults_different_pkgs:override_first", + ), + param( + "two_group_defaults_different_pkgs", + ["group1@pkg2=file2"], + DefaultsTreeNode( + node=ConfigDefault(path="two_group_defaults_different_pkgs"), + children=[ + ConfigDefault(path="_self_"), + GroupDefault(group="group1", name="file1", package="pkg1"), + GroupDefault(group="group1", name="file2", package="pkg2"), + ], + ), + id="two_group_defaults_different_pkgs:override_second", + ), + ], +) +def test_two_group_defaults_different_pkgs( + config_name: str, + overrides: List[str], + expected: DefaultsTreeNode, +) -> None: + _test_defaults_tree_impl( + config_name=config_name, input_overrides=overrides, expected=expected + ) From f8c99343a51bb630bddae576da525ef20b88ee65 Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Mon, 30 Nov 2020 12:30:29 -0800 Subject: [PATCH 30/91] Support for adding hydra/config node --- hydra/_internal/new_defaults_list.py | 95 +++++++++++++------ hydra/core/new_default_element.py | 39 ++++++++ tests/defaults_list/__init__.py | 6 +- tests/defaults_list/test_defaults_list.py | 54 ++++++++++- tests/defaults_list/test_defaults_tree.py | 39 +++++++- .../test_data/default_lists/hydra/config.yaml | 3 + .../default_lists/hydra/help/custom1.yaml | 0 .../default_lists/hydra/help/default.yaml | 0 .../default_lists/hydra/output/default.yaml | 1 + .../two_group_defaults_different_pkgs.yaml | 3 + 10 files changed, 205 insertions(+), 35 deletions(-) create mode 100644 tests/test_data/default_lists/hydra/config.yaml create mode 100644 tests/test_data/default_lists/hydra/help/custom1.yaml create mode 100644 tests/test_data/default_lists/hydra/help/default.yaml create mode 100644 tests/test_data/default_lists/hydra/output/default.yaml create mode 100644 tests/test_data/default_lists/two_group_defaults_different_pkgs.yaml diff --git a/hydra/_internal/new_defaults_list.py b/hydra/_internal/new_defaults_list.py index d6514dcea45..10e94b33e8a 100644 --- a/hydra/_internal/new_defaults_list.py +++ b/hydra/_internal/new_defaults_list.py @@ -11,6 +11,7 @@ GroupDefault, InputDefault, ResultDefault, + VirtualRoot, ) from hydra.core.object_type import ObjectType from hydra.core.override_parser.types import Override @@ -126,33 +127,52 @@ def _create_defaults_tree( is_primary_config: bool, overrides: Overrides, ) -> DefaultsTreeNode: - assert root.children is None - - if is_primary_config: - root.node.parent_base_dir = "" - root.node.parent_package = "" - parent = root.node + children = [] + if parent.is_virtual(): + for d in root.children: + assert isinstance(d, ConfigDefault) + new_root = DefaultsTreeNode(node=d, parent=root) + d.parent_base_dir = "" + d.parent_package = "" + + new_root.parent_base_dir = d.get_group_path() + subtree = _create_defaults_tree( + repo=repo, + root=new_root, + is_primary_config=False, + overrides=overrides, + ) + if subtree.children is None: + children.append(d) + else: + children.append(subtree) + else: + if is_primary_config: + root.node.parent_base_dir = "" + root.node.parent_package = "" - update_package_header(repo=repo, node=parent, is_primary_config=is_primary_config) + update_package_header( + repo=repo, node=parent, is_primary_config=is_primary_config + ) - if isinstance(parent, GroupDefault): - if overrides.is_overridden(parent): - overrides.override_default_option(parent) - # clear package header and obtain updated one from overridden config - # (for the rare case it has changed) - parent.package_header = None - update_package_header( - repo=repo, node=parent, is_primary_config=is_primary_config - ) + if isinstance(parent, GroupDefault): + if overrides.is_overridden(parent): + overrides.override_default_option(parent) + # clear package header and obtain updated one from overridden config + # (for the rare case it has changed) + parent.package_header = None + update_package_header( + repo=repo, node=parent, is_primary_config=is_primary_config + ) - path = parent.get_config_path() + path = parent.get_config_path() - loaded = repo.load_config(config_path=path, is_primary_config=is_primary_config) + loaded = repo.load_config(config_path=path, is_primary_config=is_primary_config) + + if loaded is None: + missing_config_error(repo, root.node) - if loaded is None: - missing_config_error(repo, root.node) - else: defaults_list = copy.deepcopy(loaded.new_defaults_list) if is_primary_config: @@ -171,7 +191,6 @@ def _create_defaults_tree( if isinstance(d, GroupDefault) and d.override: overrides.add_override(d) - children = [] for d in defaults_list: if d.is_self(): d.parent_base_dir = root.node.parent_base_dir @@ -196,8 +215,9 @@ def _create_defaults_tree( else: children.append(subtree) - if len(children) > 0: - root.children = children + if len(children) > 0: + root.children = children + return root @@ -240,13 +260,29 @@ def _tree_to_list( _tree_to_list(tree=child, output=output) +def _create_root(config_name: str, with_hydra: bool) -> DefaultsTreeNode: + if with_hydra: + root = DefaultsTreeNode( + node=VirtualRoot(), + children=[ + ConfigDefault(path="hydra/config"), + ConfigDefault(path=config_name), + ], + ) + else: + root = DefaultsTreeNode(node=ConfigDefault(path=config_name)) + return root + + def _create_defaults_list( repo: IConfigRepository, config_name: str, overrides: Overrides, + prepend_hydra: bool, ) -> List[ResultDefault]: - primary = ConfigDefault(path=config_name) - root = DefaultsTreeNode(node=primary) + + root = _create_root(config_name=config_name, with_hydra=prepend_hydra) + defaults_tree = _create_defaults_tree( repo=repo, root=root, @@ -264,9 +300,12 @@ def create_defaults_list( repo: IConfigRepository, config_name: str, overrides_list: List[Override], + prepend_hydra: bool, ) -> DefaultsList: overrides = Overrides(repo=repo, overrides_list=overrides_list) - defaults = _create_defaults_list(repo, config_name, overrides) + defaults = _create_defaults_list( + repo, config_name, overrides, prepend_hydra=prepend_hydra + ) overrides.ensure_overrides_used() ret = DefaultsList(defaults=defaults, config_overrides=overrides.config_overrides) return ret @@ -286,7 +325,7 @@ def missing_config_error(repo: IConfigRepository, element: InputDefault) -> None else: msg = dedent( f"""\ - Could not load {element.get_config_path()}. + Could not load '{element.get_config_path()}'. """ ) diff --git a/hydra/core/new_default_element.py b/hydra/core/new_default_element.py index a195d449ae7..46989f69072 100644 --- a/hydra/core/new_default_element.py +++ b/hydra/core/new_default_element.py @@ -39,6 +39,9 @@ def _get_attributes(self) -> List[str]: def _get_flags(self) -> List[str]: raise NotImplementedError() + def is_virtual(self) -> bool: + return False + def set_package_header(self, package_header: str) -> None: assert self.__dict__["package_header"] is None # package header is always interpreted as absolute. @@ -115,6 +118,42 @@ def __repr__(self) -> str: return f"{type(self).__name__}({ret})" +@dataclass +class VirtualRoot(InputDefault): + def is_virtual(self) -> bool: + return True + + def is_self(self) -> bool: + raise NotImplementedError() + + def get_group_path(self) -> str: + raise NotImplementedError() + + def get_config_path(self) -> str: + return "" + + def get_default_package(self) -> str: + return self.get_group_path().replace("/", ".") + + def get_final_package(self) -> str: + raise NotImplementedError() + + def _relative_group_path(self) -> str: + raise NotImplementedError() + + def get_name(self) -> str: + raise NotImplementedError() + + def _get_attributes(self) -> List[str]: + raise NotImplementedError() + + def _get_flags(self) -> List[str]: + raise NotImplementedError() + + def __repr__(self) -> str: + return "ROOT" + + @dataclass(repr=False) class ConfigDefault(InputDefault): path: str diff --git a/tests/defaults_list/__init__.py b/tests/defaults_list/__init__.py index aa114ebf04f..b25b0859098 100644 --- a/tests/defaults_list/__init__.py +++ b/tests/defaults_list/__init__.py @@ -7,8 +7,8 @@ DefaultsTreeNode, _create_defaults_tree, Overrides, + _create_root, ) -from hydra.core.new_default_element import ConfigDefault from hydra.core.override_parser.overrides_parser import OverridesParser @@ -22,11 +22,11 @@ def _test_defaults_tree_impl( config_name: str, input_overrides: List[str], expected: Any, + prepend_hydra: bool = False, ) -> None: parser = OverridesParser.create() repo = create_repo() - parent = ConfigDefault(path=config_name) - root = DefaultsTreeNode(node=parent) + root = _create_root(config_name=config_name, with_hydra=prepend_hydra) overrides_list = parser.parse_overrides(overrides=input_overrides) overrides = Overrides(repo=repo, overrides_list=overrides_list) diff --git a/tests/defaults_list/test_defaults_list.py b/tests/defaults_list/test_defaults_list.py index 5f9326e1057..6803efe40e0 100644 --- a/tests/defaults_list/test_defaults_list.py +++ b/tests/defaults_list/test_defaults_list.py @@ -39,10 +39,7 @@ # - (Y) Support marked override in all configs # - (Y) Test overriding of config groups with a specified package (@pkg) # - (Y) Overriding of config groups with a specified package (@pkg) when there are multiple choices from same group -# - Error handling for duplicate entries in result list (suggest candidates) -# - Error handling for entries that failed to override anything # - Handle hydra overrides -# TODO: test handling missing configs mentioned in defaults list (with and without optional) # TODO: test overriding configs in absolute location # TODO: test duplicate _self_ error # TODO: Interpolation support @@ -51,6 +48,14 @@ # - consider deprecating completely # TODO: Consider delete support # TODO: Consider package rename support + +# Error handling: +# TODO: (Y) Error handling for entries that failed to override anything +# TODO: test handling missing configs mentioned in defaults list (with and without optional) +# TODO: Ambiguous overrides should provide valid override keys for group +# TODO: Should duplicate entries in results list be an error? (same override key) + +# Documentation # TODO: update documentation @@ -110,6 +115,7 @@ def _test_defaults_list_impl( config_name: str, overrides: List[str], expected: Any, + prepend_hydra: bool = False, ) -> None: parser = OverridesParser.create() repo = create_repo() @@ -119,6 +125,7 @@ def _test_defaults_list_impl( repo=repo, config_name=config_name, overrides_list=overrides_list, + prepend_hydra=prepend_hydra, ) assert result.defaults == expected else: @@ -127,6 +134,7 @@ def _test_defaults_list_impl( repo=repo, config_name=config_name, overrides_list=overrides_list, + prepend_hydra=prepend_hydra, ) @@ -1004,3 +1012,43 @@ def test_overriding_package_header_from_defaults_list( _test_defaults_list_impl( config_name=config_name, overrides=overrides, expected=expected ) + + +@mark.parametrize( + "config_name, overrides, expected", + [ + param( + "empty", + [], + [ + ResultDefault( + config_path="hydra/config", + parent="", + package="hydra", + is_self=True, + ), + ResultDefault( + config_path="hydra/help/default", + parent="hydra/config", + package="hydra.help", + ), + ResultDefault( + config_path="hydra/output/default", + parent="hydra/config", + package="hydra", + ), + ResultDefault(config_path="empty", parent="", package=""), + ], + id="just_hydra_config", + ), + ], +) +def test_with_hydra_config( + config_name: str, overrides: List[str], expected: List[ResultDefault] +): + _test_defaults_list_impl( + config_name=config_name, + overrides=overrides, + expected=expected, + prepend_hydra=True, + ) diff --git a/tests/defaults_list/test_defaults_tree.py b/tests/defaults_list/test_defaults_tree.py index 15c7373cdd5..6c99df296f4 100644 --- a/tests/defaults_list/test_defaults_tree.py +++ b/tests/defaults_list/test_defaults_tree.py @@ -9,7 +9,7 @@ from hydra._internal.new_defaults_list import ( DefaultsTreeNode, ) -from hydra.core.new_default_element import GroupDefault, ConfigDefault +from hydra.core.new_default_element import GroupDefault, ConfigDefault, VirtualRoot from hydra.core.plugins import Plugins from hydra.errors import ConfigCompositionException from hydra.test_utils.test_utils import chdir_hydra_root @@ -642,3 +642,40 @@ def test_two_group_defaults_different_pkgs( _test_defaults_tree_impl( config_name=config_name, input_overrides=overrides, expected=expected ) + + +@mark.parametrize( + "config_name, overrides, expected", + [ + param( + "empty", + [], + DefaultsTreeNode( + node=VirtualRoot(), + children=[ + DefaultsTreeNode( + node=ConfigDefault(path="hydra/config"), + children=[ + ConfigDefault(path="_self_"), + GroupDefault(group="help", name="default"), + GroupDefault(group="output", name="default"), + ], + ), + ConfigDefault(path="empty"), + ], + ), + id="empty", + ) + ], +) +def test_legacy_hydra_overrides_from_primary_config( + config_name: str, + overrides: List[str], + expected: DefaultsTreeNode, +) -> None: + _test_defaults_tree_impl( + config_name=config_name, + input_overrides=overrides, + expected=expected, + prepend_hydra=True, + ) diff --git a/tests/test_data/default_lists/hydra/config.yaml b/tests/test_data/default_lists/hydra/config.yaml new file mode 100644 index 00000000000..669e65994af --- /dev/null +++ b/tests/test_data/default_lists/hydra/config.yaml @@ -0,0 +1,3 @@ +defaults: + - help: default + - output: default \ No newline at end of file diff --git a/tests/test_data/default_lists/hydra/help/custom1.yaml b/tests/test_data/default_lists/hydra/help/custom1.yaml new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/test_data/default_lists/hydra/help/default.yaml b/tests/test_data/default_lists/hydra/help/default.yaml new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/test_data/default_lists/hydra/output/default.yaml b/tests/test_data/default_lists/hydra/output/default.yaml new file mode 100644 index 00000000000..c9ff67af92d --- /dev/null +++ b/tests/test_data/default_lists/hydra/output/default.yaml @@ -0,0 +1 @@ +# @package hydra diff --git a/tests/test_data/default_lists/two_group_defaults_different_pkgs.yaml b/tests/test_data/default_lists/two_group_defaults_different_pkgs.yaml new file mode 100644 index 00000000000..91429237fd6 --- /dev/null +++ b/tests/test_data/default_lists/two_group_defaults_different_pkgs.yaml @@ -0,0 +1,3 @@ +defaults: + - group1@pkg1: file1 + - group1@pkg2: file1 From 1c83e4c07026e9f518c02a85eb7b216f5723aca0 Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Mon, 30 Nov 2020 14:46:30 -0800 Subject: [PATCH 31/91] Handling legacy hydra overrides in defaults list --- hydra/_internal/new_defaults_list.py | 88 ++++++++++++------- hydra/core/new_default_element.py | 2 +- tests/defaults_list/test_defaults_list.py | 43 ++++++++- tests/defaults_list/test_defaults_tree.py | 51 ++++++++++- .../default_lists/hydra/help/custom2.yaml | 0 .../default_lists/override_hydra.yaml | 2 + 6 files changed, 148 insertions(+), 38 deletions(-) create mode 100644 tests/test_data/default_lists/hydra/help/custom2.yaml create mode 100644 tests/test_data/default_lists/override_hydra.yaml diff --git a/hydra/_internal/new_defaults_list.py b/hydra/_internal/new_defaults_list.py index 10e94b33e8a..487d0bf8505 100644 --- a/hydra/_internal/new_defaults_list.py +++ b/hydra/_internal/new_defaults_list.py @@ -1,4 +1,5 @@ import copy +import warnings from dataclasses import dataclass from textwrap import dedent from typing import Dict, List @@ -121,6 +122,36 @@ def update_package_header( node.set_package_header(loaded.header["orig_package"]) +def _expand_virtual_root( + repo: IConfigRepository, + root: DefaultsTreeNode, + overrides: Overrides, +) -> DefaultsTreeNode: + children = [] + for d in reversed(root.children): + assert isinstance(d, ConfigDefault) + new_root = DefaultsTreeNode(node=d, parent=root) + d.parent_base_dir = "" + d.parent_package = "" + + new_root.parent_base_dir = d.get_group_path() + subtree = _create_defaults_tree( + repo=repo, + root=new_root, + is_primary_config=False, + overrides=overrides, + ) + if subtree.children is None: + children.append(d) + else: + children.append(subtree) + + if len(children) > 0: + root.children = list(reversed(children)) + + return root + + def _create_defaults_tree( repo: IConfigRepository, root: DefaultsTreeNode, @@ -130,23 +161,7 @@ def _create_defaults_tree( parent = root.node children = [] if parent.is_virtual(): - for d in root.children: - assert isinstance(d, ConfigDefault) - new_root = DefaultsTreeNode(node=d, parent=root) - d.parent_base_dir = "" - d.parent_package = "" - - new_root.parent_base_dir = d.get_group_path() - subtree = _create_defaults_tree( - repo=repo, - root=new_root, - is_primary_config=False, - overrides=overrides, - ) - if subtree.children is None: - children.append(d) - else: - children.append(subtree) + return _expand_virtual_root(repo, root, overrides) else: if is_primary_config: root.node.parent_base_dir = "" @@ -156,15 +171,14 @@ def _create_defaults_tree( repo=repo, node=parent, is_primary_config=is_primary_config ) - if isinstance(parent, GroupDefault): - if overrides.is_overridden(parent): - overrides.override_default_option(parent) - # clear package header and obtain updated one from overridden config - # (for the rare case it has changed) - parent.package_header = None - update_package_header( - repo=repo, node=parent, is_primary_config=is_primary_config - ) + if overrides.is_overridden(parent): + overrides.override_default_option(parent) + # clear package header and obtain updated one from overridden config + # (for the rare case it has changed) + parent.package_header = None + update_package_header( + repo=repo, node=parent, is_primary_config=is_primary_config + ) path = parent.get_config_path() @@ -188,10 +202,24 @@ def _create_defaults_tree( d.parent_base_dir = parent.get_group_path() d.parent_package = parent.get_final_package() - if isinstance(d, GroupDefault) and d.override: - overrides.add_override(d) + if isinstance(d, GroupDefault): + is_legacy_hydra_override = not d.override and d.group.startswith( + "hydra/" + ) + if is_legacy_hydra_override: + d.override = True + url = "https://hydra.cc/docs/next/upgrades/1.0_to_1.1/default_list_override" + msg = dedent( + f"""\ + Default list overrides now requires 'override: true', see {url} for more information. + """ + ) + warnings.warn(msg, UserWarning) - for d in defaults_list: + if d.override: + overrides.add_override(d) + + for d in reversed(defaults_list): if d.is_self(): d.parent_base_dir = root.node.parent_base_dir d.parent_package = root.node.get_package() @@ -216,7 +244,7 @@ def _create_defaults_tree( children.append(subtree) if len(children) > 0: - root.children = children + root.children = list(reversed(children)) return root diff --git a/hydra/core/new_default_element.py b/hydra/core/new_default_element.py index 46989f69072..87c0def295b 100644 --- a/hydra/core/new_default_element.py +++ b/hydra/core/new_default_element.py @@ -151,7 +151,7 @@ def _get_flags(self) -> List[str]: raise NotImplementedError() def __repr__(self) -> str: - return "ROOT" + return "VirtualRoot()" @dataclass(repr=False) diff --git a/tests/defaults_list/test_defaults_list.py b/tests/defaults_list/test_defaults_list.py index 6803efe40e0..4afe38c2807 100644 --- a/tests/defaults_list/test_defaults_list.py +++ b/tests/defaults_list/test_defaults_list.py @@ -34,12 +34,12 @@ # TODO: (Y) test with config header package override # TODO: (Y) test with both config header and defaults list pkg override # TODO: Support overriding config group values from the defaults list -# - (X) reconsider support for overriding as before +# - (Y) reconsider support for overriding as before, DECISION: Not happening. # - (Y) Support marked overrides in primary config only # - (Y) Support marked override in all configs # - (Y) Test overriding of config groups with a specified package (@pkg) # - (Y) Overriding of config groups with a specified package (@pkg) when there are multiple choices from same group -# - Handle hydra overrides +# - (Y) Handle hydra overrides # TODO: test overriding configs in absolute location # TODO: test duplicate _self_ error # TODO: Interpolation support @@ -57,6 +57,7 @@ # Documentation # TODO: update documentation +# TODO: Create https://hydra.cc/docs/next/upgrades/1.0_to_1.1/default_list_override @mark.parametrize( @@ -1015,7 +1016,7 @@ def test_overriding_package_header_from_defaults_list( @mark.parametrize( - "config_name, overrides, expected", + "config_name,overrides,expected", [ param( "empty", @@ -1041,10 +1042,44 @@ def test_overriding_package_header_from_defaults_list( ], id="just_hydra_config", ), + param( + "override_hydra", + [], + [ + ResultDefault( + config_path="hydra/config", + parent="", + package="hydra", + is_self=True, + ), + ResultDefault( + config_path="hydra/help/custom1", + parent="hydra/config", + package="hydra.help", + is_self=False, + ), + ResultDefault( + config_path="hydra/output/default", + parent="hydra/config", + package="hydra", + is_self=False, + ), + ResultDefault( + config_path="override_hydra", + parent="", + package="", + is_self=True, + ), + ], + id="override_hydra", + ), ], ) def test_with_hydra_config( - config_name: str, overrides: List[str], expected: List[ResultDefault] + config_name: str, + overrides: List[str], + expected: List[ResultDefault], + recwarn, # Testing deprecated behavior ): _test_defaults_list_impl( config_name=config_name, diff --git a/tests/defaults_list/test_defaults_tree.py b/tests/defaults_list/test_defaults_tree.py index 6c99df296f4..900c01c617a 100644 --- a/tests/defaults_list/test_defaults_tree.py +++ b/tests/defaults_list/test_defaults_tree.py @@ -645,7 +645,7 @@ def test_two_group_defaults_different_pkgs( @mark.parametrize( - "config_name, overrides, expected", + "config_name,overrides,expected", [ param( "empty", @@ -664,14 +664,59 @@ def test_two_group_defaults_different_pkgs( ConfigDefault(path="empty"), ], ), - id="empty", - ) + id="just_hydra", + ), + param( + "override_hydra", + [], + DefaultsTreeNode( + node=VirtualRoot(), + children=[ + DefaultsTreeNode( + node=ConfigDefault(path="hydra/config"), + children=[ + ConfigDefault(path="_self_"), + GroupDefault(group="help", name="custom1"), + GroupDefault(group="output", name="default"), + ], + ), + DefaultsTreeNode( + node=ConfigDefault(path="override_hydra"), + children=[ConfigDefault(path="_self_")], + ), + ], + ), + id="override_hydra", + ), + param( + "override_hydra", + ["hydra/help=custom2"], + DefaultsTreeNode( + node=VirtualRoot(), + children=[ + DefaultsTreeNode( + node=ConfigDefault(path="hydra/config"), + children=[ + ConfigDefault(path="_self_"), + GroupDefault(group="help", name="custom2"), + GroupDefault(group="output", name="default"), + ], + ), + DefaultsTreeNode( + node=ConfigDefault(path="override_hydra"), + children=[ConfigDefault(path="_self_")], + ), + ], + ), + id="override_hydra+external", + ), ], ) def test_legacy_hydra_overrides_from_primary_config( config_name: str, overrides: List[str], expected: DefaultsTreeNode, + recwarn, # Testing deprecated behavior ) -> None: _test_defaults_tree_impl( config_name=config_name, diff --git a/tests/test_data/default_lists/hydra/help/custom2.yaml b/tests/test_data/default_lists/hydra/help/custom2.yaml new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/test_data/default_lists/override_hydra.yaml b/tests/test_data/default_lists/override_hydra.yaml new file mode 100644 index 00000000000..217ca754709 --- /dev/null +++ b/tests/test_data/default_lists/override_hydra.yaml @@ -0,0 +1,2 @@ +defaults: + - hydra/help: custom1 From 035e722d261cfc255dbd7ede83e33d8d89431850 Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Mon, 30 Nov 2020 17:53:18 -0800 Subject: [PATCH 32/91] Lint passing --- hydra/_internal/hydra.py | 125 +++++++++++----------- hydra/_internal/new_defaults_list.py | 35 +++--- hydra/core/new_default_element.py | 49 ++++++--- hydra/plugins/config_source.py | 14 +-- tests/defaults_list/__init__.py | 8 +- tests/defaults_list/test_defaults_list.py | 68 ++++++------ tests/defaults_list/test_defaults_tree.py | 33 +++--- tools/configen/configen/utils.py | 4 +- 8 files changed, 188 insertions(+), 148 deletions(-) diff --git a/hydra/_internal/hydra.py b/hydra/_internal/hydra.py index 0df57c32fb5..30a0d950de8 100644 --- a/hydra/_internal/hydra.py +++ b/hydra/_internal/hydra.py @@ -31,7 +31,6 @@ from hydra.types import RunMode, TaskFunction from .config_loader_impl import ConfigLoaderImpl -from .defaults_list import expand_defaults_list from .utils import create_automatic_config_search_path log: Optional[logging.Logger] = None @@ -487,66 +486,70 @@ def _print_plugins_profiling_info(self, top_n: int) -> None: self._log_footer(header=header, filler="-") - def _print_defaults_list(self, config_name: Optional[str], overrides: List[str]): - run_mode = RunMode.RUN - ret = self.config_loader.compute_input_defaults_list( - config_name=config_name, - overrides=overrides, - run_mode=RunMode.RUN, - from_shell=True, - ) - - defaults = expand_defaults_list( - defaults=ret.input_defaults, - skip_missing=run_mode == RunMode.RUN, - ignore_config_load_failures=True, - repo=self.config_loader.repository, # type: ignore - ) - - box: List[List[str]] = [ - [ - "Config group", - "Config name", - "Package", - "Parent", - "Skip reason", - ] - ] - for d in defaults: - row = [ - d.config_group, - d.config_name, - d.package, - d.parent, - d.skip_load_reason if d.skip_load else "", - ] - row = [x if x is not None else "" for x in row] - box.append(row) - padding = get_column_widths(box) - del box[0] - log.debug("") - self._log_header("Defaults List", filler="*") - header = "| {} | {} | {} | {} | {} | ".format( - "Config group".ljust(padding[0]), - "Config name".ljust(padding[1]), - "Package".ljust(padding[2]), - "Parent".ljust(padding[3]), - "Skip reason".ljust(padding[4]), - ) - self._log_header(header=header, filler="-") - - for row in box: - log.debug( - "| {} | {} | {} | {} | {} |".format( - row[0].ljust(padding[0]), - row[1].ljust(padding[1]), - row[2].ljust(padding[2]), - row[3].ljust(padding[3]), - row[4].ljust(padding[4]), - ) - ) - - self._log_footer(header=header, filler="-") + def _print_defaults_list( + self, config_name: Optional[str], overrides: List[str] + ) -> None: + # TODO + assert False, "Not implemented" + # run_mode = RunMode.RUN + # ret = self.config_loader.compute_input_defaults_list( + # config_name=config_name, + # overrides=overrides, + # run_mode=RunMode.RUN, + # from_shell=True, + # ) + # + # defaults = expand_defaults_list( + # defaults=ret.input_defaults, + # skip_missing=run_mode == RunMode.RUN, + # ignore_config_load_failures=True, + # repo=self.config_loader.repository, # type: ignore + # ) + # + # box: List[List[str]] = [ + # [ + # "Config group", + # "Config name", + # "Package", + # "Parent", + # "Skip reason", + # ] + # ] + # for d in defaults: + # row = [ + # d.config_group, + # d.config_name, + # d.package, + # d.parent, + # d.skip_load_reason if d.skip_load else "", + # ] + # row = [x if x is not None else "" for x in row] + # box.append(row) + # padding = get_column_widths(box) + # del box[0] + # log.debug("") + # self._log_header("Defaults List", filler="*") + # header = "| {} | {} | {} | {} | {} | ".format( + # "Config group".ljust(padding[0]), + # "Config name".ljust(padding[1]), + # "Package".ljust(padding[2]), + # "Parent".ljust(padding[3]), + # "Skip reason".ljust(padding[4]), + # ) + # self._log_header(header=header, filler="-") + # + # for row in box: + # log.debug( + # "| {} | {} | {} | {} | {} |".format( + # row[0].ljust(padding[0]), + # row[1].ljust(padding[1]), + # row[2].ljust(padding[2]), + # row[3].ljust(padding[3]), + # row[4].ljust(padding[4]), + # ) + # ) + # + # self._log_footer(header=header, filler="-") def _print_debug_info(self, cfg: DictConfig) -> None: assert log is not None diff --git a/hydra/_internal/new_defaults_list.py b/hydra/_internal/new_defaults_list.py index 487d0bf8505..0f063357aa9 100644 --- a/hydra/_internal/new_defaults_list.py +++ b/hydra/_internal/new_defaults_list.py @@ -1,8 +1,10 @@ +# Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved + import copy import warnings from dataclasses import dataclass from textwrap import dedent -from typing import Dict, List +from typing import Dict, List, Optional, Union from hydra import MissingConfigException from hydra._internal.config_repository import IConfigRepository @@ -77,7 +79,7 @@ def override_default_option(self, default: GroupDefault) -> None: default.config_name_overridden = True self.override_used[key] = True - def ensure_overrides_used(self): + def ensure_overrides_used(self) -> None: for key, used in self.override_used.items(): if not used: msg = dedent( @@ -110,8 +112,10 @@ def _validate_self(containing_node: InputDefault, defaults: List[InputDefault]) def update_package_header( - repo: IConfigRepository, node: InputDefault, is_primary_config: bool -): + repo: IConfigRepository, + node: InputDefault, + is_primary_config: bool, +) -> None: # This loads the same config loaded in _create_defaults_tree # To avoid loading it twice, the repo implementation is expected to cache # loaded configs @@ -127,14 +131,14 @@ def _expand_virtual_root( root: DefaultsTreeNode, overrides: Overrides, ) -> DefaultsTreeNode: - children = [] + children: List[Union[DefaultsTreeNode, InputDefault]] = [] + assert root.children is not None for d in reversed(root.children): assert isinstance(d, ConfigDefault) new_root = DefaultsTreeNode(node=d, parent=root) d.parent_base_dir = "" d.parent_package = "" - new_root.parent_base_dir = d.get_group_path() subtree = _create_defaults_tree( repo=repo, root=new_root, @@ -159,7 +163,7 @@ def _create_defaults_tree( overrides: Overrides, ) -> DefaultsTreeNode: parent = root.node - children = [] + children: List[Union[InputDefault, DefaultsTreeNode]] = [] if parent.is_virtual(): return _expand_virtual_root(repo, root, overrides) else: @@ -172,6 +176,7 @@ def _create_defaults_tree( ) if overrides.is_overridden(parent): + assert isinstance(parent, GroupDefault) overrides.override_default_option(parent) # clear package header and obtain updated one from overridden config # (for the rare case it has changed) @@ -187,11 +192,12 @@ def _create_defaults_tree( if loaded is None: missing_config_error(repo, root.node) + assert loaded is not None defaults_list = copy.deepcopy(loaded.new_defaults_list) if is_primary_config: - for d in overrides.append_group_defaults: - defaults_list.append(d) + for gd in overrides.append_group_defaults: + defaults_list.append(gd) if len(defaults_list) > 0: _validate_self(containing_node=parent, defaults=defaults_list) @@ -203,6 +209,7 @@ def _create_defaults_tree( d.parent_package = parent.get_final_package() if isinstance(d, GroupDefault): + assert d.group is not None is_legacy_hydra_override = not d.override and d.group.startswith( "hydra/" ) @@ -231,7 +238,6 @@ def _create_defaults_tree( d.parent_base_dir = parent.get_group_path() d.parent_package = parent.get_final_package() - new_root.parent_base_dir = d.get_group_path() subtree = _create_defaults_tree( repo=repo, root=new_root, @@ -249,9 +255,12 @@ def _create_defaults_tree( return root -def _create_result_default(tree: DefaultsTreeNode, node: InputDefault) -> ResultDefault: +def _create_result_default( + tree: Optional[DefaultsTreeNode], node: InputDefault +) -> ResultDefault: res = ResultDefault() if node.is_self(): + assert tree is not None res.config_path = tree.node.get_config_path() res.is_self = True pn = tree.parent_node() @@ -272,7 +281,7 @@ def _create_result_default(tree: DefaultsTreeNode, node: InputDefault) -> Result def _tree_to_list( tree: DefaultsTreeNode, output: List[ResultDefault], -): +) -> None: node = tree.node if tree.children is None or len(tree.children) == 0: @@ -318,7 +327,7 @@ def _create_defaults_list( is_primary_config=True, ) - output = [] + output: List[ResultDefault] = [] _tree_to_list(tree=defaults_tree, output=output) # TODO: fail if duplicate items exists return output diff --git a/hydra/core/new_default_element.py b/hydra/core/new_default_element.py index 87c0def295b..b686f09daa1 100644 --- a/hydra/core/new_default_element.py +++ b/hydra/core/new_default_element.py @@ -1,3 +1,5 @@ +# Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved + from dataclasses import dataclass, field from typing import List, Optional, Union @@ -12,6 +14,11 @@ class ResultDefault: @dataclass class InputDefault: + + parent_base_dir: Optional[str] = field(default=None, compare=False, repr=False) + parent_package: Optional[str] = field(default=None, compare=False, repr=False) + package_header: Optional[str] = field(default=None, compare=False) + def is_self(self) -> bool: raise NotImplementedError() @@ -39,6 +46,11 @@ def _get_attributes(self) -> List[str]: def _get_flags(self) -> List[str]: raise NotImplementedError() + def _get_parent_package(self) -> Optional[str]: + ret = self.__dict__["parent_package"] + assert ret is None or isinstance(ret, str) + return ret + def is_virtual(self) -> bool: return False @@ -54,13 +66,17 @@ def set_package_header(self, package_header: str) -> None: self.__dict__["package_header"] = package_header def get_package_header(self) -> Optional[str]: - return self.__dict__["package_header"] + ret = self.__dict__["package_header"] + assert ret is None or isinstance(ret, str) + return ret - def get_package(self) -> str: + def get_package(self) -> Optional[str]: if self.__dict__["package"] is None: - return self.__dict__["package_header"] + ret = self.__dict__["package_header"] else: - return self.__dict__["package"] + ret = self.__dict__["package"] + assert ret is None or isinstance(ret, str) + return ret def _get_final_package( self, @@ -156,14 +172,14 @@ def __repr__(self) -> str: @dataclass(repr=False) class ConfigDefault(InputDefault): - path: str + path: Optional[str] = None package: Optional[str] = None - parent_base_dir: Optional[str] = field(default=None, compare=False, repr=False) - parent_package: Optional[str] = field(default=None, compare=False, repr=False) - package_header: Optional[str] = field(default=None, compare=False) + # parent_base_dir: Optional[str] = field(default=None, compare=False, repr=False) + # parent_package: Optional[str] = field(default=None, compare=False, repr=False) + # package_header: Optional[str] = field(default=None, compare=False) - def __post_init__(self): + def __post_init__(self) -> None: if self.is_self() and self.package is not None: raise ValueError("_self_@PACKAGE is not supported") @@ -172,6 +188,7 @@ def is_self(self) -> bool: def get_group_path(self) -> str: assert self.parent_base_dir is not None + assert self.path is not None idx = self.path.rfind("/") if idx == -1: group = "" @@ -187,6 +204,7 @@ def get_group_path(self) -> str: return f"{self.parent_base_dir}/{group}" def get_name(self) -> str: + assert self.path is not None idx = self.path.rfind("/") if idx == -1: return self.path @@ -195,6 +213,7 @@ def get_name(self) -> str: def get_config_path(self) -> str: assert self.parent_base_dir is not None + assert self.path is not None if self.parent_base_dir == "": return self.path else: @@ -206,6 +225,7 @@ def get_final_package(self) -> str: ) def _relative_group_path(self) -> str: + assert self.path is not None idx = self.path.rfind("/") if idx == -1: return "" @@ -230,12 +250,11 @@ class GroupDefault(InputDefault): override: bool = False - parent_base_dir: Optional[str] = field(default=None, compare=False, repr=False) config_name_overridden: bool = field(default=False, compare=False, repr=False) - package_header: Optional[str] = field(default=None, compare=False) - def __post_init__(self): + def __post_init__(self) -> None: assert self.group is not None and self.group != "" + assert self.name is not None def is_self(self) -> bool: return self.name == "_self_" @@ -252,17 +271,19 @@ def get_config_path(self) -> str: group_path = self.get_group_path() assert group_path != "" - return f"{group_path}/{self.name}" + return f"{group_path}/{self.get_name()}" def get_name(self) -> str: + assert self.name is not None return self.name def get_final_package(self) -> str: return self._get_final_package( - self.parent_package, self.get_package(), self.get_name() + self._get_parent_package(), self.get_package(), self.get_name() ) def _relative_group_path(self) -> str: + assert self.group is not None return self.group def _get_attributes(self) -> List[str]: diff --git a/hydra/plugins/config_source.py b/hydra/plugins/config_source.py index f968ff9432d..a1419f7cba0 100644 --- a/hydra/plugins/config_source.py +++ b/hydra/plugins/config_source.py @@ -9,7 +9,7 @@ from abc import abstractmethod from dataclasses import dataclass -from typing import List, Optional, Dict, Tuple, MutableSequence +from typing import List, Optional, Dict, Tuple, MutableSequence, Union from hydra.core import DefaultElement from hydra.core.new_default_element import InputDefault, ConfigDefault, GroupDefault @@ -389,8 +389,9 @@ def _extract_defaults_list( def _create_new_defaults_list( defaults: ListConfig, ) -> List[InputDefault]: - res: List[DefaultElement] = [] + res: List[InputDefault] = [] for item in defaults: + default: InputDefault if isinstance(item, DictConfig): optional = item.pop("optional", False) override = item.pop("override", False) @@ -424,12 +425,13 @@ def _create_new_defaults_list( return res @staticmethod - def _extract_raw_defaults_list(cfg: Container) -> List[InputDefault]: + def _extract_raw_defaults_list(cfg: Container) -> Union[ListConfig, DictConfig]: + empty = OmegaConf.create([]) if not OmegaConf.is_dict(cfg): - return [] - + return empty + assert isinstance(cfg, DictConfig) with read_write(cfg): with open_dict(cfg): - defaults = cfg.pop("defaults", []) + defaults = cfg.pop("defaults", empty) return defaults diff --git a/tests/defaults_list/__init__.py b/tests/defaults_list/__init__.py index b25b0859098..8d945d28c88 100644 --- a/tests/defaults_list/__init__.py +++ b/tests/defaults_list/__init__.py @@ -1,14 +1,14 @@ # Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved -from typing import List, Any +from typing import Any, List -from hydra._internal.config_repository import IConfigRepository, ConfigRepository +from hydra._internal.config_repository import ConfigRepository, IConfigRepository from hydra._internal.config_search_path_impl import ConfigSearchPathImpl from hydra._internal.new_defaults_list import ( - DefaultsTreeNode, - _create_defaults_tree, Overrides, + _create_defaults_tree, _create_root, ) +from hydra.core.new_default_element import DefaultsTreeNode from hydra.core.override_parser.overrides_parser import OverridesParser diff --git a/tests/defaults_list/test_defaults_list.py b/tests/defaults_list/test_defaults_list.py index 4afe38c2807..35d26896313 100644 --- a/tests/defaults_list/test_defaults_list.py +++ b/tests/defaults_list/test_defaults_list.py @@ -1,16 +1,17 @@ # Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved -from textwrap import dedent - import re +from textwrap import dedent +from typing import Any, List from pytest import mark, param, raises -from typing import List, Any -from hydra._internal.new_defaults_list import ( - create_defaults_list, +from hydra._internal.new_defaults_list import create_defaults_list +from hydra.core.new_default_element import ( + ConfigDefault, + GroupDefault, + InputDefault, ResultDefault, ) -from hydra.core.new_default_element import InputDefault, GroupDefault, ConfigDefault from hydra.core.override_parser.overrides_parser import OverridesParser from hydra.core.plugins import Plugins from hydra.errors import ConfigCompositionException @@ -60,7 +61,7 @@ # TODO: Create https://hydra.cc/docs/next/upgrades/1.0_to_1.1/default_list_override -@mark.parametrize( +@mark.parametrize( # type: ignore "config_path,expected_list", [ param("empty", [], id="empty"), @@ -106,9 +107,12 @@ ), ], ) -def test_loaded_defaults_list(config_path: str, expected_list: List[InputDefault]): +def test_loaded_defaults_list( + config_path: str, expected_list: List[InputDefault] +) -> None: repo = create_repo() result = repo.load_config(config_path=config_path, is_primary_config=True) + assert result is not None assert result.new_defaults_list == expected_list @@ -139,7 +143,7 @@ def _test_defaults_list_impl( ) -@mark.parametrize( +@mark.parametrize( # type: ignore "default,expected_group_path,expected_config_path", [ param( @@ -169,13 +173,13 @@ def _test_defaults_list_impl( ], ) def test_get_paths( - default: InputDefault, expected_group_path, expected_config_path + default: InputDefault, expected_group_path: Any, expected_config_path: Any ) -> None: assert default.get_group_path() == expected_group_path assert default.get_config_path() == expected_config_path -@mark.parametrize( +@mark.parametrize( # type: ignore "default,expected", [ param( @@ -215,11 +219,11 @@ def test_get_paths( ), ], ) -def test_get_default_package(default: InputDefault, expected) -> None: +def test_get_default_package(default: InputDefault, expected: Any) -> None: assert default.get_default_package() == expected -@mark.parametrize( +@mark.parametrize( # type: ignore "default,expected", [ # empty parent package @@ -271,11 +275,11 @@ def test_get_default_package(default: InputDefault, expected) -> None: ), ], ) -def test_get_final_package(default: InputDefault, expected) -> None: +def test_get_final_package(default: InputDefault, expected: Any) -> None: assert default.get_final_package() == expected -@mark.parametrize( +@mark.parametrize( # type: ignore "config_name, overrides, expected", [ param( @@ -357,7 +361,7 @@ def test_simple_defaults_list_cases( ) -@mark.parametrize( +@mark.parametrize( # type: ignore "config_name, overrides, expected", [ param( @@ -429,7 +433,7 @@ def test_override_package_in_defaults_list( ) -@mark.parametrize( +@mark.parametrize( # type: ignore "config_name, overrides, expected", [ param( @@ -484,7 +488,7 @@ def test_include_nested_group_pkg2( ) -@mark.parametrize( +@mark.parametrize( # type: ignore "config_name, overrides, expected", [ param( @@ -542,7 +546,7 @@ def test_group_default_pkg1( ) -@mark.parametrize( +@mark.parametrize( # type: ignore "config_name, overrides, expected", [ param( @@ -601,7 +605,7 @@ def test_include_nested_group_global( ) -@mark.parametrize( +@mark.parametrize( # type: ignore "config_name, overrides, expected", [ param( @@ -660,7 +664,7 @@ def test_include_nested_group_global_foo( ) -@mark.parametrize( +@mark.parametrize( # type: ignore "config_name, overrides, expected", [ param( @@ -719,7 +723,7 @@ def test_include_nested_group_name_( ) -@mark.parametrize( +@mark.parametrize( # type: ignore "config_name, overrides, expected", [ param( @@ -770,13 +774,13 @@ def test_include_nested_group_name_( ) def test_primary_cfg_pkg_header_foo( config_name: str, overrides: List[str], expected: List[ResultDefault] -): +) -> None: _test_defaults_list_impl( config_name=config_name, overrides=overrides, expected=expected ) -@mark.parametrize( +@mark.parametrize( # type: ignore "config_name, overrides, expected", [ param( @@ -875,13 +879,13 @@ def test_primary_cfg_pkg_header_foo( ) def test_include_nested_group_pkg_header_foo( config_name: str, overrides: List[str], expected: List[ResultDefault] -): +) -> None: _test_defaults_list_impl( config_name=config_name, overrides=overrides, expected=expected ) -@mark.parametrize( +@mark.parametrize( # type: ignore "config_name, overrides, expected", [ param( @@ -921,13 +925,13 @@ def test_include_nested_group_pkg_header_foo( ) def test_nested_package_header_is_absolute( config_name: str, overrides: List[str], expected: List[ResultDefault] -): +) -> None: _test_defaults_list_impl( config_name=config_name, overrides=overrides, expected=expected ) -@mark.parametrize( +@mark.parametrize( # type: ignore "config_name, overrides, expected", [ param( @@ -1009,13 +1013,13 @@ def test_nested_package_header_is_absolute( ) def test_overriding_package_header_from_defaults_list( config_name: str, overrides: List[str], expected: List[ResultDefault] -): +) -> None: _test_defaults_list_impl( config_name=config_name, overrides=overrides, expected=expected ) -@mark.parametrize( +@mark.parametrize( # type: ignore "config_name,overrides,expected", [ param( @@ -1079,8 +1083,8 @@ def test_with_hydra_config( config_name: str, overrides: List[str], expected: List[ResultDefault], - recwarn, # Testing deprecated behavior -): + recwarn: Any, # Testing deprecated behavior +) -> None: _test_defaults_list_impl( config_name=config_name, overrides=overrides, diff --git a/tests/defaults_list/test_defaults_tree.py b/tests/defaults_list/test_defaults_tree.py index 900c01c617a..f8f2a0a80f2 100644 --- a/tests/defaults_list/test_defaults_tree.py +++ b/tests/defaults_list/test_defaults_tree.py @@ -1,15 +1,16 @@ # Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved -from textwrap import dedent - import re +from textwrap import dedent +from typing import Any, List from pytest import mark, param, raises -from typing import List, Any -from hydra._internal.new_defaults_list import ( +from hydra.core.new_default_element import ( + ConfigDefault, + GroupDefault, + VirtualRoot, DefaultsTreeNode, ) -from hydra.core.new_default_element import GroupDefault, ConfigDefault, VirtualRoot from hydra.core.plugins import Plugins from hydra.errors import ConfigCompositionException from hydra.test_utils.test_utils import chdir_hydra_root @@ -21,7 +22,7 @@ Plugins.instance() -@mark.parametrize( +@mark.parametrize( # type: ignore "config_name, overrides, expected", [ param( @@ -135,7 +136,7 @@ def test_simple_defaults_tree_cases( ) -@mark.parametrize( +@mark.parametrize( # type: ignore "config_name, overrides, expected", [ param( @@ -221,7 +222,7 @@ def test_tree_with_append_override( ) -@mark.parametrize( +@mark.parametrize( # type: ignore "config_name, overrides, expected", [ param( @@ -296,7 +297,7 @@ def test_simple_group_override( ) -@mark.parametrize( +@mark.parametrize( # type: ignore "config_name, overrides, expected", [ param( @@ -317,7 +318,7 @@ def test_errors( ) -@mark.parametrize( +@mark.parametrize( # type: ignore "config_name, overrides, expected", [ param( @@ -392,7 +393,7 @@ def test_defaults_tree_with_package_overrides( ) -@mark.parametrize( +@mark.parametrize( # type: ignore "config_name, overrides, expected", [ param( @@ -467,7 +468,7 @@ def test_defaults_tree_with_package_overrides__group_override( ) -@mark.parametrize( +@mark.parametrize( # type: ignore "config_name, overrides, expected", [ param( @@ -584,13 +585,13 @@ def test_override_option_from_defaults_list( config_name: str, overrides: List[str], expected: DefaultsTreeNode, -): +) -> None: _test_defaults_tree_impl( config_name=config_name, input_overrides=overrides, expected=expected ) -@mark.parametrize( +@mark.parametrize( # type: ignore "config_name, overrides, expected", [ param( @@ -644,7 +645,7 @@ def test_two_group_defaults_different_pkgs( ) -@mark.parametrize( +@mark.parametrize( # type: ignore "config_name,overrides,expected", [ param( @@ -716,7 +717,7 @@ def test_legacy_hydra_overrides_from_primary_config( config_name: str, overrides: List[str], expected: DefaultsTreeNode, - recwarn, # Testing deprecated behavior + recwarn: Any, # Testing deprecated behavior ) -> None: _test_defaults_tree_impl( config_name=config_name, diff --git a/tools/configen/configen/utils.py b/tools/configen/configen/utils.py index 1640a7ef91d..652dcd060af 100644 --- a/tools/configen/configen/utils.py +++ b/tools/configen/configen/utils.py @@ -39,11 +39,11 @@ def type_str(t: Any) -> str: if hasattr(t, "__name__"): name = str(t.__name__) else: - if t._name is None: + if t.name is None: if t.__origin__ is not None: name = type_str(t.__origin__) else: - name = str(t._name) + name = str(t.name) args = getattr(t, "__args__", None) if args is not None: From 4786d602c3d16fb27ce9588ea34cb47ff7530023 Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Tue, 1 Dec 2020 12:21:13 -0800 Subject: [PATCH 33/91] Experinent use case tests --- hydra/_internal/new_defaults_list.py | 5 +- hydra/core/new_default_element.py | 89 +++++-- tests/defaults_list/test_defaults_list.py | 92 ++++++- tests/defaults_list/test_defaults_tree.py | 239 ++++++++++++++++++ .../experiment/include_absolute_config.yaml | 5 + .../experiment/override_config_group.yaml | 5 + .../experiment/override_hydra.yaml | 5 + ...roup_default_with_explicit_experiment.yaml | 3 + 8 files changed, 415 insertions(+), 28 deletions(-) create mode 100644 tests/test_data/default_lists/experiment/include_absolute_config.yaml create mode 100644 tests/test_data/default_lists/experiment/override_config_group.yaml create mode 100644 tests/test_data/default_lists/experiment/override_hydra.yaml create mode 100644 tests/test_data/default_lists/group_default_with_explicit_experiment.yaml diff --git a/hydra/_internal/new_defaults_list.py b/hydra/_internal/new_defaults_list.py index 0f063357aa9..2ac5d235054 100644 --- a/hydra/_internal/new_defaults_list.py +++ b/hydra/_internal/new_defaults_list.py @@ -133,8 +133,11 @@ def _expand_virtual_root( ) -> DefaultsTreeNode: children: List[Union[DefaultsTreeNode, InputDefault]] = [] assert root.children is not None + + for gd in overrides.append_group_defaults: + root.children.append(gd) + for d in reversed(root.children): - assert isinstance(d, ConfigDefault) new_root = DefaultsTreeNode(node=d, parent=root) d.parent_base_dir = "" d.parent_package = "" diff --git a/hydra/core/new_default_element.py b/hydra/core/new_default_element.py index b686f09daa1..32ddadf4680 100644 --- a/hydra/core/new_default_element.py +++ b/hydra/core/new_default_element.py @@ -11,6 +11,24 @@ class ResultDefault: package: Optional[str] = None is_self: bool = False + def __repr__(self) -> str: + attrs = [] + attr_names = "config_path", "package", "parent" + for attr in attr_names: + value = getattr(self, attr) + if value is not None: + attrs.append(f'{attr}="{value}"') + + flags = [] + flag_names = ["is_self"] + for flag in flag_names: + value = getattr(self, flag) + if value: + flags.append(f"{flag}=True") + + ret = f"{','.join(attrs + flags)}" + return f"{type(self).__name__}({ret})" + @dataclass class InputDefault: @@ -189,19 +207,30 @@ def is_self(self) -> bool: def get_group_path(self) -> str: assert self.parent_base_dir is not None assert self.path is not None - idx = self.path.rfind("/") + + if self.path.startswith("/"): + path = self.path[1:] + absolute = True + else: + path = self.path + absolute = False + + idx = path.rfind("/") if idx == -1: group = "" else: - group = self.path[0:idx] + group = path[0:idx] - if self.parent_base_dir == "": - return group - else: - if group == "": - return f"{self.parent_base_dir}" + if not absolute: + if self.parent_base_dir == "": + return group else: - return f"{self.parent_base_dir}/{group}" + if group == "": + return f"{self.parent_base_dir}" + else: + return f"{self.parent_base_dir}/{group}" + else: + return group def get_name(self) -> str: assert self.path is not None @@ -214,10 +243,20 @@ def get_name(self) -> str: def get_config_path(self) -> str: assert self.parent_base_dir is not None assert self.path is not None - if self.parent_base_dir == "": - return self.path + if self.path.startswith("/"): + path = self.path[1:] + absolute = True else: - return f"{self.parent_base_dir}/{self.path}" + path = self.path + absolute = False + + if not absolute: + if self.parent_base_dir == "": + return path + else: + return f"{self.parent_base_dir}/{path}" + else: + return path def get_final_package(self) -> str: return self._get_final_package( @@ -226,11 +265,16 @@ def get_final_package(self) -> str: def _relative_group_path(self) -> str: assert self.path is not None - idx = self.path.rfind("/") + if self.path.startswith("/"): + path = self.path[1:] + else: + path = self.path + + idx = path.rfind("/") if idx == -1: return "" else: - return self.path[0:idx] + return path[0:idx] def _get_attributes(self) -> List[str]: return ["path", "package"] @@ -262,10 +306,18 @@ def is_self(self) -> bool: def get_group_path(self) -> str: assert self.parent_base_dir is not None assert self.group is not None - if self.parent_base_dir == "": - return self.group + + if self.group.startswith("/"): + group = self.group[1:] + absolute = True else: - return f"{self.parent_base_dir}/{self.group}" + group = self.group + absolute = False + + if self.parent_base_dir == "" or absolute: + return group + else: + return f"{self.parent_base_dir}/{group}" def get_config_path(self) -> str: group_path = self.get_group_path() @@ -284,7 +336,10 @@ def get_final_package(self) -> str: def _relative_group_path(self) -> str: assert self.group is not None - return self.group + if self.group.startswith("/"): + return self.group[1:] + else: + return self.group def _get_attributes(self) -> List[str]: return ["group", "name", "package"] diff --git a/tests/defaults_list/test_defaults_list.py b/tests/defaults_list/test_defaults_list.py index 35d26896313..5812607947f 100644 --- a/tests/defaults_list/test_defaults_list.py +++ b/tests/defaults_list/test_defaults_list.py @@ -41,24 +41,35 @@ # - (Y) Test overriding of config groups with a specified package (@pkg) # - (Y) Overriding of config groups with a specified package (@pkg) when there are multiple choices from same group # - (Y) Handle hydra overrides -# TODO: test overriding configs in absolute location -# TODO: test duplicate _self_ error +# TODO: Experiment use case: test the following from a config in an experiment group +# - (Y) Override user config group with and without an external override of the same config group +# - (Y) Experiment specified in primary defaults +# - (Y) Append experiment file from external overrides +# - (Y) Override hydra config group from experiment [+ external override] +# - (Y) Include config with an absolute path +# - (Y) Test final defaults list with an experiment file +# TODO: Primary config in a config group (automatically _global_ and considered in root?) # TODO: Interpolation support # TODO: package header: # - consider making relative # - consider deprecating completely # TODO: Consider delete support # TODO: Consider package rename support +# TODO: Integrate with Hydra +# TODO: cleanup -# Error handling: -# TODO: (Y) Error handling for entries that failed to override anything -# TODO: test handling missing configs mentioned in defaults list (with and without optional) -# TODO: Ambiguous overrides should provide valid override keys for group -# TODO: Should duplicate entries in results list be an error? (same override key) +# TODO: Error handling: +# - (Y) Error handling for entries that failed to override anything +# - Duplicate _self_ error +# - test handling missing configs mentioned in defaults list (with and without optional) +# - Ambiguous overrides should provide valid override keys for group +# - Test deprecation message when attempting to override hydra configs without override: true +# - Should duplicate entries in results list be an error? (same override key) -# Documentation -# TODO: update documentation -# TODO: Create https://hydra.cc/docs/next/upgrades/1.0_to_1.1/default_list_override +# TODO Documentation +# - Update defaults list documentation +# - Create a page describing configuring experiments with Hydra (experiment use case) +# - Create https://hydra.cc/docs/next/upgrades/1.0_to_1.1/default_list_override @mark.parametrize( # type: ignore @@ -170,6 +181,19 @@ def _test_defaults_list_impl( "zoo/foo/bar", id="group_default:with_parent_basedir", ), + # absolute group + param( + ConfigDefault(path="/foo/zoo", parent_base_dir="irrelevant"), + "foo", + "foo/zoo", + id="config_default:absolute", + ), + param( + GroupDefault(group="/foo", name="zoo", parent_base_dir="irrelevant"), + "foo", + "foo/zoo", + id="group_default:absolute", + ), ], ) def test_get_paths( @@ -217,6 +241,17 @@ def test_get_paths( "x.a.b", id="group_default", ), + # absolute group/path + param( + ConfigDefault(path="/foo/bar", parent_base_dir="irrelevant"), + "foo", + id="config_default:absolute", + ), + param( + GroupDefault(group="/foo", name="bar", parent_base_dir="irrelevant"), + "foo", + id="group_default:absolute", + ), ], ) def test_get_default_package(default: InputDefault, expected: Any) -> None: @@ -1091,3 +1126,40 @@ def test_with_hydra_config( expected=expected, prepend_hydra=True, ) + + +@mark.parametrize( # type: ignore + "config_name,overrides,expected", + [ + param( + "group_default", + ["+experiment=include_absolute_config"], + [ + ResultDefault(config_path="group_default", package="", is_self=True), + ResultDefault( + config_path="group1/file1", package="group1", parent="group_default" + ), + ResultDefault( + config_path="group1/group2/file1", + package="group1.group2", + parent="experiment/include_absolute_config", + ), + ResultDefault( + config_path="experiment/include_absolute_config", + package="", + parent="group_default", + is_self=True, + ), + ], + id="group_default:experiment=include_absolute_config", + ), + ], +) +def test_experiment_use_case( + config_name: str, overrides: List[str], expected: List[ResultDefault] +): + _test_defaults_list_impl( + config_name=config_name, + overrides=overrides, + expected=expected, + ) diff --git a/tests/defaults_list/test_defaults_tree.py b/tests/defaults_list/test_defaults_tree.py index f8f2a0a80f2..00e46cfa894 100644 --- a/tests/defaults_list/test_defaults_tree.py +++ b/tests/defaults_list/test_defaults_tree.py @@ -725,3 +725,242 @@ def test_legacy_hydra_overrides_from_primary_config( expected=expected, prepend_hydra=True, ) + + +@mark.parametrize( # type: ignore + "config_name,overrides,expected", + [ + param( + "group_default_with_explicit_experiment", + [], + DefaultsTreeNode( + node=ConfigDefault(path="group_default_with_explicit_experiment"), + children=[ + ConfigDefault(path="_self_"), + GroupDefault(group="group1", name="file2"), + DefaultsTreeNode( + node=GroupDefault( + group="experiment", name="override_config_group" + ), + children=[ConfigDefault(path="_self_")], + ), + ], + ), + id="group_default_with_explicit_experiment", + ), + param( + "group_default_with_explicit_experiment", + ["group1=file3"], + DefaultsTreeNode( + node=ConfigDefault(path="group_default_with_explicit_experiment"), + children=[ + ConfigDefault(path="_self_"), + GroupDefault(group="group1", name="file3"), + DefaultsTreeNode( + node=GroupDefault( + group="experiment", name="override_config_group" + ), + children=[ConfigDefault(path="_self_")], + ), + ], + ), + id="group_default_with_explicit_experiment:with_external_override", + ), + ], +) +def test_group_default_with_explicit_experiment( + config_name: str, + overrides: List[str], + expected: DefaultsTreeNode, +) -> None: + _test_defaults_tree_impl( + config_name=config_name, + input_overrides=overrides, + expected=expected, + ) + + +@mark.parametrize( # type: ignore + "config_name,overrides,expected", + [ + param( + "group_default", + ["+experiment=override_config_group"], + DefaultsTreeNode( + node=ConfigDefault(path="group_default"), + children=[ + ConfigDefault(path="_self_"), + GroupDefault(group="group1", name="file2"), + DefaultsTreeNode( + node=GroupDefault( + group="experiment", name="override_config_group" + ), + children=[ConfigDefault(path="_self_")], + ), + ], + ), + id="group_default_with_appended_experiment", + ), + param( + "group_default", + ["group1=file3", "+experiment=override_config_group"], + DefaultsTreeNode( + node=ConfigDefault(path="group_default"), + children=[ + ConfigDefault(path="_self_"), + GroupDefault(group="group1", name="file3"), + DefaultsTreeNode( + node=GroupDefault( + group="experiment", name="override_config_group" + ), + children=[ConfigDefault(path="_self_")], + ), + ], + ), + id="group_default_with_appended_experiment:with_external_override", + ), + ], +) +def test_group_default_with_appended_experiment( + config_name: str, + overrides: List[str], + expected: DefaultsTreeNode, +) -> None: + _test_defaults_tree_impl( + config_name=config_name, + input_overrides=overrides, + expected=expected, + ) + + +@mark.parametrize( # type: ignore + "config_name,overrides,expected", + [ + param( + "group_default", + ["+experiment=include_absolute_config"], + DefaultsTreeNode( + node=ConfigDefault(path="group_default"), + children=[ + ConfigDefault(path="_self_"), + GroupDefault(group="group1", name="file1"), + DefaultsTreeNode( + node=GroupDefault( + group="experiment", name="include_absolute_config" + ), + children=[ + GroupDefault(group="/group1/group2", name="file1"), + ConfigDefault(path="_self_"), + ], + ), + ], + ), + id="include_absolute_config", + ), + param( + "group_default", + ["+experiment=include_absolute_config", "group1/group2=file2"], + DefaultsTreeNode( + node=ConfigDefault(path="group_default"), + children=[ + ConfigDefault(path="_self_"), + GroupDefault(group="group1", name="file1"), + DefaultsTreeNode( + node=GroupDefault( + group="experiment", name="include_absolute_config" + ), + children=[ + GroupDefault(group="/group1/group2", name="file2"), + ConfigDefault(path="_self_"), + ], + ), + ], + ), + id="include_absolute_config:with_external_override", + ), + ], +) +def test_experiment_include_absolute_config( + config_name: str, + overrides: List[str], + expected: DefaultsTreeNode, +) -> None: + _test_defaults_tree_impl( + config_name=config_name, input_overrides=overrides, expected=expected + ) + + +@mark.parametrize( # type: ignore + "config_name,overrides,expected", + [ + param( + "group_default", + ["+experiment=override_hydra"], + DefaultsTreeNode( + node=VirtualRoot(), + children=[ + DefaultsTreeNode( + node=ConfigDefault(path="hydra/config"), + children=[ + ConfigDefault(path="_self_"), + GroupDefault(group="help", name="custom1"), + GroupDefault(group="output", name="default"), + ], + ), + DefaultsTreeNode( + node=ConfigDefault(path="group_default"), + children=[ + ConfigDefault(path="_self_"), + GroupDefault(group="group1", name="file1"), + ], + ), + DefaultsTreeNode( + node=GroupDefault(group="experiment", name="override_hydra"), + children=[ConfigDefault(path="_self_")], + ), + ], + ), + id="experiment_overriding_hydra_group", + ), + param( + "group_default", + ["+experiment=override_hydra", "hydra/help=custom2"], + DefaultsTreeNode( + node=VirtualRoot(), + children=[ + DefaultsTreeNode( + node=ConfigDefault(path="hydra/config"), + children=[ + ConfigDefault(path="_self_"), + GroupDefault(group="help", name="custom2"), + GroupDefault(group="output", name="default"), + ], + ), + DefaultsTreeNode( + node=ConfigDefault(path="group_default"), + children=[ + ConfigDefault(path="_self_"), + GroupDefault(group="group1", name="file1"), + ], + ), + DefaultsTreeNode( + node=GroupDefault(group="experiment", name="override_hydra"), + children=[ConfigDefault(path="_self_")], + ), + ], + ), + id="experiment_overriding_hydra_group:with_external_hydra_override", + ), + ], +) +def test_experiment_overriding_hydra_group( + config_name: str, + overrides: List[str], + expected: DefaultsTreeNode, +) -> None: + _test_defaults_tree_impl( + config_name=config_name, + input_overrides=overrides, + expected=expected, + prepend_hydra=True, + ) diff --git a/tests/test_data/default_lists/experiment/include_absolute_config.yaml b/tests/test_data/default_lists/experiment/include_absolute_config.yaml new file mode 100644 index 00000000000..72f7d8b0e00 --- /dev/null +++ b/tests/test_data/default_lists/experiment/include_absolute_config.yaml @@ -0,0 +1,5 @@ +# @package _global_ + +defaults: + - /group1/group2: file1 + - _self_ \ No newline at end of file diff --git a/tests/test_data/default_lists/experiment/override_config_group.yaml b/tests/test_data/default_lists/experiment/override_config_group.yaml new file mode 100644 index 00000000000..1f30bc3a00f --- /dev/null +++ b/tests/test_data/default_lists/experiment/override_config_group.yaml @@ -0,0 +1,5 @@ +# @package _global_ + +defaults: + - /group1: file2 + override: true diff --git a/tests/test_data/default_lists/experiment/override_hydra.yaml b/tests/test_data/default_lists/experiment/override_hydra.yaml new file mode 100644 index 00000000000..ab09c327687 --- /dev/null +++ b/tests/test_data/default_lists/experiment/override_hydra.yaml @@ -0,0 +1,5 @@ +# @package _global_ + +defaults: + - /hydra/help: custom1 + override: true \ No newline at end of file diff --git a/tests/test_data/default_lists/group_default_with_explicit_experiment.yaml b/tests/test_data/default_lists/group_default_with_explicit_experiment.yaml new file mode 100644 index 00000000000..cfcfa5c2f63 --- /dev/null +++ b/tests/test_data/default_lists/group_default_with_explicit_experiment.yaml @@ -0,0 +1,3 @@ +defaults: + - group1: file1 + - experiment: override_config_group \ No newline at end of file From 9c8057dbc4631c2b6590a4ee592e820d5e85451c Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Tue, 1 Dec 2020 14:21:33 -0800 Subject: [PATCH 34/91] test_experiment_as_primary_config --- hydra/_internal/new_defaults_list.py | 2 +- tests/defaults_list/test_defaults_list.py | 63 ++++++++++++++++++++--- tests/defaults_list/test_defaults_tree.py | 40 ++++++++++++++ 3 files changed, 98 insertions(+), 7 deletions(-) diff --git a/hydra/_internal/new_defaults_list.py b/hydra/_internal/new_defaults_list.py index 2ac5d235054..16741e39f53 100644 --- a/hydra/_internal/new_defaults_list.py +++ b/hydra/_internal/new_defaults_list.py @@ -133,7 +133,7 @@ def _expand_virtual_root( ) -> DefaultsTreeNode: children: List[Union[DefaultsTreeNode, InputDefault]] = [] assert root.children is not None - + assert len(root.children) == 2 for gd in overrides.append_group_defaults: root.children.append(gd) diff --git a/tests/defaults_list/test_defaults_list.py b/tests/defaults_list/test_defaults_list.py index 5812607947f..31a4e02fc3f 100644 --- a/tests/defaults_list/test_defaults_list.py +++ b/tests/defaults_list/test_defaults_list.py @@ -48,15 +48,13 @@ # - (Y) Override hydra config group from experiment [+ external override] # - (Y) Include config with an absolute path # - (Y) Test final defaults list with an experiment file -# TODO: Primary config in a config group (automatically _global_ and considered in root?) +# - (Y) Test experiment config as a primary (must have @package _global_ and absolute addressing of config groups) +# TODO: (Y) package header: +# - (Y) Consider making relative. Decision: package header will remain absolute. +# - (Y) Consider deprecating completely. Decision: package will not be deprecated for now # TODO: Interpolation support -# TODO: package header: -# - consider making relative -# - consider deprecating completely # TODO: Consider delete support # TODO: Consider package rename support -# TODO: Integrate with Hydra -# TODO: cleanup # TODO: Error handling: # - (Y) Error handling for entries that failed to override anything @@ -66,6 +64,13 @@ # - Test deprecation message when attempting to override hydra configs without override: true # - Should duplicate entries in results list be an error? (same override key) +# TODO: Integrate with Hydra +# - replace old defaults list computation +# - enable --info=defaults output +# - ensure all tests are passing +# - cleanup + + # TODO Documentation # - Update defaults list documentation # - Create a page describing configuring experiments with Hydra (experiment use case) @@ -1163,3 +1168,49 @@ def test_experiment_use_case( overrides=overrides, expected=expected, ) + + +@mark.parametrize( # type: ignore + "config_name,overrides,expected", + [ + param( + "experiment/override_hydra", + [], + [ + ResultDefault( + config_path="hydra/config", + package="hydra", + parent="", + is_self=True, + ), + ResultDefault( + config_path="hydra/help/custom1", + package="hydra.help", + parent="hydra/config", + ), + ResultDefault( + config_path="hydra/output/default", + package="hydra", + parent="hydra/config", + ), + ResultDefault( + config_path="experiment/override_hydra", + package="", + parent="", + is_self=True, + ), + ], + id="group_default:experiment=include_absolute_config", + ), + ], +) +def test_as_as_primary( + config_name: str, overrides: List[str], expected: List[ResultDefault] +): + _test_defaults_list_impl( + config_name=config_name, + overrides=overrides, + expected=expected, + prepend_hydra=True, + ) + ... diff --git a/tests/defaults_list/test_defaults_tree.py b/tests/defaults_list/test_defaults_tree.py index 00e46cfa894..4254749c35c 100644 --- a/tests/defaults_list/test_defaults_tree.py +++ b/tests/defaults_list/test_defaults_tree.py @@ -964,3 +964,43 @@ def test_experiment_overriding_hydra_group( expected=expected, prepend_hydra=True, ) + + +@mark.parametrize( # type: ignore + "config_name,overrides,expected", + [ + param( + "experiment/override_hydra", + [], + DefaultsTreeNode( + node=VirtualRoot(), + children=[ + DefaultsTreeNode( + node=ConfigDefault(path="hydra/config"), + children=[ + ConfigDefault(path="_self_"), + GroupDefault(group="help", name="custom1"), + GroupDefault(group="output", name="default"), + ], + ), + DefaultsTreeNode( + node=ConfigDefault(path="experiment/override_hydra"), + children=[ConfigDefault(path="_self_")], + ), + ], + ), + id="experiment_overriding_hydra_group_as_primary", + ), + ], +) +def test_experiment_as_primary_config( + config_name: str, + overrides: List[str], + expected: DefaultsTreeNode, +) -> None: + _test_defaults_tree_impl( + config_name=config_name, + input_overrides=overrides, + expected=expected, + prepend_hydra=True, + ) From 4f6795f96a72423fcb72630dc4d0532b6dc569b2 Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Tue, 1 Dec 2020 15:00:16 -0800 Subject: [PATCH 35/91] Testing config extension use cases --- hydra/core/new_default_element.py | 8 +- hydra/plugins/config_source.py | 27 +++++- tests/defaults_list/test_defaults_list.py | 5 ++ tests/defaults_list/test_defaults_tree.py | 89 +++++++++++++++++++ tests/test_data/default_lists/db/base_db.yaml | 0 .../default_lists/extend/base_db.yaml | 0 .../default_lists/extend/external.yaml | 3 + .../test_data/default_lists/extend/here.yaml | 3 + .../default_lists/extend/nested.yaml | 3 + .../default_lists/extend/nested/base_db.yaml | 0 .../extend/nested_here_keyword.yaml | 3 + .../test_extend_from_external_group.yaml | 2 + .../test_extend_from_nested_group.yaml | 2 + .../default_lists/test_extend_same_group.yaml | 2 + 14 files changed, 142 insertions(+), 5 deletions(-) create mode 100644 tests/test_data/default_lists/db/base_db.yaml create mode 100644 tests/test_data/default_lists/extend/base_db.yaml create mode 100644 tests/test_data/default_lists/extend/external.yaml create mode 100644 tests/test_data/default_lists/extend/here.yaml create mode 100644 tests/test_data/default_lists/extend/nested.yaml create mode 100644 tests/test_data/default_lists/extend/nested/base_db.yaml create mode 100644 tests/test_data/default_lists/extend/nested_here_keyword.yaml create mode 100644 tests/test_data/default_lists/test_extend_from_external_group.yaml create mode 100644 tests/test_data/default_lists/test_extend_from_nested_group.yaml create mode 100644 tests/test_data/default_lists/test_extend_same_group.yaml diff --git a/hydra/core/new_default_element.py b/hydra/core/new_default_element.py index 32ddadf4680..62a609e8e3b 100644 --- a/hydra/core/new_default_element.py +++ b/hydra/core/new_default_element.py @@ -193,13 +193,11 @@ class ConfigDefault(InputDefault): path: Optional[str] = None package: Optional[str] = None - # parent_base_dir: Optional[str] = field(default=None, compare=False, repr=False) - # parent_package: Optional[str] = field(default=None, compare=False, repr=False) - # package_header: Optional[str] = field(default=None, compare=False) - def __post_init__(self) -> None: if self.is_self() and self.package is not None: raise ValueError("_self_@PACKAGE is not supported") + if self.package == "_here_": + self.package = "" def is_self(self) -> bool: return self.path == "_self_" @@ -299,6 +297,8 @@ class GroupDefault(InputDefault): def __post_init__(self) -> None: assert self.group is not None and self.group != "" assert self.name is not None + if self.package == "_here_": + self.package = "" def is_self(self) -> bool: return self.name == "_self_" diff --git a/hydra/plugins/config_source.py b/hydra/plugins/config_source.py index a1419f7cba0..b3c844437e1 100644 --- a/hydra/plugins/config_source.py +++ b/hydra/plugins/config_source.py @@ -251,6 +251,7 @@ def _get_header_dict(config_text: str) -> Dict[str, str]: return res + # TODO: cleanup and rename _split_group2 to _split_group @staticmethod def _split_group( group_with_package: str, @@ -281,6 +282,30 @@ def _split_group( return group, package, package2 + @staticmethod + def _split_group2( + group_with_package: str, + ) -> Tuple[str, Optional[str], Optional[str]]: + idx = group_with_package.find("@") + if idx == -1: + # group + group = group_with_package + package = None + else: + # group@package + group = group_with_package[0:idx] + package = group_with_package[idx + 1 :] + + package2 = None + if package is not None: + # if we have a package, break it down if it's a rename + idx = package.find(":") + if idx != -1: + package2 = package[idx + 1 :] + package = package[0:idx] + + return group, package, package2 + @staticmethod def _create_defaults_list( config_path: Optional[str], @@ -415,7 +440,7 @@ def _create_new_defaults_list( override=override, ) elif isinstance(item, str): - path, package, _package2 = ConfigSource._split_group(item) + path, package, _package2 = ConfigSource._split_group2(item) default = ConfigDefault(path=path, package=package) else: raise ValueError( diff --git a/tests/defaults_list/test_defaults_list.py b/tests/defaults_list/test_defaults_list.py index 31a4e02fc3f..561fe21faa7 100644 --- a/tests/defaults_list/test_defaults_list.py +++ b/tests/defaults_list/test_defaults_list.py @@ -52,9 +52,14 @@ # TODO: (Y) package header: # - (Y) Consider making relative. Decision: package header will remain absolute. # - (Y) Consider deprecating completely. Decision: package will not be deprecated for now +# TODO: (Y) Test use cases for config extension: +# - (Y) Extension from the same config group +# - (Y) Extension from absolute config group +# - (Y) Extension from a nested config group # TODO: Interpolation support # TODO: Consider delete support # TODO: Consider package rename support +# TODO: Support placeholder element # TODO: Error handling: # - (Y) Error handling for entries that failed to override anything diff --git a/tests/defaults_list/test_defaults_tree.py b/tests/defaults_list/test_defaults_tree.py index 4254749c35c..fc1a0a6201b 100644 --- a/tests/defaults_list/test_defaults_tree.py +++ b/tests/defaults_list/test_defaults_tree.py @@ -1004,3 +1004,92 @@ def test_experiment_as_primary_config( expected=expected, prepend_hydra=True, ) + + +@mark.parametrize( # type: ignore + "config_name,overrides,expected", + [ + param( + "test_extend_same_group", + [], + DefaultsTreeNode( + node=ConfigDefault(path="test_extend_same_group"), + children=[ + ConfigDefault(path="_self_"), + DefaultsTreeNode( + node=GroupDefault(group="extend", name="here"), + children=[ + ConfigDefault(path="base_db"), + ConfigDefault(path="_self_"), + ], + ), + ], + ), + id="test_extend_same_group", + ), + param( + "test_extend_from_external_group", + [], + DefaultsTreeNode( + node=ConfigDefault(path="test_extend_from_external_group"), + children=[ + ConfigDefault(path="_self_"), + DefaultsTreeNode( + node=GroupDefault(group="extend", name="external"), + children=[ + ConfigDefault(path="/db/base_db", package=""), + ConfigDefault(path="_self_"), + ], + ), + ], + ), + id="test_extend_from_external_group", + ), + param( + "test_extend_from_nested_group", + [], + DefaultsTreeNode( + node=ConfigDefault(path="test_extend_from_nested_group"), + children=[ + ConfigDefault(path="_self_"), + DefaultsTreeNode( + node=GroupDefault(group="extend", name="nested"), + children=[ + ConfigDefault(path="nested/base_db", package=""), + ConfigDefault(path="_self_"), + ], + ), + ], + ), + id="test_extend_from_nested_group", + ), + param( + "test_extend_from_nested_group", + ["extend=nested_here_keyword"], + DefaultsTreeNode( + node=ConfigDefault(path="test_extend_from_nested_group"), + children=[ + ConfigDefault(path="_self_"), + DefaultsTreeNode( + node=GroupDefault(group="extend", name="nested_here_keyword"), + children=[ + ConfigDefault(path="nested/base_db", package=""), + ConfigDefault(path="_self_"), + ], + ), + ], + ), + id="nested_here_keyword", + ), + ], +) +def test_extension_use_cases( + config_name: str, + overrides: List[str], + expected: DefaultsTreeNode, +) -> None: + _test_defaults_tree_impl( + config_name=config_name, + input_overrides=overrides, + expected=expected, + ) diff --git a/tests/test_data/default_lists/db/base_db.yaml b/tests/test_data/default_lists/db/base_db.yaml new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/test_data/default_lists/extend/base_db.yaml b/tests/test_data/default_lists/extend/base_db.yaml new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/test_data/default_lists/extend/external.yaml b/tests/test_data/default_lists/extend/external.yaml new file mode 100644 index 00000000000..cc574e9b30c --- /dev/null +++ b/tests/test_data/default_lists/extend/external.yaml @@ -0,0 +1,3 @@ +defaults: + - /db/base_db@ + - _self_ diff --git a/tests/test_data/default_lists/extend/here.yaml b/tests/test_data/default_lists/extend/here.yaml new file mode 100644 index 00000000000..44b9d9f0bbc --- /dev/null +++ b/tests/test_data/default_lists/extend/here.yaml @@ -0,0 +1,3 @@ +defaults: + - base_db + - _self_ diff --git a/tests/test_data/default_lists/extend/nested.yaml b/tests/test_data/default_lists/extend/nested.yaml new file mode 100644 index 00000000000..8131816b6a7 --- /dev/null +++ b/tests/test_data/default_lists/extend/nested.yaml @@ -0,0 +1,3 @@ +defaults: + - nested/base_db@ + - _self_ diff --git a/tests/test_data/default_lists/extend/nested/base_db.yaml b/tests/test_data/default_lists/extend/nested/base_db.yaml new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/test_data/default_lists/extend/nested_here_keyword.yaml b/tests/test_data/default_lists/extend/nested_here_keyword.yaml new file mode 100644 index 00000000000..b903eda7e65 --- /dev/null +++ b/tests/test_data/default_lists/extend/nested_here_keyword.yaml @@ -0,0 +1,3 @@ +defaults: + - nested/base_db@_here_ + - _self_ diff --git a/tests/test_data/default_lists/test_extend_from_external_group.yaml b/tests/test_data/default_lists/test_extend_from_external_group.yaml new file mode 100644 index 00000000000..db0a8733333 --- /dev/null +++ b/tests/test_data/default_lists/test_extend_from_external_group.yaml @@ -0,0 +1,2 @@ +defaults: + - extend: external \ No newline at end of file diff --git a/tests/test_data/default_lists/test_extend_from_nested_group.yaml b/tests/test_data/default_lists/test_extend_from_nested_group.yaml new file mode 100644 index 00000000000..1ac890187fb --- /dev/null +++ b/tests/test_data/default_lists/test_extend_from_nested_group.yaml @@ -0,0 +1,2 @@ +defaults: + - extend: nested \ No newline at end of file diff --git a/tests/test_data/default_lists/test_extend_same_group.yaml b/tests/test_data/default_lists/test_extend_same_group.yaml new file mode 100644 index 00000000000..5d53ee0573e --- /dev/null +++ b/tests/test_data/default_lists/test_extend_same_group.yaml @@ -0,0 +1,2 @@ +defaults: + - extend: here \ No newline at end of file From bc69ee7653b6969a2789780f7bf6659eaa1aed7f Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Tue, 1 Dec 2020 18:37:59 -0800 Subject: [PATCH 36/91] Testing for cases with MISSING ('???') default element --- hydra/_internal/new_defaults_list.py | 59 +++++++++- tests/defaults_list/__init__.py | 5 +- tests/defaults_list/test_defaults_list.py | 4 + tests/defaults_list/test_defaults_tree.py | 103 ++++++++++++++++++ .../default_lists/group1/with_missing.yaml | 2 + .../test_data/default_lists/with_missing.yaml | 2 + 6 files changed, 170 insertions(+), 5 deletions(-) create mode 100644 tests/test_data/default_lists/group1/with_missing.yaml create mode 100644 tests/test_data/default_lists/with_missing.yaml diff --git a/hydra/_internal/new_defaults_list.py b/hydra/_internal/new_defaults_list.py index 16741e39f53..346c0d8eecf 100644 --- a/hydra/_internal/new_defaults_list.py +++ b/hydra/_internal/new_defaults_list.py @@ -146,6 +146,7 @@ def _expand_virtual_root( repo=repo, root=new_root, is_primary_config=False, + skip_missing=False, overrides=overrides, ) if subtree.children is None: @@ -159,10 +160,42 @@ def _expand_virtual_root( return root +def _check_not_missing( + repo: IConfigRepository, + default: InputDefault, + skip_missing: bool, +) -> bool: + path = default.get_config_path() + if path.endswith("???"): + if skip_missing: + return True + if isinstance(default, GroupDefault): + group_path = default.get_group_path() + options = repo.get_group_options( + group_path, + results_filter=ObjectType.CONFIG, + ) + opt_list = "\n".join(["\t" + x for x in options]) + msg = dedent( + f"""\ + You must specify '{group_path}', e.g, {group_path}=