[SPARK-37730][PYTHON] Replace use of MPLPlot._add_legend_handle with MPLPlot._append_legend_handles_labels #35000

mslapek · 2021-12-23T12:24:14Z

What changes were proposed in this pull request?

Replace use of MPLPlot._add_legend_handle (removed in pandas) with MPLPlot._append_legend_handles_labels in histogram and KDE plots.
Based on: pandas-dev/pandas@029907c

Why are the changes needed?

Fix of SPARK-37730. plot.hist and plot.kde don't throw AttributeError for pandas=1.3.5.

Does this PR introduce any user-facing change?

No

How was this patch tested?

~~Tested with existing plot test on CI (for older pandas only).~~ (it seems that CI doesn't run matplotlib tests, see #35000 (comment))

I've run tests on a local computer, see #35000 (comment) :

$ python python/pyspark/pandas/tests/plot/test_series_plot_matplotlib.py

❓ QUESTION: Maybe add plot testing for pandas 1.3.5 on CI? (I've noticed that CI uses pandas=1.3.4, maybe update it to 1.3.5?)

HyukjinKwon · 2021-12-23T12:45:37Z

ok to test

HyukjinKwon · 2021-12-23T12:46:47Z

Thanks for taking a look at this, @mslapek. Mind enabling GitHub Actions in your forked repository? (see also https://github.com/apache/spark/pull/35000/checks?check_run_id=4617900123). Apache Spark leverages the GitHub Actions resources from author's forked repository.

HyukjinKwon · 2021-12-23T12:47:20Z

cc @itholic would you mind taking a look please?

SparkQA · 2021-12-23T13:21:47Z

Test build #146523 has finished for PR 35000 at commit 2a33641.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class PandasOnSparkHistPlot(PandasHistPlot, HistogramPlotBase, _PandasOnSparkKdeHistCommon):
class PandasOnSparkKdePlot(PandasKdePlot, KdePlotBase, _PandasOnSparkKdeHistCommon):

mslapek · 2021-12-23T13:38:33Z

Thanks for taking a look at this, @mslapek. Mind enabling GitHub Actions in your forked repository? (see also https://github.com/apache/spark/pull/35000/checks?check_run_id=4617900123). Apache Spark leverages the GitHub Actions resources from author's forked repository.

I've enabled workflow only for "Build and test". 🤦‍♂️ Now all workflows should be enabled.

SparkQA · 2021-12-23T13:52:57Z

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

SparkQA · 2021-12-23T14:51:16Z

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

mslapek · 2021-12-24T08:02:02Z

One hour ago I got result from GitHub action "Report test results": Error: no artifacts found.

@HyukjinKwon Maybe all actions must be manually requested to rerun? 😕

https://github.com/mslapek/spark/actions/runs/1618397788

HyukjinKwon · 2021-12-24T08:31:46Z

You can rebase to the latest master branch, or push an empty commit e.g.) git commit --allow-empty

…_handles_labels

…els copied from pandas

SparkQA · 2021-12-24T11:26:04Z

Test build #146564 has finished for PR 35000 at commit 0f223f7.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class PandasOnSparkHistPlot(PandasHistPlot, HistogramPlotBase, _PandasOnSparkKdeHistCommon):
class PandasOnSparkKdePlot(PandasKdePlot, KdePlotBase, _PandasOnSparkKdeHistCommon):

SparkQA · 2021-12-24T11:41:39Z

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

SparkQA · 2021-12-24T12:45:56Z

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

AmplabJenkins · 2021-12-25T16:00:14Z

Can one of the admins verify this patch?

python/pyspark/pandas/plot/matplotlib.py

itholic · 2021-12-27T00:43:29Z

❓ QUESTION: Maybe add plot testing for pandas 1.3.5 on CI? (I've noticed that CI uses pandas=1.3.4, maybe update it to 1.3.5?)

Yeah, I think we can update the pandas version to 1.3.5 in CI, and add related tests.

And could you briefly show what you fixed with Before & After example if possible ??

HyukjinKwon · 2021-12-27T00:45:37Z

we already use latest pandas always. I suspect it's not uploaded to pypi or github actions cannot reach yet if it uses 1.3.5. matplotlib test case is disabled IIRC in CI so that's probably why the tests pass: https://app.codecov.io/gh/apache/spark/blob/master/python/pyspark/pandas/tests/plot/test_series_plot_matplotlib.py

itholic · 2021-12-27T00:53:05Z

+1 for #35000 (comment) and #35000 (comment).

Otherwise, LGTM 👍

HyukjinKwon

LGTM2 except two comments

…dles_labels copied from pandas" This reverts commit 0f223f7.

mslapek · 2021-12-27T10:22:29Z

And could you briefly show what you fixed with Before & After example if possible ??

A brief example is given in the issue: https://issues.apache.org/jira/browse/SPARK-37730

I've run locally matplotlib tests:

$ python python/pyspark/pandas/tests/plot/test_series_plot_matplotlib.py

After fix (my latest commit `a1ca73d`):

✔️ New pandas

$ python -c "import pandas as pd; print(pd.__version__)"
1.3.5

$ python python/pyspark/pandas/tests/plot/test_series_plot_matplotlib.py
...
Ran 14 tests in 35.059s

OK

✔️ Old pandas

$ python -c "import pandas as pd; print(pd.__version__)"
1.2.5

$ python python/pyspark/pandas/tests/plot/test_series_plot_matplotlib.py
...
Ran 14 tests in 36.258s

OK

Main branch before fix (commit `c0cedb1`):

❌ New pandas

$ python -c "import pandas as pd; print(pd.__version__)"
1.3.5

$ python python/pyspark/pandas/tests/plot/test_series_plot_matplotlib.py
...
ERROR: test_single_value_hist (pyspark.pandas.tests.plot.test_series_plot_matplotlib.SeriesPlotMatplotlibTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/develop/repo/spark/python/pyspark/pandas/tests/plot/test_series_plot_matplotlib.py", line 391, in test_single_value_hist
    ax2 = psdf["single"].plot.hist()
...
  File "/home/develop/repo/spark/python/pyspark/pandas/plot/matplotlib.py", line 386, in _make_plot
    self._add_legend_handle(artists[0], label, index=i)
AttributeError: 'PandasOnSparkHistPlot' object has no attribute '_add_legend_handle'

...
======================================================================
FAIL: test_pie_plot (pyspark.pandas.tests.plot.test_series_plot_matplotlib.SeriesPlotMatplotlibTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/develop/repo/spark/python/pyspark/pandas/tests/plot/test_series_plot_matplotlib.py", line 137, in test_pie_plot
    self.assertEqual(bin1, bin2)
AssertionError: b'iVB[159 chars]dpAABSIUlEQVR4nO3dd3gc1d098DPbtLvqvdlqttwLbhhs[28020 chars]gg==' != b'iVB[159 chars]dpAABRr0lEQVR4nO3dd3xedf3//+e5VvZo2szOdO/SAYWC[27868 chars]gg=='

----------------------------------------------------------------------
Ran 14 tests in 33.338s

FAILED (failures=2, errors=4)

✔️ Old pandas

$ python -c "import pandas as pd; print(pd.__version__)"
1.2.5

$ python python/pyspark/pandas/tests/plot/test_series_plot_matplotlib.py
...
Ran 14 tests in 36.266s

OK

HyukjinKwon · 2021-12-27T10:23:27Z

Thanks for testing it thoroughly. LGTM

mslapek · 2021-12-27T13:46:43Z

It seems that CI wants me to reformat file, which I haven't changed: python/pyspark/shuffle.py

I've merged main branch.

zero323 · 2021-12-28T00:18:38Z

@mslapek

It seems that CI wants me to reformat file, which I haven't changed: python/pyspark/shuffle.py

I've merged main branch.

We've recently updated (ac13494) Black to 21.12b0 and there is change of behavior compared to older releases.

HyukjinKwon · 2021-12-28T01:37:23Z

I manually ran the related tests locally, and verified that it passed.

Merged to master.

HyukjinKwon · 2021-12-28T01:37:43Z

@itholic would you mind investigating to enable matplotlib related tests?

mslapek · 2021-12-28T07:20:06Z

@HyukjinKwon @itholic Thanks for review and help! 😃

dongjoon-hyun · 2022-07-10T22:24:28Z

Thank you, @mslapek, @HyukjinKwon , @itholic .
I tested this manually on branch-3.2 and backported for Apache Spark 3.2.2 RC1.

Starting test(python3): pyspark.pandas.tests.plot.test_series_plot_matplotlib
Finished test(python3): pyspark.pandas.tests.plot.test_series_plot_matplotlib (75s)

…MPLPlot._append_legend_handles_labels ### What changes were proposed in this pull request? Replace use of MPLPlot._add_legend_handle (removed in pandas) with MPLPlot._append_legend_handles_labels in histogram and KDE plots. Based on: pandas-dev/pandas@029907c ### Why are the changes needed? Fix of SPARK-37730. plot.hist and plot.kde don't throw AttributeError for pandas=1.3.5. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? ~~Tested with existing plot test on CI (for older pandas only).~~ (it seems that CI doesn't run matplotlib tests, see #35000 (comment)) I've run tests on a local computer, see #35000 (comment) : ``` $ python python/pyspark/pandas/tests/plot/test_series_plot_matplotlib.py ``` :question: **QUESTION:** Maybe add plot testing for pandas 1.3.5 on CI? (I've noticed that CI uses `pandas=1.3.4`, maybe update it to `1.3.5`?) Closes #35000 from mslapek/fixpythonplot. Authored-by: Michał Słapek <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]> (cherry picked from commit 371e307) Signed-off-by: Dongjoon Hyun <[email protected]>

HyukjinKwon · 2022-07-11T00:23:59Z

Thanks!!!!!!

…MPLPlot._append_legend_handles_labels ### What changes were proposed in this pull request? Replace use of MPLPlot._add_legend_handle (removed in pandas) with MPLPlot._append_legend_handles_labels in histogram and KDE plots. Based on: pandas-dev/pandas@029907c ### Why are the changes needed? Fix of SPARK-37730. plot.hist and plot.kde don't throw AttributeError for pandas=1.3.5. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? ~~Tested with existing plot test on CI (for older pandas only).~~ (it seems that CI doesn't run matplotlib tests, see apache#35000 (comment)) I've run tests on a local computer, see apache#35000 (comment) : ``` $ python python/pyspark/pandas/tests/plot/test_series_plot_matplotlib.py ``` :question: **QUESTION:** Maybe add plot testing for pandas 1.3.5 on CI? (I've noticed that CI uses `pandas=1.3.4`, maybe update it to `1.3.5`?) Closes apache#35000 from mslapek/fixpythonplot. Authored-by: Michał Słapek <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]> (cherry picked from commit 371e307) Signed-off-by: Dongjoon Hyun <[email protected]>

github-actions bot added CORE PYTHON labels Dec 23, 2021

mslapek changed the title ~~[WIP][SPARK-37730][PYTHON]~~ [WIP][SPARK-37730][PYTHON] Replace use of MPLPlot._add_legend_handle with MPLPlot._add_legend_handle Dec 23, 2021

mslapek changed the title ~~[WIP][SPARK-37730][PYTHON] Replace use of MPLPlot._add_legend_handle with MPLPlot._add_legend_handle~~ [SPARK-37730][PYTHON] Replace use of MPLPlot._add_legend_handle with MPLPlot._append_legend_handles_labels Dec 23, 2021

mslapek added 2 commits December 24, 2021 11:34

Replace use of MPLPlot._add_legend_handle with MPLPlot._append_legend…

4cf2653

…_handles_labels

Add class _PandasOnSparkKdeHistCommon with _append_legend_handles_lab…

0f223f7

…els copied from pandas

mslapek force-pushed the fixpythonplot branch from 2a33641 to 0f223f7 Compare December 24, 2021 10:37

HyukjinKwon reviewed Dec 27, 2021

View reviewed changes

python/pyspark/pandas/plot/matplotlib.py Outdated Show resolved Hide resolved

HyukjinKwon reviewed Dec 27, 2021

View reviewed changes

python/pyspark/pandas/plot/matplotlib.py Outdated Show resolved Hide resolved

HyukjinKwon approved these changes Dec 27, 2021

View reviewed changes

mslapek added 2 commits December 27, 2021 09:54

Revert "Add class _PandasOnSparkKdeHistCommon with _append_legend_han…

dac3569

…dles_labels copied from pandas" This reverts commit 0f223f7.

CR fix

a1ca73d

Merge branch 'apache:master' into fixpythonplot

51d6b7e

HyukjinKwon closed this in 371e307 Dec 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-37730][PYTHON] Replace use of MPLPlot._add_legend_handle with MPLPlot._append_legend_handles_labels #35000

[SPARK-37730][PYTHON] Replace use of MPLPlot._add_legend_handle with MPLPlot._append_legend_handles_labels #35000

mslapek commented Dec 23, 2021 •

edited

Loading

HyukjinKwon commented Dec 23, 2021

HyukjinKwon commented Dec 23, 2021

HyukjinKwon commented Dec 23, 2021

SparkQA commented Dec 23, 2021

mslapek commented Dec 23, 2021

SparkQA commented Dec 23, 2021

SparkQA commented Dec 23, 2021

mslapek commented Dec 24, 2021

HyukjinKwon commented Dec 24, 2021

SparkQA commented Dec 24, 2021

SparkQA commented Dec 24, 2021

SparkQA commented Dec 24, 2021

AmplabJenkins commented Dec 25, 2021

itholic commented Dec 27, 2021

HyukjinKwon commented Dec 27, 2021

itholic commented Dec 27, 2021 •

edited

Loading

HyukjinKwon left a comment

mslapek commented Dec 27, 2021

HyukjinKwon commented Dec 27, 2021

mslapek commented Dec 27, 2021 •

edited

Loading

zero323 commented Dec 28, 2021 •

edited

Loading

HyukjinKwon commented Dec 28, 2021

HyukjinKwon commented Dec 28, 2021

mslapek commented Dec 28, 2021

dongjoon-hyun commented Jul 10, 2022

HyukjinKwon commented Jul 11, 2022

[SPARK-37730][PYTHON] Replace use of MPLPlot._add_legend_handle with MPLPlot._append_legend_handles_labels #35000

[SPARK-37730][PYTHON] Replace use of MPLPlot._add_legend_handle with MPLPlot._append_legend_handles_labels #35000

Conversation

mslapek commented Dec 23, 2021 • 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?

HyukjinKwon commented Dec 23, 2021

HyukjinKwon commented Dec 23, 2021

HyukjinKwon commented Dec 23, 2021

SparkQA commented Dec 23, 2021

mslapek commented Dec 23, 2021

SparkQA commented Dec 23, 2021

SparkQA commented Dec 23, 2021

mslapek commented Dec 24, 2021

HyukjinKwon commented Dec 24, 2021

SparkQA commented Dec 24, 2021

SparkQA commented Dec 24, 2021

SparkQA commented Dec 24, 2021

AmplabJenkins commented Dec 25, 2021

itholic commented Dec 27, 2021

HyukjinKwon commented Dec 27, 2021

itholic commented Dec 27, 2021 • edited Loading

HyukjinKwon left a comment

Choose a reason for hiding this comment

mslapek commented Dec 27, 2021

After fix (my latest commit a1ca73d):

✔️ New pandas

✔️ Old pandas

Main branch before fix (commit c0cedb1):

❌ New pandas

✔️ Old pandas

HyukjinKwon commented Dec 27, 2021

mslapek commented Dec 27, 2021 • edited Loading

zero323 commented Dec 28, 2021 • edited Loading

HyukjinKwon commented Dec 28, 2021

HyukjinKwon commented Dec 28, 2021

mslapek commented Dec 28, 2021

dongjoon-hyun commented Jul 10, 2022

HyukjinKwon commented Jul 11, 2022

mslapek commented Dec 23, 2021 •

edited

Loading

itholic commented Dec 27, 2021 •

edited

Loading

After fix (my latest commit `a1ca73d`):

Main branch before fix (commit `c0cedb1`):

mslapek commented Dec 27, 2021 •

edited

Loading

zero323 commented Dec 28, 2021 •

edited

Loading