[SPARK-13330][PYSPARK] PYTHONHASHSEED is not propgated to python worker #11211

zjffdu · 2016-02-16T02:14:19Z

What changes were proposed in this pull request?

self.environment will be propagated to executor. Should set PYTHONHASHSEED as long as the python version is greater than 3.3

How was this patch tested?

Manually tested it.

SparkQA · 2016-02-16T02:57:54Z

Test build #51335 has finished for PR 11211 at commit f459705.

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

JoshRosen · 2016-02-16T04:18:26Z

How do we handle this in Python 2? if we're running Python 2.x, do we currently propagate PYTHONHASHSEED to the worker?

Also, how are we going to ensure that this change isn't accidentally rolled back? This seems subtle, so adding an explanatory paragraph comment into the source code near this line would make sense.

zjffdu · 2016-02-16T05:22:40Z

PYTHONHASHSEED is set in script spark-submit no matter what version of python. And it would only be set in executor when python version is greater than 3.3. PYTHONHASHSEED is introduced in python 3.2.3 (https://docs.python.org/3.3/using/cmdline.html). I am not sure the purpose of disable random hash, just feel that we can set PYTHONHASHSEED as 0 in all the cases since it looks like there's no case we want to enable the random of hash. And it's also fine to set it in python 2, because it is only introduced after 3.2.3

JoshRosen · 2016-02-16T16:24:38Z

I think that we need to disable randomness of the hash in order to be able to safely compare hashcodes which were generated by different Python processes / machines.

Could you do a little detective work to look through the Git history and try to figure out whether this propagation ever worked or determine whether it was broken recently? If it used to work and was broken only in 1.6 then I think that there might be another root cause that we should find rather than patching around this one specific issue in Python.

zjffdu · 2016-02-17T10:47:17Z

I verified it on spark-1.4.1 and spark-1.5.2, both of them have this issue. I believe it exists for a long time. Besides that I found pyspark specific environment variable is not propagated to driver if it is cluster mode. Create SPARK-13360 for it, after SPARK-13360 I will update this PR.

JoshRosen · 2016-02-17T17:25:45Z

I'm a bit concerned that the change implemented here won't be correct in case PYTHONHASHSEED is set to a non-zero value on the driver. In that case, wouldn't the driver and executors wind up with different hash seeds, leading to incomparable hash values?

zjffdu · 2016-02-18T00:13:16Z

Right, it should be set as the same as driver rather than set it as 0 explicitly.

JoshRosen · 2016-02-18T01:00:27Z

Okay. Are you planning to update this PR to handle that case?

zjffdu · 2016-02-18T02:56:41Z

I plan to do it after SPARK-13360, or I can do them together in this PR if it is OK

zjffdu · 2016-02-24T02:15:08Z

@JoshRosen I update the patch and also merge SPARK-13360 into this PR because they are related. Please help review it. Thanks

zjffdu · 2016-02-24T02:48:23Z

Jenkins, please build it again.

SparkQA · 2016-02-24T04:25:21Z

Test build #51835 has finished for PR 11211 at commit 43dfd47.

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

felixcheung · 2016-02-27T19:01:19Z

python/pyspark/context.py

is there a reason for removing the python sys.version check?

PYTHONHASHSEED is introduced from 3.2.3, it doesn't matter we use it in versions before 3.2.3

Is there any use in allowing the user to override the value?

It's a bit of a stretch, but if a user has another Python application that is producing hashed data with a fixed seed (say a flask app), a user might want to set this to the same value.

zjffdu · 2016-03-08T12:23:44Z

Ping @JoshRosen

JoshRosen · 2016-03-23T17:33:11Z

Now that your other patch has been merged, is this ready to update?

zjffdu · 2016-03-30T05:22:50Z

Update the patch.

SparkQA · 2016-03-30T07:44:34Z

Test build #54497 has finished for PR 11211 at commit e68899c.

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

JoshRosen · 2016-04-05T00:01:06Z

python/pyspark/rdd.py

Good catch on the minimum supported version.

I'm always a little wary of string comparisons for these...

>>> '3.10.0' > '3.2.3' False

I would avoid the string comparison here. If you must, you can use distutils.version.StrictVersion

In [15]: StrictVersion('3.10.0') > StrictVersion('3.2.3') Out[15]: True

but sys.version will break anyway on some Python distributions.

In [16]: print(sys.version) 3.4.4 |Continuum Analytics, Inc.| (default, Jan 9 2016, 17:30:09) [GCC 4.2.1 (Apple Inc. build 5577)]

You might want to use sys.version_info instead.

We also use sys.version_info in the PySpark setup.py file now.

JoshRosen · 2016-04-05T00:05:06Z

Quick clarification: would calling self._conf.setExecutorEnv(key, value) in context.py be sufficient to propagate the variable to the executor, even in YARN mode?

zjffdu · 2016-04-19T09:36:20Z

@JoshRosen Sorry for response late. Yes, self._conf.setExecutorEnv(key, value) in context.py will propagate the variable to the executor, even in YARN mode

JoshRosen · 2016-04-21T18:47:15Z

Okay, so why don't we go ahead and use setExecutorEnv then?

zjffdu · 2016-04-22T01:27:12Z

The executor here may be a little misleading. It means the python worker rather than the spark executor.
Python worker get its env from PythonRDD and PythonRDD get the env from context.environment. ( I have updated the title)

See PythonRDD.scala

val worker: Socket = env.createPythonWorker(pythonExec, envVars.asScala.toMap)

SparkQA · 2016-05-23T17:49:37Z

Test build #59134 has finished for PR 11211 at commit e68899c.

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

holdenk · 2016-10-08T04:57:43Z

For what its worth it seems like this has maybe caused issues in the past too - https://issues.apache.org/jira/browse/SPARK-12100

zjffdu · 2016-10-09T03:33:38Z

Conflict is resolved.

SparkQA · 2016-10-09T05:48:59Z

Test build #66590 has finished for PR 11211 at commit b761b85.

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

felixcheung · 2016-10-09T22:16:18Z

LGTM. @JoshRosen do you have more thought on this?

vanzin · 2016-12-02T02:46:31Z

core/src/main/scala/org/apache/spark/deploy/PythonRunner.scala

I'd rather do sys.env.get("PYTHONHASHSEED").foreach { ... } to avoid polluting the environment when not needed. (Also to avoid hardcoding the default in multiple places.)

vanzin · 2016-12-02T02:47:08Z

yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala

Same comment regarding not polluting the environment.

HyukjinKwon · 2017-02-09T12:48:24Z

(ping @zjffdu)

holdenk · 2017-02-13T22:00:37Z

gentle re-ping - is this something you have badnwidth to work on @zjffdu?

zjffdu · 2017-02-14T03:26:29Z

Sorry for late reply, I may come back to this issue late of this week.

SparkQA · 2017-02-17T11:40:52Z

Test build #73046 has finished for PR 11211 at commit e352c36.

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

SparkQA · 2017-02-17T14:21:28Z

Test build #73049 has finished for PR 11211 at commit 0acbb18.

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

zjffdu · 2017-02-21T05:32:46Z

ping @holdenk @HyukjinKwon PR is updated, please help review. Thanks

vanzin

LGTM, I'll let Holden take a last look.

holdenk · 2017-02-22T23:19:15Z

LGTM as well, thanks for taking a look @vanzin and thanks for taking the time on this @zjffdu & sticking with it.

holdenk · 2017-02-22T23:20:49Z

Actually one last thing before we merge this PR - would you be ok with updating the description to match the template that is now used? Just that way it is consistent with everything.

zjffdu · 2017-02-23T06:23:33Z

@holdenk description is updated.

holdenk · 2017-02-24T23:10:29Z

thanks @zjffdu , merged to master.

## What changes were proposed in this pull request? self.environment will be propagated to executor. Should set PYTHONHASHSEED as long as the python version is greater than 3.3 ## How was this patch tested? Manually tested it. Author: Jeff Zhang <[email protected]> Closes apache#11211 from zjffdu/SPARK-13330.

mxhellor · 2018-06-20T09:51:13Z

how do we resolve the problem?

felixcheung reviewed Feb 27, 2016
View reviewed changes

zjffdu force-pushed the SPARK-13330 branch from 43dfd47 to e68899c Compare March 30, 2016 05:22

JoshRosen reviewed Apr 5, 2016
View reviewed changes

zjffdu changed the title ~~[SPARK-13330][PYSPARK] PYTHONHASHSEED is not propgated to executor~~ [SPARK-13330][PYSPARK] PYTHONHASHSEED is not propgated to python worker Apr 22, 2016

zjffdu force-pushed the SPARK-13330 branch from e68899c to b761b85 Compare October 9, 2016 03:33

vanzin reviewed Dec 2, 2016

View reviewed changes

zjffdu added 5 commits February 17, 2017 17:24

[SPARK-13330][PYSPARK] PYTHONHASHSEED is not propgated to executor

3439c58

Fix it

527b410

Merge commit of SPARK-13360

5d78f3f

fix it

1d12645

fix code style

acea6c3

zjffdu force-pushed the SPARK-13330 branch from b761b85 to e352c36 Compare February 17, 2017 09:32

address comments

0acbb18

zjffdu force-pushed the SPARK-13330 branch from e352c36 to 0acbb18 Compare February 17, 2017 11:45

vanzin reviewed Feb 21, 2017

View reviewed changes

asfgit closed this in 330c3e3 Feb 24, 2017

[SPARK-13330][PYSPARK] PYTHONHASHSEED is not propgated to python worker #11211

[SPARK-13330][PYSPARK] PYTHONHASHSEED is not propgated to python worker #11211

Uh oh!

Conversation

zjffdu commented Feb 16, 2016 • 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!

SparkQA commented Feb 16, 2016

Uh oh!

JoshRosen commented Feb 16, 2016

Uh oh!

zjffdu commented Feb 16, 2016

Uh oh!

JoshRosen commented Feb 16, 2016

Uh oh!

zjffdu commented Feb 17, 2016

Uh oh!

JoshRosen commented Feb 17, 2016

Uh oh!

zjffdu commented Feb 18, 2016

Uh oh!

JoshRosen commented Feb 18, 2016

Uh oh!

zjffdu commented Feb 18, 2016

Uh oh!

zjffdu commented Feb 24, 2016

Uh oh!

zjffdu commented Feb 24, 2016

Uh oh!

SparkQA commented Feb 24, 2016

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!

zjffdu commented Mar 8, 2016

Uh oh!

JoshRosen commented Mar 23, 2016

Uh oh!

zjffdu commented Mar 30, 2016

Uh oh!

SparkQA commented Mar 30, 2016

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!

JoshRosen commented Apr 5, 2016

Uh oh!

zjffdu commented Apr 19, 2016

Uh oh!

JoshRosen commented Apr 21, 2016

Uh oh!

zjffdu commented Apr 22, 2016

Uh oh!

SparkQA commented May 23, 2016

Uh oh!

holdenk commented Oct 8, 2016

Uh oh!

zjffdu commented Oct 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Oct 9, 2016

Uh oh!

felixcheung commented Oct 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zjffdu commented Feb 16, 2016 •

edited

Loading

zjffdu commented Oct 9, 2016 •

edited

Loading