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

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

Open
4 tasks done
kaimo455 opened this issue Aug 24, 2023 · 5 comments · May be fixed by #1117
Open
4 tasks done

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

kaimo455 opened this issue Aug 24, 2023 · 5 comments · May be fixed by #1117
Labels
bug Something isn't working

Comments

@kaimo455
Copy link

kaimo455 commented Aug 24, 2023 •
edited
Loading

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

  • OmegaConf version: 2.3.0
  • Python version: 3.9
  • Operating system: Win10
  • Please provide a minimal repro

Direct Solution
I create a pull request for that fix, please see #1117
@kaimo455 kaimo455 added the bug Something isn't working label Aug 24, 2023
@kaimo455 kaimo455 linked a pull request Aug 24, 2023 that will close this issue
Open
@kaimo455
Copy link
Author

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
Copy link
Collaborator

Jasha10 commented Aug 26, 2023 •
edited
Loading

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 <module>
    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
Copy link
Collaborator

Jasha10 commented Aug 26, 2023

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
Copy link
Owner

omry commented Sep 3, 2023

@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
Copy link
Collaborator

odelalleau commented Sep 3, 2023 •
edited
Loading

@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.

@Jasha10 Jasha10 linked a pull request Sep 4, 2023 that will close this issue
[Bug Fix] OmegaConf.missing_keys(cfg) may fail if contained customi… #1117
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Bug Fix] OmegaConf.missing_keys(cfg) may fail if contained customi… kaimo455/omegaconf
4 participants
@omry @odelalleau @Jasha10 @kaimo455