Skip to content
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-27805][PYTHON] Propagate SparkExceptions during toPandas with arrow enabled #24677

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 13 additions & 7 deletions python/pyspark/serializers.py
Expand Up @@ -206,13 +206,19 @@ def load_stream(self, stream):
for batch in self.serializer.load_stream(stream):
yield batch

# load the batch order indices
num = read_int(stream)
batch_order = []
for i in xrange(num):
index = read_int(stream)
batch_order.append(index)
yield batch_order
# check success
success = read_bool(stream)
if success:
# load the batch order indices
num = read_int(stream)
batch_order = []
for i in xrange(num):
index = read_int(stream)
batch_order.append(index)
yield batch_order
else:
error_msg = UTF8Deserializer().loads(stream)
raise RuntimeError("An error occurred while collecting: {}".format(error_msg))
BryanCutler marked this conversation as resolved.
Show resolved Hide resolved

def __repr__(self):
return "ArrowCollectSerializer(%s)" % self.serializer
Expand Down
12 changes: 12 additions & 0 deletions python/pyspark/sql/tests/test_arrow.py
Expand Up @@ -191,6 +191,18 @@ def test_no_partition_frame(self):
self.assertEqual(pdf.columns[0], "field1")
self.assertTrue(pdf.empty)

def test_propagates_spark_exception(self):
df = self.spark.range(3).toDF("i")
from pyspark.sql.functions import udf
BryanCutler marked this conversation as resolved.
Show resolved Hide resolved

def raise_exception():
raise Exception("My error")
exception_udf = udf(raise_exception, IntegerType())
df = df.withColumn("error", exception_udf())
with QuietTest(self.sc):
with self.assertRaisesRegexp(RuntimeError, 'My error'):
df.toPandas()

def _createDataFrame_toggle(self, pdf, schema=None):
with self.sql_conf({"spark.sql.execution.arrow.enabled": False}):
df_no_arrow = self.spark.createDataFrame(pdf, schema=schema)
Expand Down
41 changes: 28 additions & 13 deletions sql/core/src/main/scala/org/apache/spark/sql/Dataset.scala
Expand Up @@ -26,7 +26,7 @@ import scala.util.control.NonFatal

import org.apache.commons.lang3.StringUtils

import org.apache.spark.TaskContext
import org.apache.spark.{SparkException, TaskContext}
import org.apache.spark.annotation.{DeveloperApi, Evolving, Experimental, Stable, Unstable}
import org.apache.spark.api.java.JavaRDD
import org.apache.spark.api.java.function._
Expand Down Expand Up @@ -3313,20 +3313,35 @@ class Dataset[T] private[sql](
}
}

val arrowBatchRdd = toArrowBatchRdd(plan)
sparkSession.sparkContext.runJob(
arrowBatchRdd,
(it: Iterator[Array[Byte]]) => it.toArray,
handlePartitionBatches)
var sparkException: Option[SparkException] = Option.empty
BryanCutler marked this conversation as resolved.
Show resolved Hide resolved
try {
val arrowBatchRdd = toArrowBatchRdd(plan)
sparkSession.sparkContext.runJob(
arrowBatchRdd,
(it: Iterator[Array[Byte]]) => it.toArray,
handlePartitionBatches)
} catch {
case e: SparkException =>
sparkException = Option.apply(e)
BryanCutler marked this conversation as resolved.
Show resolved Hide resolved
}

// After processing all partitions, end the stream and write batch order indices
// After processing all partitions, end the batch stream
batchWriter.end()
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if this and the code below should be in a finally block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we put it into a finally block but only catch SparkException then that would be wrong: If a different exception gets thrown then we would go into case None, end the stream as if nothing happened and only get partial, incorrect data on the python side.
If we want to put this into a finally block then we should catch all exceptions but I figured I'd do the same as in https://github.com/apache/spark/pull/24070/files#r279589039

It should be fine as is, if any exception that isn't a SparkException gets thrown then we will never reach this code. Instead the OutputStream just gets closed and we get an EofError on the python side (like we do right now for all Exceptions).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any more thoughts on this @BryanCutler ?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think this is fine

out.writeInt(batchOrder.length)
// Sort by (index of partition, batch index in that partition) tuple to get the
// overall_batch_index from 0 to N-1 batches, which can be used to put the
// transferred batches in the correct order
batchOrder.zipWithIndex.sortBy(_._1).foreach { case (_, overallBatchIndex) =>
out.writeInt(overallBatchIndex)
sparkException match {
case Some(exception) =>
// Signal failure and write error message
out.writeBoolean(false)
BryanCutler marked this conversation as resolved.
Show resolved Hide resolved
PythonRDD.writeUTF(exception.getMessage, out)
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for late response, @BryanCutler

Yea, I had the same question #24677 (comment). Thanks for details at #24677 (comment). It would have been better if those are commented since at least two committers raised the same questions :-).

@HyukjinKwon do you think would it make sense to patch branch-2.4 with a manual fix?

Yea, I don't mind backporting it (don't strongly feel we should do too).

case None =>
// Signal success and write batch order indices
out.writeBoolean(true)
out.writeInt(batchOrder.length)
// Sort by (index of partition, batch index in that partition) tuple to get the
// overall_batch_index from 0 to N-1 batches, which can be used to put the
// transferred batches in the correct order
batchOrder.zipWithIndex.sortBy(_._1).foreach { case (_, overallBatchIndex) =>
out.writeInt(overallBatchIndex)
}
}
}
}
Expand Down