[SPARK-37380][PYTHON] Miscellaneous Python lint infra cleanup #34655

nchammas · 2021-11-19T00:24:22Z

What changes were proposed in this pull request?

This PR makes a small number of tweaks to our Python lint infra.

pylintrc is obsolete and not used.
tox.ini supports multiline values, so I've broken up the extremely long lines that were in there.
A requirement was missing from requirements.txt so I added it in there.
I've removed pycodestyle and merged its configs into flake8's, since flake8 includes pycodestyle checks.

Why are the changes needed?

General maintenance of our internal Python infra.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

The existing dev/lint-python script.

nchammas · 2021-11-19T00:26:37Z

dev/lint-python

@@ -275,8 +275,7 @@ SPARK_ROOT_DIR="$(dirname "${SCRIPT_DIR}")"

 pushd "$SPARK_ROOT_DIR" &> /dev/null

-# skipping local ruby bundle directory from the search
-PYTHON_SOURCE="$(find . -path ./docs/.local_ruby_bundle -prune -false -o -name "*.py")"
+PYTHON_SOURCE="$(git ls-files '*.py')"


This is an improvement, but I don't think this is that great, either. In general, I don't like that we are building a huge string of every Python file and passing it as an argument.

Instead, we should be using the appropriate include and exclude filters (e.g. via tox.ini) to capture everything to be tested. But I don't want to get into that here.

nchammas · 2021-11-19T00:28:30Z

dev/requirements.txt

@@ -19,6 +19,7 @@ coverage

 # Linter
 mypy
+git+https://github.com/typeddjango/pytest-mypy-plugins.git@b0020061f48e85743ee3335bd62a3a608d17c6bd


This was being used in CI but it was missing from here, so I added it in.

In a future PR, I want to reorganize things so that CI references the requirements file. I don't think it's good that we have dependencies specified here and then specified again inside our CI script.

HyukjinKwon · 2021-11-19T00:50:21Z

cc @zero323 and @Fokko FYI

SparkQA · 2021-11-19T01:38:06Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/49891/

SparkQA · 2021-11-19T02:40:01Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/49891/

SparkQA · 2021-11-19T03:48:49Z

Test build #145417 has finished for PR 34655 at commit cee4d6a.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

zero323 · 2021-11-19T14:32:03Z

If we are already here let me ask a stupid question ‒ why do we use both pycodestyle and flake8. Isn't the former one redundant?

nchammas · 2021-11-19T15:40:17Z

Flake8 bundles other tools, including pycodestyle. So yes, we can probably merge the pycodestyle configs into the Flake8 configs within tox.ini:

spark/dev/tox.ini

Lines 16 to 19 in 4f20898

    
           [pycodestyle] 
        
           ignore=E203,E226,E241,E305,E402,E722,E731,E741,W503,W504,E501 
        
           max-line-length=100 
        
           exclude=*/target/*,python/pyspark/cloudpickle/*.py,shared.py,python/docs/source/conf.py,work/*/*.py,python/.eggs/*,dist/*,.git/*,dev/ansible-for-test-node/*

I'll go ahead and do that.

SparkQA · 2021-11-19T18:58:56Z

Test build #145462 has finished for PR 34655 at commit 48cd470.

This patch fails Python style tests.
This patch merges cleanly.
This patch adds no public classes.

nchammas · 2021-11-19T19:09:32Z

Test build #145462 has finished for PR 34655 at commit 48cd470.

This patch fails Python style tests.

This patch merges cleanly.

This patch adds no public classes.

I suspect this is because Jenkins is using a different version of Flake8 than what is pinned in GitHub CI. (This is why I really want us to single-source all Python dev requirements.)

spark/dev/ansible-for-test-node/roles/jenkins-worker/files/python_environments/spark-py36-spec.txt

Line 100 in 4e1e33b

https://repo.anaconda.com/pkgs/main/noarch/flake8-3.9.0-pyhd3eb1b0_0.conda

I will try to fix this. Hopefully, I won't need any special access to Jenkins.

nchammas · 2021-11-19T19:24:06Z

OK, never mind. I will need to push updates to the Jenkins workers via Ansible to fix this, which I don't want to get into here.

Instead, I'll bump down the version of Flake8 to match what's in Jenkins.

SparkQA · 2021-11-19T19:53:16Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/49934/

SparkQA · 2021-11-19T20:39:00Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/49934/

nchammas · 2021-11-19T21:28:25Z

Turns out there was some difference in Flake8's behavior when running Python 3.6 vs. 3.9, so I've adjusted the configs accordingly. Should be fixed now.

SparkQA · 2021-11-19T22:25:10Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/49939/

SparkQA · 2021-11-19T23:06:12Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/49939/

SparkQA · 2021-11-20T00:40:30Z

Test build #145467 has finished for PR 34655 at commit 5418ec5.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

srowen

Seems reasonable to me

nchammas · 2021-11-22T19:26:52Z

@zero323 - Is the cleanup of pycodestyle configs along the lines of what you were expecting?

zero323 · 2021-11-22T20:30:53Z

@zero323 - Is the cleanup of pycodestyle configs along the lines of what you were expecting?

Looks sensible, but I wouldn't mind more eyes on this.

nchammas · 2021-11-23T15:18:15Z

@HyukjinKwon - Is there anyone else you think should review this?

HyukjinKwon · 2021-11-23T23:58:05Z

Merged to master.

github-actions bot added BUILD PYTHON labels Nov 19, 2021

nchammas commented Nov 19, 2021

View reviewed changes

HyukjinKwon changed the title ~~[SPARK-37380] Miscellaneous Python lint infra cleanup~~ [SPARK-37380][PYTHON] Miscellaneous Python lint infra cleanup Nov 19, 2021

nchammas added 9 commits November 19, 2021 11:09

list python files via git

74d6f50

include missing dependency from CI

ef40b64

reformat tox.ini for readability

1e6d51e

delete obsolete pylintrc

3f6eb5f

cut out pycodestyle

ee1ddc5

merge pycodestyle configs into flake8 configs

f2f33fb

pin CI version of flake8

2fe5007

code tweaks to fit updated lint config

6740403

more cleanup to tox.ini

48cd470

nchammas force-pushed the SPARK-37380-python-lint-infra branch from cee4d6a to 48cd470 Compare November 19, 2021 18:54

github-actions bot added CORE INFRA ML SQL labels Nov 19, 2021

flake8 config changes for py36

5418ec5

srowen reviewed Nov 21, 2021

View reviewed changes

HyukjinKwon approved these changes Nov 23, 2021

View reviewed changes

HyukjinKwon closed this in c1029dc Nov 23, 2021

nchammas deleted the SPARK-37380-python-lint-infra branch November 24, 2021 15:30

This was referenced Jan 7, 2022

[SPARK-37823][INFRA][TESTS] Add is-changed.py dev script #35112

Closed

[SPARK-37834][PYTHON][INFRA] Reenable length check in Python linter #35123

Closed

[SPARK-37836][PYTHON][INFRA] Enable F841, E722, E305 and E226 for PEP 8 compliance #35126

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-37380][PYTHON] Miscellaneous Python lint infra cleanup #34655

[SPARK-37380][PYTHON] Miscellaneous Python lint infra cleanup #34655

nchammas commented Nov 19, 2021 •

edited

nchammas Nov 19, 2021 •

edited

nchammas Nov 19, 2021

HyukjinKwon commented Nov 19, 2021

SparkQA commented Nov 19, 2021

SparkQA commented Nov 19, 2021

SparkQA commented Nov 19, 2021

zero323 commented Nov 19, 2021

nchammas commented Nov 19, 2021

SparkQA commented Nov 19, 2021

nchammas commented Nov 19, 2021 •

edited

nchammas commented Nov 19, 2021

SparkQA commented Nov 19, 2021

SparkQA commented Nov 19, 2021

nchammas commented Nov 19, 2021

SparkQA commented Nov 19, 2021

SparkQA commented Nov 19, 2021

SparkQA commented Nov 20, 2021

srowen left a comment

nchammas commented Nov 22, 2021

zero323 commented Nov 22, 2021

nchammas commented Nov 23, 2021

HyukjinKwon commented Nov 23, 2021

[SPARK-37380][PYTHON] Miscellaneous Python lint infra cleanup #34655

[SPARK-37380][PYTHON] Miscellaneous Python lint infra cleanup #34655

Conversation

nchammas commented Nov 19, 2021 • edited

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

nchammas Nov 19, 2021 • edited

Choose a reason for hiding this comment

nchammas Nov 19, 2021

Choose a reason for hiding this comment

HyukjinKwon commented Nov 19, 2021

SparkQA commented Nov 19, 2021

SparkQA commented Nov 19, 2021

SparkQA commented Nov 19, 2021

zero323 commented Nov 19, 2021

nchammas commented Nov 19, 2021

SparkQA commented Nov 19, 2021

nchammas commented Nov 19, 2021 • edited

nchammas commented Nov 19, 2021

SparkQA commented Nov 19, 2021

SparkQA commented Nov 19, 2021

nchammas commented Nov 19, 2021

SparkQA commented Nov 19, 2021

SparkQA commented Nov 19, 2021

SparkQA commented Nov 20, 2021

srowen left a comment

Choose a reason for hiding this comment

nchammas commented Nov 22, 2021

zero323 commented Nov 22, 2021

nchammas commented Nov 23, 2021

HyukjinKwon commented Nov 23, 2021

nchammas commented Nov 19, 2021 •

edited

nchammas Nov 19, 2021 •

edited

nchammas commented Nov 19, 2021 •

edited