[GitHub] [airflow] o-nikolas commented on a diff in pull request #33481: Make auth managers provide their own airflow CLI commands

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

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


##########
airflow/cli/cli_parser.py:
##########
@@ -62,6 +63,13 @@
     # Do no re-raise the exception since we want the CLI to still function for
     # other commands.
 
+try:
+    auth_mgr = get_auth_manager()

Review Comment:
   It looks like this helper gets the class from config and constructs an instance before returning. Could we update that to take an optional arg (or add another helper) to just return the class? For executors the `get_cli_commands` function is a static method and you just need the class imported to call it, no need to create an instance (i.e. run `__init__` and all it's side effects).
   
   Also how expensive is `init_auth_manager` and the fab class itself to import? It's important to optimize anything on the CLI path to be very fast to import since it will slow down the CLI otherwise. You can see some examples of what I did in the executor space and the benchmarks I provided:
   - Optimizing the [base executor class](https://github.com/apache/airflow/pull/30361)
   - Optimizing [Celery](https://github.com/apache/airflow/pull/31001) and [Kubernetes](https://github.com/apache/airflow/pull/30727) executor classes.



##########
airflow/providers/celery/executors/celery_executor.py:
##########
@@ -143,6 +142,7 @@ def __getattr__(name):
 )
 
 # worker cli args
+ARG_AUTOSCALE = Arg(("-a", "--autoscale"), help="Minimum and Maximum number of worker to autoscale")

Review Comment:
   This one felt generic enough to leave in the cli_config for other future worker based executors, but I don't feel too strongly.



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