Skip to content

Commit

Permalink
Re-enables MyPy in non-failure mode (#19890)
Browse files Browse the repository at this point in the history
This PR enables mypy back as pre-commit for local changes after
the #19317 switched to Python 3.7 but also it separates out
mypy to a separate non-failing step in CI.

In the CI we will be able to see remaining mypy errors.

This will allow us to gradually fix all the mypy errors and enable
mypy back when we got all the problems fixed.
  • Loading branch information
potiuk authored Nov 30, 2021
1 parent 33a4502 commit 15a9d79
Show file tree
Hide file tree
Showing 10 changed files with 68 additions and 37 deletions.
10 changes: 8 additions & 2 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,6 @@ jobs:
needs: [build-info, ci-images]
env:
RUNS_ON: ${{ fromJson(needs.build-info.outputs.runsOn) }}
SKIP: "identity"
MOUNT_SELECTED_LOCAL_SOURCES: "true"
PYTHON_MAJOR_MINOR_VERSION: ${{needs.build-info.outputs.defaultPythonVersion}}
if: needs.build-info.outputs.basic-checks-only == 'false'
Expand Down Expand Up @@ -471,10 +470,17 @@ ${{ hashFiles('.pre-commit-config.yaml') }}"
with:
path: 'airflow/ui/node_modules'
key: ${{ runner.os }}-ui-node-modules-${{ hashFiles('airflow/ui/**/yarn.lock') }}
- name: "Static checks"
- name: "Static checks (no mypy)"
run: ./scripts/ci/static_checks/run_static_checks.sh
env:
VERBOSE: false
SKIP: "identity,mypy"
- name: "MyPy static checks"
run: ./scripts/ci/static_checks/run_static_checks.sh mypy
env:
VERBOSE: false
DO_NOT_FAIL_ON_ERROR: "true"
COLUMNS: 300

# Those checks are run if no image needs to be built for checks. This is for simple changes that
# Do not touch any of the python code or any of the important files that might require building
Expand Down
9 changes: 5 additions & 4 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -721,22 +721,23 @@ repos:
- id: mypy
name: Run mypy
language: system
entry: ./scripts/ci/pre_commit/pre_commit_mypy.sh
entry: ./scripts/ci/pre_commit/pre_commit_mypy.sh --namespace-packages
files: \.py$
exclude: ^dev|^provider_packages|^chart|^docs|^airflow/_vendor/
exclude: ^provider_packages|^chart|^docs|^airflow/_vendor/
require_serial: true
- id: mypy
name: Run mypy for helm chart tests
language: system
entry: ./scripts/ci/pre_commit/pre_commit_mypy.sh
files: ^chart/.*\.py$
require_serial: false
require_serial: true
- id: mypy
name: Run mypy for /docs/ folder
language: system
entry: ./scripts/ci/pre_commit/pre_commit_mypy.sh
files: ^docs/.*\.py$
exclude: rtd-deprecation
require_serial: false
require_serial: true
- id: flake8
name: Run flake8
language: system
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
try:
from yaml import CSafeLoader as SafeLoader
except ImportError:
from yaml import SafeLoader # type: ignore[no-redef]
from yaml import SafeLoader # type: ignore


def main() -> int:
Expand Down
12 changes: 6 additions & 6 deletions scripts/ci/pre_commit/pre_commit_check_provider_yaml_files.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
from collections import Counter
from glob import glob
from itertools import chain, product
from typing import Any, Dict, Iterable, List
from typing import Any, Dict, Iterable, List, Set

import jsonschema
import yaml
Expand All @@ -34,7 +34,7 @@
try:
from yaml import CSafeLoader as SafeLoader
except ImportError:
from yaml import SafeLoader # type: ignore[no-redef]
from yaml import SafeLoader # type: ignore

if __name__ != "__main__":
raise Exception(
Expand Down Expand Up @@ -80,7 +80,7 @@ def _load_package_data(package_paths: Iterable[str]):
return result


def get_all_integration_names(yaml_files):
def get_all_integration_names(yaml_files) -> List[str]:
all_integrations = [
i['integration-name'] for f in yaml_files.values() if 'integrations' in f for i in f["integrations"]
]
Expand Down Expand Up @@ -137,7 +137,7 @@ def assert_sets_equal(set1, set2):


def check_if_objects_belongs_to_package(
object_names: List[str], provider_package: str, yaml_file_path: str, resource_type: str
object_names: Set[str], provider_package: str, yaml_file_path: str, resource_type: str
):
for object_name in object_names:
if not object_name.startswith(provider_package):
Expand Down Expand Up @@ -283,8 +283,8 @@ def check_invalid_integration(yaml_files: Dict[str, Dict]):

def check_doc_files(yaml_files: Dict[str, Dict]):
print("Checking doc files")
current_doc_urls = []
current_logo_urls = []
current_doc_urls: List[str] = []
current_logo_urls: List[str] = []
for provider in yaml_files.values():
if 'integrations' in provider:
current_doc_urls.extend(
Expand Down
2 changes: 2 additions & 0 deletions scripts/ci/pre_commit/pre_commit_json_schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,8 @@ def _default_validator(validator, default, instance, schema):
def _load_spec(spec_file: Optional[str], spec_url: Optional[str]):
if spec_url:
spec_file = fetch_and_cache(url=spec_url, output_filename=re.sub(r"[^a-zA-Z0-9]", "-", spec_url))
if not spec_file:
raise Exception(f"The {spec_file} was None and {spec_url} did not lead to any file loading.")
with open(spec_file) as schema_file:
schema = json.loads(schema_file.read())
return schema
Expand Down
3 changes: 0 additions & 3 deletions scripts/ci/pre_commit/pre_commit_mypy.sh
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,5 @@ export FORCE_ANSWER_TO_QUESTIONS=${FORCE_ANSWER_TO_QUESTIONS:="quit"}
export REMEMBER_LAST_ANSWER="true"
export PRINT_INFO_FROM_SCRIPTS="false"

# Temporarily remove mypy checks until we fix them for Python 3.7
exit 0

# shellcheck source=scripts/ci/static_checks/mypy.sh
. "$( dirname "${BASH_SOURCE[0]}" )/../static_checks/mypy.sh" "${@}"
2 changes: 1 addition & 1 deletion scripts/ci/static_checks/mypy.sh
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ function run_mypy() {
files=("$@")
fi

docker_v run "${EXTRA_DOCKER_FLAGS[@]}" \
docker_v run "${EXTRA_DOCKER_FLAGS[@]}" -t \
--entrypoint "/usr/local/bin/dumb-init" \
"-v" "${AIRFLOW_SOURCES}/.mypy_cache:/opt/airflow/.mypy_cache" \
"${AIRFLOW_CI_IMAGE_WITH_TAG}" \
Expand Down
11 changes: 11 additions & 0 deletions scripts/ci/static_checks/run_static_checks.sh
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,13 @@ python -m pip install --user pre-commit \

export PATH=~/.local/bin:${PATH}

if [[ ${DO_NOT_FAIL_ON_ERROR=} != "" ]]; then
echo
echo "Skip failing on error"
echo
set +e
fi

if [[ $# == "0" ]]; then
pre-commit run --all-files --show-diff-on-failure --color always
else
Expand All @@ -42,3 +49,7 @@ else
pre-commit run "${pre_commit_check}" --all-files --show-diff-on-failure --color always
done
fi

if [[ ${DO_NOT_FAIL_ON_ERROR=} != "" ]]; then
exit 0
fi
16 changes: 1 addition & 15 deletions scripts/in_container/run_mypy.sh
Original file line number Diff line number Diff line change
Expand Up @@ -20,18 +20,4 @@
. "$( dirname "${BASH_SOURCE[0]}" )/_in_container_script_init.sh"
export PYTHONPATH=${AIRFLOW_SOURCES}

mypy_args=()

# Mypy doesn't cope very well with namespace packages when give filenames (it gets confused about the lack of __init__.py in airflow.providers, and thinks airflow.providers.docker is the same as a "docker" top level module).
#
# So we instead need to convert the file names in to module names to check
for filename in "$@";
do
if [[ "${filename}" == docs/* ]]; then
mypy_args+=("$filename")
else
mypy_args+=("-m" "$(filename_to_python_module "$filename")")
fi
done

mypy --namespace-packages "${mypy_args[@]}"
mypy "${@}"
38 changes: 33 additions & 5 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -513,6 +513,31 @@ def write_version(filename: str = os.path.join(*[my_dir, "airflow", "git_version
]
# End dependencies group

# Mypy 0.900 and above ships only with stubs from stdlib so if we need other stubs, we need to install them
# manually as `types-*`. See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
# for details. Wy want to install them explicitly because we want to eventually move to
# mypyd which does not support installing the types dynamically with --install-types
mypy_dependencies = [
'mypy==0.910',
'types-croniter',
'types-docutils',
'types-freezegun',
'types-paramiko',
'types-protobuf',
'types-python-dateutil',
'types-python-slugify',
'types-pytz',
'types-redis',
'types-requests',
'types-setuptools',
'types-termcolor',
'types-tabulate',
'types-Markdown',
'types-PyMySQL',
'types-PyYAML',
]

# Dependencies needed for development only
devel_only = [
'aws_xray_sdk',
'beautifulsoup4~=4.7.1',
Expand All @@ -533,7 +558,6 @@ def write_version(filename: str = os.path.join(*[my_dir, "airflow", "git_version
'jsondiff',
'mongomock',
'moto~=2.2, >=2.2.12',
'mypy==0.770',
'parameterized',
'paramiko',
'pipdeptree',
Expand All @@ -558,7 +582,7 @@ def write_version(filename: str = os.path.join(*[my_dir, "airflow", "git_version
'yamllint',
]

devel = cgroups + devel_only + doc + kubernetes + mysql + pandas + password
devel = cgroups + devel_only + doc + kubernetes + mypy_dependencies + mysql + pandas + password
devel_hadoop = devel + hdfs + hive + kerberos + presto + webhdfs

# Dict of all providers which are part of the Apache Airflow repository together with their requirements
Expand Down Expand Up @@ -893,6 +917,10 @@ def get_all_provider_packages() -> str:
class AirflowDistribution(Distribution):
"""The setuptools.Distribution subclass with Airflow specific behaviour"""

def __init__(self, attrs=None):
super().__init__(attrs)
self.install_requires = None

def parse_config_files(self, *args, **kwargs) -> None:
"""
Ensure that when we have been asked to install providers from sources
Expand Down Expand Up @@ -989,7 +1017,7 @@ def add_all_provider_packages() -> None:
class Develop(develop_orig):
"""Forces removal of providers in editable mode."""

def run(self) -> None:
def run(self) -> None: # type: ignore
self.announce('Installing in editable mode. Uninstalling provider packages!', level=log.INFO)
# We need to run "python3 -m pip" because it might be that older PIP binary is in the path
# And it results with an error when running pip directly (cannot import pip module)
Expand Down Expand Up @@ -1052,11 +1080,11 @@ def include_provider_namespace_packages_when_installing_from_sources() -> None:
'extra_clean': CleanCommand,
'compile_assets': CompileAssets,
'list_extras': ListExtras,
'install': Install,
'install': Install, # type: ignore
'develop': Develop,
},
test_suite='setup.airflow_test_suite',
**setup_kwargs,
**setup_kwargs, # type: ignore
)


Expand Down

0 comments on commit 15a9d79

Please sign in to comment.