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

unify cylc cli (under click?) #3525

Closed
oliver-sanders opened this issue Mar 16, 2020 · 5 comments
Closed

unify cylc cli (under click?) #3525

oliver-sanders opened this issue Mar 16, 2020 · 5 comments

Comments

@oliver-sanders
Copy link
Member

oliver-sanders commented Mar 16, 2020

Context:

At CylcCon2020 we agreed that the cylc command can live in the cylc-flow repository, other repos can add sub-commands via entry points. That makes things much easier.

This proposal:

addresses #2972
provides solution for #3468 (click has pager functionality)
closes #3488 (as there would be no command re-invocation)

Proposal:

  • Implement the cylc command line using click.MultiCommand:
    • This unifies all Cylc sub-commands under a single interface.
    • Interfaces of subcommands are dynamically loaded via entrypoints.
    • We don't rely on command re-invocation, we defer to click's nested CLI parsing.
  • All sub-commands would need to be converted to click interfaces (which we were going to do anyway).

After a quick play I'm fairly happy this will suit our requirements, we will need to put in a special wrapper to bash sub-commands (if we still need to support them).

Caveats:

  1. Click is strict about how argument work with parent and sub-commands.

    If we want common Cylc options to belong to the parent command then they must be specified
    before the sub-command.

    E.G:

    # correct
    $ cylc --debug run workflow-name
    # incorrect
    $ cylc run workflow-name --debug

    This is strictly correct but also strictly annoying, working around this would involve something similar to the current cylc.flow.option_parser module (which is what we are trying to get rid of).

  2. cylc help --all would involve importing every supported command via entry points, no more formatting text from a dictionary. I don't think this is an issue.

  3. As far as I can tell Click is for command line interfaces, not Python interfaces, in Cylc9 we will be opening up Python interfaces to the extent of cylc.run('my-workflow'), it would be nice if we had a nice way to achieve consistency of arguments and documentation between to two interfaces.

(1) is annoying but we can workaround it.
(2) isn't an issue.
(3) is bad unless we can find a nice way of opening a Python interface to click commands.

@oliver-sanders oliver-sanders added this to the cylc-8.0.0 milestone Mar 16, 2020
@oliver-sanders oliver-sanders added the question Flag this as a question for the next Cylc project meeting. label Mar 16, 2020
@TomekTrzeciak
Copy link
Contributor

  1. As far as I can tell Click is for command line interfaces, not Python interfaces, in Cylc9 we will be opening up Python interfaces to the extent of cylc.run('my-workflow'), it would be nice if we had a nice way to achieve consistency of arguments and documentation between to two interfaces.

I thought this wasn't too important for Cylc - #2802 (comment) - but if it is, then you may give Clize another look before you're too invested in Click. Having CLIs, while retaining the underlying Python interfaces and documentation that drive them, is one of the things that Clize makes quite easy.

It may also help with point 1 in your list - you can use decorators and/or special annotations to inject additional options that are common to your CLIs. We use that for example to handle --output option in a common fashion across IMPROVER CLIs.

@oliver-sanders oliver-sanders mentioned this issue Mar 19, 2020
9 tasks
@oliver-sanders
Copy link
Member Author

oliver-sanders commented Mar 19, 2020

I thought this wasn't too important for Cylc

It might not be, truth is, we haven't really got a grapple on what the Cylc9 Python interfaces will be or look like yet.

It makes good sense to be able to run workflows via a Python API I think. That way you could play about with the Scheduler object, say in a Jupyter notebook. Infact you could actually compose a Cylc workflow into a Python program, doesn't sound like a good idea but it would be possible. Would certainly be nice for testing at least.

Perhaps stuff like this:

from cylc import flow

Foo = flow.Task('Foo', 'foo-script')
Bar = flow.Task('Bar', 'bar-script')

with flow.Flow() as flow:
    with flow.every(1):  # R/P1
        Foo >> Bar

schd = flow.Scheduler(flow, icp=1))
schd.Trigger(Foo)  # (re)trigger Foo
schd.wait()  # wait for workflow to complete
schd.log(Foo, 1, file='stdout')  # access log files etc

So after a quick think I guess we wouldn't really want to open up Python interfaces to runtime commands because you would do that via the Scheduler object (or some remote proxy for one).

That leaves us with the offline commands which are mostly of little interest except validate which should be performed on a configuration object.

tldr; to answer my own question, I don't think we need dual-purpose CLI/Python endpoints, does anyone have any different thoughts.

@hjoliver
Copy link
Member

hjoliver commented Aug 4, 2020

Needs to be researched by someone!

  • stick with click and separately define our Python API
  • or find an alternative that does both and satisfies all our needs?

@TomekTrzeciak
Copy link
Contributor

Needs to be researched by someone!

  • stick with click and separately define our Python API
  • or find an alternative that does both and satisfies all our needs?

I browsed through the click code a bit since my last post here. I think it should be quite easy to achieve API/CLI combo with click similarly to what clize offers, if you want. click.command returns an object with the original function accessible via callback attribute, so you could easily get it back, e.g.:

def unclick(obj):
    f = obj.callback
    f.as_click_command = obj
    return f

@unclick
@click.command()
def api_func(*args, **kwargs):
    ...

if __name__ == "__main__":
    api_func.as_click_command()

@oliver-sanders oliver-sanders changed the title unify cylc cli under click unify cylc cli (under click?) Dec 11, 2020
@oliver-sanders
Copy link
Member Author

Superseding this issue with #4231

  • Changing the CLI framework is now fairly low priority.
  • The appropriate framework is not so obvious.
  • The performance benefits of multiple parsers outweigh the bonuses of a unified parser.

@oliver-sanders oliver-sanders removed this from the cylc-8.0.0 milestone May 26, 2021
@oliver-sanders oliver-sanders added superseded and removed question Flag this as a question for the next Cylc project meeting. labels May 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants