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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
