NIO parameter_meta flag for localization optimization #3738

cjllanwarne · 2018-06-06T22:33:21Z

No description provided.

Horneth · 2018-06-06T22:47:54Z

...ala/cromwell/backend/google/pipelines/common/PipelinesApiAsyncBackendJobExecutionActor.scala

-      }
+  override lazy val inputsToNotLocalize: Set[WomFile] = {
+    val result = jobDescriptor.findInputFilesByParameterMeta {
+      // TODO: Is "nio" the right name?


I feel like there potentially could be use cases where one does not want a file to be localized for other reasons than using NIO ? It also somehow conveys the feeling that this file will be accessed by the task using NIO which there's really no way to be sure of from the WDL/engine side of things.
OTOH I don't really care.. 😄

maybe { localize: false }?

I totally agree and nio is the shortest placeholder I could come up with as I was writing it... what I really want it to convey is that localization is optional, and the engine is allowed but not required to not localize it

isn't it a bit stronger than that? like ideally the engine should not localize the file if possible?

{ localize: preferably_no }

@Horneth To be clear there should be no fuzziness on the WDL/engine side of things as this is 100% a Cromwell feature and not a WDL feature.

hmm not sure what you mean. It's a Cromwell feature that can only be taken advantage of through WDL for now (once this gets merged). It also involves a WDL syntax

Right but there's 0% expectation that it'll work anywhere but Cromwell. It's effectively piggybacking on existing WDL syntax to create a hint

And in theory if we adopted a CWL ParameterMetaHint or LocalizationOptionalHint, we could allow this optimization in CWL workflows too

{ localizationOptional: true }? (getting a bit long)

mcovarr · 2018-06-07T15:12:34Z

...ala/cromwell/backend/google/pipelines/common/PipelinesApiAsyncBackendJobExecutionActor.scala

-      }
+  override lazy val inputsToNotLocalize: Set[WomFile] = {
+    val result = jobDescriptor.findInputFilesByParameterMeta {
+      // TODO: Is "nio" the right name?


maybe { localize: false }?

mcovarr · 2018-06-07T15:14:45Z

backend/src/main/scala/cromwell/backend/standard/StandardAsyncExecutionActor.scala

@@ -730,7 +731,8 @@ trait StandardAsyncExecutionActor extends AsyncBackendJobExecutionActor with Sta
    * @return The Try wrapped and mapped WOM value.
    */
  final def outputValueMapper(womValue: WomValue): Try[WomValue] = {
-    WomFileMapper.mapWomFiles(mapOutputWomFile)(womValue)
+    // TODO: is this inputsToNot*DE*Localize?


why would inputs be delocalized?

oops - this should really be outputsToNotDelocalize, and since the inputsToNotDelocalize is not relevant to that, I think this should probably just hard-code Set.empty for now

mcovarr · 2018-06-07T15:18:03Z

...ala/cromwell/backend/google/pipelines/common/PipelinesApiAsyncBackendJobExecutionActor.scala

+//      case (_, file: WomFile) => inputsToNotLocalize.contains(file)
+//      case (_, WomOptionalValue(_, Some(file: WomFile))) => inputsToNotLocalize.contains(file)
+//      case _ => false
+//    }


mcovarr · 2018-06-07T15:20:03Z

...forms/draft3/src/main/scala/wdl/draft3/transforms/ast2wdlom/AstToTaskDefinitionElement.scala

+    val parameterMetaSectionElement: ErrorOr[Option[ParameterMetaSectionElement]] = for {
+      maxOne <- validateOneMax(bodyElements.filterByType[ParameterMetaSectionElement], "parameterMeta"): ErrorOr[Option[ParameterMetaSectionElement]]
+
+    } yield maxOne


one line forcomp = map and maybe you can do away with one of the ascriptions

geoffjentry · 2018-06-07T15:44:32Z

centaur/src/main/resources/standardTestCases/wdl_draft3/draft3_nio_file/draft3_nio_file.wdl

@@ -0,0 +1,118 @@
+version 1.0
+
+workflow draft3_nio_file {


name conflicts with version string above

geoffjentry · 2018-06-07T15:46:28Z

@cjllanwarne It'd be helpful if you could add a description of how this is intended to work, I can kinda see it in the tests but want to make sure I'm not interpreting things incorrectly

aednichols · 2018-06-07T17:49:15Z

backend/src/main/scala/cromwell/backend/standard/StandardAsyncExecutionActor.scala

@@ -145,14 +144,16 @@ trait StandardAsyncExecutionActor extends AsyncBackendJobExecutionActor with Sta
  // keeping track of the paths cleanly without so many value mappers 
  def mapCommandLineJobInputWomFile(womFile: WomFile): WomFile = mapCommandLineWomFile(womFile)

+  def inputsToNotLocalize: Set[WomFile] = Set.empty


Would be nice to explain here that this optimization is flipped on/off on a per-backend basis, with a default of off

aednichols · 2018-06-07T17:54:30Z

...ala/cromwell/backend/google/pipelines/common/PipelinesApiAsyncBackendJobExecutionActor.scala

-      }
+  override lazy val inputsToNotLocalize: Set[WomFile] = {
+    val result = jobDescriptor.findInputFilesByParameterMeta {
+      // TODO: Is "nio" the right name?


{ localizationOptional: true }? (getting a bit long)

cjllanwarne · 2018-06-07T18:23:36Z

@geoffjentry and @aednichols regarding documentation I added this: https://github.com/broadinstitute/cromwell/blob/cjl_nio_meta/docs/optimizations/FileLocalization.md

Is that sufficient of is there more to be said?

aednichols · 2018-06-07T18:29:38Z

Looks good other than "perhaps" might be a bit confusing - someone could be led to believe that there is additional non-determinism within Cromwell, beyond specifying the parameter_meta bit and a compatible backend

aednichols · 2018-06-07T18:31:05Z

Spitballing:

It allows you to save time and money by not localizing some Files to the execution environment when running on compatible backends

Horneth · 2018-06-07T18:54:36Z

docs/optimizations/FileLocalization.md

@@ -0,0 +1,52 @@
+# The 'localization_optional' Optimization


Sounds like the title of a Robert Ludlum novel 😄

Or the title of a Big Bang Theory episode

katevoss · 2018-06-07T18:47:23Z

mkdocs.yml

@@ -32,6 +32,8 @@ pages:
 - Workflow Options:
  - Overview: wf_options/Overview.md
  - Google Cloud: wf_options/Google.md
+- Workflow Optimizations
+  - "Task inputs: localization_optional": optimizations/FileLocalization.md


Unnecessary quotes, it should be:

- Workflow Optimizations - Task inputs: localization_optional: optimizations/FileLocalization.md

Or did you use quotes so that you could use a : in the name on the menu?

Yeah, it's needed for the colon

katevoss · 2018-06-07T18:50:07Z

docs/optimizations/FileLocalization.md

@@ -0,0 +1,52 @@
+# The 'localization_optional' Optimization


"Optimization" is a new section to the docs. It would be great if you could include a sentence or two about the types of optimizations that users should expect to see here in the future. What are these optimizations for? Why would a user use this one? What are the draw backs, and when would you NOT recommend it for users?

Sure - are you saying we should have some hierarchy like this?

- Workflow Optimizations - Workflow Optimizations: optimizations/optimizations.md - "Task inputs: localization_optional": optimizations/FileLocalization.md

I think the hierarchy sounds good 👍 optimizations.md doesn't have to be very long, just a brief introduction to what users should expect to see there.

@katevoss added!

katevoss · 2018-06-07T18:51:57Z

docs/optimizations/FileLocalization.md

+## Scope
+
+The 'localization_optional' optimization applies to `File` and `File?` inputs on `task`s. It allows
+you to save time and money by perhaps not localizing some `File`s to the execution environment.


This is unclear: "perhaps not localizing...". Can you explain why it may or may not localize a file? Even if you go into more depth later in the doc, it's best to have a short summary at the top so users don't have to read it all.

Are you implying that somebody might not want to read all of my glorious prose?

katevoss · 2018-06-07T18:55:04Z

docs/optimizations/FileLocalization.md

+## Effect
+
+The optimization signals to Cromwell that the task has been written in such a way that:
+ * The task will still work if the file were to be localized - say from a cloud bucket to the task's execution environment; but...


Suggested rephrase:

The task will work if Cromwell localizes the file

For example, a file from a cloud bucket is localized to the task's execution environment;

katevoss · 2018-06-07T19:01:06Z

docs/optimizations/FileLocalization.md

@@ -0,0 +1,52 @@
+# The 'localization_optional' Optimization


Or the title of a Big Bang Theory episode

katevoss · 2018-06-07T19:02:20Z

docs/optimizations/FileLocalization.md

+
+The optimization signals to Cromwell that the task has been written in such a way that:
+ * The task will still work if the file were to be localized - say from a cloud bucket to the task's execution environment; but...
+ * The task will *also* work if the file is *not* localized prior to execution - and remains a path to a value in an object store, for example.  


Suggested rephrase:

AND

The task will also work if Cromwell does not localize the file

For example, with "localization_optional" enabled, a file will remain as a path to a cloud bucket.

Questions:

Why did you include the information "prior to execution"?

Is a "cloud bucket" a more specific example of a "value in an object store"? If not, what's a clearer way of saying "path to a cloud bucket" or "value in object store"?

Artistic flourish. Will remove

I'll change them to be consistently "in an object store"

I think that this might be too strong:

For example, with "localization_optional" enabled, a file will remain as a path to a cloud bucket.

ie just because localization_optional is specified, it doesn't force Cromwell to not localize the file

katevoss · 2018-06-07T19:02:56Z

docs/optimizations/FileLocalization.md

+ * The task will still work if the file were to be localized - say from a cloud bucket to the task's execution environment; but...
+ * The task will *also* work if the file is *not* localized prior to execution - and remains a path to a value in an object store, for example.  
+
+Therefore, Cromwell will (usually) choose not to localize this file, in order to save time and money on disk size and `File` localization.


When will Cromwell choose to localize the file, even when "localization_optional" is enabled?

katevoss · 2018-06-07T19:06:17Z

docs/optimizations/FileLocalization.md

+
+* Your task should always be written bearing in mind that a `File` tagged for this optimization
+ *might* still be localized at the whim of the execution engine. 
+* Furthermore, if your WDL is intended to be shared with some other group, that group might 


Suggested rephrase:

If you share your WDL it may be run on engines other than Cromwell, and that engine may not recognize this hint.

What is a hint? I think it is a new concept in Cromwell-WDL-land, I think it warrants a sentence or two explanation.

katevoss · 2018-06-07T19:06:51Z

CHANGELOG.md

@@ -2,6 +2,10 @@

 ## 33 Release Notes

+### File Localization (NIO) Hint
+
+Cromwell now allows tasks in WDL 1.0 can now specify a hint in their `parameter_meta` that some `File` inputs do not need to be localized for the task to run successfully.


Include a link to the new doc in the release notes

katevoss · 2018-06-08T17:21:01Z

docs/optimizations/FileLocalization.md

@@ -42,11 +53,10 @@ task nio_task {

 This optimization is currently only applied to localization in the Pipelines API (GCE) backends.

-## Warning
+## Portability Warning

 Be very careful not to write unportable WDL files!


TOL: try to avoid double-negatives. Which is to say, write in the positive tone.

"Be very careful to write portable WDL files", rather than "Be very careful not to write unportable WDL files"

aednichols

👍 LGTM

Horneth reviewed Jun 6, 2018

View reviewed changes

mcovarr reviewed Jun 7, 2018

View reviewed changes

geoffjentry reviewed Jun 7, 2018

View reviewed changes

gemmalam added 🌲Redwood labels Jun 7, 2018

aednichols reviewed Jun 7, 2018

View reviewed changes

Horneth reviewed Jun 7, 2018

View reviewed changes

katevoss reviewed Jun 7, 2018

View reviewed changes

katevoss reviewed Jun 8, 2018

View reviewed changes

mcovarr approved these changes Jun 11, 2018

View reviewed changes

cjllanwarne force-pushed the cjl_nio_meta branch from f11c5e3 to 6b8a1cf Compare June 12, 2018 16:10

cjllanwarne removed the 👍Red Thumb Required 🙏 label Jun 12, 2018

cjllanwarne force-pushed the cjl_nio_meta branch from 6b8a1cf to 45816af Compare June 12, 2018 18:58

cjllanwarne mentioned this pull request Jun 13, 2018

Extend localization_optional to complex types (for least surprise) #3771

Merged

1 task

aednichols approved these changes Jun 13, 2018

View reviewed changes

NIO parameter_meta flag for localization optimization

e3ba75f

cjllanwarne force-pushed the cjl_nio_meta branch from 45816af to e3ba75f Compare June 13, 2018 21:58

cjllanwarne merged commit 9619c08 into develop Jun 13, 2018

katevoss mentioned this pull request Jun 14, 2018

Added line return to doc so bullet points show up correctly #3779

Merged

cjllanwarne deleted the cjl_nio_meta branch May 10, 2019 15:22

NIO parameter_meta flag for localization optimization #3738

NIO parameter_meta flag for localization optimization #3738

Conversation

cjllanwarne commented Jun 6, 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

mcovarr Jun 7, 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

aednichols Jun 7, 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

Choose a reason for hiding this comment

geoffjentry commented Jun 7, 2018

Choose a reason for hiding this comment

aednichols Jun 7, 2018 • edited

Choose a reason for hiding this comment

cjllanwarne commented Jun 7, 2018

aednichols commented Jun 7, 2018

aednichols commented Jun 7, 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

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

cjllanwarne Jun 7, 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

aednichols left a comment

Choose a reason for hiding this comment

mcovarr Jun 7, 2018 •

edited

aednichols Jun 7, 2018 •

edited

aednichols Jun 7, 2018 •

edited

cjllanwarne Jun 7, 2018 •

edited