[SPARK-25962][BUILD][PYTHON] Specify minimum versions for both pydocstyle and flake8 in 'lint-python' script #22963

HyukjinKwon · 2018-11-07T10:52:06Z

What changes were proposed in this pull request?

This PR explicitly specifies flake8 and pydocstyle versions.

It checks flake8 binary executable
flake8 version check >= 3.5.0
pydocstyle >= 3.0.0 (previously it was == 3.0.0)

How was this patch tested?

Manually tested.

…on' script

HyukjinKwon · 2018-11-07T10:52:29Z

dev/lint-python

-flake8 . --count --select=E901,E999,F821,F822,F823 --max-line-length=100 --show-source --statistics
-flake8_status="${PIPESTATUS[0]}"
+# Check by flake8
+if hash "$FLAKE8BUILD" 2> /dev/null; then


I manually tested each branch.

SparkQA · 2018-11-07T10:54:54Z

Test build #98545 has finished for PR 22963 at commit e597243.

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

HyukjinKwon · 2018-11-07T10:58:24Z

dev/lint-python

+FLAKE8BUILD="flake8"
+MINIMUM_FLAKE8="3.4.0"
+
 SPHINXBUILD=${SPHINXBUILD:=sphinx-build}


@shaneknapp and @cloud-fan, it seems flake8 does work but indeed the problem was compatibility with pycodestyle within flake8 itself.

Our script uses pycodestyle 2.4.0 but it's manually downloaded. So, Jenkins's pycodestyle might be different or old and this might be the actual root cause.

I locally tested with latest pycodestyle and flake 3.6.0 and it worked.

BTW, i tested flake8 3.0.0, 3.1.0, 3.2.0, 3.3.0, 3.4.0, 3.5.0 and 3.6.0 while I am here.

SparkQA · 2018-11-07T11:00:10Z

Test build #98546 has finished for PR 22963 at commit b7f0e2f.

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

HyukjinKwon · 2018-11-07T11:02:50Z

dev/lint-python

 fi

-# stop the build if there are Python syntax errors or undefined names
-flake8 . --count --select=E901,E999,F821,F822,F823 --max-line-length=100 --show-source --statistics


BTW, it was never a good idea to use external executable program directly without any check.

SparkQA · 2018-11-07T11:05:05Z

Test build #98547 has finished for PR 22963 at commit a7192a3.

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

SparkQA · 2018-11-07T11:10:13Z

Test build #98548 has finished for PR 22963 at commit bef4190.

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

HyukjinKwon · 2018-11-07T11:46:01Z

cc @srowen, @holdenk and @rekhajoshm

SparkQA · 2018-11-07T15:53:52Z

Test build #98549 has finished for PR 22963 at commit 802a9d4.

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

HyukjinKwon · 2018-11-08T04:26:07Z

Merged to master.

HyukjinKwon · 2018-11-08T04:26:16Z

Thanks, @srowen and @dongjoon-hyun.

shaneknapp · 2018-11-08T19:08:50Z

sorry to jump in late on this, but i just wanted to check in on some of this stuff... dev/lint-python is a nightmare and i just wanted to discuss a couple of things:

the script downloads pycodestyle if it's not the right version. however, flake8, pycodestyle and sphinx are all installed and managed by the build system (see versions @ the end of this comment). i'll update all of the workers (and the ansible) to install the right version(s) of things as found in the script today.
in addition to (1), i'd like to do one of two things: either remove the pycodestyle download step, OR add installation steps for flake8, pydocstyle and sphinx.
we currently don't have pydocstyle installed on any of the workers. i am more than happy to install it immediately, but am concerned about build breakages as we've never tested this before.
holy crap dev/lint-python makes my eyes bleed!

currently installed packages + versions:

-bash-4.1$ which pycodestyle && pycodestyle --version
/home/anaconda/bin/pycodestyle
2.3.1
-bash-4.1$ which flake8 && flake8 --version
/home/anaconda/bin/flake8
3.5.0 (mccabe: 0.6.1, pycodestyle: 2.3.1, pyflakes: 1.6.0) CPython 2.7.13 on Linux
-bash-4.1$ which sphinx-build && sphinx-build --version
/home/anaconda/envs/py3k/bin/sphinx-build
Sphinx (sphinx-build) 1.2.3

shaneknapp · 2018-11-08T19:20:09Z

ok, i missed that previous comment about flake8 and pycodestyle version incompatibility.

since we're running flake8 3.50, i agree that this could be causing problems w/pycodestyle running properly:

flake8 3.5.0 has requirement pycodestyle<2.4.0,>=2.0.0, but you'll have pycodestyle 2.4.0 which is incompatible.

i'll bump flake8 to 3.6 on all of the workers, and then bump pycodestyle to 2.4.0

shaneknapp · 2018-11-08T19:48:48Z

interesting. from the flake8 webpage:

It is very important to install Flake8 on the correct version of Python for your needs. If you 
want Flake8 to properly parse new language features in Python 3.5 (for example), you need it to 
be installed on 3.5 for Flake8 to understand those features. In many ways, Flake8 is tied to the
version of Python on which it runs.

we're testing flake8 w/python 3.4.5 on the centos workers, and soon to be python 3.5 everywhere. this means that flake8 probably hasn't been behaving properly since the get-go.

also, this PR sets the flake8 version to 3.6, which means we should be testing against python3.6... i was only planning on bumping the python version from 3.4 -> 3.5, however.

what's also confusing is that there are version of flake8 3.6.0 for python2.7, 3.6 and 3.7.

i think we have some serious, and heretofore unknown, version incompatibilities in our python testing environment.

what i'm going to do/test:

pin flake8 to version 3.6.0, and the python2.7 testing environment.
pin pycodestyle to 2.4.0, again in the python2.7 testing environment.
leave sphinx-build in the python 3.{4,5} environment (as it always has been)
install pydocstyle in a test env and see what breaks

(edited due to getting confused w/so many version numbers)

srowen · 2018-11-08T19:52:31Z

I don't have the context here to have a strong opinion, but, it seems like we should make the test env setup self-contained if possible, to avoid dependencies on and maintenance of the build env. After all others need to run these tests too. To that end, downloading and installing particular versions seems reasonable (if they're not already installed at the right version?) Is that hard?

Whatever makes this more robust and needs less maintenance from you sounds good. I think you're welcome to clean up the script as you like too.

shaneknapp · 2018-11-08T20:33:37Z

pydocstyle tests passed w/o issue btw:
https://amplab.cs.berkeley.edu/jenkins/job/ubuntuSparkPRB/134/consoleFull

this is on ubuntu w/python 3.5, flake8 3.6.0, pydocstyle 3.0.0 and pycodestyle 2.4.0.

i also updated all of the centos jenkins workers to have flake8 3.6.0 and pycodestyle 2.4.0. recent PRB builds have passed w/o issue.

shaneknapp · 2018-11-08T20:34:57Z

I don't have the context here to have a strong opinion, but, it seems like we should make the test env setup self-contained if possible, to avoid dependencies on and maintenance of the build env. After all others need to run these tests too. To that end, downloading and installing particular versions seems reasonable (if they're not already installed at the right version?) Is that hard?

it should be reasonable to download a binary as we do for pycodestyle. i'll look around and see what i can find.

Whatever makes this more robust and needs less maintenance from you sounds good. I think you're welcome to clean up the script as you like too.

i will absolutely be taking a long, hard look at lint-python...

shaneknapp · 2018-11-09T19:21:57Z

orthogonal to this PR, but just as an FYI:

#22994

rekhajoshm · 2018-11-09T19:40:53Z

pydocstyle tests passed w/o issue btw:
https://amplab.cs.berkeley.edu/jenkins/job/ubuntuSparkPRB/134/consoleFull

this is on ubuntu w/python 3.5, flake8 3.6.0, pydocstyle 3.0.0 and pycodestyle 2.4.0.

@shaneknapp - That is expected, as for now, dev/tox.ini has all checks in ignore section for pydocstyle. thanks.

HyukjinKwon · 2018-11-09T22:01:14Z

OMG, I don't know why I missed these comments. I will read it tomorrow (now it's 6 am and I could get sleep .. )

HyukjinKwon · 2018-11-10T09:33:01Z

I also agree with @srowen's (#22963 (comment)) . Will try to have some time to take a look for a help soon.

shaneknapp · 2018-11-10T18:13:32Z

re #22963 (comment)

i checked, and the only one we can seemingly download independently is pycodestyle.

…tyle and flake8 in 'lint-python' script ## What changes were proposed in this pull request? This PR explicitly specifies `flake8` and `pydocstyle` versions. - It checks flake8 binary executable - flake8 version check >= 3.5.0 - pydocstyle >= 3.0.0 (previously it was == 3.0.0) ## How was this patch tested? Manually tested. Closes apache#22963 from HyukjinKwon/SPARK-25962. Authored-by: hyukjinkwon <[email protected]> Signed-off-by: hyukjinkwon <[email protected]>

Specify minimum versions for both pydocstyle and flake8 in 'lint-pyth…

e597243

…on' script

HyukjinKwon changed the title ~~[SPARK-25962][BUILD][PYTHON]Specify minimum versions for both pydocstyle and flake8 in 'lint-python' script~~ [SPARK-25962][BUILD][PYTHON] Specify minimum versions for both pydocstyle and flake8 in 'lint-python' script Nov 7, 2018

HyukjinKwon commented Nov 7, 2018

View reviewed changes

lower to 3.4.0 for Jenkins

b7f0e2f

HyukjinKwon commented Nov 7, 2018

View reviewed changes

3.0.0

a7192a3

HyukjinKwon commented Nov 7, 2018

View reviewed changes

print out flake8 version

bef4190

Check directly from binary executbles

802a9d4

srowen approved these changes Nov 7, 2018

View reviewed changes

asfgit closed this in a8e1c98 Nov 8, 2018

HyukjinKwon deleted the SPARK-25962 branch March 3, 2020 01:20

[SPARK-25962][BUILD][PYTHON] Specify minimum versions for both pydocstyle and flake8 in 'lint-python' script #22963

[SPARK-25962][BUILD][PYTHON] Specify minimum versions for both pydocstyle and flake8 in 'lint-python' script #22963

Uh oh!

Conversation

HyukjinKwon commented Nov 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

HyukjinKwon Nov 7, 2018

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 7, 2018

Uh oh!

HyukjinKwon Nov 7, 2018

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Nov 7, 2018

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 7, 2018

Uh oh!

HyukjinKwon Nov 7, 2018

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 7, 2018

Uh oh!

SparkQA commented Nov 7, 2018

Uh oh!

HyukjinKwon commented Nov 7, 2018

Uh oh!

SparkQA commented Nov 7, 2018

Uh oh!

HyukjinKwon commented Nov 8, 2018

Uh oh!

HyukjinKwon commented Nov 8, 2018

Uh oh!

shaneknapp commented Nov 8, 2018

Uh oh!

shaneknapp commented Nov 8, 2018

Uh oh!

shaneknapp commented Nov 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srowen commented Nov 8, 2018

Uh oh!

shaneknapp commented Nov 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shaneknapp commented Nov 8, 2018

Uh oh!

shaneknapp commented Nov 9, 2018

Uh oh!

rekhajoshm commented Nov 9, 2018

Uh oh!

HyukjinKwon commented Nov 9, 2018

Uh oh!

HyukjinKwon commented Nov 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shaneknapp commented Nov 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

HyukjinKwon commented Nov 7, 2018 •

edited

Loading

shaneknapp commented Nov 8, 2018 •

edited

Loading

shaneknapp commented Nov 8, 2018 •

edited

Loading

HyukjinKwon commented Nov 10, 2018 •

edited

Loading