TAB completion crashes on unknown commands if followed by parameters

[giovannipizzi]

If I write verdi ru count_query.py and I press TAB I get on screen

  File "/Users/pizzi/.virtualenvs/aiida_core_dev3/bin/verdi", line 10, in <module>
    sys.exit(verdi())
  File "/Users/pizzi/.virtualenvs/aiida_core_dev3/lib/python3.6/site-packages/click/core.py", line 764, in __call__
    return self.main(*args, **kwargs)
  File "/Users/pizzi/.virtualenvs/aiida_core_dev3/lib/python3.6/site-packages/click/core.py", line 712, in main
    _bashcomplete(self, prog_name, complete_var)
  File "/Users/pizzi/.virtualenvs/aiida_core_dev3/lib/python3.6/site-packages/click_completion/patch.py", line 111, in _shellcomplete
    do_bash_complete(cli, prog_name)
  File "/Users/pizzi/.virtualenvs/aiida_core_dev3/lib/python3.6/site-packages/click_completion/core.py", line 168, in do_bash_complete
    echo('\t'.join(re.sub(r"""([\s\\"'()])""", r'\\\1', opt) for opt, _ in choices), nl=False)
  File "/Users/pizzi/.virtualenvs/aiida_core_dev3/lib/python3.6/site-packages/click_completion/core.py", line 168, in <genexpr>
    echo('\t'.join(re.sub(r"""([\s\\"'()])""", r'\\\1', opt) for opt, _ in choices), nl=False)
  File "/Users/pizzi/.virtualenvs/aiida_core_dev3/lib/python3.6/site-packages/click_completion/core.py", line 86, in get_choices
    ctx = resolve_ctx(cli, prog_name, args)
  File "/Users/pizzi/.virtualenvs/aiida_core_dev3/lib/python3.6/site-packages/click_completion/lib.py", line 82, in resolve_ctx
    cmd = ctx.command.get_command(ctx, a[0])
  File "/Users/pizzi/git/aiida-core/aiida/cmdline/commands/cmd_verdi.py", line 77, in get_command
    '{matches}'.format(cmd=cmd_name, matches='\n'.join('\t{}'.format(m) for m in sorted(matches)))
  File "/Users/pizzi/.virtualenvs/aiida_core_dev3/lib/python3.6/site-packages/click/core.py", line 496, in fail
    raise UsageError(message, self)
click.exceptions.UsageError: 'ru' is not a verdi command.

The most similar commands are: 
        group
        run

I think this is triggering some buggy path and this is because there is a parameter after the wrong/incomplete command ru (instead tab completion works if I press tab directly after verdi ru)

[sphuber]

This goes not just for parameters but also sub commands, e.g. verdi comput list followed by TAB will have the same result. The question is: what should the behavior be in this case though? It is pretty much impossible to programmatically "fix" this and correct the incorrect subcommand and retrigger the auto-completion. Should we just catch this exception and print a critical error saying the command does not exist?

[giovannipizzi]

I think it should simply not try to complete (it's bad to print out things while completing I think)

[danielhollas]

I just ran into this while playing with the aiida environment for the workshop next week. I think this might be especially confusing to beginners who might more easily mistake the verdi commands.

I'd be happy to fix this next week. I agree that the correct course of action is to do nothing in this case.

It seems that adding a try..except handling here ⏬ might do the trick, though I am a bit worried about possible side effects since I am not familiar with the click library. EDIT: Nope, that's not it, see comment below.

aiida-core/aiida/cmdline/commands/cmd_verdi.py

Line 53 in 5ab86cc

cmd = click.Group.get_command(self, ctx, cmd_name)

[danielhollas]

Hmm, this is tricky. The get_command function specialized in cmd_verdi.py gets called both when a user enters the verdi command via click/core.py, and when tab-completing, via click_completion/lib.py. Here's the trace for the former, the latter trace is given in the OP.

  File "/opt/conda/bin/verdi", line 8, in <module>
    sys.exit(verdi())
  File "/opt/conda/lib/python3.7/site-packages/click/core.py", line 829, in __call__
    return self.main(*args, **kwargs)
  File "/opt/conda/lib/python3.7/site-packages/click/core.py", line 782, in main
    rv = self.invoke(ctx)
  File "/opt/conda/lib/python3.7/site-packages/click/core.py", line 1254, in invoke
    cmd_name, cmd, args = self.resolve_command(ctx, args)
  File "/opt/conda/lib/python3.7/site-packages/click/core.py", line 1297, in resolve_command
    cmd = self.get_command(ctx, cmd_name)

It seems that these two code paths need different behavior (cannot call fail() when tab-completing. Not sure how to proceed here, maybe this is an issue in click_completion and not aiida?

[sphuber]

Thanks @danielhollas for the additional info. How do you trigger the two different path ways? I can only get pathway of the OP.

[sphuber]

I think it should simply not try to complete (it's bad to print out things while completing I think

This would be an option but I am pretty sure that we would get messages from users saying that tab-completion is broken and it turns out they just had an invalid subcommand in their chain. E.g. when doing:

verdi nod repo <TAB>

and it just won't complete anything, but it is easy to overlook the type in the subcommand node. I think there is something to say for failing early. At least it is immediately clear that something is wrong. So I would maybe we in favor of closing this issue. Maybe what we can do is hide the stack trace and simply print it as an error as it would have if you would have pressed enter on the incorrect command:

(aiida_dev) sph@citadel:~/code/aiida/env/dev/aiida-core$ verdi ru test.py
Usage: verdi [OPTIONS] COMMAND [ARGS]...
Try 'verdi -h' for help.

Error: 'ru' is not a verdi command.

The most similar commands are: 
	group
	run

Seeing the stack trace may scare users and think something is wrong with AiiDA, whereas the error clearly shows what went wrong and they can correct their command.

[danielhollas]

@sphuber the first pathway is triggered if you enter an invalid command and hit enter, e.g.

$ verdi st

Note that in this case aiida prints an error message without the stacktrace, as it should (I have printed the stack trace manually via traceback just to figure out what is going on). Sorry for not being clear.

I think there is something to say for failing early. At least it is immediately clear that something is wrong.

While I agree with this in principle, even without stack trace this is still a surprising behavior for Linux users. I am not aware of any Linux command that would behave like this. In fact, when tab completion is not working, I am (personally) immediately triggered to verify my command is correct.

[danielhollas]

Note that per the issue below, most of the tab completion functionality is now natively supported by click (and it's not clear whether the click-completion package is still maintained) so the solution here might be to upgrade click.

click-contrib/click-completion#37

[giovannipizzi]

@sphuber I agree with @danielhollas that I would not show an error - I never saw a command showing an error on tab completion. If you e.g. try with git (and have proper completion enabled), git branch <TAB> will show the branches, but git branc <TAB> will just use the default completion (files in the current folder). Maybe the best is to do the same - if we don't understand the command, we just fall back to the default completion of bash (rather than not completing at all). I would be OK with both options.

@danielhollas thanks!
Indeed if upgrading click is enough that would be great, even if we need to double check as we do some "dirty tricks" to enable interactive commands in a way that also work when provided programmatically (e.g. I think verdi computer setup where you can also pass them interactively).

Also pinging @csadorf who's our dependency manager for his feedback.

[sphuber]

I'm fine with either solution really. I don't think it is a bug in click and so doesn't require updating. I think all the desired behaviour requires is to simply remove the conditional for if not matches and not call fail. Will a PR soon.

Regarding click v8.0 providing some auto-complete functionality, we would have to see indeed whether it provides the same functionality as the click-completion add on that we use now. If the case, we can drop that requirement and just use native support in click==8.0.

As for your point on interactive commands @giovannipizzi , such as verdi computer setup, what do you mean exactly? I don't think the interactivity has anything to do with the auto-completion does it?

[sphuber]

Actually, the solution is going to be a bit more tricky. If we want to keep the current behavior of suggesting close commands in the case a non-existing command is specified, we cannot get rid of that conditional. We just have to disable it when get_command is being called during tab-completion. I am not sure yet how we can detect the difference. Not sure if this is even possible in principle.

[giovannipizzi]

As for your point on interactive commands @giovannipizzi , such as verdi computer setup, what do you mean exactly? I don't think the interactivity has anything to do with the auto-completion does it?

My comment was about what to take into account if we bump to the next major version of click.

I think what I had in mind is this:

aiida-core/aiida/cmdline/params/options/interactive.py

Lines 21 to 46 in a2ddbd5

	class InteractiveOption(ConditionalOption):
	"""
	Prompts for input, intercepting certain keyword arguments to replace :mod:`click`'s prompting
	behaviour with a more feature-rich one.
	.. note:: This class has a parameter ``required_fn`` that can be passed to its ``__init__`` (inherited
	from the superclass :py:class:`~aiida.cmdline.params.options.conditional.ConditionalOption`) and a
	``prompt_fn``.
	- ``required_fn`` is about "is this parameter required" depending on the value of other params.
	- ``prompt_fn`` is about "should I prompt for this value if in interactive mode" and only makes sense
	in this class and not in :py:class:`~aiida.cmdline.params.options.conditional.ConditionalOption`.
	In most usecases, if I have a ``prompt_fn``, then I would like to have also (the same) ``required_fn``.
	The implementation still makes them independent for usecases where they might be different functions
	(e.g. if the variable is anyway not required, but you want to decide whether to prompt for it or not).
	Usage::
	import click
	@click.command()
	@click.option('label', prompt='Label', cls=InteractiveOption)
	def foo(label):
	click.echo(f'Labeling with label: {label}')
	"""

Our class redefines a lot of the internal click methods of the super class, and (IIRC from when I was looking and working on it, probably 2-3 years ago) it changes the "logic" of "when/under which situation" those methods are called, to add interactiveness in the way we wanted.

I think the tests we have should catch possible issues, but it would be good to double check explicitly those interactive commands when we upgrade the click version.

(Ideally, it would be great to contribute that class back to click so we're not responsible anymore for maintaining it, but I don't know if they are intested and willing to take it. If anybody has time to look into it and possibly make a PR to them, it would be great - we would also learn if what we do is the "right" way of doing it, and maybe they now have something equivalent?)

[danielhollas]

Thanks @sphuber @giovannipizzi ! 🍻