[GitHub] [airflow] msb217 commented on a change in pull request #7456: [AIRFLOW-6817] [WIP] - Move airflow __init__.py imports to sub-packages
msb217 commented on a change in pull request #7456: [AIRFLOW-6817] [WIP] - Move airflow __init__.py imports to sub-packages URL: https://github.com/apache/airflow/pull/7456#discussion_r382743849 ## File path: tests/jobs/test_local_task_job.py ## @@ -24,10 +24,11 @@ import pytest from mock import patch -from airflow import AirflowException, models, settings +from airflow import settings +from airflow.exceptions import AirflowException from airflow.executors.sequential_executor import SequentialExecutor from airflow.jobs import LocalTaskJob -from airflow.models import DAG, TaskInstance as TI +from airflow.models import DAG, DagBag, TaskInstance as TI Review comment: Yes I think that would be good for consistency. Would it be worth considering a similar change to `airflow/models/__init__.py`? Removing the implicit imports of models? The implicit imports of models are used quite frequently 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
[GitHub] [airflow] msb217 commented on a change in pull request #7456: [AIRFLOW-6817] [WIP] - Move airflow __init__.py imports to sub-packages
msb217 commented on a change in pull request #7456: [AIRFLOW-6817] [WIP] - Move airflow __init__.py imports to sub-packages URL: https://github.com/apache/airflow/pull/7456#discussion_r382743849 ## File path: tests/jobs/test_local_task_job.py ## @@ -24,10 +24,11 @@ import pytest from mock import patch -from airflow import AirflowException, models, settings +from airflow import settings +from airflow.exceptions import AirflowException from airflow.executors.sequential_executor import SequentialExecutor from airflow.jobs import LocalTaskJob -from airflow.models import DAG, TaskInstance as TI +from airflow.models import DAG, DagBag, TaskInstance as TI Review comment: Yes I think that would be good for consistency. Would it be worth considering a similar change to `airflow/models/__init__.py`? Removing the implicit imports of models? 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
[GitHub] [airflow] msb217 commented on a change in pull request #7456: [AIRFLOW-6817] [WIP] - Move airflow __init__.py imports to sub-packages
msb217 commented on a change in pull request #7456: [AIRFLOW-6817] [WIP] - Move airflow __init__.py imports to sub-packages URL: https://github.com/apache/airflow/pull/7456#discussion_r382698927 ## File path: airflow/__init__.py ## @@ -30,13 +30,8 @@ # pylint: disable=wrong-import-position from typing import Callable, Optional -from airflow import utils from airflow import settings from airflow import version -from airflow.utils.log.logging_mixin import LoggingMixin -from airflow.configuration import conf -from airflow.exceptions import AirflowException -from airflow.models.dag import DAG Review comment: To do this I'm assuming you'd like to see the `integrate_plugins()` functionality decoupled? So for example, `integrate_operator_plugins()` would correspond to the `airflow.operators` module. The function `integrate_sensor_plugins()` would correspond to the `airflow.sensors` module and so on for hooks/macros. I'm sure it'd be more complex than what I'm describing, but the idea of only initializing plugins for their respective sub-modules is what you'd be going for, correct? 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
[GitHub] [airflow] msb217 commented on a change in pull request #7456: [AIRFLOW-6817] [WIP] - Move airflow __init__.py imports to sub-packages
msb217 commented on a change in pull request #7456: [AIRFLOW-6817] [WIP] - Move airflow __init__.py imports to sub-packages URL: https://github.com/apache/airflow/pull/7456#discussion_r382699222 ## File path: airflow/providers/qubole/example_dags/example_qubole_sensor.py ## @@ -0,0 +1,80 @@ +# Review comment: Yes - sorry I accidentally included this after a rebase. When I squash/rebase this should be resolved 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
[GitHub] [airflow] msb217 commented on a change in pull request #7456: [AIRFLOW-6817] [WIP] - Move airflow __init__.py imports to sub-packages
msb217 commented on a change in pull request #7456: [AIRFLOW-6817] [WIP] - Move airflow __init__.py imports to sub-packages URL: https://github.com/apache/airflow/pull/7456#discussion_r382698927 ## File path: airflow/__init__.py ## @@ -30,13 +30,8 @@ # pylint: disable=wrong-import-position from typing import Callable, Optional -from airflow import utils from airflow import settings from airflow import version -from airflow.utils.log.logging_mixin import LoggingMixin -from airflow.configuration import conf -from airflow.exceptions import AirflowException -from airflow.models.dag import DAG Review comment: To do this I'm assuming you'd like to see the `integrate_plugins()` functionality decoupled? So for example, `integrate_operator_plugins()` would correspond to the `airflow.operators` module. The function `integrate_sensor_plugins()` would correspond to the `airflow.sensors` module and so on for hooks/macros. I'm sure it'd be more complex than what I'm describing, but the idea of only initializing plugins for their respective modules is what you'd be going for, correct? 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
