fIx isort problems introduced by recent isort release (#28434)

The recent isort changed their mind on sorting the imports. This change follows the change and bumps isort to latest released version (isort has no install_requires on its own so bumping min version has no effect on other dependencies) This change adds a number of isort:skip_file, isort:off, isort:skips in order to handle a very annoying bug in isort, that no matter how much you try, it sometimes treat "known first party" packages differently - depending on how many files it processes at a time. We should be able to restore it after this bug is fixed: PyCQA/isort#2045 This change also updates the common.sql API to skip them from isort for the very same reason (depending on how many files are modified, the isort order might change. GitOrigin-RevId: f115b207bc844c10569b2df6fc9acfa32a3c7f41
GoogleCloudPlatform · Nov 7, 2023 · 43dcdeb · 43dcdeb
1 parent e516356
commit 43dcdeb
Show file tree

Hide file tree

Showing 16 changed files with 95 additions and 31 deletions.
diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
@@ -146,6 +146,21 @@ repos:
           - --fuzzy-match-generates-todo
         files: >
           \.cfg$|\.conf$|\.ini$|\.ldif$|\.properties$|\.readthedocs$|\.service$|\.tf$|Dockerfile.*$
+  - repo: local
+    hooks:
+      - id: update-common-sql-api-stubs
+        name: Check and update common.sql API stubs
+        entry: ./scripts/ci/pre_commit/pre_commit_update_common_sql_api_stubs.py
+        language: python
+        files: ^scripts/ci/pre_commit/pre_commit_update_common_sql_api\.py|^airflow/providers/common/sql/.*\.pyi?$
+        additional_dependencies: ['rich>=12.4.4', 'mypy==0.971', 'black==22.3.0', 'jinja2']
+        pass_filenames: false
+        require_serial: true
+  - repo: https://github.com/PyCQA/isort
+    rev: 5.11.2
+    hooks:
+      - id: isort
+        name: Run isort to sort imports in Python files
   # Keep version of black in sync wit blacken-docs, pre-commit-hook-names, update-common-sql-api-stubs
   - repo: https://github.com/psf/black
     rev: 22.3.0
@@ -233,11 +248,6 @@ repos:
         entry: yamllint -c yamllint-config.yml --strict
         types: [yaml]
         exclude: ^.*init_git_sync\.template\.yaml$|^.*airflow\.template\.yaml$|^chart/(?:templates|files)/.*\.yaml$|openapi/.*\.yaml$|^\.pre-commit-config\.yaml$|^airflow/_vendor/
-  - repo: https://github.com/PyCQA/isort
-    rev: 5.10.1
-    hooks:
-      - id: isort
-        name: Run isort to sort imports in Python files
   - repo: https://github.com/pycqa/pydocstyle
     rev: 6.1.1
     hooks:
@@ -396,14 +406,6 @@ repos:
         language: python
         files: ^docs
         pass_filenames: false
-      - id: update-common-sql-api-stubs
-        name: Check and update common.sql API stubs
-        entry: ./scripts/ci/pre_commit/pre_commit_update_common_sql_api_stubs.py
-        language: python
-        files: ^scripts/ci/pre_commit/pre_commit_update_common_sql_api\.py|^airflow/providers/common/sql/.*\.pyi?$
-        additional_dependencies: ['rich>=12.4.4', 'mypy==0.971', 'black==22.3.0']
-        pass_filenames: false
-        require_serial: true
       - id: check-pydevd-left-in-code
         language: pygrep
         name: Check for pydevd debug statements accidentally left

diff --git a/airflow/providers/common/sql/hooks/sql.pyi b/airflow/providers/common/sql/hooks/sql.pyi
@@ -27,6 +27,10 @@
 #
 # You can read more in the README_API.md file
 #
+"""
+Definition of the public interface for airflow.providers.common.sql.hooks.sql
+isort:skip_file
+"""
 from _typeshed import Incomplete
 from airflow.hooks.dbapi import DbApiHook as BaseForDbApiHook
 from typing import Any, Callable, Iterable, Mapping, Sequence

diff --git a/airflow/providers/common/sql/operators/sql.pyi b/airflow/providers/common/sql/operators/sql.pyi
@@ -27,6 +27,10 @@
 #
 # You can read more in the README_API.md file
 #
+"""
+Definition of the public interface for airflow.providers.common.sql.operators.sql
+isort:skip_file
+"""
 from _typeshed import Incomplete
 from airflow.models import BaseOperator, SkipMixin
 from airflow.providers.common.sql.hooks.sql import DbApiHook

diff --git a/airflow/providers/common/sql/sensors/sql.pyi b/airflow/providers/common/sql/sensors/sql.pyi
@@ -27,6 +27,10 @@
 #
 # You can read more in the README_API.md file
 #
+"""
+Definition of the public interface for airflow.providers.common.sql.sensors.sql
+isort:skip_file
+"""
 from _typeshed import Incomplete
 from airflow.sensors.base import BaseSensorOperator
 from typing import Any, Sequence

diff --git a/docker_tests/test_docker_compose_quick_start.py b/docker_tests/test_docker_compose_quick_start.py
@@ -28,10 +28,13 @@
 
 import requests
 
+# isort:off (needed to workaround isort bug)
 from docker_tests.command_utils import run_command
 from docker_tests.constants import SOURCE_ROOT
 from docker_tests.docker_tests_utils import docker_image
 
+# isort:on (needed to workaround isort bug)
+
 AIRFLOW_WWW_USER_USERNAME = os.environ.get("_AIRFLOW_WWW_USER_USERNAME", "airflow")
 AIRFLOW_WWW_USER_PASSWORD = os.environ.get("_AIRFLOW_WWW_USER_PASSWORD", "airflow")
 DAG_ID = "example_bash_operator"

diff --git a/docker_tests/test_examples_of_prod_image_building.py b/docker_tests/test_examples_of_prod_image_building.py
@@ -25,9 +25,12 @@
 import pytest
 import requests
 
+# isort:off (needed to workaround isort bug)
 from docker_tests.command_utils import run_command
 from docker_tests.constants import SOURCE_ROOT
 
+# isort:on (needed to workaround isort bug)
+
 DOCKER_EXAMPLES_DIR = SOURCE_ROOT / "docs" / "docker-stack" / "docker-examples"
 
 

diff --git a/docker_tests/test_prod_image.py b/docker_tests/test_prod_image.py
@@ -24,6 +24,7 @@
 
 import pytest
 
+# isort:off (needed to workaround isort bug)
 from docker_tests.command_utils import run_command
 from docker_tests.constants import SOURCE_ROOT
 from docker_tests.docker_tests_utils import (
@@ -32,6 +33,8 @@
     run_bash_in_docker,
     run_python_in_docker,
 )
+
+# isort:on (needed to workaround isort bug)
 from setup import PREINSTALLED_PROVIDERS
 
 INSTALLED_PROVIDER_PATH = SOURCE_ROOT / "scripts" / "ci" / "installed_providers.txt"

diff --git a/docs/build_docs.py b/docs/build_docs.py
@@ -15,6 +15,11 @@
 # KIND, either express or implied.  See the License for the
 # specific language governing permissions and limitations
 # under the License.
+"""
+Builds documentation and runs spell checking
+
+# isort:skip_file (needed to workaround isort bug)
+"""
 from __future__ import annotations
 
 import argparse
@@ -25,9 +30,6 @@
 from itertools import filterfalse, tee
 from typing import Callable, Iterable, NamedTuple, TypeVar
 
-from rich.console import Console
-from tabulate import tabulate
-
 from docs.exts.docs_build import dev_index_generator, lint_checks
 from docs.exts.docs_build.code_utils import CONSOLE_WIDTH, PROVIDER_INIT_FILE
 from docs.exts.docs_build.docs_builder import DOCS_DIR, AirflowDocsBuilder, get_available_packages
@@ -37,6 +39,9 @@
 from docs.exts.docs_build.package_filter import process_package_filters
 from docs.exts.docs_build.spelling_checks import SpellingError, display_spelling_error_summary
 
+from rich.console import Console
+from tabulate import tabulate
+
 TEXT_RED = "\033[31m"
 TEXT_RESET = "\033[0m"
 

diff --git a/docs/exts/docs_build/dev_index_generator.py b/docs/exts/docs_build/dev_index_generator.py
@@ -23,8 +23,11 @@
 
 import jinja2
 
+# isort:off (needed to workaround isort bug)
 from docs.exts.provider_yaml_utils import load_package_data
 
+# isort:on (needed to workaround isort bug)
+
 CURRENT_DIR = os.path.abspath(os.path.dirname(__file__))
 DOCS_DIR = os.path.abspath(os.path.join(CURRENT_DIR, os.pardir, os.pardir))
 BUILD_DIR = os.path.abspath(os.path.join(DOCS_DIR, "_build"))

diff --git a/docs/exts/docs_build/errors.py b/docs/exts/docs_build/errors.py
@@ -23,7 +23,9 @@
 from rich.console import Console
 
 from airflow.utils.code_utils import prepare_code_snippet
-from docs.exts.docs_build.code_utils import CONSOLE_WIDTH
+
+from docs.exts.docs_build.code_utils import CONSOLE_WIDTH  # isort:skip (needed to workaround isort bug)
+
 
 CURRENT_DIR = os.path.abspath(os.path.join(os.path.dirname(__file__)))
 DOCS_DIR = os.path.abspath(os.path.join(CURRENT_DIR, os.pardir, os.pardir))

diff --git a/docs/publish_docs.py b/docs/publish_docs.py
@@ -21,10 +21,13 @@
 import argparse
 import os
 
+# isort:off (needed to workaround isort bug)
 from exts.docs_build.docs_builder import AirflowDocsBuilder
 from exts.docs_build.package_filter import process_package_filters
 from exts.provider_yaml_utils import load_package_data
 
+# isort:on (needed to workaround isort bug)
+
 AIRFLOW_SITE_DIR = os.environ.get("AIRFLOW_SITE_DIRECTORY")
 
 

diff --git a/kubernetes_tests/test_kubernetes_executor.py b/kubernetes_tests/test_kubernetes_executor.py
@@ -20,7 +20,7 @@
 
 import pytest
 
-from kubernetes_tests.test_base import EXECUTOR, TestBase
+from kubernetes_tests.test_base import EXECUTOR, TestBase  # isort:skip (needed to workaround isort bug)
 
 
 @pytest.mark.skipif(EXECUTOR != "KubernetesExecutor", reason="Only runs on KubernetesExecutor")

diff --git a/kubernetes_tests/test_other_executors.py b/kubernetes_tests/test_other_executors.py
@@ -20,7 +20,7 @@
 
 import pytest
 
-from kubernetes_tests.test_base import EXECUTOR, TestBase
+from kubernetes_tests.test_base import EXECUTOR, TestBase  # isort:skip (needed to workaround isort bug)
 
 
 # These tests are here because only KubernetesExecutor can run the tests in

diff --git a/pyproject.toml b/pyproject.toml
@@ -35,5 +35,6 @@ known_first_party = ["airflow", "airflow_breeze", "docker_tests", "docs", "kuber
 # The test_python.py is needed because adding __future__.annotations breaks runtime checks that are
 # needed for the test to work
 skip = ["build", ".tox", "venv", "tests/decorators/test_python.py"]
+lines_between_types = 0
 skip_glob = ["*.pyi"]
 profile = "black"
diff --git a/scripts/ci/pre_commit/pre_commit_update_common_sql_api_stubs.py b/scripts/ci/pre_commit/pre_commit_update_common_sql_api_stubs.py
@@ -21,6 +21,8 @@
 import os
 import shutil
 import subprocess
+
+import jinja2
 import sys
 import textwrap
 from functools import lru_cache
@@ -139,18 +141,22 @@ def post_process_historically_publicised_methods(stub_file_path: Path, line: str
         new_lines.append(line)
 
 
-def post_process_generated_stub_file(stub_file_path: Path, lines: list[str], patch_historical_methods=False):
+def post_process_generated_stub_file(
+    module_name: str, stub_file_path: Path, lines: list[str], patch_historical_methods=False
+):
     """
     Post process the stub file - add the preamble and optionally patch historical methods.
     Adding preamble always, makes sure that we can update the preamble and have it automatically updated
     in generated files even if no API specification changes.
 
+    :param module_name: name of the module of the file
     :param stub_file_path: path of the stub fil
     :param lines: lines that were read from the file (with stripped comments)
     :param patch_historical_methods:  whether we should patch historical methods
     :return: resulting lines of the file after post-processing
     """
-    new_lines = PREAMBLE.splitlines()
+    template = jinja2.Template(PREAMBLE)
+    new_lines = template.render(module_name=module_name).splitlines()
     for line in lines:
         if patch_historical_methods:
             post_process_historically_publicised_methods(stub_file_path, line, new_lines)
@@ -159,28 +165,39 @@ def post_process_generated_stub_file(stub_file_path: Path, lines: list[str], pat
     return new_lines
 
 
-def read_pyi_file_content(pyi_file_path: Path, patch_historical_methods=False) -> list[str] | None:
+def read_pyi_file_content(
+    module_name: str, pyi_file_path: Path, patch_historical_methods=False
+) -> list[str] | None:
     """
     Reads stub file content with post-processing and optionally patching historical methods. The comments
-    are stripped and preamble is always added. It makes sure that we can update the preamble
-    and have it automatically updated in generated files even if no API specification changes.
+    and initial javadoc are stripped and preamble is always added. It makes sure that we can update
+    the preamble and have it automatically updated in generated files even if no API specification changes.
 
     If None is returned, the file should be deleted.
 
-    :param pyi_file_path:
-    :param patch_historical_methods:
+    :param module_name: name of the module in question
+    :param pyi_file_path: the path of the file to read
+    :param patch_historical_methods: whether the historical methods should be patched
     :return: list of lines of post-processed content or None if the file should be deleted.
     """
-    lines = [
+    lines_no_comments = [
         line
         for line in pyi_file_path.read_text(encoding="utf-8").splitlines()
         if line.strip() and not line.strip().startswith("#")
     ]
+    remove_docstring = False
+    lines = []
+    for line in lines_no_comments:
+        if line.strip().startswith('"""'):
+            remove_docstring = not remove_docstring
+            continue
+        if not remove_docstring:
+            lines.append(line)
     if (pyi_file_path.name == "__init__.pyi") and lines == []:
         console.print(f"[yellow]Skip {pyi_file_path} as it is an empty stub for __init__.py file")
         return None
     return post_process_generated_stub_file(
-        pyi_file_path, lines, patch_historical_methods=patch_historical_methods
+        module_name, pyi_file_path, lines, patch_historical_methods=patch_historical_methods
     )
 
 
@@ -194,7 +211,10 @@ def compare_stub_files(generated_stub_path: Path, force_override: bool) -> tuple
     _removals, _additions = 0, 0
     rel_path = generated_stub_path.relative_to(OUT_DIR)
     target_path = PROVIDERS_ROOT / rel_path
-    generated_pyi_content = read_pyi_file_content(generated_stub_path, patch_historical_methods=True)
+    module_name = "airflow.providers." + os.fspath(rel_path.with_suffix("")).replace(os.path.sep, ".")
+    generated_pyi_content = read_pyi_file_content(
+        module_name, generated_stub_path, patch_historical_methods=True
+    )
     if generated_pyi_content is None:
         os.unlink(generated_stub_path)
         if target_path.exists():
@@ -217,7 +237,7 @@ def compare_stub_files(generated_stub_path: Path, force_override: bool) -> tuple
         console.print(f"[yellow]New file {target_path} has been missing. Treated as addition.")
         target_path.write_text("\n".join(generated_pyi_content), encoding="utf-8")
         return 0, 1
-    target_pyi_content = read_pyi_file_content(target_path, patch_historical_methods=False)
+    target_pyi_content = read_pyi_file_content(module_name, target_path, patch_historical_methods=False)
     if target_pyi_content is None:
         target_pyi_content = []
     if generated_pyi_content != target_pyi_content:
@@ -288,6 +308,10 @@ def compare_stub_files(generated_stub_path: Path, force_override: bool) -> tuple
 #
 # You can read more in the README_API.md file
 #
+\"\"\"
+Definition of the public interface for {{ module_name }}
+isort:skip_file
+\"\"\"
 """
 
 

diff --git a/setup.py b/setup.py
@@ -377,7 +377,10 @@ def write_version(filename: str = str(AIRFLOW_SOURCES_ROOT / "airflow" / "git_ve
     "flaky",
     "gitpython",
     "ipdb",
-    "isort",
+    # make sure that we are using stable sorting order from 5.* version (some changes were introduced
+    # in 5.11.3. Black is not compatible yet, so we need to limit isort
+    # we can remove the limit when black and isort agree on the order
+    "isort==5.11.2",
     "jira",
     "jsondiff",
     "mongomock",