[SPARK-22151] : PYTHONPATH not picked up from the spark.yarn.appMaste… #21468

pgandhi999 · 2018-05-31T14:41:08Z

…rEnv properly

Running in yarn cluster mode and trying to set pythonpath via spark.yarn.appMasterEnv.PYTHONPATH doesn't work.

the yarn Client code looks at the env variables:
val pythonPathStr = (sys.env.get("PYTHONPATH") ++ pythonPath)
But when you set spark.yarn.appMasterEnv it puts it into the local env.

So the python path set in spark.yarn.appMasterEnv isn't properly set.

You can work around if you are running in cluster mode by setting it on the client like:

PYTHONPATH=./addon/python/ spark-submit

What changes were proposed in this pull request?

In Client.scala, PYTHONPATH was being overridden, so changed code to append values to PYTHONPATH instead of overriding them.

How was this patch tested?

Added log statements to ApplicationMaster.scala to check for environment variable PYTHONPATH, ran a spark job in cluster mode before the change and verified the issue. Performed the same test after the change and verified the fix.

…rEnv properly Running in yarn cluster mode and trying to set pythonpath via spark.yarn.appMasterEnv.PYTHONPATH doesn't work. the yarn Client code looks at the env variables: val pythonPathStr = (sys.env.get("PYTHONPATH") ++ pythonPath) But when you set spark.yarn.appMasterEnv it puts it into the local env. So the python path set in spark.yarn.appMasterEnv isn't properly set. You can work around if you are running in cluster mode by setting it on the client like: PYTHONPATH=./addon/python/ spark-submit In Client.scala, PYTHONPATH was being overridden, so changed code to append values to PYTHONPATH

tgravescs · 2018-05-31T14:44:07Z

Jenkins, ok to test

SparkQA · 2018-05-31T15:14:32Z

Test build #91345 has finished for PR 21468 at commit 0aee8fa.

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

redsanket · 2018-06-04T19:12:04Z

LGTM @pgandhi999 Hope @tgravescs can confirm it

pgandhi999 · 2018-06-04T19:44:23Z

Thank you for your feedback, @redsanket

vanzin · 2018-06-04T19:41:24Z

resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala

        .mkString(ApplicationConstants.CLASS_PATH_SEPARATOR)
-      env("PYTHONPATH") = pythonPathStr
-      sparkConf.setExecutorEnv("PYTHONPATH", pythonPathStr)
+      val newValue =


You could just say env.get("PYTHONPATH") ++=: pythonPath before turning the list into a string.

But there's also two extra questions here:

precedence; should the env come before or after the files added with py-files? I kinda think after makes more sense, since files are generally provided in the command line.

should appMasterEnv be reflected in executors? With your code it is. I'm not so sure it should.

good questions

precedence: So right now you can work around this issue by exporting PYTHONPATH before you launch spark-submit, I think this is something that could just be in someone's env on the launcher box and might not be what you want in a yarn container. I would think that specifying explicit pythonpath via spark.yarn.appMasterEnv would take precedence over that since you explicitly configured. Now the second question is where that fails with the py-files and that one isn't as clear to me since like you said its explicitly specified. Maybe we do py-files then spark.yarn.appMasterEnv.PYTHONPATH and then last env PYTHONPATH. that is different from the way it is now though. thoughts?

agree this should not be reflected in the executors so if it is we shouldn't do that. We should make sure the spark. executorEnv.PYTHONPATH works

Thank you for your comments @vanzin , have made the necessary changes. As far as precedence is concerned, I am still not sure whether I understood your question at first, however @tgravescs clarified it for me.

Also note as @vanzin said you can just use the ++=: operator with the listbuffer to prepend and get rid of the if conditions before converting to string.
env.get("PYTHONPATH") ++=: (sys.env.get("PYTHONPATH") ++=: pythonPath)

@tgravescs Have replaced the if-else code with ++ operator.

- spark.executorEnv should not be overridden by appMasterEnv - Replacing ++ with ++=:

SparkQA · 2018-06-08T16:38:23Z

Test build #91571 has finished for PR 21468 at commit 5e733ae.

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

HyukjinKwon · 2018-06-09T08:54:44Z

ok to test

SparkQA · 2018-06-09T09:19:45Z

Test build #91603 has finished for PR 21468 at commit 5e733ae.

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

pgandhi999 · 2018-06-09T19:48:12Z

Thank you @HyukjinKwon

tgravescs · 2018-06-22T14:21:43Z

sorry for the delay on this, will get to next monday,

vanzin

The code seems correct but could be simplified a lot, which also would help with clarity.

vanzin · 2018-06-25T23:12:07Z

resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala

    // Finally, update the Spark config to propagate PYTHONPATH to the AM and executors.
    if (pythonPath.nonEmpty) {
-      val pythonPathStr = (sys.env.get("PYTHONPATH") ++ pythonPath)
+      val pythonPathStr = (sys.env.get("PYTHONPATH") ++=: pythonPath)


This is modifying pythonPath, but it's not used again after this line, so why?

Makes sense, have changed it to ++

vanzin · 2018-06-25T23:16:23Z

resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala

        .mkString(ApplicationConstants.CLASS_PATH_SEPARATOR)
-      env("PYTHONPATH") = pythonPathStr
-      sparkConf.setExecutorEnv("PYTHONPATH", pythonPathStr)
+      val newValue =


This seems unnecessary. What you're trying to do here is concatenate the values of the current PYTHONPATH env variable, the PYTHONPATH created by Spark, and the PYTHONPATH set in spark.yarn.appMasterEnv.

That's, respectively, sys.env.get("PYTHONPATH"), pythonPath, and env.get("PYTHONPATH"). So just concatenate those.

I would have done it, but I need the pythonPathStr variable to set it in executorEnv so have not modified that bit. Let me know if you think otherwise.

vanzin · 2018-06-25T23:20:20Z

resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala

+          pythonPathStr
+        }
+      env("PYTHONPATH") = newValue
+      if (!sparkConf.getExecutorEnv.toMap.contains("PYTHONPATH")) {


I see that the previous code was overriding this in the executor env; but perhaps the right thing here is to concatenate them, otherwise the executor might be missing the py4j/pyspark stuff this class adds.

So, basically, what you want is:

driver: env.get(pp) ++ sys.env.get(pp) ++ pythonPath

executor: sparkConf.getExecutorEnv(pp) ++ sys.env.get(pp) ++ pythonPath

That is indeed an issue, fixed it. Thanks for pointing that out.

tgravescs · 2018-06-28T14:34:19Z

resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala

+        }
+      env("PYTHONPATH") = newValue
+      if (!sparkConf.getExecutorEnv.toMap.contains("PYTHONPATH")) {
+        sparkConf.setExecutorEnv("PYTHONPATH", newValue)


when I did some testing of this, this was causing the job to fail, Parth is investigating. I agree with Vanzin on the executor path comment as well.

…THONPATH Instead of overriding the executorenv PYTHONPATH, appending existing pythonpath string to it and avoiding passing value of appMasterEnv.PYTHONPATH to the executorEnv variable

SparkQA · 2018-06-28T18:34:16Z

Test build #92434 has finished for PR 21468 at commit 6ba543e.

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

tgravescs · 2018-07-12T20:38:58Z

resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala

+      } else {
+        val pythonPathExecutorEnv = sparkConf.getExecutorEnv.toMap.get("PYTHONPATH").get +
+          ApplicationConstants.CLASS_PATH_SEPARATOR + pythonPathStr
+        sparkConf.setExecutorEnv("PYTHONPATH", pythonPathExecutorEnv)


move the setExecutorEnv outside of the if and have the if return the value.

SparkQA · 2018-07-16T19:09:59Z

Test build #93128 has finished for PR 21468 at commit 5423bef.

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

SparkQA · 2018-07-16T22:21:48Z

Test build #93140 has finished for PR 21468 at commit 49f37a8.

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

tgravescs · 2018-07-17T15:46:42Z

+1

tgravescs · 2018-07-18T19:07:19Z

merged thanks @pgandhi999

vanzin reviewed Jun 4, 2018

View reviewed changes

[SPARK-22151] : Made changes as per comments

5e733ae

- spark.executorEnv should not be overridden by appMasterEnv - Replacing ++ with ++=:

vanzin reviewed Jun 25, 2018

View reviewed changes

tgravescs reviewed Jun 28, 2018

View reviewed changes

[SPARK-22151] : Appending original pythonpath value to ExecutorEnv.PY…

6ba543e

…THONPATH Instead of overriding the executorenv PYTHONPATH, appending existing pythonpath string to it and avoiding passing value of appMasterEnv.PYTHONPATH to the executorEnv variable

tgravescs reviewed Jul 12, 2018

View reviewed changes

[SPARK-22151] : Addressing comments on setting executorEnv config

5423bef

[SPARK-22151] : Reducing number of lines by using ++ operator

49f37a8

asfgit closed this in 1272b20 Jul 18, 2018

[SPARK-22151] : PYTHONPATH not picked up from the spark.yarn.appMaste… #21468

[SPARK-22151] : PYTHONPATH not picked up from the spark.yarn.appMaste… #21468

Uh oh!

Conversation

pgandhi999 commented May 31, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

tgravescs commented May 31, 2018

Uh oh!

SparkQA commented May 31, 2018

Uh oh!

redsanket commented Jun 4, 2018

Uh oh!

pgandhi999 commented Jun 4, 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!

SparkQA commented Jun 8, 2018

Uh oh!

HyukjinKwon commented Jun 9, 2018

Uh oh!

SparkQA commented Jun 9, 2018

Uh oh!

pgandhi999 commented Jun 9, 2018

Uh oh!

tgravescs commented Jun 22, 2018

Uh oh!

vanzin 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!

vanzin Jun 25, 2018 • 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!

pgandhi999 Jun 28, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 16, 2018

Uh oh!

SparkQA commented Jul 16, 2018

Uh oh!

tgravescs commented Jul 17, 2018

Uh oh!

tgravescs commented Jul 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

vanzin Jun 25, 2018 •

edited

Loading

pgandhi999 Jun 28, 2018 •

edited

Loading