Grammar support for interpolations like ${.}. ${..} etc.

[omry]

This enables implementing a form of select with a default value:
Side note, we should rethink support for passing the parent node automatically on demand (${.}), it will simplify this use case.

from typing import Any

from omegaconf import OmegaConf
from omegaconf.omegaconf import _EMPTY_MARKER_

yaml = """\
a: 10
no_default: ${oc.select:${.}, a}
not_using_default: ${oc.select:${.}, a, 20}
using_default: ${oc.select:${.}, not_there, 20}
"""


def oc_select(node: Any, key: str, default: Any = _EMPTY_MARKER_):
    if default is _EMPTY_MARKER_:
        return OmegaConf.select(node, key)
    else:
        return OmegaConf.select(node, key, default=default)


OmegaConf.register_new_resolver("oc.select", oc_select)

cfg = OmegaConf.create(yaml)
assert cfg.a == 10
assert cfg.no_default == 10
assert cfg.not_using_default == 10
assert cfg.using_default == 20

Notes:

1. I did not like the complexity of the testing in test_grammar.py. seems like a new testing framework instead of a test designed to test some logic.
   I created a simpler more localized test for interpolations. happy to discuss.

2. while it's not very big, it can be easier to review one commit at a time.

[odelalleau]

Dummy review because Github is having trouble registering my comments => hoping that closing this review will fix it

[omry]

GitHub got issues now.
Leave it alone for now and come back later.

https://www.githubstatus.com/

[odelalleau]

Side note, we should rethink support for passing the parent node automatically on demand (${.}), it will simplify this use case.

Not sure, I'd say that the example you provided is clearer with explicitly passing ${.} (if it was implicit we also couldn't select from arbitrary nodes).

1. I did not like the complexity of the testing in test_grammar.py. seems like a new testing framework instead of a test designed to test some logic.
   I created a simpler more localized test for interpolations. happy to discuss.

Yeah the boilerplate code ended up being more complex than I would have liked. Adding new tests should be straightorward though (once you understand how things work). The motivation was to be able to add a lot of tests with a minimum of extra "fluff" for each test. IMO this makes tests more readable (not the code, but the test data).

There were no relative interpolation tests though it seems in that file (definitely an oversight) => right now tests with .. wouldn't be supported since we assume that we resolve an interpolation at the top-level. However it would be trivial to move it down one level.

I don't mind changing things if you can be specific on the formatting you would like to see (I'd rather not take a guess and end up with something worse).

Edit: oh, there was also another motivation, which is to just copy the test data into Hydra => it's easier if it's isolated

[odelalleau]

Great, thanks!

[omry]

Side note, we should rethink support for passing the parent node automatically on demand (${.}), it will simplify this use case.

Not sure, I'd say that the example you provided is clearer with explicitly passing ${.} (if it was implicit we also couldn't select from arbitrary nodes).

The motivation is to support the use case of providing a default value for interpolation.

foo :${bar} # or 10 if bar does not exist.

This is coming from #541, which is proposing a completely new syntax for it:

a : 10
b : ${a,  20}  # 20 if a is not there.

I think a new syntax is an overkill and we should be able to leverage custom resolvers.
The idea to add oc.select() came from that use case. Interplation syntax already supports selecting from arbitrary nodes.
foo is foo in the config root. .foo is a sibling foo etc.

The following is cleaner than forcing the ${.} everywhere and it seems just as flexible.

v1: val1
foo:
  v2 : val2
  b: ${oc.select: v1, default} # val1
  c: ${oc.select: .v2, default}  # val2

In fact, this function is more Container.select() than OmegaConf.select().
maybe the resolver name should be oc.container.select or oc.node.select.

1. I did not like the complexity of the testing in test_grammar.py. seems like a new testing framework instead of a test designed to test some logic.
   I created a simpler more localized test for interpolations. happy to discuss.

Yeah the boilerplate code ended up being more complex than I would have liked. Adding new tests should be straightorward though (once you understand how things work). The motivation was to be able to add a lot of tests with a minimum of extra "fluff" for each test. IMO this makes tests more readable (not the code, but the test data).

Will respond to this part later. gtg.

[odelalleau]

The following is cleaner than forcing the ${.} everywhere and it seems just as flexible.

Got it. No objection from me as long as it remains optional.

[Jasha10]

Would it be difficult to add support for going up multiple levels in the hierarchy?
For example, something like ${...} or ${...key} with three dots or more.

[omry]

Would it be difficult to add support for going up multiple levels in the hierarchy?
For example, something like ${...} or ${...key} with three dots or more.

Like this?

In [2]: cfg = OmegaConf.create({"v": 1, "a": {"b": {"c": "${...v}"}}})

In [3]: cfg
Out[3]: {'v': 1, 'a': {'b': {'c': '${...v}'}}}

In [4]: cfg.a.b.c
Out[4]: 1

[Jasha10]

Like this?

Yes, exactly :)

[omry]

1. I did not like the complexity of the testing in test_grammar.py. seems like a new testing framework instead of a test designed to test some logic.
   I created a simpler more localized test for interpolations. happy to discuss.

Yeah the boilerplate code ended up being more complex than I would have liked. Adding new tests should be straightorward though (once you understand how things work). The motivation was to be able to add a lot of tests with a minimum of extra "fluff" for each test. IMO this makes tests more readable (not the code, but the test data).

This in general is objectively a bit hard to test directly because we depend on a config object in the parsing.
Still, I feel like you your test has too many input modes, which makes it harder to understand.
In fact, I would claim that without debugging the test it's not really possible to fully understand the how the test is working.
That's not a good situation for a test.
I think my test can be simplified (a part can be factored out to a different function. we can do it if and when we add a similar test).

There were no relative interpolation tests though it seems in that file (definitely an oversight) => right now tests with .. wouldn't be supported since we assume that we resolve an interpolation at the top-level. However it would be trivial to move it down one level.

Can you explain what is not supported with an example?

I don't mind changing things if you can be specific on the formatting you would like to see (I'd rather not take a guess and end up with something worse).

Edit: oh, there was also another motivation, which is to just copy the test data into Hydra => it's easier if it's isolated

You can do this:

interpolation_test_data = [
    # config root
    param(
        "${}",
        "",
        {"dict": {"bar": 20}, "list": [1, 2]},
        id="absolute_config_root",
    ),
    # simple
    param("${dict.bar}", "", 20, id="dict_value"),
    ...
]


@mark.parametrize(
    ("inter", "key", "expected"),
    interpolation_test_data,
)
def test_parse_interpolation(inter: Any, key: Any, expected: Any) -> None:
 ...

I think it's still valuable to have the test data close to the test itself. stashing it all at the top of the file makes it significantly harder to understand the tests.

[omry]

Like this?

Yes, exactly :)

Don't try {..} through unless you like stack overflows.

In [38]: cfg = OmegaConf.create({"v": 1, "a": "${}", "b": {"c" : "${..}"}})

In [39]: cfg
Out[39]: {'v': 1, 'a': '${}', 'b': {'c': '${..}'}}

In [40]: cfg.a
Out[40]: {'v': 1, 'a': '${}', 'b': {'c': '${..}'}}

In [41]: cfg.a.a.a
Out[41]: {'v': 1, 'a': '${}', 'b': {'c': '${..}'}}

In [42]: cfg.b.c
Out[42]: {'v': 1, 'a': '${}', 'b': {'c': '${..}'}}

# OmegaConf.to_container(cfg, resolve=True) # boom

[odelalleau]

In fact, I would claim that without debugging the test it's not really possible to fully understand the how the test is working.

Right, I can definitely see how the current code is difficult to understand. However, once you know what's going on, you don't have to look at the code anymore and you can just add new test items in the corresponding list.

There were no relative interpolation tests though it seems in that file (definitely an oversight) => right now tests with .. wouldn't be supported since we assume that we resolve an interpolation at the top-level. However it would be trivial to move it down one level.

Can you explain what is not supported with an example?

If I add the following item to the list PARAMS_SINGLE_ELEMENT_WITH_INTERPOLATION:

("rel_test", "${..foo}", None),

then it will fail with with this traceback:

                root = root._get_parent()
                if root is None:
>                   raise ConfigKeyError(f"Error resolving key '{orig}'")
E                   omegaconf.errors.InterpolationKeyError: ConfigKeyError while resolving interpolation: Error resolving key '..foo'

because the parent is the root config, so the parent of the parent doesn't exist.

It'd be easy to fix by doing the resolution at a deeper level in the config.

You can do this:

interpolation_test_data = [
    # config root
    param(
        "${}",
        "",
        {"dict": {"bar": 20}, "list": [1, 2]},
        id="absolute_config_root",
    ),
    # simple
    param("${dict.bar}", "", 20, id="dict_value"),
    ...
]


@mark.parametrize(
    ("inter", "key", "expected"),
    interpolation_test_data,
)
def test_parse_interpolation(inter: Any, key: Any, expected: Any) -> None:
 ...

Yeah, that's essentially what I did except that I got rid of the param and id stuff (not a big deal, but I find it a bit cleaner, and more importantly, it is less likely for a test to be formatted over multiple lines by black, which makes it harder to read). Also I don't like adding a key item that would be "" 99% of the time.

I think it's still valuable to have the test data close to the test itself. stashing it all at the top of the file makes it significantly harder to understand the tests.

I hear you. But that's kind of how it is in a sense: the way I see it, TestOmegaConfGrammar is actually one single test, and the 3 lists of parameters on top of it are semantically almost the same (and can be almost seen as a single list of everything we want to test here). Having all the data at the top makes it easier in practice because you don't have to scroll down through code to find the section you are looking for (also, the config definition is closer).

The exception is test_deprecated_empty_args() which has its own list of parameters within the class: the reason is that it's testing a deprecated feature so I thought it wouldn't be worth creating an extra list just for it, since it would be removed later.

In terms of practical action item, I can easily move everything to @mark.parametrize() with param(..., id=...) next to each method if you want. But I think the main complexity of the test right now is in the code that processes this data. It's a bit tricky in particular because of the error handling. I could split the test in two to handle errors separately if you prefer, though personally for such a large amount of tests, I find the current approach more readable (not in the test code, but when looking at the test data, because you can easily see both the working and error situations for each category of item being parsed).

Let me know whether to proceed with some of these changes (or others).

[omry]

There were no relative interpolation tests though it seems in that file (definitely an oversight) => right now tests with .. wouldn't be supported since we assume that we resolve an interpolation at the top-level. However it would be trivial to move it down one level.

Can you explain what is not supported with an example?

If I add the following item to the list PARAMS_SINGLE_ELEMENT_WITH_INTERPOLATION:

("rel_test", "${..foo}", None),

then it will fail with with this traceback:

                root = root._get_parent()
                if root is None:
>                   raise ConfigKeyError(f"Error resolving key '{orig}'")
E                   omegaconf.errors.InterpolationKeyError: ConfigKeyError while resolving interpolation: Error resolving key '..foo'

What are you expecting ${..foo} to resolve to in the above fragment and why?
Is there a user facing issue this is causing?

because the parent is the root config, so the parent of the parent doesn't exist.

It'd be easy to fix by doing the resolution at a deeper level in the config.

Can you file a followup PR?

You can do this:

interpolation_test_data = [
    # config root
    param(
        "${}",
        "",
        {"dict": {"bar": 20}, "list": [1, 2]},
        id="absolute_config_root",
    ),
    # simple
    param("${dict.bar}", "", 20, id="dict_value"),
    ...
]


@mark.parametrize(
    ("inter", "key", "expected"),
    interpolation_test_data,
)
def test_parse_interpolation(inter: Any, key: Any, expected: Any) -> None:
 ...

Yeah, that's essentially what I did except that I got rid of the param and id stuff (not a big deal, but I find it a bit cleaner, and more importantly, it is less likely for a test to be formatted over multiple lines by black, which makes it harder to read). Also I don't like adding a key item that would be "" 99% of the time.

I think we should use a single testing style in the codebase.
I agree that it's annoying the black is breaking the test lines. I am actually okay with increasing black line length in OmegaConf and Hydra. This can be a standalone diff to OmegaConf first).

I think it's still valuable to have the test data close to the test itself. stashing it all at the top of the file makes it significantly harder to understand the tests.

I hear you. But that's kind of how it is in a sense: the way I see it, TestOmegaConfGrammar is actually one single test, and the 3 lists of parameters on top of it are semantically almost the same (and can be almost seen as a single list of everything we want to test here). Having all the data at the top makes it easier in practice because you don't have to scroll down through code to find the section you are looking for (also, the config definition is closer).

I have similar patterns in other places, like in test_errors.py (that I am not really happy with).
I can tell you that even though it has advantages, it also have many disadvantages over more compact style grouping tests by the functionality they are testing.

Here is an example of a large test in Hydra. The test functions are usually the same and are calling a common generic function.
Splitting by functionality makes it easier to introduce tests for new areas (like I did with testing relative interpolations in this PR).
Having a common function to test with reduces the overhead of many different test functions.

Another problem I have with the current test is that the base config it's using to test has everything and it's mother, and it's defined far away from the test code.
It would be easier to understand in the way my test is done, where the test config is defined in or near the test function.
I think different test functions typically are focused on different needs and it does not make sense for all of them to get a config with every possible thing.

The exception is test_deprecated_empty_args() which has its own list of parameters within the class: the reason is that it's testing a deprecated feature so I thought it wouldn't be worth creating an extra list just for it, since it would be removed later.

In terms of practical action item, I can easily move everything to @mark.parametrize() with param(..., id=...) next to each method if you want. But I think the main complexity of the test right now is in the code that processes this data. It's a bit tricky in particular because of the error handling. I could split the test in two to handle errors separately if you prefer, though personally for such a large amount of tests, I find the current approach more readable (not in the test code, but when looking at the test data, because you can easily see both the working and error situations for each category of item being parsed).

Let me know whether to proceed with some of these changes (or others).

I think a lot of the complexity now is that the test is trying to handle an arbitrary list of parameters in a generic way, even though the list looks different in each case.

Things can probably be simplified by allowing each test to be more specialized (while reusing whatever is possible through some utility functions).

All this is not particularly urgent, so no need to interrupt what you are working on. but I think we should add it to the queue.

[odelalleau]

If I add the following item to the list PARAMS_SINGLE_ELEMENT_WITH_INTERPOLATION:

("rel_test", "${..foo}", None),

then it will fail with with this traceback:

                root = root._get_parent()
                if root is None:
>                   raise ConfigKeyError(f"Error resolving key '{orig}'")
E                   omegaconf.errors.InterpolationKeyError: ConfigKeyError while resolving interpolation: Error resolving key '..foo'

What are you expecting ${..foo} to resolve to in the above fragment and why?
Is there a user facing issue this is causing?

Oh, I was expecting ${..foo} to crash since that's how this test currently works (it resolves the expression as if it was in a top-level node in the config). There is no user-facing issue.

because the parent is the root config, so the parent of the parent doesn't exist.
It'd be easy to fix by doing the resolution at a deeper level in the config.

Can you file a followup PR?

Probably not worth it at this point since you already added relative interpolation tests in this PR. Would probably make more sense to do it at the same time the tests would be refactored.

All this is not particularly urgent, so no need to interrupt what you are working on. but I think we should add it to the queue.

Ok will add it in my list somewhere. To be honest though I'm still not entirely sure how such splitting would look like. There aren't that many different things tested here, it's essentially all the same stuff (take an expression, parse it with the desired rule, check the result). But we can discuss it later when we get to it.

[omry]

What I did in Hydra, which makes a lot of sense there - is to test primitive rules separately.

@mark.parametrize(
    "value,expected",
    [
        param("[]", [], id="list:empty"),
        param("[1]", [1], id="list:item"),
        param(
            "['a b']",
            [QuotedString(text="a b", quote=Quote.single)],
            id="list:quoted_item",
        ),
        param(
            "['[a,b]']",
            [QuotedString(text="[a,b]", quote=Quote.single)],
            id="list:quoted_item",
        ),
        param("[[a]]", [["a"]], id="list:nested_list"),
        param("[[[a]]]", [[["a"]]], id="list:double_nested_list"),
        param("[1,[a]]", [1, ["a"]], id="list:simple_and_list_elements"),
    ],
)
def test_list_container(value: str, expected: Any) -> None:
    ret = parse_rule(value, "listContainer")
    assert ret == expected

Not sure it's applicable here.
I also don't want this to become a major time sink. See if you can clear things up a bit.

[omry]

I guess it could be applicable in the context of an individual config object. (that can have the minimum needed to test the specific rule).

[odelalleau]

What I did in Hydra, which makes a lot of sense there - is to test primitive rules separately.
(...)
Not sure it's applicable here.

We could to some extent (that would essentially mean splitting PARAMS_SINGLE_ELEMENT_NO_INTERPOLATION). One small thing though is that I would prefer to create new rules for testing single rules, like:

singleListContainer: listContainer EOF;

because if we only test listContainer, the parser is happy with an expression that starts with a list, regardless of what comes after. So for instance [1, 2, 3]] would be accepted as a valid list.

[omry]

I think it's reasonable to use the existing rules as is.
Not sure I like adding a bunch of extra rules and logic in the code for testing purposes here.
Do you know what is commonly done in such cases with other grammars elsewhere? Personally I was always okay with this.

[odelalleau]

I think it's reasonable to use the existing rules as is.
Not sure I like adding a bunch of extra rules and logic in the code for testing purposes here.
Do you know what is commonly done in such cases with other grammars elsewhere? Personally I was always okay with this.

Thinking about it a bit more, I think a better solution is to check that the whole string was processed.