Skip to content

OmegaConf.to_container(..., throw_on_missing) keyword arg #730

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Jasha10 merged 33 commits into from
Jun 13, 2021
Merged

OmegaConf.to_container(..., throw_on_missing) keyword arg #730

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
c658e55
add throw_on_missing param to OmegaConf.to_container
Jasha10 May 21, 2021
998802a
extract get_node_value routine in BaseContainer._to_content
Jasha10 May 21, 2021
cb78bc7
throw_on_missing==True suppresses InterpolationToMissingValueError
Jasha10 May 21, 2021
3bbe835
Test InterpolationToMissingValueError for Structured Conf
Jasha10 May 21, 2021
ea7ad68
Test throw for missing-in-dict-in-structured conf
Jasha10 May 21, 2021
a283995
test_errors: to_container:throw_on_missing-interpolation
Jasha10 May 21, 2021
c655234
test_errors: to_object:structured-throw_on_missing-interpolation
Jasha10 May 21, 2021
f5a6206
test_errors: to_container:throw_on_missing-{dict,list}
Jasha10 May 21, 2021
94b9a80
test_errors: to_container:throw_on_missing-{dict-value,list-item}
Jasha10 May 21, 2021
2ca0538
update DictConfig._to_object docstring
Jasha10 May 30, 2021
40278d8
OmegaConf.to_container docstring: describe `throw_on_missing` flag
Jasha10 May 30, 2021
1e7af80
update OmegaConf.to_object docstring
Jasha10 May 30, 2021
28bcd62
more consistent formatting for pytest param ids
Jasha10 May 30, 2021
834a0bc
more consistent formatting for pytest param ids (2)
Jasha10 May 30, 2021
52d5c89
consistent "Missing mandatory" errmsg for leaf vs non-leaf nodes
Jasha10 May 30, 2021
c1893e3
dont catch InterpolationResolutionError
Jasha10 Jun 2, 2021
f664ecd
Update to_object InterpolationResolutionError handling
Jasha10 Jun 2, 2021
ba97dcb
DictConfig._to_object: check node._is_missing AFTER resolving
Jasha10 Jun 2, 2021
df47f0e
remove TODO note
Jasha10 Jun 4, 2021
bbfbe02
Merge branch master & fix merge conflicts
Jasha10 Jun 4, 2021
0df8e1a
uncomment {Dict,List}Config("???") test case (now passing after mergi…
Jasha10 Jun 4, 2021
6f696f3
In TestResolveBadInterpolation: Add expected exception type
Jasha10 Jun 7, 2021
05221aa
restore a test case that was accidentally removed
Jasha10 Jun 7, 2021
e9fa803
add news fragment
Jasha10 Jun 7, 2021
05a3e51
docs for to_container throw_on_missing keyword argument
Jasha10 Jun 7, 2021
4d3195f
reorder fields so that InterpError is raised before MissingMandatoryV…
Jasha10 Jun 8, 2021
0d87d3c
fix typo in docs/source/usage.rst
Jasha10 Jun 8, 2021
e11bb05
deduplicate test cases in TestResolveBadInterpolation
Jasha10 Jun 8, 2021
a059a8f
Ensure top-level interpolations are handled (and restore a removed te…
Jasha10 Jun 8, 2021
a4cc88c
Update omegaconf/basecontainer.py
Jasha10 Jun 9, 2021
6d570df
test InterpolationResolutionError given ListConfig("${bar}") at confi…
Jasha10 Jun 9, 2021
f98b1f0
test for valid toplevel interpolation in {Dict,List}Config
Jasha10 Jun 9, 2021
e67739b
use _ensure_container in test_to_container
Jasha10 Jun 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 43 additions & 29 deletions omegaconf/basecontainer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
from .base import Container, ContainerMetadata, DictKeyType, Node, SCMode
from .errors import (
ConfigCycleDetectedException,
InterpolationToMissingValueError,
MissingMandatoryValue,
ReadonlyConfigError,
ValidationError,
Expand Down Expand Up @@ -174,6 +175,7 @@ def is_empty(self) -> bool:
def _to_content(
conf: Container,
resolve: bool,
throw_on_missing: bool,
enum_to_str: bool = False,
structured_config_mode: SCMode = SCMode.DICT,
) -> Union[None, Any, str, Dict[DictKeyType, Any], List[Any]]:
Expand All @@ -186,6 +188,33 @@ def convert(val: Node) -> Any:

return value

def get_node_value(key: Union[DictKeyType, int]) -> Any:
try:
node = conf._get_node(key, throw_on_missing_value=throw_on_missing)
except MissingMandatoryValue as e:
conf._format_and_raise(key=key, value=None, cause=e)
Copy link
Copy Markdown
Owner

@omry omry Jun 3, 2021

Choose a reason for hiding this comment

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

Generally speaking, it's better to format exceptions at high level functions, otherwise you risk formatting them multiple times.

(e.g: in OmegaConf.to_container() or other high level functions calling _to_content()).

Copy link
Copy Markdown
Collaborator Author

@Jasha10 Jasha10 Jun 4, 2021

Choose a reason for hiding this comment

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

Hmm... If I do formatting in OmegaConf.to_container(), then I will not be able to set the $KEY properly (since the for loop iterating through keys happens in this lower-level _to_content function).

Copy link
Copy Markdown
Owner

@omry omry Jun 4, 2021

Choose a reason for hiding this comment

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

Is this related to this change? raise MissingMandatoryValue("Missing mandatory value: $KEY") ?
Why not just use the key directly when raising the exception?

Copy link
Copy Markdown
Collaborator Author

@Jasha10 Jasha10 Jun 7, 2021

Choose a reason for hiding this comment

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

Is this related to this change?

Yes, I think it's related -- In the past, a MissingMandatoryValue would not have been raised.

Why not just use the key directly when raising the exception?

Something like this?

raise MissingMandatoryValue(f"Missing mandatory value: {key}")

My point was: if we call format_and_raise in OmegaConf.to_container instead of here in BaseContainer._to_content, then the format_and_raise function will not have the correct key passed in to it (since the high-level function does not know what key caused the exception).

Copy link
Copy Markdown
Owner

@omry omry Jun 8, 2021

Choose a reason for hiding this comment

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

Something like this?

raise MissingMandatoryValue(f"Missing mandatory value: {key}")

Yes.
Depending on exactly what is going on here, you may need to pass the key from the outside or use the key from the offending node (node._key()).

Copy link
Copy Markdown
Collaborator Author

@Jasha10 Jasha10 Jun 9, 2021

Choose a reason for hiding this comment

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

I tried it in this commit on a separate branch (which is based off of this PR branch).
The tests pass, but we are losing some information in the error messages. For example, here is the error before the commit:

>>> OmegaConf.to_object(OmegaConf.create({"a": {"b": "???"}}))
Traceback (most recent call last):
...
omegaconf.errors.MissingMandatoryValue: Missing mandatory value: b
    full_key: a.b
    object_type=dict

and here is the error after the commit:

>>> OmegaConf.to_object(OmegaConf.create({"a": {"b": "???"}}))
Traceback (most recent call last):
...
omegaconf.errors.MissingMandatoryValue: Missing mandatory value: b
    full_key:
    object_type=dict

There is no longer information about the full_key that caused the error.

Generally speaking, it's better to format exceptions at high level functions, otherwise you risk formatting them multiple times.

Is it really that bad to format multiple times?

assert isinstance(node, Node)
if resolve:
try:
node = node._dereference_node()
except InterpolationToMissingValueError as e:
if throw_on_missing:
conf._format_and_raise(key=key, value=None, cause=e)
else:
return "???"

if isinstance(node, Container):
value = BaseContainer._to_content(
node,
resolve=resolve,
throw_on_missing=throw_on_missing,
enum_to_str=enum_to_str,
structured_config_mode=structured_config_mode,
)
else:
value = convert(node)
return value

assert isinstance(conf, Container)
if conf._is_none():
return None
Expand All @@ -194,7 +223,16 @@ def convert(val: Node) -> Any:
assert isinstance(inter, str)
return inter
elif conf._is_missing():
return MISSING
if throw_on_missing:
conf._format_and_raise(
key=None,
value=None,
cause=MissingMandatoryValue(
f"Encountered a missing {type(conf).__name__} with `throw_on_missing==True`"
),
)
else:
return MISSING
elif isinstance(conf, DictConfig):
if (
conf._metadata.object_type is not None
Expand All @@ -208,40 +246,16 @@ def convert(val: Node) -> Any:

retdict: Dict[str, Any] = {}
for key in conf.keys():
node = conf._get_node(key)
assert isinstance(node, Node)
if resolve:
node = node._dereference_node()

value = get_node_value(key)
if enum_to_str and isinstance(key, Enum):
key = f"{key.name}"
if isinstance(node, Container):
retdict[key] = BaseContainer._to_content(
node,
resolve=resolve,
enum_to_str=enum_to_str,
structured_config_mode=structured_config_mode,
)
else:
retdict[key] = convert(node)
retdict[key] = value
return retdict
elif isinstance(conf, ListConfig):
retlist: List[Any] = []
for index in range(len(conf)):
node = conf._get_node(index)
assert isinstance(node, Node)
if resolve:
node = node._dereference_node()
if isinstance(node, Container):
item = BaseContainer._to_content(
node,
resolve=resolve,
enum_to_str=enum_to_str,
structured_config_mode=structured_config_mode,
)
retlist.append(item)
else:
retlist.append(convert(node))
item = get_node_value(index)
retlist.append(item)

return retlist

Expand Down
19 changes: 10 additions & 9 deletions omegaconf/dictconfig.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,14 @@
type_str,
valid_value_annotation_type,
)
from .base import Container, ContainerMetadata, DictKeyType, Node, SCMode
from .base import Container, ContainerMetadata, DictKeyType, Node
from .basecontainer import BaseContainer
from .errors import (
ConfigAttributeError,
ConfigKeyError,
ConfigTypeError,
InterpolationResolutionError,
InterpolationToMissingValueError,
KeyValidationError,
MissingMandatoryValue,
OmegaConfBaseException,
Expand Down Expand Up @@ -469,7 +470,7 @@ def _get_node(
if throw_on_missing_key:
raise ConfigKeyError(f"Missing key {key}")
elif throw_on_missing_value and value._is_missing():
raise MissingMandatoryValue("Missing mandatory value")
raise MissingMandatoryValue("Missing mandatory value: $KEY")
Copy link
Copy Markdown
Owner

@omry omry Jun 8, 2021

Choose a reason for hiding this comment

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

You can try this here:

            raise MissingMandatoryValue("Missing mandatory value: '{value._key()}'")

Copy link
Copy Markdown
Collaborator Author

@Jasha10 Jasha10 Jun 9, 2021

Choose a reason for hiding this comment

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

See my comment above.

return value

def pop(self, key: DictKeyType, default: Any = _DEFAULT_MARKER_) -> Any:
Expand Down Expand Up @@ -711,6 +712,8 @@ def _to_object(self) -> Any:
This requires `self` to be a structured config.
Nested subconfigs are converted to_container with resolve=True.
"""
from omegaconf import OmegaConf

object_type = self._metadata.object_type
assert is_structured_config(object_type)
object_type_field_names = set(get_structured_config_field_names(object_type))
Expand All @@ -720,14 +723,12 @@ def _to_object(self) -> Any:
for k in self.keys():
node = self._get_node(k)
assert isinstance(node, Node)
node = node._dereference_node()
try:
node = node._dereference_node()
except InterpolationToMissingValueError as e:
self._format_and_raise(key=k, value=None, cause=e)
if isinstance(node, Container):
v = BaseContainer._to_content(
node,
resolve=True,
enum_to_str=False,
structured_config_mode=SCMode.INSTANTIATE,
)
v = OmegaConf.to_object(node)
else:
v = node._value()

Expand Down
2 changes: 1 addition & 1 deletion omegaconf/listconfig.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,7 @@ def _get_node(
else:
assert isinstance(value, Node)
if throw_on_missing_value and value._is_missing():
raise MissingMandatoryValue("Missing mandatory value")
raise MissingMandatoryValue("Missing mandatory value: $KEY")
return value
except (IndexError, TypeError, MissingMandatoryValue, KeyValidationError) as e:
if isinstance(e, MissingMandatoryValue) and throw_on_missing_value:
Expand Down
3 changes: 3 additions & 0 deletions omegaconf/omegaconf.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -526,6 +526,7 @@ def to_container(
cfg: Any,
*,
resolve: bool = False,
throw_on_missing: bool = False,
enum_to_str: bool = False,
structured_config_mode: SCMode = SCMode.DICT,
) -> Union[Dict[DictKeyType, Any], List[Any], None, str]:
Expand All @@ -550,6 +551,7 @@ def to_container(
return BaseContainer._to_content(
cfg,
resolve=resolve,
throw_on_missing=throw_on_missing,
enum_to_str=enum_to_str,
structured_config_mode=structured_config_mode,
)
Expand All @@ -569,6 +571,7 @@ def to_object(cfg: Any) -> Union[Dict[DictKeyType, Any], List[Any], None, str, A
return OmegaConf.to_container(
cfg=cfg,
resolve=True,
throw_on_missing=True,
enum_to_str=False,
structured_config_mode=SCMode.INSTANTIATE,
)
Expand Down
11 changes: 11 additions & 0 deletions tests/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,17 @@ class FoobarParams:
params: FoobarParams = FoobarParams()


@dataclass
class NestedInterpolationToMissing:
name: str = MISSING

@dataclass
class BazParams:
baz: str = "${..name}"

subcfg: BazParams = BazParams()


@dataclass
class StructuredWithMissing:
num: int = MISSING
Expand Down
78 changes: 78 additions & 0 deletions tests/test_errors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
ConcretePlugin,
IllegalType,
Module,
NestedInterpolationToMissing,
Package,
Plugin,
Str2Int,
Expand Down Expand Up @@ -1288,6 +1289,83 @@ def finalize(self, cfg: Any) -> None:
),
id="to_object:structured-missing-field",
),
param(
Expected(
create=lambda: OmegaConf.structured(NestedInterpolationToMissing).subcfg,
op=lambda cfg: OmegaConf.to_object(cfg),
exception_type=InterpolationToMissingValueError,
msg=(
"MissingMandatoryValue while resolving interpolation: "
"Missing mandatory value: name"
),
key="baz",
full_key="subcfg.baz",
child_node=lambda cfg: cfg._get_node("baz"),
ref_type=NestedInterpolationToMissing.BazParams,
),
id="to_object:structured-throw_on_missing-interpolation",
),
# to_container throw_on_missing
param(
Expected(
create=lambda: OmegaConf.create(
{"missing_val": "???", "subcfg": {"x": "${missing_val}"}}
).subcfg,
op=lambda cfg: OmegaConf.to_container(
cfg, resolve=True, throw_on_missing=True
),
exception_type=InterpolationToMissingValueError,
msg=(
"MissingMandatoryValue while resolving interpolation: "
"Missing mandatory value: missing_val"
),
key="x",
full_key="subcfg.x",
child_node=lambda cfg: cfg._get_node("x"),
),
id="to_container:throw_on_missing-interpolation",
),
param(
Expected(
create=lambda: DictConfig("???"),
op=lambda cfg: OmegaConf.to_container(cfg, throw_on_missing=True),
exception_type=MissingMandatoryValue,
msg="Encountered a missing DictConfig with `throw_on_missing==True`",
),
id="to_container:throw_on_missing-dict",
),
param(
Expected(
create=lambda: ListConfig("???"),
op=lambda cfg: OmegaConf.to_container(cfg, throw_on_missing=True),
exception_type=MissingMandatoryValue,
msg="Encountered a missing ListConfig with `throw_on_missing==True`",
),
id="to_container:throw_on_missing-list",
),
param(
Expected(
create=lambda: DictConfig({"a": "???"}),
op=lambda cfg: OmegaConf.to_container(cfg, throw_on_missing=True),
exception_type=MissingMandatoryValue,
msg="Missing mandatory value: a",
key="a",
child_node=lambda cfg: cfg._get_node("a"),
),
id="to_container:throw_on_missing-dict-value",
),
param(
Expected(
create=lambda: ListConfig(["???"]),
op=lambda cfg: OmegaConf.to_container(cfg, throw_on_missing=True),
exception_type=MissingMandatoryValue,
msg="Missing mandatory value: 0",
key=0,
full_key="[0]",
child_node=lambda cfg: cfg._get_node(0),
),
id="to_container:throw_on_missing-list-item",
),
Copy link
Copy Markdown
Owner

@omry omry Jun 8, 2021

Choose a reason for hiding this comment

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

This is adding almost 200 lines to an already huge test file.
We should refactor this file somehow at some point.

]


Expand Down
Loading