traitlets icon indicating copy to clipboard operation
traitlets copied to clipboard

Discussion: optionally restoring argparse nargs/multiplicity support

Open azjps opened this issue 4 years ago • 0 comments

Hi, I realize this may be re-opening a can of worms, but I was looking for a way to re-support --key v1 v2 v3 for Container traits in argparse-based CLI handling, originally added in #322 by @ankostis. That support was one of the main reasons I felt comfortable refactoring a codebase to use traitlets. While trying to upgrade to traitlets==5.1, I found out that traitlets was now silently discarding v2 v3 due to https://github.com/ipython/traitlets/pull/582#issuecomment-671922717 (none of my applications use positional arguments).

I was able to patch this for my applications in a rather convoluted way by having them inherit a mixin to change the argparse loader, using #360:

class FixNArgsMixin(HasTraits):
    def _create_loader(self, ..):
        class _DefaultOptionDict(loader._DefaultOptionDict):
            def _add_kv_action(self, key):
                self[key] = loader._KVAction(
                     # ..
                     nargs="+",  # not ideal, but not sure what else can be done, unless we restrict ourselves to Application.classes, resolve these traits and look up trait.multiplicity 
                )
           class KVArgParser(argparse.ArgumentParser):
                 # same as loader.KVArgParser, but with _DefaultOptionDict
           class KVArgParseConfigLoader(loader.KVArgParseConfigLoader):
                 parser_class = KVArgParser
           return KVArgParserConfigLoader(..)
           
class MyApp(FixNArgsMixin, Application):
    foo = List(config=True).tag(multiplicity="+")
MyApp().initialize(["--MyApp.foo", "a", "b"])        

This is pretty verbose and frail since it re-implements a significant amount of the traitlets.config.loader internals. One idea I was considering is to support an allow_nargs=True argument to _KVArgParser() which will set nargs="+" in _DefaultOptionDict, and that way the above method could be shortened to return KVArgParserConfigLoader(.., allow_nargs=True).

Why I use nargs="+":

  1. Compatibility with existing script APIs
  2. Principle of least surprise, its natural to python users that lists can be built from CLI via nargs="+", and matches the argparse behavior of consuming nargs="+" instead of positional arguments
  3. Significantly easier to type/autocomplete out --App.my_long_trait_name 1 2 3 4 vs --App.my_long_trait_name 1 --App.my_long_trait_name 2 --App.my_long_trait_name 3 --App.my_long_trait_name 4

Note: I understand that the multiplicity feature was buyer-beware since it was not officially released (and that traitlets is to an extent "semi-private"), and also the significant complexity in trying to handle nargs together with positional arguments, hence why I don't wish to request any changes in the current default behavior.

cc @minrk @Carreau, apologies in advance for wall of text

azjps avatar Dec 08 '21 12:12 azjps