Skip to content

Commit

Permalink
Pylint checks should be way faster now (apache#10207)
Browse files Browse the repository at this point in the history
* Pylint checks should be way faster now

Instead of running separate pylint checks for tests and main source
we are running a single check now. This is possible thanks to a
nice hack - we have pylint plugin that injects the right
"# pylint: disable=" comment for all test files while reading
the file content by astroid (just before tokenization)

Thanks to that we can also separate out pylint checks
to a separate job in CI - this way all pylint checks will
be run in parallel to all other checks effectively halfing
the time needed to get the static check feedback and potentially
cancelling other jobs much faster.

* fixup! Pylint checks should be way faster now
  • Loading branch information
potiuk authored Aug 7, 2020
1 parent 1dc8b78 commit 9e3b7d9
Show file tree
Hide file tree
Showing 15 changed files with 116 additions and 165 deletions.
4 changes: 2 additions & 2 deletions .github/workflows/cancel_other_workflow_runs.yml
Original file line number Diff line number Diff line change
Expand Up @@ -32,5 +32,5 @@ jobs:
token: ${{ secrets.GITHUB_TOKEN }}
workflow: ci.yml
failFastJobNames: >
["^Static checks$", "^Build docs$", "^Backport packages$",
"^Checks: Helm tests$", "^Build prod image .*", "^Test OpenAPI*"]
["^Static checks.*", "^Build docs$", "^Backport packages$",
"^Checks: Helm tests$", "^Build prod image .*", "^Test OpenAPI.*"]
33 changes: 30 additions & 3 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,12 @@ jobs:

static-checks:
timeout-minutes: 60
name: "Static checks"
name: "Static checks: no pylint"
runs-on: ubuntu-latest
needs: [cancel-previous-workflow-run]
env:
MOUNT_SOURCE_DIR_FOR_STATIC_CHECKS: "true"
SKIP: "pylint"
steps:
- uses: actions/checkout@v2
- uses: actions/setup-python@v2
Expand All @@ -68,17 +69,43 @@ jobs:
- name: Cache pre-commit env
uses: actions/cache@v2
env:
cache-name: cache-pre-commit-v1
cache-name: cache-pre-commit-no-pylint-v1
with:
path: ~/.cache/pre-commit
key: ${{ env.cache-name }}-${{ github.job }}-${{ hashFiles('.pre-commit-config.yaml') }}
- name: "Free space"
run: ./scripts/ci/tools/ci_free_space_on_ci.sh
- name: "Build CI image"
run: ./scripts/ci/images/ci_prepare_ci_image_on_ci.sh
- name: "Static checks"
- name: "Static checks: no pylint"
run: ./scripts/ci/static_checks/ci_run_static_checks.sh

static-checks-pylint:
timeout-minutes: 60
name: "Static checks: pylint"
runs-on: ubuntu-latest
needs: [cancel-previous-workflow-run]
env:
MOUNT_SOURCE_DIR_FOR_STATIC_CHECKS: "true"
steps:
- uses: actions/checkout@v2
- uses: actions/setup-python@v2
with:
python-version: '3.7'
- name: Cache pre-commit env
uses: actions/cache@v2
env:
cache-name: cache-pre-commit-pylint-v1
with:
path: ~/.cache/pre-commit
key: ${{ env.cache-name }}-${{ github.job }}-${{ hashFiles('.pre-commit-config.yaml') }}
- name: "Free space"
run: ./scripts/ci/tools/ci_free_space_on_ci.sh
- name: "Build CI image"
run: ./scripts/ci/images/ci_prepare_ci_image_on_ci.sh
- name: "Static checks: pylint"
run: ./scripts/ci/static_checks/ci_run_static_checks.sh pylint

docs:
timeout-minutes: 60
name: "Build docs"
Expand Down
14 changes: 3 additions & 11 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -363,20 +363,12 @@ repos:
exclude: ^dev|^backport_packages
require_serial: true
- id: pylint
name: Run pylint for main sources
name: Run pylint
language: system
entry: "./scripts/ci/pre_commit/pre_commit_pylint_main.sh"
entry: "./scripts/ci/pre_commit/pre_commit_pylint.sh"
files: \.py$
exclude: ^tests/.*\.py$|^scripts/.*\.py$|^dev|^backport_packages|^kubernetes_tests
pass_filenames: true
require_serial: true # Pylint tests should be run in one chunk to detect all cycles
- id: pylint-tests
name: Run pylint for tests
language: system
entry: "./scripts/ci/pre_commit/pre_commit_pylint_tests.sh"
files: ^tests/.*\.py$
exclude: ^scripts/.*\.py$|^dev|^backport_packages
pass_filenames: true
require_serial: true
- id: flake8
name: Run flake8
language: system
Expand Down
12 changes: 4 additions & 8 deletions STATIC_CODE_CHECKS.rst
Original file line number Diff line number Diff line change
Expand Up @@ -230,13 +230,11 @@ To fix a pylint issue, do the following:
1. Remove module/modules from the
`scripts/ci/static_checks/pylint_todo.txt <scripts/ci/pylint_todo.txt>`__.

2. Run `scripts/ci/static_checks/ci_pylint_main.sh <scripts/ci/ci_pylint_main.sh>`__ and
`scripts/ci/ci_pylint_tests.sh <scripts/ci/static_checks/ci_pylint_tests.sh>`__.
2. Run `scripts/ci/static_checks/ci_pylint.sh <scripts/ci/ci_pylint.sh>`__.

3. Fix all the issues reported by pylint.

4. Re-run `scripts/ci/static_checks/ci_pylint_main.sh <scripts/ci/ci_pylint_main.sh>`__ and
`scripts/ci/ci_pylint_tests.sh <scripts/ci/static_checks/ci_pylint_tests.sh>`__.
4. Re-run `scripts/ci/static_checks/ci_pylint.sh <scripts/ci/ci_pylint.sh>`__.

5. If you see "success", submit a PR following
`Pull Request guidelines <#pull-request-guidelines>`__.
Expand Down Expand Up @@ -375,8 +373,7 @@ this, run the following scripts:
* `<scripts/ci/static_checks/ci_flake8.sh>`_ - runs Flake8 source code style enforcement tool.
* `<scripts/ci/static_checks/ci_lint_dockerfile.sh>`_ - runs lint checker for the dockerfiles.
* `<scripts/ci/static_checks/ci_mypy.sh>`_ - runs a check for mypy type annotation consistency.
* `<scripts/ci/static_checks/ci_pylint_main.sh>`_ - runs pylint static code checker for main files.
* `<scripts/ci/static_checks/ci_pylint_tests.sh>`_ - runs pylint static code checker for tests.
* `<scripts/ci/static_checks/ci_pylint.sh>`_ - runs pylint static code checker.

The scripts may ask you to rebuild the images, if needed.

Expand All @@ -393,8 +390,7 @@ If you are already in the Breeze Docker environment (by running the ``./breeze``
you can also run the same static checks via run_scripts:

* Mypy: ``./scripts/ci/in_container/run_mypy.sh airflow tests``
* Pylint for main files: ``./scripts/ci/in_container/run_pylint_main.sh``
* Pylint for test files: ``./scripts/ci/in_container/run_pylint_tests.sh``
* Pylint: ``./scripts/ci/in_container/run_pylint.sh``
* Flake8: ``./scripts/ci/in_container/run_flake8.sh``
* License check: ``./scripts/ci/in_container/run_check_licence.sh``
* Documentation: ``./scripts/ci/in_container/run_docs_build.sh``
Expand Down
2 changes: 1 addition & 1 deletion airflow/jobs/scheduler_job.py
Original file line number Diff line number Diff line change
Expand Up @@ -982,7 +982,7 @@ def _prepare_simple_dags(
return simple_dags


class SchedulerJob(BaseJob):
class SchedulerJob(BaseJob): # pylint: disable=too-many-instance-attributes
"""
This SchedulerJob runs for a specific time interval and schedules the jobs
that are ready to run. It figures out the latest runs for each
Expand Down
2 changes: 1 addition & 1 deletion pylintrc
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ limit-inference-results=100

# List of plugins (as comma separated values of python modules names) to load,
# usually to register additional checkers.
load-plugins=tests.airflow_pylint.do_not_use_asserts
load-plugins=tests.airflow_pylint.do_not_use_asserts,tests.airflow_pylint.disable_checks_for_tests

# Pickle collected data for later comparisons.
persistent=yes
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,6 @@ if [[ ${#@} == "0" ]]; then
-path "./.eggs" -prune -o \
-path "./docs/_build" -prune -o \
-path "./build" -prune -o \
-path "./tests" -prune -o \
-path "./kubernetes_tests" -prune -o \
-name "*.py" \
-not -name 'webserver_config.py' | \
grep ".*.py$" | \
Expand Down
51 changes: 0 additions & 51 deletions scripts/ci/in_container/run_pylint_tests.sh

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,5 @@
export FORCE_ANSWER_TO_QUESTIONS=${FORCE_ANSWER_TO_QUESTIONS:="quit"}
export REMEMBER_LAST_ANSWER="true"

# shellcheck source=scripts/ci/static_checks/ci_pylint_main.sh
. "$( dirname "${BASH_SOURCE[0]}" )/../static_checks/ci_pylint_main.sh" "${@}"
# shellcheck source=scripts/ci/static_checks/ci_pylint.sh
. "$( dirname "${BASH_SOURCE[0]}" )/../static_checks/ci_pylint.sh" "${@}"
22 changes: 0 additions & 22 deletions scripts/ci/pre_commit/pre_commit_pylint_tests.sh

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -20,19 +20,19 @@ export PYTHON_MAJOR_MINOR_VERSION=${PYTHON_MAJOR_MINOR_VERSION:-3.6}
# shellcheck source=scripts/ci/libraries/_script_init.sh
. "$( dirname "${BASH_SOURCE[0]}" )/../libraries/_script_init.sh"

function run_pylint_main() {
function run_pylint() {
FILES=("$@")
if [[ "${#FILES[@]}" == "0" ]]; then
docker run "${EXTRA_DOCKER_FLAGS[@]}" \
--entrypoint "/usr/local/bin/dumb-init" \
"${AIRFLOW_CI_IMAGE}" \
"--" "/opt/airflow/scripts/ci/in_container/run_pylint_main.sh" \
"--" "/opt/airflow/scripts/ci/in_container/run_pylint.sh" \
| tee -a "${OUTPUT_LOG}"
else
docker run "${EXTRA_DOCKER_FLAGS[@]}" \
--entrypoint "/usr/local/bin/dumb-init" \
"${AIRFLOW_CI_IMAGE}" \
"--" "/opt/airflow/scripts/ci/in_container/run_pylint_main.sh" "${FILES[@]}" \
"--" "/opt/airflow/scripts/ci/in_container/run_pylint.sh" "${FILES[@]}" \
| tee -a "${OUTPUT_LOG}"
fi
}
Expand All @@ -49,8 +49,8 @@ if [[ "${#@}" != "0" ]]; then
if [[ "${#FILTERED_FILES[@]}" == "0" ]]; then
echo "Filtered out all files. Skipping pylint."
else
run_pylint_main "${FILTERED_FILES[@]}"
run_pylint "${FILTERED_FILES[@]}"
fi
else
run_pylint_main
run_pylint
fi
57 changes: 0 additions & 57 deletions scripts/ci/static_checks/ci_pylint_tests.sh

This file was deleted.

60 changes: 60 additions & 0 deletions tests/airflow_pylint/disable_checks_for_tests.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
# 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.


from astroid import MANAGER, scoped_nodes
from pylint.lint import PyLinter

DISABLED_CHECKS_FOR_TESTS = \
"missing-docstring, no-self-use, too-many-public-methods, protected-access, do-not-use-asserts"


def register(_: PyLinter):
"""
Skip registering any plugin. This is not a real plugin - we only need it to register transform before
running pylint.
:param _:
:return:
"""


def transform(mod):
"""
It's a small hack but one that gives us a lot of speedup in pylint tests. We are replacing the first
line of the file with pylint-disable (or update existing one) when file name start with `test_` or
(for providers) when it is the full path of the package (both cases occur in pylint)
:param mod: astroid module
:return: None
"""
if mod.name.startswith("test_") or \
mod.name.startswith("tests.") or \
mod.name.startswith("kubernetes_tests."):
decoded_lines = mod.stream().read().decode("utf-8").split("\n")
if decoded_lines[0].startswith("# pylint: disable="):
decoded_lines[0] = decoded_lines[0] + " " + DISABLED_CHECKS_FOR_TESTS
elif decoded_lines[0].startswith("#") or decoded_lines[0].strip() == "":
decoded_lines[0] = "# pylint: disable=" + DISABLED_CHECKS_FOR_TESTS
else:
raise Exception(f"The first line of module {mod.name} is not a comment or empty. "
f"Please make sure it is!")
# pylint will read from `.file_bytes` attribute later when tokenization
mod.file_bytes = "\n".join(decoded_lines).encode("utf-8")


MANAGER.register_transform(scoped_nodes.Module, transform)
Loading

0 comments on commit 9e3b7d9

Please sign in to comment.