Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AIRFLOW-6010] Remove cyclic imports and pylint disables #6601

Merged
merged 1 commit into from
Nov 26, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,33 @@ repos:
entry: "pydevd.*settrace\\("
pass_filenames: true
files: \.py$
- id: base-operator
language: pygrep
name: Make sure BaseOperator is imported from airflow.models.baseoperator in core
entry: "from airflow.models import.* BaseOperator"
files: \.py$
pass_filenames: true
exclude: >
(?x)
^airflow/gcp/.*$|
^airflow/hooks/.*$|
^airflow/operators/.*$|
^airflow/sensors/.*$|
^airflow/providers/.*$|
^airflow/contrib/.*$
- id: base-operator
language: pygrep
name: Make sure BaseOperator is imported from airflow.models outside of core
entry: "from airflow.models.baseoperator import.* BaseOperator"
pass_filenames: true
files: >
(?x)
^airflow/gcp/.*$|
^airflow/hooks/.*$|
^airflow/operators/.*$|
^airflow/sensors/.*$|
^airflow/providers/.*\.py$|
^airflow/contrib/.*\.py$
- id: build
name: Check if image build is needed
entry: ./scripts/ci/pre_commit_ci_build.sh
Expand Down
31 changes: 30 additions & 1 deletion CONTRIBUTING.rst
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,7 @@ image built locally):
=================================== ================================================================ ============
**Hooks** **Description** **Breeze**
=================================== ================================================================ ============
``airflow-settings`` Check if airflow import settings are used well.
``base-operator`` Checks that BaseOperator is imported properly
----------------------------------- ---------------------------------------------------------------- ------------
``build`` Builds image for check-apache-licence, mypy, pylint, flake8. *
----------------------------------- ---------------------------------------------------------------- ------------
Expand Down Expand Up @@ -513,6 +513,35 @@ You can always skip running the tests by providing ``--no-verify`` flag to the

To check other usage types of the pre-commit framework, see `Pre-commit website <https://pre-commit.com/>`__.

Importing Airflow core objects
==============================

When you implement core features or DAGs you might need to import some of the core objects or modules.
Since Apache Airflow can be used both as application (by internal classes) and as library (by DAGs), there are
different ways those core objects and packages are imported.

Airflow imports some of the core objects directly to 'airflow' package so that they can be used from there.

Those criteria were assumed for choosing what import path to use:

* If you work on a core feature inside Apache Airflow, you should import the objects directly from the
package where the object is defined - this minimises the risk of cyclic imports.
* If you import the objects from any of 'providers' classes, you should import the objects from
'airflow' or 'airflow.models', It is very important for back-porting operators/hooks/sensors
to Airflow 1.10.* (AIP-21)
potiuk marked this conversation as resolved.
Show resolved Hide resolved
* If you import objects from within a DAG you write, you should import them from 'airflow' or
'airflow.models' package where stable location of such import is important.

Those checks enforced for the most important and repeated objects via pre-commit hooks as described below.

BaseOperator
------------

The BaseOperator should be imported:
* as ``from airflow.models import BaseOperator`` in external DAG/operator
* as ``from airflow.models.baseoperator import BaseOperator`` in Airflow core to avoid cyclic imports


Travis CI Testing Framework
===========================

Expand Down
8 changes: 8 additions & 0 deletions UPDATING.md
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,14 @@ assists users migrating to a new version.

## Airflow Master

### Changes to settings

CONTEXT_MANAGER_DAG was removed from settings. It's role has been taken by `DagContext` in
'airflow.models.dag'. One of the reasons was that settings should be rather static than store
dynamic context from the DAG, but the main one is that moving the context out of settings allowed to
untangle cyclic imports between DAG, BaseOperator, SerializedDAG, SerializedBaseOperator which was
part of AIRFLOW-6010.
potiuk marked this conversation as resolved.
Show resolved Hide resolved

#### Change default aws_conn_id in EMR operators

The default value for the [aws_conn_id](https://airflow.apache.org/howto/manage-connections.html#amazon-web-services) was accidently set to 's3_default' instead of 'aws_default' in some of the emr operators in previous
Expand Down
2 changes: 2 additions & 0 deletions airflow/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@
# pylint:disable=wrong-import-position
from typing import Callable, Optional

# noinspection PyUnresolvedReferences
from airflow import utils
kaxil marked this conversation as resolved.
Show resolved Hide resolved
from airflow import settings
from airflow import version
from airflow.utils.log.logging_mixin import LoggingMixin
Expand Down
3 changes: 2 additions & 1 deletion airflow/api/common/experimental/delete_dag.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@

from airflow import models
from airflow.exceptions import DagNotFound
from airflow.models import DagModel, SerializedDagModel, TaskFail
from airflow.models import DagModel, TaskFail
from airflow.models.serialized_dag import SerializedDagModel
from airflow.settings import STORE_SERIALIZED_DAGS
from airflow.utils.db import provide_session
from airflow.utils.log.logging_mixin import LoggingMixin
Expand Down
3 changes: 2 additions & 1 deletion airflow/api/common/experimental/mark_tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@
from sqlalchemy import or_

from airflow.jobs import BackfillJob
from airflow.models import BaseOperator, DagRun, TaskInstance
from airflow.models import DagRun, TaskInstance
from airflow.models.baseoperator import BaseOperator
from airflow.operators.subdag_operator import SubDagOperator
from airflow.utils import timezone
from airflow.utils.db import provide_session
Expand Down
2 changes: 1 addition & 1 deletion airflow/configuration.py
Original file line number Diff line number Diff line change
Expand Up @@ -574,7 +574,7 @@ def get_airflow_test_config(airflow_home):
has_option = conf.has_option
remove_option = conf.remove_option
as_dict = conf.as_dict
set = conf.set # noqa
set = conf.set # noqa

for func in [load_test_config, get, getboolean, getfloat, getint, has_option,
remove_option, as_dict, set]:
Expand Down
2 changes: 1 addition & 1 deletion airflow/contrib/operators/dynamodb_to_s3.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
from boto.compat import json # type: ignore

from airflow.contrib.hooks.aws_dynamodb_hook import AwsDynamoDBHook
from airflow.models.baseoperator import BaseOperator
from airflow.models import BaseOperator
kaxil marked this conversation as resolved.
Show resolved Hide resolved
from airflow.providers.amazon.aws.hooks.s3 import S3Hook


Expand Down
2 changes: 1 addition & 1 deletion airflow/contrib/operators/qubole_operator.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
COMMAND_ARGS, HYPHEN_ARGS, POSITIONAL_ARGS, QuboleHook, flatten_list,
)
from airflow.hooks.base_hook import BaseHook
from airflow.models.baseoperator import BaseOperator, BaseOperatorLink
from airflow.models import BaseOperator, BaseOperatorLink
from airflow.models.taskinstance import TaskInstance
from airflow.utils.decorators import apply_defaults

Expand Down
2 changes: 1 addition & 1 deletion airflow/gcp/operators/bigquery.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
from airflow.exceptions import AirflowException
from airflow.gcp.hooks.bigquery import BigQueryHook
from airflow.gcp.hooks.gcs import GoogleCloudStorageHook, _parse_gcs_url
from airflow.models.baseoperator import BaseOperator, BaseOperatorLink
from airflow.models import BaseOperator, BaseOperatorLink
from airflow.models.taskinstance import TaskInstance
from airflow.operators.check_operator import CheckOperator, IntervalCheckOperator, ValueCheckOperator
from airflow.utils.decorators import apply_defaults
Expand Down
4 changes: 2 additions & 2 deletions airflow/models/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
# under the License.
"""Airflow models"""
from airflow.models.base import ID_LEN, Base # noqa: F401
from airflow.models.baseoperator import BaseOperator # noqa: F401
from airflow.models.baseoperator import BaseOperator, BaseOperatorLink # noqa: F401
from airflow.models.connection import Connection # noqa: F401
from airflow.models.dag import DAG, DagModel # noqa: F401
from airflow.models.dagbag import DagBag # noqa: F401
Expand All @@ -28,7 +28,6 @@
from airflow.models.kubernetes import KubeResourceVersion, KubeWorkerIdentifier # noqa: F401
from airflow.models.log import Log # noqa: F401
from airflow.models.pool import Pool # noqa: F401
from airflow.models.serialized_dag import SerializedDagModel # noqa: F401
from airflow.models.skipmixin import SkipMixin # noqa: F401
from airflow.models.slamiss import SlaMiss # noqa: F401
from airflow.models.taskfail import TaskFail # noqa: F401
Expand All @@ -39,4 +38,5 @@

# Load SQLAlchemy models during package initialization
# Must be loaded after loading DAG model.
# noinspection PyUnresolvedReferences
potiuk marked this conversation as resolved.
Show resolved Hide resolved
import airflow.jobs # noqa: F401 isort # isort:skip
Loading