[SPARK-18481][ML] ML 2.1 QA: Remove deprecated methods for ML #15913

yanboliang · 2016-11-17T04:37:20Z

What changes were proposed in this pull request?

Remove deprecated methods for ML.

How was this patch tested?

Existing tests.

SparkQA · 2016-11-17T05:04:17Z

Test build #68743 has finished for PR 15913 at commit ecddf15.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-11-17T07:52:36Z

Test build #68757 has started for PR 15913 at commit 9a8c926.

SparkQA · 2016-11-17T18:12:41Z

Test build #68780 has finished for PR 15913 at commit 9a8c926.

This patch fails from timeout after a configured wait of 250m.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-11-18T07:37:44Z

Test build #68829 has finished for PR 15913 at commit 9a8c926.

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

SparkQA · 2016-11-18T13:27:29Z

Test build #68849 has finished for PR 15913 at commit 13988b4.

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

…onModelParams.

…nction.

SparkQA · 2016-11-19T15:46:41Z

Test build #68892 has finished for PR 15913 at commit 36c2e24.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-11-19T18:07:25Z

Test build #68893 has finished for PR 15913 at commit 8d3c47a.

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

yanboliang · 2016-11-20T03:46:55Z

Jenkins, test this please.

SparkQA · 2016-11-20T06:08:17Z

Test build #68900 has finished for PR 15913 at commit 8d3c47a.

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

yanboliang · 2016-11-21T06:28:49Z

cc @jkbradley @sethah

holdenk · 2016-11-22T20:07:41Z

mllib/src/main/scala/org/apache/spark/ml/tree/treeModels.scala

-  /**
-   * Number of trees in ensemble
-   */
-  val getNumTrees: Int = trees.length


Is there a reason this was moved? It seems unrelated to the cleanup described in the title.

This is a headache. Before, getNumTrees on gbts/rfs would dispatch here. Now, we removed numTrees from RF (it was deprecated) and so now we can have rf inherit from RandomForestParams which (now) has a param numTrees and its associated getter. So if we hadn't removed this, rf would have two getNumTrees methods and one would take precedence. We removed it from here to avoid that, then we had to add them back in the individual GBT subclasses. I think I've got that right.

@sethah You explained this problem very clearly and totally right. Thanks.

holdenk · 2016-11-22T20:12:02Z

mllib/src/main/scala/org/apache/spark/ml/tree/treeParams.scala

-  def setStepSize(value: Double): this.type = set(stepSize, value)
-
-  override def validateParams(): Unit = {
+  def setStepSize(value: Double): this.type = {
    require(ParamValidators.inRange(0, 1, lowerInclusive = false, upperInclusive = true)(


I'm not sure this is where we want to do this? The deprecation warning said to move checks from here into transformSchema - but its possible the deprecation warning was incomplete.

I think it's better to fail at set time rather than when fit is called. This was also not used before, seems we missed it when we deprecated validateParams() ?

The original validateParams() was only used to check for interactions between parameters. Parameter value checks which do not depend on other parameters are handled by ````Param.validate()``` at the definition, i.e.

final val k = new IntParam(this, "k", "The number of clusters to create. " + "Must be > 1.", ParamValidators.gt(1))

However, stepSize was a trait and inherited by lots of sub-classes which have different constraints for this parameter, so I add the check at setter.
Yeah, I think the original place of this check(putting in validateParams) was a bug, since it does not involves interactions between parameters. We missed it when we deprecated validateParams(), since there is no test case to validate this param before. I added corresponding test in this PR.

That makes more sense :) +1

This validation should really be in the Param itself. It's not since it's a sharedParam, but perhaps we should just copy the shared Param code here. The problem with putting validation in the setter is that Params can get set in other ways too (such as gbt.set(stepSize, value)

sethah

Looks pretty good overall, a few small comments.

sethah · 2016-11-23T16:58:16Z

mllib/src/main/scala/org/apache/spark/ml/classification/LogisticRegression.scala

@@ -176,8 +176,12 @@ private[classification] trait LogisticRegressionParams extends ProbabilisticClas
    }
  }

-  override def validateParams(): Unit = {
+  override protected def validateAndTransformSchema(


Is this actually necessary? Before, validateParams() was never used. Seems like we could just remove it.

I think it's a bug that validateParams was never used. It should validate params interaction before fitting(if necessary), this is why we deprecate validateParams and move what it does to transformSchema. We do not have corresponding test cases before, so no test was broken when we deprecated validateParams. I added test cases in this PR.

sethah · 2016-11-23T17:09:43Z

mllib/src/main/scala/org/apache/spark/ml/tree/treeParams.scala

-  def setStepSize(value: Double): this.type = set(stepSize, value)
-
-  override def validateParams(): Unit = {
+  def setStepSize(value: Double): this.type = {
    require(ParamValidators.inRange(0, 1, lowerInclusive = false, upperInclusive = true)(


I think it's better to fail at set time rather than when fit is called. This was also not used before, seems we missed it when we deprecated validateParams() ?

sethah · 2016-11-23T17:55:14Z

mllib/src/main/scala/org/apache/spark/ml/tree/treeModels.scala

-  /**
-   * Number of trees in ensemble
-   */
-  val getNumTrees: Int = trees.length


This is a headache. Before, getNumTrees on gbts/rfs would dispatch here. Now, we removed numTrees from RF (it was deprecated) and so now we can have rf inherit from RandomForestParams which (now) has a param numTrees and its associated getter. So if we hadn't removed this, rf would have two getNumTrees methods and one would take precedence. We removed it from here to avoid that, then we had to add them back in the individual GBT subclasses. I think I've got that right.

sethah · 2016-11-23T17:55:56Z

mllib/src/main/scala/org/apache/spark/ml/regression/GBTRegressor.scala

+  /**
+   * Number of trees in ensemble
+   */
+  val getNumTrees: Int = trees.length


Should this have an @Since tag?

sethah · 2016-11-23T17:59:48Z

mllib/src/main/scala/org/apache/spark/ml/tree/treeParams.scala

+   * (default = 20)
+   * @group param
+   */
+  final val numTrees: IntParam = new IntParam(this, "numTrees", "Number of trees to train (>= 1)",


Note: the reason that we cannot add this to both GBT and RF (i.e. in TreeEnsembleParams) is because the param maxIter controls how many trees a GBT has. The semantics in the algos are a bit different.

Good suggestion, added.

sethah · 2016-11-23T18:21:34Z

python/pyspark/ml/util.py

-        """Sets the SQL context to use for saving."""
+        """
+        Sets the SQL context to use for saving.
+        .. note:: Deprecated in 2.1, use session instead.


If we remove it from Scala in 2.2, then we have to remove it here in 2.2 right? I guess we should note it.

sethah · 2016-11-23T18:25:03Z

mllib/src/main/scala/org/apache/spark/ml/feature/ChiSqSelector.scala

-   */
-  @Since("1.6.0")
-  @deprecated("labelCol is not used by ChiSqSelectorModel.", "2.0.0")
-  def setLabelCol(value: String): this.type = set(labelCol, value)


We need to remove it from Python as well.

All python ML models don't support setter/getter params currently, so no need to remove it.

SparkQA · 2016-11-24T06:37:32Z

Test build #69111 has started for PR 15913 at commit 1d8cef5.

SparkQA · 2016-11-24T07:56:44Z

Test build #69108 has finished for PR 15913 at commit 0c85fd4.

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

yanboliang · 2016-11-24T11:03:25Z

Jenkins, test this please.

SparkQA · 2016-11-24T13:36:44Z

Test build #69130 has finished for PR 15913 at commit 1d8cef5.

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

jkbradley

Thanks for the PR! Adding a couple of comments

jkbradley · 2016-11-24T18:32:02Z

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

-   * Validates parameter values stored internally.
-   * Raise an exception if any parameter value is invalid.
-   *
-   * This only needs to check for interactions between parameters.


Could you please add the relevant text from here to transformSchema?

jkbradley · 2016-11-24T18:38:45Z

mllib/src/main/scala/org/apache/spark/ml/tree/treeParams.scala

-  def setStepSize(value: Double): this.type = set(stepSize, value)
-
-  override def validateParams(): Unit = {
+  def setStepSize(value: Double): this.type = {
    require(ParamValidators.inRange(0, 1, lowerInclusive = false, upperInclusive = true)(


This validation should really be in the Param itself. It's not since it's a sharedParam, but perhaps we should just copy the shared Param code here. The problem with putting validation in the setter is that Params can get set in other ways too (such as gbt.set(stepSize, value)

jkbradley · 2016-11-24T18:38:53Z

mllib/src/main/scala/org/apache/spark/ml/tree/treeParams.scala

-      getStepSize), "GBT parameter stepSize should be in interval (0, 1], " +
-      s"but it given invalid value $getStepSize.")
+      value), "GBT parameter stepSize should be in interval (0, 1], " +
+      s"but it given invalid value $value.")


"it given invalid value" -> "it was given an invalid value"

jkbradley · 2016-11-24T18:39:52Z

mllib/src/main/scala/org/apache/spark/ml/tree/treeParams.scala

+  setDefault(numTrees -> 20)
+
+  /** @group setParam */
+  def setNumTrees(value: Int): this.type = set(numTrees, value)


Are these setter methods in traits Java-compatible?

Yeah, we already have setNumTrees which calls super.setNumTrees in RandomForestClassifier and RandomForestRegressor.

jkbradley · 2016-11-24T19:04:45Z

One more thing: This breaks several public APIs, although in mostly minor ways and with good reasons. Could you please update the JIRA to list the public API changes?

SparkQA · 2016-11-25T10:14:14Z

Test build #69158 has finished for PR 15913 at commit 3c8ee2d.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-11-25T10:20:59Z

Test build #69159 has finished for PR 15913 at commit c874d0e.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-11-25T12:32:01Z

Test build #69160 has finished for PR 15913 at commit 157acf9.

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

SparkQA · 2016-11-25T18:51:17Z

Test build #69167 has finished for PR 15913 at commit 864be6e.

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

jkbradley · 2016-11-26T00:10:52Z

mllib/src/main/scala/org/apache/spark/ml/tree/treeParams.scala

+  /** @group getParam */
+  final def getStepSize: Double = $(stepSize)
+
+  /** @group setParam */
  def setStepSize(value: Double): this.type = set(stepSize, value)


I asked about this being Java-friendly because Param setter methods in traits used to have the wrong type in Java. I wonder if this is no longer true. Still, it might make sense to remove the setter method from the trait since it does not make sense to have it in the Model classes. We could put the setter method in each subclass and then deprecate the method in the Model classes.

This issue also shows up in MimaExcludes. If setFeatureSubsetStrategy were put in the concrete classes from the outset, then you would not need to include it in MimaExcludes now.

I'm Ok with doing the change now or in a follow-up PR.

I believe I was the one who incorrectly put the setters in the traits...

Yeah, I understand what you mean. If we would like to correct the setter methods in traits, we involve changes to lots of traits which include DecisionTreeParams, TreeClassifierParams, TreeRegressorParams, RandomForestParams, GBTParams, etc. So i will merge this firstly after it passed Jenkins and address this issue in a separate follow-up PR. Thanks.

@jkbradley I have sent #16017 to fix this issue, please feel free to comment that. Thanks.

yanboliang · 2016-11-26T10:50:17Z

Jenkins, test this please.

SparkQA · 2016-11-26T13:13:46Z

Test build #69183 has finished for PR 15913 at commit 864be6e.

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

yanboliang · 2016-11-26T13:26:11Z

Merged into master and branch-2.1. Thanks for all reviewing.

## What changes were proposed in this pull request? Remove deprecated methods for ML. ## How was this patch tested? Existing tests. Author: Yanbo Liang <ybliang8@gmail.com> Closes #15913 from yanboliang/spark-18481. (cherry picked from commit c4a7eef) Signed-off-by: Yanbo Liang <ybliang8@gmail.com>

## What changes were proposed in this pull request? Mainly two changes: * Move DT/RF/GBT Param setter methods to subclasses. * Deprecate corresponding setter methods in the model classes. See discussion here #15913 (comment). ## How was this patch tested? Existing tests. Author: Yanbo Liang <ybliang8@gmail.com> Closes #16017 from yanboliang/spark-18592. (cherry picked from commit 95f7985) Signed-off-by: Joseph K. Bradley <joseph@databricks.com>

## What changes were proposed in this pull request? Mainly two changes: * Move DT/RF/GBT Param setter methods to subclasses. * Deprecate corresponding setter methods in the model classes. See discussion here #15913 (comment). ## How was this patch tested? Existing tests. Author: Yanbo Liang <ybliang8@gmail.com> Closes #16017 from yanboliang/spark-18592.

## What changes were proposed in this pull request? Remove deprecated methods for ML. ## How was this patch tested? Existing tests. Author: Yanbo Liang <ybliang8@gmail.com> Closes apache#15913 from yanboliang/spark-18481.

## What changes were proposed in this pull request? Mainly two changes: * Move DT/RF/GBT Param setter methods to subclasses. * Deprecate corresponding setter methods in the model classes. See discussion here apache#15913 (comment). ## How was this patch tested? Existing tests. Author: Yanbo Liang <ybliang8@gmail.com> Closes apache#16017 from yanboliang/spark-18592.

## What changes were proposed in this pull request? Remove deprecated methods for ML. ## How was this patch tested? Existing tests. Author: Yanbo Liang <ybliang8@gmail.com> Closes apache#15913 from yanboliang/spark-18481.

## What changes were proposed in this pull request? Mainly two changes: * Move DT/RF/GBT Param setter methods to subclasses. * Deprecate corresponding setter methods in the model classes. See discussion here apache#15913 (comment). ## How was this patch tested? Existing tests. Author: Yanbo Liang <ybliang8@gmail.com> Closes apache#16017 from yanboliang/spark-18592.

Remove deprecated methods for ML.

ecddf15

Add mima excludes.

9a8c926

Fix wrong location of GBT step size check.

13988b4

yanboliang added 2 commits November 19, 2016 07:03

Remove RandomForestClassificationModelParams and RandomForestRegressi…

7beb568

…onModelParams.

Better deprecated docs for estimator/transfomer read/write context fu…

36c2e24

…nction.

Add functions to mima excludes.

8d3c47a

holdenk reviewed Nov 22, 2016

View reviewed changes

sethah reviewed Nov 23, 2016

View reviewed changes

yanboliang added 2 commits November 23, 2016 21:52

Update docs.

0c85fd4

Add test cases for param validation.

1d8cef5

jkbradley reviewed Nov 24, 2016

View reviewed changes

yanboliang added 2 commits November 25, 2016 01:41

Use separate stepSize and add doc for transformSchema

3c8ee2d

Update docs

c874d0e

Add excludes to MimaExcludes

157acf9

Set default value after variable declarion

864be6e

jkbradley reviewed Nov 26, 2016

View reviewed changes

asfgit closed this in c4a7eef Nov 26, 2016

yanboliang deleted the spark-18481 branch November 26, 2016 13:33

yanboliang mentioned this pull request Nov 26, 2016

[SPARK-18592][ML] Move DT/RF/GBT Param setter methods to subclasses #16017

Closed

yanboliang mentioned this pull request May 5, 2017

[SPARK-20606][ML] ML 2.2 QA: Remove deprecated methods for ML #17867

Closed

[SPARK-18481][ML] ML 2.1 QA: Remove deprecated methods for ML #15913

[SPARK-18481][ML] ML 2.1 QA: Remove deprecated methods for ML #15913

Conversation

yanboliang commented Nov 17, 2016

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Nov 17, 2016

SparkQA commented Nov 17, 2016

SparkQA commented Nov 17, 2016

SparkQA commented Nov 18, 2016

SparkQA commented Nov 18, 2016

SparkQA commented Nov 19, 2016

SparkQA commented Nov 19, 2016

yanboliang commented Nov 20, 2016

SparkQA commented Nov 20, 2016

yanboliang commented Nov 21, 2016

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

yanboliang Nov 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sethah 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

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 Nov 24, 2016

SparkQA commented Nov 24, 2016

yanboliang commented Nov 24, 2016

SparkQA commented Nov 24, 2016

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

jkbradley commented Nov 24, 2016

SparkQA commented Nov 25, 2016

SparkQA commented Nov 25, 2016

SparkQA commented Nov 25, 2016

SparkQA commented Nov 25, 2016

Choose a reason for hiding this comment

yanboliang Nov 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanboliang commented Nov 26, 2016

SparkQA commented Nov 26, 2016

yanboliang commented Nov 26, 2016

yanboliang Nov 24, 2016 •

edited

Loading

yanboliang Nov 26, 2016 •

edited

Loading