Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
pgandhi999 wants to merge 5 commits into from
Closed

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

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0aee8fa
[SPARK-22151] : PYTHONPATH not picked up from the spark.yarn.appMaste…
May 31, 2018
5e733ae
[SPARK-22151] : Made changes as per comments
Jun 8, 2018
6ba543e
[SPARK-22151] : Appending original pythonpath value to ExecutorEnv.PY…
Jun 28, 2018
5423bef
[SPARK-22151] : Addressing comments on setting executorEnv config
Jul 16, 2018
49f37a8
[SPARK-22151] : Reducing number of lines by using ++ operator
Jul 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
[SPARK-22151] : Made changes as per comments 
- spark.executorEnv should not be overridden by appMasterEnv

- Replacing ++ with ++=:
  • Loading branch information
pgandhi committed Jun 8, 2018
commit 5e733aeee66e7aedcccd9eed76f539ce77919e78
6 changes: 4 additions & 2 deletions resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -811,7 +811,7 @@ private[spark] class Client(

// 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)
Copy link
Contributor

@vanzin vanzin Jun 25, 2018

Choose a reason for hiding this comment

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

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

Copy link
Author

@pgandhi999 pgandhi999 Jun 28, 2018

Choose a reason for hiding this comment

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

Makes sense, have changed it to ++

.mkString(ApplicationConstants.CLASS_PATH_SEPARATOR)
val newValue =
Copy link
Contributor

@vanzin vanzin Jun 4, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@tgravescs tgravescs Jun 7, 2018

Choose a reason for hiding this comment

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

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

Copy link
Author

@pgandhi999 pgandhi999 Jun 8, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@tgravescs tgravescs Jul 16, 2018

Choose a reason for hiding this comment

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

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)

Copy link
Author

@pgandhi999 pgandhi999 Jul 16, 2018

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@vanzin vanzin Jun 25, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Author

@pgandhi999 pgandhi999 Jun 28, 2018

Choose a reason for hiding this comment

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

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.

if (env.contains("PYTHONPATH")) {
Expand All @@ -820,7 +820,9 @@ private[spark] class Client(
pythonPathStr
}
env("PYTHONPATH") = newValue
sparkConf.setExecutorEnv("PYTHONPATH", newValue)
if (!sparkConf.getExecutorEnv.toMap.contains("PYTHONPATH")) {
Copy link
Contributor

@vanzin vanzin Jun 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Author

@pgandhi999 pgandhi999 Jun 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

sparkConf.setExecutorEnv("PYTHONPATH", newValue)
Copy link
Contributor

@tgravescs tgravescs Jun 28, 2018

Choose a reason for hiding this comment

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

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.

}
}

if (isClusterMode) {
Expand Down