Add hyper-parameter tuner interface #386

cmjiang · 2018-08-07T23:25:26Z

@joshvfleming @basukinjal @yunboouyang
This PR, adding a hyper-parameter tuner, has no conflicts with PR#385. After this PR and PR#385 are both merged, let's merge PR#377 rebased on them to completely decouple the auto-tuning system.

basukinjal · 2018-08-09T16:15:30Z

photon-client/src/main/scala/com/linkedin/photon/ml/hyperparameter/tuner/AtlasTuner.scala

+import com.linkedin.photon.ml.hyperparameter.search.{GaussianProcessSearch, RandomSearch}
+
+/**
+ * A hyper-parameter tuner which depends on an internal library.


Let's make it more explicit and say internal LinkedIn library. Since internal might mean within photon-ml.

basukinjal · 2018-08-09T16:19:11Z

photon-client/src/main/scala/com/linkedin/photon/ml/hyperparameter/tuner/AtlasTuner.scala

@@ -0,0 +1,72 @@
+/*


Bit unclear on this part. So the AtlasTuner will move out of photon-ml correct? and photon-ml will only contain the DummyTuner?

Will change it. Thank you!

basukinjal · 2018-08-09T16:20:25Z

photon-client/src/main/scala/com/linkedin/photon/ml/hyperparameter/tuner/AtlasTuner.scala

+import com.linkedin.photon.ml.cli.game.training.GameTrainingDriver.{getOrDefault, hyperParameterTuning, hyperParameterTuningIter}
+import com.linkedin.photon.ml.estimators.{GameEstimator, GameEstimatorEvaluationFunction}
+import com.linkedin.photon.ml.estimators.GameEstimator.GameResult
+import com.linkedin.photon.ml.hyperparameter.search.{GaussianProcessSearch, RandomSearch}


Related to the above comment, in that setup, we will move out all functions from hyperparameter correct?

basukinjal · 2018-08-09T16:22:16Z

...ent/src/main/scala/com/linkedin/photon/ml/hyperparameter/tuner/HyperparameterTunerName.scala

@@ -0,0 +1,23 @@
+/*
+ * Copyright 2017 LinkedIn Corp. All rights reserved.


Make this 2018

Will change it. Thanks.

basukinjal · 2018-08-10T16:17:06Z

photon-lib/src/main/scala/com/linkedin/photon/ml/hyperparameter/tuner/AtlasTuner.scala

@@ -0,0 +1,63 @@
+/*


So this will now move out of photon-ml into atlas-ml and depend on photon-ml for the trait correct?

basukinjal · 2018-08-10T16:18:56Z

photon-lib/src/main/scala/com/linkedin/photon/ml/hyperparameter/tuner/AtlasTuner.scala

+import com.linkedin.photon.ml.evaluation.Evaluator
+import com.linkedin.photon.ml.hyperparameter.EvaluationFunction
+import com.linkedin.photon.ml.hyperparameter.search.{GaussianProcessSearch, RandomSearch}
+


Just to confirm, the lines 19-22 will stay in photon-ml as traits and 23 will move to atlas-ml. Correct?

Yes. And the evaluator will be removed later.

basukinjal

LGTM now. @joshvfleming Please a look and then we can move to Atlas.

cmjiang · 2018-08-10T22:46:20Z

.../src/main/scala/com/linkedin/photon/ml/hyperparameter/tuner/HyperparameterTunerFactory.scala

+  def apply[T](tunerName: HyperparameterTunerName): HyperparameterTuner[T] = {
+    tunerName match {
+      case DUMMY => new DummyTuner[T]
+      case ATLAS => new AtlasTuner[T]


There is still a dependency issue here...

basukinjal · 2018-08-10T23:02:18Z

Let change as per the offline discussion.

basukinjal · 2018-08-14T18:51:18Z

.../src/main/scala/com/linkedin/photon/ml/hyperparameter/tuner/HyperparameterTunerFactory.scala

+object HyperparameterTunerFactory {
+
+  val DUMMY_TUNER = "com.linkedin.photon.ml.hyperparameter.tuner.DummyTuner"
+  val ATLAS_TUNER = "com.linkedin.photon.ml.hyperparameter.tuner.AtlasTuner"


Are both of these moving to Atlas? In either case, let's change the string to " com.linkedin.atlas.tuner.AtlasTuner"

Since it is not moved yet, the test with com.linkedin.atlas.tuner.AtlasTuner will report an error message. Please see the test result with invalid tuner in li-photon-plugin.

Let's put a comment here with a brief explanation of why it's arranged this way, and that the atlas-specific code will be removed.

joshvfleming

LGTM

joshvfleming · 2018-08-14T20:06:14Z

.../src/main/scala/com/linkedin/photon/ml/hyperparameter/tuner/HyperparameterTunerFactory.scala

+object HyperparameterTunerFactory {
+
+  val DUMMY_TUNER = "com.linkedin.photon.ml.hyperparameter.tuner.DummyTuner"
+  val ATLAS_TUNER = "com.linkedin.photon.ml.hyperparameter.tuner.AtlasTuner"


Let's put a comment here with a brief explanation of why it's arranged this way, and that the atlas-specific code will be removed.

cmjiang · 2018-08-14T20:25:52Z

.../src/main/scala/com/linkedin/photon/ml/hyperparameter/tuner/HyperparameterTunerFactory.scala

+  // TODO: Move AtlasTuner into atlas-ml for the auto-tuning system migration:
+  // TODO: val ATLAS_TUNER = "com.linkedin.atlas.ml.hyperparameter.tuner.AtlasTuner".
+  // TODO: Temporarily stay in photon-ml for test purpose.
+  val ATLAS_TUNER = "com.linkedin.photon.ml.hyperparameter.tuner.AtlasTuner"


Comments added here @basukinjal @joshvfleming

basukinjal

LGTM. @joshvfleming Lets merge this.

joshvfleming · 2018-08-15T15:19:56Z

@cmjiang The test failure here appears to be legitimate. I can reproduce it locally:

java.lang.AssertionError: expected [true] but found [false]
    at org.testng.Assert.fail(Assert.java:94)
    at org.testng.Assert.failNotEquals(Assert.java:496)
    at org.testng.Assert.assertTrue(Assert.java:42)
    at org.testng.Assert.assertTrue(Assert.java:52)
    at com.linkedin.photon.ml.cli.game.training.GameTrainingDriverIntegTest$$anonfun$testHyperParameterTuning$1.apply$mcV$sp(GameTrainingDriverIntegTest.scala:376)
    ...

ashelkovnykov · 2018-08-15T17:08:16Z

I don't like that there is both a tuner name and a tuning mode now - one is a subtype of the other, really. The end goal is that we want to do either no tuning, random tuning, or Bayesian tuning. Now this is split over two inputs, many combinations of which are invalid (Atlas + NONE, Dummy + Bayesian, etc.).

Why even have a "dummy" tuner? Why not have an Option so that None is the null tuner?

joshvfleming · 2018-08-15T17:14:50Z

@ashelkovnykov The idea is that rather than an if/else branch in the driver code, the factory would default to a noop tuner in the case of no tuning. That way, the logic of deciding what to do is contained in the factory instead of the driver.

But -- we'll probably need to check whether a parameter has been passed for this in the first place, so perhaps branching on Some/None is okay.

basukinjal · 2018-08-15T17:43:15Z

@ashelkovnykov @joshvfleming I think this is being done just so that photon-ml does not need to depend on atlas-ml in the short term. Once we make atlas-ml open sourced, this branching can be removed as photon-ml will directly depend on atlas-ml and the driver will work only with None, Random or Bayesian. We can then maybe remove this tunerFactory.

joshvfleming · 2018-08-15T17:48:27Z

@basukinjal @ashelkovnykov Yeah. The whole point of this is that it's an intermediate step to create a functioning separation between the two codebases. It might be ugly to have the interface in place, but that's better than having the entire atlas codebase embedded here as it is now.

cmjiang · 2018-08-18T01:04:05Z

...c/integTest/scala/com/linkedin/photon/ml/cli/game/training/GameTrainingDriverIntegTest.scala

@@ -363,7 +363,8 @@ class GameTrainingDriverIntegTest extends SparkTestUtils with GameTestUtils with
    val newArgs = mixedEffectSeriousRunArgs
      .copy
      .put(GameTrainingDriver.rootOutputDirectory, outputDir)
-      .put(GameTrainingDriver.outputMode, ModelOutputMode.TUNED)
+      .put(GameTrainingDriver.outputMode, ModelOutputMode.ALL)


Here we must change the outputMode to ModelOutputMode.ALL. If we use ModelOutputMode.TUNED, we don't have any tuned model with a dummy tuner. Then it returns an empty output model which causes a failure at line 377 below.

Actually I am wondering if we can remove this testHyperParameterTuning in GameTrainingDriverIntegTest since we won't have atlas-ml in photon-ml, all the following lines in this test become non-sense for testing.

Probably best to comment it out, since once Atlas is open-sourced the test will be relevant again.

@cmjiang Great find! Let's get the ball rolling on atlas now. This LGTM. @joshvfleming Let's merge!

Commented the test out and put a comment on this change.

basukinjal reviewed Aug 9, 2018

View reviewed changes

cmjiang force-pushed the tuner branch from 70d910a to 3ec7f1c Compare August 9, 2018 23:59

basukinjal reviewed Aug 10, 2018

View reviewed changes

cmjiang force-pushed the tuner branch from 3ec7f1c to b0d1659 Compare August 10, 2018 22:18

basukinjal approved these changes Aug 10, 2018

View reviewed changes

cmjiang commented Aug 10, 2018

View reviewed changes

cmjiang force-pushed the tuner branch 5 times, most recently from 62549b3 to 64fb466 Compare August 13, 2018 21:53

basukinjal reviewed Aug 14, 2018

View reviewed changes

joshvfleming reviewed Aug 14, 2018

View reviewed changes

cmjiang force-pushed the tuner branch from 64fb466 to 9c9ceb2 Compare August 14, 2018 20:23

cmjiang commented Aug 14, 2018

View reviewed changes

basukinjal approved these changes Aug 14, 2018

View reviewed changes

cmjiang force-pushed the tuner branch from 9c9ceb2 to d0d6954 Compare August 18, 2018 00:55

cmjiang commented Aug 18, 2018

View reviewed changes

Add hyper-parameter tuner interface

a9d2769

cmjiang force-pushed the tuner branch from d0d6954 to a9d2769 Compare August 18, 2018 05:11

joshvfleming merged commit ee56747 into linkedin:master Aug 20, 2018

cmjiang deleted the tuner branch August 20, 2018 20:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add hyper-parameter tuner interface #386

Add hyper-parameter tuner interface #386

cmjiang commented Aug 7, 2018

basukinjal Aug 9, 2018 •

edited

basukinjal Aug 9, 2018

cmjiang Aug 9, 2018

basukinjal Aug 9, 2018

basukinjal Aug 9, 2018

cmjiang Aug 9, 2018

basukinjal Aug 10, 2018 •

edited

cmjiang Aug 10, 2018

basukinjal Aug 10, 2018

cmjiang Aug 10, 2018

basukinjal left a comment

cmjiang Aug 10, 2018

basukinjal commented Aug 10, 2018

basukinjal Aug 14, 2018

cmjiang Aug 14, 2018

joshvfleming Aug 14, 2018

joshvfleming left a comment

joshvfleming Aug 14, 2018

cmjiang Aug 14, 2018

basukinjal left a comment

joshvfleming commented Aug 15, 2018

ashelkovnykov commented Aug 15, 2018

joshvfleming commented Aug 15, 2018 •

edited

basukinjal commented Aug 15, 2018

joshvfleming commented Aug 15, 2018

cmjiang Aug 18, 2018

cmjiang Aug 18, 2018

ashelkovnykov Aug 18, 2018

basukinjal Aug 18, 2018

cmjiang Aug 18, 2018

		@@ -0,0 +1,23 @@
		/*
		* Copyright 2017 LinkedIn Corp. All rights reserved.

Add hyper-parameter tuner interface #386

Add hyper-parameter tuner interface #386

Conversation

cmjiang commented Aug 7, 2018

basukinjal Aug 9, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

basukinjal Aug 10, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

basukinjal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

basukinjal commented Aug 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshvfleming left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

basukinjal left a comment

Choose a reason for hiding this comment

joshvfleming commented Aug 15, 2018

ashelkovnykov commented Aug 15, 2018

joshvfleming commented Aug 15, 2018 • edited

basukinjal commented Aug 15, 2018

joshvfleming commented Aug 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

basukinjal Aug 9, 2018 •

edited

basukinjal Aug 10, 2018 •

edited

joshvfleming commented Aug 15, 2018 •

edited