cylc-flow icon indicating copy to clipboard operation
cylc-flow copied to clipboard

bash completion

Open oliver-sanders opened this issue 2 years ago • 6 comments

Closes #4566

Surprisingly responsive auto-completion for Bash with scope for extension to other shells.

bash-completion

The POC was so straight forward it only took a few lines to turn it into a fully-functioning prototype!

A new Bash completion script which completes:

  • Sub-commands.
  • Command options.
  • Workflow IDs.
  • Cycle points.
  • Tasks.
  • Jobs.
  • A small selection of other things...
$ cylc c<tab>
cat-log            clean              completion-server  cycle-point
check-versions     client             config             
$ cylc cat-log --<tab>
--file           --geditor        --mode           --rotation
--force-remote   --help           --remote-arg     --submit-number
$ cylc cat-log --mode=cat <tab><tab><tab>
code-tests/r12345/run1//     cylctb-xatzd3w/run1//
code-tests/r12345/run2//     cylctb-xxF2lYD//
code-tests/r23456/run1//     mi-aa001/aircraft_global//
code-tests/r34567/run1//     mi-aa001/aircraft_ukv//
code-tests/r34567/run2//     mi-aa001/land_surface_ukv//
code-tests/r34567/run3//     mi-aa001/marine_global//
cylctb-xCgu_Hq//             one//
cylctb-xIYJwFM//             
$ cylc cat-log --mode=cat one//1/foo/<tab>
one//1/foo/01/  one//1/foo/NN/  
$ cylc cat-log --mode=cat one//1/foo/NN  # woohoo!

How it works:

  • The "completions" are provided by the cylc completion-server.
  • When you first type cylc<tab> a completion server is started up as a Bash "coprocess".
  • Coprocesses are process you can write to and read from at the same time.
  • When you press <tab> the CLI string is sent to the server (by writing to it).
  • The completion script then dumps its responses from the server into the format Bash is expecting.
  • The first completion is slow (because you have to wait for the server to start).
  • All subsequent completions are fast (because the server is already running).

Advantages of the coprocess approach:

  • Write the business logic in Python not Bash!
  • Use existing Cylc interfaces in the completion (e.g. cylc scan, ID parsing, run dir location)
  • Test completions using pytest.
  • Scanning is quick without the need to start a new Python process / load the Cylc codebase.
  • The same completion server can provide completions for different shells (Bash, zsh, Fish, etc).

Note: Bash 4.2+

Requirements check-list

  • [x] I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • [x] Contains logically grouped changes (else tidy your branch by rebase).
  • [x] Does not contain off-topic changes (use other PRs for other changes).
  • [x] Applied any dependency changes to both setup.cfg and conda-environment.yml.
  • [x] Appropriate tests are included (100% coverage)
  • [x] No change log entry required (why? e.g. invisible to users).
  • [x] No documentation update required.

oliver-sanders avatar Jun 27 '22 16:06 oliver-sanders

completion of task IDs goes down to submit number, although that usually isn't valid

This would require the completion to introspect Cylc commands to see what kind of IDs they are expecting (i.e. workflow, cycle, task or job). Possible but tricky (and a bit of a flaw in the existing CLI ATM), for the moment it just completes the ID as far as the user takes it.

can it be made to handle latest run inference? If I type cylc trigger foo/run2// it works, but not foo//

Hmm, does this mean the trailing // in foo//disables run inferrence? If so that's probably something to fix in the ID parser.

[edit] I think inferrence is working correctly for me, what are you seeing?

$ cylc trigger foo//
InputError: IDs must be tasks  # valid error
$ cylc broadcast baz//
WorkflowStopped: baz/run2 is not running  # successful inference

could it print a message for commands than can take arguments beyond what the completion suggests

The completion server does not "print", it returns a list of possibilities to the shell which the shell then formats for the user. So every item we return must be a valid completion.

That said we can print to stderr and the completion will do this on startup e.g. if a new version of the completion script is available, however, I've kept this minimal.

oliver-sanders avatar Aug 11 '22 10:08 oliver-sanders

3f011d8 handles the issue where the cylc completion-server command returns 1 undetectably (e.g. if your wrapper script is pointing at Cylc 7 rather than Cylc 8).

This is the nicest solution I could come up with. When you try to get completions with a Cylc 7 environment the completion-server will fail to launch and you'll get a bg notification to that effect which serves as a reminder that completion won't work here. When you switch to a Cylc 8 environment the server will start up the next time you ask for a completion.

To test this try:

. cylc/flow/cylc-completion.bash

export CYLC_VERSION=7
cylc <tab><tab>
# completions should fail

export CYLC_VERSION=8
cylc <tab><tab>
# completion should start working

oliver-sanders avatar Aug 18 '22 15:08 oliver-sanders

Tests seem to be failing for arbitrary sorting reasons I can't reproduce locally (probably relying on fs listing order?), will sort that out.

oliver-sanders avatar Aug 18 '22 15:08 oliver-sanders

(rebased)

oliver-sanders avatar Aug 31 '22 13:08 oliver-sanders

(I'll take another look at this again soon - I'm keen to use it)

hjoliver avatar Sep 15 '22 06:09 hjoliver

(ditto, major timesaver)

oliver-sanders avatar Sep 15 '22 10:09 oliver-sanders

Addressed last review point, rebased.

oliver-sanders avatar Oct 18 '22 10:10 oliver-sanders

(I had a good play with this yesterday, continuing today...)

hjoliver avatar Oct 18 '22 21:10 hjoliver

[edit] I think inferrence is working correctly for me, what are you seeing?

If I deliberately terminate the commandline with // after the workflow name and then hit TAB, the server doesn't suggest completions based on a runN inference.

$ cylc scan --state=all -n bug
bug/run1
bug/run2 niwa-1007885.niwa.local:43018

$ cylc trigger bug/run2//<TAB>
bug/run2//1/  bug/run2//3/  bug/run2//5/  bug/run2//7/  
bug/run2//2/  bug/run2//4/  bug/run2//6/  

$ cylc trigger bug//<TAB>
<NOTHING>

Not a big deal at all, since starting to TAB at the name forces me down the explicit run-number route. But a possible small follow-up perhaps.

hjoliver avatar Oct 19 '22 04:10 hjoliver

I'll comment below on several small points,

So, I think my original points are still valid (but can be addressed as follow-ups if at all):

Completion of task IDs goes down to submit number, although that usually isn't valid

This would require the completion to introspect Cylc commands to see what kind of IDs they are expecting (i.e. workflow, cycle, task or job). Possible but tricky (and a bit of a flaw in the existing CLI ATM), for the moment it just completes the ID as far as the user takes it.

Fair enough, but I think users could reasonably expect the completions to be valid command lines even if they hit TAB one too many times, so it could be confusing. Introspection would be ideal, but I don't think it would be too kludgy in this case to simply categorize commands according to ID type in the completion server code. There are only a few types and this information will change very infrequently if at all.

can it be made to handle latest run inference? If I type cylc trigger foo/run2// it works, but not foo//

(I explained this just above)

could it print a message for commands than can take arguments beyond what the completion suggests (e.g. trigger completes for existing cycle points and tasks, but the command can target any cycle point and task). Or does everything it prints have to be a potential command line?

every item we return must be a valid completion.

That said we can print to stderr and the completion will do this on startup e.g. if a new version of the completion script is available, however, I've kept this minimal.

Potential user confusion again as commands that can target future tasks (e.g. hold) may not be wanted for the past and current task instances that the server returns. So perhaps we could print a warning to stderr to indicate that future tasks are valid as well as the provided completions?

hjoliver avatar Oct 19 '22 04:10 hjoliver

A final small thing: cylc scan shouldn't get completed with IDs.

hjoliver avatar Oct 19 '22 05:10 hjoliver

If I deliberately terminate the commandline with // after the workflow name and then hit TAB, the server doesn't suggest completions based on a runN inference.

I don't think it's possible to do this.

Shell completions can only add characters, not remove them. So if you type:

cylc trigger foo//<TAB>

The completion cannot replace foo// with foo/run1// because that would involve replacing existing characters.

Also it would involve mixing workflow ID suggestions with cycle point suggestions.

oliver-sanders avatar Oct 20 '22 09:10 oliver-sanders

A final small thing: cylc scan shouldn't get completed with IDs.

Have added exceptions for scan, cycle-point and message to prevent argument completion for these commands.

oliver-sanders avatar Oct 20 '22 10:10 oliver-sanders

The completion cannot replace foo// with foo/run1// because that would involve replacing existing characters.

That's not what I meant.

In principle the server could figure out that runN is implied, and list completions appropriate for the actual run, but without making it explicit:

cylc blah foo/run1//<TAB>  ---> foo/run1//1   
# or
cylc blah foo//<TAB> ---> foo//1  

hjoliver avatar Oct 20 '22 10:10 hjoliver

Right, sorted.

oliver-sanders avatar Oct 20 '22 14:10 oliver-sanders

Sorted and tested:

$ cylc trigger generic/run2//20191209T
generic/run2//20191209T0900Z/  generic/run2//20191209T1200Z/
$ cylc trigger generic//20191209T
generic//20191209T0900Z/  generic//20191209T1200Z/  

oliver-sanders avatar Oct 25 '22 09:10 oliver-sanders

These are both features which will be released at the same time so I suspect this wouldn't be a problem.

Hadn't thought of cylc gui but it can be easily excluded if needed, but as you say, these should both be going into the same PR so should be good.

oliver-sanders avatar Nov 01 '22 16:11 oliver-sanders

in that case ... :boom:

hjoliver avatar Nov 01 '22 20:11 hjoliver