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.

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

Deprecate OmegaConf.is_none() #547

Closed
odelalleau opened this issue Feb 17, 2021 · 1 comment · Fixed by #595
Closed

Deprecate OmegaConf.is_none() #547

odelalleau opened this issue Feb 17, 2021 · 1 comment · Fixed by #595
Assignees
Milestone

Comments

@odelalleau
Copy link
Collaborator

odelalleau commented Feb 17, 2021

PROBLEM

With the change to OmegaConf.is_missing() in #545, the current behavior of OmegaConf.is_none() in OmegaConf 2.0 can be confusing (because it resolves interpolations, while OmegaConf.is_missing() doesn't).

In addition, OmegaConf.is_none() has potentially non-intuitive behavior in situations where the key can't be resolved (see below for details).

SOLUTION
OmegaConf.is_none() is being deprecated in OmegaConf 2.1, and will be removed in OmegaConf 2.2

MIGRATION INSTRUCTIONS

Please do not hesitate to open an issue if you run into problems that the instructions below do not address.

Scenario 1 -- Your code is checking a specific key:

OmegaConf.is_none(cfg, "foo")
OmegaConf.is_none(cfg, key)

It is recommended to change these to:

cfg.foo is None
cfg[key] is None

Note however that when using this new syntax, an exception may be raised when accessing the key, while OmegaConf.is_none() used to silence such exceptions. If you want to keep the exact (but possibly unintuitive) behavior from before, you can implement your own is_none() function as follows:

def is_none(obj: Container, key: Union[int, DictKeyType]) -> bool:
    try:
        return obj[key] is None
    except ConfigKeyError:
        return True  # `True` if the key does not exist in the config
    except (InterpolationResolutionError, MissingMandatoryValue):
        return False  # `False` if it is either missing ("???") or an invalid interpolation

Scenario 2 -- Your code is not providing a key:

OmegaConf.is_none(obj)

There is currently no plan to keep supporting this usage in OmegaConf 2.2. If you need it, please open an issue explaining why.

Additional context
See discussion in #545 (comment)

Dev note: at the same time, the internal Node._is_none() can be modified to not resolve interpolations.

@Jasha10
Copy link
Collaborator

Jasha10 commented Feb 19, 2021

=> let's deprecate OmegaConf.is_none(), instructing users to use cfg.foo is None instead.

I like this -- cfg.foo is None is more pythonic than OmegaConf.is_none(cfg, "foo").

@omry omry added this to the OmegaConf 2.1 milestone Feb 19, 2021
odelalleau added a commit to odelalleau/omegaconf that referenced this issue Mar 10, 2021
odelalleau added a commit to odelalleau/omegaconf that referenced this issue Mar 10, 2021
@omry omry closed this as completed in #595 Mar 13, 2021
pixelb added a commit to pixelb/omegaconf that referenced this issue May 12, 2022
Was slated for removal in 2.2

The deprecation was handled in issue omry#547
This addresses issue omry#821
pixelb added a commit to pixelb/omegaconf that referenced this issue May 12, 2022
Was slated for removal in 2.2

The deprecation was handled in issue omry#547
This addresses issue omry#821
pixelb added a commit to pixelb/omegaconf that referenced this issue May 12, 2022
Was slated for removal in 2.2

The deprecation was handled in issue omry#547
This addresses issue omry#821
pixelb added a commit to pixelb/omegaconf that referenced this issue May 12, 2022
Was slated for removal in 2.2

The deprecation was handled in issue omry#547
This addresses issue omry#821
pixelb added a commit that referenced this issue May 13, 2022
Was slated for removal in 2.2

The deprecation was handled in issue #547
This addresses issue #821
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants