You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/06/21 23:22:38 UTC
[GitHub] [airflow] blag opened a new pull request, #24590: Lay the groundwork for migrating Airflow CLI to Rich+Click
blag opened a new pull request, #24590:
URL: https://github.com/apache/airflow/pull/24590
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 the first PR to convert a command that I will publish shortly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] blag commented on a diff in pull request #24590: Lay the groundwork for migrating Airflow CLI to Rich+Click
Posted by GitBox <gi...@apache.org>.
blag commented on code in PR #24590:
URL: https://github.com/apache/airflow/pull/24590#discussion_r903157142
##########
airflow/cli/__main__.py:
##########
@@ -0,0 +1,24 @@
+#!/usr/bin/env python
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from airflow.cli import airflow_cmd
+
+
Review Comment:
This will fill in with imports as subcommands are converted. See [here](https://github.com/apache/airflow/pull/22613/files#diff-2b07ba57aa3d95539c22f30cdef2bb9bab49e1658cf03c44bdc143973da30550R21).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] blag commented on a diff in pull request #24590: Lay the groundwork for migrating Airflow CLI to Rich+Click
Posted by GitBox <gi...@apache.org>.
blag commented on code in PR #24590:
URL: https://github.com/apache/airflow/pull/24590#discussion_r904135278
##########
airflow/cli/__init__.py:
##########
@@ -15,3 +15,108 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+import os
+
+import rich_click as click
Review Comment:
That looks like a reasonable suggestion, but that import comes [straight out of the rich_click documentation](https://github.com/ewels/rich-click#import-as-click):
> ## Usage ##
> ### Import as click ###
>
> To use rich-click, switch out your normal click import with rich-click, using the same namespace:
>
> ```python
> import rich_click as click
> ```
>
> That's it ✨ Then continue to use click as you would normally.
The rich_click package tries to be a superset of Click. When people are trying to add additional options or commands, I am trying to direct them to the Click documentation, since that is what's relevant to them.
If that justification doesn't sway you, I'm happy to change it if you insist (just confirm in a comment below).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil merged pull request #24590: Lay the groundwork for migrating Airflow CLI to Rich+Click
Posted by GitBox <gi...@apache.org>.
kaxil merged PR #24590:
URL: https://github.com/apache/airflow/pull/24590
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] blag commented on a diff in pull request #24590: Lay the groundwork for migrating Airflow CLI to Rich+Click
Posted by GitBox <gi...@apache.org>.
blag commented on code in PR #24590:
URL: https://github.com/apache/airflow/pull/24590#discussion_r903156566
##########
airflow/cli/__init__.py:
##########
@@ -15,3 +15,108 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+import os
+
+import rich_click as click
+
+from airflow import settings
+from airflow.utils.cli import ColorMode
+from airflow.utils.timezone import parse as parsedate
+
+BUILD_DOCS = "BUILDING_AIRFLOW_DOCS" in os.environ
+
+click_color = click.option(
Review Comment:
Click options that are common across many different subcommands.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] blag commented on a diff in pull request #24590: Lay the groundwork for migrating Airflow CLI to Rich+Click
Posted by GitBox <gi...@apache.org>.
blag commented on code in PR #24590:
URL: https://github.com/apache/airflow/pull/24590#discussion_r905304680
##########
airflow/cli/__init__.py:
##########
@@ -15,3 +15,108 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+import os
+
+import rich_click as click
Review Comment:
This got merged before I had a chance to change this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] blag commented on a diff in pull request #24590: Lay the groundwork for migrating Airflow CLI to Rich+Click
Posted by GitBox <gi...@apache.org>.
blag commented on code in PR #24590:
URL: https://github.com/apache/airflow/pull/24590#discussion_r903157442
##########
airflow/utils/cli.py:
##########
@@ -46,14 +46,10 @@
def _check_cli_args(args):
if not args:
- raise ValueError("Args should be set")
- if not isinstance(args[0], Namespace):
- raise ValueError(
- f"1st positional argument should be argparse.Namespace instance, but is {type(args[0])}"
- )
+ raise ValueError(f"Args should be set: {args} [{type(args)}]")
-def action_cli(func=None, check_db=True):
+def action_cli(func=None, check_db=True, check_cli_args=True):
Review Comment:
The new `check_cli_args` flag is used [here](https://github.com/apache/airflow/pull/22613/files#diff-2bddc31b61cb7aa1de96c8e41d506d6c9d76345db432893ce55d3ec75e87e2fcR96).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] blag commented on a diff in pull request #24590: Lay the groundwork for migrating Airflow CLI to Rich+Click
Posted by GitBox <gi...@apache.org>.
blag commented on code in PR #24590:
URL: https://github.com/apache/airflow/pull/24590#discussion_r903157917
##########
airflow/utils/cli.py:
##########
@@ -306,11 +298,13 @@ class ColorMode:
AUTO = "auto"
-def should_use_colors(args) -> bool:
+def should_use_colors(args_or_color) -> bool:
"""Processes arguments and decides whether to enable color in output"""
- if args.color == ColorMode.ON:
+ # args.color is from argparse, Click CLI will pass in the color directly
+ color = args.color if hasattr(args, 'color') else args_or_color
Review Comment:
Backwards compatibility with argparse. I included the comment so we can grep for argparse and simplify this once we no longer need argparse.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] blag commented on pull request #24590: Lay the groundwork for migrating Airflow CLI to Rich+Click
Posted by GitBox <gi...@apache.org>.
blag commented on PR #24590:
URL: https://github.com/apache/airflow/pull/24590#issuecomment-1176594052
@Bowrna Absolutely, I'll keep you in mind if/when I decide to try refactoring the CLI again. Thank you again for all of your work!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #24590: Lay the groundwork for migrating Airflow CLI to Rich+Click
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24590:
URL: https://github.com/apache/airflow/pull/24590#discussion_r905254731
##########
setup.cfg:
##########
@@ -140,6 +140,7 @@ install_requires =
python-nvd3>=0.15.0
python-slugify>=5.0
rich>=12.4.4
+ rich-click>=1.3.1
Review Comment:
One small thing (for next PR). Rich-click 1.5 introduced some changes that might cause incompatibilities. I tihnk bumping up to > 1.5 is a good idea.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #24590: Lay the groundwork for migrating Airflow CLI to Rich+Click
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #24590:
URL: https://github.com/apache/airflow/pull/24590#discussion_r903287780
##########
airflow/cli/__init__.py:
##########
@@ -15,3 +15,108 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+import os
+
+import rich_click as click
Review Comment:
Since this import is used only locally in this file, I’d prefer it use `rich_click`.
```suggestion
import rich_click
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] blag commented on a diff in pull request #24590: Lay the groundwork for migrating Airflow CLI to Rich+Click
Posted by GitBox <gi...@apache.org>.
blag commented on code in PR #24590:
URL: https://github.com/apache/airflow/pull/24590#discussion_r903158221
##########
tests/utils/test_cli_util.py:
##########
@@ -38,7 +38,7 @@ def test_metrics_build(self):
func_name = 'test'
exec_date = datetime.utcnow()
namespace = Namespace(dag_id='foo', task_id='bar', subcommand='test', execution_date=exec_date)
- metrics = cli._build_metrics(func_name, namespace)
+ metrics = cli._build_metrics(func_name, [namespace], {})
Review Comment:
We changed the function arguments, but it's supposed to be a private function anyway.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Bowrna commented on pull request #24590: Lay the groundwork for migrating Airflow CLI to Rich+Click
Posted by GitBox <gi...@apache.org>.
Bowrna commented on PR #24590:
URL: https://github.com/apache/airflow/pull/24590#issuecomment-1172930945
@blag It's slightly disheartening to see the Click CLI for airflow getting closed after working on it for a few commands. If there is any plan to do the groundwork or research work for checking the best way to set CLI commands for airflow, could you include me too? I am glad to extend my help and give my contribution to that part. Especially with trying something like using `mypyc` to speed up the code execution, i can help in some research work in that area.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org