ashb commented on a change in pull request #6596: [AIRFLOW-6004] Untangle Executors class to avoid cyclic imports. Depends on [AIRFLOW-6010] URL: https://github.com/apache/airflow/pull/6596#discussion_r347861744
########## 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: ## Result of the current situation > Can you tell which way is better Neither :) They both work. But yes, having some internal/enforce rules for the airflow code base is probably a good thing for consistency. ## DAG developer's perspective > Mabe the deprecation warning should only be thrown if you are importing airflow from within airflow core code itself Yes if this was achievable I would be happy with this approach. Yes, the deprecation warning wouldn't _require_ a rewrite, but it would be annoying/noisy until the change was done - i.e. not something I want to force on users without a definite benefit to it. > I hope you can agree with me that it makes sense for Airflow "core" in airflow repository use a single, direct import (airflow.models.dag.DAG) where circular imports will be least likely Grudgingly, because people will look at the internals and copy that, and I really feel like this is leaking abstractions and `airflow.models.DAG` or `airflow.DAG` is what consumers of the library should be using. ## Avoiding cyclic imports The avoiding cycling imports doesn't worry me, as the code either works, or it crashes the tests and we fix it on a case-by-base basis. I haven't ever seen a case where what an end user imports causes or avoids a cyclic import -- it's all only within airflow PRs. Is there a case I've not seen where we've got a broken import based on which order packages are imported in tests/user code? ## Importing 'airflow' package first `python -c import airflow.hooks` will _always_ import `airflow/__init__.py` first and then load `airflow/hooks/__init__.py`. That is how python imports work. I'm all in favour of removing the side-effect-from-importing (I reported https://issues.apache.org/jira/browse/AIRFLOW-1931 a long time ago) but the cyclic import issue is just not a concern to me -- ---------------------------------------------------------------- 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: us...@infra.apache.org With regards, Apache Git Services
