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/ListConfig.get/pop() should not fall back to default value on interpolation errors #565

Closed
odelalleau opened this issue Feb 24, 2021 · 0 comments · Fixed by #545
Assignees
Milestone

Comments

@odelalleau
Copy link
Collaborator

odelalleau commented Feb 24, 2021

Is your feature request related to a problem? Please describe.
Currently, calling cfg.get(key, default_value) or cfg.pop(key, default_value) where cfg is either a DictConfig or a ListConfig will return default_value if key is an interpolation whose resolution fails (e.g., to a non-existing node, or a non-existing custom resolver). This can be dangerous as it may hide errors.

Describe the solution you'd like
If key exists and is an interpolation, get() and pop() should either return the resolved value (if possible), or raise an exception (if the interpolation can't be resolved).

In addition, OmegaConf.select() should be modified to be consistent with this new behavior, i.e., calling OmegaConf.select(cfg, key, default=val) should not return the default value if key is an interpolation to a non-existing node.

Additional context
Fixed in #545. Creating this issue to keep track of changes.

@odelalleau odelalleau self-assigned this Feb 24, 2021
@odelalleau odelalleau added this to the OmegaConf 2.1 milestone Feb 24, 2021
@omry omry closed this as completed in #545 Mar 3, 2021
omry pushed a commit that referenced this issue Mar 3, 2021
* Interpolations are never considered to be missing anymore, even if
  they point to a missing node

* When resolving an expression containing an interpolation pointing to a
  missing node, an `InterpolationToMissingValueError` exception is raised

* When resolving an expression containing an interpolation pointing to a
  node that does not exist, an `InterpolationKeyError` exception is raised

* `key in cfg` returns True whenever `key` is an interpolation, even if it cannot be resolved (including interpolations to missing nodes)

* `get()` and `pop()` no longer return the default value in case of interpolation resolution failure (same thing for `OmegaConf.select()`)

* If `throw_on_resolution_failure` is False, then resolving an
  interpolation resulting in a resolution failure always leads to the
  result being `None` (instead of potentially being an expression computed
  from `None`)

Fixes #543
Fixes #561
Fixes #562
Fixes #565
odelalleau added a commit to odelalleau/omegaconf that referenced this issue Mar 10, 2021
* Interpolations are never considered to be missing anymore, even if
  they point to a missing node

* When resolving an expression containing an interpolation pointing to a
  missing node, an `InterpolationToMissingValueError` exception is raised

* When resolving an expression containing an interpolation pointing to a
  node that does not exist, an `InterpolationKeyError` exception is raised

* `key in cfg` returns True whenever `key` is an interpolation, even if it cannot be resolved (including interpolations to missing nodes)

* `get()` and `pop()` no longer return the default value in case of interpolation resolution failure (same thing for `OmegaConf.select()`)

* If `throw_on_resolution_failure` is False, then resolving an
  interpolation resulting in a resolution failure always leads to the
  result being `None` (instead of potentially being an expression computed
  from `None`)

Fixes omry#543
Fixes omry#561
Fixes omry#562
Fixes omry#565
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.

1 participant