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-5285] Pylint pre-commit filters out pylint_todo files #5884

Merged

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Aug 22, 2019

We have now all pylint checks incorporated into pre-commit hooks including filtering out the files from todo list. Also all checks are now moved to a single job in Travis, utilising multiple processors so it should be even faster and less overhead to run all the tests.

Make sure you have checked all steps below.

Jira

Description

  • Here are some details about my PR, including screenshots of any UI changes:

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

  • My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain docstrings that explain what it does
    • If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

Code Quality

  • Passes flake8

@potiuk
Copy link
Member Author

potiuk commented Aug 22, 2019

CC : @nuclearpinguin

@potiuk
Copy link
Member Author

potiuk commented Aug 22, 2019

@nuclearpinguin @mik-laj - I'd love if you test in on Mac running for --all-files and for a small change. I enabled parallel execution for the tests and previously when I tested it on Mac I had a feeling it is very slow but likely this was because of image rebuilding in parallel. Now we have a single test at the beginning which rebuilds the images once if needed (and REBUILD=true used) and all the other checks should run fast with already rebuilt image.

@turbaszek
Copy link
Member

turbaszek commented Aug 22, 2019

Sure I will test it ✅

@potiuk
Copy link
Member Author

potiuk commented Aug 22, 2019

Also I tried another variant (fixup) - where travis still runs pylint separately. I think this will be best for now as running all tests on Travis took 16 minutes https://travis-ci.org/apache/airflow/jobs/575437986 (we have two CPU machines there only and with limited memory). By having parallel pre-commit on a fast 8CPU machine this will go down to 3-4 minutes, but for now splitting the pylint jobs separately might be much better (but still we run it all via pre-commit). https://travis-ci.org/apache/airflow/builds/575447025

@potiuk
Copy link
Member Author

potiuk commented Aug 22, 2019

Just to give a heads-up - seems that it works fine -> it takes < 5 minutes on my Mac to run 'pre-commit run --all-files' :)

@mik-laj mik-laj added the area:dev-env CI, pre-commit, pylint and other changes that do not change the behavior of the final code label Aug 22, 2019
@potiuk
Copy link
Member Author

potiuk commented Aug 23, 2019

Trying more sensible split trying to optimize image builds and Travis.

@potiuk potiuk force-pushed the pre-commit-filters-todo-files-on-pylint branch from 3ba364b to 26307d8 Compare August 23, 2019 00:36
@potiuk
Copy link
Member Author

potiuk commented Aug 23, 2019

Seems like perfect split now:

Screenshot 2019-08-22 at 20 43 36

@turbaszek
Copy link
Member

turbaszek commented Aug 23, 2019

Running pre-commit run --all-files took 4m32s.

Small change in code and docs took 51s while only change in docs took 32s (I checked this because of feeling that check-apache-license take a relatively long time).

Change in operator, hook and tests took 1m25s so I think it's a quite acceptable time.

@ashb
Copy link
Member

ashb commented Aug 23, 2019

Out of interest how long does it take if you delete the check-apache-license step from your local config file?

@turbaszek
Copy link
Member

@ashb cira 4m10s, so same difference of ~20 seconds.

@potiuk
Copy link
Member Author

potiuk commented Aug 23, 2019

Then for the Travis build we also save time on downloading the image. In the latest version we only pull/build checklicence image for checklicence step, and only pull/build slim-ci image for the rest. That also saves a bit of time. Locally when you run the pre-commit both images will normally be already built so we only save time for running the test. It's highly optimised in this version.

@potiuk
Copy link
Member Author

potiuk commented Aug 23, 2019

Anything else needed to merge this one ?

@potiuk potiuk merged commit d24db82 into apache:master Aug 23, 2019
potiuk added a commit that referenced this pull request Aug 23, 2019
kaxil pushed a commit that referenced this pull request Aug 30, 2019
Jerryguo pushed a commit to Jerryguo/airflow that referenced this pull request Sep 2, 2019
…e#5884)

* [AIRFLOW-5285] Pylint pre-commit filters out pylint_todo files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dev-env CI, pre-commit, pylint and other changes that do not change the behavior of the final code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants