CI: add pre-commit action, include pyupgrade #36471

MarcoGorelli · 2020-09-19T07:15:22Z

xref this comment #36426 (comment) by @TomAugspurger

I would recommend running the linting commands on CI through pre-commit. That way the versions in .pre-commit-config.yaml are used everywhere.

So, this PR does that.

It also adds pyupgrade as a pre-commit hook (xref #36450 ) and removes some unnecessary configurations from the .pre-commit-config.yaml file (all of these hooks already specify language: python, see e.g. https://github.com/psf/black/blob/master/.pre-commit-hooks.yaml )

TODO

update https://pandas.pydata.org/docs/development/contributing.html#code-standards (thanks Ali!)

…age'

MarcoGorelli · 2020-09-19T07:16:02Z

.pre-commit-config.yaml

@@ -3,23 +3,19 @@ repos:
    rev: 19.10b0
    hooks:
    -   id: black
-        language_version: python3


This is already specified here: https://github.com/psf/black/blob/master/.pre-commit-hooks.yaml

MarcoGorelli · 2020-09-19T07:16:54Z

.pre-commit-config.yaml

 -   repo: https://gitlab.com/pycqa/flake8
    rev: 3.8.3
    hooks:
    -   id: flake8
-        language: python_venv


Same, they already specify this here https://gitlab.com/pycqa/flake8/-/blob/master/.pre-commit-hooks.yaml

and according to pre-commit docs https://pre-commit.com/

The python_venv language is now an alias to python

MarcoGorelli · 2020-09-19T07:21:48Z

.pre-commit-config.yaml

        files: \.pxi\.in$
        types:
          - file
        args: [--append-config=flake8/cython-template.cfg]
-   repo: https://github.com/pre-commit/mirrors-isort


this is deprecated, see https://github.com/pre-commit/mirrors-isort

This mirror repository is deprecated, use isort directly.

MarcoGorelli · 2020-09-19T07:22:21Z

.pre-commit-config.yaml

    hooks:
    -   id: isort
-        language: python_venv


already specified in https://github.com/PyCQA/isort/blob/develop/.pre-commit-hooks.yaml

ci/code_checks.sh

alimcmaster1 · 2020-09-19T16:23:07Z

Might need to rework the code_check docs if we do this: https://pandas.pydata.org/docs/development/contributing.html#code-standards

jreback · 2020-09-19T20:03:55Z

ci/code_checks.sh

@@ -48,31 +48,6 @@ fi
 ### LINTING ###
 if [[ -z "$CHECK" || "$CHECK" == "lint" ]]; then

-    echo "black --version"


how are these no longer needed?

They're run during the pre-commit CI check, see https://github.com/pandas-dev/pandas/pull/36471/checks?check_run_id=1137265216 . Could always duplicate them just in case if you like

but do we run the pre-commit checks on CI? i think have a script that is separate from the pre-commit is needed as well (it can duplicate or be called by pre-commit), but key is to have everything in sync.

Two cents: I think keeping this for a time just to ensure it never disagrees with pre-commit makes sense, then can remove later once we're fairly sure they're identical (also makes it easier to revert if misconfigured)

but do we run the pre-commit checks on CI?

That's what this PR adds - if you go down to "All checks have passed" and expand, you'll see an extra GitHub Action called "pre-commit". If you expand that one further, it says

Run pre-commit/[email protected] install pre-commit Cache Size: ~21 MB (21895398 B) /bin/tar --use-compress-program zstd -d -xf /home/runner/work/_temp/f09139a0-b8b5-427f-bc76-10b2403b8816/cache.tzst -P -C /home/runner/work/pandas/pandas /opt/hostedtoolcache/Python/3.8.5/x64/bin/pre-commit run --show-diff-on-failure --color=always --all-files black....................................................................Passed flake8...................................................................Passed flake8-pyx...............................................................Passed flake8-pxd...............................................................Passed isort....................................................................Passed pyupgrade................................................................Passed

Two cents: I think keeping this for a time just to ensure it never disagrees with pre-commit makes sense, then can remove later once we're fairly sure they're identical (also makes it easier to revert if misconfigured)

This seems like a good approach! Also would be good to check that github actions CI fails if any of these pre commit stages doesn’t pass.

Sure - have added the ci/code checks back in for now then

Also would be good to check that github actions CI fails if any of these pre commit stages doesn’t pass.

From my experience using this in my own repo (https://github.com/nbQA-dev/nbQA) that's what happens

Should we open an issue to remember to deduplicate these checks later?

.github/workflows/pre-commit.yml

TomAugspurger · 2020-09-21T14:18:57Z

Thanks. Happy to have these run in both code_checks and the pre-commit GH Action for a bit to make sure things are working properly.

simonjayhawkins · 2020-09-21T14:29:54Z

@MarcoGorelli should pyupgrade be added to environment.yml?

MarcoGorelli · 2020-09-21T16:25:11Z

@MarcoGorelli should pyupgrade be added to environment.yml?

Not necessary, as pre-commit runs it a separate environment and that's the only place here pyupgrade is being used

https://github.com/pandas-dev/pandas/pull/36471/checks?check_run_id=1143999007 shows:

black....................................................................Passed
flake8...................................................................Passed
flake8-pyx...............................................................Passed
flake8-pxd...............................................................Passed
isort....................................................................Passed
pyupgrade................................................................Passed

simonjayhawkins · 2020-09-21T17:12:07Z

pre-commit for the contributor is optional. (https://pandas.pydata.org/pandas-docs/dev/development/contributing.html#pre-commit)

so as a reviewer, what's the process to recommend if a pyupgrade failure occurs. advise the contributor to install pyupgrade or does the output of the failed job tell you what needs to be done?

MarcoGorelli · 2020-09-21T17:29:24Z

pre-commit for the contributor is optional. (https://pandas.pydata.org/pandas-docs/dev/development/contributing.html#pre-commit)

so as a reviewer, what's the process to recommend if a pyupgrade failure occurs. advise the contributor to install pyupgrade or does the output of the failed job tell you what needs to be done?

That's a really good point, and yes - it'll show you the diff, e.g. see here https://github.com/pymc-devs/pymc3/runs/1121338004 for an example from PyMC3, which shows:

nbqa-isort...............................................................Failed
- hook id: nbqa
- files were modified by this hook

Fixing /home/runner/work/pymc3/pymc3/docs/source/notebooks/GLM-logistic.ipynb

pre-commit hook(s) made changes.
If you are seeing this message in CI, reproduce locally with: `pre-commit run --all-files`.
To run `pre-commit` as part of git workflow, use `pre-commit install`.
All changes made by hooks:
diff --git a/docs/source/notebooks/GLM-logistic.ipynb b/docs/source/notebooks/GLM-logistic.ipynb
index 96c9d11..71d53e8 100644
--- a/docs/source/notebooks/GLM-logistic.ipynb
+++ b/docs/source/notebooks/GLM-logistic.ipynb
@@ -29,6 +29,9 @@
     }
    ],
    "source": [
+    "from collections import OrderedDict\n",
+    "from time import time\n",
+    "\n",
     "import matplotlib.pyplot as plt\n",
     "import numpy as np\n",
     "import pandas as pd\n",
@@ -37,10 +40,8 @@
     "import theano as thno\n",
     "import theano.tensor as T\n",
     "\n",
-    "from collections import OrderedDict\n",
     "from scipy import integrate\n",
     "from scipy.optimize import fmin_powell\n",
-    "from time import time\n",
     "\n",
     "print(\"Running on PyMC3 v{}\".format(pm.__version__))"
    ]
##[error]The process '/opt/hostedtoolcache/Python/3.8.5/x64/bin/pre-commit' failed with exit code 1

dsaxton · 2020-09-21T17:44:33Z

@MarcoGorelli should pyupgrade be added to environment.yml?

Since all the other development tools (e.g., testing, typing and linting) are in environment.yml I think adding it makes sense, especially if it's also in CI (would probably minimize friction later when errors do occur). Thoughts @MarcoGorelli ?

MarcoGorelli · 2020-09-21T17:48:42Z

Makes sense - have added that an pre-commit (since its the recommended way to run pyupgrade)

simonjayhawkins · 2020-09-21T17:56:26Z

Makes sense - have added that an pre-commit (since its the recommended way to run pyupgrade)

AFAIK pre-commit doesn't run out-of-the-box on Windows after using pip install pre-commit

you get bash: pre-commit: command not found in git bash and in anaconda prompt

'pre-commit' is not recognized as an internal or external command,
operable program or batch file.

so I think additional instructions/link to relevant docs could be included to lower the barrier to entry for new contributors.

MarcoGorelli · 2020-09-22T07:38:31Z

🤔 I have it working on my Windows laptop for work, pretty sure all I did was pip install pre-commit in the anaconda prompt. Is that not your experience? Could you try again with their latest version?

The docs already link to the pre-commit website https://pandas.pydata.org/pandas-docs/stable/development/contributing.html#pre-commit , do you think more should be added (in terms of installation instructions)?

simonjayhawkins · 2020-09-22T10:51:18Z

🤔 I have it working on my Windows laptop for work, pretty sure all I did was pip install pre-commit in the anaconda prompt. Is that not your experience? Could you try again with their latest version?

using pip install -U pre-commit updated from 2.6.0 -> 2.7.1 in the environment that I was using and solves 'pre-commit' is not recognized

but creating a clean environment from environment.yml (from this PR)

(test) C:\Users\simon\pandas>conda list pre-commit
# packages in environment at C:\Users\simon\Anaconda3\envs\test:
#
# Name                    Version                   Build  Channel
pre-commit                2.7.1            py38h32f6830_0    conda-forge

(test) C:\Users\simon\pandas>pre-commit
[INFO] Initializing environment for https://github.com/PyCQA/isort.
[INFO] Initializing environment for https://github.com/asottile/pyupgrade.
[INFO] Installing environment for https://github.com/PyCQA/isort.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
An unexpected error has occurred: CalledProcessError: command: ('C:\\Users\\simon\\Anaconda3\\envs\\test\\python.exe', '-mvirtualenv', 'C:\\Users\\simon\\.cache\\pre-commit\\repo6988w0ti\\py_env-default', '-p', 'C:\\Users\\simon\\Anaconda3\\envs\\test\\python.exe')
return code: 1
expected return code: 0
stdout:
    ModuleNotFoundError: No module named 'virtualenv.seed.via_app_data'

stderr: (none)
Check the log at C:\Users\simon\.cache\pre-commit\pre-commit.log

moving pre-commit to the pip section and creating a new environment from that seems to work out-the-box.

(test2) C:\Users\simon\pandas>conda list pre-commit
# packages in environment at C:\Users\simon\Anaconda3\envs\test2:
#
# Name                    Version                   Build  Channel
pre-commit                2.7.1                    pypi_0    pypi

(test2) C:\Users\simon\pandas>pre-commit
[WARNING] Unstaged files detected.
[INFO] Stashing unstaged files to C:\Users\simon\.cache\pre-commit\patch1600770739.
[INFO] Installing environment for https://github.com/PyCQA/isort.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/asottile/pyupgrade.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
black................................................(no files to check)Skipped
flake8...............................................(no files to check)Skipped
flake8-pyx...........................................(no files to check)Skipped
flake8-pxd...........................................(no files to check)Skipped
isort................................................(no files to check)Skipped
pyupgrade............................................(no files to check)Skipped
[INFO] Restored changes from C:\Users\simon\.cache\pre-commit\patch1600770739.

of course, this comparison is not very well controlled without caches etc being cleared and the first environment installing pre-commit using conda now works as well.

(test) C:\Users\simon\pandas>pre-commit
[WARNING] Unstaged files detected.
[INFO] Stashing unstaged files to C:\Users\simon\.cache\pre-commit\patch1600771578.
black................................................(no files to check)Skipped
flake8...............................................(no files to check)Skipped
flake8-pyx...........................................(no files to check)Skipped
flake8-pxd...........................................(no files to check)Skipped
isort................................................(no files to check)Skipped
pyupgrade............................................(no files to check)Skipped
[INFO] Restored changes from C:\Users\simon\.cache\pre-commit\patch1600771578.

and I'm can't remember what I did in the past to get pre-commit to work in conda environments that may have affected my settings/configuration.

so maybe just leave as is and see if any other contributors report issues.

MarcoGorelli · 2020-09-22T12:43:36Z

Cool, thanks for the detailed report

jreback · 2020-09-22T13:54:35Z

thanks @MarcoGorelli lgtm. @TomAugspurger @simonjayhawkins if ok pls merge.

simonjayhawkins

lgtm.

TomAugspurger · 2020-09-22T14:31:10Z

Thanks!

MarcoGorelli added 3 commits September 19, 2020 08:05

add CI pre-commit check, include pyupgrade, remove unnecessary 'langu…

8197c61

…age'

upgrade doc/sphinxext/announce.py

1a6b477

remove doubled checks

e8135b4

MarcoGorelli commented Sep 19, 2020

View reviewed changes

update deprecated repo

f4788fa

MarcoGorelli commented Sep 19, 2020

View reviewed changes

ci/code_checks.sh Outdated Show resolved Hide resolved

add flake8-rst back to code_checks

27e285e

MarcoGorelli requested a review from TomAugspurger September 19, 2020 07:37

alimcmaster1 added the CI Continuous Integration label Sep 19, 2020

jreback requested changes Sep 19, 2020

View reviewed changes

dsaxton reviewed Sep 20, 2020

View reviewed changes

.github/workflows/pre-commit.yml Show resolved Hide resolved

dsaxton reviewed Sep 20, 2020

View reviewed changes

.github/workflows/pre-commit.yml Show resolved Hide resolved

don't remove from code checks yet

1e5bc80

MarcoGorelli added 2 commits September 21, 2020 18:42

📝 update code standards

9c98406

double-backtick

635b8d7

MarcoGorelli added 2 commits September 21, 2020 18:46

➕ add pyupgrade to environment.yml

e2a334a

➕ add pre-commit to environment.yml

b8aea46

generate pip reqs from conda

5b6faa1

jreback added this to the 1.2 milestone Sep 22, 2020

jreback approved these changes Sep 22, 2020

View reviewed changes

simonjayhawkins approved these changes Sep 22, 2020

View reviewed changes

TomAugspurger merged commit e1d0b67 into pandas-dev:master Sep 22, 2020

MarcoGorelli deleted the pyupgrade-pre-commit branch September 22, 2020 16:17

dsaxton mentioned this pull request Sep 25, 2020

CI: Deduplicate linting checks #36642

Closed

kesmit13 pushed a commit to kesmit13/pandas that referenced this pull request Nov 2, 2020

CI: add pre-commit action, include pyupgrade (pandas-dev#36471)

6b76569

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CI: add pre-commit action, include pyupgrade #36471

CI: add pre-commit action, include pyupgrade #36471

MarcoGorelli commented Sep 19, 2020 •

edited

Loading

MarcoGorelli Sep 19, 2020

MarcoGorelli Sep 19, 2020

MarcoGorelli Sep 19, 2020

MarcoGorelli Sep 19, 2020

alimcmaster1 commented Sep 19, 2020

jreback Sep 19, 2020

MarcoGorelli Sep 19, 2020 •

edited

Loading

jreback Sep 19, 2020

dsaxton Sep 20, 2020

MarcoGorelli Sep 20, 2020 •

edited

Loading

alimcmaster1 Sep 20, 2020

MarcoGorelli Sep 21, 2020

dsaxton Sep 25, 2020

simonjayhawkins Sep 25, 2020

dsaxton Sep 25, 2020

TomAugspurger commented Sep 21, 2020

simonjayhawkins commented Sep 21, 2020

MarcoGorelli commented Sep 21, 2020

simonjayhawkins commented Sep 21, 2020

MarcoGorelli commented Sep 21, 2020 •

edited

Loading

dsaxton commented Sep 21, 2020

MarcoGorelli commented Sep 21, 2020

simonjayhawkins commented Sep 21, 2020

MarcoGorelli commented Sep 22, 2020

simonjayhawkins commented Sep 22, 2020

MarcoGorelli commented Sep 22, 2020

jreback commented Sep 22, 2020

simonjayhawkins left a comment

TomAugspurger commented Sep 22, 2020

CI: add pre-commit action, include pyupgrade #36471

CI: add pre-commit action, include pyupgrade #36471

Conversation

MarcoGorelli commented Sep 19, 2020 • edited Loading

TODO

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alimcmaster1 commented Sep 19, 2020

Choose a reason for hiding this comment

MarcoGorelli Sep 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoGorelli Sep 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAugspurger commented Sep 21, 2020

simonjayhawkins commented Sep 21, 2020

MarcoGorelli commented Sep 21, 2020

simonjayhawkins commented Sep 21, 2020

MarcoGorelli commented Sep 21, 2020 • edited Loading

dsaxton commented Sep 21, 2020

MarcoGorelli commented Sep 21, 2020

simonjayhawkins commented Sep 21, 2020

MarcoGorelli commented Sep 22, 2020

simonjayhawkins commented Sep 22, 2020

MarcoGorelli commented Sep 22, 2020

jreback commented Sep 22, 2020

simonjayhawkins left a comment

Choose a reason for hiding this comment

TomAugspurger commented Sep 22, 2020

MarcoGorelli commented Sep 19, 2020 •

edited

Loading

MarcoGorelli Sep 19, 2020 •

edited

Loading

MarcoGorelli Sep 20, 2020 •

edited

Loading

MarcoGorelli commented Sep 21, 2020 •

edited

Loading