From 6f8e29e241d3466ce37e99a43cfedfe045bc5969 Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Fri, 12 Feb 2021 11:49:58 -0500 Subject: [PATCH 01/42] Improve handling of interpolations pointing to missing nodes * Interpolations are never considered to be missing anymore, even if they point to a missing node * When resolving an expression containing an interpolation pointing to a missing node, if `throw_on_missing` is False, then the result is always `None` (while it used to either be a missing Node, or an expression computed from the "???" string) * Similarly, this commit also ensures that if `throw_on_resolution_failure` is False, then resolving an interpolation resulting in a resolution failure always leads to the result being `None` (instead of potentially being an expression computed from `None`) Fixes #543 --- news/{462.api_change.1 => 462.api_change} | 0 news/462.api_change.2 | 1 - news/543.api_change | 1 + omegaconf/base.py | 80 +++++++++-------------- tests/test_basic_ops_dict.py | 4 +- tests/test_interpolation.py | 22 ++++++- tests/test_matrix.py | 4 +- tests/test_omegaconf.py | 21 ++++-- 8 files changed, 74 insertions(+), 59 deletions(-) rename news/{462.api_change.1 => 462.api_change} (100%) delete mode 100644 news/462.api_change.2 create mode 100644 news/543.api_change diff --git a/news/462.api_change.1 b/news/462.api_change similarity index 100% rename from news/462.api_change.1 rename to news/462.api_change diff --git a/news/462.api_change.2 b/news/462.api_change.2 deleted file mode 100644 index b1fcd583c..000000000 --- a/news/462.api_change.2 +++ /dev/null @@ -1 +0,0 @@ -String interpolation interpolating a missing key no longer counts as missing. diff --git a/news/543.api_change b/news/543.api_change new file mode 100644 index 000000000..d70aac081 --- /dev/null +++ b/news/543.api_change @@ -0,0 +1 @@ +Interpolations interpolating a missing key no longer count as missing. diff --git a/omegaconf/base.py b/omegaconf/base.py index a8617270b..5ddc5d86c 100644 --- a/omegaconf/base.py +++ b/omegaconf/base.py @@ -410,13 +410,20 @@ def _resolve_interpolation_from_parse_tree( ) -> Optional["Node"]: from .nodes import StringNode - resolved = self.resolve_parse_tree( - parse_tree=parse_tree, - key=key, - parent=parent, - throw_on_missing=throw_on_missing, - throw_on_resolution_failure=throw_on_resolution_failure, - ) + try: + resolved = self.resolve_parse_tree( + parse_tree=parse_tree, + key=key, + parent=parent, + ) + except MissingMandatoryValue: + if throw_on_missing: + raise + return None + except Exception: + if throw_on_resolution_failure: + raise + return None if resolved is None: return None @@ -435,36 +442,28 @@ def _resolve_interpolation_from_parse_tree( def _resolve_node_interpolation( self, inter_key: str, - throw_on_missing: bool, - throw_on_resolution_failure: bool, - ) -> Optional["Node"]: + ) -> "Node": """A node interpolation is of the form `${foo.bar}`""" root_node, inter_key = self._resolve_key_and_root(inter_key) parent, last_key, value = root_node._select_impl( inter_key, - throw_on_missing=throw_on_missing, - throw_on_resolution_failure=throw_on_resolution_failure, + throw_on_missing=True, + throw_on_resolution_failure=True, ) - if parent is None or value is None: - if throw_on_resolution_failure: - raise InterpolationResolutionError( - f"Interpolation key '{inter_key}' not found" - ) - else: - return None - assert isinstance(value, Node) - return value + raise InterpolationResolutionError( + f"Interpolation key '{inter_key}' not found" + ) + else: + return value def _evaluate_custom_resolver( self, key: Any, inter_type: str, inter_args: Tuple[Any, ...], - throw_on_missing: bool, - throw_on_resolution_failure: bool, inter_args_str: Tuple[str, ...], - ) -> Optional["Node"]: + ) -> Any: from omegaconf import OmegaConf from .nodes import ValueNode @@ -482,18 +481,11 @@ def _evaluate_custom_resolver( ), ) except Exception as e: - if throw_on_resolution_failure: - self._format_and_raise(key=None, value=None, cause=e) - assert False - else: - return None + self._format_and_raise(key=None, value=None, cause=e) else: - if throw_on_resolution_failure: - raise UnsupportedInterpolationType( - f"Unsupported interpolation type {inter_type}" - ) - else: - return None + raise UnsupportedInterpolationType( + f"Unsupported interpolation type {inter_type}" + ) def _maybe_resolve_interpolation( self, @@ -522,8 +514,6 @@ def resolve_parse_tree( parse_tree: ParserRuleContext, key: Optional[Any] = None, parent: Optional["Container"] = None, - throw_on_missing: bool = True, - throw_on_resolution_failure: bool = True, ) -> Any: """ Resolve a given parse tree into its value. @@ -533,26 +523,17 @@ def resolve_parse_tree( """ from .nodes import StringNode - # Common arguments to all callbacks. - callback_args: Dict[str, Any] = dict( - throw_on_missing=throw_on_missing, - throw_on_resolution_failure=throw_on_resolution_failure, - ) - def node_interpolation_callback(inter_key: str) -> Optional["Node"]: - return self._resolve_node_interpolation( - inter_key=inter_key, **callback_args - ) + return self._resolve_node_interpolation(inter_key=inter_key) def resolver_interpolation_callback( name: str, args: Tuple[Any, ...], args_str: Tuple[str, ...] - ) -> Optional["Node"]: + ) -> Any: return self._evaluate_custom_resolver( key=key, inter_type=name, inter_args=args, inter_args_str=args_str, - **callback_args, ) def quoted_string_callback(quoted_str: str) -> str: @@ -565,7 +546,8 @@ def quoted_string_callback(quoted_str: str) -> str: parent=parent, is_optional=False, ), - **callback_args, + throw_on_missing=True, + throw_on_resolution_failure=True, ) return str(quoted_val) diff --git a/tests/test_basic_ops_dict.py b/tests/test_basic_ops_dict.py index 88ecc4331..3f3d5ecee 100644 --- a/tests/test_basic_ops_dict.py +++ b/tests/test_basic_ops_dict.py @@ -802,10 +802,10 @@ def test_is_missing() -> None: } ) assert cfg._get_node("foo")._is_missing() # type: ignore - assert cfg._get_node("inter")._is_missing() # type: ignore + assert not cfg._get_node("inter")._is_missing() # type: ignore assert not cfg._get_node("str_inter")._is_missing() # type: ignore assert cfg._get_node("missing_node")._is_missing() # type: ignore - assert cfg._get_node("missing_node_inter")._is_missing() # type: ignore + assert not cfg._get_node("missing_node_inter")._is_missing() # type: ignore @pytest.mark.parametrize("ref_type", [None, Any]) diff --git a/tests/test_interpolation.py b/tests/test_interpolation.py index 40bb7f8c7..9b185c153 100644 --- a/tests/test_interpolation.py +++ b/tests/test_interpolation.py @@ -103,8 +103,9 @@ def test_interpolation_with_missing() -> None: "x": {"missing": "???"}, } ) + assert OmegaConf.is_missing(cfg.x, "missing") assert not OmegaConf.is_missing(cfg, "a") - assert OmegaConf.is_missing(cfg, "b") + assert not OmegaConf.is_missing(cfg, "b") def test_assign_to_interpolation() -> None: @@ -709,3 +710,22 @@ def test_empty_stack() -> None: """ with pytest.raises(GrammarParseError): grammar_parser.parse("ab}", lexer_mode="VALUE_MODE") + + +@pytest.mark.parametrize("ref", ["missing", "invalid"]) +def test_invalid_intermediate_result_when_not_throwing( + ref: str, restore_resolvers: Any +) -> None: + """ + Check that the resolution of nested interpolations stops on missing / resolution failure. + """ + OmegaConf.register_new_resolver("check_none", lambda x: x is None) + cfg = OmegaConf.create({"x": f"${{check_none:${{{ref}}}}}", "missing": "???"}) + x_node = cfg._get_node("x") + assert isinstance(x_node, Node) + assert ( + x_node._dereference_node( + throw_on_missing=False, throw_on_resolution_failure=False + ) + is None + ) diff --git a/tests/test_matrix.py b/tests/test_matrix.py index 74ce9b835..21007b374 100644 --- a/tests/test_matrix.py +++ b/tests/test_matrix.py @@ -298,9 +298,9 @@ def test_interpolation( inter=True, exp=None, ) - verify(cfg, "int_missing", none=False, opt=True, missing=True, inter=True) + verify(cfg, "int_missing", none=False, opt=True, missing=False, inter=True) verify( - cfg, "int_opt_missing", none=False, opt=True, missing=True, inter=True + cfg, "int_opt_missing", none=False, opt=True, missing=False, inter=True ) verify( diff --git a/tests/test_omegaconf.py b/tests/test_omegaconf.py index a7997beb4..ddf4f3685 100644 --- a/tests/test_omegaconf.py +++ b/tests/test_omegaconf.py @@ -12,6 +12,7 @@ IntegerNode, ListConfig, MissingMandatoryValue, + Node, OmegaConf, StringNode, ) @@ -39,7 +40,7 @@ pytest.param( {"foo": "${bar}", "bar": DictConfig(content=MISSING)}, "foo", - True, + False, raises(MissingMandatoryValue), id="missing_interpolated_dict", ), @@ -57,7 +58,12 @@ raises(MissingMandatoryValue), id="missing_dict", ), - ({"foo": "${bar}", "bar": MISSING}, "foo", True, raises(MissingMandatoryValue)), + ( + {"foo": "${bar}", "bar": MISSING}, + "foo", + False, + raises(MissingMandatoryValue), + ), ( {"foo": "foo_${bar}", "bar": MISSING}, "foo", @@ -74,7 +80,7 @@ ( {"foo": StringNode(value="???"), "inter": "${foo}"}, "inter", - True, + False, raises(MissingMandatoryValue), ), (StructuredWithMissing, "num", True, raises(MissingMandatoryValue)), @@ -87,7 +93,7 @@ (StructuredWithMissing, "opt_user", True, raises(MissingMandatoryValue)), (StructuredWithMissing, "inter_user", True, raises(MissingMandatoryValue)), (StructuredWithMissing, "inter_opt_user", True, raises(MissingMandatoryValue)), - (StructuredWithMissing, "inter_num", True, raises(MissingMandatoryValue)), + (StructuredWithMissing, "inter_num", False, raises(MissingMandatoryValue)), ], ) def test_is_missing( @@ -117,6 +123,13 @@ def test_is_missing_resets() -> None: assert OmegaConf.is_missing(cfg, "list") +def test_dereference_interpolation_to_missing() -> None: + cfg = OmegaConf.create({"x": "${y}", "y": "???"}) + x_node = cfg._get_node("x") + assert isinstance(x_node, Node) + assert x_node._dereference_node() is None + + @pytest.mark.parametrize( "cfg, expected", [ From 7de6b6271f324f80faabedc64a541e37f9a00c5b Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Fri, 12 Feb 2021 17:42:30 -0500 Subject: [PATCH 02/42] Add exception check to `test_dereference_interpolation_to_missing` --- tests/test_omegaconf.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test_omegaconf.py b/tests/test_omegaconf.py index ddf4f3685..832900822 100644 --- a/tests/test_omegaconf.py +++ b/tests/test_omegaconf.py @@ -128,6 +128,8 @@ def test_dereference_interpolation_to_missing() -> None: x_node = cfg._get_node("x") assert isinstance(x_node, Node) assert x_node._dereference_node() is None + with pytest.raises(MissingMandatoryValue): + cfg.x @pytest.mark.parametrize( From 6e03ccef111e13a71af09f6260ba824e48f350db Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Fri, 12 Feb 2021 18:42:03 -0500 Subject: [PATCH 03/42] Better explanation of `test_invalid_intermediate_result_when_not_throwing()` --- tests/test_interpolation.py | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/tests/test_interpolation.py b/tests/test_interpolation.py index 9b185c153..9904344db 100644 --- a/tests/test_interpolation.py +++ b/tests/test_interpolation.py @@ -717,10 +717,19 @@ def test_invalid_intermediate_result_when_not_throwing( ref: str, restore_resolvers: Any ) -> None: """ - Check that the resolution of nested interpolations stops on missing / resolution failure. + Test the handling of missing / resolution failures in nested interpolations. + + The main goal of this test is to make sure that the resolution of an interpolation + is stopped immediately when a missing / resolution failure occurs, even if + `throw_on_missing` and `throw_on_resolution_failure` are set to False. + When this happens while dereferencing a node, the result should be `None`. """ - OmegaConf.register_new_resolver("check_none", lambda x: x is None) - cfg = OmegaConf.create({"x": f"${{check_none:${{{ref}}}}}", "missing": "???"}) + + def fail_if_called(x: Any) -> None: + assert False + + OmegaConf.register_new_resolver("fail_if_called", fail_if_called) + cfg = OmegaConf.create({"x": f"${{fail_if_called:${{{ref}}}}}", "missing": "???"}) x_node = cfg._get_node("x") assert isinstance(x_node, Node) assert ( From 0252661c2d93fbd022157e218c180bd8abd82a90 Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Fri, 12 Feb 2021 18:59:31 -0500 Subject: [PATCH 04/42] Simplify `Node._is_missing()` (and fix associated issue with containers) --- omegaconf/base.py | 8 ++++---- omegaconf/basecontainer.py | 12 ------------ omegaconf/nodes.py | 17 ----------------- tests/test_omegaconf.py | 18 ++++++++++++++++-- 4 files changed, 20 insertions(+), 35 deletions(-) diff --git a/omegaconf/base.py b/omegaconf/base.py index 5ddc5d86c..ac3fcdb4c 100644 --- a/omegaconf/base.py +++ b/omegaconf/base.py @@ -11,6 +11,7 @@ ValueKind, _get_value, _is_missing_literal, + _is_missing_value, format_and_raise, get_value_kind, ) @@ -228,6 +229,9 @@ def _get_root(self) -> "Container": assert root is not None and isinstance(root, Container) return root + def _is_missing(self) -> bool: + return _is_missing_value(self) + @abstractmethod def __eq__(self, other: Any) -> bool: ... @@ -256,10 +260,6 @@ def _is_none(self) -> bool: def _is_optional(self) -> bool: ... - @abstractmethod - def _is_missing(self) -> bool: - ... - @abstractmethod def _is_interpolation(self) -> bool: ... diff --git a/omegaconf/basecontainer.py b/omegaconf/basecontainer.py index 56235aa9c..c01baa268 100644 --- a/omegaconf/basecontainer.py +++ b/omegaconf/basecontainer.py @@ -666,18 +666,6 @@ def _item_eq( def _is_none(self) -> bool: return self.__dict__["_content"] is None - def _is_missing(self) -> bool: - try: - self._dereference_node( - throw_on_resolution_failure=False, throw_on_missing=True - ) - return False - except MissingMandatoryValue: - ret = True - - assert isinstance(ret, bool) - return ret - def _is_optional(self) -> bool: return self.__dict__["_metadata"].optional is True diff --git a/omegaconf/nodes.py b/omegaconf/nodes.py index 85940a9ba..19bea86fe 100644 --- a/omegaconf/nodes.py +++ b/omegaconf/nodes.py @@ -101,23 +101,6 @@ def _is_none(self) -> bool: def _is_optional(self) -> bool: return self._metadata.optional - def _is_missing(self) -> bool: - if self._is_interpolation(): - try: - node = self._dereference_node( - throw_on_resolution_failure=False, throw_on_missing=False - ) - if node is None: - # resolution failure - return False - else: - return node._is_missing() - except ConfigKeyError: - # Will happen if interpolation is accessing keys that does not exist. - return False - else: - return _is_missing_value(self) - def _is_interpolation(self) -> bool: return _is_interpolation(self._value()) diff --git a/tests/test_omegaconf.py b/tests/test_omegaconf.py index 832900822..c2a1e9d76 100644 --- a/tests/test_omegaconf.py +++ b/tests/test_omegaconf.py @@ -58,6 +58,20 @@ raises(MissingMandatoryValue), id="missing_dict", ), + pytest.param( + {"foo": ListConfig(content="${missing}"), "missing": "???"}, + "foo", + False, + raises(MissingMandatoryValue), + id="missing_list_interpolation", + ), + pytest.param( + {"foo": DictConfig(content="${missing}"), "missing": "???"}, + "foo", + False, + raises(MissingMandatoryValue), + id="missing_dict_interpolation", + ), ( {"foo": "${bar}", "bar": MISSING}, "foo", @@ -91,8 +105,8 @@ (StructuredWithMissing, "opt_list", True, raises(MissingMandatoryValue)), (StructuredWithMissing, "user", True, raises(MissingMandatoryValue)), (StructuredWithMissing, "opt_user", True, raises(MissingMandatoryValue)), - (StructuredWithMissing, "inter_user", True, raises(MissingMandatoryValue)), - (StructuredWithMissing, "inter_opt_user", True, raises(MissingMandatoryValue)), + (StructuredWithMissing, "inter_user", False, raises(MissingMandatoryValue)), + (StructuredWithMissing, "inter_opt_user", False, raises(MissingMandatoryValue)), (StructuredWithMissing, "inter_num", False, raises(MissingMandatoryValue)), ], ) From 17ea356de85c0901ac8ad171ae937023f0d429ff Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Fri, 12 Feb 2021 23:38:31 -0500 Subject: [PATCH 05/42] Remove pointless `throw_on_missing` arguments Since an interpolation cannot be missing, the `throw_on_missing` argument should not be passed around when resolving interpolations: encountering a missing value while resolving an interpolation must be considered as a resolution failure. --- omegaconf/base.py | 10 ---------- omegaconf/basecontainer.py | 1 - omegaconf/nodes.py | 6 ++---- tests/test_base_config.py | 1 - tests/test_omegaconf.py | 2 +- 5 files changed, 3 insertions(+), 17 deletions(-) diff --git a/omegaconf/base.py b/omegaconf/base.py index ac3fcdb4c..e6fc5e1c3 100644 --- a/omegaconf/base.py +++ b/omegaconf/base.py @@ -207,7 +207,6 @@ def _dereference_node( key=key, value=self, parse_tree=parse(_get_value(self)), - throw_on_missing=throw_on_missing, throw_on_resolution_failure=throw_on_resolution_failure, ) else: @@ -394,7 +393,6 @@ def _select_impl( parent=root, key=last_key, value=value, - throw_on_missing=throw_on_missing, throw_on_resolution_failure=throw_on_resolution_failure, ) return root, last_key, value @@ -405,7 +403,6 @@ def _resolve_interpolation_from_parse_tree( value: "Node", key: Any, parse_tree: OmegaConfGrammarParser.ConfigValueContext, - throw_on_missing: bool, throw_on_resolution_failure: bool, ) -> Optional["Node"]: from .nodes import StringNode @@ -416,10 +413,6 @@ def _resolve_interpolation_from_parse_tree( key=key, parent=parent, ) - except MissingMandatoryValue: - if throw_on_missing: - raise - return None except Exception: if throw_on_resolution_failure: raise @@ -492,7 +485,6 @@ def _maybe_resolve_interpolation( parent: Optional["Container"], key: Any, value: "Node", - throw_on_missing: bool, throw_on_resolution_failure: bool, ) -> Any: value_kind = get_value_kind(value) @@ -505,7 +497,6 @@ def _maybe_resolve_interpolation( value=value, key=key, parse_tree=parse_tree, - throw_on_missing=throw_on_missing, throw_on_resolution_failure=throw_on_resolution_failure, ) @@ -546,7 +537,6 @@ def quoted_string_callback(quoted_str: str) -> str: parent=parent, is_optional=False, ), - throw_on_missing=True, throw_on_resolution_failure=True, ) return str(quoted_val) diff --git a/omegaconf/basecontainer.py b/omegaconf/basecontainer.py index c01baa268..984510614 100644 --- a/omegaconf/basecontainer.py +++ b/omegaconf/basecontainer.py @@ -59,7 +59,6 @@ def _resolve_with_default( parent=self, key=key, value=value, - throw_on_missing=not has_default, throw_on_resolution_failure=not has_default, ) if resolved is None and has_default: diff --git a/omegaconf/nodes.py b/omegaconf/nodes.py index 19bea86fe..169a268c1 100644 --- a/omegaconf/nodes.py +++ b/omegaconf/nodes.py @@ -88,11 +88,9 @@ def _deepcopy_impl(self, res: Any, memo: Dict[int, Any]) -> None: def _is_none(self) -> bool: if self._is_interpolation(): - node = self._dereference_node( - throw_on_resolution_failure=False, throw_on_missing=False - ) + node = self._dereference_node(throw_on_resolution_failure=False) if node is None: - # missing or resolution failure + # resolution failure return False else: node = self diff --git a/tests/test_base_config.py b/tests/test_base_config.py index cf37ea6fa..8d1d263b5 100644 --- a/tests/test_base_config.py +++ b/tests/test_base_config.py @@ -511,7 +511,6 @@ def test_resolve_str_interpolation(query: str, result: Any) -> None: parent=None, key=None, value=StringNode(value=query), - throw_on_missing=False, throw_on_resolution_failure=True, ) == result diff --git a/tests/test_omegaconf.py b/tests/test_omegaconf.py index c2a1e9d76..183f1b59d 100644 --- a/tests/test_omegaconf.py +++ b/tests/test_omegaconf.py @@ -141,7 +141,7 @@ def test_dereference_interpolation_to_missing() -> None: cfg = OmegaConf.create({"x": "${y}", "y": "???"}) x_node = cfg._get_node("x") assert isinstance(x_node, Node) - assert x_node._dereference_node() is None + assert x_node._dereference_node(throw_on_resolution_failure=False) is None with pytest.raises(MissingMandatoryValue): cfg.x From 290987552b981773c597e85d8c0de380d9c59dec Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Sat, 13 Feb 2021 01:20:08 -0500 Subject: [PATCH 06/42] Fix coverage --- omegaconf/base.py | 5 ++--- omegaconf/basecontainer.py | 15 +++++++-------- tests/test_omegaconf.py | 8 ++++++++ 3 files changed, 17 insertions(+), 11 deletions(-) diff --git a/omegaconf/base.py b/omegaconf/base.py index e6fc5e1c3..5b0695ec8 100644 --- a/omegaconf/base.py +++ b/omegaconf/base.py @@ -418,9 +418,8 @@ def _resolve_interpolation_from_parse_tree( raise return None - if resolved is None: - return None - elif isinstance(resolved, str): + assert resolved is not None + if isinstance(resolved, str): # Result is a string: create a new StringNode for it. return StringNode( value=resolved, diff --git a/omegaconf/basecontainer.py b/omegaconf/basecontainer.py index 984510614..33ccb7319 100644 --- a/omegaconf/basecontainer.py +++ b/omegaconf/basecontainer.py @@ -50,11 +50,16 @@ def _resolve_with_default( ) -> Any: """returns the value with the specified key, like obj.key and obj['key']""" - val = _get_value(value) has_default = default_value is not DEFAULT_VALUE_MARKER - if has_default and (val is None or _is_missing_value(val)): + + if has_default and _get_value(value) is None: return default_value + if _is_missing_value(value): + if has_default: + return default_value + raise MissingMandatoryValue("Missing mandatory value: $FULL_KEY") + resolved = self._maybe_resolve_interpolation( parent=self, key=key, @@ -64,12 +69,6 @@ def _resolve_with_default( if resolved is None and has_default: return default_value - if _is_missing_value(resolved): - if has_default: - return default_value - else: - raise MissingMandatoryValue("Missing mandatory value: $FULL_KEY") - return _get_value(resolved) def __str__(self) -> str: diff --git a/tests/test_omegaconf.py b/tests/test_omegaconf.py index 183f1b59d..75a561977 100644 --- a/tests/test_omegaconf.py +++ b/tests/test_omegaconf.py @@ -137,6 +137,14 @@ def test_is_missing_resets() -> None: assert OmegaConf.is_missing(cfg, "list") +def test_dereference_missing() -> None: + cfg = OmegaConf.create({"x": "???"}) + x_node = cfg._get_node("x") + assert isinstance(x_node, Node) + with pytest.raises(MissingMandatoryValue): + x_node._dereference_node(throw_on_missing=True) + + def test_dereference_interpolation_to_missing() -> None: cfg = OmegaConf.create({"x": "${y}", "y": "???"}) x_node = cfg._get_node("x") From c9345b57041c05118724619c1ea6915dd078bffe Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Tue, 16 Feb 2021 17:47:39 -0500 Subject: [PATCH 07/42] Fix error message in case of error when calling a resolver --- omegaconf/base.py | 19 ++++++++----------- tests/test_errors.py | 19 +++++++++++++++++++ 2 files changed, 27 insertions(+), 11 deletions(-) diff --git a/omegaconf/base.py b/omegaconf/base.py index 5b0695ec8..f0a232efc 100644 --- a/omegaconf/base.py +++ b/omegaconf/base.py @@ -463,17 +463,14 @@ def _evaluate_custom_resolver( resolver = OmegaConf.get_resolver(inter_type) if resolver is not None: root_node = self._get_root() - try: - value = resolver(root_node, inter_args, inter_args_str) - return ValueNode( - value=value, - parent=self, - metadata=Metadata( - ref_type=Any, object_type=Any, key=key, optional=True - ), - ) - except Exception as e: - self._format_and_raise(key=None, value=None, cause=e) + value = resolver(root_node, inter_args, inter_args_str) + return ValueNode( + value=value, + parent=self, + metadata=Metadata( + ref_type=Any, object_type=Any, key=key, optional=True + ), + ) else: raise UnsupportedInterpolationType( f"Unsupported interpolation type {inter_type}" diff --git a/tests/test_errors.py b/tests/test_errors.py index 1ccc811bd..7510022bf 100644 --- a/tests/test_errors.py +++ b/tests/test_errors.py @@ -1248,6 +1248,25 @@ def assert_false() -> None: c.trigger +@pytest.mark.parametrize( + "register_func", [OmegaConf.register_resolver, OmegaConf.register_new_resolver] +) +def test_resolver_error(restore_resolvers: Any, register_func: Any) -> None: + def div(x: Any, y: Any) -> float: + return float(x) / float(y) + + register_func("div", div) + c = OmegaConf.create({"div_by_zero": "${div:1,0}"}) + expected_msg = dedent( + """\ + float division by zero + full_key: div_by_zero + object_type=dict""" + ) + with pytest.raises(ZeroDivisionError, match=re.escape(expected_msg)): + c.div_by_zero + + @pytest.mark.parametrize( ["create_func", "arg"], [ From 7964e8087826c493417ab4c5677163827aae242a Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Tue, 16 Feb 2021 17:53:47 -0500 Subject: [PATCH 08/42] Use % for more readable string --- tests/test_interpolation.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_interpolation.py b/tests/test_interpolation.py index 9904344db..08b08e4ac 100644 --- a/tests/test_interpolation.py +++ b/tests/test_interpolation.py @@ -729,7 +729,7 @@ def fail_if_called(x: Any) -> None: assert False OmegaConf.register_new_resolver("fail_if_called", fail_if_called) - cfg = OmegaConf.create({"x": f"${{fail_if_called:${{{ref}}}}}", "missing": "???"}) + cfg = OmegaConf.create({"x": "${fail_if_called:${%s}}" % ref, "missing": "???"}) x_node = cfg._get_node("x") assert isinstance(x_node, Node) assert ( From d907f636c423099a7f5bb3c3e236511361cd132b Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Tue, 16 Feb 2021 18:00:05 -0500 Subject: [PATCH 09/42] Move test for dereference_node() to test_nodes.py --- tests/test_nodes.py | 24 +++++++++++++++++++++++- tests/test_omegaconf.py | 18 ------------------ 2 files changed, 23 insertions(+), 19 deletions(-) diff --git a/tests/test_nodes.py b/tests/test_nodes.py index c6ad1ab9f..dbef962a8 100644 --- a/tests/test_nodes.py +++ b/tests/test_nodes.py @@ -12,11 +12,16 @@ FloatNode, IntegerNode, ListConfig, + Node, OmegaConf, StringNode, ValueNode, ) -from omegaconf.errors import UnsupportedValueType, ValidationError +from omegaconf.errors import ( + MissingMandatoryValue, + UnsupportedValueType, + ValidationError, +) from tests import Color, IllegalType, User @@ -559,3 +564,20 @@ def test_allow_objects() -> None: iv = IllegalType() c.foo = iv assert c.foo == iv + + +def test_dereference_missing() -> None: + cfg = OmegaConf.create({"x": "???"}) + x_node = cfg._get_node("x") + assert isinstance(x_node, Node) + with pytest.raises(MissingMandatoryValue): + x_node._dereference_node(throw_on_missing=True) + + +def test_dereference_interpolation_to_missing() -> None: + cfg = OmegaConf.create({"x": "${y}", "y": "???"}) + x_node = cfg._get_node("x") + assert isinstance(x_node, Node) + assert x_node._dereference_node(throw_on_resolution_failure=False) is None + with pytest.raises(MissingMandatoryValue): + cfg.x diff --git a/tests/test_omegaconf.py b/tests/test_omegaconf.py index 75a561977..7d126a681 100644 --- a/tests/test_omegaconf.py +++ b/tests/test_omegaconf.py @@ -12,7 +12,6 @@ IntegerNode, ListConfig, MissingMandatoryValue, - Node, OmegaConf, StringNode, ) @@ -137,23 +136,6 @@ def test_is_missing_resets() -> None: assert OmegaConf.is_missing(cfg, "list") -def test_dereference_missing() -> None: - cfg = OmegaConf.create({"x": "???"}) - x_node = cfg._get_node("x") - assert isinstance(x_node, Node) - with pytest.raises(MissingMandatoryValue): - x_node._dereference_node(throw_on_missing=True) - - -def test_dereference_interpolation_to_missing() -> None: - cfg = OmegaConf.create({"x": "${y}", "y": "???"}) - x_node = cfg._get_node("x") - assert isinstance(x_node, Node) - assert x_node._dereference_node(throw_on_resolution_failure=False) is None - with pytest.raises(MissingMandatoryValue): - cfg.x - - @pytest.mark.parametrize( "cfg, expected", [ From 759dcf067f4a98da83c42f35f7ee7107ddf29eec Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Tue, 16 Feb 2021 21:09:32 -0500 Subject: [PATCH 10/42] Better code formatting for readability --- tests/test_interpolation.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/test_interpolation.py b/tests/test_interpolation.py index 08b08e4ac..f6ca64035 100644 --- a/tests/test_interpolation.py +++ b/tests/test_interpolation.py @@ -729,7 +729,12 @@ def fail_if_called(x: Any) -> None: assert False OmegaConf.register_new_resolver("fail_if_called", fail_if_called) - cfg = OmegaConf.create({"x": "${fail_if_called:${%s}}" % ref, "missing": "???"}) + cfg = OmegaConf.create( + { + "x": "${fail_if_called:${%s}}" % ref, + "missing": "???", + } + ) x_node = cfg._get_node("x") assert isinstance(x_node, Node) assert ( From 3e455cf7ec6de871b9aa22d04022692f0da0f849 Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Tue, 16 Feb 2021 21:13:47 -0500 Subject: [PATCH 11/42] Rename variable to make it clear it is a node --- omegaconf/basecontainer.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/omegaconf/basecontainer.py b/omegaconf/basecontainer.py index 33ccb7319..6e0989c88 100644 --- a/omegaconf/basecontainer.py +++ b/omegaconf/basecontainer.py @@ -60,16 +60,16 @@ def _resolve_with_default( return default_value raise MissingMandatoryValue("Missing mandatory value: $FULL_KEY") - resolved = self._maybe_resolve_interpolation( + resolved_node = self._maybe_resolve_interpolation( parent=self, key=key, value=value, throw_on_resolution_failure=not has_default, ) - if resolved is None and has_default: + if resolved_node is None and has_default: return default_value - return _get_value(resolved) + return _get_value(resolved_node) def __str__(self) -> str: return self.__repr__() From a6357f615a3247a9825312a0d885758d1052f61f Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Mon, 22 Feb 2021 19:47:04 -0500 Subject: [PATCH 12/42] Fix missing import after rebase --- tests/test_errors.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_errors.py b/tests/test_errors.py index 7510022bf..97c5546b1 100644 --- a/tests/test_errors.py +++ b/tests/test_errors.py @@ -1,6 +1,7 @@ import re from dataclasses import dataclass from enum import Enum +from textwrap import dedent from typing import Any, Dict, List, Optional, Type import pytest From 0aed761e80df25b73fb7d7d9b9652564c9f5bb55 Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Mon, 22 Feb 2021 21:15:47 -0500 Subject: [PATCH 13/42] Rename InterpolationResolutionError -> InterpolationKeyError --- omegaconf/base.py | 6 ++---- omegaconf/dictconfig.py | 4 ++-- omegaconf/errors.py | 2 +- omegaconf/omegaconf.py | 8 ++++---- tests/test_basic_ops_list.py | 4 ++-- tests/test_errors.py | 8 ++++---- tests/test_grammar.py | 6 +++--- tests/test_interpolation.py | 12 ++++++------ tests/test_omegaconf.py | 4 ++-- 9 files changed, 26 insertions(+), 28 deletions(-) diff --git a/omegaconf/base.py b/omegaconf/base.py index f0a232efc..f424f9a4d 100644 --- a/omegaconf/base.py +++ b/omegaconf/base.py @@ -17,7 +17,7 @@ ) from .errors import ( ConfigKeyError, - InterpolationResolutionError, + InterpolationKeyError, MissingMandatoryValue, OmegaConfBaseException, UnsupportedInterpolationType, @@ -443,9 +443,7 @@ def _resolve_node_interpolation( throw_on_resolution_failure=True, ) if parent is None or value is None: - raise InterpolationResolutionError( - f"Interpolation key '{inter_key}' not found" - ) + raise InterpolationKeyError(f"Interpolation key '{inter_key}' not found") else: return value diff --git a/omegaconf/dictconfig.py b/omegaconf/dictconfig.py index e964af38e..ca5ff30ed 100644 --- a/omegaconf/dictconfig.py +++ b/omegaconf/dictconfig.py @@ -39,7 +39,7 @@ ConfigAttributeError, ConfigKeyError, ConfigTypeError, - InterpolationResolutionError, + InterpolationKeyError, KeyValidationError, MissingMandatoryValue, OmegaConfBaseException, @@ -518,7 +518,7 @@ def __contains__(self, key: object) -> bool: except UnsupportedInterpolationType: # Value that has unsupported interpolation counts as existing. return True - except (MissingMandatoryValue, KeyError, InterpolationResolutionError): + except (MissingMandatoryValue, KeyError, InterpolationKeyError): return False def __iter__(self) -> Iterator[DictKeyType]: diff --git a/omegaconf/errors.py b/omegaconf/errors.py index 0ff62e601..b35a89902 100644 --- a/omegaconf/errors.py +++ b/omegaconf/errors.py @@ -63,7 +63,7 @@ class UnsupportedInterpolationType(OmegaConfBaseException, ValueError): """ -class InterpolationResolutionError(OmegaConfBaseException, ValueError): +class InterpolationKeyError(OmegaConfBaseException, ValueError): """ Thrown an error occurs when resolving an interpolation """ diff --git a/omegaconf/omegaconf.py b/omegaconf/omegaconf.py index 5d2cbf361..dc351c9f1 100644 --- a/omegaconf/omegaconf.py +++ b/omegaconf/omegaconf.py @@ -52,7 +52,7 @@ from .errors import ( ConfigKeyError, GrammarParseError, - InterpolationResolutionError, + InterpolationKeyError, MissingMandatoryValue, OmegaConfBaseException, UnsupportedInterpolationType, @@ -124,8 +124,8 @@ def env(key: str, default: Any = _EMPTY_MARKER_) -> Any: empty_config = DictConfig({}) try: val = empty_config.resolve_parse_tree(parse_tree) - except InterpolationResolutionError as exc: - raise InterpolationResolutionError( + except InterpolationKeyError as exc: + raise InterpolationKeyError( f"When attempting to resolve env variable '{key}', a node interpolation " f"caused the following exception: {exc}. Node interpolations are not " f"supported in environment variables: either remove them, or escape " @@ -732,7 +732,7 @@ def select( throw_on_missing=throw_on_missing, throw_on_resolution_failure=throw_on_resolution_failure, ) - except (ConfigKeyError, InterpolationResolutionError): + except (ConfigKeyError, InterpolationKeyError): if default is not _EMPTY_MARKER_: return default else: diff --git a/tests/test_basic_ops_list.py b/tests/test_basic_ops_list.py index ef7bddb84..efff9f49f 100644 --- a/tests/test_basic_ops_list.py +++ b/tests/test_basic_ops_list.py @@ -8,7 +8,7 @@ from omegaconf import MISSING, AnyNode, ListConfig, OmegaConf, flag_override from omegaconf.errors import ( ConfigTypeError, - InterpolationResolutionError, + InterpolationKeyError, KeyValidationError, MissingMandatoryValue, UnsupportedValueType, @@ -70,7 +70,7 @@ def test_iterate_list_with_missing_interpolation() -> None: c = OmegaConf.create([1, "${10}"]) itr = iter(c) assert 1 == next(itr) - with pytest.raises(InterpolationResolutionError): + with pytest.raises(InterpolationKeyError): next(itr) diff --git a/tests/test_errors.py b/tests/test_errors.py index 97c5546b1..ea40cbdae 100644 --- a/tests/test_errors.py +++ b/tests/test_errors.py @@ -23,7 +23,7 @@ ConfigTypeError, ConfigValueError, GrammarParseError, - InterpolationResolutionError, + InterpolationKeyError, KeyValidationError, MissingMandatoryValue, OmegaConfBaseException, @@ -220,7 +220,7 @@ def finalize(self, cfg: Any) -> None: Expected( create=lambda: OmegaConf.create({"foo": "${missing}"}), op=lambda cfg: getattr(cfg, "foo"), - exception_type=InterpolationResolutionError, + exception_type=InterpolationKeyError, msg="Interpolation key 'missing' not found", key="foo", child_node=lambda cfg: cfg._get_node("foo"), @@ -231,7 +231,7 @@ def finalize(self, cfg: Any) -> None: Expected( create=lambda: OmegaConf.create({"foo": "foo_${missing}"}), op=lambda cfg: getattr(cfg, "foo"), - exception_type=InterpolationResolutionError, + exception_type=InterpolationKeyError, msg="Interpolation key 'missing' not found", key="foo", child_node=lambda cfg: cfg._get_node("foo"), @@ -242,7 +242,7 @@ def finalize(self, cfg: Any) -> None: Expected( create=lambda: OmegaConf.create({"foo": {"bar": "${.missing}"}}), op=lambda cfg: getattr(cfg.foo, "bar"), - exception_type=InterpolationResolutionError, + exception_type=InterpolationKeyError, msg="Interpolation key 'missing' not found", key="bar", full_key="foo.bar", diff --git a/tests/test_grammar.py b/tests/test_grammar.py index 28f5b07b8..9f9b2ff88 100644 --- a/tests/test_grammar.py +++ b/tests/test_grammar.py @@ -14,7 +14,7 @@ ) from omegaconf.errors import ( GrammarParseError, - InterpolationResolutionError, + InterpolationKeyError, UnsupportedInterpolationType, ) @@ -187,12 +187,12 @@ ("dict_access", "${dict.a}", 0), ("list_access", "${list.0}", -1), ("list_access_underscore", "${list.1_0}", 9), - ("list_access_bad_negative", "${list.-1}", InterpolationResolutionError), + ("list_access_bad_negative", "${list.-1}", InterpolationKeyError), ("dict_access_list_like_1", "${0}", 0), ("dict_access_list_like_2", "${1.2}", 12), ("bool_like_keys", "${FalsE.TruE}", True), ("null_like_key_ok", "${None.null}", 1), - ("null_like_key_bad_case", "${NoNe.null}", InterpolationResolutionError), + ("null_like_key_bad_case", "${NoNe.null}", InterpolationKeyError), ("null_like_key_quoted_1", "${'None'.'null'}", GrammarParseError), ("null_like_key_quoted_2", "${'None.null'}", GrammarParseError), ("dotpath_bad_type", "${dict.${float}}", (None, GrammarParseError)), diff --git a/tests/test_interpolation.py b/tests/test_interpolation.py index f6ca64035..7ae642118 100644 --- a/tests/test_interpolation.py +++ b/tests/test_interpolation.py @@ -21,7 +21,7 @@ from omegaconf.errors import ( ConfigAttributeError, GrammarParseError, - InterpolationResolutionError, + InterpolationKeyError, OmegaConfBaseException, UnsupportedInterpolationType, ) @@ -44,13 +44,13 @@ pytest.param( {"a": "${x}"}, "a", - pytest.raises(InterpolationResolutionError), + pytest.raises(InterpolationKeyError), id="not_found", ), pytest.param( {"a": "${x.y}"}, "a", - pytest.raises(InterpolationResolutionError), + pytest.raises(InterpolationKeyError), id="not_found", ), pytest.param({"a": "foo_${b}", "b": "bar"}, "a", "foo_bar", id="str_inter"), @@ -321,7 +321,7 @@ def test_env_node_interpolation(monkeypatch: Any) -> None: # Test that node interpolations are not supported in env variables. monkeypatch.setenv("my_key", "${other_key}") c = OmegaConf.create(dict(my_key="${env:my_key}", other_key=123)) - with pytest.raises(InterpolationResolutionError): + with pytest.raises(InterpolationKeyError): c.my_key @@ -618,7 +618,7 @@ def create() -> DictConfig: # With '.', we try to access `${ab.de}`. # With '}', we try to access `${ab}`. cfg = create() - with pytest.raises(InterpolationResolutionError): + with pytest.raises(InterpolationKeyError): cfg.invalid elif c == ":": # With ':', we try to run a resolver `${ab:de}` @@ -635,7 +635,7 @@ def test_interpolation_in_list_key_error() -> None: # Test that a KeyError is thrown if an str_interpolation key is not available c = OmegaConf.create(["${10}"]) - with pytest.raises(InterpolationResolutionError): + with pytest.raises(InterpolationKeyError): c[0] diff --git a/tests/test_omegaconf.py b/tests/test_omegaconf.py index 7d126a681..43f567541 100644 --- a/tests/test_omegaconf.py +++ b/tests/test_omegaconf.py @@ -17,7 +17,7 @@ ) from omegaconf.errors import ( ConfigKeyError, - InterpolationResolutionError, + InterpolationKeyError, UnsupportedInterpolationType, ) from tests import ( @@ -34,7 +34,7 @@ [ ({}, "foo", False, raises(ConfigKeyError)), ({"foo": True}, "foo", False, does_not_raise()), - ({"foo": "${no_such_key}"}, "foo", False, raises(InterpolationResolutionError)), + ({"foo": "${no_such_key}"}, "foo", False, raises(InterpolationKeyError)), ({"foo": MISSING}, "foo", True, raises(MissingMandatoryValue)), pytest.param( {"foo": "${bar}", "bar": DictConfig(content=MISSING)}, From 0f6aeaee3f3ae22a286aa56470ecfa60dfc04e52 Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Mon, 22 Feb 2021 22:33:57 -0500 Subject: [PATCH 14/42] Use a common exception class for interpolation errors All exceptions raised while resolving interpolations now inherit from InterpolationResolutionError. One potential issue to address is what to do with `key in cfg` when `key` is an interpolation pointing to a missing node. Currently this will raise an exception (while it used to return False). --- omegaconf/base.py | 34 +++++++++++++++++++++++++++------- omegaconf/errors.py | 18 +++++++++++++++--- omegaconf/grammar_visitor.py | 6 +++--- omegaconf/omegaconf.py | 6 +++--- tests/test_basic_ops_dict.py | 20 ++++++++++++++++++-- tests/test_errors.py | 5 +++-- tests/test_grammar.py | 5 +++-- tests/test_interpolation.py | 11 +++++++---- tests/test_nodes.py | 3 ++- tests/test_omegaconf.py | 34 +++++++++++++++++++++++++--------- 10 files changed, 106 insertions(+), 36 deletions(-) diff --git a/omegaconf/base.py b/omegaconf/base.py index f424f9a4d..cd2a5cb93 100644 --- a/omegaconf/base.py +++ b/omegaconf/base.py @@ -18,6 +18,8 @@ from .errors import ( ConfigKeyError, InterpolationKeyError, + InterpolationResolutionError, + InterpolationToMissingValueError, MissingMandatoryValue, OmegaConfBaseException, UnsupportedInterpolationType, @@ -436,12 +438,22 @@ def _resolve_node_interpolation( inter_key: str, ) -> "Node": """A node interpolation is of the form `${foo.bar}`""" - root_node, inter_key = self._resolve_key_and_root(inter_key) - parent, last_key, value = root_node._select_impl( - inter_key, - throw_on_missing=True, - throw_on_resolution_failure=True, - ) + try: + root_node, inter_key = self._resolve_key_and_root(inter_key) + except ConfigKeyError as exc: + raise InterpolationKeyError(str(exc)) + + try: + parent, last_key, value = root_node._select_impl( + inter_key, + throw_on_missing=True, + throw_on_resolution_failure=True, + ) + except MissingMandatoryValue as exc: + raise InterpolationToMissingValueError(str(exc)) + except ConfigKeyError as exc: + raise InterpolationKeyError(str(exc)) + if parent is None or value is None: raise InterpolationKeyError(f"Interpolation key '{inter_key}' not found") else: @@ -540,7 +552,15 @@ def quoted_string_callback(quoted_str: str) -> str: resolver_interpolation_callback=resolver_interpolation_callback, quoted_string_callback=quoted_string_callback, ) - return visitor.visit(parse_tree) + try: + return visitor.visit(parse_tree) + except InterpolationResolutionError: + raise + except Exception as exc: + # Other kinds of exceptions are wrapped in an `InterpolationResolutionError`. + raise InterpolationResolutionError( + f"{type(exc).__name__} raised while resolving interpolation: {exc}" + ) def _re_parent(self) -> None: from .dictconfig import DictConfig diff --git a/omegaconf/errors.py b/omegaconf/errors.py index b35a89902..a7a2cf858 100644 --- a/omegaconf/errors.py +++ b/omegaconf/errors.py @@ -57,15 +57,27 @@ class ReadonlyConfigError(OmegaConfBaseException): """ -class UnsupportedInterpolationType(OmegaConfBaseException, ValueError): +class InterpolationResolutionError(OmegaConfBaseException, ValueError): + """ + Base class for exceptions raised when resolving an interpolation. + """ + + +class UnsupportedInterpolationType(InterpolationResolutionError): """ Thrown when an attempt to use an unregistered interpolation is made """ -class InterpolationKeyError(OmegaConfBaseException, ValueError): +class InterpolationKeyError(InterpolationResolutionError): + """ + Thrown when a node does not exist when resolving an interpolation. + """ + + +class InterpolationToMissingValueError(InterpolationResolutionError): """ - Thrown an error occurs when resolving an interpolation + Thrown when a node interpolation points to a node that is set to ???. """ diff --git a/omegaconf/grammar_visitor.py b/omegaconf/grammar_visitor.py index debfa7f52..0a8ae3e86 100644 --- a/omegaconf/grammar_visitor.py +++ b/omegaconf/grammar_visitor.py @@ -14,7 +14,7 @@ from antlr4 import TerminalNode -from .errors import GrammarParseError +from .errors import InterpolationResolutionError if TYPE_CHECKING: from .base import Node # noqa F401 @@ -83,7 +83,7 @@ def visitConfigKey(self, ctx: OmegaConfGrammarParser.ConfigKeyContext) -> str: if isinstance(child, OmegaConfGrammarParser.InterpolationContext): res = _get_value(self.visitInterpolation(child)) if not isinstance(res, str): - raise GrammarParseError( + raise InterpolationResolutionError( f"The following interpolation is used to denote a config key and " f"thus should return a string, but instead returned `{res}` of " f"type `{type(res)}`: {ctx.getChild(0).getText()}" @@ -233,7 +233,7 @@ def visitResolverName(self, ctx: OmegaConfGrammarParser.ResolverNameContext) -> assert isinstance(child, OmegaConfGrammarParser.InterpolationContext) item = _get_value(self.visitInterpolation(child)) if not isinstance(item, str): - raise GrammarParseError( + raise InterpolationResolutionError( f"The name of a resolver must be a string, but the interpolation " f"{child.getText()} resolved to `{item}` which is of type " f"{type(item)}" diff --git a/omegaconf/omegaconf.py b/omegaconf/omegaconf.py index dc351c9f1..592d15db2 100644 --- a/omegaconf/omegaconf.py +++ b/omegaconf/omegaconf.py @@ -739,10 +739,10 @@ def select( raise if ( - _root is not None + default is not _EMPTY_MARKER_ + and _root is not None and _last_key is not None - and _last_key not in _root - and default is not _EMPTY_MARKER_ + and (value is None or _last_key not in _root) ): return default diff --git a/tests/test_basic_ops_dict.py b/tests/test_basic_ops_dict.py index 3f3d5ecee..3808ef9f6 100644 --- a/tests/test_basic_ops_dict.py +++ b/tests/test_basic_ops_dict.py @@ -24,6 +24,7 @@ ConfigAttributeError, ConfigKeyError, ConfigTypeError, + InterpolationToMissingValueError, KeyValidationError, ) from tests import ( @@ -427,7 +428,11 @@ def test_dict_structured_mode_pop() -> None: Enum1.FOO, pytest.raises(MissingMandatoryValue), ), - ({"a": "${b}", "b": "???"}, "a", pytest.raises(MissingMandatoryValue)), + ( + {"a": "${b}", "b": "???"}, + "a", + pytest.raises(InterpolationToMissingValueError), + ), ], ) def test_dict_pop_error(cfg: Dict[Any, Any], key: Any, expectation: Any) -> None: @@ -446,7 +451,6 @@ def test_dict_pop_error(cfg: Dict[Any, Any], key: Any, expectation: Any) -> None ({"a": 1, "b": {}}, "c", False), ({"a": 1, "b": "${a}"}, "b", True), ({"a": 1, "b": "???"}, "b", False), - ({"a": 1, "b": "???", "c": "${b}"}, "c", False), ({"a": 1, "b": "${not_found}"}, "b", False), ({"a": "${unknown_resolver:bar}"}, "a", True), ({"a": None, "b": "${a}"}, "b", True), @@ -505,6 +509,18 @@ def test_in_dict(conf: Any, key: str, expected: Any) -> None: assert (key in conf) == expected +@pytest.mark.parametrize( + "conf,key,expected", + [ + ({"a": 1, "b": "???", "c": "${b}"}, "c", InterpolationToMissingValueError), + ], +) +def test_in_dict_errors(conf: Any, key: str, expected: Any) -> None: + conf = OmegaConf.create(conf) + with pytest.raises(expected): + key in conf + + def test_get_root() -> None: c = OmegaConf.create({"a": 123, "b": {"bb": 456, "cc": 7}}) assert c._get_root() == c diff --git a/tests/test_errors.py b/tests/test_errors.py index ea40cbdae..9137c6e4f 100644 --- a/tests/test_errors.py +++ b/tests/test_errors.py @@ -24,6 +24,7 @@ ConfigValueError, GrammarParseError, InterpolationKeyError, + InterpolationResolutionError, KeyValidationError, MissingMandatoryValue, OmegaConfBaseException, @@ -1245,7 +1246,7 @@ def assert_false() -> None: # error is just one way of achieving this goal. register_func("assert_false", assert_false) c = OmegaConf.create({"trigger": "${assert_false:}"}) - with pytest.raises(AssertionError): + with pytest.raises(InterpolationResolutionError, match=re.escape("assert False")): c.trigger @@ -1264,7 +1265,7 @@ def div(x: Any, y: Any) -> float: full_key: div_by_zero object_type=dict""" ) - with pytest.raises(ZeroDivisionError, match=re.escape(expected_msg)): + with pytest.raises(InterpolationResolutionError, match=re.escape(expected_msg)): c.div_by_zero diff --git a/tests/test_grammar.py b/tests/test_grammar.py index 9f9b2ff88..4226e4629 100644 --- a/tests/test_grammar.py +++ b/tests/test_grammar.py @@ -15,6 +15,7 @@ from omegaconf.errors import ( GrammarParseError, InterpolationKeyError, + InterpolationResolutionError, UnsupportedInterpolationType, ) @@ -195,7 +196,7 @@ ("null_like_key_bad_case", "${NoNe.null}", InterpolationKeyError), ("null_like_key_quoted_1", "${'None'.'null'}", GrammarParseError), ("null_like_key_quoted_2", "${'None.null'}", GrammarParseError), - ("dotpath_bad_type", "${dict.${float}}", (None, GrammarParseError)), + ("dotpath_bad_type", "${dict.${float}}", (None, InterpolationResolutionError)), ("at_in_key", "${x@y}", 123), # Interpolations in dictionaries. ("dict_interpolation_value", "{hi: ${str}, int: ${int}}", {"hi": "hi", "int": 123}), @@ -257,7 +258,7 @@ ("float_resolver_quoted", "${'1.1':1,2,3}", GrammarParseError), ("float_resolver_noquote", "${1.1:1,2,3}", GrammarParseError), ("float_resolver_exp", "${1e1:1,2,3}", GrammarParseError), - ("inter_float_resolver", "${${float}:1,2,3}", (None, GrammarParseError)), + ("inter_float_resolver", "${${float}:1,2,3}", (None, InterpolationResolutionError)), # NaN as dictionary key (a resolver is used here to output only the key). ("dict_nan_key_1", "${first:{nan: 0}}", math.nan), ("dict_nan_key_2", "${first:{${test:nan}: 0}}", GrammarParseError), diff --git a/tests/test_interpolation.py b/tests/test_interpolation.py index 7ae642118..2b65e4ab1 100644 --- a/tests/test_interpolation.py +++ b/tests/test_interpolation.py @@ -19,9 +19,9 @@ ) from omegaconf._utils import _ensure_container from omegaconf.errors import ( - ConfigAttributeError, GrammarParseError, InterpolationKeyError, + InterpolationResolutionError, OmegaConfBaseException, UnsupportedInterpolationType, ) @@ -82,7 +82,10 @@ pytest.param({"a": {"z": "${..b}"}, "b": 10}, "a.z", 10, id="relative"), pytest.param({"a": {"z": "${..a.b}", "b": 10}}, "a.z", 10, id="relative"), pytest.param( - {"a": "${..b}", "b": 10}, "a", pytest.raises(KeyError), id="relative" + {"a": "${..b}", "b": 10}, + "a", + pytest.raises(InterpolationKeyError), + id="relative", ), ], ) @@ -151,7 +154,7 @@ def test_merge_with_interpolation() -> None: def test_non_container_interpolation() -> None: cfg = OmegaConf.create(dict(foo=0, bar="${foo.baz}")) - with pytest.raises(ConfigAttributeError): + with pytest.raises(InterpolationKeyError): cfg.bar @@ -220,7 +223,7 @@ def test_type_inherit_type(cfg: Any) -> None: None, "path", pytest.raises( - ValidationError, + InterpolationResolutionError, match=re.escape("Environment variable 'not_found' not found"), ), id="not_found", diff --git a/tests/test_nodes.py b/tests/test_nodes.py index dbef962a8..5cfd84695 100644 --- a/tests/test_nodes.py +++ b/tests/test_nodes.py @@ -18,6 +18,7 @@ ValueNode, ) from omegaconf.errors import ( + InterpolationToMissingValueError, MissingMandatoryValue, UnsupportedValueType, ValidationError, @@ -579,5 +580,5 @@ def test_dereference_interpolation_to_missing() -> None: x_node = cfg._get_node("x") assert isinstance(x_node, Node) assert x_node._dereference_node(throw_on_resolution_failure=False) is None - with pytest.raises(MissingMandatoryValue): + with pytest.raises(InterpolationToMissingValueError): cfg.x diff --git a/tests/test_omegaconf.py b/tests/test_omegaconf.py index 43f567541..943072814 100644 --- a/tests/test_omegaconf.py +++ b/tests/test_omegaconf.py @@ -18,6 +18,7 @@ from omegaconf.errors import ( ConfigKeyError, InterpolationKeyError, + InterpolationToMissingValueError, UnsupportedInterpolationType, ) from tests import ( @@ -40,7 +41,7 @@ {"foo": "${bar}", "bar": DictConfig(content=MISSING)}, "foo", False, - raises(MissingMandatoryValue), + raises(InterpolationToMissingValueError), id="missing_interpolated_dict", ), pytest.param( @@ -61,27 +62,27 @@ {"foo": ListConfig(content="${missing}"), "missing": "???"}, "foo", False, - raises(MissingMandatoryValue), + raises(InterpolationToMissingValueError), id="missing_list_interpolation", ), pytest.param( {"foo": DictConfig(content="${missing}"), "missing": "???"}, "foo", False, - raises(MissingMandatoryValue), + raises(InterpolationToMissingValueError), id="missing_dict_interpolation", ), ( {"foo": "${bar}", "bar": MISSING}, "foo", False, - raises(MissingMandatoryValue), + raises(InterpolationToMissingValueError), ), ( {"foo": "foo_${bar}", "bar": MISSING}, "foo", False, - raises(MissingMandatoryValue), + raises(InterpolationToMissingValueError), ), ( {"foo": "${unknown_resolver:foo}"}, @@ -94,7 +95,7 @@ {"foo": StringNode(value="???"), "inter": "${foo}"}, "inter", False, - raises(MissingMandatoryValue), + raises(InterpolationToMissingValueError), ), (StructuredWithMissing, "num", True, raises(MissingMandatoryValue)), (StructuredWithMissing, "opt_num", True, raises(MissingMandatoryValue)), @@ -104,9 +105,24 @@ (StructuredWithMissing, "opt_list", True, raises(MissingMandatoryValue)), (StructuredWithMissing, "user", True, raises(MissingMandatoryValue)), (StructuredWithMissing, "opt_user", True, raises(MissingMandatoryValue)), - (StructuredWithMissing, "inter_user", False, raises(MissingMandatoryValue)), - (StructuredWithMissing, "inter_opt_user", False, raises(MissingMandatoryValue)), - (StructuredWithMissing, "inter_num", False, raises(MissingMandatoryValue)), + ( + StructuredWithMissing, + "inter_user", + False, + raises(InterpolationToMissingValueError), + ), + ( + StructuredWithMissing, + "inter_opt_user", + False, + raises(InterpolationToMissingValueError), + ), + ( + StructuredWithMissing, + "inter_num", + False, + raises(InterpolationToMissingValueError), + ), ], ) def test_is_missing( From 84bdd85ec1343fcc9df6bf7145fb53cf7ae85042 Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Mon, 22 Feb 2021 23:09:49 -0500 Subject: [PATCH 15/42] Remove the `throw_on_missing` argument to `_dereference_node()` The removal of this argument from the call in `_select_impl()` may look suspicious, since `throw_on_missing` can be True here. However, if it is true, then `ret` cannot be missing since otherwise an exception would have been raised in the call to `_select_one()` above. --- omegaconf/base.py | 39 +++++++++++++++---------------------- omegaconf/basecontainer.py | 16 ++++----------- tests/test_interpolation.py | 7 +------ tests/test_nodes.py | 4 +--- 4 files changed, 22 insertions(+), 44 deletions(-) diff --git a/omegaconf/base.py b/omegaconf/base.py index cd2a5cb93..c60b7e150 100644 --- a/omegaconf/base.py +++ b/omegaconf/base.py @@ -193,32 +193,26 @@ def _get_full_key(self, key: Optional[Union[DictKeyType, int]]) -> str: def _dereference_node( self, - throw_on_missing: bool = False, throw_on_resolution_failure: bool = True, ) -> Optional["Node"]: - if self._is_interpolation(): - parent = self._get_parent() - if parent is None: - raise OmegaConfBaseException( - "Cannot resolve interpolation for a node without a parent" - ) - assert parent is not None - key = self._key() - return parent._resolve_interpolation_from_parse_tree( - parent=parent, - key=key, - value=self, - parse_tree=parse(_get_value(self)), - throw_on_resolution_failure=throw_on_resolution_failure, - ) - else: - # not interpolation, compare directly - if throw_on_missing: - value = self._value() - if _is_missing_literal(value): - raise MissingMandatoryValue("Missing mandatory value") + if not self._is_interpolation(): return self + parent = self._get_parent() + if parent is None: + raise OmegaConfBaseException( + "Cannot resolve interpolation for a node without a parent" + ) + assert parent is not None + key = self._key() + return parent._resolve_interpolation_from_parse_tree( + parent=parent, + key=key, + value=self, + parse_tree=parse(_get_value(self)), + throw_on_resolution_failure=throw_on_resolution_failure, + ) + def _get_root(self) -> "Container": root: Optional[Container] = self._get_parent() if root is None: @@ -366,7 +360,6 @@ def _select_impl( ) if isinstance(ret, Node): ret = ret._dereference_node( - throw_on_missing=throw_on_missing, throw_on_resolution_failure=throw_on_resolution_failure, ) diff --git a/omegaconf/basecontainer.py b/omegaconf/basecontainer.py index 6e0989c88..2a2d04963 100644 --- a/omegaconf/basecontainer.py +++ b/omegaconf/basecontainer.py @@ -219,9 +219,7 @@ def convert(val: Node) -> Any: node = conf._get_node(key) assert isinstance(node, Node) if resolve: - node = node._dereference_node( - throw_on_missing=False, throw_on_resolution_failure=True - ) + node = node._dereference_node(throw_on_resolution_failure=True) assert node is not None if enum_to_str and isinstance(key, Enum): @@ -242,9 +240,7 @@ def convert(val: Node) -> Any: node = conf._get_node(index) assert isinstance(node, Node) if resolve: - node = node._dereference_node( - throw_on_missing=False, throw_on_resolution_failure=True - ) + node = node._dereference_node(throw_on_resolution_failure=True) assert node is not None if isinstance(node, Container): item = BaseContainer._to_content( @@ -629,13 +625,9 @@ def _item_eq( dv2: Optional[Node] = v2 if v1_inter: - dv1 = v1._dereference_node( - throw_on_missing=False, throw_on_resolution_failure=False - ) + dv1 = v1._dereference_node(throw_on_resolution_failure=False) if v2_inter: - dv2 = v2._dereference_node( - throw_on_missing=False, throw_on_resolution_failure=False - ) + dv2 = v2._dereference_node(throw_on_resolution_failure=False) if v1_inter and v2_inter: if dv1 is None or dv2 is None: diff --git a/tests/test_interpolation.py b/tests/test_interpolation.py index 2b65e4ab1..6955c521f 100644 --- a/tests/test_interpolation.py +++ b/tests/test_interpolation.py @@ -740,9 +740,4 @@ def fail_if_called(x: Any) -> None: ) x_node = cfg._get_node("x") assert isinstance(x_node, Node) - assert ( - x_node._dereference_node( - throw_on_missing=False, throw_on_resolution_failure=False - ) - is None - ) + assert x_node._dereference_node(throw_on_resolution_failure=False) is None diff --git a/tests/test_nodes.py b/tests/test_nodes.py index 5cfd84695..f71fcd2dd 100644 --- a/tests/test_nodes.py +++ b/tests/test_nodes.py @@ -19,7 +19,6 @@ ) from omegaconf.errors import ( InterpolationToMissingValueError, - MissingMandatoryValue, UnsupportedValueType, ValidationError, ) @@ -571,8 +570,7 @@ def test_dereference_missing() -> None: cfg = OmegaConf.create({"x": "???"}) x_node = cfg._get_node("x") assert isinstance(x_node, Node) - with pytest.raises(MissingMandatoryValue): - x_node._dereference_node(throw_on_missing=True) + assert x_node._dereference_node() is x_node def test_dereference_interpolation_to_missing() -> None: From 398f21f019f70e49060e35085264634e8a93b6da Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Tue, 23 Feb 2021 07:49:14 -0500 Subject: [PATCH 16/42] Remove unused import --- omegaconf/nodes.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/omegaconf/nodes.py b/omegaconf/nodes.py index 169a268c1..2119e3cf3 100644 --- a/omegaconf/nodes.py +++ b/omegaconf/nodes.py @@ -13,12 +13,7 @@ is_primitive_container, ) from omegaconf.base import Container, DictKeyType, Metadata, Node -from omegaconf.errors import ( - ConfigKeyError, - ReadonlyConfigError, - UnsupportedValueType, - ValidationError, -) +from omegaconf.errors import ReadonlyConfigError, UnsupportedValueType, ValidationError class ValueNode(Node): From bfcd95a2d0a2afdbde692691dfae75ebb6686364 Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Tue, 23 Feb 2021 08:24:31 -0500 Subject: [PATCH 17/42] Fix coverage --- tests/test_errors.py | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/tests/test_errors.py b/tests/test_errors.py index 9137c6e4f..ea02bdaac 100644 --- a/tests/test_errors.py +++ b/tests/test_errors.py @@ -16,7 +16,7 @@ UnsupportedValueType, ValidationError, ) -from omegaconf._utils import type_str +from omegaconf._utils import format_and_raise, type_str from omegaconf.errors import ( ConfigAttributeError, ConfigKeyError, @@ -1234,20 +1234,17 @@ def test_errors(expected: Expected, monkeypatch: Any) -> None: assert e.__cause__ is None -@pytest.mark.parametrize( - "register_func", [OmegaConf.register_resolver, OmegaConf.register_new_resolver] -) -def test_assertion_error(restore_resolvers: Any, register_func: Any) -> None: - def assert_false() -> None: +def test_assertion_error() -> None: + """Test the case where an `AssertionError` is processed in `format_and_raise()`""" + try: assert False - - # The purpose of this test is to cover the case where an `AssertionError` - # is processed in `format_and_raise()`. Using a resolver to trigger the assertion - # error is just one way of achieving this goal. - register_func("assert_false", assert_false) - c = OmegaConf.create({"trigger": "${assert_false:}"}) - with pytest.raises(InterpolationResolutionError, match=re.escape("assert False")): - c.trigger + except AssertionError as exc: + try: + format_and_raise(node=None, key=None, value=None, msg=str(exc), cause=exc) + except AssertionError as exc2: + assert exc2 is exc # we expect the original exception to be raised + else: + assert False @pytest.mark.parametrize( From e3552b6a2ee7b7f4a39cdf43ee4dccbd48a0956e Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Tue, 23 Feb 2021 12:00:55 -0500 Subject: [PATCH 18/42] Keys for which interpolation resolution fails are never missing --- omegaconf/dictconfig.py | 10 +++++----- tests/test_basic_ops_dict.py | 15 ++------------- 2 files changed, 7 insertions(+), 18 deletions(-) diff --git a/omegaconf/dictconfig.py b/omegaconf/dictconfig.py index ca5ff30ed..f0679af57 100644 --- a/omegaconf/dictconfig.py +++ b/omegaconf/dictconfig.py @@ -39,12 +39,11 @@ ConfigAttributeError, ConfigKeyError, ConfigTypeError, - InterpolationKeyError, + InterpolationResolutionError, KeyValidationError, MissingMandatoryValue, OmegaConfBaseException, ReadonlyConfigError, - UnsupportedInterpolationType, ValidationError, ) from .nodes import EnumNode, ValueNode @@ -515,10 +514,11 @@ def __contains__(self, key: object) -> bool: try: self._resolve_with_default(key=key, value=node) return True - except UnsupportedInterpolationType: - # Value that has unsupported interpolation counts as existing. + except InterpolationResolutionError: + # Interpolations that fail count as existing. return True - except (MissingMandatoryValue, KeyError, InterpolationKeyError): + except MissingMandatoryValue: + # Missing values count as *not* existing. return False def __iter__(self) -> Iterator[DictKeyType]: diff --git a/tests/test_basic_ops_dict.py b/tests/test_basic_ops_dict.py index 3808ef9f6..5953bcd6c 100644 --- a/tests/test_basic_ops_dict.py +++ b/tests/test_basic_ops_dict.py @@ -451,7 +451,8 @@ def test_dict_pop_error(cfg: Dict[Any, Any], key: Any, expectation: Any) -> None ({"a": 1, "b": {}}, "c", False), ({"a": 1, "b": "${a}"}, "b", True), ({"a": 1, "b": "???"}, "b", False), - ({"a": 1, "b": "${not_found}"}, "b", False), + ({"a": 1, "b": "???", "c": "${b}"}, "c", True), + ({"a": 1, "b": "${not_found}"}, "b", True), ({"a": "${unknown_resolver:bar}"}, "a", True), ({"a": None, "b": "${a}"}, "b", True), ({"a": "cat", "b": "${a}"}, "b", True), @@ -509,18 +510,6 @@ def test_in_dict(conf: Any, key: str, expected: Any) -> None: assert (key in conf) == expected -@pytest.mark.parametrize( - "conf,key,expected", - [ - ({"a": 1, "b": "???", "c": "${b}"}, "c", InterpolationToMissingValueError), - ], -) -def test_in_dict_errors(conf: Any, key: str, expected: Any) -> None: - conf = OmegaConf.create(conf) - with pytest.raises(expected): - key in conf - - def test_get_root() -> None: c = OmegaConf.create({"a": 123, "b": {"bb": 456, "cc": 7}}) assert c._get_root() == c From 3a6d1707e7d5940b69e41103227224be66bf0d26 Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Tue, 23 Feb 2021 12:05:37 -0500 Subject: [PATCH 19/42] Remove duplicated test It is the same as the one three lines below --- tests/test_basic_ops_dict.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/test_basic_ops_dict.py b/tests/test_basic_ops_dict.py index 5953bcd6c..99ce88d96 100644 --- a/tests/test_basic_ops_dict.py +++ b/tests/test_basic_ops_dict.py @@ -186,7 +186,6 @@ def test_scientific_notation_float() -> None: ({"hello": {"a": 2}}, "", "missing"), ({"hello": {"a": 2}}, "hello", "missing"), ({"hello": "???"}, "", "hello"), - ({"hello": "${foo}", "foo": "???"}, "", "hello"), ({"hello": None}, "", "hello"), ({"hello": "${foo}"}, "", "hello"), ({"hello": "${foo}", "foo": "???"}, "", "hello"), From d2db10da7b5d6a00b2972616a9a22d8711ec9fde Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Tue, 23 Feb 2021 12:20:34 -0500 Subject: [PATCH 20/42] Interpolation resolution errors are now raised even when a default is provided --- omegaconf/basecontainer.py | 2 +- tests/test_basic_ops_dict.py | 49 ++++++++++++++++++++++++++++-------- 2 files changed, 40 insertions(+), 11 deletions(-) diff --git a/omegaconf/basecontainer.py b/omegaconf/basecontainer.py index 2a2d04963..8fe1ef35e 100644 --- a/omegaconf/basecontainer.py +++ b/omegaconf/basecontainer.py @@ -64,7 +64,7 @@ def _resolve_with_default( parent=self, key=key, value=value, - throw_on_resolution_failure=not has_default, + throw_on_resolution_failure=True, ) if resolved_node is None and has_default: return default_value diff --git a/tests/test_basic_ops_dict.py b/tests/test_basic_ops_dict.py index 99ce88d96..914fc1fa6 100644 --- a/tests/test_basic_ops_dict.py +++ b/tests/test_basic_ops_dict.py @@ -5,6 +5,7 @@ from typing import Any, Dict, List, Optional, Union import pytest +from _pytest.python_api import RaisesContext from omegaconf import ( MISSING, @@ -24,6 +25,7 @@ ConfigAttributeError, ConfigKeyError, ConfigTypeError, + InterpolationKeyError, InterpolationToMissingValueError, KeyValidationError, ) @@ -187,11 +189,8 @@ def test_scientific_notation_float() -> None: ({"hello": {"a": 2}}, "hello", "missing"), ({"hello": "???"}, "", "hello"), ({"hello": None}, "", "hello"), - ({"hello": "${foo}"}, "", "hello"), - ({"hello": "${foo}", "foo": "???"}, "", "hello"), ({"hello": DictConfig(is_optional=True, content=None)}, "", "hello"), ({"hello": DictConfig(content="???")}, "", "hello"), - ({"hello": DictConfig(content="${foo}")}, "", "hello"), ({"hello": ListConfig(is_optional=True, content=None)}, "", "hello"), ({"hello": ListConfig(content="???")}, "", "hello"), ], @@ -205,6 +204,26 @@ def test_dict_get_with_default( assert c.get(key, default_val) == default_val +@pytest.mark.parametrize( + "d,select,key,exc", + [ + ({"hello": "${foo}"}, "", "hello", InterpolationKeyError), + ( + {"hello": "${foo}", "foo": "???"}, + "", + "hello", + InterpolationToMissingValueError, + ), + ({"hello": DictConfig(content="${foo}")}, "", "hello", InterpolationKeyError), + ], +) +def test_dict_get_with_default_errors(d: Any, select: Any, key: Any, exc: type) -> None: + c = OmegaConf.create(d) + c = OmegaConf.select(c, select) + with pytest.raises(exc): + c.get(key, default_value=123) + + def test_map_expansion() -> None: c = OmegaConf.create("{a: 2, b: 10}") assert isinstance(c, DictConfig) @@ -314,7 +333,11 @@ def test_iterate_dict_with_interpolation() -> None: {"a": "${b}", "b": 2}, "a", "__NO_DEFAULT__", 2, id="interpolation" ), pytest.param( - {"a": "${b}"}, "a", "default", "default", id="interpolation_with_default" + {"a": "${b}"}, + "a", + "default", + pytest.raises(InterpolationKeyError), + id="interpolation_with_default", ), # enum key pytest.param( @@ -364,13 +387,19 @@ def test_iterate_dict_with_interpolation() -> None: def test_dict_pop(cfg: Dict[Any, Any], key: Any, default_: Any, expected: Any) -> None: c = OmegaConf.create(cfg) - if default_ != "__NO_DEFAULT__": - val = c.pop(key, default_) - else: - val = c.pop(key) + def pop() -> Any: + if default_ != "__NO_DEFAULT__": + return c.pop(key, default_) + else: + return c.pop(key) - assert val == expected - assert type(val) == type(expected) + if isinstance(expected, RaisesContext): + with expected: + pop() + else: + val = pop() + assert val == expected + assert type(val) == type(expected) def test_dict_struct_mode_pop() -> None: From 99bc55b06b0c1ba6325c6348722886a6e9d00a28 Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Tue, 23 Feb 2021 12:30:38 -0500 Subject: [PATCH 21/42] Fix coverage --- omegaconf/basecontainer.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/omegaconf/basecontainer.py b/omegaconf/basecontainer.py index 8fe1ef35e..d19539c43 100644 --- a/omegaconf/basecontainer.py +++ b/omegaconf/basecontainer.py @@ -66,8 +66,7 @@ def _resolve_with_default( value=value, throw_on_resolution_failure=True, ) - if resolved_node is None and has_default: - return default_value + assert resolved_node is not None return _get_value(resolved_node) From 69a1041a51b6bb5cd8bf2783dbefcbdebeb28dab Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Wed, 24 Feb 2021 09:18:59 -0500 Subject: [PATCH 22/42] Add/update news fragments --- news/543.api_change | 2 +- news/561.api_change | 1 + news/562.api_change | 2 ++ news/565.api_change | 2 ++ 4 files changed, 6 insertions(+), 1 deletion(-) create mode 100644 news/561.api_change create mode 100644 news/562.api_change create mode 100644 news/565.api_change diff --git a/news/543.api_change b/news/543.api_change index d70aac081..9dd1c518f 100644 --- a/news/543.api_change +++ b/news/543.api_change @@ -1 +1 @@ -Interpolations interpolating a missing key no longer count as missing. +An interpolation interpolating a missing key no longer counts as missing. Resolving such an interpolation raises an `InterpolationToMissingValueError` exception. diff --git a/news/561.api_change b/news/561.api_change new file mode 100644 index 000000000..f59c4276a --- /dev/null +++ b/news/561.api_change @@ -0,0 +1 @@ +All exceptions raised during the resolution of an interpolation are either `InterpolationResolutionError` or a subclass of it. diff --git a/news/562.api_change b/news/562.api_change new file mode 100644 index 000000000..7919edcea --- /dev/null +++ b/news/562.api_change @@ -0,0 +1,2 @@ +`key in cfg` now returns True whenever `key` is an interpolation (it used to be False for interpolations to non-existing nodes or to nodes set to `???`) + diff --git a/news/565.api_change b/news/565.api_change new file mode 100644 index 000000000..940b9e009 --- /dev/null +++ b/news/565.api_change @@ -0,0 +1,2 @@ +Container methods `get()` and `pop()` do not return their default value anymore when the accessed key is an interpolation that cannot be resolved: instead, an exception is raised. + From 46bc121142f5e8012124e2f3309ed548ef339826 Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Wed, 24 Feb 2021 09:19:11 -0500 Subject: [PATCH 23/42] Add more tests for `pop()` and `get()` on ListConfig --- tests/test_basic_ops_list.py | 39 ++++++++++++++++++++++++++++-------- 1 file changed, 31 insertions(+), 8 deletions(-) diff --git a/tests/test_basic_ops_list.py b/tests/test_basic_ops_list.py index efff9f49f..fe5115d3b 100644 --- a/tests/test_basic_ops_list.py +++ b/tests/test_basic_ops_list.py @@ -9,6 +9,7 @@ from omegaconf.errors import ( ConfigTypeError, InterpolationKeyError, + InterpolationToMissingValueError, KeyValidationError, MissingMandatoryValue, UnsupportedValueType, @@ -87,13 +88,33 @@ def test_items_with_interpolation() -> None: assert c == ["foo", "foo"] -def test_list_pop() -> None: - c = OmegaConf.create([1, 2, 3, 4]) - assert c.pop(0) == 1 - assert c.pop() == 4 - assert c == [2, 3] - with pytest.raises(IndexError): - c.pop(100) +@pytest.mark.parametrize( + ["cfg", "key", "expected_out", "expected_cfg"], + [ + pytest.param([1, 2, 3], 0, 1, [2, 3]), + pytest.param([1, 2, 3], None, 3, [1, 2]), + pytest.param([1, 2, 3], 100, IndexError, ...), + pytest.param(["???", 2, 3], 0, None, [2, 3]), + pytest.param(["${4}", 2, 3], 0, InterpolationKeyError, ...), + pytest.param(["${1}", "???", 3], 0, InterpolationToMissingValueError, ...), + ], +) +def test_list_pop( + cfg: List[Any], key: Optional[int], expected_out: Any, expected_cfg: Any +) -> None: + c = OmegaConf.create(cfg) + pop_args = [] if key is None else [key] + if isinstance(expected_out, type): + with pytest.raises(expected_out): + c.pop(*pop_args) + else: + val = c.pop(*pop_args) + assert val == expected_out + + if expected_cfg is ...: # shortcut to mean it is unchanged + expected_cfg = cfg + assert c == expected_cfg + validate_list_keys(c) @@ -613,6 +634,8 @@ def test_getitem_slice(sli: slice) -> None: [ (OmegaConf.create([1, 2]), 0, 1), (OmegaConf.create([1, 2]), "foo", KeyValidationError), + (OmegaConf.create([1, "${2}"]), 1, InterpolationKeyError), + (OmegaConf.create(["???", "${0}"]), 1, InterpolationToMissingValueError), (ListConfig(content=None), 0, TypeError), (ListConfig(content="???"), 0, MissingMandatoryValue), ], @@ -622,7 +645,7 @@ def test_get(lst: Any, idx: Any, expected: Any) -> None: with pytest.raises(expected): lst.get(idx) else: - lst.__getitem__(idx) == expected + assert lst.__getitem__(idx) == expected def test_getattr() -> None: From 728856f4fdcf5c7575606488809aa2cf5dadf4d8 Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Thu, 25 Feb 2021 17:51:19 -0500 Subject: [PATCH 24/42] Remove now useless check --- omegaconf/omegaconf.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/omegaconf/omegaconf.py b/omegaconf/omegaconf.py index 592d15db2..dd2f2bdec 100644 --- a/omegaconf/omegaconf.py +++ b/omegaconf/omegaconf.py @@ -742,7 +742,7 @@ def select( default is not _EMPTY_MARKER_ and _root is not None and _last_key is not None - and (value is None or _last_key not in _root) + and _last_key not in _root ): return default From 7f455762937de1b686cbe79986a8f198f008f050 Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Thu, 25 Feb 2021 17:56:05 -0500 Subject: [PATCH 25/42] Taking suggestion from Github --- news/563.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/news/563.bugfix b/news/563.bugfix index 073d1f076..a70adf663 100644 --- a/news/563.bugfix +++ b/news/563.bugfix @@ -1 +1 @@ -Calling `OmegaConf.select()` on a missing node from a ListConfig with `throw_on_missing` set to True now raises the intended exception. +`OmegaConf.select()` of a missing ("???") node from a ListConfig with `throw_on_missing` set to True now raises the intended exception. From a4e6f0f5d794c57be3968b004c72aaa60cf34ac4 Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Thu, 25 Feb 2021 18:09:41 -0500 Subject: [PATCH 26/42] Simplify test --- tests/test_basic_ops_dict.py | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/tests/test_basic_ops_dict.py b/tests/test_basic_ops_dict.py index 914fc1fa6..0820b88aa 100644 --- a/tests/test_basic_ops_dict.py +++ b/tests/test_basic_ops_dict.py @@ -205,23 +205,20 @@ def test_dict_get_with_default( @pytest.mark.parametrize( - "d,select,key,exc", + "d,exc", [ - ({"hello": "${foo}"}, "", "hello", InterpolationKeyError), + ({"hello": "${foo}"}, InterpolationKeyError), ( {"hello": "${foo}", "foo": "???"}, - "", - "hello", InterpolationToMissingValueError, ), - ({"hello": DictConfig(content="${foo}")}, "", "hello", InterpolationKeyError), + ({"hello": DictConfig(content="${foo}")}, InterpolationKeyError), ], ) -def test_dict_get_with_default_errors(d: Any, select: Any, key: Any, exc: type) -> None: +def test_dict_get_with_default_errors(d: Any, exc: type) -> None: c = OmegaConf.create(d) - c = OmegaConf.select(c, select) with pytest.raises(exc): - c.get(key, default_value=123) + c.get("hello", default_value=123) def test_map_expansion() -> None: From 36eb818c9b54c297cd0383e209b8f998945a8b1d Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Thu, 25 Feb 2021 18:18:20 -0500 Subject: [PATCH 27/42] Split test of ListConfig.pop() in two --- tests/test_basic_ops_list.py | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/tests/test_basic_ops_list.py b/tests/test_basic_ops_list.py index fe5115d3b..a32570f78 100644 --- a/tests/test_basic_ops_list.py +++ b/tests/test_basic_ops_list.py @@ -93,28 +93,32 @@ def test_items_with_interpolation() -> None: [ pytest.param([1, 2, 3], 0, 1, [2, 3]), pytest.param([1, 2, 3], None, 3, [1, 2]), - pytest.param([1, 2, 3], 100, IndexError, ...), pytest.param(["???", 2, 3], 0, None, [2, 3]), - pytest.param(["${4}", 2, 3], 0, InterpolationKeyError, ...), - pytest.param(["${1}", "???", 3], 0, InterpolationToMissingValueError, ...), ], ) def test_list_pop( cfg: List[Any], key: Optional[int], expected_out: Any, expected_cfg: Any ) -> None: c = OmegaConf.create(cfg) - pop_args = [] if key is None else [key] - if isinstance(expected_out, type): - with pytest.raises(expected_out): - c.pop(*pop_args) - else: - val = c.pop(*pop_args) - assert val == expected_out - - if expected_cfg is ...: # shortcut to mean it is unchanged - expected_cfg = cfg + val = c.pop() if key is None else c.pop(key) + assert val == expected_out assert c == expected_cfg + validate_list_keys(c) + +@pytest.mark.parametrize( + ["cfg", "key", "exc"], + [ + pytest.param([1, 2, 3], 100, IndexError), + pytest.param(["${4}", 2, 3], 0, InterpolationKeyError), + pytest.param(["${1}", "???", 3], 0, InterpolationToMissingValueError), + ], +) +def test_list_pop_errors(cfg: List[Any], key: int, exc: type) -> None: + c = OmegaConf.create(cfg) + with pytest.raises(exc): + c.pop(key) + assert c == cfg validate_list_keys(c) From ef1d38f0e6d511498428d9999cf25e1c850948ed Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Thu, 25 Feb 2021 19:01:38 -0500 Subject: [PATCH 28/42] Minor fix to test documentation --- tests/test_interpolation.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_interpolation.py b/tests/test_interpolation.py index 6955c521f..5f13b55f6 100644 --- a/tests/test_interpolation.py +++ b/tests/test_interpolation.py @@ -724,7 +724,7 @@ def test_invalid_intermediate_result_when_not_throwing( The main goal of this test is to make sure that the resolution of an interpolation is stopped immediately when a missing / resolution failure occurs, even if - `throw_on_missing` and `throw_on_resolution_failure` are set to False. + `throw_on_resolution_failure` is set to False. When this happens while dereferencing a node, the result should be `None`. """ From 5718bf0623b1f7bd57da4dc37e7bfd07fe884943 Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Thu, 25 Feb 2021 23:15:50 -0500 Subject: [PATCH 29/42] Preserve traceback when re-raising exceptions --- omegaconf/base.py | 16 +++++++++----- omegaconf/omegaconf.py | 3 ++- tests/test_errors.py | 44 ++++++++++++++++++++++++++++++++++++- tests/test_interpolation.py | 12 +++++++--- 4 files changed, 65 insertions(+), 10 deletions(-) diff --git a/omegaconf/base.py b/omegaconf/base.py index c60b7e150..7c6548118 100644 --- a/omegaconf/base.py +++ b/omegaconf/base.py @@ -1,4 +1,5 @@ import copy +import sys from abc import ABC, abstractmethod from collections import defaultdict from dataclasses import dataclass, field @@ -10,7 +11,6 @@ from ._utils import ( ValueKind, _get_value, - _is_missing_literal, _is_missing_value, format_and_raise, get_value_kind, @@ -434,7 +434,9 @@ def _resolve_node_interpolation( try: root_node, inter_key = self._resolve_key_and_root(inter_key) except ConfigKeyError as exc: - raise InterpolationKeyError(str(exc)) + raise InterpolationKeyError( + f"ConfigKeyError while resolving interpolation: {exc}" + ).with_traceback(sys.exc_info()[2]) try: parent, last_key, value = root_node._select_impl( @@ -443,9 +445,13 @@ def _resolve_node_interpolation( throw_on_resolution_failure=True, ) except MissingMandatoryValue as exc: - raise InterpolationToMissingValueError(str(exc)) + raise InterpolationToMissingValueError( + f"MissingMandatoryValue while resolving interpolation: {exc}" + ).with_traceback(sys.exc_info()[2]) except ConfigKeyError as exc: - raise InterpolationKeyError(str(exc)) + raise InterpolationKeyError( + f"ConfigKeyError while resolving interpolation: {exc}" + ).with_traceback(sys.exc_info()[2]) if parent is None or value is None: raise InterpolationKeyError(f"Interpolation key '{inter_key}' not found") @@ -553,7 +559,7 @@ def quoted_string_callback(quoted_str: str) -> str: # Other kinds of exceptions are wrapped in an `InterpolationResolutionError`. raise InterpolationResolutionError( f"{type(exc).__name__} raised while resolving interpolation: {exc}" - ) + ).with_traceback(sys.exc_info()[2]) def _re_parent(self) -> None: from .dictconfig import DictConfig diff --git a/omegaconf/omegaconf.py b/omegaconf/omegaconf.py index dd2f2bdec..6b3a475da 100644 --- a/omegaconf/omegaconf.py +++ b/omegaconf/omegaconf.py @@ -130,7 +130,8 @@ def env(key: str, default: Any = _EMPTY_MARKER_) -> Any: f"caused the following exception: {exc}. Node interpolations are not " f"supported in environment variables: either remove them, or escape " f"them to keep them as a strings." - ) + ).with_traceback(sys.exc_info()[2]) + return _get_value(val) # Note that the `env` resolver does *NOT* use the cache. diff --git a/tests/test_errors.py b/tests/test_errors.py index ea02bdaac..e5675c77e 100644 --- a/tests/test_errors.py +++ b/tests/test_errors.py @@ -25,6 +25,7 @@ GrammarParseError, InterpolationKeyError, InterpolationResolutionError, + InterpolationToMissingValueError, KeyValidationError, MissingMandatoryValue, OmegaConfBaseException, @@ -252,6 +253,47 @@ def finalize(self, cfg: Any) -> None: ), id="dict,accessing_missing_relative_interpolation", ), + pytest.param( + Expected( + create=lambda: OmegaConf.create({"foo": "${..missing}"}), + op=lambda cfg: getattr(cfg, "foo"), + exception_type=InterpolationKeyError, + msg="ConfigKeyError while resolving interpolation: Error resolving key '..missing'", + key="foo", + child_node=lambda cfg: cfg._get_node("foo"), + ), + id="dict,accessing_invalid_double_relative_interpolation", + ), + pytest.param( + Expected( + create=lambda: OmegaConf.create({"foo": "${int.missing}", "int": 0}), + op=lambda cfg: getattr(cfg, "foo"), + exception_type=InterpolationKeyError, + msg=( + "ConfigKeyError while resolving interpolation: Error trying to access int.missing: " + "node `int` is not a container and thus cannot contain `missing`" + ), + key="foo", + child_node=lambda cfg: cfg._get_node("foo"), + ), + id="dict,accessing_non_container_interpolation", + ), + pytest.param( + Expected( + create=lambda: OmegaConf.create( + {"foo": "${${missing_val}}", "missing_val": "???"} + ), + op=lambda cfg: getattr(cfg, "foo"), + exception_type=InterpolationToMissingValueError, + msg=( + "MissingMandatoryValue while resolving interpolation: " + "Missing mandatory value : missing_val" + ), + key="foo", + child_node=lambda cfg: cfg._get_node("foo"), + ), + id="dict,accessing_missing_nested_interpolation", + ), # setattr pytest.param( Expected( @@ -1258,7 +1300,7 @@ def div(x: Any, y: Any) -> float: c = OmegaConf.create({"div_by_zero": "${div:1,0}"}) expected_msg = dedent( """\ - float division by zero + ZeroDivisionError raised while resolving interpolation: float division by zero full_key: div_by_zero object_type=dict""" ) diff --git a/tests/test_interpolation.py b/tests/test_interpolation.py index 5f13b55f6..0538695fd 100644 --- a/tests/test_interpolation.py +++ b/tests/test_interpolation.py @@ -322,9 +322,15 @@ def test_env_values_are_typed(monkeypatch: Any, value: Any, expected: Any) -> No def test_env_node_interpolation(monkeypatch: Any) -> None: # Test that node interpolations are not supported in env variables. - monkeypatch.setenv("my_key", "${other_key}") - c = OmegaConf.create(dict(my_key="${env:my_key}", other_key=123)) - with pytest.raises(InterpolationKeyError): + monkeypatch.setenv("MYKEY", "${other_key}") + c = OmegaConf.create(dict(my_key="${env:MYKEY}", other_key=123)) + with pytest.raises( + InterpolationKeyError, + match=re.escape( + "When attempting to resolve env variable 'MYKEY', a node interpolation caused " + "the following exception: Interpolation key 'other_key' not found." + ), + ): c.my_key From 2fa1d7fe4d9b5cfed80d15960b4f92e5558a3e0e Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Thu, 25 Feb 2021 23:24:56 -0500 Subject: [PATCH 30/42] Only catch InterpolationResolutionError --- omegaconf/base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/omegaconf/base.py b/omegaconf/base.py index 7c6548118..f9c93f015 100644 --- a/omegaconf/base.py +++ b/omegaconf/base.py @@ -408,7 +408,7 @@ def _resolve_interpolation_from_parse_tree( key=key, parent=parent, ) - except Exception: + except InterpolationResolutionError: if throw_on_resolution_failure: raise return None From 381387c2ed2b7c40f5aac3553b0d72a1e7da7340 Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Thu, 25 Feb 2021 23:26:22 -0500 Subject: [PATCH 31/42] Remove unused import --- omegaconf/nodes.py | 1 - 1 file changed, 1 deletion(-) diff --git a/omegaconf/nodes.py b/omegaconf/nodes.py index 2119e3cf3..ef4806638 100644 --- a/omegaconf/nodes.py +++ b/omegaconf/nodes.py @@ -7,7 +7,6 @@ from omegaconf._utils import ( ValueKind, _is_interpolation, - _is_missing_value, get_type_of, get_value_kind, is_primitive_container, From c87648ebd1adc185ef527bb5aab93c78c0c668fa Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Fri, 26 Feb 2021 15:06:36 -0500 Subject: [PATCH 32/42] Make `OmegaConf.select()` consistent with `cfg.get()` regarding interpolations to non-existing nodes --- news/565.api_change | 2 +- omegaconf/omegaconf.py | 2 +- tests/test_select.py | 16 +++++++++++++++- 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/news/565.api_change b/news/565.api_change index 940b9e009..8a153b548 100644 --- a/news/565.api_change +++ b/news/565.api_change @@ -1,2 +1,2 @@ -Container methods `get()` and `pop()` do not return their default value anymore when the accessed key is an interpolation that cannot be resolved: instead, an exception is raised. +`OmegaConf.select()` as well as container methods `get()` and `pop()` do not return their default value anymore when the accessed key is an interpolation that cannot be resolved: instead, an exception is raised. diff --git a/omegaconf/omegaconf.py b/omegaconf/omegaconf.py index 6b3a475da..66d8b77ca 100644 --- a/omegaconf/omegaconf.py +++ b/omegaconf/omegaconf.py @@ -733,7 +733,7 @@ def select( throw_on_missing=throw_on_missing, throw_on_resolution_failure=throw_on_resolution_failure, ) - except (ConfigKeyError, InterpolationKeyError): + except ConfigKeyError: if default is not _EMPTY_MARKER_: return default else: diff --git a/tests/test_select.py b/tests/test_select.py index 3d8b213d3..2d0db4306 100644 --- a/tests/test_select.py +++ b/tests/test_select.py @@ -7,6 +7,7 @@ from omegaconf import MissingMandatoryValue, OmegaConf from omegaconf._utils import _ensure_container +from omegaconf.errors import InterpolationKeyError @pytest.mark.parametrize("struct", [True, False, None]) @@ -64,7 +65,6 @@ def test_select( [ pytest.param({}, "not_found", id="empty"), pytest.param({"missing": "???"}, "missing", id="missing"), - pytest.param({"inter": "${bad_key}"}, "inter", id="inter_bad_key"), ], ) def test_select_default( @@ -101,6 +101,20 @@ def test_select_default_throw_on_missing( OmegaConf.select(cfg, key, default=default, throw_on_missing=True) +@pytest.mark.parametrize( + ["cfg", "key"], + [ + pytest.param({"inter": "${bad_key}"}, "inter", id="inter_bad_key"), + ], +) +def test_select_default_throw_on_resolution_failure(cfg: Any, key: Any) -> None: + cfg = OmegaConf.create(cfg) + + # throw on resolution failure still throws if default is provided + with pytest.raises(InterpolationKeyError): + OmegaConf.select(cfg, key, default=123, throw_on_resolution_failure=True) + + @pytest.mark.parametrize( ("cfg", "node_key", "expected"), [ From 05d1e25d14add97bfad66e363e1e98e5b99faf78 Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Fri, 26 Feb 2021 15:47:30 -0500 Subject: [PATCH 33/42] Fix coverage --- tests/test_select.py | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/tests/test_select.py b/tests/test_select.py index 2d0db4306..ea6ba44cc 100644 --- a/tests/test_select.py +++ b/tests/test_select.py @@ -7,7 +7,7 @@ from omegaconf import MissingMandatoryValue, OmegaConf from omegaconf._utils import _ensure_container -from omegaconf.errors import InterpolationKeyError +from omegaconf.errors import ConfigKeyError, InterpolationKeyError @pytest.mark.parametrize("struct", [True, False, None]) @@ -61,21 +61,41 @@ def test_select( @pytest.mark.parametrize("struct", [False, True]) @pytest.mark.parametrize("default", [10, None]) @pytest.mark.parametrize( - "cfg, key", + ["cfg", "key", "exc_no_default"], [ - pytest.param({}, "not_found", id="empty"), - pytest.param({"missing": "???"}, "missing", id="missing"), + pytest.param({}, "not_found", None, id="empty"), + pytest.param({"missing": "???"}, "missing", None, id="missing"), + pytest.param( + {"int": 0}, + "int.y", + pytest.raises( + ConfigKeyError, + match=re.escape( + "Error trying to access int.y: node `int` is not a container " + "and thus cannot contain `y`" + ), + ), + id="non_container", + ), ], ) def test_select_default( cfg: Any, struct: bool, key: Any, + exc_no_default: Any, default: Any, ) -> None: cfg = _ensure_container(cfg) OmegaConf.set_struct(cfg, struct) assert OmegaConf.select(cfg, key, default=default) == default + if exc_no_default is None: + # Not providing a default value is expected to work and return `None`. + assert OmegaConf.select(cfg, key) is None + else: + # Not providing a default value is expected to raise an exception. + with exc_no_default: + OmegaConf.select(cfg, key) @pytest.mark.parametrize("struct", [False, True]) From 16734a79b693f8b4b855053400430baed47da262 Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Tue, 2 Mar 2021 14:47:29 -0500 Subject: [PATCH 34/42] Update news/562.api_change Co-authored-by: Omry Yadan --- news/562.api_change | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/news/562.api_change b/news/562.api_change index 7919edcea..44a341eaa 100644 --- a/news/562.api_change +++ b/news/562.api_change @@ -1,2 +1 @@ -`key in cfg` now returns True whenever `key` is an interpolation (it used to be False for interpolations to non-existing nodes or to nodes set to `???`) - +`key in cfg` now returns True when `key` is an interpolation even if the interpolation target is a missing ("???") value. From 2f85931ae8494c038441348799319f958230f3e0 Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Tue, 2 Mar 2021 14:56:13 -0500 Subject: [PATCH 35/42] Cover more cases in test_dict_get_with_default_errors --- tests/test_basic_ops_dict.py | 77 +++++++++++++++++++----------------- 1 file changed, 40 insertions(+), 37 deletions(-) diff --git a/tests/test_basic_ops_dict.py b/tests/test_basic_ops_dict.py index 0820b88aa..2878d21ff 100644 --- a/tests/test_basic_ops_dict.py +++ b/tests/test_basic_ops_dict.py @@ -182,43 +182,46 @@ def test_scientific_notation_float() -> None: @pytest.mark.parametrize("struct", [None, True, False]) @pytest.mark.parametrize("default_val", [4, True, False, None]) -@pytest.mark.parametrize( - "d,select,key", - [ - ({"hello": {"a": 2}}, "", "missing"), - ({"hello": {"a": 2}}, "hello", "missing"), - ({"hello": "???"}, "", "hello"), - ({"hello": None}, "", "hello"), - ({"hello": DictConfig(is_optional=True, content=None)}, "", "hello"), - ({"hello": DictConfig(content="???")}, "", "hello"), - ({"hello": ListConfig(is_optional=True, content=None)}, "", "hello"), - ({"hello": ListConfig(content="???")}, "", "hello"), - ], -) -def test_dict_get_with_default( - d: Any, select: Any, key: Any, default_val: Any, struct: Any -) -> None: - c = OmegaConf.create(d) - c = OmegaConf.select(c, select) - OmegaConf.set_struct(c, struct) - assert c.get(key, default_val) == default_val - - -@pytest.mark.parametrize( - "d,exc", - [ - ({"hello": "${foo}"}, InterpolationKeyError), - ( - {"hello": "${foo}", "foo": "???"}, - InterpolationToMissingValueError, - ), - ({"hello": DictConfig(content="${foo}")}, InterpolationKeyError), - ], -) -def test_dict_get_with_default_errors(d: Any, exc: type) -> None: - c = OmegaConf.create(d) - with pytest.raises(exc): - c.get("hello", default_value=123) +class TestGetWithDefault: + @pytest.mark.parametrize( + "d,select,key", + [ + ({"hello": {"a": 2}}, "", "missing"), + ({"hello": {"a": 2}}, "hello", "missing"), + ({"hello": "???"}, "", "hello"), + ({"hello": None}, "", "hello"), + ({"hello": DictConfig(is_optional=True, content=None)}, "", "hello"), + ({"hello": DictConfig(content="???")}, "", "hello"), + ({"hello": ListConfig(is_optional=True, content=None)}, "", "hello"), + ({"hello": ListConfig(content="???")}, "", "hello"), + ], + ) + def test_dict_get_with_default( + self, d: Any, select: Any, key: Any, default_val: Any, struct: Optional[bool] + ) -> None: + c = OmegaConf.create(d) + c = OmegaConf.select(c, select) + OmegaConf.set_struct(c, struct) + assert c.get(key, default_val) == default_val + + @pytest.mark.parametrize( + "d,exc", + [ + ({"hello": "${foo}"}, InterpolationKeyError), + ( + {"hello": "${foo}", "foo": "???"}, + InterpolationToMissingValueError, + ), + ({"hello": DictConfig(content="${foo}")}, InterpolationKeyError), + ], + ) + def test_dict_get_with_default_errors( + self, d: Any, exc: type, struct: Optional[bool], default_val: Any + ) -> None: + c = OmegaConf.create(d) + OmegaConf.set_struct(c, struct) + with pytest.raises(exc): + c.get("hello", default_value=123) def test_map_expansion() -> None: From 1f0601a7838b772edd80af05584f3431fd947724 Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Tue, 2 Mar 2021 15:27:06 -0500 Subject: [PATCH 36/42] Simplify a test --- tests/test_basic_ops_dict.py | 27 +++++++-------------------- 1 file changed, 7 insertions(+), 20 deletions(-) diff --git a/tests/test_basic_ops_dict.py b/tests/test_basic_ops_dict.py index 2878d21ff..a919a5762 100644 --- a/tests/test_basic_ops_dict.py +++ b/tests/test_basic_ops_dict.py @@ -5,7 +5,6 @@ from typing import Any, Dict, List, Optional, Union import pytest -from _pytest.python_api import RaisesContext from omegaconf import ( MISSING, @@ -332,13 +331,6 @@ def test_iterate_dict_with_interpolation() -> None: pytest.param( {"a": "${b}", "b": 2}, "a", "__NO_DEFAULT__", 2, id="interpolation" ), - pytest.param( - {"a": "${b}"}, - "a", - "default", - pytest.raises(InterpolationKeyError), - id="interpolation_with_default", - ), # enum key pytest.param( {Enum1.FOO: "bar"}, @@ -387,19 +379,13 @@ def test_iterate_dict_with_interpolation() -> None: def test_dict_pop(cfg: Dict[Any, Any], key: Any, default_: Any, expected: Any) -> None: c = OmegaConf.create(cfg) - def pop() -> Any: - if default_ != "__NO_DEFAULT__": - return c.pop(key, default_) - else: - return c.pop(key) - - if isinstance(expected, RaisesContext): - with expected: - pop() + if default_ != "__NO_DEFAULT__": + val = c.pop(key, default_) else: - val = pop() - assert val == expected - assert type(val) == type(expected) + val = c.pop(key) + + assert val == expected + assert type(val) == type(expected) def test_dict_struct_mode_pop() -> None: @@ -450,6 +436,7 @@ def test_dict_structured_mode_pop() -> None: ({"a": "???", "b": 2}, "a", pytest.raises(MissingMandatoryValue)), ({1: "???", 2: "b"}, 1, pytest.raises(MissingMandatoryValue)), ({123.45: "???", 67.89: "b"}, 123.45, pytest.raises(MissingMandatoryValue)), + ({"a": "${b}"}, "a", pytest.raises(InterpolationKeyError)), ({True: "???", False: "b"}, True, pytest.raises(MissingMandatoryValue)), ( {Enum1.FOO: "???", Enum1.BAR: "bar"}, From 85478e45c9c67eaad4e869853a4e214de1407d03 Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Tue, 2 Mar 2021 15:33:58 -0500 Subject: [PATCH 37/42] Replace dict(..) with {..} in test_interpolation --- tests/test_interpolation.py | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/tests/test_interpolation.py b/tests/test_interpolation.py index 0538695fd..8a912b657 100644 --- a/tests/test_interpolation.py +++ b/tests/test_interpolation.py @@ -153,7 +153,7 @@ def test_merge_with_interpolation() -> None: def test_non_container_interpolation() -> None: - cfg = OmegaConf.create(dict(foo=0, bar="${foo.baz}")) + cfg = OmegaConf.create({"foo": 0, "bar": "${foo.baz}"}) with pytest.raises(InterpolationKeyError): cfg.bar @@ -316,14 +316,14 @@ def test_env_is_cached(monkeypatch: Any) -> None: def test_env_values_are_typed(monkeypatch: Any, value: Any, expected: Any) -> None: monkeypatch.setenv("my_key", value) monkeypatch.setenv("my_key_2", "456") - c = OmegaConf.create(dict(my_key="${env:my_key}")) + c = OmegaConf.create({"my_key": "${env:my_key}"}) assert c.my_key == expected def test_env_node_interpolation(monkeypatch: Any) -> None: # Test that node interpolations are not supported in env variables. monkeypatch.setenv("MYKEY", "${other_key}") - c = OmegaConf.create(dict(my_key="${env:MYKEY}", other_key=123)) + c = OmegaConf.create({"my_key": "${env:MYKEY}", "other_key": 123}) with pytest.raises( InterpolationKeyError, match=re.escape( @@ -438,15 +438,15 @@ def test_resolver_cache_3_dict_list(restore_resolvers: Any) -> None: """ OmegaConf.register_new_resolver("random", lambda _: random.uniform(0, 1)) c = OmegaConf.create( - dict( - lst1="${random:[0, 1]}", - lst2="${random:[0, 1]}", - lst3="${random:[]}", - dct1="${random:{a: 1, b: 2}}", - dct2="${random:{b: 2, a: 1}}", - mixed1="${random:{x: [1.1], y: {a: true, b: false, c: null, d: []}}}", - mixed2="${random:{x: [1.1], y: {b: false, c: null, a: true, d: []}}}", - ) + { + "lst1": "${random:[0, 1]}", + "lst2": "${random:[0, 1]}", + "lst3": "${random:[]}", + "dct1": "${random:{a: 1, b: 2}}", + "dct2": "${random:{b: 2, a: 1}}", + "mixed1": "${random:{x: [1.1], y: {a: true, b: false, c: null, d: []}}}", + "mixed2": "${random:{x: [1.1], y: {b: false, c: null, a: true, d: []}}}", + } ) assert c.lst1 == c.lst1 assert c.lst1 == c.lst2 @@ -462,7 +462,7 @@ def test_resolver_no_cache(restore_resolvers: Any) -> None: OmegaConf.register_new_resolver( "random", lambda _: random.uniform(0, 1), use_cache=False ) - c = OmegaConf.create(dict(k="${random:__}")) + c = OmegaConf.create({"k": "${random:__}"}) assert c.k != c.k @@ -591,7 +591,7 @@ def test_copy_cache(restore_resolvers: Any) -> None: def test_clear_cache(restore_resolvers: Any) -> None: OmegaConf.register_new_resolver("random", lambda _: random.randint(0, 10000000)) - c = OmegaConf.create(dict(k="${random:__}")) + c = OmegaConf.create({"k": "${random:__}"}) old = c.k OmegaConf.clear_cache(c) assert old != c.k @@ -599,7 +599,7 @@ def test_clear_cache(restore_resolvers: Any) -> None: def test_supported_chars() -> None: supported_chars = "abc123_/:-\\+.$%*@" - c = OmegaConf.create(dict(dir1="${copy:" + supported_chars + "}")) + c = OmegaConf.create({"dir1": "${copy:" + supported_chars + "}"}) OmegaConf.register_new_resolver("copy", lambda x: x) assert c.dir1 == supported_chars From 509bda517b7d4c2ee8c679d4c48aba62efb9ccd8 Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Tue, 2 Mar 2021 15:37:22 -0500 Subject: [PATCH 38/42] Minor formatting fix for consistency --- tests/test_select.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_select.py b/tests/test_select.py index ea6ba44cc..85fd06836 100644 --- a/tests/test_select.py +++ b/tests/test_select.py @@ -61,7 +61,7 @@ def test_select( @pytest.mark.parametrize("struct", [False, True]) @pytest.mark.parametrize("default", [10, None]) @pytest.mark.parametrize( - ["cfg", "key", "exc_no_default"], + ("cfg", "key", "exc_no_default"), [ pytest.param({}, "not_found", None, id="empty"), pytest.param({"missing": "???"}, "missing", None, id="missing"), @@ -122,7 +122,7 @@ def test_select_default_throw_on_missing( @pytest.mark.parametrize( - ["cfg", "key"], + ("cfg", "key"), [ pytest.param({"inter": "${bad_key}"}, "inter", id="inter_bad_key"), ], From c3965270ec159affc02801ab3fcfc824f3a1a034 Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Tue, 2 Mar 2021 15:47:26 -0500 Subject: [PATCH 39/42] Test refactor, step 1 --- tests/test_select.py | 42 ++++++++++++++++++++++++------------------ 1 file changed, 24 insertions(+), 18 deletions(-) diff --git a/tests/test_select.py b/tests/test_select.py index 85fd06836..29f81d213 100644 --- a/tests/test_select.py +++ b/tests/test_select.py @@ -61,10 +61,28 @@ def test_select( @pytest.mark.parametrize("struct", [False, True]) @pytest.mark.parametrize("default", [10, None]) @pytest.mark.parametrize( - ("cfg", "key", "exc_no_default"), + ("cfg", "key"), + [ + pytest.param({}, "not_found", id="empty"), + pytest.param({"missing": "???"}, "missing", id="missing"), + pytest.param({"int": 0}, "int.y", id="non_container"), + ], +) +def test_select_default( + cfg: Any, + struct: bool, + key: Any, + default: Any, +) -> None: + cfg = _ensure_container(cfg) + OmegaConf.set_struct(cfg, struct) + assert OmegaConf.select(cfg, key, default=default) == default + + +@pytest.mark.parametrize("struct", [False, True]) +@pytest.mark.parametrize( + ("cfg", "key", "exc"), [ - pytest.param({}, "not_found", None, id="empty"), - pytest.param({"missing": "???"}, "missing", None, id="missing"), pytest.param( {"int": 0}, "int.y", @@ -79,23 +97,11 @@ def test_select( ), ], ) -def test_select_default( - cfg: Any, - struct: bool, - key: Any, - exc_no_default: Any, - default: Any, -) -> None: +def test_select_error(cfg: Any, key: Any, exc: Any, struct: bool) -> None: cfg = _ensure_container(cfg) OmegaConf.set_struct(cfg, struct) - assert OmegaConf.select(cfg, key, default=default) == default - if exc_no_default is None: - # Not providing a default value is expected to work and return `None`. - assert OmegaConf.select(cfg, key) is None - else: - # Not providing a default value is expected to raise an exception. - with exc_no_default: - OmegaConf.select(cfg, key) + with exc: + OmegaConf.select(cfg, key) @pytest.mark.parametrize("struct", [False, True]) From ef61b9a95fd1b6e12e9bb6072fc7beee776d8e28 Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Tue, 2 Mar 2021 16:02:32 -0500 Subject: [PATCH 40/42] Refactor test_select (step 2) * Remove test_select_key_from_empty() which is already tested in test_select() * Move all tests inside a common class that is parameterized on the "struct" flag --- tests/test_select.py | 344 ++++++++++++++++++++++--------------------- 1 file changed, 177 insertions(+), 167 deletions(-) diff --git a/tests/test_select.py b/tests/test_select.py index 29f81d213..ed6cd8c8e 100644 --- a/tests/test_select.py +++ b/tests/test_select.py @@ -10,174 +10,184 @@ from omegaconf.errors import ConfigKeyError, InterpolationKeyError -@pytest.mark.parametrize("struct", [True, False, None]) -def test_select_key_from_empty(struct: Optional[bool]) -> None: - c = OmegaConf.create() - OmegaConf.set_struct(c, struct) - assert OmegaConf.select(c, "not_there") is None - - -@pytest.mark.parametrize( - "register_func", [OmegaConf.register_resolver, OmegaConf.register_new_resolver] -) -@pytest.mark.parametrize( - "cfg, key, expected", - [ - pytest.param({}, "nope", None, id="dict:none"), - pytest.param({}, "not.there", None, id="dict:none"), - pytest.param({}, "still.not.there", None, id="dict:none"), - pytest.param({"c": 1}, "c", 1, id="dict:int"), - pytest.param({"a": {"v": 1}}, "a.v", 1, id="dict:int"), - pytest.param({"a": {"v": 1}}, "a", {"v": 1}, id="dict:dict"), - pytest.param({"missing": "???"}, "missing", None, id="dict:missing"), - pytest.param([], "0", None, id="list:oob"), - pytest.param([1, "2"], "0", 1, id="list:int"), - pytest.param([1, "2"], "1", "2", id="list:str"), - pytest.param(["???"], "0", None, id="list:missing"), - pytest.param([1, {"a": 10, "c": ["foo", "bar"]}], "0", 1), - pytest.param([1, {"a": 10, "c": ["foo", "bar"]}], "1.a", 10), - pytest.param([1, {"a": 10, "c": ["foo", "bar"]}], "1.b", None), - pytest.param([1, {"a": 10, "c": ["foo", "bar"]}], "1.c.0", "foo"), - pytest.param([1, {"a": 10, "c": ["foo", "bar"]}], "1.c.1", "bar"), - pytest.param([1, 2, 3], "a", raises(TypeError)), - pytest.param({"a": {"v": 1}}, "", {"a": {"v": 1}}, id="select_root"), - pytest.param({"a": {"b": 1}, "c": "one=${a.b}"}, "c", "one=1", id="inter"), - pytest.param({"a": {"b": "one=${n}"}, "n": 1}, "a.b", "one=1", id="inter"), - pytest.param({"a": {"b": "one=${func:1}"}}, "a.b", "one=_1_", id="resolver"), - ], -) -def test_select( - restore_resolvers: Any, cfg: Any, key: Any, expected: Any, register_func: Any -) -> None: - register_func("func", lambda x: f"_{x}_") - cfg = _ensure_container(cfg) - if isinstance(expected, RaisesContext): - with expected: - OmegaConf.select(cfg, key) - else: - assert OmegaConf.select(cfg, key) == expected - - -@pytest.mark.parametrize("struct", [False, True]) -@pytest.mark.parametrize("default", [10, None]) -@pytest.mark.parametrize( - ("cfg", "key"), - [ - pytest.param({}, "not_found", id="empty"), - pytest.param({"missing": "???"}, "missing", id="missing"), - pytest.param({"int": 0}, "int.y", id="non_container"), - ], -) -def test_select_default( - cfg: Any, - struct: bool, - key: Any, - default: Any, -) -> None: - cfg = _ensure_container(cfg) - OmegaConf.set_struct(cfg, struct) - assert OmegaConf.select(cfg, key, default=default) == default - - -@pytest.mark.parametrize("struct", [False, True]) -@pytest.mark.parametrize( - ("cfg", "key", "exc"), - [ - pytest.param( - {"int": 0}, - "int.y", - pytest.raises( - ConfigKeyError, - match=re.escape( - "Error trying to access int.y: node `int` is not a container " - "and thus cannot contain `y`" +@pytest.mark.parametrize("struct", [False, True, None]) +class TestSelect: + @pytest.mark.parametrize( + "register_func", [OmegaConf.register_resolver, OmegaConf.register_new_resolver] + ) + @pytest.mark.parametrize( + "cfg, key, expected", + [ + pytest.param({}, "nope", None, id="dict:none"), + pytest.param({}, "not.there", None, id="dict:none"), + pytest.param({}, "still.not.there", None, id="dict:none"), + pytest.param({"c": 1}, "c", 1, id="dict:int"), + pytest.param({"a": {"v": 1}}, "a.v", 1, id="dict:int"), + pytest.param({"a": {"v": 1}}, "a", {"v": 1}, id="dict:dict"), + pytest.param({"missing": "???"}, "missing", None, id="dict:missing"), + pytest.param([], "0", None, id="list:oob"), + pytest.param([1, "2"], "0", 1, id="list:int"), + pytest.param([1, "2"], "1", "2", id="list:str"), + pytest.param(["???"], "0", None, id="list:missing"), + pytest.param([1, {"a": 10, "c": ["foo", "bar"]}], "0", 1), + pytest.param([1, {"a": 10, "c": ["foo", "bar"]}], "1.a", 10), + pytest.param([1, {"a": 10, "c": ["foo", "bar"]}], "1.b", None), + pytest.param([1, {"a": 10, "c": ["foo", "bar"]}], "1.c.0", "foo"), + pytest.param([1, {"a": 10, "c": ["foo", "bar"]}], "1.c.1", "bar"), + pytest.param([1, 2, 3], "a", raises(TypeError)), + pytest.param({"a": {"v": 1}}, "", {"a": {"v": 1}}, id="select_root"), + pytest.param({"a": {"b": 1}, "c": "one=${a.b}"}, "c", "one=1", id="inter"), + pytest.param({"a": {"b": "one=${n}"}, "n": 1}, "a.b", "one=1", id="inter"), + pytest.param( + {"a": {"b": "one=${func:1}"}}, "a.b", "one=_1_", id="resolver" + ), + ], + ) + def test_select( + self, + restore_resolvers: Any, + cfg: Any, + key: Any, + expected: Any, + register_func: Any, + struct: Optional[bool], + ) -> None: + register_func("func", lambda x: f"_{x}_") + cfg = _ensure_container(cfg) + OmegaConf.set_struct(cfg, struct) + if isinstance(expected, RaisesContext): + with expected: + OmegaConf.select(cfg, key) + else: + assert OmegaConf.select(cfg, key) == expected + + @pytest.mark.parametrize("default", [10, None]) + @pytest.mark.parametrize( + ("cfg", "key"), + [ + pytest.param({}, "not_found", id="empty"), + pytest.param({"missing": "???"}, "missing", id="missing"), + pytest.param({"int": 0}, "int.y", id="non_container"), + ], + ) + def test_select_default( + self, + cfg: Any, + struct: Optional[bool], + key: Any, + default: Any, + ) -> None: + cfg = _ensure_container(cfg) + OmegaConf.set_struct(cfg, struct) + assert OmegaConf.select(cfg, key, default=default) == default + + @pytest.mark.parametrize("default", [10, None]) + @pytest.mark.parametrize( + "cfg, key", + [ + pytest.param({"missing": "???"}, "missing", id="missing_dict"), + pytest.param(["???"], "0", id="missing_list"), + ], + ) + def test_select_default_throw_on_missing( + self, + cfg: Any, + struct: Optional[bool], + key: Any, + default: Any, + ) -> None: + cfg = _ensure_container(cfg) + OmegaConf.set_struct(cfg, struct) + + # throw on missing still throws if default is provided + with pytest.raises(MissingMandatoryValue): + OmegaConf.select(cfg, key, default=default, throw_on_missing=True) + + @pytest.mark.parametrize( + ("cfg", "key", "exc"), + [ + pytest.param( + {"int": 0}, + "int.y", + pytest.raises( + ConfigKeyError, + match=re.escape( + "Error trying to access int.y: node `int` is not a container " + "and thus cannot contain `y`" + ), ), + id="non_container", ), - id="non_container", - ), - ], -) -def test_select_error(cfg: Any, key: Any, exc: Any, struct: bool) -> None: - cfg = _ensure_container(cfg) - OmegaConf.set_struct(cfg, struct) - with exc: - OmegaConf.select(cfg, key) - - -@pytest.mark.parametrize("struct", [False, True]) -@pytest.mark.parametrize("default", [10, None]) -@pytest.mark.parametrize( - "cfg, key", - [ - pytest.param({"missing": "???"}, "missing", id="missing_dict"), - pytest.param(["???"], "0", id="missing_list"), - ], -) -def test_select_default_throw_on_missing( - cfg: Any, - struct: bool, - key: Any, - default: Any, -) -> None: - cfg = _ensure_container(cfg) - OmegaConf.set_struct(cfg, struct) - - # throw on missing still throws if default is provided - with pytest.raises(MissingMandatoryValue): - OmegaConf.select(cfg, key, default=default, throw_on_missing=True) - - -@pytest.mark.parametrize( - ("cfg", "key"), - [ - pytest.param({"inter": "${bad_key}"}, "inter", id="inter_bad_key"), - ], -) -def test_select_default_throw_on_resolution_failure(cfg: Any, key: Any) -> None: - cfg = OmegaConf.create(cfg) - - # throw on resolution failure still throws if default is provided - with pytest.raises(InterpolationKeyError): - OmegaConf.select(cfg, key, default=123, throw_on_resolution_failure=True) - - -@pytest.mark.parametrize( - ("cfg", "node_key", "expected"), - [ - pytest.param({"foo": "${bar}", "bar": 10}, "foo", 10, id="simple"), - pytest.param({"foo": "${bar}"}, "foo", None, id="no_key"), - pytest.param({"foo": "${bar}", "bar": "???"}, "foo", None, id="key_missing"), - pytest.param( - {"foo": "${bar}", "bar": "${zoo}", "zoo": "???"}, - "foo", - None, - id="key_missing_indirect", - ), - ], -) -def test_select_invalid_interpolation(cfg: Any, node_key: str, expected: Any) -> None: - cfg = _ensure_container(cfg) - resolved = OmegaConf.select( - cfg, - node_key, - throw_on_missing=False, - throw_on_resolution_failure=False, + ], ) - assert resolved == expected - - -def test_select_from_dict() -> None: - c = OmegaConf.create({"missing": "???"}) - with pytest.raises(MissingMandatoryValue): - OmegaConf.select(c, "missing", throw_on_missing=True) - assert OmegaConf.select(c, "missing", throw_on_missing=False) is None - assert OmegaConf.select(c, "missing") is None - + def test_select_error( + self, cfg: Any, key: Any, exc: Any, struct: Optional[bool] + ) -> None: + cfg = _ensure_container(cfg) + OmegaConf.set_struct(cfg, struct) + with exc: + OmegaConf.select(cfg, key) -def test_select_deprecated() -> None: - c = OmegaConf.create({"foo": "bar"}) - with pytest.warns( - expected_warning=UserWarning, - match=re.escape("select() is deprecated, use OmegaConf.select(). (Since 2.0)"), - ): - c.select("foo") + @pytest.mark.parametrize( + ("cfg", "key"), + [ + pytest.param({"inter": "${bad_key}"}, "inter", id="inter_bad_key"), + ], + ) + def test_select_default_throw_on_resolution_failure( + self, cfg: Any, key: Any, struct: Optional[bool] + ) -> None: + cfg = _ensure_container(cfg) + OmegaConf.set_struct(cfg, struct) + + # throw on resolution failure still throws if default is provided + with pytest.raises(InterpolationKeyError): + OmegaConf.select(cfg, key, default=123, throw_on_resolution_failure=True) + + @pytest.mark.parametrize( + ("cfg", "node_key", "expected"), + [ + pytest.param({"foo": "${bar}", "bar": 10}, "foo", 10, id="simple"), + pytest.param({"foo": "${bar}"}, "foo", None, id="no_key"), + pytest.param( + {"foo": "${bar}", "bar": "???"}, "foo", None, id="key_missing" + ), + pytest.param( + {"foo": "${bar}", "bar": "${zoo}", "zoo": "???"}, + "foo", + None, + id="key_missing_indirect", + ), + ], + ) + def test_select_invalid_interpolation( + self, cfg: Any, node_key: str, expected: Any, struct: Optional[bool] + ) -> None: + cfg = _ensure_container(cfg) + OmegaConf.set_struct(cfg, struct) + resolved = OmegaConf.select( + cfg, + node_key, + throw_on_missing=False, + throw_on_resolution_failure=False, + ) + assert resolved == expected + + def test_select_from_dict(self, struct: Optional[bool]) -> None: + cfg = OmegaConf.create({"missing": "???"}) + OmegaConf.set_struct(cfg, struct) + with pytest.raises(MissingMandatoryValue): + OmegaConf.select(cfg, "missing", throw_on_missing=True) + assert OmegaConf.select(cfg, "missing", throw_on_missing=False) is None + assert OmegaConf.select(cfg, "missing") is None + + def test_select_deprecated(self, struct: Optional[bool]) -> None: + cfg = OmegaConf.create({"foo": "bar"}) + OmegaConf.set_struct(cfg, struct) + with pytest.warns( + expected_warning=UserWarning, + match=re.escape( + "select() is deprecated, use OmegaConf.select(). (Since 2.0)" + ), + ): + cfg.select("foo") From 1a5ea905d4df0a44e82bd46dd39085aaa8549c99 Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Tue, 2 Mar 2021 18:48:17 -0500 Subject: [PATCH 41/42] Update news/543.api_change Co-authored-by: Omry Yadan --- news/543.api_change | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/news/543.api_change b/news/543.api_change index 9dd1c518f..06f55ecc5 100644 --- a/news/543.api_change +++ b/news/543.api_change @@ -1 +1 @@ -An interpolation interpolating a missing key no longer counts as missing. Resolving such an interpolation raises an `InterpolationToMissingValueError` exception. +`OmegaConf.is_missing(cfg, key)` no longer returns True if key is an interpolation to a missing key. Resolving such an interpolation raises an `InterpolationToMissingValueError` exception. From a851a91f30d82179d2209073647bf2bc03ad401f Mon Sep 17 00:00:00 2001 From: Olivier Delalleau <507137+odelalleau@users.noreply.github.com> Date: Tue, 2 Mar 2021 18:49:56 -0500 Subject: [PATCH 42/42] Update news according to suggestion --- news/543.api_change | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/news/543.api_change b/news/543.api_change index 06f55ecc5..78391de36 100644 --- a/news/543.api_change +++ b/news/543.api_change @@ -1 +1 @@ -`OmegaConf.is_missing(cfg, key)` no longer returns True if key is an interpolation to a missing key. Resolving such an interpolation raises an `InterpolationToMissingValueError` exception. +`OmegaConf.select()`, `DictConfig.{get(),pop()}`, `ListConfig.{get(),pop()}` no longer return the specified default value when the accessed key is an interpolation that cannot be resolved: instead, an exception is raised.