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

DictConfig is not behaving like typing.MutableMapping #869

Closed
MattiasDC opened this issue Feb 28, 2022 · 2 comments
Closed

DictConfig is not behaving like typing.MutableMapping #869

MattiasDC opened this issue Feb 28, 2022 · 2 comments
Labels
bug Something isn't working

Comments

@MattiasDC
Copy link

MattiasDC commented Feb 28, 2022

DictConfig is not behaving like typing.MutableMapping
The DictConfig methods I'm talking about are: keys and __getitem__. Both of these methods are part of the typing.Mapping specification from which DictConfig indirectly inherits (via typing.MutableMapping). The keys method however gives back keys, which cannot be retrieved by using __getitem__(self, key) (or equivalently [key] accessor). This is true for DictConfig which have missing values ('???'). I want to understand why the keys method gives back keys which have missing values. Is this unintended or by design? Furthermore it should be noted that this gives an incompatibility with tensorflow, which I will discuss in the reproduction scenario.

To Reproduce

import omegaconf as oc

d = oc.OmegaConf.create({"a" : "???"})
assert "a" in d.keys # no issue
d["a"]

"""
d["a"] statement throws:
lib/python3.8/site-packages/omegaconf/basecontainer.py", line 63, in _resolve_with_default
    raise MissingMandatoryValue("Missing mandatory value: $FULL_KEY")
omegaconf.errors.MissingMandatoryValue: Missing mandatory value: a
    full_key: a
    object_type=dict
"""
import tensorflow as tf
import omegaconf as oc

d = oc.OmegaConf.create({"a" : "???"})
tf.nest.flatten(d)
"""
tf.nest.flatten throws:

lib/python3.8/site-packages/tensorflow/python/util/nest.py", line 417, in flatten
    return _pywrap_utils.Flatten(structure, expand_composites)
RuntimeError: Mapping was modified during iteration over it
"""

The error is thrown because tensorflow first does a 'd.keys()' call and then loops over the items while accessing the values using 'd[key]', which subsequently throws the omegaconf error.

Note that tf.nest.flatten is executed everytime an attribute is set on a keras model, as can be shown below:

import tensorflow as tf
import omegaconf as oc

class DummyModel(tf.keras.Model):
    def __init__(self):
        super().__init__()
        self.config = oc.OmegaConf.create({"a" : "???"})

DummyModel()
"""
The assignment of self.id throws:
lib/python3.8/site-packages/tensorflow/python/util/nest.py", line 417, in flatten
    return _pywrap_utils.Flatten(structure, expand_composites)
RuntimeError: Mapping was modified during iteration over it
"""

This is because tf.keras.model overrides the __setattr__ method and executes tf.nest.flatten in it, as can be seen here:https://github.com/tensorflow/tensorflow/blob/master/tensorflow/python/keras/engine/training.py#:~:text=for%20v%20in-,nest.flatten(value),-)%3A
Because the flatten function thinks the DictConfig is a proper Mapping, it tries to flatten it accordingly and runs into problems because of the missing value.

Expected behavior
DictConfig.keys doesn't return keys which have a missing value as value.

Additional context

  • OmegaConf version: 2.1.1
  • Python version: 3.9
  • Operating system: Linux
  • Tensorflow version: 2.6.0
@MattiasDC MattiasDC added the bug Something isn't working label Feb 28, 2022
@Jasha10
Copy link
Collaborator

Jasha10 commented Mar 1, 2022

Hi @MattiasDC,
Throwing omegaconf.errors.MissingMandatoryValue is by design.

In essence, OmegaConf's missing values are sentinel placeholder values; they should be taken care of before the config is passed to application logic. The intention is that OmegaConf objects containing missing values are to be regarded as incompletely-specified, and attempts to access a missing value should cause fast failure.

The OmegaConf v2.2 release will include an OmegaConf.missing_keys function that can be used to recursively traverse an OmegaConf object to ensure that no missing keys are present. Using OmegaConf v2.1, you can call OmegaConf.to_container(..., throw_on_missing=True) or OmegaConf.to_object(...) to ensure that no missing values are present in the OmegaConf object.

I don't think changing the MissingMandatoryValue behavior here is possible because it would be a big breaking change.

@Jasha10 Jasha10 closed this as completed Mar 1, 2022
@MattiasDC
Copy link
Author

Hi @Jasha10,

I was indeed leaning towards the idea that it was by design. It was indeed a misconfiguration that led to this error of tensorflow. I however had a though time debugging this, looking for the reason for this tensorflow error.

I hope that if someone also experiences this behavior with tensorflow, someone may in the future bump on this closed issue and see the reason/find the fix. :)

Thanks for adding the possibilities to resolve this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants