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

Jump to bottom

Deprecate OmegaConf.is_optional #700

Merged
merged 8 commits into from
Apr 29, 2021
Merged

Deprecate OmegaConf.is_optional #700

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3290348
make OmegaConf.is_optional call _utils._is_optional
Jasha10 Apr 28, 2021
3c8279b
change internal usage OmegaConf.is_optional -> _utils._is_optional
Jasha10 Apr 28, 2021
42ddb2d
warn that OmegaConf.is_optional is deprecated
Jasha10 Apr 28, 2021
3f2fe49
move from test_omegaconf.py to test_utils.py
Jasha10 Apr 28, 2021
0b43f8d
separate OmegaConf.is_optional vs _utils._is_optional implementations
Jasha10 Apr 29, 2021
beca3b7
code coverage for deprecated OmegaConf.is_optional
Jasha10 Apr 29, 2021
7bef172
replace `if isinstance` with `assert isinstance`
Jasha10 Apr 29, 2021
a9b1555
Move a test from test_utils.py to test_omegaconf.py
Jasha10 Apr 29, 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
14 changes: 14 additions & 0 deletions omegaconf/_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ def _is_union(type_: Any) -> bool:


def _resolve_optional(type_: Any) -> Tuple[bool, Any]:
"""Check whether `type_` is equivalent to `typing.Optional[T]` for some T."""
if getattr(type_, "__origin__", None) is Union:
args = type_.__args__
if len(args) == 2 and args[1] == type(None): # noqa E721
Expand All @@ -189,6 +190,19 @@ def _resolve_optional(type_: Any) -> Tuple[bool, Any]:
return False, type_


def _is_optional(obj: Any, key: Optional[Union[int, str]] = None) -> bool:
"""Check `obj` metadata to see if the given node is optional."""
from .base import Container, Node

if key is not None:
assert isinstance(obj, Container)
obj = obj._get_node(key)
if isinstance(obj, Node):
return obj._is_optional()
else:
return True
omry marked this conversation as resolved.
Show resolved Hide resolved


def _resolve_forward(type_: Type[Any], module: str) -> Type[Any]:
import typing # lgtm [py/import-and-import-from]

Expand Down
5 changes: 2 additions & 3 deletions omegaconf/basecontainer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
_is_missing_literal,
_is_missing_value,
_is_none,
_is_optional,
_resolve_optional,
get_ref_type,
get_structured_config_data,
Expand Down Expand Up @@ -512,9 +513,7 @@ def _set_item_impl(self, key: Any, value: Any) -> None:
)

def wrap(key: Any, val: Any) -> Node:
from omegaconf import OmegaConf

is_optional = OmegaConf.is_optional(self)
is_optional = _is_optional(self)
if not is_structured_config(val):
ref_type = self._metadata.element_type
else:
Expand Down
9 changes: 5 additions & 4 deletions omegaconf/listconfig.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
from ._utils import (
ValueKind,
_is_none,
_is_optional,
format_and_raise,
get_value_kind,
is_int,
Expand Down Expand Up @@ -246,7 +247,7 @@ def __setitem__(self, index: Union[int, slice], value: Any) -> None:

def append(self, item: Any) -> None:
try:
from omegaconf.omegaconf import OmegaConf, _maybe_wrap
from omegaconf.omegaconf import _maybe_wrap

index = len(self)
self._validate_set(key=index, value=item)
Expand All @@ -255,7 +256,7 @@ def append(self, item: Any) -> None:
ref_type=self.__dict__["_metadata"].element_type,
key=index,
value=item,
is_optional=OmegaConf.is_optional(self),
is_optional=_is_optional(self),
parent=self,
)
self.__dict__["_content"].append(node)
Expand All @@ -271,7 +272,7 @@ def _update_keys(self) -> None:
node._metadata.key = i

def insert(self, index: int, item: Any) -> None:
from omegaconf.omegaconf import OmegaConf, _maybe_wrap
from omegaconf.omegaconf import _maybe_wrap

try:
if self._get_flag("readonly"):
Expand All @@ -291,7 +292,7 @@ def insert(self, index: int, item: Any) -> None:
ref_type=self.__dict__["_metadata"].element_type,
key=index,
value=item,
is_optional=OmegaConf.is_optional(self),
is_optional=_is_optional(self),
parent=self,
)
self._validate_set(key=index, value=node)
Expand Down
14 changes: 7 additions & 7 deletions omegaconf/omegaconf.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
_DEFAULT_MARKER_,
_ensure_container,
_is_none,
_is_optional,
format_and_raise,
get_dict_key_value_types,
get_list_element_type,
Expand Down Expand Up @@ -585,15 +586,14 @@ def is_missing(cfg: Any, key: DictKeyType) -> bool:
except (UnsupportedInterpolationType, KeyError, AttributeError):
return False

# DEPRECATED: remove in 2.2
@staticmethod
def is_optional(obj: Any, key: Optional[Union[int, str]] = None) -> bool:
if key is not None:
assert isinstance(obj, Container)
obj = obj._get_node(key)
if isinstance(obj, Node):
return obj._is_optional()
else:
return True
warnings.warn(
"`OmegaConf.is_optional()` is deprecated, see https://github.com/omry/omegaconf/issues/698",
stacklevel=2,
)
return _is_optional(obj, key)
Copy link
Owner

Choose a reason for hiding this comment

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

I am not sure there is a good reason behind defaulting to True.
If there isn't any, I think you can maintain the old deprecated implementation here for now and make a more strict internal _is_optional.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that the defaulting to True is for the case where _is_optional(obj, key) is called and key cannot be found.

Currently, no code in the omegaconf/ directory is using this "defaulting to True" branch, as all the calls use the one-argument signature _is_optional(obj) (though some code in the tests/ directory does use the two-argument signature).

Instead of returning True in the case where key cannot be found, what do you think about raising ConfigKeyError?

Copy link
Owner

@omry omry Apr 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Since it's an internal function and the branch is not used now, I suggest using assertion to ensure that it's not tripping anyone.
If we ever need to do something if the key does not exist we can easily change this.

Make a pass on the usage to convince yourself it's not current;y possible for a user to hit this branch though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Make a pass on the usage to convince yourself it's not current;y possible for a user to hit this branch though.

Yup, it's not possible for a user to hit.


# DEPRECATED: remove in 2.2
@staticmethod
Expand Down
12 changes: 6 additions & 6 deletions tests/structured_conf/test_structured_basic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
_utils,
flag_override,
)
from omegaconf._utils import get_ref_type
from omegaconf._utils import _is_optional, get_ref_type
from omegaconf.errors import ConfigKeyError, UnsupportedValueType
from tests import IllegalType

Expand Down Expand Up @@ -251,24 +251,24 @@ def test_missing2(self, module: Any) -> None:
def test_plugin_holder(self, module: Any) -> None:
cfg = OmegaConf.create(module.PluginHolder)

assert OmegaConf.is_optional(cfg, "none")
assert _is_optional(cfg, "none")
assert _utils.get_ref_type(cfg, "none") == Optional[module.Plugin]
assert OmegaConf.get_type(cfg, "none") is None

assert not OmegaConf.is_optional(cfg, "missing")
assert not _is_optional(cfg, "missing")
assert _utils.get_ref_type(cfg, "missing") == module.Plugin
assert OmegaConf.get_type(cfg, "missing") is None

assert not OmegaConf.is_optional(cfg, "plugin")
assert not _is_optional(cfg, "plugin")
assert _utils.get_ref_type(cfg, "plugin") == module.Plugin
assert OmegaConf.get_type(cfg, "plugin") == module.Plugin

cfg.plugin = module.ConcretePlugin()
assert not OmegaConf.is_optional(cfg, "plugin")
assert not _is_optional(cfg, "plugin")
assert _utils.get_ref_type(cfg, "plugin") == module.Plugin
assert OmegaConf.get_type(cfg, "plugin") == module.ConcretePlugin

assert not OmegaConf.is_optional(cfg, "plugin2")
assert not _is_optional(cfg, "plugin2")
assert _utils.get_ref_type(cfg, "plugin2") == module.Plugin
assert OmegaConf.get_type(cfg, "plugin2") == module.ConcretePlugin

Expand Down
3 changes: 2 additions & 1 deletion tests/test_matrix.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
ValidationError,
ValueNode,
)
from omegaconf._utils import _is_optional
from tests import Color, Group

SKIP = object()
Expand Down Expand Up @@ -47,7 +48,7 @@ def verify(
assert OmegaConf.is_missing(cfg, key) == missing
with warns(UserWarning):
assert OmegaConf.is_none(cfg, key) == none_public
assert OmegaConf.is_optional(cfg, key) == opt
assert _is_optional(cfg, key) == opt
assert OmegaConf.is_interpolation(cfg, key) == inter


Expand Down
72 changes: 4 additions & 68 deletions tests/test_omegaconf.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import re
from typing import Any

from pytest import mark, param, raises, warns
Expand Down Expand Up @@ -215,74 +216,9 @@ def test_is_dict(cfg: Any, expected: bool) -> None:
assert OmegaConf.is_dict(cfg) == expected


@mark.parametrize("is_optional", [True, False])
@mark.parametrize(
"fac",
[
(
lambda is_optional, missing: StringNode(
value="foo" if not missing else "???", is_optional=is_optional
)
),
(
lambda is_optional, missing: IntegerNode(
value=10 if not missing else "???", is_optional=is_optional
)
),
(
lambda is_optional, missing: FloatNode(
value=10 if not missing else "???", is_optional=is_optional
)
),
(
lambda is_optional, missing: BooleanNode(
value=True if not missing else "???", is_optional=is_optional
)
),
(
lambda is_optional, missing: EnumNode(
enum_type=Color,
value=Color.RED if not missing else "???",
is_optional=is_optional,
)
),
(
lambda is_optional, missing: ListConfig(
content=[1, 2, 3] if not missing else "???", is_optional=is_optional
)
),
(
lambda is_optional, missing: DictConfig(
content={"foo": "bar"} if not missing else "???",
is_optional=is_optional,
)
),
(
lambda is_optional, missing: DictConfig(
ref_type=ConcretePlugin,
content=ConcretePlugin() if not missing else "???",
is_optional=is_optional,
)
),
],
)
def test_is_optional(fac: Any, is_optional: bool) -> None:
obj = fac(is_optional, False)
assert OmegaConf.is_optional(obj) == is_optional

cfg = OmegaConf.create({"node": obj})
assert OmegaConf.is_optional(cfg, "node") == is_optional

obj = fac(is_optional, True)
assert OmegaConf.is_optional(obj) == is_optional

cfg = OmegaConf.create({"node": obj})
assert OmegaConf.is_optional(cfg, "node") == is_optional


def test_is_optional_non_node() -> None:
assert OmegaConf.is_optional("not_a_node")
assert OmegaConf.is_optional("???")
def test_is_optional_deprecation() -> None:
with warns(UserWarning, match=re.escape("`OmegaConf.is_optional()` is deprecated")):
OmegaConf.is_optional("xxx")


@mark.parametrize("is_none", [True, False])
Expand Down
71 changes: 71 additions & 0 deletions tests/test_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
Marker,
_ensure_container,
_get_value,
_is_optional,
is_dict_annotation,
is_list_annotation,
nullcontext,
Expand Down Expand Up @@ -647,3 +648,73 @@ def test_nullcontext() -> None:
obj = object()
with nullcontext(obj) as x:
assert x is obj


@mark.parametrize("is_optional", [True, False])
@mark.parametrize(
"fac",
[
(
lambda is_optional, missing: StringNode(
value="foo" if not missing else "???", is_optional=is_optional
)
),
(
lambda is_optional, missing: IntegerNode(
value=10 if not missing else "???", is_optional=is_optional
)
),
(
lambda is_optional, missing: FloatNode(
value=10 if not missing else "???", is_optional=is_optional
)
),
(
lambda is_optional, missing: BooleanNode(
value=True if not missing else "???", is_optional=is_optional
)
),
(
lambda is_optional, missing: EnumNode(
enum_type=Color,
value=Color.RED if not missing else "???",
is_optional=is_optional,
)
),
(
lambda is_optional, missing: ListConfig(
content=[1, 2, 3] if not missing else "???", is_optional=is_optional
)
),
(
lambda is_optional, missing: DictConfig(
content={"foo": "bar"} if not missing else "???",
is_optional=is_optional,
)
),
(
lambda is_optional, missing: DictConfig(
ref_type=ConcretePlugin,
content=ConcretePlugin() if not missing else "???",
is_optional=is_optional,
)
),
],
)
def test_is_optional(fac: Any, is_optional: bool) -> None:
obj = fac(is_optional, False)
assert _is_optional(obj) == is_optional

cfg = OmegaConf.create({"node": obj})
assert _is_optional(cfg, "node") == is_optional

obj = fac(is_optional, True)
assert _is_optional(obj) == is_optional

cfg = OmegaConf.create({"node": obj})
assert _is_optional(cfg, "node") == is_optional


def test_is_optional_non_node() -> None:
assert _is_optional("not_a_node")
assert _is_optional("???")