Support passing variable number of arguments to resolvers. #46

swist · 2019-10-23T15:45:44Z

This PR adds the ability to pass in 0 or more arguments to resolvers:

${foo:}               # zero arguments
${foo:a}              # one argument (currently supported)
${foo:a,b,..,n}       # n arguments

swist · 2019-10-23T15:46:05Z

BTW when do you plan to publish a new version to pypi? Would be nice to not have to run my fork

coveralls · 2019-10-23T15:48:16Z

Pull Request Test Coverage Report for Build 245

15 of 15 (100.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.1%) to 99.262%

Totals
Change from base Build 233:	0.1%
Covered Lines:	2421
Relevant Lines:	2439

💛 - Coveralls

omry · 2019-10-23T15:51:51Z

I am planning to make a pass at the open issues first.
It may take a few months as I am still heavily focused on Hydra and also got a bunch of travel coming in the following months.

In the mean time, you can install from master by putting something like this in your setup.py:
(Or probably also instructing pip to install this directly).

        install_requires=[
            "omegaconf@git+ssh://[email protected]/omry/omegaconf.git@master",
        ],

omry · 2019-10-23T15:53:26Z

What do you think about extending the resolver to properly accept multiple arguments instead?
Allowing any argument like this will deny the possibility of cleanly doing it in the future.

tests/test_interpolation.py

omry

Can you keep the value (new terminology: ${func:value} as a string until it's actually passed to the user's function?
not sure if this is possible, but it feels like touches a bit too much.
handle and test escaping.
Update documentation.

omegaconf/config.py

swist · 2019-10-23T18:20:54Z

Cool, will update later tomorrow, got the tokenizer in the right place, just having some pains dealing with escaping spaces - ideally something like this would work, but no clean solution comes to mind off the top of my head

tokenize('cat dog, dog cat') == ['cat dog', 'dog cat']
tokenize('cat dog,\ dog cat') == ['cat dog', ' dog cat']

i.e. only need to escape whitespace at the "boundaries" of the word

[edit] d'oh, just realised this is what you wrote earlier

omry · 2019-10-23T18:32:27Z

Great, I will take a closer look later.
Ideally, this would be handled by a proper tokenizer like antlr, but OmegaConf does not (yet) has this dependency.
It would also be pretty involved to set this up cleanly (see http://github.com/omry/antelope for an example).

Re:

tokenize('cat dog, dog cat') == ['cat dog', 'dog cat']
tokenize('cat dog,\ dog cat') == ['cat dog', ' dog cat']

I think something like this would be a good overall strategy:

split by un-escaped commas:
'cat dog,\ dog cat' -> ['cat dog', '\ dog cat']
replace ',' and '\ ' by ',' and ' '.
for each element, strip whitespace for both ends.

The first part is a bit tricky, you can achieve that relatively cleanly by replacing ',' with an illegal character like *, then split by that, and then replace it back.

swist · 2019-10-23T18:39:33Z

Un-brainfarted myself. I think the current version makes sense. The strategy is as follows:

split by unescaped commas
remove unescaped whitespace at word ends for each in split
unescape commas and whitespace

Also added a doc example

omry

Almost there!

omegaconf/config.py

omegaconf/omegaconf.py

tests/test_interpolation.py

docs/source/usage.rst

omry

two small comments.

docs/source/usage.rst

omry · 2019-10-23T21:52:23Z

tests/test_interpolation.py

+@pytest.mark.parametrize('resolver,name,key,result', [
+    (lambda *args: args, 'arg_list', "${my_resolver:cat, dog}",("cat", "dog")),
+    (lambda *args: args, 'escape_comma',"${my_resolver:cat\, do g}",("cat, do g",)),
+    (lambda *args: args, 'escape_whitespace',"${my_resolver:cat\, do g}",("cat, do g",)),
+    (lambda: 'zero', 'zero_arg',"${my_resolver:}",("zero")),
+])


Not familiar with this syntex. Why do you need it here?

I'm not sure what you mean? Would you prefer I passed in entire config dictionaries as opposed to pairs of key, string_to_resolve?

oh, I was confused by the lambda being passed there, I get why you need this now.

OmegaConf's templating language allows very few characters. There are some use cases where you'd want to have special characters so that your custom resolver can handle resolution there (kind of like embedding a templating language inside of OmegaConfs). This allows such behaviour without seemingly breaking anything too majorly

omry · 2019-10-24T20:28:32Z

tests/test_interpolation.py

+@pytest.mark.parametrize('resolver,name,key,result', [
+    (lambda *args: args, 'arg_list', "${my_resolver:cat, dog}",("cat", "dog")),
+    (lambda *args: args, 'escape_comma',"${my_resolver:cat\, do g}",("cat, do g",)),
+    (lambda *args: args, 'escape_whitespace',"${my_resolver:cat\, do g}",("cat, do g",)),
+    (lambda: 'zero', 'zero_arg',"${my_resolver:}",("zero")),
+])


oh, I was confused by the lambda being passed there, I get why you need this now.

omry · 2019-10-24T20:30:17Z

Thanks a lot for this!
By the way, It will be useful to include to add an optional default value to the environment variable resolver by default.

omry · 2019-10-24T20:31:44Z

General feedback:
When I merge, I can squash in the GitHub UI just prior to merging the PR.
For future diffs, it will be easier if you don't force push to allow me to see the incremental difference from the previously reviewed diff(s).

swist · 2019-10-25T10:13:49Z

Cool no problem, I normally default to pull-rebase so that merge commits don't come through and I didn't want to spam history with a bunch of wip commits.

omry reviewed Oct 23, 2019

View reviewed changes

tests/test_interpolation.py Outdated Show resolved Hide resolved

swist force-pushed the feature/better-regex-for-custom-resolvers branch from 9430abb to 221d5c7 Compare October 23, 2019 16:38

omry requested changes Oct 23, 2019

View reviewed changes

omegaconf/config.py Outdated Show resolved Hide resolved

swist force-pushed the feature/better-regex-for-custom-resolvers branch 3 times, most recently from 553d882 to a987662 Compare October 23, 2019 17:46

swist force-pushed the feature/better-regex-for-custom-resolvers branch from a987662 to 658adef Compare October 23, 2019 18:37

swist force-pushed the feature/better-regex-for-custom-resolvers branch from 658adef to e96e53d Compare October 23, 2019 19:49

omry requested changes Oct 23, 2019

View reviewed changes

swist force-pushed the feature/better-regex-for-custom-resolvers branch 4 times, most recently from b81cd62 to 79f114a Compare October 23, 2019 21:32

omry approved these changes Oct 23, 2019

View reviewed changes

swist force-pushed the feature/better-regex-for-custom-resolvers branch from 79f114a to 3c053c4 Compare October 24, 2019 08:37

omry approved these changes Oct 24, 2019

View reviewed changes

omry changed the title ~~Loosen up regex requirements in resolve single~~ Support passing variable number of arguments to resolvers. Oct 24, 2019

omry merged commit efbf485 into omry:master Oct 24, 2019

omry mentioned this pull request Nov 19, 2019

1.4.0 #85

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support passing variable number of arguments to resolvers. #46

Support passing variable number of arguments to resolvers. #46

swist commented Oct 23, 2019 •

edited by omry

Loading

swist commented Oct 23, 2019

coveralls commented Oct 23, 2019 •

edited

Loading

omry commented Oct 23, 2019

omry commented Oct 23, 2019

omry left a comment

swist commented Oct 23, 2019 •

edited

Loading

omry commented Oct 23, 2019

swist commented Oct 23, 2019 •

edited

Loading

omry left a comment

omry left a comment

omry Oct 23, 2019

swist Oct 24, 2019

omry Oct 24, 2019

omry Oct 24, 2019

omry commented Oct 24, 2019

omry commented Oct 24, 2019

swist commented Oct 25, 2019

Support passing variable number of arguments to resolvers. #46

Support passing variable number of arguments to resolvers. #46

Conversation

swist commented Oct 23, 2019 • edited by omry Loading

swist commented Oct 23, 2019

coveralls commented Oct 23, 2019 • edited Loading

Pull Request Test Coverage Report for Build 245

💛 - Coveralls

omry commented Oct 23, 2019

omry commented Oct 23, 2019

omry left a comment

Choose a reason for hiding this comment

swist commented Oct 23, 2019 • edited Loading

omry commented Oct 23, 2019

swist commented Oct 23, 2019 • edited Loading

omry left a comment

Choose a reason for hiding this comment

omry left a comment

Choose a reason for hiding this comment

omry Oct 23, 2019

Choose a reason for hiding this comment

swist Oct 24, 2019

Choose a reason for hiding this comment

omry Oct 24, 2019

Choose a reason for hiding this comment

omry Oct 24, 2019

Choose a reason for hiding this comment

omry commented Oct 24, 2019

omry commented Oct 24, 2019

swist commented Oct 25, 2019

swist commented Oct 23, 2019 •

edited by omry

Loading

coveralls commented Oct 23, 2019 •

edited

Loading

swist commented Oct 23, 2019 •

edited

Loading

swist commented Oct 23, 2019 •

edited

Loading