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

[Feature Request] resolve individual resolver arguments #100

Closed
YannDubs opened this issue Dec 23, 2019 · 22 comments · Fixed by #308 or #445
Closed

[Feature Request] resolve individual resolver arguments #100

YannDubs opened this issue Dec 23, 2019 · 22 comments · Fixed by #308 or #445
Labels
enhancement New feature or request
Milestone

Comments

@YannDubs
Copy link

YannDubs commented Dec 23, 2019

It would be nice to enable recursive interpolation.

Example :

import omegaconf
from omegaconf import OmegaConf

OmegaConf.register_resolver("plus", lambda x,y: float(x)+float(y))

c = OmegaConf.create({
        "i": 1,
        'key1': '${plus:1,2}',
        'key2': '${plus:${i},2}'
 })
print(c.key1)
# 3.0
print(c.key2)
# '${plus:1,2}'

But one would hope to get 3.0 out

Note that 2 recursion would be enough to enable the user to create deeper interpolation if they need it (by calling an other key which had already been interpolated).

@omry
Copy link
Owner

omry commented Dec 24, 2019

Interesting, do you have a concrete use case where this would be useful?

@YannDubs
Copy link
Author

YannDubs commented Dec 24, 2019

For example when I run a test a machine learning model using hydra I usually increment the seed k times to get confidence intervals on my results. To do so I have seed=123 in the configuration and run=k. Then at the beginning of the main function I do args.seed += args.run. When running from the CLI I simply do python main.py run=1,2,3,4 [...] -m.

This would be totally fine if I had only one value to update but there are multiple values I update at the begining of the main function. I thus call a update_args_ function and do all the updates there. But it would be much cleaner to do these simple updates directly in the config files. I.e. to use something like :

run: 0
seed: ${plus:123,${run}}

And in case I want to run a specific seed I can still use python main.py seed=1234. Note that the seed example kind of breaks here because I would never do seed cherry picking.

@omry
Copy link
Owner

omry commented Dec 24, 2019

Gotcha.
You could also directly change the seed for this case:

python main.py seed=1,2,3,4 [...] -m

I will consider supporting it though.

I don't want to push it too far (for example, nesting resolvers is definitely out of scope : foo: ${tolower:${env:USER}}, given that without a proper parser it will become very difficult to implement correctly, consider this for example:${add:${add{1,2},3}, without a parse tree this is a serious pain to get right.

@YannDubs
Copy link
Author

YannDubs commented Dec 24, 2019

Here is an other example which is more complex:

I'm running K-shot (few) learning tasks. So I can run MNIST with 10 examples or 100 examples. One way of doing it with hydra would be to have a config file for all K (MNIST10,MNIST100). This is neither scalable nor clean. So what I end up doing is having a single config file for MNIST and can change K from the command line.

This all sounds good until you know that usually the K in K-shot learning is a multiple of the number of classes. In MNIST there are 10 labels, meaning that I would usually do K=10,50,100,1000. But what happens when I have mutliple datasets (e.g. CIFAR100 and MNIST with 100 and 10 labels respectively). What I'm currently doing is :

#MNIST config
n_labels: 10

#CIFAR100 config
n_labels: 100

# General Config:
k: null
factor: 1
# update_args_
if args.k is not None:
    args.k = args.factor * args.dataset.n_labels

Then from the CLI I can either fix K directly or change factor. Again it would be cleaner to have k:${mult:${factor},${dataset.n_labels}}

@YannDubs
Copy link
Author

I don't want to push it too far (for example, nesting resolvers is definitely out of scope : foo: ${tolower:${env:USER}}, given that without a proper parser it will become very difficult to implement correctly, consider this for example:${add:${add{1,2},3}, without a parse tree this is a serious pain to get right.

I completely agree, that's why I was saying that allowing depth 2 without nesting resolvers is, I think, already enough to allow the user to do whatever they like (by definining temporary variables if they really need to, although I don't even think this should be done).

@omry
Copy link
Owner

omry commented Dec 24, 2019

There is another feature under consideration (#91) that will also help with this use case.

@omry omry changed the title [Feature Request] Recursive interpolation. [Feature Request] resolve indivudual resolver arguments Dec 24, 2019
@YannDubs
Copy link
Author

Yes the other one would cover all the usecases I actually have. Although this one is more general (but harder to read).

@omry
Copy link
Owner

omry commented Dec 24, 2019

Feel free to send a PR attempting this (with tests).
now that #87 almost ready, it will be a while before a new version of OmegaConf can be used in Hydra. #87 will be OmegaConf 2.0 and will most likely break compatibility in all sorts of ways (most obvious is that it drops Python 2.7 support, but there will be additional incompatible changes).

@omry omry changed the title [Feature Request] resolve indivudual resolver arguments [Feature Request] resolve individual resolver arguments Dec 28, 2019
@omry omry added this to the OmegaConf 2.1 milestone Mar 6, 2020
@omry omry added the enhancement New feature or request label Mar 24, 2020
@omry omry modified the milestones: OmegaConf 2.1, OmegaConf 2.0 Apr 22, 2020
@omry
Copy link
Owner

omry commented Apr 22, 2020

conditionally for 2.0 if @apsdehal implements on time.

@omry omry modified the milestones: OmegaConf 2.0, OmegaConf 2.1 May 4, 2020
@ScottAlexanderCameron
Copy link

Hi,
I have been having similar obstacles in some projects I have been working on which use hydra, and I have found a temporary workaround. It's not perfect, but I thought I would post it here anyway. The idea is to register a resolver which has a reference to the root container and manually fetch the variables which should be interpolated. For this to work, the resolver must be registered only after the container is created.

from functools import reduce
import numpy as np
from omegaconf import OmegaConf

cfg = OmegaConf.create(
    """
    env:
        obs_shape: [1, 2, 3]
    obs_flat: '${flatten:env.obs_shape}'
    """
)

OmegaConf.register_resolver(
    "flatten",
    lambda arg: np.prod(
        reduce(lambda container, name: container[name], arg.split("."), cfg)
    ),
)

print(OmegaConf.to_container(cfg, resolve=True,))
# {'env': {'obs_shape': [1, 2, 3]}, 'obs_flat': 6}

This can obviously be extended if you want to allow literals as well as variable names, or to call other resolvers, etc.

@omry
Copy link
Owner

omry commented May 19, 2020

Nice.
I am not what you are doing with split(".") there, but you might be looking to use OmegaConf.select() (I think it was cfg.select() in 1.4.1).

@ScottAlexanderCameron
Copy link

Thanks 🙂 👍 , I didn't see that before

@odelalleau
Copy link
Collaborator

I don't want to push it too far (for example, nesting resolvers is definitely out of scope : foo: ${tolower:${env:USER}}, given that without a proper parser it will become very difficult to implement correctly, consider this for example:${add:${add{1,2},3}, without a parse tree this is a serious pain to get right.

I'm not sure it's that difficult given how simple the grammar is (at least if we don't aim for maximal speed). I actually played around a bit with it and feel like I may have something worth a shot. I'll try and put up a draft PR for you to check later today.

@omry
Copy link
Owner

omry commented Jul 16, 2020

Awesome.
Now that I added a proper grammar to Hydra in master, I will likely do the same for OmegaConf in the next major version which can simplify this.
In the mean time I am open to a PR addressing this.

@charleswilmot
Copy link

I noticed an issue with the nested interpolation. When calling

OmegaConf.to_yaml(cfg, resolve=True)

The nested interpolation fields are surrounded with " ' ", thus when latter reloaded, the resulting value shows as a str. This might be problematic sometimes.

@omry
Copy link
Owner

omry commented Aug 12, 2020

@charleswilmot , open a new issue. include your OmegaConf version and a minimal repo, along with what you are expecting to see.

@odelalleau
Copy link
Collaborator

@omry could you please re-open this issue? (following #328)

@omry
Copy link
Owner

omry commented Aug 15, 2020

Yes, forgot about it.
To anyone following:
This has been reverted and will be re-introduced with OmegaConf 2.1 using a more robust implementation.

@omry omry reopened this Aug 15, 2020
odelalleau added a commit to odelalleau/omegaconf that referenced this issue Aug 17, 2020
@odelalleau
Copy link
Collaborator

I noticed an issue with the nested interpolation. When calling

OmegaConf.to_yaml(cfg, resolve=True)

The nested interpolation fields are surrounded with " ' ", thus when latter reloaded, the resulting value shows as a str. This might be problematic sometimes.

@charleswilmot I tried to repro but failed -- the new version from #321 may have fixed it, could you please check? Thanks!

odelalleau added a commit to odelalleau/omegaconf that referenced this issue Sep 9, 2020
odelalleau added a commit to odelalleau/omegaconf that referenced this issue Sep 10, 2020
odelalleau added a commit to odelalleau/omegaconf that referenced this issue Sep 11, 2020
odelalleau added a commit to odelalleau/omegaconf that referenced this issue Sep 16, 2020
odelalleau added a commit to odelalleau/omegaconf that referenced this issue Sep 23, 2020
odelalleau added a commit to odelalleau/omegaconf that referenced this issue Oct 27, 2020
odelalleau added a commit to odelalleau/omegaconf that referenced this issue Nov 26, 2020
* Add a grammar to parse interpolations
* Add support for nested interpolations
* Deprecate register_resolver() and introduce new_register_resolver()
(that allows resolvers to use non-string arguments, and to decide
whether or not to use the cache)
* The `env` resolver now parses environment variables in a way that is
consistent with the new interpolation grammar

Fixes omry#100 and omry#318
odelalleau added a commit to odelalleau/omegaconf that referenced this issue Nov 26, 2020
* Add a grammar to parse interpolations
* Add support for nested interpolations
* Deprecate register_resolver() and introduce new_register_resolver()
(that allows resolvers to use non-string arguments, and to decide
whether or not to use the cache)
* The `env` resolver now parses environment variables in a way that is
consistent with the new interpolation grammar

Fixes omry#100 and omry#318
odelalleau added a commit to odelalleau/omegaconf that referenced this issue Nov 28, 2020
* Add a grammar to parse interpolations
* Add support for nested interpolations
* Deprecate register_resolver() and introduce new_register_resolver()
(that allows resolvers to use non-string arguments, and to decide
whether or not to use the cache)
* The `env` resolver now parses environment variables in a way that is
consistent with the new interpolation grammar

Fixes omry#100 and omry#318
odelalleau added a commit to odelalleau/omegaconf that referenced this issue Feb 5, 2021
* Add a grammar to parse interpolations
* Add support for nested interpolations
* Deprecate register_resolver() and introduce new_register_resolver()
(that allows resolvers to use non-string arguments, and to decide
whether or not to use the cache)
* The `env` resolver now parses environment variables in a way that is
consistent with the new interpolation grammar

Fixes omry#100 and omry#318
odelalleau added a commit to odelalleau/omegaconf that referenced this issue Feb 10, 2021
* Add a grammar to parse interpolations
* Add support for nested interpolations
* Deprecate register_resolver() and introduce new_register_resolver()
(that allows resolvers to use non-string arguments, and to decide
whether or not to use the cache)
* The `env` resolver now parses environment variables in a way that is
consistent with the new interpolation grammar

Fixes omry#100 and omry#318
odelalleau added a commit to odelalleau/omegaconf that referenced this issue Feb 10, 2021
* Add a grammar to parse interpolations
* Add support for nested interpolations
* Deprecate register_resolver() and introduce new_register_resolver()
(that allows resolvers to use non-string arguments, and to decide
whether or not to use the cache)
* The `env` resolver now parses environment variables in a way that is
consistent with the new interpolation grammar

Fixes omry#100 and omry#318
odelalleau added a commit to odelalleau/omegaconf that referenced this issue Feb 11, 2021
* Add a grammar to parse interpolations
* Add support for nested interpolations
* Deprecate register_resolver() and introduce new_register_resolver()
(that allows resolvers to use non-string arguments, and to decide
whether or not to use the cache)
* The `env` resolver now parses environment variables in a way that is
consistent with the new interpolation grammar

Fixes omry#100 and omry#318
@omry omry closed this as completed in #445 Feb 12, 2021
omry pushed a commit that referenced this issue Feb 12, 2021
* Add a grammar to parse interpolations
* Add support for nested interpolations
* Deprecate register_resolver() and introduce new_register_resolver() (that allows resolvers to use non-string arguments, and to decide whether or not to use the cache)
* The `env` resolver now parses environment variables in a way that is consistent with the new interpolation grammar

Fixes #100 #230 #266 #318
@jdonzallaz
Copy link

Hi, I'm having the same issue, trying to resolve an interpolation in a custom resolver.
What it the current status?

@odelalleau
Copy link
Collaborator

Hi, I'm having the same issue, trying to resolve an interpolation in a custom resolver.
What it the current status?

This should work with dev setup on the current master branch, but there is no publicly available package yet with this new feature.

@jdonzallaz
Copy link

Thank you, it works !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
7 participants