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. > Seems like people randomly choose the import they should use without knowing the consequences The only consequence is "if we change the layout it might break in the future". There is zero effect right now to import cycles or not. ## 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. Part of this might be fixed by updating the docs to not build/publish docs for any `airflow.model.*` package (and bonus points for re-writing any references to just `airflow.models.Class`?) ## 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 -- to my knowledge import cycles can only be created when _changing_ the airflow code (i.e. writing a PR) if you aren't changing the code then it is impossible to get a cycle. ---------------------------------------------------------------- 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
