Making model selectors robust to failing models #372

leahmcguire · 2019-07-26T20:29:27Z

Related issues
#370

Describe the proposed solution
Make model selector no fail when a portion of attempted models don't work. Also exposed parameter to limit max time will wait for modeling to finish in model selector

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Additional context
Add any other context about the changes here.

codecov · 2019-07-26T20:50:05Z

Codecov Report

Merging #372 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #372      +/-   ##
=========================================
+ Coverage   86.77%   86.8%   +0.02%     
=========================================
  Files         336     336              
  Lines       10922   10928       +6     
  Branches      335     354      +19     
=========================================
+ Hits         9478    9486       +8     
+ Misses       1444    1442       -2

Impacted Files	Coverage Δ
...op/stages/impl/selector/ModelSelectorFactory.scala	`85.71% <ø> (ø)`	⬆️
...sification/BinaryClassificationModelSelector.scala	`98.24% <ø> (ø)`	⬆️
...ssification/MultiClassificationModelSelector.scala	`97.56% <ø> (ø)`	⬆️
...a/com/salesforce/op/filters/RawFeatureFilter.scala	`92.93% <ø> (ø)`	⬆️
...op/stages/impl/tuning/OpTrainValidationSplit.scala	`100% <ø> (ø)`	⬆️
.../src/main/scala/com/salesforce/op/OpWorkflow.scala	`87.5% <100%> (ø)`	⬆️
...sforce/op/stages/impl/selector/ModelSelector.scala	`98.18% <100%> (-0.07%)`	⬇️
...salesforce/op/stages/impl/tuning/OpValidator.scala	`94.59% <100%> (+0.47%)`	⬆️
...orce/op/stages/impl/tuning/OpCrossValidation.scala	`97.67% <100%> (ø)`	⬆️
...p/stages/impl/selector/DefaultSelectorParams.scala	`100% <100%> (ø)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b505ff7...365bbd6. Read the comment docs.

tovbinm · 2019-07-26T20:51:31Z

core/src/main/scala/com/salesforce/op/stages/impl/tuning/OpValidator.scala

-          log.info(s"Got metric $metric for model $name trained with ${params(i)}.")
-          metrics(i) = metric
+        val paramsMetrics = params.map { p =>
+          Try {


You might wanna consider to parallelize by splits AND models, I.e just use futures instead of tries nested inside futures.

I tried that but I think the copy of the model causes some kind of lock - when I had them both in futures the first test where the model selectors are ran for 30 minutes without ever finishing....

Let’s put a todo there or perhaps Chris S. Can help?

@tovbinm @leahmcguire I was able to replace params with params.par and although it worked it was much slower than one would expect. First, does it make sense to fit and eval all models in parallel when there is only one spark context?
BTW do you have jvisualvm avail in Zulu?

You can always get visualvm standalone https://visualvm.github.io/download.html

#373 - works fine for me. Though I am not sure about running anything in parallel with spark context as dependency.

Please refer to the implementation of cross validation in Spark (it uses Futures and there is a good reason why).

And that’s why I propose to stay with Futures, but just use them correctly.

@tovbinm If you are referring to https://github.com/apache/spark/blob/master/mllib/src/main/scala/org/apache/spark/ml/tuning/CrossValidator.scala then the only difference with my PR is that they are using foldMetricFutures.map(ThreadUtils.awaitResult(_, Duration.Inf)) instead of Future.sequence therefore can you elaborate more?

tovbinm · 2019-07-26T20:52:14Z

core/src/test/scala/com/salesforce/op/stages/impl/regression/RegressionModelSelectorTest.scala


  val data = sc.parallelize(rawData).toDF("label", "features")
+  data.show()


No show() in tests.

tovbinm · 2019-07-26T20:53:31Z

core/src/main/scala/com/salesforce/op/stages/impl/tuning/OpValidator.scala

+        log.warn(s"Model attempted in model selector failed with following issue: \n${e.getMessage}")
+        None
+    }}
+    val summary = SparkThreadUtils.utils.awaitResult(Future.sequence(summaryOfAttempts), maxWait).flatten.toArray


Perhaps it’s worth to log the maxWait value

core/src/main/scala/com/salesforce/op/OpWorkflow.scala

Jauntbox

I don't see any explicit changes to the ModelInsights, so what does it look like there when a model fails. Is it easy to parse out a list of failed model types / settings from ModelInsights if we wanted to add these to downstream metrics?

It looks like the summary for a failed model is None, which then disappears when it's flatMapd over, is that right?

…to lm/modelFail

leahmcguire · 2019-07-26T23:37:32Z

If a model fails it doesnt appear in model insights - if you wanted to know which models you tried and failed you would need to compare the modelselector params to the sequence of evaluated models

Jauntbox · 2019-07-27T00:14:41Z

If a model fails it doesnt appear in model insights - if you wanted to know which models you tried and failed you would need to compare the modelselector params to the sequence of evaluated models

Ok, so for our use case we could still get a list of failed models with that comparison.

What about more complicated cases where we have a random search or a data-dependent hyperparameter search? It looks like for random search we'd still have the info stored in the ParamMap in the models argument to ModelSelector so we'd probably do the same thing for a data-dependent hyperparam search too?

leahmcguire · 2019-07-27T00:27:57Z

Maybe - I still need to think about how we would implement data dependent hyperparmeter search. I can make it a requirement that the models tried exist somewhere...

gerashegalov · 2019-07-29T19:23:25Z

core/src/test/scala/com/salesforce/op/stages/impl/regression/RegressionModelSelectorTest.scala

+  }
+
+
+  it should "fail when all models fail" in {


can you explain in comments or assertions which model fails and why in each case?

maxWait could be tested explicitly with some short duration.

gerashegalov · 2019-07-29T19:28:15Z

core/src/main/scala/com/salesforce/op/stages/impl/selector/ModelSelector.scala

+    val dataUse = dataOpt.getOrElse(data)
+
+    val theBestEstimator = validator.validate(modelInfo = modelsUse, dataset = dataUse,
+      label = in1.name, features = in2.name, dag = dag, splitter = splitter


should we use labelColName everywhere for consistency?

…modelFail

…to lm/modelFail

leahmcguire · 2019-07-31T17:02:37Z

addressed all the comments - can someone approve?

gerashegalov · 2019-07-31T18:39:20Z

core/src/main/scala/com/salesforce/op/stages/impl/selector/ModelSelector.scala

-    }
+    setInputSchema(dataset.schema).transformSchema(dataset.schema)
+    require(!dataset.isEmpty, "Dataset cannot be empty")
+    val data = dataset.select(in1.name, in2.name)


nit: labelColName?

gerashegalov · 2019-07-31T18:40:37Z

core/src/main/scala/com/salesforce/op/stages/impl/selector/ModelSelector.scala

+    setInputSchema(dataset.schema).transformSchema(dataset.schema)
+    require(!dataset.isEmpty, "Dataset cannot be empty")
+    val data = dataset.select(in1.name, in2.name)
+    val (BestEstimator(name, estimator, summary), splitterSummary, datasetWithID) = bestEstimator.map{ e =>


nit: whitespace inconsistent, some map { and map{

gerashegalov · 2019-07-31T18:43:23Z

core/src/main/scala/com/salesforce/op/stages/impl/tuning/OpValidator.scala

+            val model = estimator.fit(train, p).asInstanceOf[M]
+            val metric = evaluator.evaluate(model.transform(test, p))
+            log.info(s"Got metric $metric for model $name trained with $p.")
+            Some(p -> metric)


nit: we seem to prefer Option(blah) many places

gerashegalov · 2019-07-31T18:51:14Z

core/src/main/scala/com/salesforce/op/stages/impl/tuning/OpValidator.scala

-          metrics(i) = metric
+
+        val paramsMetricsF = params.seq.map { p =>
+          val f = Future {


nit: we can get away without defining f since we don't use it much

Future { // }.recover { // }

gerashegalov · 2019-07-31T18:56:06Z

core/src/main/scala/com/salesforce/op/stages/impl/tuning/OpValidator.scala

+            Some(p -> metric)
+          }
+          f.recover({ case e: Throwable =>
+            log.warn(s"Model $name attempted in model selector with failed with following issue: \n${e.getMessage}")


let us add the raw e as a second arg to warn to get a stack trace if logger is configured this way

gerashegalov · 2019-07-31T19:46:52Z

core/src/main/scala/com/salesforce/op/stages/impl/tuning/OpValidator.scala

+
+    val summaryOfAttempts = summaryFuts.map { f => f.map(Option(_)).recover {
+      case e: Throwable =>
+        log.warn(s"Model attempted in model selector failed with following issue: \n${e.getMessage}")


prefer adding raw e as the second arg to logger

gerashegalov · 2019-07-31T19:50:49Z

core/src/test/scala/com/salesforce/op/OpWorkflowCVTest.scala

      parallelism = 4,
+      seed = 10L,


the args are not in particular order, so can we put it back to the original spot to eliminate this diff

gerashegalov · 2019-07-31T19:51:50Z

core/src/test/scala/com/salesforce/op/OpWorkflowCVTest.scala

      parallelism = 4,
+      seed = 10L,


let us get rid of that non-diff

gerashegalov · 2019-07-31T19:58:38Z

core/src/test/scala/com/salesforce/op/OpWorkflowCVTest.scala

+      summary2.selectedModelInfo.get.validationResults
+    ).forall{ case (v1, v2) =>
+      println(v1.metricValues.asInstanceOf[SingleMetric].value, v2.metricValues.asInstanceOf[SingleMetric].value)
+        v1.metricValues.asInstanceOf[SingleMetric].value < v2.metricValues.asInstanceOf[SingleMetric].value


nit: indentation

gerashegalov · 2019-07-31T20:08:39Z

core/src/test/scala/com/salesforce/op/stages/impl/regression/RegressionModelSelectorTest.scala

+      .setInput(label, features)
+
+
+    intercept[Exception](testEstimator.fit(data))


let us use more specific exceptions in intercepts. e.g. TimeoutException here

Jauntbox

LGTM

gerashegalov

LGTM

This reverts commit 496174c.

Bug fixes: - Ensure correct metrics despite model failures on some CV folds [#404](#404) - Fix flaky `ModelInsight` tests [#395](#395) - Avoid creating `SparseVector`s for LOCO [#377](#377) New features / updates: - Model combiner [#385](#399) - Added new sample for HousingPrices [#365](#365) - Test to verify that custom metrics appear in model insight metrics [#387](#387) - Add `FeatureDistribution` to `SerializationFormat`s [#383](#383) - Add metadata to `OpStandadrdScaler` to allow for descaling [#378](#378) - Improve json serde error in `evalMetFromJson` [#380](#380) - Track mean & standard deviation as metrics for numeric features and for text length of text features [#354](#354) - Making model selectors robust to failing models [#372](#372) - Use compact and compressed model json by default [#375](#375) - Descale feature contribution for Linear Regression & Logistic Regression [#345](#345) Dependency updates: - Update tika version [#382](#382)

salesforce-cla · 2020-10-12T09:37:44Z

Thanks for the contribution! Before we can merge this, we need @wsuchy to sign the Salesforce.com Contributor License Agreement.

salesforce-cla · 2020-12-16T18:40:45Z

Thanks for the contribution! Unfortunately we can't verify the commit author(s): leahmcguire <l***@s***.com> Leah McGuire <l***@s***.com>. One possible solution is to add that email to your GitHub account. Alternatively you can change your commits to another email and force push the change. After getting your commits associated with your GitHub account, refresh the status of this Pull Request.

leahmcguire added 3 commits July 26, 2019 10:25

made nested futures in param fitting

d50caa6

switched to trys so dont have blocking calls

0362a81

moved stuff into sub functions in ModelSelector

8da577a

leahmcguire requested a review from tovbinm as a code owner July 26, 2019 20:29

leahmcguire requested review from gerashegalov, mweilsalesforce and Jauntbox July 26, 2019 20:30

leahmcguire mentioned this pull request Jul 26, 2019

Some of the binary-classifiers, multi-classifiers, regressors don't work in some cases #370

Closed

Merge branch 'master' into lm/modelFail

d3a3284

leahmcguire added the ready for review label Jul 26, 2019

tovbinm reviewed Jul 26, 2019

View reviewed changes

addressing comments

04400d5

Jauntbox reviewed Jul 26, 2019

View reviewed changes

leahmcguire added 2 commits July 26, 2019 16:33

removed unused ids

32d5ae7

Merge branch 'lm/modelFail' of github.com:salesforce/TransmogrifAI in…

58c8b4e

…to lm/modelFail

leahmcguire added 2 commits July 29, 2019 08:35

test fix

6a42540

put back seed

24068e1

gerashegalov reviewed Jul 29, 2019

View reviewed changes

wsuchy and others added 4 commits July 30, 2019 10:08

used list of futures (#373)

cc0b89f

updating tests

9fe44fe

Merge branch 'master' of github.com:salesforce/TransmogrifAI into lm/…

5f59926

…modelFail

Merge branch 'lm/modelFail' of github.com:salesforce/TransmogrifAI in…

9d47923

…to lm/modelFail

tovbinm approved these changes Jul 31, 2019

View reviewed changes

gerashegalov reviewed Jul 31, 2019

View reviewed changes

Jauntbox approved these changes Jul 31, 2019

View reviewed changes

addressing comments

365bbd6

gerashegalov approved these changes Aug 2, 2019

View reviewed changes

tovbinm merged commit 496174c into master Aug 2, 2019

tovbinm deleted the lm/modelFail branch August 2, 2019 05:02

TuanNguyen27 added a commit that referenced this pull request Aug 2, 2019

Revert "Making model selectors robust to failing models (#372)"

56b32d5

This reverts commit 496174c.

ericwayman pushed a commit that referenced this pull request Aug 16, 2019

Making model selectors robust to failing models (#372)

31b2d7d

gerashegalov mentioned this pull request Sep 8, 2019

0.6.1 release #403

Merged

salesforce-cla bot added the cla:missing label Oct 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Making model selectors robust to failing models #372

Making model selectors robust to failing models #372

leahmcguire commented Jul 26, 2019 •

edited

Loading

codecov bot commented Jul 26, 2019 •

edited

Loading

tovbinm Jul 26, 2019

leahmcguire Jul 26, 2019

tovbinm Jul 27, 2019

wsuchy Jul 29, 2019

gerashegalov Jul 29, 2019

wsuchy Jul 29, 2019 •

edited

Loading

tovbinm Jul 29, 2019

tovbinm Jul 29, 2019

wsuchy Jul 29, 2019

tovbinm Jul 26, 2019

tovbinm Jul 26, 2019

Jauntbox left a comment

leahmcguire commented Jul 26, 2019

Jauntbox commented Jul 27, 2019

leahmcguire commented Jul 27, 2019

gerashegalov Jul 29, 2019

gerashegalov Jul 29, 2019

gerashegalov Jul 29, 2019

leahmcguire commented Jul 31, 2019

gerashegalov Jul 31, 2019

gerashegalov Jul 31, 2019

gerashegalov Jul 31, 2019

gerashegalov Jul 31, 2019

gerashegalov Jul 31, 2019

gerashegalov Jul 31, 2019

gerashegalov Jul 31, 2019

gerashegalov Jul 31, 2019

gerashegalov Jul 31, 2019

gerashegalov Jul 31, 2019

Jauntbox left a comment

gerashegalov left a comment

salesforce-cla bot commented Oct 12, 2020

salesforce-cla bot commented Dec 16, 2020


		val data = sc.parallelize(rawData).toDF("label", "features")
		data.show()

		.setInput(label, features)


		intercept[Exception](testEstimator.fit(data))

Making model selectors robust to failing models #372

Making model selectors robust to failing models #372

Conversation

leahmcguire commented Jul 26, 2019 • edited Loading

codecov bot commented Jul 26, 2019 • edited Loading

Codecov Report

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

wsuchy Jul 29, 2019 • edited Loading

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

Jauntbox left a comment

Choose a reason for hiding this comment

leahmcguire commented Jul 26, 2019

Jauntbox commented Jul 27, 2019

leahmcguire commented Jul 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leahmcguire commented Jul 31, 2019

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

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

Jauntbox left a comment

Choose a reason for hiding this comment

gerashegalov left a comment

Choose a reason for hiding this comment

salesforce-cla bot commented Oct 12, 2020

salesforce-cla bot commented Dec 16, 2020

leahmcguire commented Jul 26, 2019 •

edited

Loading

codecov bot commented Jul 26, 2019 •

edited

Loading

wsuchy Jul 29, 2019 •

edited

Loading