[SPARK-11893] Model export/import for spark.ml: TrainValidationSplit #9971

yinxusen · 2015-11-25T11:44:32Z

https://issues.apache.org/jira/browse/SPARK-11893

@jkbradley In order to share read/write with TrainValidationSplit, I move the SharedReadWrite out of CrossValidator into a new trait SharedReadWrite in the tunning package.

To reduce the repeated tests, I move the complex tests from CrossValidatorSuite to SharedReadWriteSuite, and create a fake validator called MyValidator to test the shared code.

With SharedReadWrite, potential newly added Validator can share the read/write common part, and only need to implement their extra params save/load.

SparkQA · 2015-11-25T12:48:48Z

Test build #46685 has finished for PR 9971 at commit 71355ca.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

yinxusen · 2016-01-11T10:31:49Z

test it please

SparkQA · 2016-01-11T10:35:36Z

Test build #49134 has finished for PR 9971 at commit 023371b.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

yinxusen · 2016-01-11T11:13:56Z

test it please

SparkQA · 2016-01-11T12:10:19Z

Test build #49140 has finished for PR 9971 at commit 8c02be2.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

yinxusen · 2016-01-11T13:39:58Z

Ping @jkbradley

jkbradley · 2016-03-23T19:30:14Z

@yinxusen Hi, sorry for the long wait. I'd like to review this now. Could you please rebase from master?

yinxusen · 2016-03-23T21:42:57Z

Sure will do it soon

Sent from my iPhone

On Mar 23, 2016, at 12:31, jkbradley [email protected] wrote:

@yinxusen Hi, sorry for the long wait. I'd like to review this now. Could you please rebase from master?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

yinxusen · 2016-03-23T21:50:56Z

test it please

SparkQA · 2016-03-23T21:57:57Z

Test build #53973 has finished for PR 9971 at commit 7f2dd31.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

yinxusen · 2016-03-23T22:05:05Z

test it please

SparkQA · 2016-03-23T22:52:34Z

Test build #53976 has finished for PR 9971 at commit 581fc5b.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

jkbradley · 2016-03-24T00:14:53Z

Ok thanks! I'll review this now

jkbradley · 2016-03-24T00:32:34Z

Initial comment: I like the idea of testing Validators jointly, but I don't think creating a MyValidator class is the best way since it involves a lot of new code and since that new code is mimicking what is already in CrossValidator. I'd prefer to keep the original CrossValidatorSuite tests since I believe those test the same functionality.

jkbradley · 2016-03-24T00:37:58Z

mllib/src/main/scala/org/apache/spark/ml/tuning/SharedReadWrite.scala

+   * Examine the given estimator (which may be a compound estimator) and extract a mapping
+   * from UIDs to corresponding [[Params]] instances.
+   */
+  def getUidMap(instance: Params): Map[String, Params] = {


Did you mean to use this in CrossValidator as well? There is still a copy in CrossValidator.scala.

Also, this method and its helper should probably live in ml/util/ReadWrite.scala since they apply to tuning, Pipeline, and classification (OneVsRest)

sorry, it's an error imported by merging with master. I'll fix it soon.

jkbradley · 2016-03-24T00:41:06Z

I'll hold off on a detailed review until these initial items are addressed since they will require significant code movement.

yinxusen · 2016-03-24T00:56:00Z

Sure I'll ping you after that.

Sent from my iPhone

On Mar 23, 2016, at 17:42, jkbradley [email protected] wrote:

I'll hold off on a detailed review until these initial items are addressed since they will require significant code movement.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

SparkQA · 2016-03-24T21:39:14Z

Test build #54086 has finished for PR 9971 at commit ea077d8.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

yinxusen · 2016-03-24T21:41:49Z

Ping @jkbradley, I've fixed those. You can move on now.

SparkQA · 2016-03-24T21:55:21Z

Test build #54089 has finished for PR 9971 at commit af0039f.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

jkbradley · 2016-03-25T20:52:44Z

mllib/src/main/scala/org/apache/spark/ml/util/ReadWrite.scala

+      case v: ValidatorParams => Array(v.getEstimator, v.getEvaluator)
+      case ovr: OneVsRestParams =>
+        // TODO: SPARK-11892: This case may require special handling.
+        throw new UnsupportedOperationException("CrossValidator write will fail because it" +


"CrossValidator" --> ${instance.getClass.getName}

jkbradley · 2016-03-25T20:53:20Z

Thanks! I just made another pass. I'll check again since this will require a bit more code movement.

yinxusen · 2016-03-25T21:32:50Z

Thanks @jkbradley I moved the validateParams into ValidatorParams, and changed the MetaPipelineReadWrite into an Object.

SparkQA · 2016-03-25T21:37:51Z

Test build #54220 has finished for PR 9971 at commit 8920986.

This patch fails to build.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class CrossValidatorModelWriter(instance: CrossValidatorModel) extends MLWriter

SparkQA · 2016-03-25T22:43:37Z

Test build #54225 has finished for PR 9971 at commit 2666913.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class TrainValidationSplitModelWriter(instance: TrainValidationSplitModel) extends MLWriter

yinxusen · 2016-03-25T22:53:28Z

@jkbradley OK for another look

jkbradley · 2016-03-28T18:41:40Z

@yinxusen Thanks for the update. It looks good, but there was one missed item + a few more I found. I'm going to send a PR to update this PR.

jkbradley · 2016-03-28T20:04:39Z

I just sent a PR: [https://github.com/yinxusen/pull/5]

Yinxusen spark 11893 cleanups

yinxusen · 2016-03-28T20:20:51Z

Merged! @jkbradley

yinxusen · 2016-03-28T20:21:53Z

test it please

SparkQA · 2016-03-28T21:03:55Z

Test build #54355 has finished for PR 9971 at commit f9172d1.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

jkbradley · 2016-03-28T22:39:28Z

LGTM
Merging with master
Thanks!

yinxusen added 6 commits November 25, 2015 17:28

move shared read/write out

c3e73b4

fix params errors

7f3428a

add fake validator test

cdc41b7

simplify cross validation test save/load

bb5423e

add TrainValidationSplitSuite

15d0df6

change 1.6 to 1.7

71355ca

merge with master and change version

023371b

change order of import

8c02be2

yinxusen mentioned this pull request Feb 8, 2016

[SPARK-10780][ML] Add an initial model to kmeans #11119

Closed

jkbradley mentioned this pull request Mar 23, 2016

[SPARK-11892][ML] Model export/import for spark.ml: OneVsRest #9934

Closed

merge with master

7f2dd31

fix errors

581fc5b

jkbradley reviewed Mar 24, 2016
View reviewed changes

move sharedReadWrite to MetaPipelineReadWrite

195ebcc

yinxusen added 4 commits March 24, 2016 13:30

revert CrossValidatorSuite

6021a99

remove sharedreadwritesuite

bc7f1a7

Merge branch 'master' into SPARK-11893

ea077d8

fix nits

af0039f

jkbradley reviewed Mar 25, 2016
View reviewed changes

change positions

8920986

remove trait extends

2666913

jkbradley added 3 commits March 28, 2016 12:55

Cleanups around validator read/write

69e4af2

more cleanups

550f4ad

added doc to save/loadImpl

53397c2

Merge pull request #5 from jkbradley/yinxusen-SPARK-11893

f9172d1

Yinxusen spark 11893 cleanups

asfgit closed this in 8c11d1a Mar 28, 2016

[SPARK-11893] Model export/import for spark.ml: TrainValidationSplit #9971

[SPARK-11893] Model export/import for spark.ml: TrainValidationSplit #9971

Uh oh!

Conversation

yinxusen commented Nov 25, 2015

Uh oh!

SparkQA commented Nov 25, 2015

Uh oh!

yinxusen commented Jan 11, 2016

Uh oh!

SparkQA commented Jan 11, 2016

Uh oh!

yinxusen commented Jan 11, 2016

Uh oh!

SparkQA commented Jan 11, 2016

Uh oh!

yinxusen commented Jan 11, 2016

Uh oh!

jkbradley commented Mar 23, 2016

Uh oh!

yinxusen commented Mar 23, 2016

Uh oh!

yinxusen commented Mar 23, 2016

Uh oh!

SparkQA commented Mar 23, 2016

Uh oh!

yinxusen commented Mar 23, 2016

Uh oh!

SparkQA commented Mar 23, 2016

Uh oh!

jkbradley commented Mar 24, 2016

Uh oh!

jkbradley commented Mar 24, 2016

Uh oh!

jkbradley Mar 24, 2016

Choose a reason for hiding this comment

Uh oh!

yinxusen Mar 24, 2016

Choose a reason for hiding this comment

Uh oh!

jkbradley commented Mar 24, 2016

Uh oh!

yinxusen commented Mar 24, 2016

Uh oh!

SparkQA commented Mar 24, 2016

Uh oh!

yinxusen commented Mar 24, 2016

Uh oh!

SparkQA commented Mar 24, 2016

Uh oh!

jkbradley Mar 25, 2016

Choose a reason for hiding this comment

Uh oh!

jkbradley commented Mar 25, 2016

Uh oh!

yinxusen commented Mar 25, 2016

Uh oh!

SparkQA commented Mar 25, 2016

Uh oh!

SparkQA commented Mar 25, 2016

Uh oh!

yinxusen commented Mar 25, 2016

Uh oh!

jkbradley commented Mar 28, 2016

Uh oh!

jkbradley commented Mar 28, 2016

Uh oh!

yinxusen commented Mar 28, 2016

Uh oh!

yinxusen commented Mar 28, 2016

Uh oh!

SparkQA commented Mar 28, 2016

Uh oh!

jkbradley commented Mar 28, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone