Re: [PR] AIP-61: Allow multiple executors to load their CLI commands [airflow]

["o-nikolas (via GitHub)" <gi...@apache.org>]

o-nikolas commented on code in PR #39077:
URL: https://github.com/apache/airflow/pull/39077#discussion_r1569142772


##########
airflow/cli/cli_parser.py:
##########
@@ -56,19 +56,22 @@
 airflow_commands = core_commands.copy()  # make a copy to prevent bad interactions in tests
 
 log = logging.getLogger(__name__)
-try:
-    executor, _ = ExecutorLoader.import_default_executor_cls(validate=False)
-    airflow_commands.extend(executor.get_cli_commands())
-except Exception:
-    executor_name = ExecutorLoader.get_default_executor_name()
-    log.exception("Failed to load CLI commands from executor: %s", executor_name)
-    log.error(
-        "Ensure all dependencies are met and try again. If using a Celery based executor install "
-        "a 3.3.0+ version of the Celery provider. If using a Kubernetes executor, install a "
-        "7.4.0+ version of the CNCF provider"
-    )
-    # Do not re-raise the exception since we want the CLI to still function for
-    # other commands.
+
+
+executors = [executor for executor, _ in ExecutorLoader.import_all_executors()]
+
+for executor in executors:

Review Comment:
   You want the imports to be within the try/catch here, which is quite important. The biggest issue we're looking out for here is the case of the executor module not being able to load (maybe it has some dependency that's not installed) which is what you'd see if you configured an executor in airflow config but didn't install the provider package say.
   
   With this in mind, you might have to pivot to adding a method to just fetch the names, and then load them individually so that one failed executor doesn't stop others from loading their commands.
   
   So the pseudo code will look like:
   ```python
   for executor_name in ExecutorLoader.get_all_executor_names():
      try:
          # Both the below lines need to be in the try
          loaded_executor = ExecutorLoader.load_executor(executor_name)
          airflow_commands.extend(loaded_executor.get_cli_commands())
      except Exception:
          <all the same exception handling/messaging here>



-- 
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