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

Use argparse to configure container-traits from command-line #322

Merged
merged 8 commits into from
Oct 25, 2016

Conversation

ankostis
Copy link
Contributor

@ankostis ankostis commented Sep 22, 2016

  • The idea was to scan all conf-classes to collect List/Set/Tuple/Dict traits and use them to setup argparse for reading cmd-line args either with action='append' or nargs='*'`.
  • Additionally, do the same for any matching aliases (for flags this does not make sense).
  • But eventually had to feed all config-traits through argparse when classes are provided.
    The reason is that any "regular"(not handled by argparse) trait with a name that is a prefix of some container-trait will get miss-classified as that container-trait (disabling this allow_abbrev behavior is not supported in py2).

Design

  1. The Application on parse_command_line() uses its classes field to collects all relevant HasTraits classes and subclasses to push them into the KVArgParseConfigLoader constructor;
  2. The KVArgParseConfigLoader.load_config() invokes _add_arguments() for all list/Set/tuple traits found on the collected classes , and appends them as argparse arguments according to the multiplicity metadata found on these traits:
    1. multiplicity == 'append': use add_argument(..., action='append') (default if metadata missing);
    2. multiplicity == [ '*' | '+' | N ]: use add_argument(..., nargs=multiplicity).
  3. During the above step, all container traits are collected in the new loder's field argparse_traits to be used in the next step to create the appropriate config-tree instance.
  4. After argparse has parsed cmd-line arguments, the KVArgParseConfigLoader._convert_to_config() py-evaluates any lists-of-values (or list-of-tuples for Dict traits) one by one before creating and assigning the appropriate instance (i.e. dict for Dict traits) into the returned config-instance .

Example

Assuming this 2 configurables + 1 app:

class Base(Configurable): 
    a = List([]).tag(config=True)

class Sub(Base): 
    atuple = Tuple([]).tag(config=True, multiplicity='*')
    aset = Set(set()).tag(config=True, multiplicity='append')

class Cmd(Application):
    classes = [Sub]  ## Note on #289: frequently list given like that.
    aliases = {
        'a':  'Base.a', 
        'aa': 'Sub.a', 
        't':  'Sub.atuple', 
        's':  'Sub.aset'
    }

Then, in command-line it should be possible to enter all these commands:

cmd  --Base.a=1 -a=foo      ## Base.a = [1, 'foo']
cmd  --aa=1  --Sub.a=foo    ## Sub.a  = [1, 'foo']
cmd  -t 1 ab cd 4  --Sub.aset a  -s b  -s a  ## Sub.b = (1, 'ab', 'cd', 4), Sub.set = {'a', 'b'}
cmd  --Base.a=1 --Base.a=2  --Sub.a=foo      ## Base.a = [1, 2], Sub.a = ['foo']
  • Notice that in the last case, Sub class trumps over any Base values.

For a Dict trait, the cmd-line syntax is this:

  cmd   --Base.adict kk=vv -D=key1=v1  -D key2=True -Dkey3="'True'"

resulting in this:

Base.adict = {'kk': 'v', 'key1': 'v1', 'key2': True, 'key3: 'True'}

Future

  • Merge parts of KVArgParseConfigLoader into ArgParseConfigLoader class - actually I would try to compress the loader hierarchy.

ankostis added a commit to ankostis/traitlets that referenced this pull request Sep 23, 2016
ankostis added a commit to ankostis/traitlets that referenced this pull request Sep 23, 2016
@ankostis
Copy link
Contributor Author

@minrk At traitlets/traitlests.py#1378 it tests whether a variable can be called; why is it using type(var) is types.FunctionType instead of iscallable(var)?

The same question applies for all such tests.

ankostis added a commit to ankostis/traitlets that referenced this pull request Sep 23, 2016
+ Both `class.trait` & aliases added as argparse-args.
ankostis added a commit to ankostis/traitlets that referenced this pull request Sep 23, 2016
+ Both `class.trait` & aliases added as argparse-args.
ankostis added a commit to ankostis/traitlets that referenced this pull request Sep 23, 2016
+ Both `class.trait` & aliases added as argparse-args.
ankostis added a commit to ankostis/traitlets that referenced this pull request Sep 23, 2016
+ Only `class.trait` added as argparse-args - not aliases yet.
ankostis added a commit to ankostis/traitlets that referenced this pull request Sep 23, 2016
@minrk
Copy link
Member

minrk commented Sep 23, 2016

@ankostis I suspect checking for callable is the right thing to do in those cases. I've updated the checks in #323. That's a real old line, probably from the time when Python 3 had removed callable, which it's since put back.

@ankostis
Copy link
Contributor Author

@minrk PLEASE REVIEW the code + specs in the OP, to eventual become material for the manual.

@ankostis ankostis changed the title Config container-traits from command-line using *argparse* syntax Use argparse to configure container-traits from command-line Sep 23, 2016
@ankostis
Copy link
Contributor Author

ankostis commented Sep 23, 2016

PR works fine - maybe some TCs could also test more fringe cases, e.g. what happens when a List trait without defaults and without allow_null does not receive cmd-line values, etc.
But I had some mild problems extending it to Dict-traits.

In general, for a next release it might make more sense to drop the "mixed" parsing of args when conf-classes are available and delegate everything to arparse.
Specifically to rely on type functions of ArgumentParser.add_argument() method instead of _exec_config_str(), and introduce a new method that assembles all the collected values (list, dictionary) and assigns them into the Config instance according to the config-trait they originate.
The later was the problem I mentioned for extending this PR also for Dict-traits.

By the way, why in loader.py#L517 it uses eval to set the Config tree?

ankostis added a commit to ankostis/traitlets that referenced this pull request Sep 24, 2016
+ Use a regex as a `type` argument in `argparse.add_argument()` to parse
each kv-expression.
+ Add `KVAgParseLoader` "index" field `argparse_traits` to use it when
building the `config` items for Dict traits.
+ Assuming alias {'D': 'Foo.bar'} and `Foo.bar = Dict()` then one can
write:

    cmd -D k1=v1 -D K2=123
ankostis added a commit to ankostis/traitlets that referenced this pull request Sep 24, 2016
+ Use a regex as a `type` argument in `argparse.add_argument()` to parse
each kv-expression.
+ Add `KVAgParseLoader` "index" field `argparse_traits` to use it when
building the `config` items for Dict traits.
- Had to add globals/locals into `exec()` setting the config value.
+ Assuming alias {'D': 'Foo.bar'} and `Foo.bar = Dict()` then one can
write:

    cmd -D k1=v1 -D K2=123
@ankostis
Copy link
Contributor Author

ankostis asked:

By the way, why in loader.py#L517 it uses eval to set the Config tree?

Answering myself, I found 329db36 where @ellisonbg in 2009(!) did this change:

-            if v is not NoDefault:
-                setattr(self.config, k, v)
+            if v is not NoConfigDefault:
+                exec_str = 'self.config.' + k + '= v'
+                exec exec_str in locals(), globals()

unfortunately without explaining the reason - the only relative part of the commit message must be this:

... The config system now supports config
subsections, like config.Global. These sections, which must start
with an uppercase char, are auto-created upon a getattr.

When using setattr() many TCs fail with this message:

Traceback (most recent call last):
  File "D:\Work\traitlets.git\traitlets\config\tests\test_loader.py", line 336, in test_seq_traits
    config = cl.load_config(argv, aliases=aliases)
  File "D:\Work\traitlets.git\traitlets\config\loader.py", line 761, in load_config
    self._convert_to_config()
  File "D:\Work\traitlets.git\traitlets\config\loader.py", line 903, in _convert_to_config
    self._exec_config_str(k, v, trait=trait)
  File "D:\Work\traitlets.git\traitlets\config\loader.py", line 529, in _exec_config_str
    setattr(self.config, lhs, value)
  File "D:\Work\traitlets.git\traitlets\config\loader.py", line 287, in __setattr__
    self.__setitem__(key, value)
  File "D:\Work\traitlets.git\traitlets\config\loader.py", line 272, in __setitem__
    'char must be Config instances: %r, %r' % (key, value))
ValueError: values whose keys begin with an uppercase char must be Config instances: 'CSub.e', [1, 'a', 'bcd']

Any explaination why?

ankostis added a commit to ankostis/traitlets that referenced this pull request Sep 24, 2016
+ Use a regex as a `type` argument in `argparse.add_argument()` to parse
each kv-expression.
+ Add `KVAgParseLoader` "index" field `argparse_traits` to use it when
building the `config` items for Dict traits.
- Had to add globals/locals into `exec()` setting the config value.
+ Assuming alias {'D': 'Foo.bar'} and `Foo.bar = Dict()` then one can
write:

    cmd -D k1=v1 -D K2=123
@ankostis
Copy link
Contributor Author

With the latest commit, also Dict traits can be set by cmd-line argparse options.
Design OP document updated.

@minrk
Copy link
Member

minrk commented Sep 25, 2016

A simple setattr doesn't work because it's nested. It can be A.B.c. You could parse out the name and make a nested series of setattrs. I'm not sure that would help anything. I don't think it's relevant to this PR.

@ankostis
Copy link
Contributor Author

ankostis commented Sep 25, 2016

Thanks. So eval is not used for the globals(), and I can actually remove it; will re-write last commit.

You see, after my changes, in py2.7 I was getting the following error in exec line, and I had to add locals() and probably globals(). My question was whether the eval was needed because internally the config object did something with the globals.

../../../virtualenv/python2.7.9/lib/python2.7/site-packages/_pytest/python.py:463: in _importtestmodule
    mod = self.fspath.pyimport(ensuresyspath=True)
../../../virtualenv/python2.7.9/lib/python2.7/site-packages/py/_path/local.py:641: in pyimport
    __import__(modname)
traitlets/config/__init__.py:6: in <module>
    from .application import *
traitlets/config/application.py:19: in <module>
    from traitlets.config.configurable import Configurable, SingletonConfigurable
traitlets/config/configurable.py:12: in <module>
    from .loader import Config, LazyConfigValue, _is_section_key
E   SyntaxError: unqualified exec is not allowed in function '_exec_config_str' because it contains a nested function with free variables (loader.py, line 528)

@minrk
Copy link
Member

minrk commented Sep 26, 2016

I hadn't merged #325 yet, so rebasing wouldn't have picked it up. It's merged now, though.

+ Only `class.trait` added as argparse-args - not aliases yet.
+ Use a regex as a `type` argument in `argparse.add_argument()` to parse
each kv-expression.
+ Add `KVAgParseLoader` "index" field `argparse_traits` to use it when
building the `config` items for Dict traits.
- Had to add globals/locals into `exec()` setting the config value.
+ Assuming alias {'D': 'Foo.bar'} and `Foo.bar = Dict()` then one can
write:

    cmd -D k1=v1 -D K2=123
+ Add ellipses and explain the element type.
+ Add TCs with output samples.
+ Reason: if container-traits are handled by *argparse*, then any
regular trait with name that is a prefix of some container-trait would
get miss-classified as that container-trait.
+ Retrofitted app-TCs to detect the above clash.
- Had to disable 1 TC: test-app.test_ipython_cli_priority()
ankostis added a commit to ankostis/traitlets that referenced this pull request Sep 26, 2016
@ankostis
Copy link
Contributor Author

Indeed, the problem is fixed, TC passes.

@minrk
Copy link
Member

minrk commented Sep 26, 2016

I've tested and this is working great, thanks!

Pinging @jhamrick, who I believe has used the current --Class.trait="['complicated', 'list', 'config']" syntax before. This should make it much easier to specify list traits on the command-line, but I think it's going to be quite difficult to keep backward-compatibility with that syntax.

@ankostis
Copy link
Contributor Author

ankostis commented Sep 26, 2016

@minrk @jhamrick please check the OP, for material that can be included in the docs.

@jhamrick
Copy link
Contributor

Cool! I had basically given up on trying to use lists and dictionaries on the command line, so this won't cause any problems for me. Thanks! 🎉

@ankostis
Copy link
Contributor Author

ankostis commented Sep 26, 2016

Since you're both reviewing, a note from my side for the multiple places where cmd-line values are parsed:

  • once by argparse when extracting them out from argv (for Dict-traits, this entails matching them against the _kv_opt_pattern regex ),
  • then by [ast.literal_eval()](https://github.com/ankostis/traitlets/blob/list_args/traitlets/config/loader.py#L506), for each Container-item or Dict`-value, and finally,
  • by the Trait that is the receiver of the value, when assigning into it the Config object holding those values.

Maybe the places where these parsings take place can be centralized a bit, regarding the Loader hierarchy of classes (see my Future section on the opening post.

@minrk minrk added this to the 5.0 milestone Sep 27, 2016
@ankostis
Copy link
Contributor Author

Is there any plans for releasing this?

@minrk minrk merged commit 19eb1ed into ipython:master Oct 25, 2016
@minrk
Copy link
Member

minrk commented Oct 25, 2016

@ankostis not immediately, we still have a few things to get through.

@ankostis ankostis mentioned this pull request Aug 21, 2017
2 tasks
@Carreau Carreau added 5.0-re-review Need to re-review for potential API impact changes. 5.0-minor rereviewed, minor change need to be put in changelog. and removed 5.0-re-review Need to re-review for potential API impact changes. labels Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.0-minor rereviewed, minor change need to be put in changelog.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants