[SPARK-23455][ML] Default Params in ML should be saved separately in metadata #20633

viirya · 2018-02-17T08:55:06Z

What changes were proposed in this pull request?

We save ML's user-supplied params and default params as one entity in metadata. During loading the saved models, we set all the loaded params into created ML model instances as user-supplied params.

It causes some problems, e.g., if we strictly disallow some params to be set at the same time, a default param can fail the param check because it is treated as user-supplied param after loading.

The loaded default params should not be set as user-supplied params. We should save ML default params separately in metadata.

For backward compatibility, when loading metadata, if it is a metadata file from previous Spark, we shouldn't raise error if we can't find the default param field.

How was this patch tested?

Pass existing tests and added tests.

SparkQA · 2018-02-17T12:39:52Z

Test build #87521 has finished for PR 20633 at commit 69648d6.

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

viirya · 2018-02-17T14:57:23Z

cc @jkbradley This is to save default params separately in metadata file. Please help review after 2.3. Thanks!

viirya · 2018-03-02T04:51:49Z

also cc @MLnick @WeichenXu123 for review.

WeichenXu123 · 2018-03-05T10:14:11Z

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

     */
-    def getParamValue(paramName: String): JValue = {
+    def getParamValue(paramName: String, isDefaultParam: Boolean = false): JValue = {


Should the logic be "lookup params first, if not exist then lookup defaultParams" ?
If so, we don't need the isDefaultParam param.

Sounds good. I will change this.

WeichenXu123 · 2018-03-05T10:42:34Z

@viirya Have you run backward compatibility test ? e.g., saving estimator/models via master version spark, and load estimator/models via this PR version spark. To check whether it works fine and all params get saved and restored correctly.
Especially this should be done against QuantileDiscretizer/Bucketizer which you remove temporary fix code.

viirya · 2018-03-06T06:03:30Z

@WeichenXu123 I've added unit test in DefaultReadWriteSuite/DefaultReadWriteTest to test if this can read old metadata back.

Sounds like the backward compatibility test you suggested should be checked manually. I will test it. Thanks!

SparkQA · 2018-03-06T08:05:01Z

Test build #87996 has finished for PR 20633 at commit 166cdbb.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2018-03-06T08:11:50Z

retest this please.

viirya · 2018-03-06T09:35:33Z

@WeichenXu123 Thanks for comment! I've run the backward compatibility test locally against QuantileDiscretizer/Bucketizer. They work fine. After this PR, the saved models from 2.3.0 can be read back and all params are restored correctly.

WeichenXu123 · 2018-03-06T10:07:24Z

LGTM. Thanks! @jkbradley

SparkQA · 2018-03-06T12:12:36Z

Test build #87998 has finished for PR 20633 at commit 166cdbb.

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

viirya · 2018-03-10T10:10:50Z

cc @dbtsai if you have time to look at this too.

jkbradley

Thanks for the PR! I made a review pass, and it looks like a nice setup.

jkbradley · 2018-03-21T00:25:24Z

mllib/src/main/scala/org/apache/spark/ml/param/params.scala

@@ -791,7 +791,7 @@ trait Params extends Identifiable with Serializable {
   *               this method gets called.
   * @param value  the default value
   */
-  protected final def setDefault[T](param: Param[T], value: T): this.type = {
+  private[ml] final def setDefault[T](param: Param[T], value: T): this.type = {


We should leave this as protected. It's important that 3rd-party libraries be able to extend MLlib APIs, and this is one they need.

Because we need to call setDefault in Metadata.setParams, I can't leave it as protected. But I think it is important to keep the extensibility. And I think we can't make it as public too. I add object Params and use it to help us update default param of a Params.

Makes sense; I like your solution.

jkbradley · 2018-03-21T00:28:56Z

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

    val basicMetadata = ("class" -> cls) ~
      ("timestamp" -> System.currentTimeMillis()) ~
      ("sparkVersion" -> sc.version) ~
      ("uid" -> uid) ~
+      ("defaultParamMap" -> jsonDefaultParams) ~


nit: How about putting this below paramMap since that's nicer for viewing the JSON?

jkbradley · 2018-03-21T00:39:13Z

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

+    def getAndSetParams(
+        instance: Params,
+        skipParams: Option[List[String]] = None): Unit = {
+      setParams(instance, false, skipParams)


style nit: It's nice to pass boolean args by name

jkbradley · 2018-03-21T00:55:01Z

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

+            }
+          }
+        // For metadata file prior to Spark 2.4, there is no default section.
+        case JNothing if isDefault && (major == 2 && minor < 4 || major < 2) =>


This logic would be simpler if this check were put in the getAndSetParams method, which could just skip calling setParams(instance, true, skipParams) for Spark 2.3-.

jkbradley · 2018-03-21T01:00:37Z

mllib/src/test/scala/org/apache/spark/ml/util/DefaultReadWriteTest.scala

+    }
+  }
+
+  test("User-suppiled value for default param should be kept after persistence") {


suppiled -> supplied

jkbradley · 2018-03-21T01:03:41Z

mllib/src/test/scala/org/apache/spark/ml/util/DefaultReadWriteTest.scala

+    assert(loadedMyParams.get(myParams.intParamWithDefault).get == 100)
+  }
+
+  test("Read metadata without default field prior to 2.4") {


Nice! I like this setup.

SparkQA · 2018-03-22T07:05:01Z

Test build #88502 has finished for PR 20633 at commit a2b6917.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2018-03-22T07:19:29Z

retest this please.

SparkQA · 2018-03-22T11:23:53Z

Test build #88510 has finished for PR 20633 at commit a2b6917.

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

viirya · 2018-03-23T04:33:37Z

@jkbradley Thanks for your comments! I've addressed them. Please review it again. Thank you.

viirya · 2018-03-31T14:23:56Z

ping @jkbradley

jkbradley · 2018-04-03T20:39:35Z

mllib/src/main/scala/org/apache/spark/ml/param/params.scala

@@ -905,6 +905,15 @@ trait Params extends Identifiable with Serializable {
  }
 }

+object Params {


Shall we make this object package private (for cleaner docs)

jkbradley · 2018-04-03T20:39:45Z

mllib/src/main/scala/org/apache/spark/ml/param/params.scala

@@ -791,7 +791,7 @@ trait Params extends Identifiable with Serializable {
   *               this method gets called.
   * @param value  the default value
   */
-  protected final def setDefault[T](param: Param[T], value: T): this.type = {
+  private[ml] final def setDefault[T](param: Param[T], value: T): this.type = {


Makes sense; I like your solution.

jkbradley

Sorry for the slow response!

jkbradley · 2018-04-03T20:45:32Z

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

+
+      // For metadata file prior to Spark 2.4, there is no default section.
+      val (major, minor) = VersionUtils.majorMinorVersion(sparkVersion)
+      if (major >= 2 && minor >= 4) {


This should be:
if (major > 2 || (major == 2 && minor >= 4)) {

Ah, you're correct. I'll fix it.

Good catch!

SparkQA · 2018-04-04T12:13:19Z

Test build #88880 has finished for PR 20633 at commit de93e65.

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

dbtsai · 2018-04-17T00:09:09Z

Can you address the conflicts? Gonna start to review it soon. Thanks.

…arams

viirya · 2018-04-17T00:30:23Z

@dbtsai Thanks! I've solved the conflicts.

SparkQA · 2018-04-17T05:00:58Z

Test build #89421 has finished for PR 20633 at commit 80b668a.

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

viirya · 2018-04-19T11:32:26Z

ping @jkbradley

jkbradley · 2018-04-23T22:49:19Z

Sorry for the pause in review.
LGTM
Merging with master (pending fresh tests)

@dbtsai I'm going to merge this since I'm worried it will collect more conflicts, but let's discuss more if needed.

@viirya We'll need to update Python's DefaultParamsReader as well for Spark 2.4 in order to keep it in sync with Scala/Java. R thankfully should not require anything since it only has wrappers. I'll make & link a JIRA: https://issues.apache.org/jira/browse/SPARK-24058 Will you have time to work on that?

Thanks @viirya !

viirya · 2018-04-23T23:13:44Z

@jkbradley Thanks! I'll work on SPARK-24058.

SparkQA · 2018-04-24T03:19:17Z

Test build #4156 has finished for PR 20633 at commit 80b668a.

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

Save default params separately in JSON.

69648d6

WeichenXu123 reviewed Mar 5, 2018

View reviewed changes

Remove isDefaultParam.

166cdbb

jkbradley reviewed Mar 21, 2018

View reviewed changes

Address comments.

a2b6917

jkbradley reviewed Apr 3, 2018

View reviewed changes

Fix version check.

de93e65

Merge remote-tracking branch 'upstream/master' into save-ml-default-p…

80b668a

…arams

asfgit closed this in 8301375 Apr 24, 2018

viirya deleted the save-ml-default-params branch December 27, 2023 18:35

[SPARK-23455][ML] Default Params in ML should be saved separately in metadata #20633

[SPARK-23455][ML] Default Params in ML should be saved separately in metadata #20633

Conversation

viirya commented Feb 17, 2018

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Feb 17, 2018

viirya commented Feb 17, 2018

viirya commented Mar 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WeichenXu123 commented Mar 5, 2018

viirya commented Mar 6, 2018

SparkQA commented Mar 6, 2018

viirya commented Mar 6, 2018

viirya commented Mar 6, 2018 • edited Loading

WeichenXu123 commented Mar 6, 2018

SparkQA commented Mar 6, 2018

viirya commented Mar 10, 2018

jkbradley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Mar 22, 2018

viirya commented Mar 22, 2018

SparkQA commented Mar 22, 2018

viirya commented Mar 23, 2018

viirya commented Mar 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkbradley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Apr 4, 2018

dbtsai commented Apr 17, 2018

viirya commented Apr 17, 2018

SparkQA commented Apr 17, 2018

viirya commented Apr 19, 2018

jkbradley commented Apr 23, 2018 • edited Loading

viirya commented Apr 23, 2018

SparkQA commented Apr 24, 2018

viirya commented Mar 2, 2018 •

edited

Loading

viirya commented Mar 6, 2018 •

edited

Loading

jkbradley commented Apr 23, 2018 •

edited

Loading