[SPARK-32094][PYTHON] Update cloudpickle to v1.5.0 #29114

HyukjinKwon · 2020-07-15T01:16:23Z

What changes were proposed in this pull request?

This PR aims to upgrade PySpark's embedded cloudpickle to the latest cloudpickle v1.5.0 (See https://github.com/cloudpipe/cloudpickle/blob/v1.5.0/cloudpickle/cloudpickle.py)

Why are the changes needed?

There are many bug fixes. For example, the bug described in the JIRA:

dill unpickling fails because they define types.ClassType, which is undefined in dill. This results in the following error:

Traceback (most recent call last):
  File "/usr/local/lib/python3.6/site-packages/apache_beam/internal/pickler.py", line 279, in loads
    return dill.loads(s)
  File "/usr/local/lib/python3.6/site-packages/dill/_dill.py", line 317, in loads
    return load(file, ignore)
  File "/usr/local/lib/python3.6/site-packages/dill/_dill.py", line 305, in load
    obj = pik.load()
  File "/usr/local/lib/python3.6/site-packages/dill/_dill.py", line 577, in _load_type
    return _reverse_typemap[name]
KeyError: 'ClassType'

See also cloudpipe/cloudpickle#82. This was fixed for cloudpickle 1.3.0+ (cloudpipe/cloudpickle#337), but PySpark's cloudpickle.py doesn't have this change yet.

More notably, now it supports C pickle implementation with Python 3.8 which hugely improve performance. This is already adopted in another project such as Ray.

Does this PR introduce any user-facing change?

Yes, as described above, the bug fixes. Internally, users also could leverage the fast cloudpickle backed by C pickle.

How was this patch tested?

Jenkins will test it out.

HyukjinKwon · 2020-07-15T01:20:17Z

python/pyspark/cloudpickle.py

@@ -1,1362 +0,0 @@
-"""


There are too many diffs so seems github doesn't know that it was renamed.

HyukjinKwon · 2020-07-15T01:20:50Z

python/pyspark/cloudpickle/__init__.py

+from __future__ import absolute_import
+
+
+from pyspark.cloudpickle.cloudpickle import *  # noqa


Everything is same as the original cloudpickle except these two line diff. We should add pyspark to import.

HyukjinKwon · 2020-07-15T01:23:25Z

This PR will be co-authored together with @codesue. Thanks for the initial efforts you made at #28950

HyukjinKwon · 2020-07-15T01:45:31Z

We should test with Python 3.8 in the CI too due to Python 3.8 specific optimization. I would like to block this PR by #29116

…tions ### What changes were proposed in this pull request? This PR aims to test PySpark with Python 3.8 in Github Actions. In the script side, it is already ready: https://github.com/apache/spark/blob/4ad9bfd53b84a6d2497668c73af6899bae14c187/python/run-tests.py#L161 This PR includes small related fixes together: 1. Install Python 3.8 2. Only install one Python implementation instead of installing many for SQL and Yarn test cases because they need one Python executable in their test cases that is higher than Python 2. 3. Do not install Python 2 which is not needed anymore after we dropped Python 2 at SPARK-32138 4. Remove a comment about installing PyPy3 on Jenkins - SPARK-32278. It is already installed. ### Why are the changes needed? Currently, only PyPy3 and Python 3.6 are being tested with PySpark in Github Actions. We should test the latest version of Python as well because some optimizations can be only enabled with Python 3.8+. See also #29114 ### Does this PR introduce _any_ user-facing change? No, dev-only. ### How was this patch tested? Was not tested. Github Actions build in this PR will test it out. Closes #29116 from HyukjinKwon/test-python3.8-togehter. Authored-by: HyukjinKwon <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

SparkQA · 2020-07-15T07:05:02Z

Test build #125878 has finished for PR 29114 at commit 465fd8a.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2020-07-15T07:55:07Z

retest this please

HyukjinKwon · 2020-07-15T15:56:05Z

retest this please

SparkQA · 2020-07-15T21:14:47Z

Test build #125883 has finished for PR 29114 at commit 465fd8a.

This patch fails Spark unit tests.
This patch does not merge cleanly.
This patch adds no public classes.

HyukjinKwon · 2020-07-16T04:12:23Z

retest this please

viirya

Will cloudpickle_fast.py be used as automatically?

HyukjinKwon · 2020-07-16T06:14:47Z

Yeah, it does with Python 3.8. It was clear before because it was conditionally chosen in __init__.py but it was changed in the latest version at cloudpipe/cloudpickle@938553f#diff-17e83ea5b8e0670ba384e6bc36815316L9

viirya · 2020-07-16T06:23:22Z

Oh I see. So you must import from cloudpickle_fast now.

SparkQA · 2020-07-16T07:05:02Z

Test build #125938 has finished for PR 29114 at commit 77055c6.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-07-16T10:30:25Z

Test build #125945 has finished for PR 29114 at commit 74ba675.

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

HyukjinKwon · 2020-07-16T11:57:08Z

I just did a very quick test. Seems like if your function to serialize is big, it can benefit best from it up to 1250%:

>>> from pyspark import cloudpickle
>>> import timeit
>>> x = list(range(10000000))
>>> def bigfunc(a):
...     b = x
...     return a
...
>>> timeit.timeit(lambda: spark.range(1).rdd.map(bigfunc).count(), number=1)

Before:

16.589810816000004 (sec)

After:

1.3231475199999991 (sec)

HyukjinKwon · 2020-07-16T11:57:29Z

@BryanCutler, @viirya, @ueshin can you take a look when you're available?

viirya · 2020-07-16T19:42:19Z

LICENSE

@@ -229,7 +229,7 @@ BSD 3-Clause
 ------------

 python/lib/py4j-*-src.zip
-python/pyspark/cloudpickle.py
+python/pyspark/cloudpickle/*.py


nit: is it only for cloudpickle.py and cloudpickle_fast.py?

It catches the directory python/pyspark/cloudpickle/ :-).

Doesn't this use for license declaration?

I think it is not big deal to have BSD 3 for other two files. :)

I think we ported __init__.py and compat.py here too and it includes all.

Looks like their license wasn't changed from the very first release (https://github.com/cloudpipe/cloudpickle/blob/master/LICENSE). I guess it might be fine..

viirya · 2020-07-16T19:54:10Z

python/pyspark/cloudpickle/cloudpickle.py

+import typing
+import warnings
+
+from .compat import pickle


This looks different to original change too?

I believe it's same: https://github.com/cloudpipe/cloudpickle/blob/v1.5.0/cloudpickle/cloudpickle.py#L57

Oh I missed it. :)

BryanCutler

LGTM, sounds like a good improvement. It's come up before, but what do others think about moving this to the pyspark/lib folder? Now that we have been using the releases as-is for a while and there are multiple files, it might be better fit there. Could be done as a followup to this too.

HyukjinKwon · 2020-07-17T02:46:20Z

@BryanCutler, yes, I do think we should do it. I filed a JIRA - SPARK-32340

HyukjinKwon · 2020-07-17T02:49:03Z

Merged to master. Thank you so much guys <3!

HyukjinKwon · 2020-07-17T02:53:24Z

Ah .. my bad. I added an empty commit to credit together to @codesue as a co-author .. but it was gone during rebasing ..

…tions This PR aims to test PySpark with Python 3.8 in Github Actions. In the script side, it is already ready: https://github.com/apache/spark/blob/4ad9bfd53b84a6d2497668c73af6899bae14c187/python/run-tests.py#L161 This PR includes small related fixes together: 1. Install Python 3.8 2. Only install one Python implementation instead of installing many for SQL and Yarn test cases because they need one Python executable in their test cases that is higher than Python 2. 3. Do not install Python 2 which is not needed anymore after we dropped Python 2 at SPARK-32138 4. Remove a comment about installing PyPy3 on Jenkins - SPARK-32278. It is already installed. Currently, only PyPy3 and Python 3.6 are being tested with PySpark in Github Actions. We should test the latest version of Python as well because some optimizations can be only enabled with Python 3.8+. See also apache#29114 No, dev-only. Was not tested. Github Actions build in this PR will test it out. Closes apache#29116 from HyukjinKwon/test-python3.8-togehter. Authored-by: HyukjinKwon <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

probot-autolabeler bot added BUILD CORE PYTHON labels Jul 15, 2020

HyukjinKwon mentioned this pull request Jul 15, 2020

[SPARK-32094][PYTHON] Update cloudpickle to v1.4.1 #28950

Closed

HyukjinKwon commented Jul 15, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

HyukjinKwon mentioned this pull request Jul 15, 2020

[SPARK-32316][TESTS][INFRA] Test PySpark with Python 3.8 in Github Actions #29116

Closed

This comment has been minimized.

Sign in to view

HyukjinKwon force-pushed the SPARK-32094 branch from bdf31a8 to 465fd8a Compare July 15, 2020 04:07

This comment has been minimized.

Sign in to view

HyukjinKwon force-pushed the SPARK-32094 branch from 465fd8a to 77055c6 Compare July 16, 2020 01:49

HyukjinKwon mentioned this pull request Jul 16, 2020

[SPARK-32036] Replace references to blacklist/whitelist language with more appropriate terminology, excluding the blacklisting feature. #28874

Closed

Update cloudpickle to v1.5.0

74ba675

HyukjinKwon force-pushed the SPARK-32094 branch from 77055c6 to 74ba675 Compare July 16, 2020 03:41

viirya reviewed Jul 16, 2020

View reviewed changes

BryanCutler reviewed Jul 16, 2020

View reviewed changes

HyukjinKwon closed this in ea9e8f3 Jul 17, 2020

HyukjinKwon deleted the SPARK-32094 branch July 27, 2020 07:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-32094][PYTHON] Update cloudpickle to v1.5.0 #29114

[SPARK-32094][PYTHON] Update cloudpickle to v1.5.0 #29114

HyukjinKwon commented Jul 15, 2020

HyukjinKwon Jul 15, 2020

HyukjinKwon Jul 15, 2020

HyukjinKwon commented Jul 15, 2020

This comment has been minimized.

HyukjinKwon commented Jul 15, 2020

This comment has been minimized.

This comment has been minimized.

SparkQA commented Jul 15, 2020

HyukjinKwon commented Jul 15, 2020

HyukjinKwon commented Jul 15, 2020

SparkQA commented Jul 15, 2020

HyukjinKwon commented Jul 16, 2020

viirya left a comment

HyukjinKwon commented Jul 16, 2020

viirya commented Jul 16, 2020

SparkQA commented Jul 16, 2020

SparkQA commented Jul 16, 2020

HyukjinKwon commented Jul 16, 2020

HyukjinKwon commented Jul 16, 2020

viirya Jul 16, 2020

HyukjinKwon Jul 17, 2020 •

edited

Loading

viirya Jul 17, 2020

HyukjinKwon Jul 17, 2020

viirya Jul 17, 2020

HyukjinKwon Jul 17, 2020

viirya Jul 16, 2020

HyukjinKwon Jul 17, 2020

viirya Jul 17, 2020

BryanCutler left a comment

HyukjinKwon commented Jul 17, 2020

HyukjinKwon commented Jul 17, 2020 •

edited

Loading

HyukjinKwon commented Jul 17, 2020

		from __future__ import absolute_import


		from pyspark.cloudpickle.cloudpickle import * # noqa

[SPARK-32094][PYTHON] Update cloudpickle to v1.5.0 #29114

[SPARK-32094][PYTHON] Update cloudpickle to v1.5.0 #29114

Conversation

HyukjinKwon commented Jul 15, 2020

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

HyukjinKwon commented Jul 15, 2020

This comment has been minimized.

HyukjinKwon commented Jul 15, 2020

This comment has been minimized.

This comment has been minimized.

SparkQA commented Jul 15, 2020

HyukjinKwon commented Jul 15, 2020

HyukjinKwon commented Jul 15, 2020

SparkQA commented Jul 15, 2020

HyukjinKwon commented Jul 16, 2020

viirya left a comment

Choose a reason for hiding this comment

HyukjinKwon commented Jul 16, 2020

viirya commented Jul 16, 2020

SparkQA commented Jul 16, 2020

SparkQA commented Jul 16, 2020

HyukjinKwon commented Jul 16, 2020

HyukjinKwon commented Jul 16, 2020

Choose a reason for hiding this comment

HyukjinKwon Jul 17, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BryanCutler left a comment

Choose a reason for hiding this comment

HyukjinKwon commented Jul 17, 2020

HyukjinKwon commented Jul 17, 2020 • edited Loading

HyukjinKwon commented Jul 17, 2020

HyukjinKwon Jul 17, 2020 •

edited

Loading

HyukjinKwon commented Jul 17, 2020 •

edited

Loading