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

ajaysaini725 · 2017-06-12T21:48:29Z

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.

…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.

jkbradley · 2017-06-12T21:57:08Z

add to whitelist

SparkQA · 2017-06-12T21:59:51Z

Test build #77948 has finished for PR 18281 at commit b69f201.

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

SparkQA · 2017-06-12T22:04:44Z

Test build #77950 has finished for PR 18281 at commit b69f201.

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

SparkQA · 2017-06-12T23:36:55Z

Test build #77951 has finished for PR 18281 at commit e750d3e.

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

…n of the one vs. rest algorithm.

SparkQA · 2017-06-13T01:12:15Z

Test build #77954 has finished for PR 18281 at commit 81d458b.

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

jkbradley · 2017-06-13T16:46:31Z

taking a look now

jkbradley

Thanks for the PR! My comments are mostly about the unit tests.

Also, since SPARK-21028 has been closed, can you please remove it from the PR title?

jkbradley · 2017-06-13T16:46:24Z

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

  def getClassifier: ClassifierType = $(classifier)
+
+  val parallelism = new IntParam(this, "parallelism",
+    "parallelism parameter for tuning amount of parallelism", ParamValidators.gtEq(1))


This is not very informative. Can you please make it more explicit?

Also

add Scala doc

add Since annotation

Mark this as an expertParam (the "group" annotation in the Scala doc); see other parts of the code for examples

jkbradley · 2017-06-13T16:49:44Z

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

    set(classifier, value.asInstanceOf[ClassifierType])
  }

+  /** @group setParam */


next release will be 2.3

jkbradley · 2017-06-13T16:52:42Z

mllib/src/test/scala/org/apache/spark/ml/classification/OneVsRestSuite.scala

+    assert(ova.getPredictionCol === "prediction")
+    val ovaModel = ova.fit(dataset)
+
+    MLTestingUtils.checkCopyAndUids(ova, ovaModel)


This is a generic test which only needs to be done in 1 test for each algorithm. You can remove it here.

jkbradley · 2017-06-13T16:52:57Z

mllib/src/test/scala/org/apache/spark/ml/classification/OneVsRestSuite.scala

+    val ova = new OneVsRest()
+      .setClassifier(new LogisticRegression)
+      .setParallelism(8)
+    assert(ova.getLabelCol === "label")


No need to check Params. That's not what this unit test is for.

There are several things like that below; just go through and remove items which are not part of this unit test.

jkbradley · 2017-06-13T16:54:51Z

mllib/src/test/scala/org/apache/spark/ml/classification/OneVsRestSuite.scala

    assert(expectedMetrics.confusionMatrix ~== ovaMetrics.confusionMatrix absTol 400)
  }

+  test("one-vs-rest: tuning parallelism produces correct output") {


"produces correct output" --> "does not affect output"

This test appears to check OVR vs. another algorithm. I think a more precise test would check that tuning parallelism still produces exactly the same models. Could you please update it to do so?

jkbradley · 2017-06-13T16:57:40Z

python/pyspark/ml/classification.py


        return self._copyValues(OneVsRestModel(models=models))

+    def setParallelism(self, value):


Add Since annotations

jkbradley · 2017-06-13T16:59:45Z

python/pyspark/ml/classification.py


-        # TODO: Parallel training for all classes.
-        models = [trainSingleClass(i) for i in range(numClasses)]
+        pool = ThreadPool(processes=self.getParallelism())


One new thought: It'd be good to set processes to min(parallelism, numClasses). Same in Scala.

jkbradley · 2017-06-13T17:00:27Z

python/pyspark/ml/tests.py

+
+class ParOneVsRestTests(SparkSessionTestCase):
+
+    def test_copy(self):


What is this testing?

jkbradley · 2017-06-13T17:00:57Z

python/pyspark/ml/tests.py

+        model1 = model.copy({model.predictionCol: "indexed"})
+        self.assertEqual(model1.getPredictionCol(), "indexed")
+
+    def test_output_columns(self):


Ditto: is this needed?

How about adding a test like the one I proposed for Scala, which makes sure the same model is learned regardless of parallelism?

jkbradley · 2017-06-13T17:13:06Z

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

      multiclassLabeled.persist(StorageLevel.MEMORY_AND_DISK)
    }

+    val iters = Range(0, numClasses).par


CC @thunterdb just to double-check

@jkbradley thanks for calling this out. Indeed, the code as it stands may cause some non-deterministic issues in complex environments. I put a comment about that in that PR:
#16774
See how it is done in this file:
https://github.com/apache/spark/pull/16774/files#diff-d14539cadce0fba9d8b7d970adaf8b26

It should be a quick change.

Discussed offline with @thunterdb : Setting the parallel collection tasksupport value is essentially doing the same thing as in @BryanCutler 's PR: ForkJoinTaskSupport is using the ExecutionContext created by ForkJoinPool under the hood.

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

SparkQA · 2017-06-13T20:32:38Z

Test build #78004 has started for PR 18281 at commit 2133378.

thunterdb

@ajaysaini725 using scala's parallel collections is not going to work well in that context, see my comment. It is easy to fix by looking at the linked file.

thunterdb · 2017-06-13T20:38:00Z

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

      multiclassLabeled.persist(StorageLevel.MEMORY_AND_DISK)
    }

+    val iters = Range(0, numClasses).par


@jkbradley thanks for calling this out. Indeed, the code as it stands may cause some non-deterministic issues in complex environments. I put a comment about that in that PR:
#16774
See how it is done in this file:
https://github.com/apache/spark/pull/16774/files#diff-d14539cadce0fba9d8b7d970adaf8b26

It should be a quick change.

BryanCutler · 2017-06-13T22:14:38Z

Thanks @thunterdb , I was just about to comment also on the similarities to #16774. It would probably be best to align a couple things between the two:

the parallelism param - my PR has just a slightly different name numParallelEval that was meant more to describe evaluation, but maybe something like parallelism would be fine too
what should the default parallelism be?
is this also going to have have the ability to override the default thread-pool with a different ExecutorService, e.g. a managed thread-pool? cc @MLnick

What are your guys thoughts on these?

shaneknapp · 2017-06-13T23:19:46Z

test this please

SparkQA · 2017-06-14T00:26:43Z

Test build #78009 has finished for PR 18281 at commit 2133378.

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

jkbradley · 2017-06-14T01:35:01Z

I agree; it'd be good to match on the Param name. Do you think "parallelism" is too vague? If not, then I like it since it's simple.

I'd vote for default parallelism of 1 to be safe (match current CrossValidator behavior). We can change this PR to use 1 as well for uniformity.

Do we really need to expose the ability to pass in an ExecutorService? Given how few people have requested any kind of parallelism, I have a hard time imagining people needing to pass in an ExecutorService. If one of you has a need for it, I guess that's fine, but I'd recommend it be marked as a DeveloperApi and clearly documented (e.g., not saved by persistence, etc.).

MLnick · 2017-06-14T06:34:49Z

Originally in the CV pr Bryan used par collections. Tim pointed out perhaps futures would be better and to add the ability to use different custom threadpools. However I think the intention was same as here - i.e. To just ensure the threadpool is a new dedicated one and not the default in order not to block multi tenant apps. I tend to prefer the par collection approach for simplicity and readability. Also if we can share the param then great - "parallelism" works I think and is generic enough to be used by multiple components. Also perhaps this wouldn't work with Scala 2.10? I think that may be ok now since we intend to drop support in 2.3.x ? If we agree on approach then the CV PR can be changed back to using par collections with custom task support.

…

On Wed, 14 Jun 2017 at 03:36, jkbradley ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In mllib/src/main/scala/org/apache/spark/ml/classification/OneVsRest.scala <#18281 (comment)>: > @@ -325,8 +343,13 @@ final class OneVsRest @SInCE("1.4.0") ( multiclassLabeled.persist(StorageLevel.MEMORY_AND_DISK) } + val iters = Range(0, numClasses).par Discussed offline with @thunterdb <https://github.com/thunterdb> : Setting the parallel collection tasksupport value is essentially doing the same thing as in @BryanCutler <https://github.com/bryancutler> 's PR: ForkJoinTaskSupport is using the ExecutionContext created by ForkJoinPool under the hood. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#18281 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA_SB35eWKaGmfDIF7gnqVwqrFnwDArMks5sDzkqgaJpZM4N3uuq> .

jkbradley · 2017-06-14T16:49:49Z

You're right about Scala being an issue. This actually works with Scala 2.10 and 2.11 but not 2.12, in which Scala drops its own ForkJoinPool in favor of the java one. As long as we drop 2.10 before adding 2.12, then we'll be OK.

@BryanCutler do par collections seem workable?

jkbradley · 2017-06-14T16:51:32Z

One comment about putting parallelism in a trait vs. not: Would we want to avoid creating a "threadpool" when parallelism = 1? In that (common) case, maybe we'd want to avoid par collections. If so, then the trait could provide a helper method which takes a range/seq and a function accepting values from that range. That helper could use par collections or not, depending on the setting of parallelism. Good, or too complex?

BryanCutler

Thanks for working on this PR, looks good overall. I think we just need to agree on the best approach for this kind of thing.

BryanCutler · 2017-06-14T17:09:06Z

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

+    val iters = Range(0, numClasses).par
+    iters.tasksupport = new ForkJoinTaskSupport(
+      new ForkJoinPool(Math.min(getParallelism, numClasses))
+    )


I don't think ForkJoinPool is the best thing to use here. It's more geared for a bunch of small tasks that might spawn other tasks. Just a regular thread-pool is fine. Also, it is a little better for the case of parallelism 1 to use sameThreadExecutor. This will just run in the current thread and has no overhead (equivalent to running in serial), where a thread-pool of size 1 will still create another thread and does have some overhead.

That sounds like a good way to implement my suggestion above. +1

Actually, quick question: Where are docs for sameThreadExecutor? ...having trouble finding API docs on Google.

Doc is here: https://google.github.io/guava/releases/17.0/api/docs/com/google/common/util/concurrent/MoreExecutors.html#sameThreadExecutor()

It's already in Spark as ThreadUtils.sameThread

BryanCutler · 2017-06-14T17:29:25Z

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

    // create k columns, one for each binary classifier.
-    val models = Range(0, numClasses).par.map { index =>
+    val models = iters.map { index =>
      // generate new label metadata for the binary problem.


Using Futures is a simple change and imho is clearer than using parallel collections once you start messing around with the task support. It would just require changing a couple lines to something like this

... Future {classifier.fit(trainingDataset, paramMap)} (executionContext) }.map(ThreadUtils.awaitResult(_, Duration.Inf)

What do Futures add? They require more code, and the functionality seems to be the same for the purposes of OneVsRest.

They don't necessarily add anything to here, but they are a more standard way of parallelism in Spark over using TaskSupport and it's more flexible for setting an ExecutorService. I'm not sure if you can set TaskSupport to sameThreadExecutor or what really happens behind the scenes if you make a ThreadPoolTaskSupport with 1 thread.

BryanCutler · 2017-06-14T17:30:48Z

mllib/src/test/scala/org/apache/spark/ml/classification/OneVsRestSuite.scala

+
+    val metricsPar2 = new MulticlassMetrics(ovaResultsPar2)
+    val metricsPar4 = new MulticlassMetrics(ovaResultsPar4)
+    assert(metricsPar2.confusionMatrix ~== metricsPar4.confusionMatrix absTol 400)


I think this test would be better if you just compared single threaded vs multi

jkbradley · 2017-06-16T20:12:16Z

@BryanCutler Thanks for the thoughts! I didn't see a response w.r.t. putting parallelism in a trait, so I'll say we won't do it for now. The usage of par collections / Futures in OneVsRest vs tuning looks a bit different.

BryanCutler · 2017-06-16T22:21:13Z

w.r.t. putting parallelism in a trait

I think this would be a goot idea and it could be shared with OneVsRest, CrossValidator and TrainValidationSplit right off the bat. It could have the parallelism Param and return an ExecutionContext based on paralelsim to either return a thread-pool or same-thread executor.

The usage of par collections / Futures in OneVsRest vs tuning looks a bit different.

It is a little different. For CrossValidator, Futures are nice because I could add a callback to unpersist training data once all training is complete and can continue to evaluate each model as soon as it is ready instead of blocking.

jkbradley · 2017-06-22T00:31:25Z

Catching up here, it sounds like the current recommendations (which I'm on board with) are to:

Switch to Futures, including using sameThreadExecutor for the case of parallelism=1
Try putting the use of Futures in a trait, unless it makes the code too awkward. If @BryanCutler can reuse that in the other PR, that'd be great, though it might require some generalization (if worthwhile).
one or two smaller comments above

SparkQA · 2017-07-27T03:07:10Z

Test build #79985 has finished for PR 18281 at commit 9f34404.

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

jkbradley · 2017-07-28T00:15:06Z

@holdenk Some of those improvements on handling parallelism sounds useful, but I'd prefer we merge this and then add more improvements. This PR should be a strict improvement there (moving from no parallelism to some potential for parallelism).

jkbradley · 2017-07-28T00:21:46Z

mllib/src/main/scala/org/apache/spark/ml/param/shared/HasParallelism.scala

+  def getParallelism: Int = $(parallelism)
+
+  /** @group expertSetParam */
+  def setParallelism(value: Int): this.type = {


You can remove this now that it is in OneVsRest

MLnick · 2017-08-03T10:08:19Z

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

  def setPredictionCol(value: String): this.type = set(predictionCol, value)

+  /** @group expertGetParam */
+  override def getParallelism: Int = $(parallelism)


This one can just go in the trait right?

MLnick · 2017-08-03T10:15:48Z

python/pyspark/ml/classification.py

+                  classifier=None, parallelism=1):
        """
-        setParams(self, featuresCol=None, labelCol=None, predictionCol=None, classifier=None):
+        setParams(self, featuresCol=None, labelCol=None, predictionCol=None, \


The default args here in the doc should match the method (for featuresCol, labelCol and predictionCol)

MLnick · 2017-08-03T10:18:39Z

mllib/src/test/scala/org/apache/spark/ml/classification/OneVsRestSuite.scala

+
+    ovaModelPar1.models.zip(ovaModelPar2.models).foreach {
+      case (lrModel1: LogisticRegressionModel, lrModel2: LogisticRegressionModel) =>
+        assert(lrModel1.coefficients === lrModel2.coefficients)


Perhaps we should use the approx equal version for vectors and matrices here and above? It seems the test does pass, but perhaps that would be better, to avoid future flakiness for whatever reason. Also, we do so in the Python tests so it would be more consistent.

MLnick · 2017-08-03T10:19:49Z

@ajaysaini725 Could you resolve merge conflicts and address the remaining outstanding review comments?

I left a few minor additional comments. Overall I think this is just about ready.

MLnick · 2017-08-03T10:21:07Z

I also think we can leave any potential improvements for parallelism on the Python side (as well as the test side if we come up with a good way of testing that fitting is actually being done in parallel) for a later PR.

BryanCutler · 2017-08-03T17:55:53Z

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",


This should probably indicate that the value range is "(>= 1)" at the end of the description.

SparkQA · 2017-08-11T21:24:16Z

Test build #80554 has finished for PR 18281 at commit 9328117.

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

SparkQA · 2017-08-12T00:29:09Z

Test build #80560 has finished for PR 18281 at commit 585a3f8.

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

BryanCutler · 2017-08-17T17:46:30Z

@ajaysaini725 can you fix the outstanding issues here? I would like to make use of the HasParallelism trait for #16774, so it is on hold for this PR. Please let us know if you will be able to update soon. Thanks!

MLnick · 2017-08-18T12:37:48Z

By the way we should note in the migration guide that this is a behavior change on the Scala side - currently par is used so the parallelism would be that of the default threadpool, while we are now setting the default to 1 (i.e. serial).

holdenk · 2017-08-19T00:04:44Z

Also to be clear, I'm fine with the changes I've suggested being left for a follow up (but if we do go ahead and merge this without those changes lets make it an explicit follow up task).

felixcheung · 2017-08-20T19:10:14Z

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

 import org.apache.spark.ml.attribute._
 import org.apache.spark.ml.linalg.Vector
 import org.apache.spark.ml.param.{Param, ParamMap, ParamPair, Params}
+<<<<<<< HEAD


some sort of merge problem? this shouldn't be in a commit

SparkQA · 2017-08-23T17:54:14Z

Test build #81043 has finished for PR 18281 at commit f65381a.

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

SparkQA · 2017-08-23T19:44:42Z

Test build #81045 has finished for PR 18281 at commit 2a335fe.

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

SparkQA · 2017-08-23T21:44:54Z

Test build #3899 has finished for PR 18281 at commit 2a335fe.

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

WeichenXu123 · 2017-09-03T04:32:44Z

I take this PR over in #19110 because the original author is busy but we need merge this PR soon.
Thanks!

jkbradley · 2017-09-05T16:10:31Z

Thanks @ajaysaini725 for your work (and permission given offline to take this over)! We can close this issue now.

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.

ajaysaini725 changed the title ~~Added tunable parallelism to the pyspark implementation of one vs. re…~~ Added tunable parallelism to one vs. rest in pyspark Jun 12, 2017

ajaysaini725 changed the title ~~Added tunable parallelism to one vs. rest in pyspark~~ [SPARK-21027][ML][PYTHON] Added tunable parallelism to one vs. rest in pyspark Jun 12, 2017

Fixed python style.

e750d3e

Added functionality for tuning parellelism in the Scala implementatio…

81d458b

…n of the one vs. rest algorithm.

ajaysaini725 changed the title ~~[SPARK-21027][ML][PYTHON] Added tunable parallelism to one vs. rest in pyspark~~ [SPARK-21027][SPARK-21028][ML][PYTHON] Added tunable parallelism to one vs. rest in pyspark Jun 13, 2017

ajaysaini725 changed the title ~~[SPARK-21027][SPARK-21028][ML][PYTHON] Added tunable parallelism to one vs. rest in pyspark~~ [SPARK-21027][SPARK-21028][ML][PYTHON] Added tunable parallelism to one vs. rest in both Scala mllib and Pyspark Jun 13, 2017

jkbradley reviewed Jun 13, 2017

View reviewed changes

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

2133378

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

ajaysaini725 changed the title ~~[SPARK-21027][SPARK-21028][ML][PYTHON] Added tunable parallelism to one vs. rest in both Scala mllib and Pyspark~~ [SPARK-21027][ML][PYTHON] Added tunable parallelism to one vs. rest in both Scala mllib and Pyspark Jun 13, 2017

thunterdb suggested changes Jun 13, 2017

View reviewed changes

BryanCutler reviewed Jun 14, 2017

View reviewed changes

Modified parallel one vs rest to use futures.

c59b1d8

Fixed style issue.

9f34404

jkbradley reviewed Jul 28, 2017

View reviewed changes

MLnick reviewed Aug 3, 2017

View reviewed changes

BryanCutler reviewed Aug 3, 2017

View reviewed changes

Fixed merge conflict

585a3f8

ajaysaini725 force-pushed the spark-21027 branch from 9328117 to 585a3f8 Compare August 12, 2017 00:23

felixcheung reviewed Aug 20, 2017

View reviewed changes

MLnick mentioned this pull request Aug 23, 2017

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

Closed

Fixed remaining part of merge conflict.

f65381a

Fixed style problem

2a335fe

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 #19110

Closed

asfgit closed this in 720c94f Sep 12, 2017


		return self._copyValues(OneVsRestModel(models=models))

		def setParallelism(self, value):


		class ParOneVsRestTests(SparkSessionTestCase):

		def test_copy(self):

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

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

Uh oh!

Conversation

ajaysaini725 commented Jun 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkbradley commented Jun 12, 2017

Uh oh!

SparkQA commented Jun 12, 2017

Uh oh!

SparkQA commented Jun 12, 2017

Uh oh!

SparkQA commented Jun 12, 2017

Uh oh!

SparkQA commented Jun 13, 2017

Uh oh!

jkbradley commented Jun 13, 2017

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!

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!

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 Jun 13, 2017

Uh oh!

thunterdb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BryanCutler commented Jun 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shaneknapp commented Jun 13, 2017

Uh oh!

SparkQA commented Jun 14, 2017

Uh oh!

jkbradley commented Jun 14, 2017

Uh oh!

MLnick commented Jun 14, 2017 via email

Uh oh!

jkbradley commented Jun 14, 2017

Uh oh!

jkbradley commented Jun 14, 2017

Uh oh!

BryanCutler left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ajaysaini725 commented Jun 12, 2017 •

edited

Loading

BryanCutler commented Jun 13, 2017 •

edited

Loading

BryanCutler left a comment •

edited

Loading

BryanCutler commented Jun 16, 2017 •

edited

Loading

jkbradley commented Jul 28, 2017 •

edited

Loading