You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by ConeyLiu <gi...@git.apache.org> on 2017/11/06 02:46:44 UTC
[GitHub] spark pull request #19661: [SPARK-22450][Core][Mllib]safely register class f...
GitHub user ConeyLiu opened a pull request:
https://github.com/apache/spark/pull/19661
[SPARK-22450][Core][Mllib]safely register class for mllib
## What changes were proposed in this pull request?
There are still some algorithms based on mllib, such as KMeans. For now, many mllib common class (such as: Vector, DenseVector, SparseVector, Matrix, DenseMatrix, SparseMatrix) are not registered in Kryo. So there are some performance issues for those object serialization or deserialization.
Previously dicussed: https://github.com/apache/spark/pull/19586
## How was this patch tested?
New test case.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ConeyLiu/spark register_vector
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/19661.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #19661
----
commit 317229fabd7b470c8c349a4f12604ea22af0d27f
Author: Xianyang Liu <xi...@intel.com>
Date: 2017-11-06T02:40:55Z
safely register class for mllib
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19661: [SPARK-22450][Core][Mllib]safely register class for mlli...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19661
**[Test build #83676 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83676/testReport)** for PR 19661 at commit [`77f994f`](https://github.com/apache/spark/commit/77f994fcb83af482ddf4e851cb662486db095090).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19661: [SPARK-22450][Core][Mllib]safely register class f...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/19661
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19661: [SPARK-22450][Core][Mllib]safely register class for mlli...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19661
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19661: [SPARK-22450][Core][Mllib]safely register class f...
Posted by jerryshao <gi...@git.apache.org>.
Github user jerryshao commented on a diff in the pull request:
https://github.com/apache/spark/pull/19661#discussion_r149619662
--- Diff: core/src/main/scala/org/apache/spark/serializer/KryoSerializer.scala ---
@@ -178,10 +178,40 @@ class KryoSerializer(conf: SparkConf)
kryo.register(Utils.classForName("scala.collection.immutable.Map$EmptyMap$"))
kryo.register(classOf[ArrayBuffer[Any]])
+ // We can't load those class directly in order to avoid unnecessary jar dependencies.
+ // We load them safely, ignore it if the class not found.
+ Seq("org.apache.spark.mllib.linalg.Vector",
+ "org.apache.spark.mllib.linalg.DenseVector",
+ "org.apache.spark.mllib.linalg.SparseVector",
+ "org.apache.spark.mllib.linalg.Matrix",
+ "org.apache.spark.mllib.linalg.DenseMatrix",
+ "org.apache.spark.mllib.linalg.SparseMatrix",
+ "org.apache.spark.ml.linalg.Vector",
+ "org.apache.spark.ml.linalg.DenseVector",
+ "org.apache.spark.ml.linalg.SparseVector",
+ "org.apache.spark.ml.linalg.Matrix",
+ "org.apache.spark.ml.linalg.DenseMatrix",
+ "org.apache.spark.ml.linalg.SparseMatrix",
+ "org.apache.spark.ml.feature.Instance",
+ "org.apache.spark.ml.feature.OffsetInstance"
+ ).flatMap(safeClassLoader(_)).foreach(kryo.register(_))
--- End diff --
Maybe you can change to
```scala
Seq(xxx).foreach(c => Try(Utils.classForName(c)).foreach(kryo.register))
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19661: [SPARK-22450][Core][Mllib]safely register class for mlli...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19661
**[Test build #83665 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83665/testReport)** for PR 19661 at commit [`d7090bb`](https://github.com/apache/spark/commit/d7090bbf60ea98e9ade9534b78e249b0f25621e4).
* This patch **fails due to an unknown error code, -9**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19661: [SPARK-22450][Core][Mllib]safely register class f...
Posted by ConeyLiu <gi...@git.apache.org>.
Github user ConeyLiu commented on a diff in the pull request:
https://github.com/apache/spark/pull/19661#discussion_r150157116
--- Diff: core/src/main/scala/org/apache/spark/serializer/KryoSerializer.scala ---
@@ -178,6 +179,28 @@ class KryoSerializer(conf: SparkConf)
kryo.register(Utils.classForName("scala.collection.immutable.Map$EmptyMap$"))
kryo.register(classOf[ArrayBuffer[Any]])
+ // We can't load those class directly in order to avoid unnecessary jar dependencies.
+ // We load them safely, ignore it if the class not found.
+ Seq("org.apache.spark.mllib.linalg.Vector",
+ "org.apache.spark.mllib.linalg.DenseVector",
+ "org.apache.spark.mllib.linalg.SparseVector",
+ "org.apache.spark.mllib.linalg.Matrix",
+ "org.apache.spark.mllib.linalg.DenseMatrix",
+ "org.apache.spark.mllib.linalg.SparseMatrix",
+ "org.apache.spark.ml.linalg.Vector",
+ "org.apache.spark.ml.linalg.DenseVector",
+ "org.apache.spark.ml.linalg.SparseVector",
+ "org.apache.spark.ml.linalg.Matrix",
+ "org.apache.spark.ml.linalg.DenseMatrix",
+ "org.apache.spark.ml.linalg.SparseMatrix",
+ "org.apache.spark.ml.feature.Instance",
+ "org.apache.spark.ml.feature.OffsetInstance"
+ ).map(name => Try(Utils.classForName(name))).foreach { t =>
--- End diff --
updated. thanks for the advice.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19661: [SPARK-22450][Core][Mllib]safely register class f...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/19661#discussion_r150172689
--- Diff: core/src/test/scala/org/apache/spark/serializer/KryoSerializerSuite.scala ---
@@ -108,6 +108,27 @@ class KryoSerializerSuite extends SparkFunSuite with SharedSparkContext {
check(Array(Array("1", "2"), Array("1", "2", "3", "4")))
}
+ test("safely register class for mllib/ml") {
+ val conf = new SparkConf(false)
+ val ser = new KryoSerializer(conf)
+
+ Seq("org.apache.spark.mllib.linalg.Vector",
+ "org.apache.spark.mllib.linalg.DenseVector",
+ "org.apache.spark.mllib.linalg.SparseVector",
+ "org.apache.spark.mllib.linalg.Matrix",
+ "org.apache.spark.mllib.linalg.DenseMatrix",
+ "org.apache.spark.mllib.linalg.SparseMatrix",
+ "org.apache.spark.ml.linalg.Vector",
+ "org.apache.spark.ml.linalg.DenseVector",
+ "org.apache.spark.ml.linalg.SparseVector",
+ "org.apache.spark.ml.linalg.Matrix",
+ "org.apache.spark.ml.linalg.DenseMatrix",
+ "org.apache.spark.ml.linalg.SparseMatrix",
+ "org.apache.spark.ml.feature.Instance",
+ "org.apache.spark.ml.feature.OffsetInstance"
+ ).foreach(!Utils.classIsLoadable(_))
--- End diff --
sorry missed this one. Let's remove it, reviewers can easily see there is no extra jar dependency, because no change to the pom.xml
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19661: [SPARK-22450][Core][Mllib]safely register class for mlli...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19661
**[Test build #83665 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83665/testReport)** for PR 19661 at commit [`d7090bb`](https://github.com/apache/spark/commit/d7090bbf60ea98e9ade9534b78e249b0f25621e4).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19661: [SPARK-22450][Core][Mllib]safely register class for mlli...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/19661
thanks, merging to master!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19661: [SPARK-22450][Core][Mllib]safely register class f...
Posted by ConeyLiu <gi...@git.apache.org>.
Github user ConeyLiu commented on a diff in the pull request:
https://github.com/apache/spark/pull/19661#discussion_r150173067
--- Diff: core/src/test/scala/org/apache/spark/serializer/KryoSerializerSuite.scala ---
@@ -108,6 +108,27 @@ class KryoSerializerSuite extends SparkFunSuite with SharedSparkContext {
check(Array(Array("1", "2"), Array("1", "2", "3", "4")))
}
+ test("safely register class for mllib/ml") {
+ val conf = new SparkConf(false)
+ val ser = new KryoSerializer(conf)
+
+ Seq("org.apache.spark.mllib.linalg.Vector",
+ "org.apache.spark.mllib.linalg.DenseVector",
+ "org.apache.spark.mllib.linalg.SparseVector",
+ "org.apache.spark.mllib.linalg.Matrix",
+ "org.apache.spark.mllib.linalg.DenseMatrix",
+ "org.apache.spark.mllib.linalg.SparseMatrix",
+ "org.apache.spark.ml.linalg.Vector",
+ "org.apache.spark.ml.linalg.DenseVector",
+ "org.apache.spark.ml.linalg.SparseVector",
+ "org.apache.spark.ml.linalg.Matrix",
+ "org.apache.spark.ml.linalg.DenseMatrix",
+ "org.apache.spark.ml.linalg.SparseMatrix",
+ "org.apache.spark.ml.feature.Instance",
+ "org.apache.spark.ml.feature.OffsetInstance"
+ ).foreach(!Utils.classIsLoadable(_))
--- End diff --
Ok, remove it.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19661: [SPARK-22450][Core][Mllib]safely register class for mlli...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19661
**[Test build #83642 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83642/testReport)** for PR 19661 at commit [`2eb1b62`](https://github.com/apache/spark/commit/2eb1b62c6fb281f89f05aa8a3c0fcd923ed62cf4).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19661: [SPARK-22450][Core][Mllib]safely register class f...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/19661#discussion_r149927241
--- Diff: core/src/main/scala/org/apache/spark/serializer/KryoSerializer.scala ---
@@ -178,6 +179,28 @@ class KryoSerializer(conf: SparkConf)
kryo.register(Utils.classForName("scala.collection.immutable.Map$EmptyMap$"))
kryo.register(classOf[ArrayBuffer[Any]])
+ // We can't load those class directly in order to avoid unnecessary jar dependencies.
+ // We load them safely, ignore it if the class not found.
+ Seq("org.apache.spark.mllib.linalg.Vector",
+ "org.apache.spark.mllib.linalg.DenseVector",
+ "org.apache.spark.mllib.linalg.SparseVector",
+ "org.apache.spark.mllib.linalg.Matrix",
+ "org.apache.spark.mllib.linalg.DenseMatrix",
+ "org.apache.spark.mllib.linalg.SparseMatrix",
+ "org.apache.spark.ml.linalg.Vector",
+ "org.apache.spark.ml.linalg.DenseVector",
+ "org.apache.spark.ml.linalg.SparseVector",
+ "org.apache.spark.ml.linalg.Matrix",
+ "org.apache.spark.ml.linalg.DenseMatrix",
+ "org.apache.spark.ml.linalg.SparseMatrix",
+ "org.apache.spark.ml.feature.Instance",
+ "org.apache.spark.ml.feature.OffsetInstance"
+ ).map(name => Try(Utils.classForName(name))).foreach { t =>
--- End diff --
a bit curious, can't we do
```
Seq(
...
).foreach { clsName =>
try {
val cls = Utils.classForName(clsName)
kryo.register(cls)
} catch {
case NonFatal(_) => // do nothing
}
}
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19661: [SPARK-22450][Core][Mllib]safely register class f...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/19661#discussion_r149340000
--- Diff: core/src/main/scala/org/apache/spark/serializer/KryoSerializer.scala ---
@@ -178,10 +178,40 @@ class KryoSerializer(conf: SparkConf)
kryo.register(Utils.classForName("scala.collection.immutable.Map$EmptyMap$"))
kryo.register(classOf[ArrayBuffer[Any]])
+ // We can't load those class directly in order to avoid unnecessary jar dependencies.
+ // We load them safely, ignore it if the class not found.
+ Seq("org.apache.spark.mllib.linalg.Vector",
+ "org.apache.spark.mllib.linalg.DenseVector",
+ "org.apache.spark.mllib.linalg.SparseVector",
+ "org.apache.spark.mllib.linalg.Matrix",
+ "org.apache.spark.mllib.linalg.DenseMatrix",
+ "org.apache.spark.mllib.linalg.SparseMatrix",
+ "org.apache.spark.ml.linalg.Vector",
+ "org.apache.spark.ml.linalg.DenseVector",
+ "org.apache.spark.ml.linalg.SparseVector",
+ "org.apache.spark.ml.linalg.Matrix",
+ "org.apache.spark.ml.linalg.DenseMatrix",
+ "org.apache.spark.ml.linalg.SparseMatrix",
+ "org.apache.spark.ml.feature.Instance",
+ "org.apache.spark.ml.feature.OffsetInstance"
+ ).flatMap(safeClassLoader(_)).foreach(kryo.register(_))
--- End diff --
please inline this `safeClassLoader`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19661: [SPARK-22450][Core][Mllib]safely register class f...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/19661#discussion_r149012452
--- Diff: core/src/main/scala/org/apache/spark/serializer/KryoSerializer.scala ---
@@ -178,10 +178,31 @@ class KryoSerializer(conf: SparkConf)
kryo.register(Utils.classForName("scala.collection.immutable.Map$EmptyMap$"))
kryo.register(classOf[ArrayBuffer[Any]])
+ // We can't load those class directly in order to avoid unnecessary jar dependencies.
+ // We load them safely, ignore it if the class not found.
+ safeClassLoader("org.apache.spark.mllib.linalg.Vector").foreach(kryo.register(_))
--- End diff --
I don't have a strong objection to this, it's a little hacky though. cc @srowen
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19661: [SPARK-22450][Core][Mllib]safely register class f...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/19661#discussion_r149016829
--- Diff: core/src/main/scala/org/apache/spark/serializer/KryoSerializer.scala ---
@@ -178,10 +178,31 @@ class KryoSerializer(conf: SparkConf)
kryo.register(Utils.classForName("scala.collection.immutable.Map$EmptyMap$"))
kryo.register(classOf[ArrayBuffer[Any]])
+ // We can't load those class directly in order to avoid unnecessary jar dependencies.
+ // We load them safely, ignore it if the class not found.
+ safeClassLoader("org.apache.spark.mllib.linalg.Vector").foreach(kryo.register(_))
--- End diff --
It is a little hacky, but not crazy. But rather than this, why not just write here something like...
```
Seq("org.apache...", "...").foreach { className =>
try {
kryo.register(Utils.classForName(className))
} catch {
case _: ClassNotFoundException => // do nothing
}
}
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19661: [SPARK-22450][Core][Mllib]safely register class for mlli...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19661
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/83642/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19661: [SPARK-22450][Core][Mllib]safely register class f...
Posted by jerryshao <gi...@git.apache.org>.
Github user jerryshao commented on a diff in the pull request:
https://github.com/apache/spark/pull/19661#discussion_r150171482
--- Diff: core/src/test/scala/org/apache/spark/serializer/KryoSerializerSuite.scala ---
@@ -108,6 +108,27 @@ class KryoSerializerSuite extends SparkFunSuite with SharedSparkContext {
check(Array(Array("1", "2"), Array("1", "2", "3", "4")))
}
+ test("safely register class for mllib/ml") {
+ val conf = new SparkConf(false)
+ val ser = new KryoSerializer(conf)
+
+ Seq("org.apache.spark.mllib.linalg.Vector",
+ "org.apache.spark.mllib.linalg.DenseVector",
+ "org.apache.spark.mllib.linalg.SparseVector",
+ "org.apache.spark.mllib.linalg.Matrix",
+ "org.apache.spark.mllib.linalg.DenseMatrix",
+ "org.apache.spark.mllib.linalg.SparseMatrix",
+ "org.apache.spark.ml.linalg.Vector",
+ "org.apache.spark.ml.linalg.DenseVector",
+ "org.apache.spark.ml.linalg.SparseVector",
+ "org.apache.spark.ml.linalg.Matrix",
+ "org.apache.spark.ml.linalg.DenseMatrix",
+ "org.apache.spark.ml.linalg.SparseMatrix",
+ "org.apache.spark.ml.feature.Instance",
+ "org.apache.spark.ml.feature.OffsetInstance"
+ ).foreach(!Utils.classIsLoadable(_))
--- End diff --
This UT looks doesn't actually reflect your purpose above, seems this always be passed. Also `conf` and `ser` above seems never used here.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19661: [SPARK-22450][Core][Mllib]safely register class for mlli...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/19661
ok to test
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19661: [SPARK-22450][Core][Mllib]safely register class for mlli...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19661
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19661: [SPARK-22450][Core][Mllib]safely register class for mlli...
Posted by WeichenXu123 <gi...@git.apache.org>.
Github user WeichenXu123 commented on the issue:
https://github.com/apache/spark/pull/19661
And I don't know whether these class dependency injection into spark-core lib is reasonable ...
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19661: [SPARK-22450][Core][Mllib]safely register class for mlli...
Posted by ConeyLiu <gi...@git.apache.org>.
Github user ConeyLiu commented on the issue:
https://github.com/apache/spark/pull/19661
Thanks for reviewing. The code is updated.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19661: [SPARK-22450][Core][Mllib]safely register class for mlli...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19661
**[Test build #83676 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83676/testReport)** for PR 19661 at commit [`77f994f`](https://github.com/apache/spark/commit/77f994fcb83af482ddf4e851cb662486db095090).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19661: [SPARK-22450][Core][Mllib]safely register class for mlli...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19661
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/83676/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19661: [SPARK-22450][Core][Mllib]safely register class for mlli...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19661
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19661: [SPARK-22450][Core][Mllib]safely register class for mlli...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19661
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19661: [SPARK-22450][Core][Mllib]safely register class f...
Posted by ConeyLiu <gi...@git.apache.org>.
Github user ConeyLiu commented on a diff in the pull request:
https://github.com/apache/spark/pull/19661#discussion_r149553694
--- Diff: core/src/main/scala/org/apache/spark/serializer/KryoSerializer.scala ---
@@ -178,10 +178,40 @@ class KryoSerializer(conf: SparkConf)
kryo.register(Utils.classForName("scala.collection.immutable.Map$EmptyMap$"))
kryo.register(classOf[ArrayBuffer[Any]])
+ // We can't load those class directly in order to avoid unnecessary jar dependencies.
+ // We load them safely, ignore it if the class not found.
+ Seq("org.apache.spark.mllib.linalg.Vector",
+ "org.apache.spark.mllib.linalg.DenseVector",
+ "org.apache.spark.mllib.linalg.SparseVector",
+ "org.apache.spark.mllib.linalg.Matrix",
+ "org.apache.spark.mllib.linalg.DenseMatrix",
+ "org.apache.spark.mllib.linalg.SparseMatrix",
+ "org.apache.spark.ml.linalg.Vector",
+ "org.apache.spark.ml.linalg.DenseVector",
+ "org.apache.spark.ml.linalg.SparseVector",
+ "org.apache.spark.ml.linalg.Matrix",
+ "org.apache.spark.ml.linalg.DenseMatrix",
+ "org.apache.spark.ml.linalg.SparseMatrix",
+ "org.apache.spark.ml.feature.Instance",
+ "org.apache.spark.ml.feature.OffsetInstance"
+ ).flatMap(safeClassLoader(_)).foreach(kryo.register(_))
--- End diff --
Hi @cloud-fan , I tried the following codeļ¼
```scala
flatMap(cn => Try{Utils.classForName(cn)}.toOption).foreach(kryo.register(_))
```
and
```scala
flatMap{ cn =>
try {
val clazz = Utils.classForName(cn)
Some(clazz)
} catch {
case _: ClassNotFoundException => None
}
}.foreach(kryo.register(_))
```
Both reported the same errors:
```
Error:(198, 18) type mismatch;
found : String => Iterable[Class[_$2]]( forSome { type _$2 })
required: String => scala.collection.GenTraversableOnce[B]
).flatMap{cn => Option(Utils.classForName(cn))}.foreach(kryo.register(_))
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19661: [SPARK-22450][Core][Mllib]safely register class f...
Posted by ConeyLiu <gi...@git.apache.org>.
Github user ConeyLiu commented on a diff in the pull request:
https://github.com/apache/spark/pull/19661#discussion_r149846210
--- Diff: core/src/main/scala/org/apache/spark/serializer/KryoSerializer.scala ---
@@ -178,10 +178,40 @@ class KryoSerializer(conf: SparkConf)
kryo.register(Utils.classForName("scala.collection.immutable.Map$EmptyMap$"))
kryo.register(classOf[ArrayBuffer[Any]])
+ // We can't load those class directly in order to avoid unnecessary jar dependencies.
+ // We load them safely, ignore it if the class not found.
+ Seq("org.apache.spark.mllib.linalg.Vector",
+ "org.apache.spark.mllib.linalg.DenseVector",
+ "org.apache.spark.mllib.linalg.SparseVector",
+ "org.apache.spark.mllib.linalg.Matrix",
+ "org.apache.spark.mllib.linalg.DenseMatrix",
+ "org.apache.spark.mllib.linalg.SparseMatrix",
+ "org.apache.spark.ml.linalg.Vector",
+ "org.apache.spark.ml.linalg.DenseVector",
+ "org.apache.spark.ml.linalg.SparseVector",
+ "org.apache.spark.ml.linalg.Matrix",
+ "org.apache.spark.ml.linalg.DenseMatrix",
+ "org.apache.spark.ml.linalg.SparseMatrix",
+ "org.apache.spark.ml.feature.Instance",
+ "org.apache.spark.ml.feature.OffsetInstance"
+ ).flatMap(safeClassLoader(_)).foreach(kryo.register(_))
--- End diff --
thanks a lot, I updated the code.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19661: [SPARK-22450][Core][Mllib]safely register class for mlli...
Posted by ConeyLiu <gi...@git.apache.org>.
Github user ConeyLiu commented on the issue:
https://github.com/apache/spark/pull/19661
thanks everyone.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19661: [SPARK-22450][Core][Mllib]safely register class for mlli...
Posted by ConeyLiu <gi...@git.apache.org>.
Github user ConeyLiu commented on the issue:
https://github.com/apache/spark/pull/19661
>So why don't you include some classes such as org.apache.spark.ml.feature.Instance ?
I'm not family with those algorithm, I can add them such as `org.apache.spark.ml.feature.Instance` .
If this method is not reasonable, we maybe just reminder user to register them in the doc.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19661: [SPARK-22450][Core][Mllib]safely register class for mlli...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19661
**[Test build #83642 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83642/testReport)** for PR 19661 at commit [`2eb1b62`](https://github.com/apache/spark/commit/2eb1b62c6fb281f89f05aa8a3c0fcd923ed62cf4).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19661: [SPARK-22450][Core][Mllib]safely register class for mlli...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19661
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/83665/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19661: [SPARK-22450][Core][Mllib]safely register class for mlli...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/19661
LGTM
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19661: [SPARK-22450][Core][Mllib]safely register class for mlli...
Posted by ConeyLiu <gi...@git.apache.org>.
Github user ConeyLiu commented on the issue:
https://github.com/apache/spark/pull/19661
#19586
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19661: [SPARK-22450][Core][Mllib]safely register class f...
Posted by ConeyLiu <gi...@git.apache.org>.
Github user ConeyLiu commented on a diff in the pull request:
https://github.com/apache/spark/pull/19661#discussion_r150172493
--- Diff: core/src/test/scala/org/apache/spark/serializer/KryoSerializerSuite.scala ---
@@ -108,6 +108,27 @@ class KryoSerializerSuite extends SparkFunSuite with SharedSparkContext {
check(Array(Array("1", "2"), Array("1", "2", "3", "4")))
}
+ test("safely register class for mllib/ml") {
+ val conf = new SparkConf(false)
+ val ser = new KryoSerializer(conf)
+
+ Seq("org.apache.spark.mllib.linalg.Vector",
+ "org.apache.spark.mllib.linalg.DenseVector",
+ "org.apache.spark.mllib.linalg.SparseVector",
+ "org.apache.spark.mllib.linalg.Matrix",
+ "org.apache.spark.mllib.linalg.DenseMatrix",
+ "org.apache.spark.mllib.linalg.SparseMatrix",
+ "org.apache.spark.ml.linalg.Vector",
+ "org.apache.spark.ml.linalg.DenseVector",
+ "org.apache.spark.ml.linalg.SparseVector",
+ "org.apache.spark.ml.linalg.Matrix",
+ "org.apache.spark.ml.linalg.DenseMatrix",
+ "org.apache.spark.ml.linalg.SparseMatrix",
+ "org.apache.spark.ml.feature.Instance",
+ "org.apache.spark.ml.feature.OffsetInstance"
+ ).foreach(!Utils.classIsLoadable(_))
--- End diff --
This just want to indicate we didn't introduce extra jar dependency. I can delete it if it's unnecessary.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19661: [SPARK-22450][Core][Mllib]safely register class for mlli...
Posted by WeichenXu123 <gi...@git.apache.org>.
Github user WeichenXu123 commented on the issue:
https://github.com/apache/spark/pull/19661
So why do you include the class such as `org.apache.spark.ml.feature.Instance`.
You can look into a lot of algos, in `ml` package (not `mllib`), still use something like `RDD[Instance]` and cache them, (these algos only interface use dataframe). such as LinearRegression, LogisticRegression and so on.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org