-
Notifications
You must be signed in to change notification settings - Fork 118
Python Bindings for launching PySpark Jobs from the JVM (v1) #351
Changes from 10 commits
d3cf58f
bafc13c
59d9f0a
4daf634
51105ca
bd30f40
720776e
4b5f470
1361a26
0abc3b1
0869b07
38d48ce
9bf7b9d
4561194
2cf96cc
eb1079a
4a6b779
363919a
9c7adb1
0388aa4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,11 +47,14 @@ private[spark] class Client( | |
appName: String, | ||
kubernetesResourceNamePrefix: String, | ||
kubernetesAppId: String, | ||
mainAppResource: String, | ||
isPython: Boolean, | ||
mainClass: String, | ||
sparkConf: SparkConf, | ||
appArgs: Array[String], | ||
sparkJars: Seq[String], | ||
sparkFiles: Seq[String], | ||
pySparkFiles: List[String], | ||
waitForAppCompletion: Boolean, | ||
kubernetesClient: KubernetesClient, | ||
initContainerComponentsProvider: DriverInitContainerComponentsProvider, | ||
|
@@ -83,7 +86,14 @@ private[spark] class Client( | |
def run(): Unit = { | ||
validateNoDuplicateFileNames(sparkJars) | ||
validateNoDuplicateFileNames(sparkFiles) | ||
|
||
if (isPython) {validateNoDuplicateFileNames(pySparkFiles)} | ||
val arguments = if (isPython) pySparkFiles match { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wonder if this could be factored to scale out cleaner. For example, if we add R next, is that going to be a new 3rd layer of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see --r-files as a submission type so it is something to look into. |
||
case Nil => appArgs | ||
case a::b => a match { | ||
case _ if a==mainAppResource && b==Nil => appArgs | ||
case _ => appArgs.drop(1) | ||
} | ||
} else appArgs | ||
val driverCustomLabels = ConfigurationUtils.combinePrefixedKeyValuePairsWithDeprecatedConf( | ||
sparkConf, | ||
KUBERNETES_DRIVER_LABEL_PREFIX, | ||
|
@@ -135,7 +145,7 @@ private[spark] class Client( | |
.endEnv() | ||
.addNewEnv() | ||
.withName(ENV_DRIVER_ARGS) | ||
.withValue(appArgs.mkString(" ")) | ||
.withValue(arguments.mkString(" ")) | ||
.endEnv() | ||
.withNewResources() | ||
.addToRequests("cpu", driverCpuQuantity) | ||
|
@@ -173,10 +183,12 @@ private[spark] class Client( | |
.bootstrapInitContainerAndVolumes(driverContainer.getName, basePod) | ||
|
||
val containerLocalizedFilesResolver = initContainerComponentsProvider | ||
.provideContainerLocalizedFilesResolver() | ||
.provideContainerLocalizedFilesResolver(mainAppResource) | ||
val resolvedSparkJars = containerLocalizedFilesResolver.resolveSubmittedSparkJars() | ||
val resolvedSparkFiles = containerLocalizedFilesResolver.resolveSubmittedSparkFiles() | ||
|
||
val resolvedPySparkFiles = containerLocalizedFilesResolver.resolveSubmittedPySparkFiles() | ||
val resolvedPrimaryPySparkResource = if (!isPython) "" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use an |
||
else { containerLocalizedFilesResolver.resolvePrimaryResourceFile() } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Braces should always follow
regardless of how long the logic in each block is. The exception to this rule is for something that should act like a ternary operator and can fit entirely in the same line. |
||
val executorInitContainerConfiguration = initContainerComponentsProvider | ||
.provideExecutorInitContainerConfiguration() | ||
val sparkConfWithExecutorInit = executorInitContainerConfiguration | ||
|
@@ -204,7 +216,7 @@ private[spark] class Client( | |
val resolvedDriverJavaOpts = resolvedSparkConf.getAll.map { | ||
case (confKey, confValue) => s"-D$confKey=$confValue" | ||
}.mkString(" ") + driverJavaOptions.map(" " + _).getOrElse("") | ||
val resolvedDriverPod = podWithInitContainerAndMountedCreds.editSpec() | ||
val resolvedDriverPodBuilder = podWithInitContainerAndMountedCreds.editSpec() | ||
.editMatchingContainer(new ContainerNameEqualityPredicate(driverContainer.getName)) | ||
.addNewEnv() | ||
.withName(ENV_MOUNTED_CLASSPATH) | ||
|
@@ -216,7 +228,18 @@ private[spark] class Client( | |
.endEnv() | ||
.endContainer() | ||
.endSpec() | ||
.build() | ||
val resolvedDriverPod = if (!isPython) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we try to boil all of the logic guarded by the |
||
resolvedDriverPodBuilder.build() | ||
} else { | ||
initContainerComponentsProvider | ||
.provideDriverPodFileMounter() | ||
.addPySparkFiles( | ||
resolvedPrimaryPySparkResource, | ||
resolvedPySparkFiles.mkString(","), | ||
driverContainer.getName, | ||
resolvedDriverPodBuilder) | ||
.build() | ||
} | ||
Utils.tryWithResource( | ||
kubernetesClient | ||
.pods() | ||
|
@@ -274,22 +297,31 @@ private[spark] object Client { | |
val appArgs = args.drop(2) | ||
run(sparkConf, mainAppResource, mainClass, appArgs) | ||
} | ||
|
||
def run( | ||
sparkConf: SparkConf, | ||
mainAppResource: String, | ||
mainClass: String, | ||
appArgs: Array[String]): Unit = { | ||
val sparkJars = sparkConf.getOption("spark.jars") | ||
.map(_.split(",")) | ||
.getOrElse(Array.empty[String]) ++ | ||
Option(mainAppResource) | ||
val isPython = mainAppResource.endsWith(".py") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alluding to a remark from earlier - we might want to treat these arguments differently. For example we could take a command line argument that is a "language mode" and expect SparkSubmit to give us the right language mode and handle accordingly - e.g. Scala, Python, R. We have control over the arguments that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True. but R will also have MainResource be the Python file, but there are no --r-files. So arguments logic is only for Python. I think it is simple enough that refactoring the arguments, might not be necessary. Something to consider, but I agree with what you are saying There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm more wary of the fact that we're matching against There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason I match on NIl is because the appArgs being passed in are: |
||
// Since you might need jars for SQL UDFs in PySpark | ||
def sparkJarFilter() : Seq[String] = isPython match { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use if/else here if matching only on true/false. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was less lines haha :) but okay I will go back to if/else |
||
case true => Seq.empty[String] | ||
case false => Option(mainAppResource) | ||
.filterNot(_ == SparkLauncher.NO_RESOURCE) | ||
.toSeq | ||
} | ||
val sparkJars = sparkConf.getOption("spark.jars") | ||
.map(_.split(",")) | ||
.getOrElse(Array.empty[String]) ++ sparkJarFilter() | ||
val launchTime = System.currentTimeMillis | ||
val sparkFiles = sparkConf.getOption("spark.files") | ||
.map(_.split(",")) | ||
.getOrElse(Array.empty[String]) | ||
val pySparkFiles: Array[String] = if (isPython) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm starting to wonder if we want our arguments to be similar to CLI arguments. We can then adjust For example, we could expect our main method to have arguments like this:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Basically we can reformat There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ifilonenko thoughts on this? |
||
Option(appArgs(0)) match { | ||
case None => Array(mainAppResource) | ||
case Some(a) => mainAppResource +: a.split(",") } | ||
} else { Array.empty[String] } | ||
val appName = sparkConf.getOption("spark.app.name").getOrElse("spark") | ||
// The resource name prefix is derived from the application name, making it easy to connect the | ||
// names of the Kubernetes resources from e.g. Kubectl or the Kubernetes dashboard to the | ||
|
@@ -308,6 +340,7 @@ private[spark] object Client { | |
namespace, | ||
sparkJars, | ||
sparkFiles, | ||
pySparkFiles, | ||
sslOptionsProvider.getSslOptions) | ||
Utils.tryWithResource(SparkKubernetesClientFactory.createKubernetesClient( | ||
master, | ||
|
@@ -328,11 +361,14 @@ private[spark] object Client { | |
appName, | ||
kubernetesResourceNamePrefix, | ||
kubernetesAppId, | ||
mainAppResource, | ||
isPython, | ||
mainClass, | ||
sparkConf, | ||
appArgs, | ||
sparkJars, | ||
sparkFiles, | ||
pySparkFiles.toList, | ||
waitForAppCompletion, | ||
kubernetesClient, | ||
initContainerComponentsProvider, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,13 +24,19 @@ private[spark] trait ContainerLocalizedFilesResolver { | |
def resolveSubmittedAndRemoteSparkJars(): Seq[String] | ||
def resolveSubmittedSparkJars(): Seq[String] | ||
def resolveSubmittedSparkFiles(): Seq[String] | ||
def resolveSubmittedPySparkFiles(): Seq[String] | ||
def resolvePrimaryResourceFile(): String | ||
} | ||
|
||
private[spark] class ContainerLocalizedFilesResolverImpl( | ||
sparkJars: Seq[String], | ||
sparkFiles: Seq[String], | ||
pySparkFiles: Seq[String], | ||
primaryPyFile: String, | ||
jarsDownloadPath: String, | ||
filesDownloadPath: String) extends ContainerLocalizedFilesResolver { | ||
filesDownloadPath: String | ||
) extends ContainerLocalizedFilesResolver { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Move this line up - notice the diff from what this had before. |
||
|
||
|
||
override def resolveSubmittedAndRemoteSparkJars(): Seq[String] = { | ||
sparkJars.map { jar => | ||
|
@@ -53,16 +59,33 @@ private[spark] class ContainerLocalizedFilesResolverImpl( | |
resolveSubmittedFiles(sparkFiles, filesDownloadPath) | ||
} | ||
|
||
private def resolveSubmittedFiles(files: Seq[String], downloadPath: String): Seq[String] = { | ||
files.map { file => | ||
val fileUri = Utils.resolveURI(file) | ||
Option(fileUri.getScheme).getOrElse("file") match { | ||
case "file" => | ||
val fileName = new File(fileUri.getPath).getName | ||
s"$downloadPath/$fileName" | ||
case _ => | ||
file | ||
} | ||
override def resolveSubmittedPySparkFiles(): Seq[String] = { | ||
def filterMainResource(x: String) = x match { | ||
case `primaryPyFile` => None | ||
case _ => Some(resolveFile(x, filesDownloadPath)) | ||
} | ||
pySparkFiles.flatMap(x => filterMainResource(x)) | ||
} | ||
|
||
override def resolvePrimaryResourceFile(): String = { | ||
Option(primaryPyFile) match { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not? Is that Spark specific scala practice? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not just spark-specific but seems to be the standard across all of Scala. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See https://www.scala-lang.org/api/current/scala/Option.html "The most idiomatic way to use an scala.Option instance is to treat it as a collection or monad and use map,flatMap, filter, or foreach... A less-idiomatic way to use scala.Option values is via pattern matching" |
||
case None => "" | ||
case Some(p) => resolveFile(p, filesDownloadPath) | ||
} | ||
} | ||
|
||
private def resolveFile(file: String, downloadPath: String) = { | ||
val fileUri = Utils.resolveURI(file) | ||
Option(fileUri.getScheme).getOrElse("file") match { | ||
case "file" => | ||
val fileName = new File(fileUri.getPath).getName | ||
s"$downloadPath/$fileName" | ||
case _ => | ||
file | ||
} | ||
} | ||
|
||
private def resolveSubmittedFiles(files: Seq[String], downloadPath: String): Seq[String] = { | ||
files.map { file => resolveFile(file, downloadPath) } | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
/* | ||
* Licensed to the Apache Software Foundation (ASF) under one or more | ||
* contributor license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright ownership. | ||
* The ASF licenses this file to You under the Apache License, Version 2.0 | ||
* (the "License"); you may not use this file except in compliance with | ||
* the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
package org.apache.spark.deploy.kubernetes.submit | ||
|
||
import io.fabric8.kubernetes.api.model.{Container, PodBuilder} | ||
|
||
import org.apache.spark.deploy.kubernetes.constants._ | ||
|
||
/** | ||
* Trait that is responsible for providing full file-paths dynamically after | ||
* the filesDownloadPath has been defined. The file-names are then stored in the | ||
* environmental variables in the driver-pod. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given this description, can we use ContainerLocalizedFileResolver? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
*/ | ||
private[spark] trait DriverPodKubernetesFileMounter { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With the V2 file staging server, do we need special code for staging python files? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They're just added to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there's a legitimate question of if we want these files to be deployed in the same location as where the |
||
def addPySparkFiles(primaryFile: String, pySparkFiles: String, | ||
mainContainerName: String, originalPodSpec: PodBuilder) : PodBuilder | ||
} | ||
|
||
private[spark] class DriverPodKubernetesFileMounterImpl() | ||
extends DriverPodKubernetesFileMounter { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the idea of making this generic - we might want to put the submitted jars in here too. It's worth noting that for the next refactor pass. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the above case, I only load file:// into spark-files, but resolve paths and mount for the purpose of the docker image environment variables using this Trait. |
||
override def addPySparkFiles( | ||
primaryFile: String, | ||
pySparkFiles: String, | ||
mainContainerName: String, | ||
originalPodSpec: PodBuilder): PodBuilder = { | ||
|
||
originalPodSpec | ||
.editSpec() | ||
.editMatchingContainer(new ContainerNameEqualityPredicate(mainContainerName)) | ||
.addNewEnv() | ||
.withName(ENV_PYSPARK_PRIMARY) | ||
.withValue(primaryFile) | ||
.endEnv() | ||
.addNewEnv() | ||
.withName(ENV_PYSPARK_FILES) | ||
.withValue(pySparkFiles) | ||
.endEnv() | ||
.endContainer() | ||
.endSpec() | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could factor out
childArgs += args.primaryResource
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
val mainAppResource = args(0)
so it is necessary to point to the mainAppResource in Client.main()