[SPARK-22060][ML] Fix CrossValidator/TrainValidationSplit param persist/load bug #19278

WeichenXu123 · 2017-09-19T13:47:43Z

What changes were proposed in this pull request?

Currently the param of CrossValidator/TrainValidationSplit persist/loading is hardcoding, which is different with other ML estimators. This cause persist bug for new added parallelism param.

I refactor related code, avoid hardcoding persist/load param. And in the same time, it solve the parallelism persisting bug.

This refactoring is very useful because we will add more new params in #19208 , hardcoding param persisting/loading making the thing adding new params very troublesome.

How was this patch tested?

Test added.

WeichenXu123 · 2017-09-19T13:48:02Z

cc @BryanCutler Thanks!

SparkQA · 2017-09-19T15:00:23Z

Test build #81927 has finished for PR 19278 at commit 042b3d5.

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

smurching

Thanks for the PR! I've left a few comments.

(This might be a task for a future PR) I'm curious why setEstimator() and setEvaluator() aren't part of the ValidatorParams API - they're currently called by both classes that extend ValidatorParams (TrainValidationSplit, CrossValidator) and moving these setter methods up the class hierarchy would allow you to share more loading/persistence code in ValidatorParams.saveImpl

smurching · 2017-09-19T20:33:17Z

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

Use an Option[List[String]] that defaults to None instead of a List[String] that defaults to null?

smurching · 2017-09-19T20:59:45Z

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

Update the docstring to state that params included in skipParams aren't set.

smurching · 2017-09-19T21:03:08Z

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

This exception is unused & can be removed.

BryanCutler

Thanks for fixing the parallelsim param persistence. I think it is a little confusing that some of the params are loaded manually, and some by DefaultParamReader.getAndSetParams that then requires you to then skip certain params. Is is possible to do this without the list of params to skip? If not, then maybe it would be better not to use DefaultParamsReader.getAndSetParams at all.

BryanCutler · 2017-09-20T00:25:42Z

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

Is this included by accident?

Yes. I will remove it. sorry.

BryanCutler · 2017-09-20T00:29:20Z

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

Was this intentional?

No. I will remove it. sorry.

BryanCutler · 2017-09-20T00:30:22Z

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

Should this also skip estimator and evaluator?

No. Because estimator and evaluator isn't included in metadata. You can check the saveImpl.

BryanCutler · 2017-09-20T00:35:37Z

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

do you also need to skip estimator and evaluator

No. Because estimator and evaluator isn't included in metadata. You can check the saveImpl.

WeichenXu123 · 2017-09-20T01:25:20Z

@BryanCutler The reason I add skipParams is that, if we don't use DefaultParamReader.getAndSetParams, we have to hardcoding all params which are very troublesome. And every time we add new params, we have to also update the hardcoding params, it is very easy to forget and cause bug. But if use DefaultParamReader.getAndSetParams but do not support skipParams, the estimatorParamMaps is difficult to handle.
So this design is a balance of this concerns, although it makes the code a little weird.

SparkQA · 2017-09-20T02:52:38Z

Test build #81959 has finished for PR 19278 at commit cc30578.

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

jkbradley

Thanks for catching this! One question: I think this maintains backwards compatibility, but would you mind testing that manually by:

Exporting a CV model using spark/master
Importing that CV model using this PR's branch, and making sure that works?

Thanks!

jkbradley · 2017-09-21T22:15:39Z

mllib/src/test/scala/org/apache/spark/ml/tuning/TrainValidationSplitSuite.scala

      .setTrainRatio(0.5)
      .setEstimatorParamMaps(paramMaps)
      .setSeed(42L)
+      .setParallelism(2)


Could you update the test for the Model too please?

No. The model do not own parallel parameter. This was discussed before.

Ah, you're right, thanks

jkbradley · 2017-09-21T22:15:56Z

mllib/src/test/scala/org/apache/spark/ml/tuning/CrossValidatorSuite.scala

      .setNumFolds(20)
      .setEstimatorParamMaps(paramMaps)
+      .setSeed(42L)
+      .setParallelism(2)


Update the test for the model too please

jkbradley · 2017-09-21T22:17:46Z

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

   * TODO: Move to [[Metadata]] method
   */
-  def getAndSetParams(instance: Params, metadata: Metadata): Unit = {
+  def getAndSetParams(instance: Params, metadata: Metadata,


fix scala style: 1 arg per line for multiline declarations

jkbradley · 2017-09-21T22:18:01Z

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

+   * This works if all Params (except params included by `skipParams` list) implement
+   * [[org.apache.spark.ml.param.Param.jsonDecode()]].
+   *
+   * The params included in `skipParams` won't be set. This is useful if some params don't


Document params using @param

jkbradley · 2017-09-21T22:21:57Z

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


      val (metadata, estimator, evaluator, estimatorParamMaps) =
        ValidatorParams.loadImpl(path, sc, className)
      val numFolds = (metadata.params \ "numFolds").extract[Int]


numFolds is no longer needed

WeichenXu123 · 2017-09-22T03:03:50Z

@jkbradley Sure I tested the backwards compatibility. Part of the reason I changed into DefaultParamReader.getAndSetParams is for backwards compatibility.

SparkQA · 2017-09-22T03:25:01Z

Test build #82057 has finished for PR 19278 at commit 8f78f59.

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

jkbradley · 2017-09-23T01:14:04Z

LGTM
Merging with master
Thanks @WeichenXu123 for the fix and for testing for backwards compatibility!

smurching reviewed Sep 19, 2017

View reviewed changes

BryanCutler reviewed Sep 20, 2017

View reviewed changes

WeichenXu123 added 2 commits September 20, 2017 09:35

init pr

86abf48

update

cc30578

WeichenXu123 force-pushed the fix-tuning-param-bug branch from 042b3d5 to cc30578 Compare September 20, 2017 01:45

WeichenXu123 mentioned this pull request Sep 20, 2017

[SPARK-21087] [ML] CrossValidator, TrainValidationSplit expose sub models after fitting: Scala #19208

Closed

jkbradley reviewed Sep 21, 2017

View reviewed changes

update

8f78f59

asfgit closed this in f180b65 Sep 23, 2017

WeichenXu123 deleted the fix-tuning-param-bug branch September 23, 2017 01:33

[SPARK-22060][ML] Fix CrossValidator/TrainValidationSplit param persist/load bug #19278

[SPARK-22060][ML] Fix CrossValidator/TrainValidationSplit param persist/load bug #19278

Uh oh!

Conversation

WeichenXu123 commented Sep 19, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

WeichenXu123 commented Sep 19, 2017

Uh oh!

SparkQA commented Sep 19, 2017

Uh oh!

smurching left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BryanCutler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WeichenXu123 commented Sep 20, 2017

Uh oh!

SparkQA commented Sep 20, 2017

Uh oh!

jkbradley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WeichenXu123 commented Sep 22, 2017

Uh oh!

SparkQA commented Sep 22, 2017

Uh oh!

jkbradley commented Sep 23, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants