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

Allow resolvers to output a dict / list to generate a DictConfig / ListConfig #540

Closed
odelalleau opened this issue Feb 11, 2021 · 8 comments · Fixed by #578
Closed

Allow resolvers to output a dict / list to generate a DictConfig / ListConfig #540

odelalleau opened this issue Feb 11, 2021 · 8 comments · Fixed by #578
Assignees
@odelalleau
Milestone
OmegaConf 2.1

Comments

@odelalleau
Copy link
Collaborator

As discussed in facebookresearch/hydra#1389, converting a resolver's output from dict / list to DictConfig / ListConfig would make it easier to generate parts of a config.

For instance:

    OmegaConf.register_new_resolver("make_x", lambda: {"a": 0, "b": 1})
    cfg = OmegaConf.create({"x": "${make_x:}"})
    assert isinstance(cfg.x, DictConfig)
    assert cfg.x.a == 0 and cfg.x.b == 1
@odelalleau odelalleau self-assigned this Feb 11, 2021
@omry
Copy link
Owner

omry commented Feb 16, 2021

This interacts with the idea to validate interpolations to ensure their type is matching.

@odelalleau
Copy link
Collaborator Author

odelalleau commented Feb 16, 2021 •
edited
Loading

This interacts with the idea to validate interpolations to ensure their type is matching.

Yes, I actually implemented it in the same commit (IIRC)

@odelalleau odelalleau added this to the OmegaConf 2.1 milestone Feb 24, 2021
odelalleau added a commit to odelalleau/omegaconf that referenced this issue Mar 3, 2021
@odelalleau
Validate and convert interpolation results to their intended type
dc9992a 
This commit fixes several issues:

* For nested resolvers (ex: `${f:${g:x}}`), intermediate resolver
  outputs (of `g` in this example) were wrapped in a ValueNode just to
  be unwrapped immediately, which was wasteful. This commit pushes the
  node wrapping to the very last step of the interpolation resolution.

* There was no type checking to make sure that the result of an
  interpolation had a type consistent with the node's type (when
  specified). Now a check is made and the interpolation result may be
  converted into the desired type (see omry#488).

* If a resolver interpolation returns a dict / list, it is now wrapped
  into a DictConfig / ListConfig, instead of a ValueNode. This makes it
  possible to generate configs from resolvers (see omry#540)

Fixes omry#488
Fixes omry#540
@odelalleau odelalleau mentioned this issue Mar 3, 2021
Merged
odelalleau added a commit to odelalleau/omegaconf that referenced this issue Mar 3, 2021
@odelalleau
Validate and convert interpolation results to their intended type
55ed2af 
This commit fixes several issues:

* For nested resolvers (ex: `${f:${g:x}}`), intermediate resolver
  outputs (of `g` in this example) were wrapped in a ValueNode just to
  be unwrapped immediately, which was wasteful. This commit pushes the
  node wrapping to the very last step of the interpolation resolution.

* There was no type checking to make sure that the result of an
  interpolation had a type consistent with the node's type (when
  specified). Now a check is made and the interpolation result may be
  converted into the desired type (see omry#488).

* If a resolver interpolation returns a dict / list, it is now wrapped
  into a DictConfig / ListConfig, instead of a ValueNode. This makes it
  possible to generate configs from resolvers (see omry#540)

Fixes omry#488
Fixes omry#540
odelalleau added a commit to odelalleau/omegaconf that referenced this issue Mar 10, 2021
@odelalleau
Validate and convert interpolation results to their intended type
21224f2 
This commit fixes several issues:

* For nested resolvers (ex: `${f:${g:x}}`), intermediate resolver
  outputs (of `g` in this example) were wrapped in a ValueNode just to
  be unwrapped immediately, which was wasteful. This commit pushes the
  node wrapping to the very last step of the interpolation resolution.

* There was no type checking to make sure that the result of an
  interpolation had a type consistent with the node's type (when
  specified). Now a check is made and the interpolation result may be
  converted into the desired type (see omry#488).

* If a resolver interpolation returns a dict / list, it is now wrapped
  into a DictConfig / ListConfig, instead of a ValueNode. This makes it
  possible to generate configs from resolvers (see omry#540)

Fixes omry#488
Fixes omry#540
odelalleau added a commit to odelalleau/omegaconf that referenced this issue Mar 10, 2021
@odelalleau
Add news item to describe the new feature from omry#540
36167ba
@omry omry closed this as completed in #578 Mar 13, 2021
omry pushed a commit that referenced this issue Mar 13, 2021
@odelalleau
Type validation of interpolations (#578)
3bf2c59 
Validate and convert interpolation results to their intended type

This PR fixes several issues:

* For nested resolvers (ex: `${f:${g:x}}`), intermediate resolver
  outputs (of `g` in this example) were wrapped in a ValueNode just to
  be unwrapped immediately, which was wasteful. This commit pushes the
  node wrapping to the very last step of the interpolation resolution.

* There was no type checking to make sure that the result of an
  interpolation had a type consistent with the node's type (when
  specified). Now a check is made and the interpolation result may be
  converted into the desired type (see #488).

* If a resolver interpolation returns a dict / list, it is now wrapped
  into a DictConfig / ListConfig, instead of a ValueNode. This makes it
  possible to generate configs from resolvers (see #540). These configs
  are read-only since they are being re-generated on each access.


Fixes #488
Fixes #540
@omry
Copy link
Owner

omry commented May 12, 2021

We backtracked on this one, right?
We still have this in the news dir:

- Custom resolvers can now generate transient config nodes dynamically. ([#540](https://github.com/omry/omegaconf/issues/540))

@odelalleau
Copy link
Collaborator Author

Yeah, the news item isn't entirely wrong since you can generate List/DictConfigs from a custom resolver now, but not exactly as per this issue.
=> I would either remove it or update this issue (there are two ways to do it now -- either by directly creating a container whose parent is set to _parent_, or by combining with oc.create)

@omry
Copy link
Owner

omry commented May 12, 2021

we have a news item for oc.create so I am just removing this one.

@masteranza
Copy link

@omry Could you link to the news item you're mentioning? I'm hoping to find a workaround for this: facebookresearch/hydra#1389

@omry
Copy link
Owner

omry commented Aug 16, 2021

news fragments are just one-liners that are later copied into the release notes. this will not help you.

@odelalleau
Copy link
Collaborator Author

@masteranza maybe the comment I just added in that issue can help you? facebookresearch/hydra#1389 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@odelalleau odelalleau

Labels
None yet
Projects
None yet
Milestone
OmegaConf 2.1
Development

Successfully merging a pull request may close this issue.

Type validation of interpolations odelalleau/omegaconf
4 participants
@omry @odelalleau @masteranza @Jasha10