You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by yanboliang <gi...@git.apache.org> on 2017/07/12 09:25:39 UTC
[GitHub] spark pull request #18610: [SPARK-21386] ML LinearRegression supports warm s...
GitHub user yanboliang opened a pull request:
https://github.com/apache/spark/pull/18610
[SPARK-21386] ML LinearRegression supports warm start from user provided initial model.
## What changes were proposed in this pull request?
Allow users to set initial model when training ```LinearRegressionModel```. This is the first step to make ML supports warm-start, we can distribute tasks for other algorithms after this get merged.
## How was this patch tested?
Added unit test.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/yanboliang/spark spark-21386
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/18610.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 #18610
----
commit ae8f32b2fe8b774f50ffde2931e0727eca1ea11b
Author: Yanbo Liang <yb...@gmail.com>
Date: 2017-07-12T08:45:32Z
ML LinearRegression supports warm start by user provided initial model.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18610: [SPARK-21386] ML LinearRegression supports warm start fr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18610
**[Test build #83120 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83120/testReport)** for PR 18610 at commit [`e49d452`](https://github.com/apache/spark/commit/e49d45271c923eeeba3f3bbbd79dc5cbf0c91ab1).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18610: [SPARK-21386] ML LinearRegression supports warm s...
Posted by WeichenXu123 <gi...@git.apache.org>.
Github user WeichenXu123 commented on a diff in the pull request:
https://github.com/apache/spark/pull/18610#discussion_r130015435
--- Diff: mllib/src/main/scala/org/apache/spark/ml/util/ReadWrite.scala ---
@@ -309,6 +313,23 @@ private[ml] object DefaultParamsWriter {
val metadataJson: String = compact(render(metadata))
metadataJson
}
+
+ /**
+ * Save estimator's `initialModel` to corresponding path.
+ */
+ def saveInitialModel[T <: HasInitialModel[_ <: MLWritable with Params]](
+ instance: T, path: String): Unit = {
+ if (instance.isDefined(instance.initialModel)) {
+ val initialModelPath = new Path(path, "initialModel").toString
+ val initialModel = instance.getOrDefault(instance.initialModel)
+ // When saving, only keep the direct initialModel by eliminating possible initialModels of the
+ // direct initialModel, to avoid unnecessary deep recursion of initialModel.
+ if (initialModel.hasParam("initialModel")) {
+ initialModel.clear(initialModel.getParam("initialModel"))
+ }
+ initialModel.save(initialModelPath)
--- End diff --
Fair enough.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18610: [SPARK-21386] ML LinearRegression supports warm start fr...
Posted by zhengruifeng <gi...@git.apache.org>.
Github user zhengruifeng commented on the issue:
https://github.com/apache/spark/pull/18610
@yanboliang Agree. I think if there is some prarms which control the "shape" of model coefficients, than they should be override if we use inital model. Like `k` in KMeans, GMM, LDA, other params should be let alone
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18610: [SPARK-21386] ML LinearRegression supports warm start fr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18610
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18610: [SPARK-21386] ML LinearRegression supports warm start fr...
Posted by JohnHBrock <gi...@git.apache.org>.
Github user JohnHBrock commented on the issue:
https://github.com/apache/spark/pull/18610
Is there anymore work to do before this can get merged?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18610: [SPARK-21386] ML LinearRegression supports warm start fr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18610
**[Test build #79553 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/79553/testReport)** for PR 18610 at commit [`ae8f32b`](https://github.com/apache/spark/commit/ae8f32b2fe8b774f50ffde2931e0727eca1ea11b).
* This patch **fails MiMa tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18610: [SPARK-21386] ML LinearRegression supports warm start fr...
Posted by yanboliang <gi...@git.apache.org>.
Github user yanboliang commented on the issue:
https://github.com/apache/spark/pull/18610
@MLnick Thanks for your comments. I think the implementation of this PR followed the discussion around the ```KMeans``` initial model in #11119. In that PR, we didn't use ```initialModel``` to override all param settings(e.g. ```maxIter```, ```tol``` etc), but we override some params (e.g. ```k```). But in this PR, we don't have params like ```k```, so it's not necessary to override any params. This is the reason that I start the general warm start work from ```LinearRegression``` which is more clean. And I don't think we should to use ```initialModel``` to override params like ```tolerance``` and ```iterations```. Why we need to use the tolerance of the ```initialModel```? I just want my train start from an coefficients and I will specify new tolerance to control its convergence.
BTW, I also referred other libraries(e.g. scikit-learn), the semantic of this implementation is consistent with other popular libraries. Still, I'm very happy to get your comments.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18610: [SPARK-21386] ML LinearRegression supports warm s...
Posted by hhbyyh <gi...@git.apache.org>.
Github user hhbyyh commented on a diff in the pull request:
https://github.com/apache/spark/pull/18610#discussion_r135418170
--- Diff: mllib/src/main/scala/org/apache/spark/ml/regression/LinearRegression.scala ---
@@ -226,6 +246,12 @@ class LinearRegression @Since("1.3.0") (@Since("1.3.0") override val uid: String
if (($(solver) == Auto &&
numFeatures <= WeightedLeastSquares.MAX_NUM_FEATURES) || $(solver) == Normal) {
+
+ if (isSet(initialModel)) {
+ logWarning("Initial model will be ignored if fitting by normal solver. " +
--- End diff --
Since initial model is a pretty important parameter. By setting the initial model, user would expect it to work and they may neglect the warning in the overwhelming Spark logs.
Maybe we can move the parameter check to `transformSchema` and throws an exception.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18610: [SPARK-21386] ML LinearRegression supports warm start fr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18610
**[Test build #79562 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/79562/testReport)** for PR 18610 at commit [`e49d452`](https://github.com/apache/spark/commit/e49d45271c923eeeba3f3bbbd79dc5cbf0c91ab1).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18610: [SPARK-21386] ML LinearRegression supports warm start fr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18610
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/79562/
Test FAILed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18610: [SPARK-21386] ML LinearRegression supports warm s...
Posted by sethah <gi...@git.apache.org>.
Github user sethah commented on a diff in the pull request:
https://github.com/apache/spark/pull/18610#discussion_r147532256
--- Diff: mllib/src/main/scala/org/apache/spark/ml/regression/LinearRegression.scala ---
@@ -226,6 +246,12 @@ class LinearRegression @Since("1.3.0") (@Since("1.3.0") override val uid: String
if (($(solver) == Auto &&
numFeatures <= WeightedLeastSquares.MAX_NUM_FEATURES) || $(solver) == Normal) {
+
+ if (isSet(initialModel)) {
+ logWarning("Initial model will be ignored if fitting by normal solver. " +
--- End diff --
I disagree. Normal solver provides an exact solution, so it is harmless to ignore the initial model. I don't see a reason to do anything more than log a warning.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18610: [SPARK-21386] ML LinearRegression supports warm start fr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18610
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 #18610: [SPARK-21386] ML LinearRegression supports warm start fr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18610
Merged build finished. Test FAILed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18610: [SPARK-21386] ML LinearRegression supports warm start fr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18610
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/79553/
Test FAILed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18610: [SPARK-21386] ML LinearRegression supports warm start fr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18610
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/79554/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18610: [SPARK-21386] ML LinearRegression supports warm start fr...
Posted by hhbyyh <gi...@git.apache.org>.
Github user hhbyyh commented on the issue:
https://github.com/apache/spark/pull/18610
Thanks for the reply. Since there's already an agreement, I will hold my suggestion on initialModel data type.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18610: [SPARK-21386] ML LinearRegression supports warm start fr...
Posted by hhbyyh <gi...@git.apache.org>.
Github user hhbyyh commented on the issue:
https://github.com/apache/spark/pull/18610
Just to confirm, so we have agreed that the initialModel should be of type [T <: Model[T]] rather than a String type (path to the saved model)? Sorry I didn't find the related discussion.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18610: [SPARK-21386] ML LinearRegression supports warm s...
Posted by yanboliang <gi...@git.apache.org>.
Github user yanboliang commented on a diff in the pull request:
https://github.com/apache/spark/pull/18610#discussion_r129324027
--- Diff: mllib/src/main/scala/org/apache/spark/ml/regression/LinearRegression.scala ---
@@ -365,7 +398,11 @@ class LinearRegression @Since("1.3.0") (@Since("1.3.0") override val uid: String
new BreezeOWLQN[Int, BDV[Double]]($(maxIter), 10, effectiveL1RegFun, $(tol))
}
- val initialCoefficients = Vectors.zeros(numFeatures)
+ val initialCoefficients = if (isSet(initialModel)) {
+ $(initialModel).coefficients
--- End diff --
Here we only use ```coefficients```, not use ```intercept```. This is because the ```intercept``` was computed using closed form after the coefficients are converged.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18610: [SPARK-21386] ML LinearRegression supports warm start fr...
Posted by yanboliang <gi...@git.apache.org>.
Github user yanboliang commented on the issue:
https://github.com/apache/spark/pull/18610
cc @dbtsai @sethah @MLnick @hhbyyh
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18610: [SPARK-21386] ML LinearRegression supports warm s...
Posted by yanboliang <gi...@git.apache.org>.
Github user yanboliang commented on a diff in the pull request:
https://github.com/apache/spark/pull/18610#discussion_r129997548
--- Diff: mllib/src/main/scala/org/apache/spark/ml/util/ReadWrite.scala ---
@@ -309,6 +313,23 @@ private[ml] object DefaultParamsWriter {
val metadataJson: String = compact(render(metadata))
metadataJson
}
+
+ /**
+ * Save estimator's `initialModel` to corresponding path.
+ */
+ def saveInitialModel[T <: HasInitialModel[_ <: MLWritable with Params]](
+ instance: T, path: String): Unit = {
+ if (instance.isDefined(instance.initialModel)) {
+ val initialModelPath = new Path(path, "initialModel").toString
+ val initialModel = instance.getOrDefault(instance.initialModel)
+ // When saving, only keep the direct initialModel by eliminating possible initialModels of the
+ // direct initialModel, to avoid unnecessary deep recursion of initialModel.
+ if (initialModel.hasParam("initialModel")) {
+ initialModel.clear(initialModel.getParam("initialModel"))
+ }
+ initialModel.save(initialModelPath)
--- End diff --
Actually we did in the later way, ```initialModel``` is only param for Estimator, not for Model. Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18610: [SPARK-21386] ML LinearRegression supports warm start fr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18610
**[Test build #79567 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/79567/testReport)** for PR 18610 at commit [`e49d452`](https://github.com/apache/spark/commit/e49d45271c923eeeba3f3bbbd79dc5cbf0c91ab1).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18610: [SPARK-21386] ML LinearRegression supports warm start fr...
Posted by yanboliang <gi...@git.apache.org>.
Github user yanboliang commented on the issue:
https://github.com/apache/spark/pull/18610
This failure is irrelevant. Jenkins, retest this please.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18610: [SPARK-21386] ML LinearRegression supports warm s...
Posted by yanboliang <gi...@git.apache.org>.
Github user yanboliang commented on a diff in the pull request:
https://github.com/apache/spark/pull/18610#discussion_r126911229
--- Diff: mllib/src/test/scala/org/apache/spark/ml/util/DefaultReadWriteTest.scala ---
@@ -113,7 +115,14 @@ trait DefaultReadWriteTest extends TempDirectory { self: Suite =>
val estimator2 = testDefaultReadWrite(estimator)
testEstimatorParams.foreach { case (p, v) =>
val param = estimator.getParam(p)
- assert(estimator.get(param).get === estimator2.get(param).get)
+ if (param.name == "initialModel") {
+ // Estimator's `initialModel` has same type as the model produced by this estimator.
--- End diff --
This is an assumption that ```initialModel``` must be the same type of the model that the estimator produced. We should enforce this assumption by making the trait as ```HasInitialModel[T <: Model[T]]``` or add new argument ```checkInitialModelData``` for ```testEstimatorAndModelReadWrite```. I'm open to hear others' thoughts and update here after collecting feedback. Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18610: [SPARK-21386] ML LinearRegression supports warm start fr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18610
**[Test build #79567 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/79567/testReport)** for PR 18610 at commit [`e49d452`](https://github.com/apache/spark/commit/e49d45271c923eeeba3f3bbbd79dc5cbf0c91ab1).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18610: [SPARK-21386] ML LinearRegression supports warm start fr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18610
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/79567/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18610: [SPARK-21386] ML LinearRegression supports warm start fr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18610
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18610: [SPARK-21386] ML LinearRegression supports warm s...
Posted by WeichenXu123 <gi...@git.apache.org>.
Github user WeichenXu123 commented on a diff in the pull request:
https://github.com/apache/spark/pull/18610#discussion_r129413407
--- Diff: mllib/src/main/scala/org/apache/spark/ml/util/ReadWrite.scala ---
@@ -309,6 +313,23 @@ private[ml] object DefaultParamsWriter {
val metadataJson: String = compact(render(metadata))
metadataJson
}
+
+ /**
+ * Save estimator's `initialModel` to corresponding path.
+ */
+ def saveInitialModel[T <: HasInitialModel[_ <: MLWritable with Params]](
+ instance: T, path: String): Unit = {
+ if (instance.isDefined(instance.initialModel)) {
+ val initialModelPath = new Path(path, "initialModel").toString
+ val initialModel = instance.getOrDefault(instance.initialModel)
+ // When saving, only keep the direct initialModel by eliminating possible initialModels of the
+ // direct initialModel, to avoid unnecessary deep recursion of initialModel.
+ if (initialModel.hasParam("initialModel")) {
+ initialModel.clear(initialModel.getParam("initialModel"))
+ }
+ initialModel.save(initialModelPath)
--- End diff --
In order to avoid recursion, here modify the model to cut down the recursion, but may cause the problem that influence the model which is maybe used in other place.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18610: [SPARK-21386] ML LinearRegression supports warm start fr...
Posted by yanboliang <gi...@git.apache.org>.
Github user yanboliang commented on the issue:
https://github.com/apache/spark/pull/18610
@hhbyyh We got agreement that the initialModel should be of type ```[T <: Model[T]]``` at #11119. I understand the scenario you mentioned, however, I think they are different scenarios:
1, If we set ```initialModel``` as a string path, we should guarantee there is correct model stored in the path, but we can't guarantee it only to refer the string path.
2, I agree that the scenario you mentioned above is really useful, but it think the requirement is beyond initial model. The following code snippet can meet that requirement under my framework:
```
val initialModel = LinearRegressionModel.load("path_lr_20iteration_reg0.001.model")
val lr = new LinearRegression().setInitialModel(initialModel)
val model = lr.fit(dataset)
```
3, If users have a model in memory, they want to use this model as the initial model to train another one, they need to save the model firstly and then set the corresponding path as the initial model path.
4, For your comments about serializing the initial model, I agree with you. I'm ok not to save ```initialModel``` when saving estimator.
What do you think of it? Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18610: [SPARK-21386] ML LinearRegression supports warm start fr...
Posted by zhengruifeng <gi...@git.apache.org>.
Github user zhengruifeng commented on the issue:
https://github.com/apache/spark/pull/18610
LGTM, this is really a great feature
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18610: [SPARK-21386] ML LinearRegression supports warm start fr...
Posted by MLnick <gi...@git.apache.org>.
Github user MLnick commented on the issue:
https://github.com/apache/spark/pull/18610
There was a _lot_ of discussion around the `KMeans` initial model. But here we seem to be just ignoring certain issues. Such as, does the `initialModel` override the param settings (e.g. reg param, tolerance, iterations etc). Here it is just the coefficients that are used.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18610: [SPARK-21386] ML LinearRegression supports warm start fr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18610
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/83120/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18610: [SPARK-21386] ML LinearRegression supports warm s...
Posted by WeichenXu123 <gi...@git.apache.org>.
Github user WeichenXu123 commented on a diff in the pull request:
https://github.com/apache/spark/pull/18610#discussion_r129652515
--- Diff: mllib/src/main/scala/org/apache/spark/ml/util/ReadWrite.scala ---
@@ -309,6 +313,23 @@ private[ml] object DefaultParamsWriter {
val metadataJson: String = compact(render(metadata))
metadataJson
}
+
+ /**
+ * Save estimator's `initialModel` to corresponding path.
+ */
+ def saveInitialModel[T <: HasInitialModel[_ <: MLWritable with Params]](
+ instance: T, path: String): Unit = {
+ if (instance.isDefined(instance.initialModel)) {
+ val initialModelPath = new Path(path, "initialModel").toString
+ val initialModel = instance.getOrDefault(instance.initialModel)
+ // When saving, only keep the direct initialModel by eliminating possible initialModels of the
+ // direct initialModel, to avoid unnecessary deep recursion of initialModel.
+ if (initialModel.hasParam("initialModel")) {
+ initialModel.clear(initialModel.getParam("initialModel"))
+ }
+ initialModel.save(initialModelPath)
--- End diff --
I suggest when setInitialModel, first copy the passed in model (by `Model.copy`) and then clear the `initialModel` param in the passed in model to avoid recursive risk. Because the passed in model maybe used in other place we should avoid to modify its params.
Another way is avoid to add the initial model param to Model, only add it to the Estimator. (This way I think is more reasonable).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18610: [SPARK-21386] ML LinearRegression supports warm s...
Posted by yanboliang <gi...@git.apache.org>.
Github user yanboliang commented on a diff in the pull request:
https://github.com/apache/spark/pull/18610#discussion_r129574777
--- Diff: mllib/src/main/scala/org/apache/spark/ml/util/ReadWrite.scala ---
@@ -309,6 +313,23 @@ private[ml] object DefaultParamsWriter {
val metadataJson: String = compact(render(metadata))
metadataJson
}
+
+ /**
+ * Save estimator's `initialModel` to corresponding path.
+ */
+ def saveInitialModel[T <: HasInitialModel[_ <: MLWritable with Params]](
+ instance: T, path: String): Unit = {
+ if (instance.isDefined(instance.initialModel)) {
+ val initialModelPath = new Path(path, "initialModel").toString
+ val initialModel = instance.getOrDefault(instance.initialModel)
+ // When saving, only keep the direct initialModel by eliminating possible initialModels of the
+ // direct initialModel, to avoid unnecessary deep recursion of initialModel.
+ if (initialModel.hasParam("initialModel")) {
+ initialModel.clear(initialModel.getParam("initialModel"))
+ }
+ initialModel.save(initialModelPath)
--- End diff --
@WeichenXu123 Can you elaborate the concrete scenario? Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18610: [SPARK-21386] ML LinearRegression supports warm start fr...
Posted by sethah <gi...@git.apache.org>.
Github user sethah commented on the issue:
https://github.com/apache/spark/pull/18610
A couple questions:
1. do we actually *need* to save the initialModel when we persist the current model? I'm not sure it's necessary and it adds complexity. Also, we could add it in the future anyway.
2. How can users set an initial model from an array of coefficients? If initialModel has to be a `LinearRegressionModel` then how can I create one from an array? The model constructor is private... If you can only set an initial model that was trained in Spark and saved as a Spark model, then that severely limits the utility of this functionality. Can we just overload the set method?
````scala
final val initialModel: Param[Vector]
def setInitialModel(value: Vector) = ...
def setInitialModel(value: LinearRegressionModel) = setInitialModel(value.coefficients)
````
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18610: [SPARK-21386] ML LinearRegression supports warm start fr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18610
**[Test build #83120 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83120/testReport)** for PR 18610 at commit [`e49d452`](https://github.com/apache/spark/commit/e49d45271c923eeeba3f3bbbd79dc5cbf0c91ab1).
* This patch **fails Spark unit tests**.
* This patch **does not merge 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 #18610: [SPARK-21386] ML LinearRegression supports warm start fr...
Posted by JohnHBrock <gi...@git.apache.org>.
Github user JohnHBrock commented on the issue:
https://github.com/apache/spark/pull/18610
What else needs to be done before this can be merged?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18610: [SPARK-21386] ML LinearRegression supports warm start fr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18610
**[Test build #79553 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/79553/testReport)** for PR 18610 at commit [`ae8f32b`](https://github.com/apache/spark/commit/ae8f32b2fe8b774f50ffde2931e0727eca1ea11b).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18610: [SPARK-21386] ML LinearRegression supports warm start fr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18610
Merged build finished. Test FAILed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18610: [SPARK-21386] ML LinearRegression supports warm s...
Posted by hhbyyh <gi...@git.apache.org>.
Github user hhbyyh commented on a diff in the pull request:
https://github.com/apache/spark/pull/18610#discussion_r135418289
--- Diff: mllib/src/main/scala/org/apache/spark/ml/regression/LinearRegression.scala ---
@@ -72,6 +72,22 @@ private[regression] trait LinearRegressionParams extends PredictorParams
}
/**
+ * Params for linear regression.
+ */
+private[regression] trait LinearRegressionParams extends LinearRegressionModelParams
--- End diff --
It maybe cleaner if we just move the param `initialModel` into LinearRegression? so we don't have to touch the class hierarchy.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18610: [SPARK-21386] ML LinearRegression supports warm s...
Posted by yanboliang <gi...@git.apache.org>.
Github user yanboliang commented on a diff in the pull request:
https://github.com/apache/spark/pull/18610#discussion_r136020810
--- Diff: mllib/src/main/scala/org/apache/spark/ml/regression/LinearRegression.scala ---
@@ -72,6 +72,22 @@ private[regression] trait LinearRegressionParams extends PredictorParams
}
/**
+ * Params for linear regression.
+ */
+private[regression] trait LinearRegressionParams extends LinearRegressionModelParams
--- End diff --
The refactor here is not only for this PR, since model usually has less params than estimator, we make _EstimatorParams_ extends from _ModelParams_ in other places, for example, _ALSParams_ extends _ALSModelParams_. I think the class hierarchy should be more clear.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18610: [SPARK-21386] ML LinearRegression supports warm start fr...
Posted by holdenk <gi...@git.apache.org>.
Github user holdenk commented on the issue:
https://github.com/apache/spark/pull/18610
@sethah I don't think we need to saving the initial model right now, and if its going to slow down the PR progress maybe dropping it makes sense -- but I guess it depends how complicated that is?
For the second point, I also think creating an initial model from a set of coefficients could be useful, but I think that could be addressed in a follow up issue -- what do you think?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18610: [SPARK-21386] ML LinearRegression supports warm s...
Posted by yanboliang <gi...@git.apache.org>.
Github user yanboliang commented on a diff in the pull request:
https://github.com/apache/spark/pull/18610#discussion_r136018943
--- Diff: mllib/src/main/scala/org/apache/spark/ml/regression/LinearRegression.scala ---
@@ -226,6 +246,12 @@ class LinearRegression @Since("1.3.0") (@Since("1.3.0") override val uid: String
if (($(solver) == Auto &&
numFeatures <= WeightedLeastSquares.MAX_NUM_FEATURES) || $(solver) == Normal) {
+
+ if (isSet(initialModel)) {
+ logWarning("Initial model will be ignored if fitting by normal solver. " +
--- End diff --
Fair enough, I will update it after collecting all comments. Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18610: [SPARK-21386] ML LinearRegression supports warm start fr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18610
**[Test build #79554 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/79554/testReport)** for PR 18610 at commit [`0b7509e`](https://github.com/apache/spark/commit/0b7509efb2e8690980ff5fd4f450d8540948763c).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18610: [SPARK-21386] ML LinearRegression supports warm start fr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18610
**[Test build #79554 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/79554/testReport)** for PR 18610 at commit [`0b7509e`](https://github.com/apache/spark/commit/0b7509efb2e8690980ff5fd4f450d8540948763c).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18610: [SPARK-21386] ML LinearRegression supports warm start fr...
Posted by holdenk <gi...@git.apache.org>.
Github user holdenk commented on the issue:
https://github.com/apache/spark/pull/18610
@JohnHBrock this PR is pretty old so the biggest challenge is going to be updating it to the current master branch. There's some discussion around the types needing to be changed as well. If this is a thing you want to work on I'd love to do what I can to help with the review process.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18610: [SPARK-21386] ML LinearRegression supports warm start fr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18610
**[Test build #79562 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/79562/testReport)** for PR 18610 at commit [`e49d452`](https://github.com/apache/spark/commit/e49d45271c923eeeba3f3bbbd79dc5cbf0c91ab1).
* This patch **fails SparkR unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18610: [SPARK-21386] ML LinearRegression supports warm start fr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18610
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 #18610: [SPARK-21386] ML LinearRegression supports warm start fr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18610
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/4889/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org