[SPARK-23522][Python] always use sys.exit over builtin exit #20682

benjaminp · 2018-02-27T03:32:28Z

The exit() builtin is only for interactive use. applications should use sys.exit().

What changes were proposed in this pull request?

All usage of the builtin exit() function is replaced by sys.exit().

How was this patch tested?

I ran python/run-tests.

Please review http://spark.apache.org/contributing.html before opening a pull request.

felixcheung · 2018-02-28T08:04:07Z

@HyukjinKwon @ueshin @holdenk

ueshin

I'm not exactly sure the difference between exit() and sys.exit(), but found some missing imports and a nit.

ueshin · 2018-02-28T09:45:23Z

python/pyspark/ml/clustering.py

need to import sys.

ueshin · 2018-02-28T09:52:17Z

python/pyspark/ml/recommendation.py

need to import sys.

ueshin · 2018-02-28T09:52:44Z

python/pyspark/ml/regression.py

need to import sys.

ueshin · 2018-02-28T09:53:24Z

python/pyspark/mllib/classification.py

need to import sys.

ueshin · 2018-02-28T09:53:53Z

python/pyspark/mllib/evaluation.py

need to import sys.

ueshin · 2018-02-28T09:55:17Z

python/pyspark/mllib/tree.py

need to import sys.

ueshin · 2018-02-28T09:57:41Z

python/pyspark/util.py

nit: remove an extra line.

looks like the newline is required by lint. :)

Oh, I see. Thanks!

ueshin · 2018-02-28T10:02:26Z

ok to test.

SparkQA · 2018-02-28T13:07:43Z

Test build #87769 has finished for PR 20682 at commit 311da8a.

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

SparkQA · 2018-02-28T13:45:59Z

Test build #87768 has finished for PR 20682 at commit 311da8a.

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

HyukjinKwon · 2018-02-28T13:49:38Z

Seems fine but I think I need to double check the difference too before actually merging it.

The exit() builtin is only for interactive use. applications should use sys.exit().

benjaminp · 2018-02-28T16:58:59Z

Thanks for the review and sorry about the missing sys imports. I guess my script to add them was buggy. Should be fixed now.

SparkQA · 2018-02-28T17:04:10Z

Test build #87788 has finished for PR 20682 at commit 96529ca.

This patch fails Python style tests.
This patch merges cleanly.
This patch adds no public classes.

holdenk · 2018-02-28T17:20:50Z

So quick reference for anyone looking for the python docs on why should use sys exit: https://docs.python.org/3/library/constants.html#constants-added-by-the-site-module

SparkQA · 2018-02-28T18:06:13Z

Test build #87790 has finished for PR 20682 at commit 3d28bbf.

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

viirya · 2018-03-01T05:10:32Z

retest this please.

SparkQA · 2018-03-01T08:10:41Z

Test build #87815 has finished for PR 20682 at commit 3d28bbf.

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

HyukjinKwon · 2018-03-01T11:44:11Z

retest this please.

SparkQA · 2018-03-01T15:42:42Z

Test build #87833 has finished for PR 20682 at commit 3d28bbf.

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

HyukjinKwon · 2018-03-02T00:41:48Z

python/pyspark/heapq3.py


 if __name__ == "__main__":
-    import doctest
+    import doctest, sys


Shell we import this separately? - https://www.python.org/dev/peps/pep-0008/#imports

HyukjinKwon · 2018-03-02T00:42:33Z

python/pyspark/ml/stat.py

 # limitations under the License.
 #

+import sys


Shall we add a newline below?

HyukjinKwon · 2018-03-02T00:43:44Z

python/pyspark/ml/tuning.py

 # limitations under the License.
 #
 import itertools
+import sys


Could we

import itertools import sys from multiprocessing.pool import ThreadPool import numpy as np ...

?

HyukjinKwon · 2018-03-02T00:44:03Z

python/pyspark/mllib/random.py

 """

+import sys
+


Can we remove this newline?

HyukjinKwon · 2018-03-02T00:44:46Z

python/pyspark/mllib/regression.py

 # limitations under the License.
 #

+import sys


Let's reoprder this too

import sys import warings import numpy as np ...

HyukjinKwon · 2018-03-02T00:45:02Z

python/pyspark/sql/conf.py

 # limitations under the License.
 #

+import sys


Let's add a newline below

HyukjinKwon · 2018-03-02T00:50:38Z

I think dev/run_tests doesn't actually test most of the changes (checking doctest failure) here because it needs a test failure. Can you check one of them with a manual test failure for sure? Running ./python/run_tests will be enough.

benjaminp · 2018-03-02T06:01:35Z

I've now fixed the style as you suggest and got python/run-tests passing.

SparkQA · 2018-03-02T08:05:01Z

Test build #87879 has finished for PR 20682 at commit 4dac3a7.

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

HyukjinKwon · 2018-03-02T09:50:25Z

retest this please

HyukjinKwon · 2018-03-02T09:51:39Z

python/pyspark/ml/evaluation.py

 #

+import sys
+


Let's remove newline here. I think abc is is builtin one too.

HyukjinKwon · 2018-03-02T10:03:08Z

lgtm otherwise

SparkQA · 2018-03-02T13:44:37Z

Test build #87882 has finished for PR 20682 at commit 4dac3a7.

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

SparkQA · 2018-03-02T20:36:47Z

Test build #87891 has finished for PR 20682 at commit c1b7413.

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

HyukjinKwon · 2018-03-07T03:31:30Z

retest this please

SparkQA · 2018-03-07T06:51:58Z

Test build #88036 has finished for PR 20682 at commit c1b7413.

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

HyukjinKwon · 2018-03-07T11:53:32Z

retest this please

SparkQA · 2018-03-07T15:56:11Z

Test build #88042 has finished for PR 20682 at commit c1b7413.

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

HyukjinKwon · 2018-03-08T11:38:42Z

Merged to master.

ueshin reviewed Feb 28, 2018

View reviewed changes

benjaminp added 2 commits February 28, 2018 08:54

[SPARK-23522][Python] always use sys.exit over builtin exit

96d8ca4

The exit() builtin is only for interactive use. applications should use sys.exit().

add missing sys imports

3d28bbf

benjaminp force-pushed the sys-exit branch from 311da8a to 96529ca Compare February 28, 2018 16:57

benjaminp force-pushed the sys-exit branch from 96529ca to 3d28bbf Compare February 28, 2018 17:08

HyukjinKwon reviewed Mar 2, 2018

View reviewed changes

python/pyspark/ml/stat.py

# limitations under the License.

#

import sys

Copy link

Member

HyukjinKwon Mar 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we add a newline below?

HyukjinKwon reviewed Mar 2, 2018

View reviewed changes

python/pyspark/mllib/random.py Outdated

"""

import sys

Copy link

Member

HyukjinKwon Mar 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove this newline?

HyukjinKwon reviewed Mar 2, 2018

View reviewed changes

python/pyspark/sql/conf.py

# limitations under the License.

#

import sys

Copy link

Member

HyukjinKwon Mar 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a newline below

further style fixes

4dac3a7

HyukjinKwon reviewed Mar 2, 2018

View reviewed changes

python/pyspark/ml/evaluation.py Outdated

#

import sys

Copy link

Member

HyukjinKwon Mar 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove newline here. I think abc is is builtin one too.

remove newline

c1b7413

asfgit closed this in 7013eea Mar 8, 2018

[SPARK-23522][Python] always use sys.exit over builtin exit #20682

[SPARK-23522][Python] always use sys.exit over builtin exit #20682

Uh oh!

Conversation

benjaminp commented Feb 27, 2018 • 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!

felixcheung commented Feb 28, 2018

Uh oh!

ueshin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ueshin commented Feb 28, 2018

Uh oh!

SparkQA commented Feb 28, 2018

Uh oh!

SparkQA commented Feb 28, 2018

Uh oh!

HyukjinKwon commented Feb 28, 2018

Uh oh!

benjaminp commented Feb 28, 2018

Uh oh!

SparkQA commented Feb 28, 2018

Uh oh!

holdenk commented Feb 28, 2018

Uh oh!

SparkQA commented Feb 28, 2018

Uh oh!

viirya commented Mar 1, 2018

Uh oh!

SparkQA commented Mar 1, 2018

Uh oh!

HyukjinKwon commented Mar 1, 2018

Uh oh!

SparkQA commented Mar 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Mar 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benjaminp commented Mar 2, 2018

Uh oh!

SparkQA commented Mar 2, 2018

Uh oh!

HyukjinKwon commented Mar 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Mar 2, 2018

benjaminp commented Feb 27, 2018 •

edited

Loading

HyukjinKwon commented Mar 2, 2018 •

edited

Loading