From 7f84059996e16ec23507c1f7e802d2abfe8c9655 Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Fri, 18 Oct 2024 19:38:34 +0200 Subject: [PATCH 1/2] Remove "from __future__" from airflow/providers __init__.py Cleans-up airflow and providers `__init__.py" files in order to get providers import work again. This is done by excluding the two `__init__.py` files from automated ruff isort rules adding `from __future__ import annotations`. Also removed the `__init__.py` file from "providers" directory, it is not needed there, because "providers" is just a folder where we keep provider files, it's not a Python package. That should finally get rid of the Intellij teething import problem that has been introduced in #42505. There were earlier - unsuccesful - attempts to fix it in the #43116 and #43081 that followed #42951 - but the key is that Pycharm requires the namespace's extend_path to be first "real" line of code in the `__init__.py` to understand that the package is an "explicit" namespace package - and it conflicts with the requirement of "from __future__ import annotations" to be the first line of Python code. Also this PR fixes a few other teething problems with setup of tests that were introcuded in #42505 and #43802 "masked" by having `__init__.py` added in providers package: * common.sql interface pre-commit used wrong path to generated files * openlineage extractor test that should not expect "providers.tests.*" but "tests.*" package * common_sql_api_stubs wrongly calculating generated path for stub-generated files * pytest_plugin expecting .asf.yml in "airflow" sources - even during compatibility tests with older version of airflow (where the .asf.yml is not present) --- airflow/__init__.py | 5 ++- providers/__init__.py | 23 ------------- providers/src/airflow/providers/__init__.py | 4 ++- .../tests/openlineage/extractors/test_base.py | 2 +- pyproject.toml | 3 +- .../pre_commit/update_common_sql_api_stubs.py | 4 +-- tests_common/pytest_plugin.py | 33 ++++++++++--------- 7 files changed, 30 insertions(+), 44 deletions(-) delete mode 100644 providers/__init__.py diff --git a/airflow/__init__.py b/airflow/__init__.py index f6c40b5091bbc..287aa499faa42 100644 --- a/airflow/__init__.py +++ b/airflow/__init__.py @@ -15,7 +15,10 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. -from __future__ import annotations + +# We do not use "from __future__ import annotations" here because it is not supported +# by Pycharm when we want to make sure all imports in airflow work from namespace packages +# Adding it automatically is excluded in pyproject.toml via I002 ruff rule exclusion # Make `airflow` a namespace package, supporting installing # airflow.providers.* in different locations (i.e. one in site, and one in user diff --git a/providers/__init__.py b/providers/__init__.py deleted file mode 100644 index c53de5451bd08..0000000000000 --- a/providers/__init__.py +++ /dev/null @@ -1,23 +0,0 @@ -# Licensed to the Apache Software Foundation (ASF) under one -# or more contributor license agreements. See the NOTICE file -# distributed with this work for additional information -# regarding copyright ownership. The ASF licenses this file -# to you under the Apache License, Version 2.0 (the -# "License"); you may not use this file except in compliance -# with the License. You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, -# software distributed under the License is distributed on an -# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -# KIND, either express or implied. See the License for the -# specific language governing permissions and limitations -# under the License. - -# This exists so that pytest doesn't get confused about namespace packages -# and think that `tests/conftest.py` and `providers/tests/conftest.py` are -# both "tests.conftest" -# -# This is a temporary solution until https://github.com/apache/airflow/issues/42632 -# is done diff --git a/providers/src/airflow/providers/__init__.py b/providers/src/airflow/providers/__init__.py index 66fbd04b36e4d..adfa8eb4b98ce 100644 --- a/providers/src/airflow/providers/__init__.py +++ b/providers/src/airflow/providers/__init__.py @@ -16,7 +16,9 @@ # specific language governing permissions and limitations # under the License. -from __future__ import annotations +# We do not use "from __future__ import annotations" here because it is not supported +# by Pycharm when we want to make sure all imports in airflow work from namespace packages +# Adding it automatically is excluded in pyproject.toml via I002 ruff rule exclusion # Make `airflow` a namespace package, supporting installing # airflow.providers.* in different locations (i.e. one in site, and one in user diff --git a/providers/tests/openlineage/extractors/test_base.py b/providers/tests/openlineage/extractors/test_base.py index 15c96ac675530..bc4f36c91ec9a 100644 --- a/providers/tests/openlineage/extractors/test_base.py +++ b/providers/tests/openlineage/extractors/test_base.py @@ -277,7 +277,7 @@ def test_extract_on_failure(task_state, is_airflow_2_10_or_higher, should_call_o @mock.patch("airflow.providers.openlineage.conf.custom_extractors") def test_extractors_env_var(custom_extractors): - custom_extractors.return_value = {"providers.tests.openlineage.extractors.test_base.ExampleExtractor"} + custom_extractors.return_value = {"tests.openlineage.extractors.test_base.ExampleExtractor"} extractor = ExtractorManager().get_extractor_class(ExampleOperator(task_id="example")) assert extractor is ExampleExtractor diff --git a/pyproject.toml b/pyproject.toml index 1b1f5954505d6..f4512a382621f 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -346,9 +346,10 @@ section-order = [ testing = ["dev", "providers.tests", "task_sdk.tests", "tests_common", "tests"] [tool.ruff.lint.extend-per-file-ignores] -"airflow/__init__.py" = ["F401", "TCH004"] +"airflow/__init__.py" = ["F401", "TCH004", "I002"] "airflow/models/__init__.py" = ["F401", "TCH004"] "airflow/models/sqla_models.py" = ["F401"] +"providers/src/airflow/providers/__init__.py" = ["I002"] # The test_python.py is needed because adding __future__.annotations breaks runtime checks that are # needed for the test to work diff --git a/scripts/ci/pre_commit/update_common_sql_api_stubs.py b/scripts/ci/pre_commit/update_common_sql_api_stubs.py index 371c758146a2e..ebd484b660ad4 100755 --- a/scripts/ci/pre_commit/update_common_sql_api_stubs.py +++ b/scripts/ci/pre_commit/update_common_sql_api_stubs.py @@ -44,7 +44,7 @@ ) COMMON_SQL_ROOT = (PROVIDERS_ROOT / "common" / "sql").resolve(strict=True) OUT_DIR = AIRFLOW_SOURCES_ROOT_PATH / "out" -OUT_DIR_PROVIDERS = OUT_DIR / PROVIDERS_ROOT.relative_to(AIRFLOW_SOURCES_ROOT_PATH) +OUT_DIR_PROVIDERS = OUT_DIR / "providers" COMMON_SQL_PACKAGE_PREFIX = "airflow.providers.common.sql." @@ -337,7 +337,7 @@ def compare_stub_files(generated_stub_path: Path, force_override: bool) -> tuple generated_path = OUT_DIR_PROVIDERS / target_path.relative_to(PROVIDERS_ROOT) if not generated_path.exists(): console.print( - f"[red]The {target_path} file is missing in generated files:. " + f"[red]The {generated_path} file is missing in generated files:. " f"This is treated as breaking change." ) total_removals += 1 diff --git a/tests_common/pytest_plugin.py b/tests_common/pytest_plugin.py index f2dab17b2dddb..0c7ed57fea668 100644 --- a/tests_common/pytest_plugin.py +++ b/tests_common/pytest_plugin.py @@ -371,21 +371,24 @@ def initialize_airflow_tests(request): def pytest_configure(config: pytest.Config) -> None: # Ensure that the airflow sources dir is at the end of the sys path if it's not already there. Needed to # run import from `providers/tests/` - desired = AIRFLOW_SOURCES_ROOT_DIR.as_posix() - for path in sys.path: - if path == desired: - break - else: - # This "desired" path should be the Airflow source directory (repo root) - assert (AIRFLOW_SOURCES_ROOT_DIR / ".asf.yaml").exists(), f"Path {desired} is not Airflow root" - sys.path.append(desired) - - if (backend := config.getoption("backend", default=None)) and backend not in SUPPORTED_DB_BACKENDS: - msg = ( - f"Provided DB backend {backend!r} not supported, " - f"expected one of: {', '.join(map(repr, SUPPORTED_DB_BACKENDS))}" - ) - pytest.exit(msg, returncode=6) + if os.environ.get("USE_AIRFLOW_VERSION") == "": + # if USE_AIRFLOW_VERSION is not empty, we are running tests against the installed version of Airflow + # and providers so there is no need to add the sources directory to the path + desired = AIRFLOW_SOURCES_ROOT_DIR.as_posix() + for path in sys.path: + if path == desired: + break + else: + # This "desired" path should be the Airflow source directory (repo root) + assert (AIRFLOW_SOURCES_ROOT_DIR / ".asf.yaml").exists(), f"Path {desired} is not Airflow root" + sys.path.append(desired) + + if (backend := config.getoption("backend", default=None)) and backend not in SUPPORTED_DB_BACKENDS: + msg = ( + f"Provided DB backend {backend!r} not supported, " + f"expected one of: {', '.join(map(repr, SUPPORTED_DB_BACKENDS))}" + ) + pytest.exit(msg, returncode=6) config.addinivalue_line("markers", "integration(name): mark test to run with named integration") config.addinivalue_line("markers", "backend(name): mark test to run with named backend") From 78e990cd615e6f33366f46076ac72f81e8a45d0a Mon Sep 17 00:00:00 2001 From: Kaxil Naik Date: Fri, 18 Oct 2024 22:00:52 +0100 Subject: [PATCH 2/2] Revert providers/__init__.py changes --- providers/__init__.py | 23 +++++++++++++++++++ .../tests/openlineage/extractors/test_base.py | 2 +- .../pre_commit/update_common_sql_api_stubs.py | 4 ++-- 3 files changed, 26 insertions(+), 3 deletions(-) create mode 100644 providers/__init__.py diff --git a/providers/__init__.py b/providers/__init__.py new file mode 100644 index 0000000000000..c53de5451bd08 --- /dev/null +++ b/providers/__init__.py @@ -0,0 +1,23 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# This exists so that pytest doesn't get confused about namespace packages +# and think that `tests/conftest.py` and `providers/tests/conftest.py` are +# both "tests.conftest" +# +# This is a temporary solution until https://github.com/apache/airflow/issues/42632 +# is done diff --git a/providers/tests/openlineage/extractors/test_base.py b/providers/tests/openlineage/extractors/test_base.py index bc4f36c91ec9a..15c96ac675530 100644 --- a/providers/tests/openlineage/extractors/test_base.py +++ b/providers/tests/openlineage/extractors/test_base.py @@ -277,7 +277,7 @@ def test_extract_on_failure(task_state, is_airflow_2_10_or_higher, should_call_o @mock.patch("airflow.providers.openlineage.conf.custom_extractors") def test_extractors_env_var(custom_extractors): - custom_extractors.return_value = {"tests.openlineage.extractors.test_base.ExampleExtractor"} + custom_extractors.return_value = {"providers.tests.openlineage.extractors.test_base.ExampleExtractor"} extractor = ExtractorManager().get_extractor_class(ExampleOperator(task_id="example")) assert extractor is ExampleExtractor diff --git a/scripts/ci/pre_commit/update_common_sql_api_stubs.py b/scripts/ci/pre_commit/update_common_sql_api_stubs.py index ebd484b660ad4..371c758146a2e 100755 --- a/scripts/ci/pre_commit/update_common_sql_api_stubs.py +++ b/scripts/ci/pre_commit/update_common_sql_api_stubs.py @@ -44,7 +44,7 @@ ) COMMON_SQL_ROOT = (PROVIDERS_ROOT / "common" / "sql").resolve(strict=True) OUT_DIR = AIRFLOW_SOURCES_ROOT_PATH / "out" -OUT_DIR_PROVIDERS = OUT_DIR / "providers" +OUT_DIR_PROVIDERS = OUT_DIR / PROVIDERS_ROOT.relative_to(AIRFLOW_SOURCES_ROOT_PATH) COMMON_SQL_PACKAGE_PREFIX = "airflow.providers.common.sql." @@ -337,7 +337,7 @@ def compare_stub_files(generated_stub_path: Path, force_override: bool) -> tuple generated_path = OUT_DIR_PROVIDERS / target_path.relative_to(PROVIDERS_ROOT) if not generated_path.exists(): console.print( - f"[red]The {generated_path} file is missing in generated files:. " + f"[red]The {target_path} file is missing in generated files:. " f"This is treated as breaking change." ) total_removals += 1