You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by ludatabricks <gi...@git.apache.org> on 2018/05/02 21:28:36 UTC
[GitHub] spark pull request #21218: [SPARK-24155][ML] Instrument improvements for clu...
GitHub user ludatabricks opened a pull request:
https://github.com/apache/spark/pull/21218
[SPARK-24155][ML] Instrument improvements for clustering
## What changes were proposed in this pull request?
changed the instrument for all of the clustering methods
## How was this patch tested?
N/A
Please review http://spark.apache.org/contributing.html before opening a pull request.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ludatabricks/spark-1 SPARK-23686-1
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/21218.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #21218
----
commit 1810dbe8204981986fa2598b5b022a5c74e43732
Author: Lu WANG <lu...@...>
Date: 2018-05-02T04:11:40Z
instrumentation improvement for clustering
commit 07c20a45737f9a4f008eef6da717034670427483
Author: Lu WANG <lu...@...>
Date: 2018-05-02T21:18:16Z
add more info for instrument
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21218: [SPARK-24155][ML] Instrumentation improvements fo...
Posted by mengxr <gi...@git.apache.org>.
Github user mengxr commented on a diff in the pull request:
https://github.com/apache/spark/pull/21218#discussion_r186926118
--- Diff: mllib/src/main/scala/org/apache/spark/ml/clustering/BisectingKMeans.scala ---
@@ -278,6 +279,7 @@ class BisectingKMeans @Since("2.0.0") (
val summary = new BisectingKMeansSummary(
model.transform(dataset), $(predictionCol), $(featuresCol), $(k))
model.setSummary(Some(summary))
+ instr.logNamedValue("clusterSizes", summary.clusterSizes.mkString(", "))
--- End diff --
This requires parsing the result string if we want to do some analysis. How do we do this in other places? Do we always log in JSON format, e.g.? cc: @WeichenXu123 @jkbradley
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21218: [SPARK-24155][ML] Instrumentation improvements for clust...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21218
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21218: [SPARK-24155][ML] Instrumentation improvements for clust...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21218
**[Test build #90529 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/90529/testReport)** for PR 21218 at commit [`4e2cb81`](https://github.com/apache/spark/commit/4e2cb8141c5a0389fb619f3d423021768de91904).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21218: [SPARK-24155][ML] Instrument improvements for clustering
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21218
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21218: [SPARK-24155][ML] Instrumentation improvements fo...
Posted by mengxr <gi...@git.apache.org>.
Github user mengxr commented on a diff in the pull request:
https://github.com/apache/spark/pull/21218#discussion_r187187071
--- Diff: mllib/src/main/scala/org/apache/spark/ml/clustering/BisectingKMeans.scala ---
@@ -278,6 +279,7 @@ class BisectingKMeans @Since("2.0.0") (
val summary = new BisectingKMeansSummary(
model.transform(dataset), $(predictionCol), $(featuresCol), $(k))
model.setSummary(Some(summary))
+ instr.logNamedValue("clusterSizes", summary.clusterSizes.mkString(", "))
--- End diff --
sg. @ludatabricks could you update the value to `mkString("[", ",", "]")` for now and leave a TODO to extend `logNamedValue`?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21218: [SPARK-24155][ML] Instrumentation improvements for clust...
Posted by mengxr <gi...@git.apache.org>.
Github user mengxr commented on the issue:
https://github.com/apache/spark/pull/21218
LGTM. Merged into master. Thanks!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21218: [SPARK-24155][ML] Instrumentation improvements fo...
Posted by WeichenXu123 <gi...@git.apache.org>.
Github user WeichenXu123 commented on a diff in the pull request:
https://github.com/apache/spark/pull/21218#discussion_r185756220
--- Diff: mllib/src/main/scala/org/apache/spark/ml/clustering/KMeans.scala ---
@@ -378,6 +378,7 @@ class KMeans @Since("1.5.0") (
model.transform(dataset), $(predictionCol), $(featuresCol), $(k))
model.setSummary(Some(summary))
+ instr.logNamedValue("clusterSizes", summary.clusterSizes.toString)
--- End diff --
ditto.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21218: [SPARK-24155][ML] Instrumentation improvements for clust...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21218
**[Test build #90075 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/90075/testReport)** for PR 21218 at commit [`07c20a4`](https://github.com/apache/spark/commit/07c20a45737f9a4f008eef6da717034670427483).
* This patch **fails SparkR unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21218: [SPARK-24155][ML] Instrumentation improvements fo...
Posted by ludatabricks <gi...@git.apache.org>.
Github user ludatabricks commented on a diff in the pull request:
https://github.com/apache/spark/pull/21218#discussion_r185894432
--- Diff: mllib/src/main/scala/org/apache/spark/ml/clustering/GaussianMixture.scala ---
@@ -423,6 +423,8 @@ class GaussianMixture @Since("2.0.0") (
val summary = new GaussianMixtureSummary(model.transform(dataset),
$(predictionCol), $(probabilityCol), $(featuresCol), $(k), logLikelihood)
model.setSummary(Some(summary))
+ instr.logNamedValue("logLikelihood", logLikelihood)
+ instr.logNamedValue("clusterSizes", summary.clusterSizes.toString)
--- End diff --
@WeichenXu123 The function `` clusterSizes.mkString(", ")`` could change the array to a string, separating each String in the array with comma. What do you think?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21218: [SPARK-24155][ML] Instrumentation improvements fo...
Posted by WeichenXu123 <gi...@git.apache.org>.
Github user WeichenXu123 commented on a diff in the pull request:
https://github.com/apache/spark/pull/21218#discussion_r185970925
--- Diff: mllib/src/main/scala/org/apache/spark/ml/clustering/GaussianMixture.scala ---
@@ -423,6 +423,8 @@ class GaussianMixture @Since("2.0.0") (
val summary = new GaussianMixtureSummary(model.transform(dataset),
$(predictionCol), $(probabilityCol), $(featuresCol), $(k), logLikelihood)
model.setSummary(Some(summary))
+ instr.logNamedValue("logLikelihood", logLikelihood)
+ instr.logNamedValue("clusterSizes", summary.clusterSizes.toString)
--- End diff --
also ok I think.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21218: [SPARK-24155][ML] Instrumentation improvements for clust...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21218
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21218: [SPARK-24155][ML] Instrumentation improvements fo...
Posted by WeichenXu123 <gi...@git.apache.org>.
Github user WeichenXu123 commented on a diff in the pull request:
https://github.com/apache/spark/pull/21218#discussion_r185756193
--- Diff: mllib/src/main/scala/org/apache/spark/ml/clustering/GaussianMixture.scala ---
@@ -423,6 +423,8 @@ class GaussianMixture @Since("2.0.0") (
val summary = new GaussianMixtureSummary(model.transform(dataset),
$(predictionCol), $(probabilityCol), $(featuresCol), $(k), logLikelihood)
model.setSummary(Some(summary))
+ instr.logNamedValue("logLikelihood", logLikelihood)
+ instr.logNamedValue("clusterSizes", summary.clusterSizes.toString)
--- End diff --
The `clusterSizes.toString` will get an unreadable object address string.
We need to print the content of the array. I suggest add a method like:
`def logNamedArray[T](array: Array[T]): Unit`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21218: [SPARK-24155][ML] Instrumentation improvements fo...
Posted by jkbradley <gi...@git.apache.org>.
Github user jkbradley commented on a diff in the pull request:
https://github.com/apache/spark/pull/21218#discussion_r187115704
--- Diff: mllib/src/main/scala/org/apache/spark/ml/clustering/BisectingKMeans.scala ---
@@ -278,6 +279,7 @@ class BisectingKMeans @Since("2.0.0") (
val summary = new BisectingKMeansSummary(
model.transform(dataset), $(predictionCol), $(featuresCol), $(k))
model.setSummary(Some(summary))
+ instr.logNamedValue("clusterSizes", summary.clusterSizes.mkString(", "))
--- End diff --
We log Params in JSON format, but we don't have a precedent for logging Array values. I'd be OK with ducking the issue by not logging this value, or with logging it as JSON. If the latter, then we could add a method Instrumentation.logNamedValue taking Array types.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21218: [SPARK-24155][ML] Instrument improvements for clustering
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21218
**[Test build #90075 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/90075/testReport)** for PR 21218 at commit [`07c20a4`](https://github.com/apache/spark/commit/07c20a45737f9a4f008eef6da717034670427483).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21218: [SPARK-24155][ML] Instrumentation improvements for clust...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21218
**[Test build #90152 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/90152/testReport)** for PR 21218 at commit [`9053281`](https://github.com/apache/spark/commit/90532816e2a986b11042719c37cf033272b790f1).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21218: [SPARK-24155][ML] Instrumentation improvements for clust...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21218
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/90075/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21218: [SPARK-24155][ML] Instrumentation improvements for clust...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21218
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21218: [SPARK-24155][ML] Instrumentation improvements for clust...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21218
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/90529/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21218: [SPARK-24155][ML] Instrumentation improvements fo...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/21218
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21218: [SPARK-24155][ML] Instrumentation improvements for clust...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21218
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/90152/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21218: [SPARK-24155][ML] Instrumentation improvements for clust...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21218
**[Test build #90152 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/90152/testReport)** for PR 21218 at commit [`9053281`](https://github.com/apache/spark/commit/90532816e2a986b11042719c37cf033272b790f1).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21218: [SPARK-24155][ML] Instrumentation improvements for clust...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21218
**[Test build #90529 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/90529/testReport)** for PR 21218 at commit [`4e2cb81`](https://github.com/apache/spark/commit/4e2cb8141c5a0389fb619f3d423021768de91904).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org