Save model locally and convert to zip before moving to final path and do the reverse for loading #516

leahmcguire · 2020-10-02T20:00:53Z

Related issues
The mleap bundle save works only with the local file system so workflow saves directly to hdfs fail after merging #475
#514

Describe the proposed solution
Save models first to a local tmp file and then zip and move to final location. move zipped file to local and unzip for loading

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.

tovbinm · 2020-10-02T20:12:16Z

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

-  def save(path: String, overwrite: Boolean = true): Unit = {
-    OpWorkflowModelWriter.save(this, path = path, overwrite = overwrite)
+  def save(path: String, overwrite: Boolean = true,
+    localDir: String = "tmp/model"): Unit = {


please add docs for localDir

better use something random here, e.g. localDir: String = s"tmp/model-${System.currentTimeInMillis}"

Consider calling it modelStagingDir. We don't need an explicit default, we can "borrow"/reflect into https://github.com/apache/spark/blob/065f17386d1851d732b4c1badf1ce2e14d0de338/core/src/main/scala/org/apache/spark/util/Utils.scala createTempDir and other goodies

tovbinm · 2020-10-02T20:25:38Z

utils/src/main/scala/com/salesforce/op/utils/io/file/FileCompress.scala

+   * @param inLoc location of directory to zip
+   * @param zipLoc output zip file name
+   */
+  def zip(inLoc: String, zipLoc: String): Unit = {


It could be beneficial to use this library instead. E.g

// Compress folder ZipUtil.pack(new File("/tmp/demo"), new File("/tmp/demo.zip")) // Decompress zip into a folder ZipUtil.unpack(new File("/tmp/demo.zip"), new File("/tmp/demo"))

The downsides are

it's yet another dependency to be vetted at Salesforce

and we lost the API that would allow to write the zip directly to the destination file system by providing an appropriate output stream here

Lets chat about this @gerashegalov

java.util.zip could be an alternative. zt-zip seems pretty light-weight, though.

codecov · 2020-10-04T01:44:04Z

Codecov Report

Merging #516 into master will decrease coverage by 0.02%.
The diff coverage is 72.72%.

@@            Coverage Diff             @@
##           master     #516      +/-   ##
==========================================
- Coverage   86.72%   86.70%   -0.03%     
==========================================
  Files         347      347              
  Lines       11897    11883      -14     
  Branches      381      379       -2     
==========================================
- Hits        10318    10303      -15     
- Misses       1579     1580       +1

Impacted Files	Coverage Δ
...c/main/scala/com/salesforce/op/ModelInsights.scala	`90.12% <0.00%> (-1.76%)`	⬇�?
...op/evaluators/OpMultiClassificationEvaluator.scala	`94.84% <ø> (ø)`
.../src/main/scala/com/salesforce/op/OpWorkflow.scala	`88.19% <100.00%> (ø)`
...main/scala/com/salesforce/op/OpWorkflowModel.scala	`93.90% <100.00%> (ø)`
...cala/com/salesforce/op/OpWorkflowModelReader.scala	`92.52% <100.00%> (+1.31%)`	⬆�?
...cala/com/salesforce/op/OpWorkflowModelWriter.scala	`100.00% <100.00%> (ø)`
...m/salesforce/op/utils/stages/NameDetectUtils.scala	`88.05% <0.00%> (-1.40%)`	⬇�?
...e/op/stages/impl/feature/SmartTextVectorizer.scala	`95.20% <0.00%> (-0.13%)`	⬇�?
...s/impl/feature/OPCollectionHashingVectorizer.scala	`96.50% <0.00%> (-0.05%)`	⬇�?
...p/stages/impl/feature/SmartTextMapVectorizer.scala	`100.00% <0.00%> (ø)`
... and 54 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 09200c8...00f3e8d. Read the comment docs.

…localSaveAndLoad

…ifAI into lm/localSaveAndLoad

… as a spark stage

…localSaveAndLoad

leahmcguire · 2020-10-09T19:48:04Z

combust/mleap#721

leahmcguire · 2020-10-09T19:48:26Z

You want the link in the code?

tovbinm · 2020-10-09T19:50:06Z

@leahmcguire It's fine to have it here.

TuanNguyen27

LGTM

nicodv

LGTM, but may want to avoid additional dependency

gerashegalov

LGTM, minor comment

gerashegalov · 2020-10-12T20:56:20Z

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

+  def loadModel(
+    path: String,
+    asSpark: Boolean = true,
+    localDir: String = WorkflowFileReader.localDir


just maybe call the parameter modelStagingDir too

gerashegalov · 2020-10-20T21:32:25Z

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

    implicit val conf = new org.apache.hadoop.conf.Configuration()
-    Try(WorkflowFileReader.loadFile(OpWorkflowModelReadWriteShared.jsonPath(path)))
-      .flatMap(loadJson(_, path = path)) match {
+    val localPath = new Path(new File(modelStagingDir).getAbsolutePath)


we should not mix Hadoop and JDK api, let us get rid of new File

to avoid ambiguity let us request the local filesystem explicitly

val localFileSystem = FileSystem.getLocal(spark.sparkContext.hadoopConfiguration)

gerashegalov · 2020-10-20T21:33:57Z

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

-      .flatMap(loadJson(_, path = path)) match {
+    val localPath = new Path(new File(modelStagingDir).getAbsolutePath)
+    val savePath = new Path(path)
+    val fs = savePath.getFileSystem(conf)


let us call it destinationFileSystem

gerashegalov · 2020-10-20T21:34:42Z

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

+    val localPath = new Path(new File(modelStagingDir).getAbsolutePath)
+    val savePath = new Path(path)
+    val fs = savePath.getFileSystem(conf)
+    fs.delete(localPath, true)


This is the bug where we try to delete the local path using the remote (s3,hdfs) filesystem

…path and do the reverse for loading (#516)" This reverts commit 9ab7d10.

salesforce-cla · 2020-11-23T10:07:51Z

Thanks for the contribution! It looks like @leahmcguire is an internal user so signing the CLA is not required. However, we need to confirm this.

salesforce-cla · 2020-12-12T20:49:47Z

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 2 commits October 2, 2020 09:49

made files saved to local before moving to final location

2be81a1

first pass at fixing mleap save and load by saming first to local tmp

2054f0f

leahmcguire added the work in progress label Oct 2, 2020

leahmcguire requested review from gerashegalov, Jauntbox, nicodv, tovbinm and wsuchy as code owners October 2, 2020 20:00

tovbinm reviewed Oct 2, 2020

View reviewed changes

leahmcguire added 3 commits October 3, 2020 15:25

using lib instead of reinventing file compression

2b0ecd0

moving to actual save call

7ae2426

cleanup

26ffb8e

leahmcguire added 9 commits October 3, 2020 20:21

fixed test

68c720d

allowing workflow to load as spark or not

058b6a9

Merge branch 'master' into lm/localSaveAndLoad

2b45cdb

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

b4b8da8

…localSaveAndLoad

Merge branch 'lm/localSaveAndLoad' of github.com:salesforce/Transmogr…

8c30217

…ifAI into lm/localSaveAndLoad

removed old comment

1ffaa6e

protection for reloaded tree models not having feature imp

86608ca

found and hacked around bug in mleap loading of randomforestregressor…

8f86f54

… as a spark stage

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

51e4ae9

…localSaveAndLoad

leahmcguire added ready for review and removed work in progress labels Oct 9, 2020

added link to mleap issue

9a9ba6f

TuanNguyen27 approved these changes Oct 12, 2020

View reviewed changes

nicodv approved these changes Oct 12, 2020

View reviewed changes

gerashegalov approved these changes Oct 12, 2020

View reviewed changes

leahmcguire added 5 commits October 12, 2020 21:16

changed the directory name

f0cde08

style

3ce8086

style

e6af0bc

increasing data size in test to try and make it less flakey

c1f80e5

allowing more time to produce more scores

00f3e8d

leahmcguire merged commit 9ab7d10 into master Oct 13, 2020

leahmcguire deleted the lm/localSaveAndLoad branch October 13, 2020 18:27

gerashegalov reviewed Oct 21, 2020

View reviewed changes

gerashegalov mentioned this pull request Oct 21, 2020

Fix local file system handling #521

Merged

sanmitra added a commit that referenced this pull request Nov 2, 2020

Revert "Save model locally and convert to zip before moving to final …

a7aa8bc

…path and do the reverse for loading (#516)" This reverts commit 9ab7d10.

sanmitra mentioned this pull request Nov 11, 2020

Fix writing of model in the local file system #528

Merged

ghost mentioned this pull request Nov 16, 2020

Fix writing of model in the local file system #529

Closed

salesforce-cla bot added the cla:missing label Nov 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Save model locally and convert to zip before moving to final path and do the reverse for loading #516

Save model locally and convert to zip before moving to final path and do the reverse for loading #516

leahmcguire commented Oct 2, 2020

tovbinm Oct 2, 2020

gerashegalov Oct 5, 2020

tovbinm Oct 2, 2020

gerashegalov Oct 5, 2020

leahmcguire Oct 9, 2020

nicodv Oct 12, 2020

codecov bot commented Oct 4, 2020 •

edited

Loading

leahmcguire commented Oct 9, 2020

leahmcguire commented Oct 9, 2020

tovbinm commented Oct 9, 2020

TuanNguyen27 left a comment

nicodv left a comment

gerashegalov left a comment

gerashegalov Oct 12, 2020

gerashegalov Oct 20, 2020

gerashegalov Oct 20, 2020

gerashegalov Oct 20, 2020

salesforce-cla bot commented Nov 23, 2020

salesforce-cla bot commented Dec 12, 2020

Save model locally and convert to zip before moving to final path and do the reverse for loading #516

Save model locally and convert to zip before moving to final path and do the reverse for loading #516

Conversation

leahmcguire commented Oct 2, 2020

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

codecov bot commented Oct 4, 2020 • edited Loading

Codecov Report

leahmcguire commented Oct 9, 2020

leahmcguire commented Oct 9, 2020

tovbinm commented Oct 9, 2020

TuanNguyen27 left a comment

Choose a reason for hiding this comment

nicodv left a comment

Choose a reason for hiding this comment

gerashegalov left a 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

salesforce-cla bot commented Nov 23, 2020

salesforce-cla bot commented Dec 12, 2020

codecov bot commented Oct 4, 2020 •

edited

Loading