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

Discussion: optionally restoring argparse nargs/multiplicity support #690

Open
azjps opened this issue Dec 8, 2021 · 0 comments
Open

Discussion: optionally restoring argparse nargs/multiplicity support #690

azjps opened this issue Dec 8, 2021 · 0 comments

Comments

@azjps
Copy link
Collaborator

azjps commented Dec 8, 2021

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 #582 (comment) (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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant