blag commented on code in PR #24591: URL: https://github.com/apache/airflow/pull/24591#discussion_r909432967
########## airflow/cli/__main__.py: ########## @@ -18,6 +18,7 @@ # under the License. from airflow.cli import airflow_cmd +from airflow.cli.commands import version # noqa: F401 Review Comment: @uranusjr Thanks for the pointer with `__all__`. I don't have comprehensive statistics on additional overhead, although that was discussed [starting here](https://github.com/apache/airflow/pull/22613#issuecomment-1086788871) in the "omnibus" PR, and improved in [a related PR](https://github.com/astronomer/airflow/pull/1486). I think it might be easier to address any slowdowns on a case-by-case basis for each command rewrite instead of all at once in this PR. That being said, here is some not-super-scientific timings for the `version` subcommand: #### argparse #### ``` ❯ time ( for i in $(seq 10); do airflow version --help 1>/dev/null; done ) ( for i in $(seq 10); do; airflow version --help > /dev/null; done; ) 4.59s user 0.90s system 99% cpu 5.533 total ``` #### Click #### ``` ❯ time ( for i in $(seq 10); do airflow-ng version --help 1>/dev/null; done ) ( for i in $(seq 10); do; airflow-ng version --help > /dev/null; done; ) 4.21s user 0.83s system 99% cpu 5.093 total ``` #### argparse #### ``` ❯ time ( for i in $(seq 10); do airflow version 1>/dev/null; done ) ( for i in $(seq 10); do; airflow version > /dev/null; done; ) 4.57s user 0.90s system 99% cpu 5.513 total ``` #### Click #### ``` ❯ time ( for i in $(seq 10); do airflow-ng version 1>/dev/null; done ) ( for i in $(seq 10); do; airflow-ng version > /dev/null; done; ) 4.20s user 0.82s system 98% cpu 5.069 total ``` @potiuk I have tweaked the imports for this subcommand to improve timing, and captured your suggestion in the original omnibus PR description (in the "Future Work" section). I'm happy to also create an issue to ensure that doesn't get lost, but I think that might be out of the scope for this PR. I _do_ think that should be implemented before we finally switch the `airflow` command to being Click-based, but since this is only modifying a brand new, totally separate `airflow-ng` command I don't think it should be a requirement for this PR, or other PRs converting other commands, to be merged. But I'll leave the final resolution up to you. Please let me know if you'd like me to add that pre-commit hook now, or if we can defer it to an issue for later 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
