You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by MrBago <gi...@git.apache.org> on 2018/07/05 18:26:54 UTC
[GitHub] spark pull request #21719: [SPARK-24747] Make Instrumentation class more fle...
GitHub user MrBago opened a pull request:
https://github.com/apache/spark/pull/21719
[SPARK-24747] Make Instrumentation class more flexible
## What changes were proposed in this pull request?
This PR updates the Instrumentation class to make it more flexible and a little bit easier to use. When these APIs are merged, I'll followup with a PR to update the training code to use these new APIs so we can remove the old APIs. These changes are all to private APIs so this PR doesn't make any user facing changes.
## How was this patch tested?
Existing tests.
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/MrBago/spark new-instrumentation-apis
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/21719.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 #21719
----
commit 03c9b0a9fa664171c3a6a8264fe04b3488dae4e4
Author: Bago Amirbekian <ba...@...>
Date: 2018-06-25T22:52:18Z
Added Instrumentation.instrumented API with required changes to
Instrumentation class. Updated LogisticRegression to use this API as an
example.
commit 3a6537d2861c1f6dd65b717772eb4c1f2dc7c174
Author: Bago Amirbekian <ba...@...>
Date: 2018-07-05T18:16:16Z
Allow `instrumented` method to return any type.
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21719: [SPARK-24747][ML] Make Instrumentation class more flexib...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21719
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 #21719: [SPARK-24747][ML] Make Instrumentation class more flexib...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21719
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 #21719: [SPARK-24747][ML] Make Instrumentation class more flexib...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21719
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 #21719: [SPARK-24747] Make Instrumentation class more flexible
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21719
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 #21719: [SPARK-24747][ML] Make Instrumentation class more flexib...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21719
**[Test build #93189 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/93189/testReport)** for PR 21719 at commit [`1676a6d`](https://github.com/apache/spark/commit/1676a6d8cd415ac1c25f39db8184e1b8fb99edf0).
* 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 #21719: [SPARK-24747][ML] Make Instrumentation class more flexib...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21719
**[Test build #93189 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/93189/testReport)** for PR 21719 at commit [`1676a6d`](https://github.com/apache/spark/commit/1676a6d8cd415ac1c25f39db8184e1b8fb99edf0).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21719: [SPARK-24747][ML] Make Instrumentation class more flexib...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21719
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/93189/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21719: [SPARK-24747][ML] Make Instrumentation class more flexib...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21719
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 #21719: [SPARK-24747] Make Instrumentation class more flexible
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21719
**[Test build #92653 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/92653/testReport)** for PR 21719 at commit [`3a6537d`](https://github.com/apache/spark/commit/3a6537d2861c1f6dd65b717772eb4c1f2dc7c174).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21719: [SPARK-24747][ML] Make Instrumentation class more flexib...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21719
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/885/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21719: [SPARK-24747][ML] Make Instrumentation class more flexib...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21719
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/1064/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21719: [SPARK-24747][ML] Make Instrumentation class more flexib...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21719
**[Test build #92948 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/92948/testReport)** for PR 21719 at commit [`b98d772`](https://github.com/apache/spark/commit/b98d77284f2397daef2623c012d34ff50a25f498).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21719: [SPARK-24747][ML] Make Instrumentation class more flexib...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21719
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/92906/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21719: [SPARK-24747][ML] Make Instrumentation class more...
Posted by jkbradley <gi...@git.apache.org>.
Github user jkbradley commented on a diff in the pull request:
https://github.com/apache/spark/pull/21719#discussion_r202800969
--- Diff: mllib/src/main/scala/org/apache/spark/ml/classification/LogisticRegression.scala ---
@@ -488,9 +488,10 @@ class LogisticRegression @Since("1.2.0") (
train(dataset, handlePersistence)
}
+ import Instrumentation.instrumented
--- End diff --
Put import at top of file with the other imports (just to make imports easier to track).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21719: [SPARK-24747] Make Instrumentation class more fle...
Posted by mengxr <gi...@git.apache.org>.
Github user mengxr commented on a diff in the pull request:
https://github.com/apache/spark/pull/21719#discussion_r200499164
--- Diff: mllib/src/main/scala/org/apache/spark/ml/util/Instrumentation.scala ---
@@ -169,22 +204,33 @@ private[spark] object Instrumentation {
val varianceOfLabels = "varianceOfLabels"
}
+ // TODO: Remove these
/**
* Creates an instrumentation object for a training session.
*/
- def create[E <: Estimator[_]](
- estimator: E, dataset: Dataset[_]): Instrumentation[E] = {
- create[E](estimator, dataset.rdd)
+ def create(estimator: Estimator[_], dataset: Dataset[_]): Instrumentation = {
+ create(estimator, dataset.rdd)
}
/**
* Creates an instrumentation object for a training session.
*/
- def create[E <: Estimator[_]](
- estimator: E, dataset: RDD[_]): Instrumentation[E] = {
- new Instrumentation[E](estimator, dataset)
+ def create(estimator: Estimator[_], dataset: RDD[_]): Instrumentation = {
+ new Instrumentation(estimator, dataset)
+ }
+ // end remove
+
+ def instrumented[T](body: (Instrumentation => T)): T = {
+ val instr = new Instrumentation()
+ Try(body(new Instrumentation())) match {
--- End diff --
use already constructed `instr`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21719: [SPARK-24747][ML] Make Instrumentation class more flexib...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21719
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 #21719: [SPARK-24747] Make Instrumentation class more flexible
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21719
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 #21719: [SPARK-24747][ML] Make Instrumentation class more flexib...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21719
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 pull request #21719: [SPARK-24747] Make Instrumentation class more fle...
Posted by mengxr <gi...@git.apache.org>.
Github user mengxr commented on a diff in the pull request:
https://github.com/apache/spark/pull/21719#discussion_r200499315
--- Diff: mllib/src/main/scala/org/apache/spark/ml/util/Instrumentation.scala ---
@@ -169,22 +204,33 @@ private[spark] object Instrumentation {
val varianceOfLabels = "varianceOfLabels"
}
+ // TODO: Remove these
/**
* Creates an instrumentation object for a training session.
*/
- def create[E <: Estimator[_]](
- estimator: E, dataset: Dataset[_]): Instrumentation[E] = {
- create[E](estimator, dataset.rdd)
+ def create(estimator: Estimator[_], dataset: Dataset[_]): Instrumentation = {
+ create(estimator, dataset.rdd)
}
/**
* Creates an instrumentation object for a training session.
*/
- def create[E <: Estimator[_]](
- estimator: E, dataset: RDD[_]): Instrumentation[E] = {
- new Instrumentation[E](estimator, dataset)
+ def create(estimator: Estimator[_], dataset: RDD[_]): Instrumentation = {
+ new Instrumentation(estimator, dataset)
+ }
+ // end remove
+
+ def instrumented[T](body: (Instrumentation => T)): T = {
+ val instr = new Instrumentation()
+ Try(body(new Instrumentation())) match {
+ case Failure(NonFatal(e)) =>
+ instr.logFailure(e)
+ throw e
+ case Success(model) =>
--- End diff --
`model` -> `result`, it doesn't need to be a model
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21719: [SPARK-24747] Make Instrumentation class more fle...
Posted by mengxr <gi...@git.apache.org>.
Github user mengxr commented on a diff in the pull request:
https://github.com/apache/spark/pull/21719#discussion_r200498732
--- Diff: mllib/src/main/scala/org/apache/spark/ml/util/Instrumentation.scala ---
@@ -148,12 +170,25 @@ private[spark] class Instrumentation[E <: Estimator[_]] private (
}
+ // TODO: Remove this (possibly replace with logModel?)
/**
* Logs the successful completion of the training session.
*/
def logSuccess(model: Model[_]): Unit = {
log(s"training finished")
}
+
+ def logSuccess(): Unit = {
+ log("training finished")
--- End diff --
We shouldn't have this `log` alias. I was wondering which log level it uses. Just use `logInfo` and remove `log(`.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21719: [SPARK-24747] Make Instrumentation class more fle...
Posted by mengxr <gi...@git.apache.org>.
Github user mengxr commented on a diff in the pull request:
https://github.com/apache/spark/pull/21719#discussion_r200495412
--- Diff: mllib/src/main/scala/org/apache/spark/ml/classification/LogisticRegression.scala ---
@@ -490,7 +490,7 @@ class LogisticRegression @Since("1.2.0") (
protected[spark] def train(
dataset: Dataset[_],
- handlePersistence: Boolean): LogisticRegressionModel = {
+ handlePersistence: Boolean): LogisticRegressionModel = Instrumentation.instrumented { instr =>
--- End diff --
To avoid line too wide, we might want to import `instrumented` and save "Instrumentation" from this line.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21719: [SPARK-24747][ML] Make Instrumentation class more flexib...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21719
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/92948/
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 #21719: [SPARK-24747][ML] Make Instrumentation class more...
Posted by MrBago <gi...@git.apache.org>.
Github user MrBago commented on a diff in the pull request:
https://github.com/apache/spark/pull/21719#discussion_r203126526
--- Diff: mllib/src/main/scala/org/apache/spark/ml/util/Instrumentation.scala ---
@@ -19,45 +19,60 @@ package org.apache.spark.ml.util
import java.util.UUID
-import scala.reflect.ClassTag
+import scala.util.{Failure, Success, Try}
+import scala.util.control.NonFatal
import org.json4s._
import org.json4s.JsonDSL._
import org.json4s.jackson.JsonMethods._
import org.apache.spark.internal.Logging
-import org.apache.spark.ml.{Estimator, Model}
-import org.apache.spark.ml.param.Param
+import org.apache.spark.ml.{Estimator, Model, PipelineStage}
+import org.apache.spark.ml.param.{Param, Params}
import org.apache.spark.rdd.RDD
import org.apache.spark.sql.Dataset
import org.apache.spark.util.Utils
/**
* A small wrapper that defines a training session for an estimator, and some methods to log
* useful information during this session.
- *
- * A new instance is expected to be created within fit().
- *
- * @param estimator the estimator that is being fit
- * @param dataset the training dataset
- * @tparam E the type of the estimator
*/
-private[spark] class Instrumentation[E <: Estimator[_]] private (
- val estimator: E,
- val dataset: RDD[_]) extends Logging {
+private[spark] class Instrumentation extends Logging {
private val id = UUID.randomUUID()
- private val prefix = {
+ private val shortId = id.toString.take(8)
+ private val prefix = s"[$shortId] "
+
+ // TODO: update spark.ml to use new Instrumentation APIs and remove this constructor
+ var stage: Params = _
--- End diff --
Yep, the plan is to remove stage once we port switch over to the new APIs
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21719: [SPARK-24747][ML] Make Instrumentation class more...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/21719
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21719: [SPARK-24747] Make Instrumentation class more flexible
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21719
**[Test build #92653 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/92653/testReport)** for PR 21719 at commit [`3a6537d`](https://github.com/apache/spark/commit/3a6537d2861c1f6dd65b717772eb4c1f2dc7c174).
* 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 #21719: [SPARK-24747][ML] Make Instrumentation class more flexib...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21719
**[Test build #92948 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/92948/testReport)** for PR 21719 at commit [`b98d772`](https://github.com/apache/spark/commit/b98d77284f2397daef2623c012d34ff50a25f498).
* 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 #21719: [SPARK-24747] Make Instrumentation class more flexible
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21719
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/92653/
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 #21719: [SPARK-24747][ML] Make Instrumentation class more...
Posted by jkbradley <gi...@git.apache.org>.
Github user jkbradley commented on a diff in the pull request:
https://github.com/apache/spark/pull/21719#discussion_r202805710
--- Diff: mllib/src/main/scala/org/apache/spark/ml/util/Instrumentation.scala ---
@@ -19,45 +19,60 @@ package org.apache.spark.ml.util
import java.util.UUID
-import scala.reflect.ClassTag
+import scala.util.{Failure, Success, Try}
+import scala.util.control.NonFatal
import org.json4s._
import org.json4s.JsonDSL._
import org.json4s.jackson.JsonMethods._
import org.apache.spark.internal.Logging
-import org.apache.spark.ml.{Estimator, Model}
-import org.apache.spark.ml.param.Param
+import org.apache.spark.ml.{Estimator, Model, PipelineStage}
+import org.apache.spark.ml.param.{Param, Params}
import org.apache.spark.rdd.RDD
import org.apache.spark.sql.Dataset
import org.apache.spark.util.Utils
/**
* A small wrapper that defines a training session for an estimator, and some methods to log
* useful information during this session.
- *
- * A new instance is expected to be created within fit().
- *
- * @param estimator the estimator that is being fit
- * @param dataset the training dataset
- * @tparam E the type of the estimator
*/
-private[spark] class Instrumentation[E <: Estimator[_]] private (
- val estimator: E,
- val dataset: RDD[_]) extends Logging {
+private[spark] class Instrumentation extends Logging {
private val id = UUID.randomUUID()
- private val prefix = {
+ private val shortId = id.toString.take(8)
+ private val prefix = s"[$shortId] "
+
+ // TODO: update spark.ml to use new Instrumentation APIs and remove this constructor
+ var stage: Params = _
--- End diff --
I'd recommend we either plan to remove "stage" or change "logPipelineStage" so it only allows setting "stage" once. If we go with the former, how about leaving a note to remove "stage" once spark.ml code is migrated to use the new logParams() method?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21719: [SPARK-24747][ML] Make Instrumentation class more flexib...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21719
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/908/
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 #21719: [SPARK-24747] Make Instrumentation class more fle...
Posted by mengxr <gi...@git.apache.org>.
Github user mengxr commented on a diff in the pull request:
https://github.com/apache/spark/pull/21719#discussion_r200498918
--- Diff: mllib/src/main/scala/org/apache/spark/ml/util/Instrumentation.scala ---
@@ -148,12 +170,25 @@ private[spark] class Instrumentation[E <: Estimator[_]] private (
}
+ // TODO: Remove this (possibly replace with logModel?)
/**
* Logs the successful completion of the training session.
*/
def logSuccess(model: Model[_]): Unit = {
log(s"training finished")
}
+
+ def logSuccess(): Unit = {
+ log("training finished")
+ }
+
+ /**
+ * Logs an exception raised during a training session.
+ */
+ def logFailure(e: Throwable): Unit = {
+ val msg = e.getStackTrace.mkString("\n")
+ super.logInfo(msg)
--- End diff --
Failures should go to ERROR level.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21719: [SPARK-24747][ML] Make Instrumentation class more flexib...
Posted by jkbradley <gi...@git.apache.org>.
Github user jkbradley commented on the issue:
https://github.com/apache/spark/pull/21719
jenkins test this please
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21719: [SPARK-24747][ML] Make Instrumentation class more flexib...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21719
**[Test build #92906 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/92906/testReport)** for PR 21719 at commit [`b98d772`](https://github.com/apache/spark/commit/b98d77284f2397daef2623c012d34ff50a25f498).
* This patch **fails Spark 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 #21719: [SPARK-24747] Make Instrumentation class more fle...
Posted by mengxr <gi...@git.apache.org>.
Github user mengxr commented on a diff in the pull request:
https://github.com/apache/spark/pull/21719#discussion_r200497861
--- Diff: mllib/src/main/scala/org/apache/spark/ml/util/Instrumentation.scala ---
@@ -35,32 +36,47 @@ import org.apache.spark.util.Utils
/**
* A small wrapper that defines a training session for an estimator, and some methods to log
* useful information during this session.
- *
- * A new instance is expected to be created within fit().
- *
- * @param estimator the estimator that is being fit
- * @param dataset the training dataset
- * @tparam E the type of the estimator
*/
-private[spark] class Instrumentation[E <: Estimator[_]] private (
- val estimator: E,
- val dataset: RDD[_]) extends Logging {
+private[spark] class Instrumentation extends Logging {
private val id = UUID.randomUUID()
- private val prefix = {
- // estimator.getClass.getSimpleName can cause Malformed class name error,
- // call safer `Utils.getSimpleName` instead
- val className = Utils.getSimpleName(estimator.getClass)
- s"$className-${estimator.uid}-${dataset.hashCode()}-$id: "
+ private val shortId = id.toString.take(8)
+ private var prefix = s"$shortId:"
+
+ // TODO: update spark.ml to use new Instrumentation APIs and remove this constructor
+ var estimator: Estimator[_] = _
+ private def this(estimator: Estimator[_], dataset: RDD[_]) = {
+ this()
+ logContext(estimator, dataset)
}
- init()
+ /**
+ * Log info about the estimator and dataset being fit.
+ *
+ * @param estimator the estimator that is being fit
+ * @param dataset the training dataset
+ */
+ def logContext(estimator: Estimator[_], dataset: RDD[_]): Unit = {
--- End diff --
see my comment above
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21719: [SPARK-24747] Make Instrumentation class more flexible
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21719
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/709/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21719: [SPARK-24747][ML] Make Instrumentation class more flexib...
Posted by jkbradley <gi...@git.apache.org>.
Github user jkbradley commented on the issue:
https://github.com/apache/spark/pull/21719
LGTM
Merging with master
Thanks @MrBago !
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21719: [SPARK-24747][ML] Make Instrumentation class more flexib...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21719
**[Test build #92906 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/92906/testReport)** for PR 21719 at commit [`b98d772`](https://github.com/apache/spark/commit/b98d77284f2397daef2623c012d34ff50a25f498).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21719: [SPARK-24747] Make Instrumentation class more fle...
Posted by mengxr <gi...@git.apache.org>.
Github user mengxr commented on a diff in the pull request:
https://github.com/apache/spark/pull/21719#discussion_r200497209
--- Diff: mllib/src/main/scala/org/apache/spark/ml/classification/LogisticRegression.scala ---
@@ -500,7 +500,7 @@ class LogisticRegression @Since("1.2.0") (
if (handlePersistence) instances.persist(StorageLevel.MEMORY_AND_DISK)
- val instr = Instrumentation.create(this, dataset)
+ instr.logContext(this, dataset)
--- End diff --
It doesn't log anything. I think we should auto-generate `prefix` and keep it as a constant. So logs would appear as:
~~~
[PREFIX]: instrumentation started
[PREFIX]: using estimator logReg-abc128
[PREFIX]: using dataset some hashcode
[PREFIX]: param maxIter=10
[PREFIX]: ...
[PREFIX]: run succeeded/failed
[PREFIX]: instrumentation ended
~~~
We can generate 8 random chars as the PREFIX. This is sufficient for correlate metrics from the same run. The issue with making it mutable is that we do not have a way to guarantee `logContext` is always called.
So I would suggest replacing logContext with the following:
* logEstimator or logPipelineStage
* logDataset
Btw, we can by default log call site. It provides more info for instrumentation, not necessary in this PR.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org