airflow-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [airflow] ashb commented on a change in pull request #6596: [AIRFLOW-6004] Untangle Executors class to avoid cyclic imports. Depends on [AIRFLOW-6010]
Date Tue, 19 Nov 2019 11:07:33 GMT
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.
   
   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 -- 

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

Mime
View raw message