[SPARK-19505][Python] AttributeError on Exception.message in Python3 #16845

dgingrich · 2017-02-08T01:08:28Z

What changes were proposed in this pull request?

Added util._message_exception helper to use str(e) when e.message is unavailable (Python3). Grepped for all occurrences of .message in pyspark/ and these were the only occurrences.

How was this patch tested?

Doctests for helper function

Legal

This is my original work and I license the work to the project under the project’s open source license.

HyukjinKwon · 2017-02-08T06:22:05Z

Yes, up to my knowledge, message attribute is deprecated from 2.6 and removed in 3.x. It seems reasonable. Let me please cc @holdenk.

HyukjinKwon · 2017-02-08T06:25:14Z

python/pyspark/broadcast.py

@@ -82,7 +82,7 @@ def dump(self, value, f):
        except pickle.PickleError:
            raise
        except Exception as e:
-            msg = "Could not serialize broadcast: " + e.__class__.__name__ + ": " + e.message
+            msg = "Could not serialize broadcast: " + e.__class__.__name__ + ": " + str(e)


BTW, we should keep in mind that

>>> Exception(u"jörn").message u'j\xf6rn' >>> str(Exception(u"jörn")) Traceback (most recent call last): File "<stdin>", line 1, in <module> UnicodeEncodeError: 'ascii' codec can't encode character u'\xf6' in position 1: ordinal not in range(128)

this could cause such encoding problem. I think it'd be really rare case though. It seems this usage is already there in the code base so I assume it is fine.

Good call, I forgot about Python 2's str/unicode behavior. It could be a problem if an exception includes user input as part of the message. IMO it's worth handling it rigorously since throwing in the except: hides the original error. I'll look at fixes tonight, in the worse case we could add a get_exception_message() helper.

holdenk · 2017-02-13T19:49:21Z

+1 to @HyukjinKwon comment.

* Add util._exception_message helper * Switch from "e.message" to "_exception_message(e)"

dgingrich · 2017-02-14T18:15:29Z

Added a helper that falls back from e.message to str(e). Also rebased to master.

HyukjinKwon · 2017-02-18T15:54:14Z

(Actually, I personally prefer str(e) unless we can identify any actual case although it is a potential problem but maybe too narrow to introduce another util.. Maybe, I am being too cautious.)

HyukjinKwon · 2017-02-18T15:56:09Z

BTW, could anyone say "ok to test"? Anyhow it is true that Exception does not have message in Python 3 and we need a fix.

dgingrich · 2017-02-18T17:04:48Z

IMO the helper is better since the Exception message is controlled by the user. I thought Holden was +1'ing the problem (not handling unicode) but re-reading she might have been +1'ing that it was fine. Let me know which way you prefer, it's trivial to switch back to using str.

HyukjinKwon · 2017-02-18T17:10:22Z

Yea, I think she is meant to decide this :).

dgingrich · 2017-02-18T18:54:28Z

Another alternate is to use six. In the standard library text_type works:

# Python2.6
>>> import six
>>> six.text_type(Exception(u"unicöde"))
u'unic\xf6de'

I also submitted a patch to six to add an exception_message helper (benjaminp/six#177) but I don't know if it will be accepted since Exception.message has been deprecated since 2.6.

holdenk · 2017-02-18T19:05:33Z

Let's see if I can ask Jenkins to test or if I need to get another list updates for that. Jenkins ok to test.

holdenk · 2017-02-22T22:41:57Z

Ok seems like I need to bug one of the other committers to ask Jenkins to test this, maybe @davies can do this in the meantime?

holdenk · 2017-02-24T22:34:11Z

Ok in theory I've been added, so lets see Jenkins test this please.

SparkQA · 2017-02-24T23:06:52Z

Test build #73446 has finished for PR 16845 at commit 7b8ace4.

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

holdenk

Thanks for working on this, I've got a few questions but it looks in pretty good shape already :)

holdenk · 2017-02-15T18:23:42Z

python/pyspark/broadcast.py

@@ -82,7 +83,8 @@ def dump(self, value, f):
        except pickle.PickleError:
            raise
        except Exception as e:
-            msg = "Could not serialize broadcast: " + e.__class__.__name__ + ": " + e.message
+            msg = ("Could not serialize broadcast: " + e.__class__.__name__ + ": "


Would this maybe be easier to read using format rather than string concatenation?

Sure, I was trying to minimize changes but I prefer formatting with %. Switched.

holdenk · 2017-02-24T23:27:45Z

python/pyspark/util.py

+
+if __name__ == "__main__":
+    import doctest
+    (failure_count, test_count) = doctest.testmod()


These tests won't be run by the default test script, we could fix this by updating the list of things to test - but since this function is only used in one place for now maybe it would make sense to just put this in cloudpickle.py anyways. What do you think?

Well the util is called from both cloudpickle.py and broadcast.py, so I'd rather leave the helper here and add it the list of things to test. Leaving it as is for now but willing to change.

That seems reasonable, in that case you will need to make sure this is called by the test scripts (take a look at ./dev/sparktestsupport/modules.py).

Thanks Holden, looking at this today (sorry for the delay, been busy at work).

FWIW the patch to add support for the utility function six was rejected: benjaminp/six#177

holdenk · 2017-03-09T19:38:32Z

So being able to format the messages in the 2.X line and 3 line seems like a reasonable improvement, even if we are using a deprecated feature in 2.6+ for the 2.X line. Let me know when you have a chance to update the PR so the tests are run.

dgingrich · 2017-03-09T19:49:16Z

Added to pyspark.util to modules. Let me know if you want any other changes.

dgingrich · 2017-03-31T21:32:51Z

Ping! Let me know if you need more work from me.

holdenk · 2017-04-06T10:59:46Z

LGTM - sorry for the slowness.

holdenk · 2017-04-06T11:01:08Z

Let's make sure everything is still fine since we've made some changes in the testing infrastructure though - if it passes I'll merge this tonight (european time).

Jenkins retest this please.

dgingrich · 2017-04-06T17:50:12Z

Great, thanks Holden!

holdenk · 2017-04-09T11:09:50Z

Jenkins, retest this please.

SparkQA · 2017-04-09T14:03:50Z

Test build #75631 has finished for PR 16845 at commit 91dc9bf.

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

holdenk · 2017-04-11T19:23:41Z

Merged to master

## What changes were proposed in this pull request? Added `util._message_exception` helper to use `str(e)` when `e.message` is unavailable (Python3). Grepped for all occurrences of `.message` in `pyspark/` and these were the only occurrences. ## How was this patch tested? - Doctests for helper function ## Legal This is my original work and I license the work to the project under the project’s open source license. Author: David Gingrich <[email protected]> Closes apache#16845 from dgingrich/topic-spark-19505-py3-exceptions.

HyukjinKwon reviewed Feb 8, 2017

View reviewed changes

Support Python 3 exception messages

7b8ace4

* Add util._exception_message helper * Switch from "e.message" to "_exception_message(e)"

dgingrich force-pushed the topic-spark-19505-py3-exceptions branch from cb5c34d to 7b8ace4 Compare February 14, 2017 18:02

holdenk reviewed Feb 24, 2017

View reviewed changes

Switch from string concatenation to format strings

210ac8f

dgingrich mentioned this pull request Mar 1, 2017

Add function to get Exception.message benjaminp/six#177

Closed

Added pyspark.util to test modules

91dc9bf

dgingrich force-pushed the topic-spark-19505-py3-exceptions branch from f4428a1 to 91dc9bf Compare March 9, 2017 19:47

asfgit closed this in 6297697 Apr 11, 2017

HyukjinKwon mentioned this pull request Nov 21, 2017

[SPARK-22395][SQL][PYTHON] Fix the behavior of timestamp values for Pandas to respect session timezone #19607

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-19505][Python] AttributeError on Exception.message in Python3 #16845

[SPARK-19505][Python] AttributeError on Exception.message in Python3 #16845

dgingrich commented Feb 8, 2017 •

edited

Loading

HyukjinKwon commented Feb 8, 2017 •

edited

Loading

HyukjinKwon Feb 8, 2017 •

edited

Loading

dgingrich Feb 8, 2017

holdenk commented Feb 13, 2017

dgingrich commented Feb 14, 2017

HyukjinKwon commented Feb 18, 2017

HyukjinKwon commented Feb 18, 2017

dgingrich commented Feb 18, 2017 •

edited

Loading

HyukjinKwon commented Feb 18, 2017

dgingrich commented Feb 18, 2017 •

edited

Loading

holdenk commented Feb 18, 2017

holdenk commented Feb 22, 2017

holdenk commented Feb 24, 2017

SparkQA commented Feb 24, 2017

holdenk left a comment

holdenk Feb 15, 2017

dgingrich Feb 28, 2017

holdenk Feb 24, 2017

dgingrich Feb 28, 2017

holdenk Feb 28, 2017

dgingrich Mar 9, 2017

holdenk commented Mar 9, 2017

dgingrich commented Mar 9, 2017

dgingrich commented Mar 31, 2017

holdenk commented Apr 6, 2017

holdenk commented Apr 6, 2017

dgingrich commented Apr 6, 2017

holdenk commented Apr 9, 2017

SparkQA commented Apr 9, 2017

holdenk commented Apr 11, 2017

[SPARK-19505][Python] AttributeError on Exception.message in Python3 #16845

[SPARK-19505][Python] AttributeError on Exception.message in Python3 #16845

Conversation

dgingrich commented Feb 8, 2017 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

Legal

HyukjinKwon commented Feb 8, 2017 • edited Loading

HyukjinKwon Feb 8, 2017 • edited Loading

Choose a reason for hiding this comment

dgingrich Feb 8, 2017

Choose a reason for hiding this comment

holdenk commented Feb 13, 2017

dgingrich commented Feb 14, 2017

HyukjinKwon commented Feb 18, 2017

HyukjinKwon commented Feb 18, 2017

dgingrich commented Feb 18, 2017 • edited Loading

HyukjinKwon commented Feb 18, 2017

dgingrich commented Feb 18, 2017 • edited Loading

holdenk commented Feb 18, 2017

holdenk commented Feb 22, 2017

holdenk commented Feb 24, 2017

SparkQA commented Feb 24, 2017

holdenk left a comment

Choose a reason for hiding this comment

holdenk Feb 15, 2017

Choose a reason for hiding this comment

dgingrich Feb 28, 2017

Choose a reason for hiding this comment

holdenk Feb 24, 2017

Choose a reason for hiding this comment

dgingrich Feb 28, 2017

Choose a reason for hiding this comment

holdenk Feb 28, 2017

Choose a reason for hiding this comment

dgingrich Mar 9, 2017

Choose a reason for hiding this comment

holdenk commented Mar 9, 2017

dgingrich commented Mar 9, 2017

dgingrich commented Mar 31, 2017

holdenk commented Apr 6, 2017

holdenk commented Apr 6, 2017

dgingrich commented Apr 6, 2017

holdenk commented Apr 9, 2017

SparkQA commented Apr 9, 2017

holdenk commented Apr 11, 2017

dgingrich commented Feb 8, 2017 •

edited

Loading

HyukjinKwon commented Feb 8, 2017 •

edited

Loading

HyukjinKwon Feb 8, 2017 •

edited

Loading

dgingrich commented Feb 18, 2017 •

edited

Loading

dgingrich commented Feb 18, 2017 •

edited

Loading