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.

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

[SPARK-6141][MLlib] Upgrade Breeze from 0.10 to 0.11 to fix convergence bug #4879

Closed
wants to merge 4 commits into from
Closed

Conversation

dbtsai
Copy link
Member

@dbtsai dbtsai commented Mar 3, 2015

LBFGS and OWLQN in Breeze 0.10 has convergence check bug.
This is fixed in 0.11, see the description in Breeze project for detail:

scalanlp/breeze#373 (comment)

@SparkQA
Copy link

SparkQA commented Mar 3, 2015

Test build #28238 has started for PR 4879 at commit 397a208.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Mar 3, 2015

Test build #28238 has finished for PR 4879 at commit 397a208.

  • 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/28238/
Test FAILed.

@coderxiang
Copy link
Contributor

Good job!
Do we have enough time to catch the release, especially if there are some incompatible APIs.
For my case, the coefficients differ but the performance (accuracy, auc) are similar. Might need to evaluate the trade-off here if the change is going to 1.3

@dbtsai
Copy link
Member Author

dbtsai commented Mar 3, 2015

@coderxiang Breeze seems to accidentally remove the public constructor of CSCMatrix, and we have a PR to Breeze to address it. Let's see if we can make it.

@srowen
Copy link
Member

srowen commented Mar 3, 2015

In the meantime, I do not think a 1.3 release should block on this, if 0.11 isn't quite suitable. The issue being fixed is minor-ish and I'm concerned other more subtle things might go wrong. It might be a little risky to rush this at the last minute.

@SparkQA
Copy link

SparkQA commented Mar 3, 2015

Test build #28245 has started for PR 4879 at commit d848f65.

  • This patch merges cleanly.

@dbtsai
Copy link
Member Author

dbtsai commented Mar 3, 2015

This is the fix in breeze side for missing public constructor of CSCMatrix scalanlp/breeze#375

@SparkQA
Copy link

SparkQA commented Mar 3, 2015

Test build #28245 has finished for PR 4879 at commit d848f65.

  • 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/28245/
Test FAILed.

@mengxr
Copy link
Contributor

mengxr commented Mar 4, 2015

test this please

@SparkQA
Copy link

SparkQA commented Mar 4, 2015

Test build #28255 has started for PR 4879 at commit d848f65.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Mar 4, 2015

Test build #28255 has finished for PR 4879 at commit d848f65.

  • 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/28255/
Test PASSed.

asfgit pushed a commit that referenced this pull request Mar 4, 2015
…ce bug

LBFGS and OWLQN in Breeze 0.10 has convergence check bug.
This is fixed in 0.11, see the description in Breeze project for detail:

scalanlp/breeze#373 (comment)

Author: Xiangrui Meng <[email protected]>
Author: DB Tsai <[email protected]>
Author: DB Tsai <[email protected]>

Closes #4879 from dbtsai/breeze and squashes the following commits:

d848f65 [DB Tsai] Merge pull request #1 from mengxr/AlpineNow-breeze
c2ca6ac [Xiangrui Meng] upgrade to breeze-0.11.1
35c2f26 [Xiangrui Meng] fix LRSuite
397a208 [DB Tsai] upgrade breeze

(cherry picked from commit 76e20a0)
Signed-off-by: Xiangrui Meng <[email protected]>
@mengxr
Copy link
Contributor

mengxr commented Mar 4, 2015

LGTM. Merged into master and branch-1.3. Thanks!

@asfgit asfgit closed this in 76e20a0 Mar 4, 2015
@dbtsai dbtsai deleted the breeze branch March 4, 2015 19:24
@nishkamravi2
Copy link
Contributor

@mengxr @srowen This change introduces a 10% regression in K-Means (SPARK-6234). We seem to have the following options:

  1. Absorb the perf regression
  2. Find the problem in Breeze and fix it while retaining 0.11 in Spark
  3. Revert back to 0.10 (potentially open a JIRA for Breeze and upgrade to 0.11 when fixed)

What do you guys think?

@mengxr
Copy link
Contributor

mengxr commented Mar 9, 2015

@nishkamravi2 I don't think the k-means implementation uses breeze ops now, but I may be wrong. Which versions/hashtags did you test?

@nishkamravi2
Copy link
Contributor

@mengxr The version that I run uses Breeze. Btw, a re-compile is required as well (which is a regression of sorts but probably ignorable).

@mengxr
Copy link
Contributor

mengxr commented Mar 9, 2015

@nishkamravi2 Let's continue the discussion on the JIRA you created.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants