[SPARK-39881][PYTHON] Fix erroneous check for black and reenable black validation. #37305

grundprinzip · 2022-07-26T20:11:31Z

What changes were proposed in this pull request?

The previously committed check for running black did not actually work and caused code to be committed that does not follow the linter rules. This patch fixes the way we check if black is locally installed and update the dev/reformat-python script. In addition, we run the script to fix existing style issues. Similar to the original PR #32779 this patch only applies the black checks on the pandas code.

The black version is updated in this PR because on an empty virtualenv the selected version of click ends up in a conflict due to a underspecified version of click. See psf/black#2964.

Why are the changes needed?

We have linter rules, we should actually address them.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Manual testing.

HyukjinKwon · 2022-07-27T05:02:23Z

python/pyspark/ml/feature.py

@@ -968,7 +968,7 @@ class _CountVectorizerParams(JavaParams, HasInputCol, HasOutputCol):

    def __init__(self, *args: Any):
        super(_CountVectorizerParams, self).__init__(*args)
-        self._setDefault(minTF=1.0, minDF=1.0, maxDF=2 ** 63 - 1, vocabSize=1 << 18, binary=False)
+        self._setDefault(minTF=1.0, minDF=1.0, maxDF=2**63 - 1, vocabSize=1 << 18, binary=False)


FWIW, I think this is false positive from Black presumably (per https://peps.python.org/pep-0008/#other-recommendations). I believe we use one space around these operators.

Doesn't this fall into this rule?

If operators with different priorities are used, consider adding whitespace around the operators with the lowest priority(ies). Use your own judgment; however, never use more than one space, and always have the same amount of whitespace on both sides of a binary operator:

2**63 - 1

The - has a lower priority compared to the power operator?

Is this style change because we are upgrading black?

Apparently, yes. I just tested. In the old version it would not touch this change, the new version does. Playing with it actually shows that the behaviour is specifc to the power operator. I'll check if I find some more details in the black releases.

Found the doc that mentions about this:

Almost all operators will be surrounded by single spaces, the only exceptions are unary
operators (+, -, and ~), and power operators when both operands are simple. For
powers, an operand is considered simple if it's only a NAME, numeric CONSTANT, or
attribute access (chained attribute access is allowed), with or without a preceding
unary operator.

And the commit.

The style change is from 22.1.0.

@ueshin Please see - psf/black#538

Looks like ** is not covered by PEP8, and the main reason of balck change is consider about readable, so I personaly think black choice is right.

grundprinzip · 2022-07-27T07:01:10Z

@itholic @ueshin it would be great if you could have a look at the PR since you authored / reviewed the original PR that introduced the bug - #32779

itholic

Otherwise looks fine to me.

itholic · 2022-07-27T09:10:20Z

dev/lint-python

-    $BLACK_BUILD 2> /dev/null
-    if [ $? -ne 0 ]; then
+    $PYTHON_EXECUTABLE -c 'import black' &> /dev/null


Could you explain why this should be changed ??

The existing code "$PYTHON_EXECUTABLE -m black" isn't enough to check for Black installation ??

The reason is very simple:

> python -m black Usage: python -m black [OPTIONS] SRC ... One of 'SRC' or 'code' is required. > echo $? 1

> python -m black /System/Library/Frameworks/Python.framework/Versions/2.7/Resources/Python.app/Contents/MacOS/Python: No module named black > echo $? 1

> python -c 'import black' > echo $? 0

> python -c 'import this_does_not_exist Traceback (most recent call last): File "<string>", line 1, in <module> ModuleNotFoundError: No module named 'this_does_not_exist' > echo $? 1

dev/reformat-python

itholic · 2022-07-27T09:15:32Z

python/pyspark/pandas/tests/data_type_ops/test_boolean_ops.py

        self.assertRaises(TypeError, lambda: datetime.date(1994, 1, 1) ** b_psser)
        self.assertRaises(TypeError, lambda: datetime.datetime(1994, 1, 1) ** b_psser)


I'm not sure why Black doesn't reformat this, too...

Shouldn't it be reformatted like:

- self.assertRaises(TypeError, lambda: datetime.date(1994, 1, 1) ** b_psser) - self.assertRaises(TypeError, lambda: datetime.datetime(1994, 1, 1) ** b_psser) + self.assertRaises(TypeError, lambda: datetime.date(1994, 1, 1)**b_psser) + self.assertRaises(TypeError, lambda: datetime.datetime(1994, 1, 1)**b_psser)

???

I'm no black expert, but from the discussion I had with @HyukjinKwon above, it looks like it's a operator precedence behavior thing.

If operators with different priorities are used, consider adding whitespace around the operators with the lowest priority(ies). Use your own judgment; however, never use more than one space, and always have the same amount of whitespace on both sides of a binary operator:

in

self.assertRaises(TypeError, lambda: datetime.date(1994, 1, 1) ** b_psser)

The binary operator ** is not paired with a lower priority other operator.

Yeah, I understand about the priority but just wonder why Black did:

- self.assertRaises(TypeError, lambda: True ** b_psser) + self.assertRaises(TypeError, lambda: True**b_psser)

but didn't

- self.assertRaises(TypeError, lambda: datetime.date(1994, 1, 1) ** b_psser) - self.assertRaises(TypeError, lambda: datetime.datetime(1994, 1, 1) ** b_psser) + self.assertRaises(TypeError, lambda: datetime.date(1994, 1, 1)**b_psser) + self.assertRaises(TypeError, lambda: datetime.datetime(1994, 1, 1)**b_psser)

It seems like just inconsistence behavior from Black, so maybe we can just keep is as is for now.

The reason is explained here #37305 (comment) - in short the operators are not simple ( True vs datetime.datetime(1994, 1, 1))

itholic · 2022-07-27T09:17:35Z

Anyway, +1 for upgrading the version to the latest.

itholic · 2022-07-27T09:49:20Z

Can we also add [PS] to the PR title ?

AmplabJenkins · 2022-07-27T13:14:01Z

Can one of the admins verify this patch?

ueshin

@itholic

Can we also add [PS] to the PR title ?

This should not be categorized as [PS] but PySpark in general.

@grundprinzip Could you use [PYTHON] instead?

dev/lint-python

grundprinzip

Reverting unnecessary change.

grundprinzip · 2022-07-28T05:26:12Z

Updated the PR description to indicate why the black version was updated.

Yikun

LGTM, some nits but it's also OK for me, so just go ahead if you think it's ok!

dev/lint-python

dev/pyproject.toml

Yikun · 2022-07-29T02:30:19Z

and CI failed due to Run / Scala 2.13 build with SBT git clone networking issue, I think we can pass it by re-triggering.

grundprinzip · 2022-07-29T11:11:20Z

I updated the remaining comments. Can we merge the PR?

HyukjinKwon · 2022-08-01T07:56:26Z

Merged to master.

fixing linter issues

d29d43a

github-actions bot added BUILD CORE ML PANDAS API ON SPARK PYTHON SQL labels Jul 26, 2022

fixing GH workflow installation

a62ac64

github-actions bot added the INFRA label Jul 26, 2022

grundprinzip changed the title ~~[WIP] [SPARK-39881] Fix erroneous check for black and renable black validation.~~ [WIP] [SPARK-39881] Fix erroneous check for black and reenable black validation. Jul 26, 2022

HyukjinKwon reviewed Jul 27, 2022

View reviewed changes

grundprinzip changed the title ~~[WIP] [SPARK-39881] Fix erroneous check for black and reenable black validation.~~ [SPARK-39881] Fix erroneous check for black and reenable black validation. Jul 27, 2022

itholic reviewed Jul 27, 2022

View reviewed changes

grundprinzip changed the title ~~[SPARK-39881] Fix erroneous check for black and reenable black validation.~~ [PS] [SPARK-39881] Fix erroneous check for black and reenable black validation. Jul 27, 2022

grundprinzip changed the title ~~[PS] [SPARK-39881] Fix erroneous check for black and reenable black validation.~~ [SPARK-39881][PS] Fix erroneous check for black and reenable black validation. Jul 27, 2022

ueshin reviewed Jul 27, 2022

View reviewed changes

dev/lint-python Outdated Show resolved Hide resolved

grundprinzip changed the title ~~[SPARK-39881][PS] Fix erroneous check for black and reenable black validation.~~ [SPARK-39881][PYTHON] Fix erroneous check for black and reenable black validation. Jul 27, 2022

grundprinzip commented Jul 27, 2022

View reviewed changes

Revert unnecessary change.

a626a13

Yikun approved these changes Jul 29, 2022

View reviewed changes

dev/lint-python Outdated Show resolved Hide resolved

dev/pyproject.toml Show resolved Hide resolved

grundprinzip added 2 commits July 29, 2022 13:04

Updating dev depenencies.

8691d39

Simplifying error messages.

5a89f6d

HyukjinKwon approved these changes Jul 29, 2022

View reviewed changes

HyukjinKwon closed this in 8c6c7ae Aug 1, 2022

dongjoon-hyun mentioned this pull request Mar 21, 2024

[SPARK-47508][PYTHON] Update black to 24.3.0 #45647

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-39881][PYTHON] Fix erroneous check for black and reenable black validation. #37305

[SPARK-39881][PYTHON] Fix erroneous check for black and reenable black validation. #37305

grundprinzip commented Jul 26, 2022 •

edited

Loading

HyukjinKwon Jul 27, 2022

grundprinzip Jul 27, 2022

ueshin Jul 27, 2022

grundprinzip Jul 27, 2022

ueshin Jul 27, 2022 •

edited

Loading

grundprinzip Jul 27, 2022

Yikun Jul 29, 2022

grundprinzip commented Jul 27, 2022

itholic left a comment

itholic Jul 27, 2022

grundprinzip Jul 27, 2022

itholic Jul 27, 2022

grundprinzip Jul 27, 2022

itholic Jul 27, 2022

grundprinzip Jul 28, 2022 •

edited

Loading

itholic commented Jul 27, 2022

itholic commented Jul 27, 2022

AmplabJenkins commented Jul 27, 2022

ueshin left a comment •

edited

Loading

grundprinzip left a comment

grundprinzip commented Jul 28, 2022

Yikun left a comment

Yikun commented Jul 29, 2022 •

edited

Loading

grundprinzip commented Jul 29, 2022

HyukjinKwon commented Aug 1, 2022

		self.assertRaises(TypeError, lambda: datetime.date(1994, 1, 1) ** b_psser)
		self.assertRaises(TypeError, lambda: datetime.datetime(1994, 1, 1) ** b_psser)

[SPARK-39881][PYTHON] Fix erroneous check for black and reenable black validation. #37305

[SPARK-39881][PYTHON] Fix erroneous check for black and reenable black validation. #37305

Conversation

grundprinzip commented Jul 26, 2022 • edited Loading

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?

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

ueshin Jul 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grundprinzip commented Jul 27, 2022

itholic left a 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

Choose a reason for hiding this comment

grundprinzip Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

itholic commented Jul 27, 2022

itholic commented Jul 27, 2022

AmplabJenkins commented Jul 27, 2022

ueshin left a comment • edited Loading

Choose a reason for hiding this comment

grundprinzip left a comment

Choose a reason for hiding this comment

grundprinzip commented Jul 28, 2022

Yikun left a comment

Choose a reason for hiding this comment

Yikun commented Jul 29, 2022 • edited Loading

grundprinzip commented Jul 29, 2022

HyukjinKwon commented Aug 1, 2022

grundprinzip commented Jul 26, 2022 •

edited

Loading

ueshin Jul 27, 2022 •

edited

Loading

grundprinzip Jul 28, 2022 •

edited

Loading

ueshin left a comment •

edited

Loading

Yikun commented Jul 29, 2022 •

edited

Loading