omry / omegaconf

Flexible Python configuration system. The last one you will ever need.
BSD 3-Clause "New" or "Revised" License
1.91k stars 104 forks source link

`OmegaConf.missing_keys(cfg)` may fail if contained customized resolver field that the depends on some missing fields. #1118

Open kaimo455 opened 1 year ago

kaimo455 commented 1 year ago

Describe the bug When a config contains customized resolvers which depend on some missing field(s), calling OmegaConf.missing_keys(cfg) raise an error.

To Reproduce

import omegaconf
def add(a, b):
    return a+b
omegaconf.OmegaConf.register_new_resolver('add', add)
cfg = omegaconf.OmegaConf.create({'a': '???', 'b': '???', 'c': "${add: ${a}, ${b}}"})
omegaconf.OmegaConf.missing_keys(cfg)

Expected behavior Should just return {'a', 'b'}

Additional context

Direct Solution I create a pull request for that fix, please see #1117

kaimo455 commented 1 year ago

Well, seems that even a simple interpolation field will crash, not customized resolver needed.

import omegaconf
cfg = omegaconf.OmegaConf.create({'a': '???', 'b': '${.a}'})
omegaconf.OmegaConf.missing_keys(cfg)
Jasha10 commented 1 year ago

For reference, here's the traceback produced by the example from the OP:

``` $ python tmp.py Traceback (most recent call last): File "/home/homestar/dev/omegaconf/tmp.py", line 6, in omegaconf.OmegaConf.missing_keys(cfg) File "/home/homestar/dev/omegaconf/omegaconf/omegaconf.py", line 831, in missing_keys gather(cfg) File "/home/homestar/dev/omegaconf/omegaconf/omegaconf.py", line 828, in gather elif OmegaConf.is_config(_cfg[key]): File "/home/homestar/dev/omegaconf/omegaconf/dictconfig.py", line 373, in __getitem__ self._format_and_raise(key=key, value=None, cause=e) File "/home/homestar/dev/omegaconf/omegaconf/base.py", line 230, in _format_and_raise format_and_raise( File "/home/homestar/dev/omegaconf/omegaconf/_utils.py", line 901, in format_and_raise _raise(ex, cause) File "/home/homestar/dev/omegaconf/omegaconf/_utils.py", line 799, in _raise raise ex.with_traceback(sys.exc_info()[2]) # set env var OC_CAUSE=1 for full trace File "/home/homestar/dev/omegaconf/omegaconf/dictconfig.py", line 367, in __getitem__ return self._get_impl(key=key, default_value=_DEFAULT_MARKER_) File "/home/homestar/dev/omegaconf/omegaconf/dictconfig.py", line 449, in _get_impl return self._resolve_with_default( File "/home/homestar/dev/omegaconf/omegaconf/basecontainer.py", line 99, in _resolve_with_default resolved_node = self._maybe_resolve_interpolation( File "/home/homestar/dev/omegaconf/omegaconf/base.py", line 718, in _maybe_resolve_interpolation return self._resolve_interpolation_from_parse_tree( File "/home/homestar/dev/omegaconf/omegaconf/base.py", line 583, in _resolve_interpolation_from_parse_tree resolved = self.resolve_parse_tree( File "/home/homestar/dev/omegaconf/omegaconf/base.py", line 763, in resolve_parse_tree return visitor.visit(parse_tree) File "/home/homestar/miniconda3/envs/omega310/lib/python3.10/site-packages/antlr4/tree/Tree.py", line 34, in visit return tree.accept(self) File "/home/homestar/dev/omegaconf/omegaconf/grammar/gen/OmegaConfGrammarParser.py", line 206, in accept return visitor.visitConfigValue(self) File "/home/homestar/dev/omegaconf/omegaconf/grammar_visitor.py", line 101, in visitConfigValue return self.visit(ctx.getChild(0)) File "/home/homestar/miniconda3/envs/omega310/lib/python3.10/site-packages/antlr4/tree/Tree.py", line 34, in visit return tree.accept(self) File "/home/homestar/dev/omegaconf/omegaconf/grammar/gen/OmegaConfGrammarParser.py", line 342, in accept return visitor.visitText(self) File "/home/homestar/dev/omegaconf/omegaconf/grammar_visitor.py", line 297, in visitText return self.visitInterpolation(c) File "/home/homestar/dev/omegaconf/omegaconf/grammar_visitor.py", line 125, in visitInterpolation return self.visit(ctx.getChild(0)) File "/home/homestar/miniconda3/envs/omega310/lib/python3.10/site-packages/antlr4/tree/Tree.py", line 34, in visit return tree.accept(self) File "/home/homestar/dev/omegaconf/omegaconf/grammar/gen/OmegaConfGrammarParser.py", line 1041, in accept return visitor.visitInterpolationResolver(self) File "/home/homestar/dev/omegaconf/omegaconf/grammar_visitor.py", line 174, in visitInterpolationResolver for val, txt in self.visitSequence(maybe_seq): File "/home/homestar/dev/omegaconf/omegaconf/grammar_visitor.py", line 266, in visitSequence yield _get_value(self.visitElement(child)), child.getText() File "/home/homestar/dev/omegaconf/omegaconf/grammar_visitor.py", line 119, in visitElement return self.visit(ctx.getChild(0)) File "/home/homestar/miniconda3/envs/omega310/lib/python3.10/site-packages/antlr4/tree/Tree.py", line 34, in visit return tree.accept(self) File "/home/homestar/dev/omegaconf/omegaconf/grammar/gen/OmegaConfGrammarParser.py", line 1406, in accept return visitor.visitPrimitive(self) File "/home/homestar/dev/omegaconf/omegaconf/grammar_visitor.py", line 211, in visitPrimitive return self._createPrimitive(ctx) File "/home/homestar/dev/omegaconf/omegaconf/grammar_visitor.py", line 313, in _createPrimitive return self.visitInterpolation(child) File "/home/homestar/dev/omegaconf/omegaconf/grammar_visitor.py", line 125, in visitInterpolation return self.visit(ctx.getChild(0)) File "/home/homestar/miniconda3/envs/omega310/lib/python3.10/site-packages/antlr4/tree/Tree.py", line 34, in visit return tree.accept(self) File "/home/homestar/dev/omegaconf/omegaconf/grammar/gen/OmegaConfGrammarParser.py", line 921, in accept return visitor.visitInterpolationNode(self) File "/home/homestar/dev/omegaconf/omegaconf/grammar_visitor.py", line 158, in visitInterpolationNode return self.node_interpolation_callback(inter_key, self.memo) File "/home/homestar/dev/omegaconf/omegaconf/base.py", line 744, in node_interpolation_callback return self._resolve_node_interpolation(inter_key=inter_key, memo=memo) File "/home/homestar/dev/omegaconf/omegaconf/base.py", line 670, in _resolve_node_interpolation raise InterpolationToMissingValueError( File "/home/homestar/dev/omegaconf/omegaconf/base.py", line 663, in _resolve_node_interpolation parent, last_key, value = root_node._select_impl( File "/home/homestar/dev/omegaconf/omegaconf/base.py", line 513, in _select_impl value, _ = _select_one( File "/home/homestar/dev/omegaconf/omegaconf/omegaconf.py", line 1168, in _select_one raise MissingMandatoryValue( omegaconf.errors.InterpolationToMissingValueError: MissingMandatoryValue while resolving interpolation: Missing mandatory value: a full_key: c object_type=dict ```

It looks like invoking OmegaConf.missing_keys causes 'c' to be dereferenced.

Jasha10 commented 1 year ago

Hi @kaimo455, thanks for taking the time to file a bug report and open a PR.

I see that your PR #1117 works around this crash by modifying the OmegaConf.missing_keys function to skip keys that are interpolations. This seems like a reasonable approach to me.

omry commented 11 months ago

@odelalleau, thoughts? I feel like interpolation should be evaluated and True should be returned they resolve to a missing key. For custom resolvers, I think the behavior should be True if they are dereferencing a missing key (which I think triggers a MissingMandatoryValue somewhere).

odelalleau commented 11 months ago

@odelalleau, thoughts? I feel like interpolation should be evaluated and True should be returned they resolve to a missing key. For custom resolvers, I think the behavior should be True if they are dereferencing a missing key (which I think triggers a MissingMandatoryValue somewhere).

1117 looks good to me. Back then we settled on interpolations never being considered as missing (so for instance for the config given as example in this issue, OmegaConf.is_missing(cfg, c) returns False). And trying to access cfg.c raises a InterpolationToMissingValueError, not a MissingMandatoryValue.

I don't remember the details, but I believe that there were potential issues in "propagating" missing values through interpolations / resolvers.