[SPARK-30630][ML] Remove numTrees in GBT in 3.0.0 #27330

huaxingao · 2020-01-23T02:03:44Z

What changes were proposed in this pull request?

Remove numTrees in GBT in 3.0.0.

Why are the changes needed?

Currently, GBT has

  /**
   * Number of trees in ensemble
   */
  @Since("2.0.0")
  val getNumTrees: Int = trees.length

and

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

I think we should remove one of them. We deprecated it in 2.4.5 via #27352.

Does this PR introduce any user-facing change?

Yes, remove numTrees in GBT in 3.0.0

How was this patch tested?

existing tests

SparkQA · 2020-01-23T03:15:41Z

Test build #117268 has finished for PR 27330 at commit f95744f.

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

dongjoon-hyun · 2020-01-24T03:50:39Z

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

+  /**
+   * Number of trees in ensemble
+   *
+   * @deprecated  Use [[getNumTrees]] instead.  This method will be removed in 3.1.0


Super nit. instead. This -> instead. This?

dongjoon-hyun · 2020-01-24T03:51:15Z

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

+   *
+   * @deprecated  Use [[getNumTrees]] instead.  This method will be removed in 3.1.0
+   */
+  @deprecated("Use getNumTrees instead. This method will be removed in 3.1.0.", "3.0.0")


I'm wondering if we can deprecate this in 2.4.5 and remove in 3.0.0.

so deprecate this in v2.4.5-rc1, right?

It's 2.4.5 because RC is not an official release.
RC1 vote failed and we are preparing 2.4.5 RC2.

Got it. Thanks!

dongjoon-hyun · 2020-01-24T03:52:06Z

@huaxingao . Could you file a JIRA for this?

huaxingao · 2020-01-24T05:51:46Z

@dongjoon-hyun

Could you file a JIRA for this?

Yes, I will open a JIRA. Thank you very much for reviewing this PR.

dongjoon-hyun · 2020-01-24T06:13:33Z

@huaxingao . We need to deprecate this in branch-2.4 first. We can not proceed in the reverse order. Could you make a PR for branch-2.4 too? We will review that first.

I will deprecate it in 2.4.5 and remove it in 3.0.0

SparkQA · 2020-01-24T06:23:17Z

Test build #117337 has finished for PR 27330 at commit 8744e17.

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

huaxingao · 2020-01-24T06:24:55Z

@dongjoon-hyun Yes. I am trying to make a PR for 2.4. I will also fix the MiMa problem.

SparkQA · 2020-01-24T08:05:01Z

Test build #117340 has finished for PR 27330 at commit 0b81823.

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

dongjoon-hyun · 2020-01-24T08:05:55Z

Retest this please.

dongjoon-hyun · 2020-01-24T10:06:13Z

cc @zhengruifeng

SparkQA · 2020-01-24T10:36:02Z

Test build #117345 has finished for PR 27330 at commit 0b81823.

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

srowen · 2020-01-24T13:24:53Z

OK by me. I think there was also no big hurry to deprecate in 2.4 to remove in 3.0, but it's pretty trivial either way.

dongjoon-hyun · 2020-01-24T17:46:33Z

Thank you, @srowen .

dongjoon-hyun · 2020-01-24T20:12:18Z

Merged to master. Thank you, @huaxingao and @srowen .

huaxingao · 2020-01-24T20:18:26Z

Thank you very much! @dongjoon-hyun @srowen

zhengruifeng · 2020-01-25T03:28:23Z

Sorry to come late. LGTM

dongjoon-hyun · 2020-01-25T04:07:10Z

It's not late. Thank you, @zhengruifeng . :)

gatorsmile · 2020-01-29T00:07:28Z

@dongjoon-hyun @zhengruifeng @huaxingao @srowen

We normally do not make such an exception. Can anyone explain how critical it is?

gatorsmile · 2020-01-29T00:16:57Z

Also, I did not even see the migration guide in the PR.

srowen · 2020-01-29T00:20:48Z

Not at all critical, but what's the exception here?

huaxingao · 2020-01-29T00:24:46Z

@gatorsmile It's not critical. It was deprecated in 2.4.5. I will add the release notes.

srowen · 2020-01-29T00:30:07Z

It can't hurt, but I think this is too trivial for a migration guide. It's already in the deprecation message: use method X instead of Y.

dongjoon-hyun · 2020-01-29T04:34:29Z

@gatorsmile . As mentioned in the PR description, we deprecated this at 2.4.5 which will be released before 3.0.0. Do you mean the deprecation in 2.4.5 is also exceptional?

gatorsmile · 2020-01-29T05:49:50Z

Did we deprecate any API in the maintenance releases and then remove the APIs in the next [major] release? Based on my understanding, all the APIs are deprecated in the major releases. Could any of you give me some examples in our previous releases?

dongjoon-hyun · 2020-01-29T06:00:53Z

We don't remove the API at 2.4.6, @gatorsmile . The next of 2.4.5 is 2.4.6.

Did we deprecate any API in the maintenance releases and then remove the APIs in the next release?

gatorsmile · 2020-01-29T06:01:20Z

I mean the next major release.

gatorsmile · 2020-01-29T06:01:55Z

Also, we should never remove any API in the maintenance release

dongjoon-hyun · 2020-01-29T06:02:49Z

We didn't remove any API in the maintenance release.

dongjoon-hyun · 2020-01-29T06:03:08Z

This is removed at 3.0.0 at the major version, isn't it?

dongjoon-hyun · 2020-01-29T06:04:34Z

Let me rephrase your words~ So, only 2.0.0 or 3.0.0 can do deprecation . Then, 3.0.0 or 4.0.0 can do the removal?

gatorsmile · 2020-01-29T06:05:40Z

Let me make my points clear. Did we deprecate any API in the maintenance releases?

For example, did we deprecate an API in 2.3.4 then remove the API in 2.4?

dongjoon-hyun · 2020-01-29T06:11:16Z

SPARK-24918 adds Executor Plugin at 2.4.0.
SPARK-29399 removed it at 3.0.0.

dongjoon-hyun · 2020-01-29T06:14:02Z

SPARK-28780 Delete the incorrect setWeightCol method in LinearSVCModel at 2.3.4, 2.4.4, 3.0.0?

gatorsmile · 2020-01-29T06:14:25Z

Deprecating the APIs in the major/feature releases, like 2.4.0, 2.3.0, 2.2.0, is common; but deprecating the APIs in the maintenance releases, like 2.2.1, 2.2.2, 2.3.4 is rare.

dongjoon-hyun · 2020-01-29T06:15:42Z

We deleted public API even in ~~minor~~ maintenance versions like SPARK-28780.

gatorsmile · 2020-01-29T06:18:25Z

To be honest, this is a big surprise to me. We might need more public discussions for such changes. This will easily break the applications and make our end users frustrated.

dongjoon-hyun · 2020-01-29T06:19:46Z

I totally agree with you. Your guideline also sounds reasonable.

gatorsmile · 2020-01-29T06:29:51Z

Maybe we need some public discussions about the general guideline about the API deprecation/removal in the dev list; and then everyone can follow the same rule in the future?

I do not want to offend anyone. Will not give an example in the dev-list discussion.

dongjoon-hyun · 2020-01-29T06:32:52Z

Please add the above examples. Otherwise, people don't understand what we did. Additionally, please add the following.

[SPARK-28556][SQL] QueryExecutionListener should also notify Error

https://github.com/apache/spark/pull/25292/files

This changes the public API at 3.0.0 last July, but we didn't mentioned it at any 2.4.x releases.

dongjoon-hyun · 2020-01-29T06:34:45Z

I don't think it's an offend. (including your arguments). All comments are valid opinions.

dongjoon-hyun · 2020-01-29T06:36:12Z

I gave +1 for the above one and this PR .

srowen · 2020-01-29T14:27:19Z

The policy, such as it exists, has always been roughly to remove deprecated items in a major release (fine of course), or in a minor release after it's been deprecated for at least one prior minor release (aggressive, but can be fine). Yes, we've done both many times. This is the first situation.

@gatorsmile what do you think the rule has been? I'm not sure I have perceived any other disagreement about that.

This particular change is really minor either way. Deprecating is obviously correct. Whether it's removed in 3.0 or 3.1 doesn't matter much, but, why keep it in 3.0? it's obviously an old API oversight.

[MINOR][ML] Deprecate numTrees in GBT

f95744f

srowen approved these changes Jan 23, 2020

View reviewed changes

dongjoon-hyun reviewed Jan 24, 2020

View reviewed changes

dongjoon-hyun added the ML label Jan 24, 2020

huaxingao changed the title ~~[MINOR][ML] Deprecate numTrees in GBT~~ [SPARK-30630][ML] Deprecate numTrees in GBT Jan 24, 2020

huaxingao changed the title ~~[SPARK-30630][ML] Deprecate numTrees in GBT~~ [SPARK-30630][ML] Remove numTrees in GBT in 3.0.0 Jan 24, 2020

remove numTrees in GBT in 3.0.0

8744e17

fix mima problem

0b81823

dongjoon-hyun approved these changes Jan 24, 2020

View reviewed changes

dongjoon-hyun closed this in 2f8e4d0 Jan 24, 2020

huaxingao deleted the spark-numTrees branch January 24, 2020 20:18

[SPARK-30630][ML] Remove numTrees in GBT in 3.0.0 #27330

[SPARK-30630][ML] Remove numTrees in GBT in 3.0.0 #27330

Conversation

huaxingao commented Jan 23, 2020 • edited by dongjoon-hyun Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

SparkQA commented Jan 23, 2020

dongjoon-hyun Jan 24, 2020

Choose a reason for hiding this comment

dongjoon-hyun Jan 24, 2020

Choose a reason for hiding this comment

huaxingao Jan 24, 2020

Choose a reason for hiding this comment

dongjoon-hyun Jan 24, 2020 • edited Loading

Choose a reason for hiding this comment

huaxingao Jan 24, 2020

Choose a reason for hiding this comment

dongjoon-hyun commented Jan 24, 2020

huaxingao commented Jan 24, 2020

dongjoon-hyun commented Jan 24, 2020 • edited Loading

SparkQA commented Jan 24, 2020

huaxingao commented Jan 24, 2020

SparkQA commented Jan 24, 2020

dongjoon-hyun commented Jan 24, 2020

dongjoon-hyun commented Jan 24, 2020

SparkQA commented Jan 24, 2020

srowen commented Jan 24, 2020

dongjoon-hyun commented Jan 24, 2020

dongjoon-hyun commented Jan 24, 2020

huaxingao commented Jan 24, 2020

zhengruifeng commented Jan 25, 2020

dongjoon-hyun commented Jan 25, 2020 • edited Loading

gatorsmile commented Jan 29, 2020

gatorsmile commented Jan 29, 2020

srowen commented Jan 29, 2020

huaxingao commented Jan 29, 2020

srowen commented Jan 29, 2020

dongjoon-hyun commented Jan 29, 2020

gatorsmile commented Jan 29, 2020 • edited Loading

dongjoon-hyun commented Jan 29, 2020

gatorsmile commented Jan 29, 2020

gatorsmile commented Jan 29, 2020

dongjoon-hyun commented Jan 29, 2020

dongjoon-hyun commented Jan 29, 2020

dongjoon-hyun commented Jan 29, 2020

gatorsmile commented Jan 29, 2020

dongjoon-hyun commented Jan 29, 2020

dongjoon-hyun commented Jan 29, 2020 • edited Loading

gatorsmile commented Jan 29, 2020

dongjoon-hyun commented Jan 29, 2020 • edited Loading

gatorsmile commented Jan 29, 2020

dongjoon-hyun commented Jan 29, 2020

gatorsmile commented Jan 29, 2020

dongjoon-hyun commented Jan 29, 2020

dongjoon-hyun commented Jan 29, 2020

dongjoon-hyun commented Jan 29, 2020

srowen commented Jan 29, 2020

huaxingao commented Jan 23, 2020 •

edited by dongjoon-hyun

Loading

dongjoon-hyun Jan 24, 2020 •

edited

Loading

dongjoon-hyun commented Jan 24, 2020 •

edited

Loading

dongjoon-hyun commented Jan 25, 2020 •

edited

Loading

gatorsmile commented Jan 29, 2020 •

edited

Loading

dongjoon-hyun commented Jan 29, 2020 •

edited

Loading

dongjoon-hyun commented Jan 29, 2020 •

edited

Loading