Skip to content

[SPARK-6685][ML]Use DSYRK to compute AtA in ALS - #19536

Closed
mpjlu wants to merge 14 commits into
apache:masterfrom
mpjlu:6658
Closed

[SPARK-6685][ML]Use DSYRK to compute AtA in ALS#19536
mpjlu wants to merge 14 commits into
apache:masterfrom
mpjlu:6658

Conversation

@mpjlu

@mpjlumpjlu commented Oct 19, 2017

Copy link
Copy Markdown

What changes were proposed in this pull request?

This is a reopen of PR: #13891 with mima fix and code optimization.
There is about 30-40% performance improvement comparing with #13891

How was this patch tested?

(Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests)
(If this patch involves UI changes, please attach a screenshot; otherwise, remove this)

Please review http://spark.apache.org/contributing.html before opening a pull request.

@mpjlu

Copy link
Copy Markdown
Author

Hi @yanboliang , I reopen this PR per your suggestion, thanks.
I have tested the code, the performance result is matched with @hqzizania 's results.
Thanks @hqzizania .

@hqzizania

Copy link
Copy Markdown
Contributor

Wow, thank you for reopening. LOL @mpjlu

def setColdStartStrategy(value: String): this.type = set(coldStartStrategy, value)

@Since("2.3.0")
def setThreshold(value: Int): this.type = set(threshold, value)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure callers can meaningfully understand and set this. Can't we pick a threshold programmatically?

@mpjlumpjluOct 19, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, my test result is better than the previous result, especially for native BLAS. I will update my test results here soon, and I will change this set.

ProblemFilters.exclude[ReversedMissingMethodProblem]("org.apache.spark.ml.linalg.Vector.toSparseWithSize")
) ++ Seq(
// [SPARK-6685][ML]Use DSYRK to compute AtA in ALS
ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.spark.ml.recommendation.ALS.train")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This breaks an API unnecessarily, even though it's a dev API. I think we instead need to remove this as a user-facing param and avoid it altogether.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I am ok to remove this, and use a loose threshold (e.g. 100), which is helpful for most cases. How about it?

@SparkQA

Copy link
Copy Markdown

Test build #82905 has finished for PR 19536 at commit 4fdcbe0.

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

@mpjlu

Copy link
Copy Markdown
Author

image

100k*100k, sparsity 0.05oldoldNew
rankf2jmklmkl
20494
50131213
100406030
2001569066
500900720270

This is the performance data of this PR.
The date size is 100,000*100,000, the data sparsity is 0.05.
The test environment is:
3 workers: 35 cores/worker, 210G memory/worker, 1 executor/worker.
The native BLAS is Intel MKL

@mpjlu

Copy link
Copy Markdown
Author

cc @srowen@mengxr@yanboliang
The performance data is updated, thanks.

@mpjlu

Copy link
Copy Markdown
Author

Because I don't have the environment to continue this work, I will close it. Thanks.

@mpjlumpjlu closed this Jan 15, 2018
Sign up for freeto 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.

4 participants

@mpjlu@hqzizania@SparkQA@srowen