[SPARK-21027][ML][PYTHON] Added tunable parallelism to one vs. rest in both Scala mllib and Pyspark #19110

WeichenXu123 · 2017-09-03T04:30:15Z

What changes were proposed in this pull request?

Added tunable parallelism to the pyspark implementation of one vs. rest classification. Added a parallelism parameter to the Scala implementation of one vs. rest along with functionality for using the parameter to tune the level of parallelism.

I take this PR #18281 over because the original author is busy but we need merge this PR soon.
After this been merged, we can close #18281 .

How was this patch tested?

Test suite added.

…st classification. Added a parallelism parameter to the scala implementation of one vs. rest for python persistence but have not yet used it to tune the scala parallelism implementation.

…n of the one vs. rest algorithm.

…ts for testing that parallelism doesn't affect the output.

…executor service with a given level of parallelism in a separat trait that OneVsRest inherits from.

…ng OneVsRest and OneVsRest model JavaMLReadable and JavaMLWritable)

SparkQA · 2017-09-03T05:37:10Z

Test build #81350 has finished for PR 19110 at commit 24f4499.

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

WeichenXu123 · 2017-09-03T05:48:18Z

Jenkins, test this please.

SparkQA · 2017-09-03T07:02:11Z

Test build #81352 has finished for PR 19110 at commit 24f4499.

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

SparkQA · 2017-09-04T12:02:58Z

Test build #81381 has finished for PR 19110 at commit fc6fd5e.

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

jkbradley · 2017-09-05T16:17:47Z

LGTM
Pinging on #16774 first to coordinate merging

Btw, if someone else merges this, then @ajaysaini725 and @WeichenXu123 should both be authors, with @ajaysaini725 as the primary one.

BryanCutler

Just a few minor comments, I think left over from the previous PR.

BryanCutler · 2017-09-05T17:52:47Z

python/pyspark/ml/param/_shared_params_code_gen.py

         None, "TypeConverters.toString"),
        ("aggregationDepth", "suggested depth for treeAggregate (>= 2).", "2",
+         "TypeConverters.toInt"),
+        ("parallelism", "number of threads to use when fitting models in parallel (>= 1).", "1",


Maybe this should have a more generic description since it is a shared param?

BryanCutler · 2017-09-05T17:54:45Z

mllib/src/main/scala/org/apache/spark/ml/classification/OneVsRest.scala

+  /**
+   * @group expertSetParam
+   * The implementation of parallel one vs. rest runs the classification for
+   * each class in a separate threads.


from the previous PR, @group expertSetParam should be at the bottom

SparkQA · 2017-09-06T01:34:16Z

Test build #81436 has finished for PR 19110 at commit 7d0849e.

This patch fails Python style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-09-06T02:46:53Z

Test build #81438 has finished for PR 19110 at commit edcf85c.

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

MLnick · 2017-09-06T12:15:28Z

FYI I went ahead and merged #16774 - the doc on the shared param trait is a little more detailed there which I slightly prefer. @WeichenXu123 you will just need to resolve the small merge conflict that introduces.

WeichenXu123 · 2017-09-06T13:15:50Z

@MLnick Conflict resolved. Thanks!

SparkQA · 2017-09-06T14:24:32Z

Test build #81456 has finished for PR 19110 at commit 7a1d404.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
public class NettyMemoryMetrics implements MetricSet

BryanCutler · 2017-09-06T16:48:03Z

LGTM!

WeichenXu123 · 2017-09-09T12:00:58Z

Thanks @MLnick @BryanCutler . Would you mind helping review another similar PR #19122 ? We need some other features but blocking on that PR. Thanks!

jkbradley · 2017-09-11T20:23:07Z

python/pyspark/ml/param/shared.py

+    Mixin for param parallelism: number of threads to use when fitting models in parallel.
+    """
+
+    parallelism = Param(Params._dummy(), "parallelism", "the number of threads to use when running parallel algorithms.", typeConverter=TypeConverters.toInt)


nit: Is this out of date? It's missing the "(>= 1)" from the code gen file.

jkbradley · 2017-09-11T20:23:57Z

Other than that 1 item, this looks ready

SparkQA · 2017-09-12T05:44:55Z

Test build #81653 has finished for PR 19110 at commit c24d4e2.

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

jkbradley · 2017-09-12T17:01:24Z

LGTM
Merging with master
Thanks!

zhengruifeng · 2017-09-13T03:35:58Z

mllib/src/main/scala/org/apache/spark/ml/classification/OneVsRest.scala

+   * each class in a separate threads.
+   *
+   * @group expertSetParam
+   */


missing since annotation

Thanks! I create a PR to fix this.

## What changes were proposed in this pull request? add missing since tag for `setParallelism` in apache#19110 ## How was this patch tested? N/A Author: WeichenXu <[email protected]> Closes apache#19214 from WeichenXu123/minor01.

ajaysaini725 and others added 20 commits June 12, 2017 14:46

Added tunable parallelism to the pyspark implementation of one vs. re…

b69f201

…st classification. Added a parallelism parameter to the scala implementation of one vs. rest for python persistence but have not yet used it to tune the scala parallelism implementation.

Fixed python style.

e750d3e

Added functionality for tuning parellelism in the Scala implementatio…

81d458b

…n of the one vs. rest algorithm.

Fixed code according to comments. Added both annotations and unit tes…

2133378

…ts for testing that parallelism doesn't affect the output.

Modified parallel one vs rest to use futures.

c59b1d8

Put the parallelism parameter as well as the function for getting an …

5f635a2

…executor service with a given level of parallelism in a separat trait that OneVsRest inherits from.

Responded to pull request comments.

4431ffc

Made changes based on pull request comments.

a841b3e

Fixed based on pull request comments

a95a8af

Fixed based on comments

d45bc23

Reverting merge and adding change that would fix merge conflict (maki…

30ac62d

…ng OneVsRest and OneVsRest model JavaMLReadable and JavaMLWritable)

Merge branch 'master' into spark-21027

cc634d2

Style fix with docstring

ce14172

Fixed based on comments.

1c9de16

Fixed style issue.

9f34404

Fixed merge conflict

585a3f8

Fixed remaining part of merge conflict.

f65381a

Fixed style problem

2a335fe

Merge branch 'master' into spark-21027

049f371

address review feedback issues

ddc2ff4

WeichenXu123 mentioned this pull request Sep 3, 2017

[SPARK-21027][ML][PYTHON] Added tunable parallelism to one vs. rest in both Scala mllib and Pyspark #18281

Closed

update migration guide

fc6fd5e

WeichenXu123 force-pushed the spark-21027 branch from 24f4499 to fc6fd5e Compare September 4, 2017 09:26

jkbradley mentioned this pull request Sep 5, 2017

[SPARK-19357][ML] Adding parallel model evaluation in ML tuning #16774

Closed

BryanCutler reviewed Sep 5, 2017

View reviewed changes

update desc

7d0849e

fix style

edcf85c

WeichenXu123 mentioned this pull request Sep 6, 2017

[SPARK-21911][ML][PySpark] Parallel Model Evaluation for ML Tuning in PySpark #19122

Closed

merge master & resolve conflicts

7a1d404

jkbradley reviewed Sep 11, 2017

View reviewed changes

update out-of-date shared.py

c24d4e2

asfgit closed this in 720c94f Sep 12, 2017

WeichenXu123 deleted the spark-21027 branch September 13, 2017 01:49

zhengruifeng reviewed Sep 13, 2017

View reviewed changes

WeichenXu123 mentioned this pull request Sep 13, 2017

[SPARK-21027][MINOR][FOLLOW-UP] add missing since tag #19214

Closed

[SPARK-21027][ML][PYTHON] Added tunable parallelism to one vs. rest in both Scala mllib and Pyspark #19110

[SPARK-21027][ML][PYTHON] Added tunable parallelism to one vs. rest in both Scala mllib and Pyspark #19110

Uh oh!

Conversation

WeichenXu123 commented Sep 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Sep 3, 2017

Uh oh!

WeichenXu123 commented Sep 3, 2017

Uh oh!

SparkQA commented Sep 3, 2017

Uh oh!

SparkQA commented Sep 4, 2017

Uh oh!

jkbradley commented Sep 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BryanCutler left a comment

Choose a reason for hiding this comment

Uh oh!

BryanCutler Sep 5, 2017

Choose a reason for hiding this comment

Uh oh!

BryanCutler Sep 5, 2017

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Sep 6, 2017

Uh oh!

SparkQA commented Sep 6, 2017

Uh oh!

MLnick commented Sep 6, 2017

Uh oh!

WeichenXu123 commented Sep 6, 2017

Uh oh!

SparkQA commented Sep 6, 2017

Uh oh!

BryanCutler commented Sep 6, 2017

Uh oh!

WeichenXu123 commented Sep 9, 2017

Uh oh!

jkbradley Sep 11, 2017

Choose a reason for hiding this comment

Uh oh!

jkbradley commented Sep 11, 2017

Uh oh!

SparkQA commented Sep 12, 2017

Uh oh!

jkbradley commented Sep 12, 2017

Uh oh!

zhengruifeng Sep 13, 2017

Choose a reason for hiding this comment

Uh oh!

WeichenXu123 Sep 13, 2017

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

WeichenXu123 commented Sep 3, 2017 •

edited

Loading

jkbradley commented Sep 5, 2017 •

edited

Loading