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

CLI refactor #2972

Closed
oliver-sanders opened this issue Feb 28, 2019 · 24 comments
Closed

CLI refactor #2972

oliver-sanders opened this issue Feb 28, 2019 · 24 comments
Assignees
Milestone

Comments

@oliver-sanders
Copy link
Member

oliver-sanders commented Feb 28, 2019

With Cylc8 and the upgrade to Python3 (which brings with it argparse) we have the opportunity (read excuse) to make some meaningful changes to the CLI.

This issue is for collating (and hashing out the details of) changes to make.

My suggestions:

  1. Use argparse Click argparse?
  2. Migrate Cylc subcommands to Python - Move to entry points #3413
    • Ao we can use argparse Click for them all.
    • Perhaps controversial, we may want to exclude internal cylc commands for performance reasons.
    • Move the scripts into cylc.cli or something of that ilk.
    • This ties in with Bruno's packaging efforts.
  3. Use argparse Click to implement cylc subcommands as proper subcommands.
  4. Use argument groups to rationalise help pages.
    • Lots of commands group options, pytest is a good example.
    • E.g. we could have a format group in which to group options like --format=json, --terse and --color=never.
  5. Collect the various "color" options (done)
    • --bold, --no-bold, --color, --no-color
    • Make them orthogonal and standardise them across cylc subcommands using the standard(ish) convention --color=WHEN.
    • Use colour automatically when the terminal supports it.
    • Make more use of colour (tastefully not just for the sake of it) in user commands.
    • Use colour in Cylc loggers when output to a terminal? (e.g. grey for debug, orange for warnings, red for errors, purple for critical)
    • See the changes to cylc scan on my Python3 branch.
  6. Collect the various "format" options
    • --raw, --plain, --json, --terse
    • Make them orthogonal and standardise them across cylc subcommands as --format=FMT, -t FMT.
    • See the changes to cylc scan on my Python3 branch.
  7. Consider moving some options onto the cylc command itself rather than the subcommands.
    • remrun functionality now removed, no longer a concern.
    • --host might be a good candidate i.e. cylc --host=myhost subcommand --subcmdoption.
    • I think this may allow us to perform re-invocation before parsing the sub-command options.
  8. Implement a simple batch mode on the cylc command
    Superseded by global universal identifier #3931
        $ cylc -s foo bar baz/* stop --kill
        cylc stop foo --kill
        cylc stop bar --kill
        cylc stop baz/bool --kill
        cylc stop baz/been --kill
  9. Unify --debug and --verbose: - cli: verbosity #4214
    • Make it so that verbosity can be incremented or decremented -v / -q
    • Use logging levels for this, i.e. cylc -v would get you logging.DEBUG, cylc -q would get you logging.WARNING.
    • Make --debug shorthand for --verbosity=DEBUG.
  10. Consider building a Cylc man page (we can do this with Sphinx!)

We should probably do this along with #1249 & #2928

Thoughts? Suggestions?

@oliver-sanders oliver-sanders added this to the cylc-8.0.0 milestone Feb 28, 2019
@matthewrmshin
Copy link
Contributor

On point 8. A syntax to allow control of multiple suites is good, but I'm not sure I understand the proposed syntax. A simple (quoted) glob to replace the current SUITE positional argument is probably the least disruptive?

The other points are good. It may be worth noting that argparse has an issue with mixing options and positional arguments ordering until Python 3.7.

@hjoliver
Copy link
Member

Yes, it would be great to have this done. Some of this we've wanted for ages, but there are also some good new ideas that I hadn't thought of. My only concern is how long this might distract from the core cylc-8 implementation tasks (since a CLI refactor is more highly desirable than critical). I suppose some parts of your proposal could be done later, however, ... such as better use of colour. However, if its easy enough and quick enough, let's do it!

@kinow
Copy link
Member

kinow commented Feb 28, 2019

I think this will have a positive impact on the code base, and will definitely help for packaging and also for testing. +1 and thanks for starting the task!

@oliver-sanders
Copy link
Member Author

I think with the exception of 7 this should all be pretty quick to implement.

A simple (quoted) glob to replace the current SUITE positional argument is probably the least disruptive

I'm guessing you mean something like this?

$ # usage: cylc show REG TASKID
$ cylc show 'foo bar baz/*' pub.1

That's a pretty simple way to do it.

My logic was:

  • I was thinking that the simplest way to implement batch mode is to do it at the Cylc level rather than having to implement it for each subcommand
  • I can envisage this functionality evolving past simple suite registrations (e.g. more advanced suite grouping cylc --group mygroup show)
  • This way cylc handles the batching at a the "super command" level rather than at the subcommand
    • We don't actually have to parse the CLI options to the subcommand at this stage.
    • Can do nicer batching (e.g. run subcommands via asyncio)

I'm not sure I understand the proposed syntax

usage: cylc [-r REG...] show [REG] [TASKID]

This:

$ cylc -r foo bar show baz.1 --format=json

Would run this:

$ cylc show foo baz.1 --format=json
$ cylc show bar baz.1 --format=json

Or

usage: cylc [-r REG]... show [REG] [TASKID]
$ cylc -r foo -r bar show baz.1 --format=json

Or

usage: cylc [-r REG]... [-g GROUP]... show [REG] [TASKID]

etc

@oliver-sanders
Copy link
Member Author

Matt just mentioned a good use case worth recording here:

Broadcast to all tasks set to run on host_a to run on host_b for all suites

@matthewrmshin
Copy link
Contributor

Some other main usages for multi-suite controls:

  • Stop/Restart all suites for scheduled outage of suite hosts.
  • Hold/Release all suites that run tasks on cluster A for scheduled outage of cluster A.
  • Re-trigger all failed archiving tasks for all suites after unscheduled outage of archiving system.

@oliver-sanders
Copy link
Member Author

From these use cases Cylc would need the ability to select suites by:

  • Name
  • Platform
  • Task name

@oliver-sanders oliver-sanders mentioned this issue Mar 1, 2019
5 tasks
@sadielbartholomew
Copy link
Collaborator

sadielbartholomew commented Mar 1, 2019

My only concern is how long this might distract from the core cylc-8 implementation tasks (since a CLI refactor is more highly desirable than critical). I suppose some parts of your proposal could be done later, however, ... such as better use of colour. However, if its easy enough and quick enough, let's do it!

I think this is an important point. Do we anticipate the CLI will need adjustments during Cylc 8 to fit with the new architecture &/or new control potential & mechanisms from the Web UI? If so, is it not premature to do a CLI revamp at this stage? (I'm just throwing some questions out there, as I am not yet sure where I sit on this from lack of background; I am also wary of complicating our already extensive roadmap.)

Otherwise, some CLI ideas I have that have not yet been covered, which are definitely for later rather than sooner, i.e. for post Cylc 8:

  • User-defined in-built (i.e. for after the cylc base command only) keywords/aliases i.e. stored, recognised values, for:

    • personal easy-to-remember (especially relative to Rosie suite identifier code suite names e.g. mi-ab123 for when rose suite-run is migrated) "nicknames" for suite arguments;
    • a user's commonly-used option sets &/or whole commands (obviously the shell provides aliasing functionality but to keep these isolated within the Cylc CLI & it easier to configure as below);
    • possibly also to store the previous command but to expand upon the shell history capability with argument or option overwrite capability e.g. cylc previous --host=<HOST> would run the previous command except for having a different host set.

    This could be set-up on a per-user basis in the global.rc, say under a new config item, e.g. perhaps we could support something like:

    [command line]
        [[saved commands]]
            <tag, e.g. run_my_suite> = run my_suite --host=localhost --debug
        [[saved option sets]]
            <tag, e.g. test> = --debug --no-detach --reference-test
        [[suite aliases]]
            mi-ay608 = ps_winter_test
            mi-ay609 = ps_winter_10ensembles
    

    where then cylc command_<tag> would run the command set for that tag & cylc <(sub)command> options_<tag> would run a command with the set options.

  • Persistent defaults: similarly to the above, we could allow users to specify a (terminal-)session-long default for very common options such as verbosity or host, so that e.g. in the former case they persistently want as much info as possible from their commands, they do not keep having to specify -v or --debug etc. but can set it once to the standard & override explicitly if necessary.

  • Collect verbose & debug options: these options often produce largely identical output, so as with [5] & [6] from CLI refactor #2972 (comment), collect --verbosity & --debug into one descriptor with several levels as appropriate e.g. have level 0, 1 & 2 respectively with no such option provision (i.e. non-verbose), --verbose, & --debug/doubly-verbose -v -v.

  • Interactive mode: open up a separate command shell where Cylc commands can be run without the need to prepend cylc, perhaps to include additional helpful utilities.

  • and... an Easter Egg. If you type cycl it produces some (scrolling?) ASCII art of someone riding a bike. The user community would benefit immensely, I am sure.

@kinow
Copy link
Member

kinow commented Mar 5, 2019

Do we anticipate the CLI will need adjustments during Cylc 8 to fit with the new architecture &/or new control potential & mechanisms from the Web UI?

Some time ago I created #2802 thinking in the Web UI. And I think this comment in this issue might fix that

  • Migrate Cylc subcommands to Python so we can use argparse for them all
    • Perhaps controversial, we may want to exclude internal cylc commands for performance reasons.
    • Move the scripts into cylc.cli or something of that ilk.
    • This ties in with Bruno's packaging efforts.

Right now we have a prototype UI server in https://github.com/cylc/cylc-jupyterhub, which is able to talk to the JupyterHub and also to REST or GraphQL endpoints. And I am in the progress of trying @dwsutherland 's GraphQL branch.

The GraphQL branch won't have a GraphQL endpoint until I run cylc run some-suite. That should be the "Suite Start Sub-Service: " mentioned in Cylc 8 Architecture.

For now I will run in another terminal cylc run --no-detach --hold some-suite. So that I have time to choose configure the GraphQL client. Another possibility is to execute the cylc run command from cylc-jupyterhub.

The issue is that we will have to have that ugly code to Popen.communicate, then get the bytes, encode, get the string, parse response or just get exit code, check for errors, etc.

Which is doable, but I would prefer to be able to just

  1. add cylc as a dependency to setup.py in cylc-jupyterhub
  2. import the commands called via command line with something like from cylc.cli import run
  3. use the API objects, such as run_response = run('some-suite', nodetach=true, hold=false, ...) or something like that
  4. populate the UI with the content of run_response if necessary, or go to another screen that uses cylc.cli.scan (the "Suite Listing Sub-Service" from Cylc-8 architecture).
  5. if any errors occurred, just log to warn/error/fatal/etc

is it not premature to do a CLI revamp at this stage?

Great question. We could skip it and implement everything calling the commands from command line. But changing this later, would mean to update cylc, package&release, then update in cylc-jupyterhub and modify the code. So we have to keep in mind we will have Cylc as a dependency in other projects.

For a change like this, we could go with a minor release like 8.1 if we consider for a user of cli commands there was no change, but if we consider the other developers relying in Cylc public API, this would have to be a major release like 9.0 - as we could break their code by moving the logic from commands to other places.

So I'm +1 for including this in Cylc 8 😁

User-defined in-built (i.e. for after the cylc base command only) keywords/aliases
Persistent defaults:

I like this idea too! I always run cylc --no-detach --verbose --debug. It would be great to have those parameters persisted in my local environment.

Maybe we could copy git? cylc config to list the configuration. Then cylc config --global cylc.run.args="--no-detach --verbose --debug". Or maybe it would be best to edit the text files manually... I'm fine with any solution as long as it saves me some typing.

Collect verbose & debug options

+1! -v, -vv, -vvv. For users familiar with ssh for instance, that would be quite intuitive. Right now I enable both always (even though some commands I think don't support both?) just to play safe.

Interactive mode

Sounds interesting too!

I think I liked all your suggestions! Except for easter egg, which I am not sure we would be able to properly test 😁 (I'm an old man, no sense of humour, that's fine).

If others agree too, I think this issue could proceed, and your suggestions would become issues blocked by this one :)

@matthewrmshin
Copy link
Contributor

On cylc run|restart command, we'll most likely tackle it fully as part of rose suite-run migration to Cylc. However, most of what we want to do should not conflict with what's being suggested here, so happy to go ahead as well. And yes, the CLI can simply be a thin wrapper to the Python API.

On verbosity, Rose's interface is still my favourite. Starting with verbosity=1. Each -q|--quiet does verbosity-=1 and each -v|--verbose does verbosity+=1. Simple and flexible (but can obviously be a bit pointless!)

@TomekTrzeciak
Copy link
Contributor

I think that argparse, despite being the 3rd CLIs attempt in the standard library, is definitely not the charm. Over-complicated and lots of boiler plate required for anything more substantial. There is a lot of alternatives out there if you're willing to look outside of the standard library.

Personally, I quite like the idea of auto-generating CLIs from function signatures like clize or plac - all you do is write regular Python functions (possibly with some argument annotations) and you can run them as CLIs with no extra work. The latest release of clize adds support for Sphinx doc strings, while with plac it's easy to add interpreter mode for interactive use suggested by @sadielbartholomew.

@kinow
Copy link
Member

kinow commented Mar 6, 2019

I've seen a few projects with click too. Looks simpler, but will be happy to just have moved from optparse :) so fine with argparse or any other library.

@matthewrmshin
Copy link
Contributor

Cool libraries. Let's pick one to use.

@matthewrmshin
Copy link
Contributor

@kinow I noticed your star on python-fire some time ago as well. Looks very good and backed by the big guy.

(All these remind me of Perl 6, which comes with this sort of magic as part of the language. And the good old days when I wrote a CLI parser from scratch to support GNU-style CLI (i.e. long/short options, positional arguments mixed with options, etc) for a Java utility, all configured via dependency injection with a horrible XML file, etc.)

@kinow
Copy link
Member

kinow commented Mar 6, 2019

Yup, python-fire is another good one. I had a look at the code and commented in an issue. They fixed that issue quite quickly later, which indicates the project is still maintained (google must be using that for their Python libraries).

I also thought that AWS could have something, then just checked their Python aws CLI (which is great, and used in so many places). Looks like they are using argparse too (https://github.com/aws/aws-cli/blob/develop/awscli/argparser.py). As well as colorama, a new dependency introduced in the Python3 pull request that I still want to learn about 🤓

@hjoliver
Copy link
Member

hjoliver commented Mar 6, 2019

User-defined in-built (i.e. for after the cylc base command only) keywords/aliases
Persistent defaults:

I like this idea too! I always run cylc --no-detach --verbose --debug. It would be great to have those parameters persisted in my local environment.

@sadielbartholomew and @kinow - I'm not so keen on this idea, sorry! Given this is the shell CLI, users can do this sort of thing themselves with bash aliases and shell variables defined in their login scripts, right? Plus, I have seen users who alias standard commands to their own "language" to such an extent that they end up unable to work in a standard environment, or to communicate with others who do. Built-in support for command aliasing and option grouping etc. might just encourage this sort of thing!

I think storing command history is also best left to the shell. Bash and fish, for instance, allow you to view and re-run previous commands very easily.

@kinow
Copy link
Member

kinow commented Mar 6, 2019

Sounds like a good argument to me, especially given there's a workaround. Will update my aliases list to include a cylc=cylc --no-detach --debug... 🎉

@hjoliver
Copy link
Member

hjoliver commented Mar 6, 2019

Interactive mode: open up a ...

When we have "cylc as a module" we can just use the interactive Python interpreter. Or Jupyter Notebook.

@hjoliver
Copy link
Member

hjoliver commented Mar 6, 2019

So I'm +1 for including this in Cylc 8

Agreed (with a few exceptions noted above).

Do others agree with @TomekTrzeciak that stdlib argparse is not the best route for CLI? I don't mind new 3rd party dependencies if the benefit is clear (that's much easier with proper packaging at least) but we have to be a bit careful to not to end up relying on some dead-end project if we diverge from the standard library.

@matthewrmshin
Copy link
Contributor

Agreed that we should choose a 3rd party solution, carefully.

(@wxtim has shown me yet another problem with argparse earlier, so I am now fully convinced that it is NOT the way forward. Really, how hard can it be to write a good CLI parsing framework?)

@oliver-sanders
Copy link
Member Author

to such an extent that they end up unable to work in a standard environment

I see this a lot too. Over customisation is the root of a large number of user issues. Keep it simple, keep it vanilla.

@sadielbartholomew
Copy link
Collaborator

I'm not so keen on this idea, sorry!

Fair enough @hjoliver, I follow your reasoning & though I think it can be done in a more pared-down way than I hastily wrote up as an example, to avoid those issues, it doesn't seem like the best use of time and effort in terms of new features and functionality etc.

Do others agree with @TomekTrzeciak that stdlib argparse is not the best route for CLI?

Yes, I strongly agree. We shouldn't stick with argparse just because it is an improvement on optparse, & there seem to be lots of known issues with it for more involved CLIs. Also agree that a careful decision is required as to which, though.

@hjoliver
Copy link
Member

hjoliver commented Apr 7, 2019

A point in favour of argparse: Sphinx integration #3037 (comment)

@oliver-sanders
Copy link
Member Author

This is mostly done now, punting the remaining parts to other issues in the relevant repositories.

Quick summary of the 10 points above:

  1. Use argparse Click argparse?

    Now low priority, bumped to cli: migrate away from optparse #4231

  2. Migrate Cylc subcommands to Python

    Done Move to entry points #3413 + later work

  3. Use argparse Click to implement cylc subcommands as proper subcommands.

    See (1)

  4. Use argument groups to rationalise help pages.

    CLI greatly simplified, no longer really necessary, can implement on a case-by-case basis if required.

  5. Collect the various "color" options

    Done

  6. Collect the various "format" options

    Main offenders (namely cylc scan) tackled.

  7. Consider moving some options onto the cylc command itself rather than the subcommands.

    • remrun functionality now removed, no longer a concern.
    • Performance cost would be too high, consider plugins plugin framework #4106
  8. Implement a simple batch mode on the cylc command

    Superseded by global universal identifier #3931
    $ cylc -s foo bar baz/* stop --kill
    cylc stop foo --kill
    cylc stop bar --kill
    cylc stop baz/bool --kill
    cylc stop baz/been --kill

  9. Make it so that verbosity can be incremented or decremented -v / -q

    Done - cli: verbosity #4214

  10. Consider building a Cylc man page (we can do this with Sphinx!)

    man page cylc-doc#245

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

6 participants