potiuk edited a comment on pull request #16088:
URL: https://github.com/apache/airflow/pull/16088#issuecomment-849323125


   > Looks good to me. I wonder if it would be possible to add a test for this; 
could be difficult since it depends on the global interpreter state?
   
   Yeah. Testing this is difficult. And actually we kind'a test it in the 
updated test -> the sequence `initialize` followed by `validate` is run there 
(that's why I actually added the "validate()" in the test even if it was not 
strictly necessary there. But you are right. By the time we got there in tests 
the "initialize()" command have been run multiple times already in other tests 
and global state is already changed.
   
   I think it would require some refactoring of the approach we use conf. 
Currently it is indeed a global state, but it should really be independent 
object that you can initialize and test in isolation. This is what I mentioned 
in the other related comment in 
https://github.com/apache/airflow/issues/15685#issuecomment-834181697 .  The 
way `configuration` is used is the main reason why we cannot remove the last 
few files from `pylint_todo.txt` - because pylint (rightfully) detects cyclic 
dependencies (not imports!) because of the way configuration is doing two 
things - it is both `used by` and `uses` other airflow components.
   
   For now this problem seems to be fixed by this change (confirmed by 
@dowthron  in #16079 - seems @downthron came to the same fix as I did in the 
meantime).
   
   But I am happy to discuss the way we can approach configuration differently 
as next step. The little problem with that is that it might only be really 
fixable in Airflow 3.0 because it might introduce some 
backward-incompatibilities as we might need to change how 
`airflow.configuration.conf` object is used and I believe people use it as a 
"public API" of airflow and by changing it we might break their plugins and 
dags. But maybe we can do it similarly as we did with lazy initialization of 
conf rather than import  via __getattr__ /PEP-562 + STATICA_HACK  
https://github.com/apache/airflow/blob/71ef2f2ee9ccf238a99cb0e42412d2118bad22a1/airflow/__init__.py#L55
 to handle `from airflow import DAG` and `from airflow import 
AirflowException`. 
   
   It would be great to have separate conf object that we could test 
initialization of independently and unentangle the cyclic dependencies.
   


-- 
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]


Reply via email to