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

Convert CLI commands to use Click #22613

Closed
wants to merge 46 commits into from

Conversation

blag
Copy link
Contributor

@blag blag commented Mar 30, 2022

PR status

RFC - gauging interest and feedback before continuing All systems feedback are GO! 🚀

This PR converts the airflow db CLI to using the Click and Rich frameworks with rich-click (note that I refer to the combination here as just "Click"). It adds a temporary airflow-ng console script entry point to enable easier/better comparisons between the existing airflow console script entry point and the airflow-ng one.

We will need to convert all the others subcommands, and I don't know of a good way to do so except converting them all at the same time.

How You Can Help

If you'd like to assist this conversion process, check out #22708 to see what still needs to be done. If you'd like to contribute, drop a comment in #22708 letting us know what you're working on.

Please open PRs against this astronomer:convert-to-click-cli branch, that enables us to propose all of these conversions in one go while letting Git/GitHub keep track of properly attributing commits from all contributors.

Coding Notes

We don't need to use absolutely every feature of Click out of the gate, since this PR is more about converting over to using Click. Fancifying the CLI and using more advanced features of Click (I'm specifically thinking of progress bars, which AFAIK the current CLI doesn't handle at all) can be done in future PRs.

TL;DR

  1. Copy airflow/cli/commands/<subcommand>_command.py to airflow/cli/commands/<subcommand>.py and edit the new file.
  2. import rich_click as click
  3. airflow <subcommand> --help or airflow <group> --help and airflow <group> <subcommand> --help and check the flags.
  4. Convert the subcommand to Click, and import any common/shared options from airflow/cli/__init__.py
  5. Import your subcommand in airflow/cli/__main__.py - note that you don't have to actually use it anywhere, and you should add the # noqa: F401 to prevent flake8 from failing
  6. airflow-ng <subcommand> --help or airflow <group> --help and airflow-ng <group> <subcommand> --help to check that all of the flags match.

Walkthrough

We're using Rich-Click, which seems to just wrap the Click API, so:

import rich_click as click

Convert the subcommand, checking the Click documentation as you need. Keep in mind that there are common/shared predefined in airflow/cli/__init__.py for you, but you can also add Click options directly to your subcommand if the argument/option is only used in one command.

The args object will simply go away and the function will accept normal Python arguments. Default values should be provided in the Click decorator and not in the Python function definition (I think this is the intent of the Click developers, but I'm not 100% sure so if you can confirm this please let me know). Do note that if your subcommand uses the --daemon option, the function argument is named daemon_ to differentiate it from the daemon module.

You'll need to import your new subcommand module in airflow/cli/__main__.py to get Click to register it, but you don't need to actually use the imported module anywhere. And because of that oddity, you'll need to add a # noqa: F401 comment to the import line to prevent flake8 from complaining.

Use docstrings to your advantage! If your function docstring contains one initial line, two newlines, and more text like this:

def upgrade(...):
    """
    Upgrade the metadata database to latest version

    Upgrade the schema of the metadata database.
    To print but not execute commands, use option ``--show-sql-only``.
    If using options ``--from-revision`` or ``--from-version``, you must also use
    ``--show-sql-only``, because if actually *running* migrations, we should only
    migrate from the *current* revision.
    """
    ...

that will get rendered similar to this, where the first line of the docstring is displayed in a brighter color than the rest of the description:

Screen Shot 2022-03-29 at 9 29 21 PM

To get automatic Rich highlighting, you should convert any print calls to console.print calls.

To use your converted subcommand, use the airflow-ng (entry point) command. This will eventually replace the previous airflow (entry point) command in a future release of Airflow, but I've kept it separate for now to make it easier to compare them and check for reverse compatibility.

Testing

I haven't dug super far into the existing tests yet, but the CLI does have at least has some automated test coverage already in tests/cli/test_cli_parser.py and tests/cli/commands/.

If there is some functionality that isn't currently covered by tests and would be easy to write, we should do that. Additionally, if our tests would be improved by using some of the features from Click, that would be good implement.

If somebody has a good way to test the CLI at the Bash layer, I would be interested but that might be outside the scope of this PR (might be good for future work though), and tests like that would be super helpful if we ever convert the CLI to use something else in the future as well (not that I see that happening, and until then those tests would be redundant and possibly brittle).

Besides that, all subcommands should be manually tested by one person, and ideally by two people. The sign up for that is also in issue #22708 although I think the results of manual testing would be best given here in a comment.

Examples

Other potentially helpful links

Motivation

While argparse is quite extensive in what it supports, third party frameworks like Click have faster development cycles and therefore have implemented 21st century features that argparse does not, and likely will not, support. Things like a simple Python API, automatic coloring for --help output, use of non-symbolic ASCII characters, and a better understanding of common CLI author needs are all available with Click but aren't easily supported in argparse.

With the Rich framework, it is trivial for command authors to output text formatted with colors and other font effects.

The existing argparse implementation is a bit of a mess with ARG_ variables for DRY, and diving into the CLI commands isn't very easy. This Click implementation also creates some examples for future conversions:

  • a globally reusable option (the click_yes decorator)
  • some options that are reused within a single module (click_revision, click_version, click_from_revision, click_from_version, click_show_sql_only)
  • a decorator that checks for issues with common options between the db upgrade and db downgrade subcommands

All other options are implemented as decorators for for individual functions, illustrating how to add options to subcommands in many different ways that might make sense. You can also define options that apply to command groups.

Since most options are bespoke to their subcommand, it makes sense to me that they should be defined closer to where they are used instead of all in a single file. Adding options to a command via decorators makes this very easy to do.

Reverse Compatibility

I believe we can largely, if not entirely, keep reverse compatibility with the previous argparse-based CLI. However, if we can't completely maintain reverse compatibility, this might be a better change to implement in Airflow 3.0.

Comparison

We get some things "for free" by converting over to Click instead of argparse.

Formatting

  • Note the descriptions correctly rewrapping in the airflow db upgrade --help example as opposed to the existing argparse implementation.
  • The different sections of command output are rendered in their own ASCII boxes, including command errors.

Colors!

Click automatically colorizes output without any additional hinting.

  • Note how the use of colors separates pieces of information like:
    • The long-form option
    • The short-form option
    • The type of the option value
  • Colors also draw your attention to the most important part of command output in a context-dependent manner
    • Note how the names of the different subcommands are in bright blue, and the error output is easily recognizable in a red ASCII box.
    • Some warnings (not shown) are also displayed in yellow text to differentiate them from normal output and error output, although this really depends on when the warnings are emitted at import time.

airflow db --help

Click to expand before/after

Before

Screen Shot 2022-03-29 at 9 24 38 PM

After

Screen Shot 2022-03-29 at 9 25 47 PM

airflow db upgrade --help

Click to expand before/after

Before

Screen Shot 2022-03-29 at 9 37 03 PM

After

Screen Shot 2022-03-29 at 9 29 21 PM

airflow db upgrade --hep (misspelled option)

Click to expand before/after

Before

Screen Shot 2022-03-29 at 10 05 44 PM

After

Screen Shot 2022-03-29 at 10 05 17 PM

Future Work

Pre-Commit Hooks To Check For Non-Local Imports

To help keep subcommands fast, on top of delaying non-local imports for as long as possible, we should also add a pre-commit hook similar to the pre-commit hook for Breeze that checks imports for all Click-based commands.

Command Grouping

We can further separate the airflow subcommands into different groups so they are rendered into ASCII boxes with their group label (see the "Groups and sorting" section of the rich-click documentation).

Styling Customization

Some Airflow vendors may customize the airflow command. The rich-click library allows for extensive style customization, and we can also use a Console() singleton to keep theme/console settings in a single place, and introduce a color-blind friendly mode.

Click Goodies

  • The Click API makes it easier to test the CLI programmatically entirely within Python.
  • Autocomplete with Click looks like it would be pretty easy to implement.
  • Progress bars - imagine launching a DAG via the CLI and then viewing a progress bar while it finishes.

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@potiuk
Copy link
Member

potiuk commented Mar 30, 2022

I am all for it - even in Airflow 2. We are using click, rich and rich-click essentially in all "development" tools (including the new python based Breeze2 framework) and it works very well.

@potiuk
Copy link
Member

potiuk commented Mar 30, 2022

I think rich is flexible enough to give us full compatibility.

There are few watchouts though:

  1. performance - some of our commands require to import a lot of stuff and they are lazy initialized

  2. auito-complete - there are some quirks with auto-complete for rich and rich-click which we have not solved yet entirely in Breeze 2 Breeze2 autocomplete requires click-complete to be installed #21164 - basically seems that you need to activate virtualenv with rich and rich-click and click-complete for the autocomplete to work, which is pretty unfortunate (but maybe we can find some workarounds and maybe this is not an issue for Airflow as we can assume venv of airflow with rtch and rich-click will be activated before.
    But it might be a problem when airflow is installed via pipx which is more and more likely as people learn the benefits of pipx for installing standalone tools.

@kaxil
Copy link
Member

kaxil commented Mar 30, 2022

Love it Drew <3 -- finally we will be getting rid of argparse and moving on to click + rich 🚀

@potiuk
Copy link
Member

potiuk commented Mar 30, 2022

My proposal will be to indeed implement a parallel "airflow2" entrypoint and gradually add new features there. We could even release it as experimental feature in 2.4 and let people use it for a while and then replace the old "airflow" entrypoint with it in 2.5 or 3.

@blag blag force-pushed the convert-to-click-cli branch 5 times, most recently from 1b82ffa to f1da599 Compare April 1, 2022 19:43
@potiuk
Copy link
Member

potiuk commented Apr 1, 2022

FYI @blag - it turned out that the auto-complete problem in Breeze2 was because we used some old dependency. Click nicely supports auto-complete on it's own. See #22695

@potiuk
Copy link
Member

potiuk commented Apr 1, 2022

We even added automated "setup-autocomplete" command that automatically sets the auto-complete sourcing for the user in zsh/bash/fish :)

@blag blag force-pushed the convert-to-click-cli branch 2 times, most recently from 03f1dd3 to 0bb137c Compare April 2, 2022 04:14
@Bowrna
Copy link
Contributor

Bowrna commented Apr 2, 2022

If I could do anything useful to this part of converting CLI commands to use click, please let me know. :) I am happy to take part

@potiuk
Copy link
Member

potiuk commented Apr 2, 2022

Yeah @blag - the task of CLI conversion seems like "perfect" to split it and get others involved :)

@blag
Copy link
Contributor Author

blag commented Apr 2, 2022

Every time I think I've got all of the shared flags converted I convert another command and find another flag that I need, or a better way to define flags. 😆 And I didn't want to open a tracking issue yesterday, since it was April Fools Day. I'll do so in a few minutes. Thank you for volunteering to help!

@blag blag mentioned this pull request Apr 2, 2022
27 tasks
@blag
Copy link
Contributor Author

blag commented Apr 2, 2022

Okay, tracking issue is #22708, to avoid duplicating effort and communicate what you're working on, please drop a comment in that as to what commands you're going to work on converting. Feel free to just start working on a command, don't wait for me to officially add you or anything.

I'll update the PR description to be give a bit more guidance to other contributors in this effort.

@blag blag force-pushed the convert-to-click-cli branch from 0bb137c to a8a5be7 Compare April 2, 2022 21:53
@blag
Copy link
Contributor Author

blag commented Apr 2, 2022

Updated the PR description with a lot more info, please let me know if there's anything useful that I forgot to add.

@Bowrna Feel free to drop a comment as to what you can help with in #22708 and start work, you can submit your PR against this branch instead of the normal apache:main, that way it keeps the changes to all of the subcommands in one PR. Feel free to ask any development or testing related questions here.

@tirkarthi
Copy link
Contributor

The imports might need an optimization as I see noticeable slowdown and click based cli having more imports. You can use importtime to see the import tree as it's large to paste : https://docs.python.org/3/using/cmdline.html#envvar-PYTHONPROFILEIMPORTTIME . The timings are on Python 3.10

(.env) ➜  airflow git:(cli-roles) ✗ python -X importtime -m airflow.cli 2>&1 | grep 'import time' | wc
   2764   19350  230153
(.env) ➜  airflow git:(cli-roles) ✗ python -X importtime -m airflow 2>&1 | grep 'import time' | wc   
    614    4300   40655

@blag blag force-pushed the convert-to-click-cli branch from a3441b9 to c646748 Compare June 8, 2022 07:46
@blag blag marked this pull request as ready for review June 8, 2022 21:27
@blag
Copy link
Contributor Author

blag commented Jun 8, 2022

Converting all airflow subcommands to using Click isn't yet 100% finished, but this is in such a state that I think the groundwork has largely been laid, and has somewhat solidified. I'd like to get some review on the progress so far even though tests will probably fail and possibly merge it in before it's complete. My plan is that after it's eventually merged, I and other contributors can open PRs directly against apache/airflow instead of against this PR branch.

Special thanks (so far) to:

for their code contributions and good discussions.

kaxil pushed a commit that referenced this pull request Jun 23, 2022
This is the first PR of what will be a series of PRs breaking up #22613 into smaller, more reviewable chunks. The end result will be rewriting the existing `airflow` CLI to use Click instead of argparse. For motivation, please see #22708.

This PR installs Click, adds constraints to Rich_Click so we can rely on some nice features in recent versions of that, adds a new barebones `airflow-ng` console script, and tweaks some CLI internals to be more flexible between argparse and Click.

To see how this initial groundwork will be used by future PRs, see #22613, and to see how some of this will be used please see #24591.
@blag
Copy link
Contributor Author

blag commented Jul 1, 2022

From this discussion, there is more groundwork and research necessary before we can evaluate rewriting the CLI with Click.

@blag blag closed this Jul 1, 2022
@blag blag deleted the convert-to-click-cli branch July 1, 2022 21:46
@hamdanal
Copy link

hamdanal commented Jan 7, 2023

BTW I wrote https://github.com/hamdanal/rich-argparse a while ago and I just released the stable 1.0 today. If you are interested in getting rich colors in the CLI help text without a full re-write of the CLI, it is as simple as adding formatter_class=RichHelpFormatter to your argparse.ArgumentParser initialization.

@potiuk
Copy link
Member

potiuk commented Jan 7, 2023

BTW I wrote https://github.com/hamdanal/rich-argparse a while ago and I just released the stable 1.0 today. If you are interested in getting rich colors in the CLI help text without a full re-write of the CLI, it is as simple as adding formatter_class=RichHelpFormatter to your argparse.ArgumentParser initialization.

Might be a good idea

@potiuk
Copy link
Member

potiuk commented Jan 7, 2023

Issue created: #28789

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

Successfully merging this pull request may close these issues.

7 participants