Add '-' char in ID.

[gleize]

Motivation

The override grammar doesn't handle the '-' character in the ID of groups even though the yaml file does not have this issue.
For example, this is perfectly acceptable in the yaml file :

defaults:
  - /models/classifier-heads@head: old_name

but the override parameter 'models/classifier-heads@head=new_name' throws an error indicating the group ID is incorrect.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

• Ran python setup.py antlr.
• Ran pytest (python 3.8).
• Replaced my hydra/grammar/gen folder by newly generated one and tested on my setup similar to the example described above.

Related Issues and PRs

N/A

[Jasha10]

Thanks for the PR, @gleize.

[Jasha10]

I don't see any problem with adding '-'.
@odelalleau, would you be willing to give a second opinion?

[odelalleau]

I don't see any problem with adding '-'. @odelalleau, would you be willing to give a second opinion?

Only thing that comes to mind is that in the past we thought of potentially adding some basic arithmetic operations, so that you could do something like ${x} - 1 and then - would become special somehow. That being said this is very far from a fully fleshed out idea and maybe it's not actually a problem at all.

The same change should be reflected in OmegaConf's grammar. Pay attention to the comments at the top of https://github.com/omry/omegaconf/blob/master/omegaconf/grammar/OmegaConfGrammarParser.g4

[Jasha10]

Ok, thanks very much Olivier!

@gleize, could you please open a PR against OmegaConf making the same modification to the grammar?

• ID from OmegaConfGrammarLexer.g4 needs to be updated
• _id from grammar_parser.py needs be updated
• Adding a few test cases to tests/test_grammar.py would be much appreciated.

[gleize]

Thanks @Jasha10 for the pointers.
Here is the OmegaConf PR : omry/omegaconf#880

[omry]

I think ID should not support -.
IDs are accessible at runtime via cfg.foo.
cfg.foo-bar is not legal python.
In addition, overring parameters with - could confuse argparse which if the - is prefixing the variable.

[gleize]

I think ID should not support -. IDs are accessible at runtime via cfg.foo. cfg.foo-bar is not legal python. In addition, overring parameters with - could confuse argparse which if the - is prefixing the variable.

One thing I've seen in other libraries to handle the non-pythonic IDs (e.g. cfg.foo-bar.key) is to add an alternative access method using __getitem__ (e.g. cfg["foo-bar"].key). This would enable the library to be fully compatible with the JSON/YAML formats. Right now it feels a bit weird to write syntactically correct config files that are loaded properly but cannot be used with the overriding grammar.

Regarding argparse, I'm currently using this fork and haven't had any issue with argparse. But maybe you're thinking about a very specific case where it would fail ?

[odelalleau]

I think ID should not support -. IDs are accessible at runtime via cfg.foo. cfg.foo-bar is not legal python

Why should IDs be necessarily accessible at runtime via cfg.foo?

In addition, overring parameters with - could confuse argparse which if the - is prefixing the variable.

Note that this PR actually doesn't allow - as first character so this shouldn't actually be an issue.

One thing I've seen in other libraries to handle the non-pythonic IDs (e.g. cfg.foo-bar.key) is to add an alternative access method using __getitem__ (e.g. cfg["foo-bar"].key)

That's what OmegaConf is doing as well, cfg["foo-bar"] works.

Regarding argparse, I'm currently using this fork and haven't had any issue with argparse. But maybe you're thinking about a very specific case where it would fail ?

Hydra allows some command line parameters starting with - (https://hydra.cc/docs/advanced/hydra-command-line-flags/) => this might mess with them if - was allowed as first character.

[pixelb]

It's a pity the yaml isn't already constrained to python compat ids.
However given it's not, I'm not against this patch per se, given we do support dict["__getitem__"]
However we need to think about the general case here of what's supported in yaml in and in code

[Jasha10]

After considering issue #2363, I'm inclined to move forward with this PR as well as with the sister PR omry/omegaconf#880 .

Only thing that comes to mind is that in the past we thought of potentially adding some basic arithmetic operations, so that you could do something like ${x} - 1 and then - would become special somehow. That being said this is very far from a fully fleshed out idea and maybe it's not actually a problem at all.

Here's a cross-link to the OmegaConf feature request for arithmetic support: omry/omegaconf#91 .