[SPARK-25793][ML]call SaveLoadV2_0.load for classNameV2_0 #22790

huaxingao · 2018-10-22T04:11:50Z

What changes were proposed in this pull request?

The following code in BisectingKMeansModel.load calls the wrong version of load.

      case (SaveLoadV2_0.thisClassName, SaveLoadV2_0.thisFormatVersion) =>
        val model = SaveLoadV1_0.load(sc, path)

SparkQA · 2018-10-22T09:57:06Z

Test build #97756 has finished for PR 22790 at commit d599f83.

This patch passes all tests.
This patch does not merge cleanly.
This patch adds no public classes.

viirya · 2018-10-22T13:02:18Z

mllib/src/main/scala/org/apache/spark/mllib/clustering/BisectingKMeansModel.scala

@@ -126,7 +126,7 @@ object BisectingKMeansModel extends Loader[BisectingKMeansModel] {
        val model = SaveLoadV1_0.load(sc, path)
        model
      case (SaveLoadV2_0.thisClassName, SaveLoadV2_0.thisFormatVersion) =>
-        val model = SaveLoadV1_0.load(sc, path)
+        val model = SaveLoadV2_0.load(sc, path)


cc @mgaido91

ah, nice catch!

This is not a regression, but it looks like a correctness or data loss issue at Spark 2.4.0 new feature.
Do you know why https://issues.apache.org/jira/browse/SPARK-25793 is created as a Minor and Documentation issue?

cc @cloud-fan

viirya · 2018-10-22T13:03:04Z

mllib/src/main/scala/org/apache/spark/mllib/clustering/BisectingKMeansModel.scala

@@ -126,7 +126,7 @@ object BisectingKMeansModel extends Loader[BisectingKMeansModel] {
        val model = SaveLoadV1_0.load(sc, path)
        model
      case (SaveLoadV2_0.thisClassName, SaveLoadV2_0.thisFormatVersion) =>
-        val model = SaveLoadV1_0.load(sc, path)
+        val model = SaveLoadV2_0.load(sc, path)


Is there no test to verify calling correct load method?

I think we can improve the write/load model tests in order to include also a different distance measure from the default one. In this way we should catch this error. Thanks.

Do we have ever use SaveLoadV2_0 to save model for now? Looks BisectingKMeansModel.save simply calls SaveLoadV1_0.save to save models.

@viirya @mgaido91
I will change the BisectingKMeansModel.save to

BisectingKMeansModel.SaveLoadV2_0.save(sc, this, path)

SparkQA · 2018-10-22T15:07:05Z

Test build #97794 has finished for PR 22790 at commit d599f83.

This patch fails PySpark unit tests.
This patch does not merge cleanly.
This patch adds no public classes.

SparkQA · 2018-10-22T15:08:39Z

Test build #97824 has finished for PR 22790 at commit d599f83.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2018-10-22T23:50:41Z

Retest this please.

SparkQA · 2018-10-23T01:05:14Z

Test build #97882 has finished for PR 22790 at commit d599f83.

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

cloud-fan · 2018-10-23T02:58:49Z

cc @mengxr @WeichenXu123 how serious is it? shall we treat it as a blocker?

SparkQA · 2018-10-23T07:05:02Z

Test build #97904 has finished for PR 22790 at commit 77902bc.

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

huaxingao · 2018-10-23T16:00:49Z

retest this please

mengxr · 2018-10-23T16:20:15Z

This shouldn't block 2.4.0 release. Based on the code, it doesn't introduce regression to existing features (just using V1 format and ignore trainingCost and distanceMeasure). Correctness issue occurs only when someone uses a non-default distanceMeasure and then save/load. Could someone help confirm?

If current vote passes, we can list it as an known issue in the release notes and fix it in 2.4.1. If other blockers show up, we fix it before RC5. Btw, this PR needs a regression test in order to merge.

SparkQA · 2018-10-23T17:12:13Z

Test build #97928 has finished for PR 22790 at commit 77902bc.

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

huaxingao · 2018-10-23T21:58:40Z

I added a regression test in org.apache.spark.mllib.clustering.BisectingKMeansSuite
I could add the following test in ml package.

  test("SPARK-25793") {
    val bisectingKMeans = new BisectingKMeans()
    bisectingKMeans.setDistanceMeasure(DistanceMeasure.COSINE)
    val readBisectingKMeans = testDefaultReadWrite(bisectingKMeans)
    assert(bisectingKMeans.distanceMeasure === readBisectingKMeans.distanceMeasure)
  }

But the bug doesn't really affect the above test. With the bug, even though mllib BisectingKMeansModel.load will call V1_0 load and gives a model with default value of distanceMeasure, in ml package, BisectingKMeansModelReader.load will call metadata.getAndSetParams(model) which will set the distanceMeasure to the correct value (DistanceMeasure.COSINE).

    override def load(path: String): BisectingKMeansModel = {
      val metadata = DefaultParamsReader.loadMetadata(path, sc, className)
      val dataPath = new Path(path, "data").toString
      val mllibModel = MLlibBisectingKMeansModel.load(sc, dataPath)
      val model = new BisectingKMeansModel(metadata.uid, mllibModel)
      metadata.getAndSetParams(model)
      model
    }

SparkQA · 2018-10-23T23:06:08Z

Test build #97942 has finished for PR 22790 at commit 29c9ca0.

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

mgaido91 · 2018-10-25T11:49:51Z

Agree with @mengxr about this not being a blocker. Please notice this is a problem only for the mllib API, for which we can also avoid supporting new features AFAIK.

cloud-fan · 2018-10-25T14:45:39Z

Is this ready to go? We are going to have another RC, and would be good to include it.

WeichenXu123 · 2018-10-25T14:53:37Z

LGTM.

viirya · 2018-10-25T14:56:54Z

mllib/src/main/scala/org/apache/spark/mllib/clustering/BisectingKMeansModel.scala

@@ -109,7 +109,7 @@ class BisectingKMeansModel private[clustering] (

  @Since("2.0.0")
  override def save(sc: SparkContext, path: String): Unit = {
-    BisectingKMeansModel.SaveLoadV1_0.save(sc, this, path)
+    BisectingKMeansModel.SaveLoadV2_0.save(sc, this, path)
  }

  override protected def formatVersion: String = "1.0"


Is this formatVersion needed to change too?

I'd say yes, but actually this is never used. I think we can actually remove this from the trait.

Oh, good catch! need change.

I changed the formatVersion to 2.0. There are quite a few files that implement trait Saveable and have formatVersion. I don't feel comfortable to change other files for this PR. Maybe I will open a separate jira to remove formatVersion from trait Saveable?

I agree on that, I already have a patch for removing it (moreover this PR can target 2.4, while removal should be done only on master I think). I am submitting it. Thanks.

I created #22830 for that, thanks.

mgaido91 · 2018-10-25T17:46:43Z

LGTM

SparkQA · 2018-10-25T18:59:17Z

Test build #98036 has finished for PR 22790 at commit 0decb23.

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

cloud-fan · 2018-10-26T03:09:20Z

thanks, merging to master/2.4!

## What changes were proposed in this pull request? The following code in BisectingKMeansModel.load calls the wrong version of load. ``` case (SaveLoadV2_0.thisClassName, SaveLoadV2_0.thisFormatVersion) => val model = SaveLoadV1_0.load(sc, path) ``` Closes #22790 from huaxingao/spark-25793. Authored-by: Huaxin Gao <huaxing@us.ibm.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit dc9b320) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

## What changes were proposed in this pull request? The following code in BisectingKMeansModel.load calls the wrong version of load. ``` case (SaveLoadV2_0.thisClassName, SaveLoadV2_0.thisFormatVersion) => val model = SaveLoadV1_0.load(sc, path) ``` Closes apache#22790 from huaxingao/spark-25793. Authored-by: Huaxin Gao <huaxing@us.ibm.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

[SPARK-25793][ML]call SaveLoadV2_0.load for classNameV2_0

d599f83

viirya reviewed Oct 22, 2018

View reviewed changes

change BisectingKMeansModel.save to use SaveLoadV2_0.save

77902bc

add test

29c9ca0

viirya reviewed Oct 25, 2018

View reviewed changes

change formatVersion to 2.0

0decb23

asfgit closed this in dc9b320 Oct 26, 2018

WeichenXu123 mentioned this pull request Oct 26, 2018

[SPARK-25765][ML] Add training cost to BisectingKMeans summary #22764

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-25793][ML]call SaveLoadV2_0.load for classNameV2_0 #22790

[SPARK-25793][ML]call SaveLoadV2_0.load for classNameV2_0 #22790

huaxingao commented Oct 22, 2018

SparkQA commented Oct 22, 2018

viirya Oct 22, 2018

mgaido91 Oct 22, 2018

dongjoon-hyun Oct 22, 2018 •

edited

Loading

viirya Oct 23, 2018

viirya Oct 22, 2018

mgaido91 Oct 22, 2018

viirya Oct 23, 2018

huaxingao Oct 23, 2018

SparkQA commented Oct 22, 2018

SparkQA commented Oct 22, 2018

dongjoon-hyun commented Oct 22, 2018

SparkQA commented Oct 23, 2018

cloud-fan commented Oct 23, 2018

SparkQA commented Oct 23, 2018

huaxingao commented Oct 23, 2018

mengxr commented Oct 23, 2018

SparkQA commented Oct 23, 2018

huaxingao commented Oct 23, 2018

SparkQA commented Oct 23, 2018

mgaido91 commented Oct 25, 2018

cloud-fan commented Oct 25, 2018

WeichenXu123 commented Oct 25, 2018

viirya Oct 25, 2018

mgaido91 Oct 25, 2018

WeichenXu123 Oct 25, 2018

huaxingao Oct 25, 2018

mgaido91 Oct 25, 2018

mgaido91 Oct 25, 2018

mgaido91 commented Oct 25, 2018

SparkQA commented Oct 25, 2018

cloud-fan commented Oct 26, 2018

[SPARK-25793][ML]call SaveLoadV2_0.load for classNameV2_0 #22790

[SPARK-25793][ML]call SaveLoadV2_0.load for classNameV2_0 #22790

Conversation

huaxingao commented Oct 22, 2018

What changes were proposed in this pull request?

SparkQA commented Oct 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjoon-hyun Oct 22, 2018 • edited Loading

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 Oct 22, 2018

SparkQA commented Oct 22, 2018

dongjoon-hyun commented Oct 22, 2018

SparkQA commented Oct 23, 2018

cloud-fan commented Oct 23, 2018

SparkQA commented Oct 23, 2018

huaxingao commented Oct 23, 2018

mengxr commented Oct 23, 2018

SparkQA commented Oct 23, 2018

huaxingao commented Oct 23, 2018

SparkQA commented Oct 23, 2018

mgaido91 commented Oct 25, 2018

cloud-fan commented Oct 25, 2018

WeichenXu123 commented Oct 25, 2018

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

mgaido91 commented Oct 25, 2018

SparkQA commented Oct 25, 2018

cloud-fan commented Oct 26, 2018

dongjoon-hyun Oct 22, 2018 •

edited

Loading