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

[yanboliang]

What changes were proposed in this pull request?

Remove ML methods we deprecated in 2.1.

How was this patch tested?

Existing tests.

[SparkQA]

Test build #76479 has finished for PR 17867 at commit 4922c03.

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

[srowen]

Hm, isn't it sudden to remove deprecated methods after 1 minor release?
Also why does this not remove the methods from subclasses? if they're still needed by implementations, then does it make sense to deprecate/remove them?

[yanboliang]

@srowen These methods were deprecated when 2.1 releasing, and we usually remove methods deprecated in last feature/minor release (when 2.1 releasing, we remove methods deprecated in 2.0, see #15913). We only remove the setter methods from the trait, which cause them be remove from the model classes who extend them. Since it does not make sense to have it in the model classes, and actually we never use them and mark them deprecated for one minor release cycle. You can refer the discussion here . Thanks.

[yanboliang]

@srowen Any more comments?

[srowen]

I do see methods in the code that are deprecated yet retained for a few minor releases -- generally I'd expect we only actually remove them in a major release because it is a breaking change for any caller. I guess that's my primary question: is this binary/source compatible?

[yanboliang]

Yeah, that's because we didn't always do that in time, especially for spark.mllib packages which has been in maintenance mode. But for spark.ml package, I think we always keep to remove methods deprecated in last feature/minor release. And we have already warned This method is deprecated and will be removed in 2.2.0 in API documents, it may confuse users if we don't take action follow the API annotation. I'm prefer to merge this as the old convention, and start the new convention in the following release which we should add annotation like deprecated and will be removed in 3.0.0. What do you think about it? Thanks. cc @jkbradley

[yanboliang]

I'll merge this to catch 2.2, and update user guide to reflect this change as usual, since this is one of the ML QA tasks which blocks the 2.2 release. If there are more comments, I can address them in follow-up work. Thanks.

[jkbradley]

I actually think @srowen has a good point that we should maintain more stable minor releases. I'd be in support of reverting this patch and changing the deprecation comments to say the items will be removed in "3.0.0" instead of 2.x minor versions. Is that OK with you @yanboliang ?

Note: I also noticed that MLWriter.context does not appear as deprecated in the API docs because the deprecated implementation is overridden with a non-deprecated method. (That was my bad for committing that PR.)

[yanboliang]

I'm OK to revert this change and change the deprecation comments to say the items will be removed in 3.0.0 instead of 2.x. Will do them in two follow-up PRs. Thanks.

[yanboliang]

I sent #17946 to change the deprecation comments to say the items will be removed in 3.0.0 instead of 2.x, please feel free to review and comments. @srowen@jkbradley