Skip to content

[SPARK-16240][ML] ML persistence backward compatibility for LDA #15034

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
jkbradley wants to merge 3 commits into from
Closed

[SPARK-16240][ML] ML persistence backward compatibility for LDA #15034

jkbradley wants to merge 3 commits into from

Conversation

@jkbradley
Copy link
Member

@jkbradley jkbradley commented Sep 9, 2016

What changes were proposed in this pull request?

Allow Spark 2.x to load instances of LDA, LocalLDAModel, and DistributedLDAModel saved from Spark 1.6.

How was this patch tested?

I tested this manually, saving the 3 types from 1.6 and loading them into master (2.x). In the future, we can add generic tests for testing backwards compatibility across all ML models in SPARK-15573.

@jkbradley
Copy link
Member Author

jkbradley commented Sep 9, 2016

To reviewers: This code was taken and modified from [https://github.com//pull/14112]. @GayathriMurali should be the primary author when we merge this into master and branch-2.0

@jkbradley
Copy link
Member Author

jkbradley commented Sep 9, 2016

CC @hhbyyh Would you mind taking a look? thanks!

@SparkQA
Copy link

SparkQA commented Sep 9, 2016

Test build #65168 has finished for PR 15034 at commit a931b25.

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

hhbyyh
hhbyyh reviewed Sep 10, 2016
View reviewed changes
mllib/src/main/scala/org/apache/spark/ml/clustering/LDA.scala
*/
def getAndSetParams(model: LDAParams, metadata: Metadata): Unit = {
VersionUtils.majorMinorVersion(metadata.sparkVersion) match {
case (1, 6) =>
Copy link
Contributor

@hhbyyh hhbyyh Sep 10, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we didn't support LDA serialization in 1.5, right?

Copy link
Member Author

@jkbradley jkbradley Sep 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, it was 1.6

@hhbyyh
Copy link
Contributor

hhbyyh commented Sep 10, 2016

Thanks @jkbradley. This matches what I had in mind. LGTM.

@jkbradley
Copy link
Member Author

jkbradley commented Sep 19, 2016

Thanks @hhbyyh !

@SparkQA
Copy link

SparkQA commented Sep 19, 2016

Test build #3279 has finished for PR 15034 at commit a931b25.

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

@JoshRosen
Copy link
Contributor

JoshRosen commented Sep 20, 2016

Jenkins, retest this please.

@SparkQA
Copy link

SparkQA commented Sep 20, 2016

Test build #65621 has finished for PR 15034 at commit a931b25.

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

@JoshRosen
Copy link
Contributor

JoshRosen commented Sep 20, 2016

@jkbradley, it looks like this is legitimately failing MiMa (not sure why it passed on the first run...):

[error]  * the type hierarchy of object org.apache.spark.ml.clustering.LDA is different in current version. Missing types {org.apache.spark.ml.util.DefaultParamsReadable}
[error]    filter with: ProblemFilters.exclude[MissingTypesProblem]("org.apache.spark.ml.clustering.LDA$")

@jkbradley
Copy link
Member Author

jkbradley commented Sep 22, 2016

@JoshRosen That is weird; MiMa passes for me locally, but I see that it shouldn't. I added a MiMaException; this should not be a problem for users.

@SparkQA
Copy link

SparkQA commented Sep 22, 2016

Test build #65789 has finished for PR 15034 at commit 3b499df.

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

@jkbradley
Copy link
Member Author

jkbradley commented Sep 22, 2016

I'll go ahead and merge this. Thanks @hhbyyh for reviewing it!

@asfgit asfgit closed this in f4f6bd8 Sep 22, 2016
jkbradley pushed a commit to jkbradley/spark that referenced this pull request Sep 22, 2016
@GayathriMurali @jkbradley
[SPARK-16240][ML] ML persistence backward compatibility for LDA
2a95b5c 
Allow Spark 2.x to load instances of LDA, LocalLDAModel, and DistributedLDAModel saved from Spark 1.6.

I tested this manually, saving the 3 types from 1.6 and loading them into master (2.x).  In the future, we can add generic tests for testing backwards compatibility across all ML models in SPARK-15573.

Author: Joseph K. Bradley <joseph@databricks.com>

Closes apache#15034 from jkbradley/lda-backwards.
@jkbradley jkbradley mentioned this pull request Sep 22, 2016
Closed
@jkbradley jkbradley deleted the lda-backwards branch September 22, 2016 23:43
asfgit pushed a commit that referenced this pull request Sep 23, 2016
@GayathriMurali @jkbradley
[SPARK-16240][ML] ML persistence backward compatibility for LDA - 2.0…
54d4eee 
… backport

## What changes were proposed in this pull request?

Allow Spark 2.x to load instances of LDA, LocalLDAModel, and DistributedLDAModel saved from Spark 1.6.
Backport of #15034 for branch-2.0

## How was this patch tested?

I tested this manually, saving the 3 types from 1.6 and loading them into master (2.x).  In the future, we can add generic tests for testing backwards compatibility across all ML models in SPARK-15573.

Author: Gayathri Murali <gayathri.m.softie@gmail.com>
Author: Joseph K. Bradley <joseph@databricks.com>

Closes #15205 from jkbradley/lda-backward-2.0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jkbradley @SparkQA @hhbyyh @JoshRosen