-
Notifications
You must be signed in to change notification settings - Fork 28.1k
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.
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
[SPARK-32160][CORE][PYSPARK] Disallow to create SparkContext in executors. #28986
Changes from 2 commits
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 |
---|---|---|
|
@@ -83,6 +83,9 @@ class SparkContext(config: SparkConf) extends Logging { | |
// The call site where this SparkContext was constructed. | ||
private val creationSite: CallSite = Utils.getCallSite() | ||
|
||
// In order to prevent SparkContext from being created in executors. | ||
SparkContext.assertOnDriver() | ||
|
||
// In order to prevent multiple SparkContexts from being active at the same time, mark this | ||
// context as having started construction. | ||
// NOTE: this must be placed at the beginning of the SparkContext constructor. | ||
|
@@ -2554,6 +2557,19 @@ object SparkContext extends Logging { | |
} | ||
} | ||
|
||
/** | ||
* Called to ensure that SparkContext is created or accessed only on the Driver. | ||
* | ||
* Throws an exception if a SparkContext is about to be created in executors. | ||
*/ | ||
private def assertOnDriver(): Unit = { | ||
if (TaskContext.get != null) { | ||
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. @ueshin could you submit a follow-up PR to add a conf? In Spark 3.0, turn it off by default; in Spark 3.1 turn it on by default? Also add it to the migration guide? 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. Does this actually affect any legitimate use case that would otherwise work? this should be more of a fail-fast for things that will already fail 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. This is a user error. However, the users may use a Spark library in executors but the library calls SparkContext.getOrCreate. We should still let their workloads work if it worked in the previous releases. 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 see, but would that ever succeed, using the SparkContext that was created? I'm trying to work out what library would do this and not fail. Is it, perhaps, some boilerplate initialization code that gets executed on driver and executor, and it makes a SparkContext that is never actually used on the executor, but now it fails fast? I get it, if that's the use case. A release note and/or hidden config to disable it might be an OK workaround. Alternatively if arguing that this is not-uncommon, maybe we just don't do this at all, and revert it 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. BTW, had been discussing this offline with @ueshin and @HyukjinKwon - one use case that previously (and admittedly unfortunately) relied on the ability to create I'll dig to see if there's a way to keep the MLflow use case working with this change (e.g. pyspark.ml models may ultimately perform inference via spark UDF, so maybe we can somehow extract the underlying UDF from the model and return that from 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 submitted a PR to add configs #29278. 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 it's a bit odds to have two separate configuration for Scala and Python. I don't think we will happen to allow this in PySpark specifically in the future even if it worked. I think we will discourage this behaviour anyway whether it is in Scala or Python, and deprecate/remove both configurations together eventually. In which case would we disable it in Scala side but enable it in Python side? 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.
Like how MLflow uses it today. It's pretty hard to make it work correctly in Scala side. But it's working in Python today. 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 see but I guess we should discourage/deprecate/remove the behaviour in the end - am I correct? It's a bit weird to have two configuration to control the same behaviour (to end users). And I think it's a bit unlikely when it should be disabled in Scala but enabled in Python specifically. We could just enable it in both sides. 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 pushed a commit to use a single config. |
||
// we're accessing it during task execution, fail. | ||
throw new IllegalStateException( | ||
"SparkContext should only be created and accessed on the driver.") | ||
} | ||
} | ||
|
||
/** | ||
* This function may be used to get or instantiate a SparkContext and register it as a | ||
* singleton object. Because we can only have one active SparkContext per JVM, | ||
|
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.
Have you tested this under local mode?
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.
Under local mode:
before this patch:
Although the exception is different, it fails anyway.
I think the new error message is more reasonable.