Skip to content
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

[SPARK-5598][MLLIB] model save/load for ALS #4422

Closed
wants to merge 4 commits into from
Closed

[SPARK-5598][MLLIB] model save/load for ALS #4422

wants to merge 4 commits into from

Conversation

mengxr
Copy link
Contributor

@mengxr mengxr commented Feb 6, 2015

following #4233. @jkbradley

@SparkQA
Copy link

SparkQA commented Feb 6, 2015

Test build #26907 has started for PR 4422 at commit f487cb2.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Feb 6, 2015

Test build #26907 has finished for PR 4422 at commit f487cb2.

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

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/26907/
Test PASSed.

jkbradley
jkbradley reviewed Feb 6, 2015
View reviewed changes
mllib/src/main/scala/org/apache/spark/mllib/recommendation/MatrixFactorizationModel.scala
case (className, "1.0") if className == classNameV1_0 =>
SaveLoadV1_0.load(sc, path)
case _ =>
throw new IOException("" +
Copy link
Member

Choose a reason for hiding this comment

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

Is this the preferred syntax? I've been wondering about this

Copy link
Member

Choose a reason for hiding this comment

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

Here can't you just omit the "" +? maybe it was just auto-inserted by the IDE on hitting return there.

Copy link
Member

Choose a reason for hiding this comment

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

I assume it's to make the lines below line up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's was from IDE.

@jkbradley
Copy link
Member

I just had a few small comments. One question which occurred to me: Does SQLContext have configuration info which would be relevant to saving/loading? I'm wondering if users should be able to pass in a SQLContext to save/load.

@mengxr
Copy link
Contributor Author

mengxr commented Feb 6, 2015

This is the spark.mllib package. We don't expect users to use Spark SQL here.

@SparkQA
Copy link

SparkQA commented Feb 6, 2015

Test build #26959 has started for PR 4422 at commit 2622870.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Feb 6, 2015

Test build #26964 has started for PR 4422 at commit 14b7ea6.

  • This patch merges cleanly.

@jkbradley
Copy link
Member

Having SaveLoadV1_0 be a Loader is weird since it loads metadata, and the metadata gives the version number. It works now since there is only 1 version, but it will have to stop being a Loader in the future.

@jkbradley
Copy link
Member

Other than the Loader issue, looks good.

@SparkQA
Copy link

SparkQA commented Feb 6, 2015

Test build #26964 has finished for PR 4422 at commit 14b7ea6.

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

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/26964/
Test FAILed.

@SparkQA
Copy link

SparkQA commented Feb 6, 2015

Test build #26959 has finished for PR 4422 at commit 2622870.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • "public class " + className + " implements java.io.Serializable
    • case class RegisterExecutor(executorId: String, hostPort: String, cores: Int)
    • throw new SparkException("Failed to load class to register with Kryo", e)
    • class LogisticRegression extends Estimator[LogisticRegressionModel] with LogisticRegressionParams
    • class DoubleParam(parent: Params, name: String, doc: String, defaultValue: Option[Double] = None)
    • class IntParam(parent: Params, name: String, doc: String, defaultValue: Option[Int] = None)
    • class FloatParam(parent: Params, name: String, doc: String, defaultValue: Option[Float] = None)
    • class LongParam(parent: Params, name: String, doc: String, defaultValue: Option[Long] = None)
    • class BooleanParam(parent: Params, name: String, doc: String, defaultValue: Option[Boolean] = None)
    • class EMOptimizer(
    • case class GetField(child: Expression, fieldName: String) extends UnaryExpression
    • class DefaultSource extends RelationProvider
    • case class ParquetRelation2(path: String)(@transient val sqlContext: SQLContext)
    • protected class CaseInsensitiveMap(map: Map[String, String]) extends Map[String, String]
    • trait CreateableRelationProvider

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/26959/
Test PASSed.

@SparkQA
Copy link

SparkQA commented Feb 7, 2015

Test build #27007 has started for PR 4422 at commit ba1ab4e.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Feb 7, 2015

Test build #27007 has finished for PR 4422 at commit ba1ab4e.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/27007/
Test FAILed.

@SparkQA
Copy link

SparkQA commented Feb 7, 2015

Test build #27011 has started for PR 4422 at commit a059394.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Feb 7, 2015

Test build #27011 has finished for PR 4422 at commit a059394.

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

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/27011/
Test PASSed.

asfgit pushed a commit that referenced this pull request Feb 9, 2015
@mengxr
[SPARK-5598][MLLIB] model save/load for ALS
9e4d58f 
following #4233. jkbradley

Author: Xiangrui Meng <meng@databricks.com>

Closes #4422 from mengxr/SPARK-5598 and squashes the following commits:

a059394 [Xiangrui Meng] SaveLoad not extending Loader
14b7ea6 [Xiangrui Meng] address comments
f487cb2 [Xiangrui Meng] add unit tests
62fc43c [Xiangrui Meng] implement save/load for MFM

(cherry picked from commit 5c299c5)
Signed-off-by: Xiangrui Meng <meng@databricks.com>
@mengxr
Copy link
Contributor Author

mengxr commented Feb 9, 2015

Merged into master and branch-1.3.

@asfgit asfgit closed this in 5c299c5 Feb 9, 2015
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.
5 participants
@mengxr @SparkQA @AmplabJenkins @jkbradley @srowen