[SPARK-24747][ML] Make Instrumentation class more flexible #21719

MrBago · 2018-07-05T18:26:52Z

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.

Instrumentation class. Updated LogisticRegression to use this API as an example.

SparkQA · 2018-07-05T19:38:47Z

Test build #92653 has finished for PR 21719 at commit 3a6537d.

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

mengxr

made one pass

mengxr · 2018-07-05T21:30:13Z

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

  protected[spark] def train(
      dataset: Dataset[_],
-      handlePersistence: Boolean): LogisticRegressionModel = {
+      handlePersistence: Boolean): LogisticRegressionModel = Instrumentation.instrumented { instr =>


To avoid line too wide, we might want to import instrumented and save "Instrumentation" from this line.

mengxr · 2018-07-05T21:38:06Z

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

    if (handlePersistence) instances.persist(StorageLevel.MEMORY_AND_DISK)

-    val instr = Instrumentation.create(this, dataset)
+    instr.logContext(this, dataset)


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.

mengxr · 2018-07-05T21:41:38Z

mllib/src/main/scala/org/apache/spark/ml/util/Instrumentation.scala

+   * @param estimator the estimator that is being fit
+   * @param dataset the training dataset
+   */
+  def logContext(estimator: Estimator[_], dataset: RDD[_]): Unit = {


see my comment above

mengxr · 2018-07-05T21:45:46Z

mllib/src/main/scala/org/apache/spark/ml/util/Instrumentation.scala

  }
+
+  def logSuccess(): Unit = {
+    log("training finished")


We shouldn't have this log alias. I was wondering which log level it uses. Just use logInfo and remove log(.

mengxr · 2018-07-05T21:46:40Z

mllib/src/main/scala/org/apache/spark/ml/util/Instrumentation.scala

+   */
+  def logFailure(e: Throwable): Unit = {
+    val msg = e.getStackTrace.mkString("\n")
+    super.logInfo(msg)


Failures should go to ERROR level.

mengxr · 2018-07-05T21:47:52Z

mllib/src/main/scala/org/apache/spark/ml/util/Instrumentation.scala

+
+  def instrumented[T](body: (Instrumentation => T)): T = {
+    val instr = new Instrumentation()
+    Try(body(new Instrumentation())) match {


use already constructed instr

mengxr · 2018-07-05T21:48:34Z

mllib/src/main/scala/org/apache/spark/ml/util/Instrumentation.scala

+      case Failure(NonFatal(e)) =>
+        instr.logFailure(e)
+        throw e
+      case Success(model) =>


model -> result, it doesn't need to be a model

SparkQA · 2018-07-12T02:13:57Z

Test build #92906 has finished for PR 21719 at commit b98d772.

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

jkbradley · 2018-07-12T20:11:16Z

jenkins test this please

SparkQA · 2018-07-12T21:22:56Z

Test build #92948 has finished for PR 21719 at commit b98d772.

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

jkbradley

LGTM other than a couple of nits

jkbradley · 2018-07-16T19:44:30Z

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

    train(dataset, handlePersistence)
  }

+  import Instrumentation.instrumented


Put import at top of file with the other imports (just to make imports easier to track).

jkbradley · 2018-07-16T20:01:37Z

mllib/src/main/scala/org/apache/spark/ml/util/Instrumentation.scala

+  private val prefix = s"[$shortId] "
+
+  // TODO: update spark.ml to use new Instrumentation APIs and remove this constructor
+  var stage: Params = _


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?

Yep, the plan is to remove stage once we port switch over to the new APIs

SparkQA · 2018-07-17T19:26:37Z

Test build #93189 has finished for PR 21719 at commit 1676a6d.

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

jkbradley · 2018-07-17T20:10:43Z

LGTM
Merging with master
Thanks @MrBago !

## What changes were proposed in this pull request? Followup for #21719. Update spark.ml training code to fully wrap instrumented methods and remove old instrumentation APIs. ## How was this patch tested? existing tests. Please review http://spark.apache.org/contributing.html before opening a pull request. Author: Bago Amirbekian <[email protected]> Closes #21799 from MrBago/new-instrumentation-apis2.

MrBago added 2 commits July 5, 2018 11:21

Added Instrumentation.instrumented API with required changes to

03c9b0a

Instrumentation class. Updated LogisticRegression to use this API as an example.

Allow instrumented method to return any type.

3a6537d

mengxr requested changes Jul 5, 2018

View reviewed changes

MrBago changed the title ~~[SPARK-24747] Make Instrumentation class more flexible~~ [SPARK-24747][ML] Make Instrumentation class more flexible Jul 6, 2018

MrBago force-pushed the new-instrumentation-apis branch 2 times, most recently from 40c8b41 to 3a6537d Compare July 11, 2018 22:54

PR feedback.

b98d772

jkbradley reviewed Jul 16, 2018

View reviewed changes

PR feedback.

1676a6d

asfgit closed this in 912634b Jul 17, 2018

MrBago mentioned this pull request Jul 17, 2018

[SPARK-24852][ML] Update spark.ml to use Instrumentation.instrumented. #21799

Closed

[SPARK-24747][ML] Make Instrumentation class more flexible #21719

[SPARK-24747][ML] Make Instrumentation class more flexible #21719

Uh oh!

Conversation

MrBago commented Jul 5, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Jul 5, 2018

Uh oh!

mengxr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 12, 2018

Uh oh!

jkbradley commented Jul 12, 2018

Uh oh!

SparkQA commented Jul 12, 2018

Uh oh!

jkbradley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 17, 2018

Uh oh!

jkbradley commented Jul 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants