Major changes to interpolations and resolvers

[odelalleau]

This is a squashed version of #321.

• Add a grammar to parse interpolations
• Add support for nested interpolations
• Deprecate register_resolver() and introduce register_new_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

[lgtm-com]

This pull request introduces 1 alert when merging ddb57bb into 800e8a7 - view on LGTM.com

new alerts:

• 1 for Unused import

[odelalleau]

This pull request introduces 1 alert when merging ddb57bb into 800e8a7 - view on LGTM.com

new alerts:

• 1 for Unused import

Just a note that I fixed that in a force push (it was making flake8 tests fail as well) -- this was caused by my rebase on top of master

[omry]

looks like you have conflicts with master. can you fix?

[omry]

Alright, made a review pass.
lots of comments and questions inline. overall looks good.
(Must say this was easier to review now, in part because of the previous review passes in the other PR but also due to the fact that reviewing it as one unit is easier.

[omry]

Can you go over why we think (sometimes) parsing the environment variables as opposed to always passing them as is in a string is the the best choice?

[odelalleau]

Until yesterday I thought that otherwise there would no way to write something like:

learning_rate: ${env:LR}

and have learning_rate be a float.

That being said, I just realized that {Integer,Float,Boolean}Node actually convert from string, so maybe this would work with structured configs?
(although I'm not entirely sure why this is allowed in the first place, my first instinct would be that assigning a string to an int/float/bool node should raise an exception)

[omry]

Well, you can check and see for yourself :).
I think the answer is no, because the result if the interpolation is returned directly and is not assigned to the config.

Converting on assignment was introduced really early and may have been a mistake.
The motivation is to support converting during merge: in particular merging from cli, where everything is a string.

We should consider deprecating support for assignments that mypy would frowns upon if it knew the type.

@dataclass
class Foo:
  x: int = 10

cfg: Foo = OmegaConf.structured(Foo)
cfg.x = "20"  # mypy would bitch here even though it's legal in OmegaConf.

[odelalleau]

We should consider deprecating support for assignments that mypy would frowns upon if it knew the type.

I created #459 to keep track of this

[omry]

Until yesterday I thought that otherwise there would no way to write something like:

learning_rate: ${env:LR}

and have learning_rate be a float.

Issue 488 is a recent report about this.
I think we should consider validating the return value of custom interpolations against the specified type.
This is a breaking change, so switching to new resolvers is a good opportunity (with a potential opt-out flag - maybe).
As a POC, I gave it a shot in #506 (based on the current interpolation logic). It adds validation for nodes that are annotated (ValueNodes only, and only if they are not AnyNode).

[odelalleau]

Just a note that this actually doesn't work, I'll look into fixing it. Just to be sure, do we want the following to work:

@dataclass
class Cfg:
    s: str = "7"
    i: int = II("s")

?
(the question is, since s can be cast to int, is it ok to do it implicitly here as well?)

[odelalleau]

As discussed, I reverted the changes that were (attempting at) addressing #488. I will do that in a follow-up PR instead, to avoid adding too much stuff here (since it's already way too big).

As a result, I also believe that it's best to also revisit the behavior of the env resolver in another follow-up PR, so that when env is changed to only return strings, then at least typed nodes keep working as expected (even without a new decode resolver).

@omry if you're ok with that plan then this conversation may be resolved.

[omry]

resolving.

[omry]

Just to clarify:
There are multiple things here that should probably be changed.

1. Support for type annotation for resolvers (with runtime validation).
2. Env resolver should no longer parse input environment variables.
3. Potentially new built in resolver oc.decode() that can parse an input with the grammar parser.

488 is about 1.
2 and 3 are related.

[odelalleau]

I agree, they will be addressed in that order in future PRs

[omry]

I was expecting to see some parser specific unit tests (testing the parser directly without going through the higher layers).
Did I miss it?

[odelalleau]

No, I didn't realize it was possible until later, at which point I had already written all tests this way.
Do you want me to refactor them?

[omry]

I missed this one.
Yes - I think having lower level tests will make the tests more robust and easier to work with.
Once you have lower level tests, this entire function can probably be removed.

[odelalleau]

I refactored these tests in b520815

Essentially I split the big list into 3:

• Stuff that can be parsed without interpolations (to test the basic constructs of the grammar, like int, float, str, dict, etc.). Those can be parsed without using any config object.
• Expressions containing interpolations for the parser rule singleElement (this is the biggest chunk)
• Finally, expressions for the highest-level parser rule configValue

In the end it still looks pretty similar overall, but at least it's faster and avoids using dummy resolvers everywhere just to test the correct parser rule. Let me know if that looks ok to you.

NB: in a first attempt I tried to go even lower in the rules (e.g., using the dictContainer parser rule for dicts), but the problem is that without an EOF in the parser rule, ANTLR fails to detect some errors (ex: {0: 1}} is not supposed to be a valid dict). That's why I stuck to the singleElement and configValue rules (that contain the EOF).

[odelalleau]

Regarding #448, currently it doesn't work because @ is not a valid character in IDs. I can easily fix it but I'd like to clarify a few things first:

1. @ should be only allowed in key names, not resolver names, correct?
2. @ should be allowed at any position in the key name, correct?
3. Is there any plan to support @ in keys in Hydra's override grammar? Currently this seems to conflict with the group@pkg syntax

[omry]

Regarding #448, currently it doesn't work because @ is not a valid character in IDs. I can easily fix it but I'd like to clarify a few things first:

1. @ should be only allowed in key names, not resolver names, correct?
2. @ should be allowed at any position in the key name, correct?
3. Is there any plan to support @ in keys in Hydra's override grammar? Currently this seems to conflict with the group@pkg syntax

This is an extension of group@pkg, allowing an empty package in Hydra.
It's not landed yet, but you can see it here.

group and group@pkg are now going to be supported as interpolation keys in the defaults list (this is non-standard interpolation support). to enable that OmegaConf need to accept @ as a legal character in config keys.
This can also be useful for things like emails. from the perspective of OmegaConf it's just another valid character.
Let me know if you have any more questions.

1. Yes
2. Yes
3. No conflict, this is the same, just allowing for an empty package.

[odelalleau]

1. Yes
2. Yes
3. No conflict, this is the same, just allowing for an empty package.

Ok thanks, I added this in 3af2f85 (edited: initially it was d959a13 but I realized I could make the grammar more readable) (also added another commit, af3870d, to add a missing test for @ character in unquoted strings, which was already supported)

Regarding Hydra, I tried synching facebookresearch/hydra#1170 with a config.yaml looking like:

foo:
  x@y: youpla

and run the command line

python my_app.py foo.x@y=boom

and Hydra complains with Override foo.x@y=boom looks like a config group override, but config group 'foo.x' does not exist.
From what you said I suspect this is behaving as expected, but I still thought I'd mention it just in case.

[omry]

Commented in one of the commits, this seems a bit more complicated than I would have wanted.

About Hydra:
Yes, this is the expected behavior. you can't override keys with @ in them from the command line because the syntax is reserved. if it becomes a real problem we can support escaping of the @.

#1170 is still work in progress, the new logic is not integrated there yet.
Interpolation in the defaults list will be special. You can see a bit about it here.

Here is a concrete test that has an interpolation with @ in the key.

[odelalleau]

Commented in one of the commits, this seems a bit more complicated than I would have wanted.

Link to the discussion for reference: d959a13#r44887061

Yes, this is the expected behavior. you can't override keys with @ in them from the command line because the syntax is reserved. if it becomes a real problem we can support escaping of the @.

Ok, just wanted to be sure!

Here is a concrete test that has an interpolation with @ in the key.

I see, thanks!

[odelalleau]

I did a force push to replace my old commit adding support for @ by another one (9e1bc41) that adds support for almost any character, as discussed in 200fef2#r44900426 (compared to this comment, I also added \ to the list of forbidden characters, in case we might want later to add some kind of escaping).

One open question is whether we really want to allow more varied dictionary keys: see discussion at 200fef2#commitcomment-44939891

[omry]

Recent batch of changes looks good. I still need to have a second pass on your responses to everything else.

[omry]

Partial pass. Got about a million lines left to review :)

[omry]

Got most of it, but ran out of energy for now.

[omry]

Until yesterday I thought that otherwise there would no way to write something like:

learning_rate: ${env:LR}

and have learning_rate be a float.

Issue 488 is a recent report about this.
I think we should consider validating the return value of custom interpolations against the specified type.
This is a breaking change, so switching to new resolvers is a good opportunity (with a potential opt-out flag - maybe).
As a POC, I gave it a shot in #506 (based on the current interpolation logic). It adds validation for nodes that are annotated (ValueNodes only, and only if they are not AnyNode).

[omry]

This is also closing #266, right?

[omry]

Your fixed string:

Fixes #100 #230 #266 #318

Only closed 100.
I think it works with a comma.

[odelalleau]

Your fixed string:

Fixes #100 #230 #266 #318

Only closed 100.
I think it works with a comma.

Oops, thanks for taking care of it! Looks like we actually need to repeat the "Fixes / Closes" keyword (https://stackoverflow.com/questions/3547445/closing-multiple-issues-in-github-with-a-commit-message)