-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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
Conversation
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. |
I think rich is flexible enough to give us full compatibility. There are few watchouts though:
|
Love it Drew <3 -- finally we will be getting rid of argparse and moving on to click + rich 🚀 |
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. |
1b82ffa
to
f1da599
Compare
We even added automated "setup-autocomplete" command that automatically sets the auto-complete sourcing for the user in zsh/bash/fish :) |
03f1dd3
to
0bb137c
Compare
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 |
Yeah @blag - the task of CLI conversion seems like "perfect" to split it and get others involved :) |
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! |
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. |
0bb137c
to
a8a5be7
Compare
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 |
The imports might need an optimization as I see noticeable slowdown and click based cli having more imports. You can use
|
a3441b9
to
c646748
Compare
Converting all Special thanks (so far) to: for their code contributions and good discussions. |
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.
From this discussion, there is more groundwork and research necessary before we can evaluate rewriting the CLI with Click. |
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 |
Might be a good idea |
Issue created: #28789 |
PR status
RFC - gauging interest and feedback before continuingAllsystemsfeedback are GO! 🚀This PR converts the
airflow db
CLI to using the Click and Rich frameworks withrich-click
(note that I refer to the combination here as just "Click"). It adds a temporaryairflow-ng
console script entry point to enable easier/better comparisons between the existingairflow
console script entry point and theairflow-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
airflow/cli/commands/<subcommand>_command.py
toairflow/cli/commands/<subcommand>.py
and edit the new file.airflow <subcommand> --help
orairflow <group> --help
andairflow <group> <subcommand> --help
and check the flags.airflow/cli/__init__.py
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 failingairflow-ng <subcommand> --help
orairflow <group> --help
andairflow-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:
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 nameddaemon_
to differentiate it from thedaemon
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:
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:
To get automatic Rich highlighting, you should convert any
print
calls toconsole.print
calls.To use your converted subcommand, use the
airflow-ng
(entry point) command. This will eventually replace the previousairflow
(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
andtests/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
db
: previous and convertedscheduler
: previous and convertedtriggerer
: previous and convertedwebserver
: previous and convertedstandalone
: previous and convertedOther 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:click_yes
decorator)click_revision
,click_version
,click_from_revision
,click_from_version
,click_show_sql_only
)db upgrade
anddb downgrade
subcommandsAll 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
airflow db upgrade --help
example as opposed to the existing argparse implementation.Colors!
Click automatically colorizes output without any additional hinting.
airflow db --help
Click to expand before/after
Before
After
airflow db upgrade --help
Click to expand before/after
Before
After
airflow db upgrade --hep
(misspelled option)Click to expand before/after
Before
After
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 aConsole()
singleton to keep theme/console settings in a single place, and introduce a color-blind friendly mode.Click Goodies
^ 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.