[SPARK-27612][PYTHON] Use Python's default protocol instead of highest protocol #24519

HyukjinKwon · 2019-05-03T03:03:02Z

What changes were proposed in this pull request?

This PR partially reverts #20691

After we changed the Python protocol to highest ones, seems like it introduced a correctness bug. This potentially affects all Python related code paths.

I suspect a bug related to Pryolite (maybe opcodes MEMOIZE, FRAME and/or our RowPickler). I would like to stick to default protocol for now and investigate the issue separately.

I will separately investigate later to bring highest protocol back.

Update:
I can almost confirm that this is a bug in Pyrolite.

Python lists are chunked via BatchedSerializer at parallelize (in createDataFrame), for instance, as below (each line is each batch):

[[1, 2, 3, 4], [1, 2, 3, 4], [1, 2, 3, 4], [1, 2, 3, 4], [1, 2, 3, 4], [1, 2, 3, 4], [1, 2, 3, 4], [1, 2, 3, 4]]
[[1, 2, 3, 4], [1, 2, 3, 4], [1, 2, 3, 4], [1, 2, 3, 4], [1, 2, 3, 4], [1, 2, 3, 4], [1, 2, 3, 4], [1, 2, 3, 4]]
...
[[1, 2, 3, 4], [1, 2, 3, 4], [1, 2, 3, 4], [1, 2, 3, 4]]

Here is opcodes for each batch (separated by ===..) with protocol 4:

Details

==============================
    0: \x80 PROTO      4
    2: \x95 FRAME      47
   11: ]    EMPTY_LIST
   12: \x94 MEMOIZE    (as 0)
   13: (    MARK
   14: ]        EMPTY_LIST
   15: \x94     MEMOIZE    (as 1)
   16: (        MARK
   17: K            BININT1    1
   19: K            BININT1    2
   21: K            BININT1    3
   23: K            BININT1    4
   25: e            APPENDS    (MARK at 16)
   26: \x85     TUPLE1
   27: \x94     MEMOIZE    (as 2)
   28: h        BINGET     1
   30: \x85     TUPLE1
   31: \x94     MEMOIZE    (as 3)
   32: h        BINGET     1
   34: \x85     TUPLE1
   35: \x94     MEMOIZE    (as 4)
   36: h        BINGET     1
   38: \x85     TUPLE1
   39: \x94     MEMOIZE    (as 5)
   40: h        BINGET     1
   42: \x85     TUPLE1
   43: \x94     MEMOIZE    (as 6)
   44: h        BINGET     1
   46: \x85     TUPLE1
   47: \x94     MEMOIZE    (as 7)
   48: h        BINGET     1
   50: \x85     TUPLE1
   51: \x94     MEMOIZE    (as 8)
   52: h        BINGET     1
   54: \x85     TUPLE1
   55: \x94     MEMOIZE    (as 9)
   56: e        APPENDS    (MARK at 13)
   57: .    STOP
highest protocol among opcodes = 4
==============================
==============================
    0: \x80 PROTO      4
    2: \x95 FRAME      47
   11: ]    EMPTY_LIST
   12: \x94 MEMOIZE    (as 0)
   13: (    MARK
   14: ]        EMPTY_LIST
   15: \x94     MEMOIZE    (as 1)
   16: (        MARK
   17: K            BININT1    1
   19: K            BININT1    2
   21: K            BININT1    3
   23: K            BININT1    4
   25: e            APPENDS    (MARK at 16)
   26: \x85     TUPLE1
   27: \x94     MEMOIZE    (as 2)
   28: h        BINGET     1
   30: \x85     TUPLE1
   31: \x94     MEMOIZE    (as 3)
   32: h        BINGET     1
   34: \x85     TUPLE1
   35: \x94     MEMOIZE    (as 4)
   36: h        BINGET     1
   38: \x85     TUPLE1
   39: \x94     MEMOIZE    (as 5)
   40: h        BINGET     1
   42: \x85     TUPLE1
   43: \x94     MEMOIZE    (as 6)
   44: h        BINGET     1
   46: \x85     TUPLE1
   47: \x94     MEMOIZE    (as 7)
   48: h        BINGET     1
   50: \x85     TUPLE1
   51: \x94     MEMOIZE    (as 8)
   52: h        BINGET     1
   54: \x85     TUPLE1
   55: \x94     MEMOIZE    (as 9)
   56: e        APPENDS    (MARK at 13)
   57: .    STOP
highest protocol among opcodes = 4
==============================
...
==============================
    0: \x80 PROTO      4
    2: \x95 FRAME      31
   11: ]    EMPTY_LIST
   12: \x94 MEMOIZE    (as 0)
   13: (    MARK
   14: ]        EMPTY_LIST
   15: \x94     MEMOIZE    (as 1)
   16: (        MARK
   17: K            BININT1    1
   19: K            BININT1    2
   21: K            BININT1    3
   23: K            BININT1    4
   25: e            APPENDS    (MARK at 16)
   26: \x85     TUPLE1
   27: \x94     MEMOIZE    (as 2)
   28: h        BINGET     1
   30: \x85     TUPLE1
   31: \x94     MEMOIZE    (as 3)
   32: h        BINGET     1
   34: \x85     TUPLE1
   35: \x94     MEMOIZE    (as 4)
   36: h        BINGET     1
   38: \x85     TUPLE1
   39: \x94     MEMOIZE    (as 5)
   40: e        APPENDS    (MARK at 13)
   41: .    STOP
highest protocol among opcodes = 4
==============================

Those batches become binary in an RDD at parallelize - so far results look fine
rdd._to_java_object_rdd -> SerDeUtil.pythonToJava -> Unpickler.loads
Here, Pryolite Unpickler.loads converts each Python object batch into Java objects

The converted Java objects look a bit odd from Pryolite as below (see the last three lists):

[1, 2, 3, 4]
[1, 2, 3, 4]
...
[[[1, 2, 3, 4]], [[1, 2, 3, 4]]]
[[[1, 2, 3, 4]], [[1, 2, 3, 4]]]
[[[1, 2, 3, 4]], [[1, 2, 3, 4]]]

Last three nested lists are ignored via EvaluatePython.makeFromJava and becomes null -> None.

So the Jenkins test was failed at [SPARK-27612][PYTHON] Use Python's default protocol instead of highest protocol #24519 (comment) as below:

Traceback (most recent call last):
  File "/home/jenkins/workspace/SparkPullRequestBuilder/python/pyspark/sql/tests/test_serde.py", line 134, in test_int_array_serialization
    self.assertEqual(len(list(filter(lambda r: None in r.value, df.collect()))), 0)
AssertionError: 3 != 0

How was this patch tested?

Unittest was added.

./run-tests --python-executables=python3.7 --testname "pyspark.sql.tests.test_serde SerdeTests.test_int_array_serialization"

HyukjinKwon · 2019-05-03T03:05:11Z

cc @BryanCutler and @viirya

This PR could not identify the root cause yet (but only my rough hypothesis). I would like to get this in first and then do the investigation separately.

One concern about using default protocol is that it's not tested in cloudpickle. I opened a PR to add a env to control that and test it (cloudpipe/cloudpickle#265).

HyukjinKwon · 2019-05-03T03:10:31Z

python/pyspark/serializers.py

    basestring = unicode = str
    xrange = range
-pickle_protocol = pickle.HIGHEST_PROTOCOL
+    pickle_protocol = 3


We can use pickle.DEFAULT_PROTOCOL too but let me stick with constant since seems protocol 4 has this bug.

It looks nice and solid. BTW, do you think we can have a pointer for the upstream bug issue against pickle?

Do you mean the bug issue related to the root cause somewhere? (I think) it's more like an issue within Pryolite library .. I am not 100% sure yet. I will update that when I have it. I am looking into this to identify the cause.

SparkQA · 2019-05-03T03:28:32Z

Test build #105096 has finished for PR 24519 at commit 1de0478.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds no public classes.

viirya

Agreed that this can be a temporary fix for the correctness issue. We can investigate it more for root cause after this.

viirya · 2019-05-03T03:31:06Z

The test failed? Can't default protocol fix this?

HyukjinKwon · 2019-05-03T03:37:05Z

Oh, #24519 (comment) was intended to be failed (1de0478). I wanted to make sure it's failed in Jenkins too.

dongjoon-hyun · 2019-05-03T03:38:59Z

The last commit is still under testing~

viirya · 2019-05-03T03:47:41Z

Oh, don't see that. Thanks.

SparkQA · 2019-05-03T03:49:07Z

Test build #105097 has finished for PR 24519 at commit 3ab6eb2.

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

HyukjinKwon · 2019-05-03T05:34:31Z

I can almost confirm that this is a bug in Pyrolite.

Python lists are chunked via BatchedSerializer at parallelize (in createDataFrame), for instance, as below (each line is each batch):

[[1, 2, 3, 4], [1, 2, 3, 4], [1, 2, 3, 4], [1, 2, 3, 4], [1, 2, 3, 4], [1, 2, 3, 4], [1, 2, 3, 4], [1, 2, 3, 4]]
[[1, 2, 3, 4], [1, 2, 3, 4], [1, 2, 3, 4], [1, 2, 3, 4], [1, 2, 3, 4], [1, 2, 3, 4], [1, 2, 3, 4], [1, 2, 3, 4]]
...
[[1, 2, 3, 4], [1, 2, 3, 4], [1, 2, 3, 4], [1, 2, 3, 4]]

Here is opcodes for each batch (separated by ===..) with protocol 4:

Details

==============================
    0: \x80 PROTO      4
    2: \x95 FRAME      47
   11: ]    EMPTY_LIST
   12: \x94 MEMOIZE    (as 0)
   13: (    MARK
   14: ]        EMPTY_LIST
   15: \x94     MEMOIZE    (as 1)
   16: (        MARK
   17: K            BININT1    1
   19: K            BININT1    2
   21: K            BININT1    3
   23: K            BININT1    4
   25: e            APPENDS    (MARK at 16)
   26: \x85     TUPLE1
   27: \x94     MEMOIZE    (as 2)
   28: h        BINGET     1
   30: \x85     TUPLE1
   31: \x94     MEMOIZE    (as 3)
   32: h        BINGET     1
   34: \x85     TUPLE1
   35: \x94     MEMOIZE    (as 4)
   36: h        BINGET     1
   38: \x85     TUPLE1
   39: \x94     MEMOIZE    (as 5)
   40: h        BINGET     1
   42: \x85     TUPLE1
   43: \x94     MEMOIZE    (as 6)
   44: h        BINGET     1
   46: \x85     TUPLE1
   47: \x94     MEMOIZE    (as 7)
   48: h        BINGET     1
   50: \x85     TUPLE1
   51: \x94     MEMOIZE    (as 8)
   52: h        BINGET     1
   54: \x85     TUPLE1
   55: \x94     MEMOIZE    (as 9)
   56: e        APPENDS    (MARK at 13)
   57: .    STOP
highest protocol among opcodes = 4
==============================
==============================
    0: \x80 PROTO      4
    2: \x95 FRAME      47
   11: ]    EMPTY_LIST
   12: \x94 MEMOIZE    (as 0)
   13: (    MARK
   14: ]        EMPTY_LIST
   15: \x94     MEMOIZE    (as 1)
   16: (        MARK
   17: K            BININT1    1
   19: K            BININT1    2
   21: K            BININT1    3
   23: K            BININT1    4
   25: e            APPENDS    (MARK at 16)
   26: \x85     TUPLE1
   27: \x94     MEMOIZE    (as 2)
   28: h        BINGET     1
   30: \x85     TUPLE1
   31: \x94     MEMOIZE    (as 3)
   32: h        BINGET     1
   34: \x85     TUPLE1
   35: \x94     MEMOIZE    (as 4)
   36: h        BINGET     1
   38: \x85     TUPLE1
   39: \x94     MEMOIZE    (as 5)
   40: h        BINGET     1
   42: \x85     TUPLE1
   43: \x94     MEMOIZE    (as 6)
   44: h        BINGET     1
   46: \x85     TUPLE1
   47: \x94     MEMOIZE    (as 7)
   48: h        BINGET     1
   50: \x85     TUPLE1
   51: \x94     MEMOIZE    (as 8)
   52: h        BINGET     1
   54: \x85     TUPLE1
   55: \x94     MEMOIZE    (as 9)
   56: e        APPENDS    (MARK at 13)
   57: .    STOP
highest protocol among opcodes = 4
==============================
...
==============================
    0: \x80 PROTO      4
    2: \x95 FRAME      31
   11: ]    EMPTY_LIST
   12: \x94 MEMOIZE    (as 0)
   13: (    MARK
   14: ]        EMPTY_LIST
   15: \x94     MEMOIZE    (as 1)
   16: (        MARK
   17: K            BININT1    1
   19: K            BININT1    2
   21: K            BININT1    3
   23: K            BININT1    4
   25: e            APPENDS    (MARK at 16)
   26: \x85     TUPLE1
   27: \x94     MEMOIZE    (as 2)
   28: h        BINGET     1
   30: \x85     TUPLE1
   31: \x94     MEMOIZE    (as 3)
   32: h        BINGET     1
   34: \x85     TUPLE1
   35: \x94     MEMOIZE    (as 4)
   36: h        BINGET     1
   38: \x85     TUPLE1
   39: \x94     MEMOIZE    (as 5)
   40: e        APPENDS    (MARK at 13)
   41: .    STOP
highest protocol among opcodes = 4
==============================

Those batches become binary in an RDD at parallelize - so far results look fine
rdd._to_java_object_rdd -> SerDeUtil.pythonToJava -> Unpickler.loads
Here, Pryolite Unpickler.loads converts each Python object batch into Java objects

The converted Java objects look a bit odd from Pryolite as below (see the last three lists):

[1, 2, 3, 4]
[1, 2, 3, 4]
...
[[[1, 2, 3, 4]], [[1, 2, 3, 4]]]
[[[1, 2, 3, 4]], [[1, 2, 3, 4]]]
[[[1, 2, 3, 4]], [[1, 2, 3, 4]]]

Last three nested lists are ignored via EvaluatePython.makeFromJava and becomes null -> None.

So the Jenkins test was failed at [SPARK-27612][PYTHON] Use Python's default protocol instead of highest protocol #24519 (comment) as below:

Traceback (most recent call last):
  File "/home/jenkins/workspace/SparkPullRequestBuilder/python/pyspark/sql/tests/test_serde.py", line 134, in test_int_array_serialization
    self.assertEqual(len(list(filter(lambda r: None in r.value, df.collect()))), 0)
AssertionError: 3 != 0

HyukjinKwon · 2019-05-03T05:37:30Z

Not sure if there's quick fix because notable opcodes that appeared from both protocol 4 and current reproducer is only MEMORIZE. We should peek memo at Unpickler which is private in Pryolite 4.13. It's protected in 4.17 so maybe we can later upgrade Pryolite and debug further.

HyukjinKwon · 2019-05-03T05:39:25Z

Merged to master.

Thanks for review, @dongjoon-hyun and @viirya

…ling ## What changes were proposed in this pull request? In SPARK-27612, one correctness issue was reported. When protocol 4 is used to pickle Python objects, we found that unpickled objects were wrong. A temporary fix was proposed by not using highest protocol. It was found that Opcodes.MEMOIZE was appeared in the opcodes in protocol 4. It is suspect to this issue. A deeper dive found that Opcodes.MEMOIZE stores objects into internal map of Unpickler object. We use single Unpickler object to unpickle serialized Python bytes. Stored objects intervenes next round of unpickling, if the map is not cleared. We has two options: 1. Continues to reuse Unpickler, but calls its close after each unpickling. 2. Not to reuse Unpickler and create new Unpickler object in each unpickling. This patch takes option 1. ## How was this patch tested? Passing the test added in SPARK-27612 (#24519). Closes #24521 from viirya/SPARK-27629. Authored-by: Liang-Chi Hsieh <[email protected]> Signed-off-by: HyukjinKwon <[email protected]>

dongjoon-hyun · 2020-01-25T09:41:58Z

Hi, @HyukjinKwon .
~~This issue is marked as a correctness issue. Do we need to backport this?~~
Never mind. I confirm that this doesn't happen in 2.4.4.

…ling In SPARK-27612, one correctness issue was reported. When protocol 4 is used to pickle Python objects, we found that unpickled objects were wrong. A temporary fix was proposed by not using highest protocol. It was found that Opcodes.MEMOIZE was appeared in the opcodes in protocol 4. It is suspect to this issue. A deeper dive found that Opcodes.MEMOIZE stores objects into internal map of Unpickler object. We use single Unpickler object to unpickle serialized Python bytes. Stored objects intervenes next round of unpickling, if the map is not cleared. We has two options: 1. Continues to reuse Unpickler, but calls its close after each unpickling. 2. Not to reuse Unpickler and create new Unpickler object in each unpickling. This patch takes option 1. Passing the test added in SPARK-27612 (apache#24519). Closes apache#24521 from viirya/SPARK-27629. Authored-by: Liang-Chi Hsieh <[email protected]> Signed-off-by: HyukjinKwon <[email protected]>

Use Python's default protocol instead of highest protocol

1de0478

HyukjinKwon force-pushed the SPARK-27612 branch from 4e65793 to 1de0478 Compare May 3, 2019 03:03

Revert test only codes

3ab6eb2

HyukjinKwon force-pushed the SPARK-27612 branch from 12c5e7c to 3ab6eb2 Compare May 3, 2019 03:09

HyukjinKwon commented May 3, 2019

View reviewed changes

viirya reviewed May 3, 2019

View reviewed changes

dongjoon-hyun approved these changes May 3, 2019

View reviewed changes

viirya approved these changes May 3, 2019

View reviewed changes

HyukjinKwon closed this in 5c47924 May 3, 2019

viirya mentioned this pull request May 3, 2019

[SPARK-27629][PySpark] Prevent Unpickler from intervening each unpickling #24521

Closed

HyukjinKwon deleted the SPARK-27612 branch March 3, 2020 01:18

[SPARK-27612][PYTHON] Use Python's default protocol instead of highest protocol #24519

[SPARK-27612][PYTHON] Use Python's default protocol instead of highest protocol #24519

Uh oh!

Conversation

HyukjinKwon commented May 3, 2019 • 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 commented May 3, 2019

Uh oh!

HyukjinKwon May 3, 2019

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun May 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon May 3, 2019

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 3, 2019

Uh oh!

viirya left a comment

Choose a reason for hiding this comment

Uh oh!

viirya commented May 3, 2019

Uh oh!

HyukjinKwon commented May 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongjoon-hyun commented May 3, 2019

Uh oh!

viirya commented May 3, 2019

Uh oh!

SparkQA commented May 3, 2019

Uh oh!

HyukjinKwon commented May 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HyukjinKwon commented May 3, 2019

Uh oh!

HyukjinKwon commented May 3, 2019

Uh oh!

dongjoon-hyun commented Jan 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

HyukjinKwon commented May 3, 2019 •

edited

Loading

dongjoon-hyun May 3, 2019 •

edited

Loading

HyukjinKwon commented May 3, 2019 •

edited

Loading

HyukjinKwon commented May 3, 2019 •

edited

Loading

dongjoon-hyun commented Jan 25, 2020 •

edited

Loading