Skip to content
New issue

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

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

To container throw on missing #503

Closed
wants to merge 7 commits into from
Closed

To container throw on missing #503

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
667a50e
rebase against master
Jasha10 Jan 31, 2021
fe32082
Catch interpolation resolution failure in anticipation of #545
Jasha10 Feb 18, 2021
01d5395
_get_node missing value for key of type slice: simplify error reporting
Jasha10 Feb 18, 2021
f083699
move tests from test_errors.py -> test_to_container.py
Jasha10 Feb 18, 2021
1a19966
remove unused type constant in test_to_container.py
Jasha10 Feb 18, 2021
b13e61b
refactor tests
Jasha10 Feb 18, 2021
496ae3b
formatting
Jasha10 Feb 18, 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
20 changes: 14 additions & 6 deletions omegaconf/basecontainer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,9 +229,13 @@ def convert(val: Node) -> Any:
node = conf._get_node(key, throw_on_missing_value=throw_on_missing)
assert isinstance(node, Node)
if resolve:
node = node._dereference_node(
throw_on_missing=False, throw_on_resolution_failure=True
)
try:
node = node._dereference_node(
throw_on_missing=False, throw_on_resolution_failure=True
)
except MissingMandatoryValue as e:
assert node is not None
conf._format_and_raise(key=key, value=node._value(), cause=e)
Comment on lines +232 to +238
Copy link
Owner

Choose a reason for hiding this comment

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

  1. Why is _dereference_node throwing a MandatoryMissingValue is throw_on_missing is False?
  2. Why are you passing False to throw_on_missing and not the input flag?

Copy link
Collaborator Author

@Jasha10 Jasha10 Feb 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

  1. Why is _dereference_node throwing a MandatoryMissingValue is throw_on_missing is False?

This is because we are calling _dereference_node with throw_on_resolution_failure=True.

Edit:
I was mistaken about what the throw_on_resolution_failure flag is doing.
I think I need to investigate this a bit more...

If #545 is merged in it's current state, _dereference_node will raise MandatoryMissingValue if throw_on_resolution_failure==True and the interpolation cannot be resolved.

This being said, I actually think it makes more sense to change this from

_dereference_node(..., throw_on_resolution_failure=True)

to

_dereference_node(..., throw_on_resolution_failure=throw_on_missing)

so that resolution failure will not cause error if the user does not specify the throw_on_missing option. I'll add tests to this effect...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. Why are you passing False to throw_on_missing and not the input flag?

The current workflow is:

  • first, get the node and (optionally) throw if missing:
node = conf._get_node(key, throw_on_missing_value=throw_on_missing)
  • second: if resolve==True, dereference the node:
node = node._dereference_node(throw_on_missing=False, throw_on_resolution_failure=True)

Since the _get_node call already has throw_on_missing_value=throw_on_missing, it would be redundant to use the throw_on_missing flag in the _dereference_node call as well.


assert node is not None
if isinstance(node, Container):
Expand All @@ -251,9 +255,13 @@ def convert(val: Node) -> Any:
node = conf._get_node(index, throw_on_missing_value=throw_on_missing)
assert isinstance(node, Node)
if resolve:
node = node._dereference_node(
throw_on_missing=False, throw_on_resolution_failure=True
)
try:
node = node._dereference_node(
throw_on_missing=False, throw_on_resolution_failure=True
)
except MissingMandatoryValue as e:
assert node is not None
conf._format_and_raise(key=index, value=node._value(), cause=e)
assert node is not None
if isinstance(node, Container):
item = BaseContainer._to_content(
Expand Down
5 changes: 2 additions & 3 deletions omegaconf/listconfig.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,11 +390,10 @@ def _get_node(
if value is not None:
if isinstance(key, slice):
assert isinstance(value, list)
indices = range(len(self))[key]
for idx, v in zip(indices, value):
for v in value:
if throw_on_missing_value and v._is_missing():
self._format_and_raise(
key=idx,
key=key,
value=None,
cause=MissingMandatoryValue("Missing mandatory value"),
)
Expand Down
12 changes: 6 additions & 6 deletions tests/test_base_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -560,21 +560,21 @@ def test_get_node(cfg: Any, key: Any, expected: Any) -> None:


@pytest.mark.parametrize(
"cfg, key, missing_key", # it might be that key != missing_key in the case of slice
"cfg, key",
[
# dict
pytest.param({"foo": "???"}, "foo", "foo", id="dict"),
pytest.param({"foo": "???"}, "foo", id="dict"),
# list
pytest.param([10, "???", 30], 1, 1, id="list_int"),
pytest.param([10, "???", 30], slice(1, 2), 1, id="list_slice"),
pytest.param([10, "???", 30], 1, id="list_int"),
pytest.param([10, "???", 30], slice(1, 2), id="list_slice"),
],
)
def test_get_node_throw_on_missing_value(cfg: Any, key: Any, missing_key: Any) -> None:
def test_get_node_throw_on_missing_value(cfg: Any, key: Any) -> None:
cfg = OmegaConf.create(cfg)
with pytest.raises(
MissingMandatoryValue, match="Missing mandatory value"
) as excinfo:
cfg._get_node(key, throw_on_missing_value=True)
ex: MissingMandatoryValue = excinfo.value
assert ex.key == missing_key
assert ex.key == key
assert ex.parent_node == cfg
20 changes: 19 additions & 1 deletion tests/test_errors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1180,7 +1180,25 @@ def finalize(self, cfg: Any) -> None:
child_node=lambda cfg: cfg.subcfg._get_node("x"),
parent_node=lambda cfg: cfg.subcfg,
),
id="to_container-throw_on_missing:interpolation",
id="to_container-throw_on_missing:dictconfig-interpolation",
),
pytest.param(
Expected(
create=lambda: OmegaConf.create(
{"missing": "???", "subcfg": ["${missing}"]}
),
op=lambda cfg: OmegaConf.to_container(
cfg.subcfg, resolve=True, throw_on_missing=True
),
exception_type=MissingMandatoryValue,
msg="Missing mandatory value",
key=0,
full_key="subcfg[0]",
child_node=lambda cfg: cfg.subcfg._get_node(0),
parent_node=lambda cfg: cfg.subcfg,
object_type=list,
),
id="to_container-throw_on_missing:listconfig-interpolation",
),
]

Expand Down