Re: [PR] Introduce a "cli" section in provider metadata [airflow]
potiuk commented on PR #59805: URL: https://github.com/apache/airflow/pull/59805#issuecomment-3714231641 We can always make another PR ;) -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce a "cli" section in provider metadata [airflow]
potiuk merged PR #59805: URL: https://github.com/apache/airflow/pull/59805 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce a "cli" section in provider metadata [airflow]
jason810496 commented on PR #59805: URL: https://github.com/apache/airflow/pull/59805#issuecomment-3713944434 > Shall we merge ? Iām definitely good to go š. Not sure if anyone has any additional comments. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce a "cli" section in provider metadata [airflow]
potiuk commented on PR #59805: URL: https://github.com/apache/airflow/pull/59805#issuecomment-3713876465 Shall we merge ? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce a "cli" section in provider metadata [airflow]
potiuk commented on PR #59805: URL: https://github.com/apache/airflow/pull/59805#issuecomment-3713806385 He he - initialzing all providers takes about the same time as `import pendulum` :D -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce a "cli" section in provider metadata [airflow]
jason810496 commented on PR #59805: URL: https://github.com/apache/airflow/pull/59805#issuecomment-3712964731 > Assuming this is related to the time that plugin manager needs to initialize 97 providers when running in breeze? Or have you checked where the most time still is spend? This is the part where skipping `_correctness_check` comes to play. Before skipping the `_correctness_check`, we **will import all the available executors for every CLI command called!** ( I used `py-spy` to check the most time-wasting part as well. Before the refactor, importing those heavy modules take most of the time. After the refactor, yes, the `import airflow` take most of the time as Jarek described ) -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce a "cli" section in provider metadata [airflow]
jason810496 commented on code in PR #59805: URL: https://github.com/apache/airflow/pull/59805#discussion_r2663445619 ## providers/edge3/src/airflow/providers/edge3/cli/definition.py: ## Review Comment: > which was already a bit of a mix-up. Yes, for the previous architecture, if we want to introduce CLI for other component (e.g. Notification or XCom that said), we need to introduce a new `Loader` (like `ExecutorLoader` ). That's why I use "provider-level CLI" to describe it, the 'cli' section we introduce will be 'provider' wise (once we installed it, the CLI will be shown no matter whether we set them as our Executor/AuthManager or not) -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce a "cli" section in provider metadata [airflow]
jason810496 commented on code in PR #59805:
URL: https://github.com/apache/airflow/pull/59805#discussion_r2663438252
##
providers-summary-docs/core-extensions/cli-commands.rst:
##
@@ -0,0 +1,143 @@
+ .. 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.
+
+Command Line Interface
+--
+
+.. important::
+ The Airflow Core version must be ``3.2.0`` or newer to be able to use
provider-level CLI commands.
+
+
+If your provider include :doc:`/core-extensions/auth-managers` or
:doc:`/core-extensions/executors`, you should also implement the provider-level
CLI commands
+to improve the Airflow CLI response speed and avoid loading heavy dependencies
when those commands are not needed.
+
+Even if your auth manager or executor do not implement the
``get_cli_commands`` interface
(:meth:`airflow.api_fastapi.auth.managers.base_auth_manager.BaseAuthManager.get_cli_commands`
or :meth:`airflow.executors.base_executor.BaseExecutor.get_cli_commands`), you
should still implement provider-level CLI commands that return empty list to
avoid loading auth manager or executor code for every CLI command, which will
also resolve the following warning:
+
+.. code-block:: console
+
+Please define the 'cli' section in the provider.yaml for custom auth
managers to avoid this warning.
+For community providers, please update to the version that support
'.cli.definition.get_cli_commands' function.
+For more details, see
https://airflow.apache.org/docs/apache-airflow-providers/core-extensions/cli-commands.html
+
+Please define the 'cli' section in the provider.yaml for custom executors
to avoid this warning.
+For community providers, please update to the version that support
'.cli.definition.get_cli_commands' function.
+For more details, see
https://airflow.apache.org/docs/apache-airflow-providers/core-extensions/cli-commands.html
+
+
+Implementing provider-level CLI Commands
+
+
+To implement provider-level CLI commands, follow these steps:
+
+1. Define all your CLI commands in
``airflow.providers..cli.definition`` module. Additionally,
you **should avoid defining heavy dependencies in this module** to reduce the
Airflow CLI startup time. Please use
``airflow.cli.cli_config.lazy_load_command`` utility to lazily load the actual
callable to run.
+
+.. code-block:: python
+
+from airflow.cli.cli_config import (
+ActionCommand,
+Arg,
+GroupCommand,
+lazy_load_command,
+)
+
+
+@staticmethod
+def get_my_cli_commands() -> list[GroupCommand]:
+executor_sub_commands = [
+ActionCommand(
+name="executor_subcommand_name",
+help="Description of what this specific command does",
+func=lazy_load_command("path.to.python.function.for.command"),
+args=Arg(
+"--my-arg",
+help="Description of my arg",
+action="store_true",
+),
+),
+]
+auth_manager_sub_commands = [
+ActionCommand(
+name="auth_manager_subcommand_name",
+help="Description of what this specific command does",
+func=lazy_load_command("path.to.python.function.for.command"),
+args=(),
+),
+]
+custom_sub_commands = [
+ActionCommand(
+name="custom_subcommand_name",
+help="Description of what this specific command does",
+func=lazy_load_command("path.to.python.function.for.command"),
+args=(),
+),
+]
+
+return [
+GroupCommand(
+name="my_cool_executor",
+help="Description of what this group of commands do",
+subcommands=executor_sub_commands,
+),
+GroupCommand(
+name="my_cool_auth_manager",
+help="Description of what this group of commands do",
+subcommands=auth_manager_sub_commands,
+),
+GroupCommand(
+name="my_cool_cust
Re: [PR] Introduce a "cli" section in provider metadata [airflow]
potiuk commented on code in PR #59805: URL: https://github.com/apache/airflow/pull/59805#discussion_r2663088397 ## providers/keycloak/src/airflow/providers/keycloak/auth_manager/cli/__init__.py: ## @@ -14,3 +14,4 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. +from __future__ import annotations Review Comment: we add it automaticallly by prek -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce a "cli" section in provider metadata [airflow]
potiuk commented on PR #59805: URL: https://github.com/apache/airflow/pull/59805#issuecomment-3712450229 BTW: https://github.com/user-attachments/assets/4a29b8a0-14b4-41fe-9e72-de8da6250244"; /> -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce a "cli" section in provider metadata [airflow]
potiuk commented on PR #59805: URL: https://github.com/apache/airflow/pull/59805#issuecomment-3712448762 Yes This is what I am talking about when telling about "explicit initialization" of things that we need in the commands we need. The biggest problem is that we **absolutely** do not control what is getting imported and touching anything there now introduces cyclic imports or god-knowsl-what. And we already do about a million of semi-random lazy initialization to make things faster.. That's `explicit is better than implicit` in it's purest form -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce a "cli" section in provider metadata [airflow]
jscheffl commented on PR #59805: URL: https://github.com/apache/airflow/pull/59805#issuecomment-3712441415 > > Lots and lots of imports: > > Okay, yeah, the `import airflow` problem :-( Interesting to see that indirectly loading pendulum initialized time-machine which loads even pytest... which takes alone 8% of time... Crazy. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce a "cli" section in provider metadata [airflow]
jscheffl commented on PR #59805: URL: https://github.com/apache/airflow/pull/59805#issuecomment-3712433327 > Lots and lots of imports: Okay, yeah, the `import airflow` problem :-( -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce a "cli" section in provider metadata [airflow]
potiuk commented on PR #59805: URL: https://github.com/apache/airflow/pull/59805#issuecomment-3712428212 Lots and lots of imports:  -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce a "cli" section in provider metadata [airflow]
jscheffl commented on code in PR #59805: URL: https://github.com/apache/airflow/pull/59805#discussion_r2662596384 ## providers/keycloak/src/airflow/providers/keycloak/auth_manager/cli/__init__.py: ## @@ -14,3 +14,4 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. +from __future__ import annotations Review Comment: I think this is also no needed here? ```suggestion ``` -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce a "cli" section in provider metadata [airflow]
jscheffl commented on code in PR #59805: URL: https://github.com/apache/airflow/pull/59805#discussion_r2662588202 ## providers/edge3/src/airflow/providers/edge3/cli/definition.py: ## Review Comment: mhm... whereas in the past the cli commands were always loaded from "executor" but were residing in cli package already... which was already a bit of a mix-up. So actually... the PR does not make it worse. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce a "cli" section in provider metadata [airflow]
jscheffl commented on code in PR #59805:
URL: https://github.com/apache/airflow/pull/59805#discussion_r2662585942
##
providers/edge3/src/airflow/providers/edge3/cli/definition.py:
##
Review Comment:
I have one naming problem ("Naming is always the hardest" with the
`cli.definition`. In the `edge3.cli` namespace currently the worker/client
resides. But commands are usually loaded on executor (=`edge3.executors`)
package. I have currently no real better idea for a name though...
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce a "cli" section in provider metadata [airflow]
jscheffl commented on code in PR #59805:
URL: https://github.com/apache/airflow/pull/59805#discussion_r2662539329
##
providers-summary-docs/core-extensions/cli-commands.rst:
##
@@ -0,0 +1,143 @@
+ .. 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.
+
+Command Line Interface
+--
+
+.. important::
+ The Airflow Core version must be ``3.2.0`` or newer to be able to use
provider-level CLI commands.
+
+
+If your provider include :doc:`/core-extensions/auth-managers` or
:doc:`/core-extensions/executors`, you should also implement the provider-level
CLI commands
+to improve the Airflow CLI response speed and avoid loading heavy dependencies
when those commands are not needed.
+
+Even if your auth manager or executor do not implement the
``get_cli_commands`` interface
(:meth:`airflow.api_fastapi.auth.managers.base_auth_manager.BaseAuthManager.get_cli_commands`
or :meth:`airflow.executors.base_executor.BaseExecutor.get_cli_commands`), you
should still implement provider-level CLI commands that return empty list to
avoid loading auth manager or executor code for every CLI command, which will
also resolve the following warning:
+
+.. code-block:: console
+
+Please define the 'cli' section in the provider.yaml for custom auth
managers to avoid this warning.
+For community providers, please update to the version that support
'.cli.definition.get_cli_commands' function.
+For more details, see
https://airflow.apache.org/docs/apache-airflow-providers/core-extensions/cli-commands.html
+
+Please define the 'cli' section in the provider.yaml for custom executors
to avoid this warning.
+For community providers, please update to the version that support
'.cli.definition.get_cli_commands' function.
+For more details, see
https://airflow.apache.org/docs/apache-airflow-providers/core-extensions/cli-commands.html
+
+
+Implementing provider-level CLI Commands
+
+
+To implement provider-level CLI commands, follow these steps:
+
+1. Define all your CLI commands in
``airflow.providers..cli.definition`` module. Additionally,
you **should avoid defining heavy dependencies in this module** to reduce the
Airflow CLI startup time. Please use
``airflow.cli.cli_config.lazy_load_command`` utility to lazily load the actual
callable to run.
+
+.. code-block:: python
+
+from airflow.cli.cli_config import (
+ActionCommand,
+Arg,
+GroupCommand,
+lazy_load_command,
+)
+
+
+@staticmethod
+def get_my_cli_commands() -> list[GroupCommand]:
+executor_sub_commands = [
+ActionCommand(
+name="executor_subcommand_name",
+help="Description of what this specific command does",
+func=lazy_load_command("path.to.python.function.for.command"),
+args=Arg(
+"--my-arg",
+help="Description of my arg",
+action="store_true",
+),
+),
+]
+auth_manager_sub_commands = [
+ActionCommand(
+name="auth_manager_subcommand_name",
+help="Description of what this specific command does",
+func=lazy_load_command("path.to.python.function.for.command"),
+args=(),
+),
+]
+custom_sub_commands = [
+ActionCommand(
+name="custom_subcommand_name",
+help="Description of what this specific command does",
+func=lazy_load_command("path.to.python.function.for.command"),
+args=(),
+),
+]
+
+return [
+GroupCommand(
+name="my_cool_executor",
+help="Description of what this group of commands do",
+subcommands=executor_sub_commands,
+),
+GroupCommand(
+name="my_cool_auth_manager",
+help="Description of what this group of commands do",
+subcommands=auth_manager_sub_commands,
+),
+GroupCommand(
+name="my_cool_custom_
Re: [PR] Introduce a "cli" section in provider metadata [airflow]
jason810496 commented on code in PR #59805: URL: https://github.com/apache/airflow/pull/59805#discussion_r2662062732 ## providers/amazon/src/airflow/providers/amazon/aws/auth_manager/cli/__init__.py: ## @@ -14,3 +14,4 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. +from __future__ import annotations Review Comment: Good catch! It's no need at all, I added them by accident. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce a "cli" section in provider metadata [airflow]
vincbeck commented on code in PR #59805: URL: https://github.com/apache/airflow/pull/59805#discussion_r2661890712 ## providers/amazon/src/airflow/providers/amazon/aws/auth_manager/cli/__init__.py: ## @@ -14,3 +14,4 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. +from __future__ import annotations Review Comment: Is it needed? ## providers/fab/src/airflow/providers/fab/auth_manager/cli_commands/__init__.py: ## @@ -15,3 +15,4 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. +from __future__ import annotations Review Comment: Needed? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce a "cli" section in provider metadata [airflow]
potiuk commented on PR #59805: URL: https://github.com/apache/airflow/pull/59805#issuecomment-3702141185 > Introducing ProviderManager.executor_without_check property to skip the _correctness_check, the property will return set of (executor_name, executor_provider_package_name). Only if there is any executor provider not defined the cli section, then we will call ExecutorLoader.get_executor_names to achieve the "real lazy loading". Nice. needs a bit refactoring (extracting functions) - but this is cool -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce a "cli" section in provider metadata [airflow]
jason810496 commented on code in PR #59805: URL: https://github.com/apache/airflow/pull/59805#discussion_r2649708589 ## airflow-core/src/airflow/cli/cli_parser.py: ## @@ -60,30 +59,21 @@ log = logging.getLogger(__name__) -for executor_name in ExecutorLoader.get_executor_names(validate_teams=False): Review Comment: Good point! > Possibly one way of doing it is simple mapping of the executors -> provider (for built-in executors we have mapping from the executor to provider package and checking if the provider has "cli" defined in provider info? Also likely we should raise an error if provider has an executor defined with cli commands AND cli command defined in provider info. Yes, that is the exact same idea for me. It could be done in ProviderManager, and I will add the same validation for AuthManager as well. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce a "cli" section in provider metadata [airflow]
potiuk commented on code in PR #59805: URL: https://github.com/apache/airflow/pull/59805#discussion_r2649692458 ## airflow-core/src/airflow/cli/cli_parser.py: ## @@ -60,30 +59,21 @@ log = logging.getLogger(__name__) -for executor_name in ExecutorLoader.get_executor_names(validate_teams=False): Review Comment: I think we need to add a fallback reading of CLIs in old ways - not sure how to do it without impacting performance too much, because we do not want to **always** load executor class. This is important for two reasons: 1) someone want to stay with old "community" provider version but use newer airflow 2) someone has their own executor in their 3rd-party provider with CLIs defined Possibly one way of doing it is simple mapping of the executors -> provider (for built-in executors we have mapping from the executor to provider package and checking if the provider has "cli" defined in provider info? Also likely we should raise an error if provider has an executor defined with cli commands AND cli command defined in provider info. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
