WDL Draft 2 workflow source + imports => namespace cache #3971

mcovarr · 2018-08-04T02:06:20Z

Particularly useful for not re-parsing workflows for every set of inputs in a batch submission.

mcovarr · 2018-08-04T17:20:19Z

languageFactories/wdl-draft2/src/main/scala/languages/wdl/draft2/WdlDraft2LanguageFactory.scala

+  private val namespaces: Cache[WorkflowSource, ErrorOr[WdlNamespaceWithWorkflow]] = CacheBuilder.newBuilder()
+    .concurrencyLevel(2)
+    .expireAfterWrite(20, TimeUnit.MINUTES)
+    .maximumSize(1000)


knobortunities

Horneth · 2018-08-04T21:20:04Z

languageFactories/wdl-draft2/src/main/scala/languages/wdl/draft2/WdlDraft2LanguageFactory.scala

+
+  private val namespaces: Cache[WorkflowSource, ErrorOr[WdlNamespaceWithWorkflow]] = CacheBuilder.newBuilder()
+    .concurrencyLevel(2)
+    .expireAfterWrite(20, TimeUnit.MINUTES)


ToL: Not that it would make much of a difference but I think this could even be expireAfterAccess ?

ooh I like that

kshakir

Not sure how the develop imports got out of sync with 34_hotfix but this PR needs minor patches.

kshakir · 2018-08-05T20:36:21Z

languageFactories/wdl-draft2/src/main/scala/languages/wdl/draft2/WdlDraft2LanguageFactory.scala

-import wom.values.WomValue
-import languages.wdl.draft2.WdlDraft2LanguageFactory._
+import wom.transforms.WomExecutableMaker.ops._
+import wom.values.{WomValue, _}


This import, and others imports this file, have been Intellij-ed and should be fixed.

cjllanwarne · 2018-08-06T14:59:26Z

languageFactories/wdl-draft2/src/main/scala/languages/wdl/draft2/WdlDraft2LanguageFactory.scala

-      case _: WorkflowSourceFilesWithoutImports =>
-        WdlNamespaceWithWorkflow.load(workflowSource, importResolvers map resolverConverter).toErrorOr
+    def workflowHashKey: String = {
+      workflowSource.md5Sum + (source.importsZipFileOption map { bytes => new String(MessageDigest.getInstance("MD5").digest(bytes)) }).getOrElse("")


I think so but to double check - are we happy to assert that MD5suming the zip file is less CPU intensive than just parsing the WDL?

It seems a pretty safe assumption that digesting the array of bytes is cheaper than stringifying it and parsing. 🙂

cjllanwarne · 2018-08-06T15:02:26Z

languageFactories/wdl-draft2/src/main/scala/languages/wdl/draft2/WdlDraft2LanguageFactory.scala

    }

+    lazy val wdlNamespaceValidation: ErrorOr[WdlNamespaceWithWorkflow] = namespaces.get(workflowHashKey, new Callable[ErrorOr[WdlNamespaceWithWorkflow]] {


The RHS of this is get is lazy, right...?

I wonder if we could make that explicit by separating it as

def lazyWorkflowMaker: () => Callable[ErrorOr[WdlNamespaceWithWorkflow]] = ... lazy val wdlNamespaceValidation: ErrorOr[WdlNamespaceWithWorkflow] = namespaces.get(workflowHashKey, lazyWorkflowMaker)

yes this is lazy.

cjllanwarne · 2018-08-06T15:50:28Z

Since this is the develop version: If I told you that one day I intend to shuffle this logic into the LanguageFactory trait and make it apply equally to WDL 1.0, 1.1, CWL, etc...

Would you trust me to do the right thing; or...
Would you want to add a regression test to make sure I don't ruin this great work?

geoffjentry · 2018-08-06T15:52:54Z

@cjllanwarne You're already on thin ice w/ test recommendations

cjllanwarne · 2018-08-06T15:58:51Z

@geoffjentry alright, alright, no need to get testy...

mcovarr

pfffft
it'd be nice to also have one for hashing so you don't break that either

mcovarr · 2018-08-06T18:45:14Z

languageFactories/wdl-draft2/src/main/scala/languages/wdl/draft2/WdlDraft2LanguageFactory.scala

-      case _: WorkflowSourceFilesWithoutImports =>
-        WdlNamespaceWithWorkflow.load(workflowSource, importResolvers map resolverConverter).toErrorOr
+    def workflowHashKey: String = {
+      workflowSource.md5Sum + (source.importsZipFileOption map { bytes => new String(MessageDigest.getInstance("MD5").digest(bytes)) }).getOrElse("")


It seems a pretty safe assumption that digesting the array of bytes is cheaper than stringifying it and parsing. 🙂

mcovarr · 2018-08-06T18:52:09Z

languageFactories/wdl-draft2/src/main/scala/languages/wdl/draft2/WdlDraft2LanguageFactory.scala

    }

+    lazy val wdlNamespaceValidation: ErrorOr[WdlNamespaceWithWorkflow] = namespaces.get(workflowHashKey, new Callable[ErrorOr[WdlNamespaceWithWorkflow]] {


yes this is lazy.

mcovarr · 2018-08-08T00:18:50Z

@cjllanwarne @kshakir ready for re-review.

kshakir

Still LGTM

cjllanwarne

Looks good, but I do think we want a more scary CHANGELOG entry for the hotfixees who may not realize they have to opt-in too

cjllanwarne · 2018-08-08T18:22:16Z

cromwell.examples.conf

+        #     # WDL Draft 2 namespace caching is off by default, this value must be set to true to enable it.
+        #     enabled: false
+        #     # Guava cache concurrency
+        #     concurrency: 2


What does concurrency mean in cache terms?

It's literally "Guava cache concurrency" in that this parameter is passed to the cache builder's .concurrency() method.

cjllanwarne · 2018-08-08T18:22:38Z

cromwell.examples.conf

+        #     concurrency: 2
+        #     # How long entries in the cache should live from the time of their last access.
+        #     ttl: 20 minutes
+        #     # Maximum size of the cache.


(in terms of WdlNamespace objects stored)

cjllanwarne · 2018-08-08T18:28:25Z

CHANGELOG.md

@@ -7,14 +7,22 @@
 Cromwell now allows for a user to submit the URL pointing to workflow file to run a workflow using `workflowUrl` parameter. Currently, this is only supported in `Server` mode.
 More details on how to use it can be found [here](http://cromwell.readthedocs.io/en/develop/api/RESTAPI/).

+### Languages
+
+- Added an opt-in namespace cache for the WDL Draft 2 language factory. Please see the Cromwell example configuration for details.


Maybe warn that if people are upgrading from hotfix, their currently enabled cache is now opt-in rather than on-by-default?

mcovarr force-pushed the mlc_memory_spike branch from 411c5e8 to ef118ff Compare August 4, 2018 02:07

mcovarr mentioned this pull request Aug 4, 2018

[34] WDL Draft 2 workflow source + imports => namespace cache #3972

Merged

mcovarr commented Aug 4, 2018

View reviewed changes

Horneth approved these changes Aug 4, 2018

View reviewed changes

kshakir approved these changes Aug 5, 2018

View reviewed changes

cjllanwarne reviewed Aug 6, 2018

View reviewed changes

mcovarr force-pushed the mlc_memory_spike branch from ef118ff to e1f20c0 Compare August 6, 2018 15:21

mcovarr commented Aug 6, 2018

View reviewed changes

mcovarr force-pushed the mlc_memory_spike branch 6 times, most recently from 8c7d257 to 2236a4a Compare August 7, 2018 21:24

mcovarr added the Please re-review now label Aug 8, 2018

kshakir approved these changes Aug 8, 2018

View reviewed changes

cjllanwarne approved these changes Aug 8, 2018

View reviewed changes

cjllanwarne added Back With Originator ♻️ and removed Please re-review now labels Aug 8, 2018

Cache v2 namespaces.

b6a6e87

mcovarr force-pushed the mlc_memory_spike branch from 2236a4a to b6a6e87 Compare August 8, 2018 20:45

mcovarr merged commit 9b86643 into develop Aug 8, 2018

mcovarr deleted the mlc_memory_spike branch August 8, 2018 20:46

mcovarr added a commit that referenced this pull request Aug 17, 2018

Cache v2 namespaces. (#3971)

4010c5a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WDL Draft 2 workflow source + imports => namespace cache #3971

WDL Draft 2 workflow source + imports => namespace cache #3971

mcovarr commented Aug 4, 2018

mcovarr Aug 4, 2018

Horneth Aug 4, 2018

mcovarr Aug 4, 2018

kshakir left a comment

kshakir Aug 5, 2018

cjllanwarne Aug 6, 2018

mcovarr Aug 6, 2018

cjllanwarne Aug 6, 2018

cjllanwarne Aug 6, 2018 •

edited

Loading

mcovarr Aug 6, 2018

cjllanwarne commented Aug 6, 2018

geoffjentry commented Aug 6, 2018

cjllanwarne commented Aug 6, 2018

mcovarr left a comment

mcovarr Aug 6, 2018

mcovarr Aug 6, 2018

mcovarr commented Aug 8, 2018

kshakir left a comment

cjllanwarne left a comment

cjllanwarne Aug 8, 2018

mcovarr Aug 8, 2018

cjllanwarne Aug 8, 2018

cjllanwarne Aug 8, 2018

		}

		lazy val wdlNamespaceValidation: ErrorOr[WdlNamespaceWithWorkflow] = namespaces.get(workflowHashKey, new Callable[ErrorOr[WdlNamespaceWithWorkflow]] {

WDL Draft 2 workflow source + imports => namespace cache #3971

WDL Draft 2 workflow source + imports => namespace cache #3971

Conversation

mcovarr commented Aug 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kshakir 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

cjllanwarne Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjllanwarne commented Aug 6, 2018

geoffjentry commented Aug 6, 2018

cjllanwarne commented Aug 6, 2018

mcovarr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcovarr commented Aug 8, 2018

kshakir left a comment

Choose a reason for hiding this comment

cjllanwarne 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

cjllanwarne Aug 6, 2018 •

edited

Loading