potiuk commented on a change in pull request #6596: [AIRFLOW-6004] Untangle Executors class to avoid cyclic imports URL: https://github.com/apache/airflow/pull/6596#discussion_r347391606
########## File path: tests/dags/test_subdag.py ########## @@ -24,7 +24,7 @@ from datetime import datetime, timedelta -from airflow.models import DAG +from airflow.models.dag import DAG Review comment: Correct it's not needed. However I still do not like that you can import classes from two places and none of them is "preferred". It leads to a number of inconsistencies - sometimes you do 'from airflow import DAG' sometimes 'from airflow.models import DAG' and sometimes 'from airflow.models.dag import DAG'. And it is very easy for someone to add such unwanted/cyclic dependencies if we have such "umbrella" __init__ packages. This is exactly what happened in this case - changes in KubernetesExecutor caused it to import something in airflow and this in-turned caused import of KubernetesExecutor... This can happen all the times if we have such __init__'s with logic + umbrella __init__'s that bring in unnecessary dependencies even if we import one thing from them. I am going to deprecate 'from airflow import DAG' (precisely because of the potential cyclic imports it introduces). Each of the imports has different implications - for example importing from airflow brings more dependencies and potentially executing some plugin code, importing from models brings implicit dependencies to other models, importing from models.dag is I think best approach - you do not bring additional dependencies only the ones you are interested in. An average (or even experienced) developer of Airflow has no idea about those consequences of different ways of importing such classes from different packages. I think we should encourage people to import all classes directly and discourage (by deprecation) importing from "umbrella" modules - those bringing a lot of potentially disruptive dependencies. It's really 'explicit is better than implicit' case IMHO. By importing anything from airflow or from airflow.models we bring a lot of implicit dependencies without even knowing about it. WDYT @ashb ? What are the benefits of importing such classes from higher-level packages? I cannot see any except a bit shorter import - I would love if we can simply deprecate all of the non-direct imports. This would make cyclic imports like the one we are dealing with much less of a 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. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
