Share new base transforms between WDL 1.0 and 1.1 #3852

cjllanwarne · 2018-07-03T01:36:44Z

Allows reading of WDL 1.0 and 1.1 Asts through a shared set of CheckedAtoB functions, with the flexibility to inject different transform behavior into each usage of the instantiations of the transforms.

Note: of the 9013 added lines, 7484 are the new 1.1 WDL parser and presumably about 810 are files which got moved out of draft-3/transforms and into base/transforms

Horneth · 2018-07-03T15:18:00Z

@cjllanwarne Do you need a red review on this ? If so could you elaborate just a little on what

Allows reading of WDL 1.0 and 1.1 Asts through a shared set of CheckedAtoB functions, with the flexibility to inject different transform behavior into each usage of the instantiations of the transforms.

means for me poor mortal and / or point to relevant code that I should look at ? 😄

cjllanwarne · 2018-07-03T15:58:57Z

The WDL map might help:

This PR splits the top two circles into two parallel paths: Draft3 file -> Draft3 ASTs and Biscayne file -> Biscayne ASTs (I'm using Biscayne as my codename for "whatever WDL version is next, probably 1.1", because national parks are cool). The ASTs can then go through a single transform function into a shared WDL Object Model.

The clever part is that the transform function can (but doesn't yet) be customised by replacing specific "AST to WDLOM" transformations with version specific functions (those are the CheckedAtoBs, and that's why the two lists of CheckedAtoBs each version uses are identical... for now)

Horneth

Just a few ToLs.
I know there's a tech talk next week for this, I don't know if we want to wait for that or not. I won't be there for it so depending on what you decide feel free to ignore my 👍

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

...actories/wdl-biscayne/src/main/scala/languages/wdl/biscayne/WdlBiscayneLanguageFactory.scala

+      l.forall(_.isWhitespace) || l.dropWhile(_.isWhitespace).startsWith("#")
+    }
+    val start = trimStart.next.dropWhile(_.isWhitespace)
+    start.startsWith("version 1.1") || start.startsWith("version biscayne")


I don't know if there's any project the wdl factories share but if so, looks like this code could be in it since it's almost the same here and in draft 3

Horneth · 2018-07-06T18:20:05Z

...sforms/new-base/src/main/scala/wdl/transforms/base/ast2wdlom/AstNodeToMetaValueElement.scala

@@ -0,0 +1,57 @@
+//package wdl.transforms.base.ast2wdlom


Horneth · 2018-07-06T18:21:28Z

wdl/transforms/new-base/src/main/scala/wdl/transforms/base/ast2wdlom/AstNodeToTypeElement.scala

+                          (implicit astNodeToExpressionElement: CheckedAtoB[GenericAstNode, TypeElement]): Checked[TypeElement] = typeAst.getAttributeAs[String]("name") flatMap {
+    case "Array" => typeAst.getAttributeAsVector[TypeElement]("subtype") flatMap {
+      case one if one.size == 1 => ArrayTypeElement(one.head).validNelCheck
+      case other => s"Arrays must have exactly one type parameter, but got ${other.size}".invalidNelCheck


ToL: maybe also say what the provided types were in the error message ? (here and below if so)

Horneth · 2018-07-06T18:22:01Z

wdl/transforms/new-base/src/main/scala/wdl/transforms/base/ast2wdlom/AstNodeToTypeElement.scala

+  )
+
+
+


Lots of room here 😄

aednichols

Still looking, some minor comments so far. I'd love to brainstorm a clever Scala-ey way to reduce code duplication.

aednichols · 2018-07-12T14:33:20Z

.../src/main/scala/wdl/transforms/base/linking/expression/values/EngineFunctionEvaluators.scala

@@ -205,7 +205,7 @@ object EngineFunctionEvaluators {
          read <- readFile(fileToRead, ioFunctionSet, fileSizeLimitationConfig.readBoolLimit)
          asBool <- Try(read.trim.toBoolean)
        } yield WomBoolean(asBool)
-        tryResult.map(EvaluatedValue(_, Seq.empty)).toErrorOr.contextualizeErrors(s"""read_boolean("${fileToRead.value}")""")
+        tryResult.map(EvaluatedValue(_, Seq.empty)).toErrorOr.contextualizeErrors(s"""read_int("${fileToRead.value}")""")


Intentional change? Seems incorrect.

yeah... very good catch! I have no idea what caused this but it's definitely not right...

aednichols · 2018-07-12T15:08:24Z

...forms/biscayne/src/main/scala/wdl/transforms/biscayne/ast2wdlom/BiscayneGenericAstNode.scala

@@ -0,0 +1,34 @@
+package wdl.transforms.biscayne.ast2wdlom


Could we use type parameters to reduce duplication between this file and Draft3GenericAstNode.scala? The only difference seems to be the type of the Ast (wdl.draft3.parser.WdlParser.Ast vs. wdl.biscayne.parser.WdlParser.Ast).

Incomplete but illustrates what I'm getting at:

case class BaseGenericAst[T <: GenericAstNode](ast: T) extends GenericAst { override def getAttribute(attr: String): GenericAstNode = Option(ast.getAttribute(attr)).map(T.apply).orNull override def getAttributes: Map[String, GenericAstNode] = ast.getAttributes.asScala.toMap collect { case (key, value) if value != null => key -> T(value) } override def getName: String = ast.getName }

aednichols · 2018-07-13T15:55:40Z

wdl/transforms/biscayne/src/main/scala/wdl/transforms/biscayne/ast2wdlom/ast2wdlom.scala

@@ -0,0 +1,62 @@
+package wdl.transforms.biscayne


For consistency and ease of navigation, it probably makes sense to unify the file name between Biscayne and draft-3:

wdl/transforms/draft3/src/main/scala/wdl/draft3/transforms/ast2wdlom/package.scala
wdl/transforms/biscayne/src/main/scala/wdl/transforms/biscayne/ast2wdlom/ast2wdlom.scala

aednichols

Thumbing provisional on

fixing or clarifying the read_boolean() to read_int() change
addressing (or saying why not) the ast2wdlom.scala vs. package.scala comment

cjllanwarne force-pushed the cjl_wdl_1.1_packages branch from c2899fd to 41cf6f0 Compare July 3, 2018 19:07

Horneth approved these changes Jul 6, 2018

View reviewed changes

gemmalam added the 🌲Redwood label Jul 9, 2018

cjllanwarne force-pushed the cjl_wdl_1.1_packages branch 3 times, most recently from cf6d2b2 to 1d61bf0 Compare July 11, 2018 15:29

gemmalam requested a review from aednichols July 12, 2018 12:57

aednichols reviewed Jul 12, 2018

View reviewed changes

aednichols reviewed Jul 13, 2018

View reviewed changes

cjllanwarne mentioned this pull request Jul 13, 2018

Adds as_map, as_pairs and collect_by_key #3895

Merged

2 tasks

aednichols approved these changes Jul 13, 2018

View reviewed changes

cjllanwarne added 2 commits July 13, 2018 12:52

WDL Biscayne WIP

e0dfef9

PR fixup

894824d

cjllanwarne force-pushed the cjl_wdl_1.1_packages branch from 1d61bf0 to 894824d Compare July 13, 2018 16:55

cjllanwarne merged commit cbaf799 into develop Jul 16, 2018

cjllanwarne deleted the cjl_wdl_1.1_packages branch May 10, 2019 15:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Share new base transforms between WDL 1.0 and 1.1 #3852

Share new base transforms between WDL 1.0 and 1.1 #3852

cjllanwarne commented Jul 3, 2018 •

edited

Loading

Horneth commented Jul 3, 2018

cjllanwarne commented Jul 3, 2018

Horneth left a comment

Horneth Jul 6, 2018

Horneth Jul 6, 2018

Horneth Jul 6, 2018

Horneth Jul 6, 2018

aednichols left a comment

aednichols Jul 12, 2018

cjllanwarne Jul 13, 2018

aednichols Jul 12, 2018

aednichols Jul 13, 2018

aednichols left a comment •

edited

Loading

Share new base transforms between WDL 1.0 and 1.1 #3852

Share new base transforms between WDL 1.0 and 1.1 #3852

Conversation

cjllanwarne commented Jul 3, 2018 • edited Loading

Horneth commented Jul 3, 2018

cjllanwarne commented Jul 3, 2018

Horneth 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

aednichols 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

aednichols left a comment • edited Loading

Choose a reason for hiding this comment

cjllanwarne commented Jul 3, 2018 •

edited

Loading

aednichols left a comment •

edited

Loading