From 984da7dc5ba939081656ee2433c318be566ef4be Mon Sep 17 00:00:00 2001 From: Adam Nichols Date: Tue, 12 Mar 2024 13:06:24 -0400 Subject: [PATCH 1/9] Compiles and works --- .../expression/values/LiteralEvaluators.scala | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/wdl/transforms/new-base/src/main/scala/wdl/transforms/base/linking/expression/values/LiteralEvaluators.scala b/wdl/transforms/new-base/src/main/scala/wdl/transforms/base/linking/expression/values/LiteralEvaluators.scala index 4831c34e76b..f78ddd48272 100644 --- a/wdl/transforms/new-base/src/main/scala/wdl/transforms/base/linking/expression/values/LiteralEvaluators.scala +++ b/wdl/transforms/new-base/src/main/scala/wdl/transforms/base/linking/expression/values/LiteralEvaluators.scala @@ -5,6 +5,7 @@ import cats.syntax.traverse._ import cats.syntax.validated._ import cats.instances.list._ import common.validation.ErrorOr.ErrorOr +import common.validation.Validation.TryValidation import wdl.model.draft3.elements.ExpressionElement import wdl.model.draft3.elements.ExpressionElement._ import wdl.model.draft3.graph.expression.{EvaluatedValue, ForCommandInstantiationOptions, ValueEvaluator} @@ -12,6 +13,8 @@ import wdl.model.draft3.graph.expression.ValueEvaluator.ops._ import wom.expression.IoFunctionSet import wom.values.{WomArray, WomMap, WomObject, WomPair, WomString, WomValue} +import scala.util.Try + object LiteralEvaluators { implicit val primitiveValueEvaluator: ValueEvaluator[PrimitiveLiteralExpressionElement] = @@ -84,11 +87,17 @@ object LiteralEvaluators { ) mapN { (key, value) => key -> value } } - evaluated map { kvps => - val value = kvps.map(entry => entry._1.value -> entry._2.value).toMap - val sideEffectFiles = kvps.flatMap(entry => entry._1.sideEffectFiles ++ entry._2.sideEffectFiles) - EvaluatedValue(WomMap(value.toMap), sideEffectFiles) - } + // Expose `flatten` to handle the `ErrorOr[ErrorOr[]]` + import common.validation.ErrorOr.NestedErrorOr + + (evaluated map { kvps => + val tryResult: Try[EvaluatedValue[_ <: WomValue]] = Try { + val value = kvps.map(entry => entry._1.value -> entry._2.value).toMap + val sideEffectFiles = kvps.flatMap(entry => entry._1.sideEffectFiles ++ entry._2.sideEffectFiles) + EvaluatedValue(WomMap.apply(value.toMap), sideEffectFiles) + } + tryResult.toErrorOr + }).flatten } } From 6fdb9e5e108ecf03093b8ffb47c7028742c03c5a Mon Sep 17 00:00:00 2001 From: Adam Nichols Date: Tue, 12 Mar 2024 13:26:10 -0400 Subject: [PATCH 2/9] Simplify --- .../expression/values/LiteralEvaluators.scala | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/wdl/transforms/new-base/src/main/scala/wdl/transforms/base/linking/expression/values/LiteralEvaluators.scala b/wdl/transforms/new-base/src/main/scala/wdl/transforms/base/linking/expression/values/LiteralEvaluators.scala index f78ddd48272..b2869dfc63f 100644 --- a/wdl/transforms/new-base/src/main/scala/wdl/transforms/base/linking/expression/values/LiteralEvaluators.scala +++ b/wdl/transforms/new-base/src/main/scala/wdl/transforms/base/linking/expression/values/LiteralEvaluators.scala @@ -4,8 +4,8 @@ import cats.syntax.apply._ import cats.syntax.traverse._ import cats.syntax.validated._ import cats.instances.list._ +import common.validation.ErrorOr import common.validation.ErrorOr.ErrorOr -import common.validation.Validation.TryValidation import wdl.model.draft3.elements.ExpressionElement import wdl.model.draft3.elements.ExpressionElement._ import wdl.model.draft3.graph.expression.{EvaluatedValue, ForCommandInstantiationOptions, ValueEvaluator} @@ -13,8 +13,6 @@ import wdl.model.draft3.graph.expression.ValueEvaluator.ops._ import wom.expression.IoFunctionSet import wom.values.{WomArray, WomMap, WomObject, WomPair, WomString, WomValue} -import scala.util.Try - object LiteralEvaluators { implicit val primitiveValueEvaluator: ValueEvaluator[PrimitiveLiteralExpressionElement] = @@ -87,16 +85,13 @@ object LiteralEvaluators { ) mapN { (key, value) => key -> value } } - // Expose `flatten` to handle the `ErrorOr[ErrorOr[]]` + // Expose `flatten` to handle the `ErrorOr[ErrorOr[]]` [WX-757] import common.validation.ErrorOr.NestedErrorOr (evaluated map { kvps => - val tryResult: Try[EvaluatedValue[_ <: WomValue]] = Try { - val value = kvps.map(entry => entry._1.value -> entry._2.value).toMap - val sideEffectFiles = kvps.flatMap(entry => entry._1.sideEffectFiles ++ entry._2.sideEffectFiles) - EvaluatedValue(WomMap.apply(value.toMap), sideEffectFiles) - } - tryResult.toErrorOr + val value = kvps.map(entry => entry._1.value -> entry._2.value).toMap + val sideEffectFiles = kvps.flatMap(entry => entry._1.sideEffectFiles ++ entry._2.sideEffectFiles) + ErrorOr(EvaluatedValue(WomMap.apply(value.toMap), sideEffectFiles)) }).flatten } } From 33b7031d845416a303ee1acfdc57814e0ea35faf Mon Sep 17 00:00:00 2001 From: Adam Nichols Date: Tue, 12 Mar 2024 13:26:26 -0400 Subject: [PATCH 3/9] Add test --- .../nested_struct_bad_instantiation.inputs | 4 +++ .../nested_struct_bad_instantiation.wdl | 27 +++++++++++++++++++ .../nested_struct_bad_instantiation.test | 14 ++++++++++ 3 files changed, 45 insertions(+) create mode 100644 centaur/src/main/resources/standardTestCases/failures/nested_struct_bad_instantiation/nested_struct_bad_instantiation.inputs create mode 100644 centaur/src/main/resources/standardTestCases/failures/nested_struct_bad_instantiation/nested_struct_bad_instantiation.wdl create mode 100644 centaur/src/main/resources/standardTestCases/nested_struct_bad_instantiation.test diff --git a/centaur/src/main/resources/standardTestCases/failures/nested_struct_bad_instantiation/nested_struct_bad_instantiation.inputs b/centaur/src/main/resources/standardTestCases/failures/nested_struct_bad_instantiation/nested_struct_bad_instantiation.inputs new file mode 100644 index 00000000000..6f8cb64990a --- /dev/null +++ b/centaur/src/main/resources/standardTestCases/failures/nested_struct_bad_instantiation/nested_struct_bad_instantiation.inputs @@ -0,0 +1,4 @@ +{ + "MinimalStructExample.test": "Hello World", + "MinimalStructExample.integer": 2 +} diff --git a/centaur/src/main/resources/standardTestCases/failures/nested_struct_bad_instantiation/nested_struct_bad_instantiation.wdl b/centaur/src/main/resources/standardTestCases/failures/nested_struct_bad_instantiation/nested_struct_bad_instantiation.wdl new file mode 100644 index 00000000000..dce55da0bae --- /dev/null +++ b/centaur/src/main/resources/standardTestCases/failures/nested_struct_bad_instantiation/nested_struct_bad_instantiation.wdl @@ -0,0 +1,27 @@ +version 1.0 + + +struct firstLayer { + String first + Int number +} + +struct secondLayer { + String second + firstLayer lowerLayer +} + + +workflow MinimalStructExample { + input { + String test + Int integer + } + + firstLayer example1 = {"first": test, "number": integer} + secondLayer example2 = {"second": test, "lowerLayer": example1} + + output { + String example3 = example2.second + } +} diff --git a/centaur/src/main/resources/standardTestCases/nested_struct_bad_instantiation.test b/centaur/src/main/resources/standardTestCases/nested_struct_bad_instantiation.test new file mode 100644 index 00000000000..c77bc78d80a --- /dev/null +++ b/centaur/src/main/resources/standardTestCases/nested_struct_bad_instantiation.test @@ -0,0 +1,14 @@ +name: nested_struct_bad_instantiation +testFormat: workflowfailure + +files { + workflow: failures/nested_struct_bad_instantiation/nested_struct_bad_instantiation.wdl + inputs: failures/nested_struct_bad_instantiation/nested_struct_bad_instantiation.inputs +} + +metadata { + workflowName: MinimalStructExample + status: Failed + "failures.0.message": "Workflow failed" + "failures.0.causedBy.0.message": "Failed to evaluate 'example2' (reason 1 of 1): Evaluating { "second": test, "lowerLayer": example1 } failed: Cannot construct WomMapType(WomStringType,WomAnyType) with mixed types in map values: [WomString(Hello World), WomObject(Map(first -> WomString(Hello World), number -> WomInteger(2)),WomCompositeType(Map(first -> WomStringType, number -> WomIntegerType),Some(firstLayer)))]" +} From 3884ef67ac58bfc5f5dbd198811214b2fe38433d Mon Sep 17 00:00:00 2001 From: Adam Nichols Date: Tue, 12 Mar 2024 13:49:52 -0400 Subject: [PATCH 4/9] Customize exception, eliminate stack trace --- .../lifecycle/execution/keys/ExpressionKey.scala | 4 ++-- .../engine/workflow/lifecycle/execution/package.scala | 10 ++++++++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/engine/src/main/scala/cromwell/engine/workflow/lifecycle/execution/keys/ExpressionKey.scala b/engine/src/main/scala/cromwell/engine/workflow/lifecycle/execution/keys/ExpressionKey.scala index 5fd8e36cac5..51183af9f04 100644 --- a/engine/src/main/scala/cromwell/engine/workflow/lifecycle/execution/keys/ExpressionKey.scala +++ b/engine/src/main/scala/cromwell/engine/workflow/lifecycle/execution/keys/ExpressionKey.scala @@ -6,7 +6,7 @@ import common.validation.ErrorOr._ import common.validation.Validation._ import cromwell.core.ExecutionIndex._ import cromwell.core.{ExecutionStatus, JobKey} -import cromwell.engine.workflow.lifecycle.execution.WorkflowExecutionDiff +import cromwell.engine.workflow.lifecycle.execution.{WdlRuntimeException, WorkflowExecutionDiff} import cromwell.engine.workflow.lifecycle.execution.keys.ExpressionKey.{ ExpressionEvaluationFailedResponse, ExpressionEvaluationSucceededResponse @@ -32,7 +32,7 @@ final case class ExpressionKey(node: ExpressionNodeLike, index: ExecutionIndex) .contextualizeErrors(s"evaluate '${node.fullyQualifiedName}'") match { case Right(result) => workflowExecutionActor ! ExpressionEvaluationSucceededResponse(this, result) case Left(f) => - workflowExecutionActor ! ExpressionEvaluationFailedResponse(this, new RuntimeException(f.toList.mkString(", "))) + workflowExecutionActor ! ExpressionEvaluationFailedResponse(this, WdlRuntimeException(f.toList.mkString(", "))) } WorkflowExecutionDiff(Map(this -> ExecutionStatus.Running)).validNel diff --git a/engine/src/main/scala/cromwell/engine/workflow/lifecycle/execution/package.scala b/engine/src/main/scala/cromwell/engine/workflow/lifecycle/execution/package.scala index 52fbe868f1e..e1695e1d355 100644 --- a/engine/src/main/scala/cromwell/engine/workflow/lifecycle/execution/package.scala +++ b/engine/src/main/scala/cromwell/engine/workflow/lifecycle/execution/package.scala @@ -5,6 +5,16 @@ package execution { import cromwell.core.CallKey import wom.values.WomEvaluatedCallInputs + import scala.util.control.NoStackTrace + final case class JobRunning(key: CallKey, inputs: WomEvaluatedCallInputs) final case class JobStarting(callKey: CallKey) + + /** + * An exception specific to conditions inside the executing WDL, as opposed to one that is "Cromwell's fault" + * @param message Description suitable for user display + */ + final case class WdlRuntimeException(message: String) extends RuntimeException with NoStackTrace { + override def getMessage: String = message + } } From 81fcc4664c0e26c4a3130bf2d5b6f7414fcd73a1 Mon Sep 17 00:00:00 2001 From: Adam Nichols Date: Tue, 12 Mar 2024 14:42:06 -0400 Subject: [PATCH 5/9] Fix quotes --- .../standardTestCases/nested_struct_bad_instantiation.test | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/centaur/src/main/resources/standardTestCases/nested_struct_bad_instantiation.test b/centaur/src/main/resources/standardTestCases/nested_struct_bad_instantiation.test index c77bc78d80a..a2608a0262c 100644 --- a/centaur/src/main/resources/standardTestCases/nested_struct_bad_instantiation.test +++ b/centaur/src/main/resources/standardTestCases/nested_struct_bad_instantiation.test @@ -10,5 +10,5 @@ metadata { workflowName: MinimalStructExample status: Failed "failures.0.message": "Workflow failed" - "failures.0.causedBy.0.message": "Failed to evaluate 'example2' (reason 1 of 1): Evaluating { "second": test, "lowerLayer": example1 } failed: Cannot construct WomMapType(WomStringType,WomAnyType) with mixed types in map values: [WomString(Hello World), WomObject(Map(first -> WomString(Hello World), number -> WomInteger(2)),WomCompositeType(Map(first -> WomStringType, number -> WomIntegerType),Some(firstLayer)))]" + "failures.0.causedBy.0.message": "Failed to evaluate 'example2' (reason 1 of 1): Evaluating { \"second\": test, \"lowerLayer\": example1 } failed: Cannot construct WomMapType(WomStringType,WomAnyType) with mixed types in map values: [WomString(Hello World), WomObject(Map(first -> WomString(Hello World), number -> WomInteger(2)),WomCompositeType(Map(first -> WomStringType, number -> WomIntegerType),Some(firstLayer)))]" } From 49ad42e7ffa0e37c0d3540b3fc2ca98181315fe3 Mon Sep 17 00:00:00 2001 From: Adam Nichols Date: Tue, 12 Mar 2024 17:48:58 -0400 Subject: [PATCH 6/9] Fix `WomArray` case from 2024-02-20 --- .../base/linking/expression/values/LiteralEvaluators.scala | 1 + 1 file changed, 1 insertion(+) diff --git a/wdl/transforms/new-base/src/main/scala/wdl/transforms/base/linking/expression/values/LiteralEvaluators.scala b/wdl/transforms/new-base/src/main/scala/wdl/transforms/base/linking/expression/values/LiteralEvaluators.scala index b2869dfc63f..7a31b51573d 100644 --- a/wdl/transforms/new-base/src/main/scala/wdl/transforms/base/linking/expression/values/LiteralEvaluators.scala +++ b/wdl/transforms/new-base/src/main/scala/wdl/transforms/base/linking/expression/values/LiteralEvaluators.scala @@ -6,6 +6,7 @@ import cats.syntax.validated._ import cats.instances.list._ import common.validation.ErrorOr import common.validation.ErrorOr.ErrorOr +import common.validation.ErrorOr.NestedErrorOr import wdl.model.draft3.elements.ExpressionElement import wdl.model.draft3.elements.ExpressionElement._ import wdl.model.draft3.graph.expression.{EvaluatedValue, ForCommandInstantiationOptions, ValueEvaluator} From 32b66b795a0223b20e2aa7ef06bb78241faa994b Mon Sep 17 00:00:00 2001 From: Adam Nichols Date: Tue, 12 Mar 2024 17:50:16 -0400 Subject: [PATCH 7/9] Fix `WomArray` issue from 2024-02-20 --- .../linking/expression/values/LiteralEvaluators.scala | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/wdl/transforms/new-base/src/main/scala/wdl/transforms/base/linking/expression/values/LiteralEvaluators.scala b/wdl/transforms/new-base/src/main/scala/wdl/transforms/base/linking/expression/values/LiteralEvaluators.scala index 7a31b51573d..e981b103688 100644 --- a/wdl/transforms/new-base/src/main/scala/wdl/transforms/base/linking/expression/values/LiteralEvaluators.scala +++ b/wdl/transforms/new-base/src/main/scala/wdl/transforms/base/linking/expression/values/LiteralEvaluators.scala @@ -86,9 +86,6 @@ object LiteralEvaluators { ) mapN { (key, value) => key -> value } } - // Expose `flatten` to handle the `ErrorOr[ErrorOr[]]` [WX-757] - import common.validation.ErrorOr.NestedErrorOr - (evaluated map { kvps => val value = kvps.map(entry => entry._1.value -> entry._2.value).toMap val sideEffectFiles = kvps.flatMap(entry => entry._1.sideEffectFiles ++ entry._2.sideEffectFiles) @@ -108,11 +105,11 @@ object LiteralEvaluators { entry.evaluateValue(inputs, ioFunctionSet, forCommandInstantiationOptions) } - evaluated map { evaluateds => + (evaluated map { evaluateds => val value = evaluateds.map(_.value) val sideEffectFiles = evaluateds.flatMap(_.sideEffectFiles) - EvaluatedValue(WomArray(value), sideEffectFiles) - } + ErrorOr(EvaluatedValue(WomArray.apply(value), sideEffectFiles)) + }).flatten } } From 35f47943308cc5f447c3a479e5aac718619c94a7 Mon Sep 17 00:00:00 2001 From: Adam Nichols Date: Tue, 12 Mar 2024 22:07:04 -0400 Subject: [PATCH 8/9] Deflake test? --- .../lifecycle/execution/keys/ConditionalKey.scala | 8 +++++--- .../src/test/scala/cromwell/SimpleWorkflowActorSpec.scala | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/engine/src/main/scala/cromwell/engine/workflow/lifecycle/execution/keys/ConditionalKey.scala b/engine/src/main/scala/cromwell/engine/workflow/lifecycle/execution/keys/ConditionalKey.scala index 7cc18c3b964..8114b6cf4f5 100644 --- a/engine/src/main/scala/cromwell/engine/workflow/lifecycle/execution/keys/ConditionalKey.scala +++ b/engine/src/main/scala/cromwell/engine/workflow/lifecycle/execution/keys/ConditionalKey.scala @@ -1,6 +1,7 @@ package cromwell.engine.workflow.lifecycle.execution.keys import cats.syntax.validated._ +import common.validation.ErrorOr import common.validation.ErrorOr.ErrorOr import cromwell.backend.BackendJobDescriptorKey import cromwell.core.ExecutionIndex.ExecutionIndex @@ -68,9 +69,10 @@ private[execution] case class ConditionalKey(node: ConditionalNode, index: Execu node.conditionalOutputPorts.map(op => ValueKey(op, index) -> op.womType.none).toMap } else Map.empty - WorkflowExecutionDiff(executionStoreChanges = populate(b.value) + (this -> conditionalStatus), - valueStoreAdditions = valueStoreAdditions - ).validNel + populate(b.value) map { populated => + WorkflowExecutionDiff(executionStoreChanges = populated + (this -> conditionalStatus), + valueStoreAdditions = valueStoreAdditions) + } case Some(v: WomValue) => s"'if' condition ${node.conditionExpression.womExpression.sourceString} must evaluate to a boolean but instead got ${v.womType.stableName}".invalidNel case None => diff --git a/server/src/test/scala/cromwell/SimpleWorkflowActorSpec.scala b/server/src/test/scala/cromwell/SimpleWorkflowActorSpec.scala index 4e67ac984a1..266e56060d9 100644 --- a/server/src/test/scala/cromwell/SimpleWorkflowActorSpec.scala +++ b/server/src/test/scala/cromwell/SimpleWorkflowActorSpec.scala @@ -192,7 +192,7 @@ class SimpleWorkflowActorSpec extends CromwellTestKitWordSpec with BeforeAndAfte workflowActor ! StartWorkflowCommand } Await.result(promise.future, TestExecutionTimeout) - probe.expectTerminated(workflowActor, 2.seconds) + probe.expectTerminated(workflowActor, 15.seconds) supervisor.expectMsgPF(AwaitAlmostNothing, "parent should get a failed response") { case x: WorkflowFailedResponse => x.workflowId should be(workflowId) From 85eae7e1ebe3064a903cceac45800c88275c21fb Mon Sep 17 00:00:00 2001 From: Adam Nichols Date: Tue, 12 Mar 2024 22:08:00 -0400 Subject: [PATCH 9/9] Oops, that's for a different PR --- .../lifecycle/execution/keys/ConditionalKey.scala | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/engine/src/main/scala/cromwell/engine/workflow/lifecycle/execution/keys/ConditionalKey.scala b/engine/src/main/scala/cromwell/engine/workflow/lifecycle/execution/keys/ConditionalKey.scala index 8114b6cf4f5..7cc18c3b964 100644 --- a/engine/src/main/scala/cromwell/engine/workflow/lifecycle/execution/keys/ConditionalKey.scala +++ b/engine/src/main/scala/cromwell/engine/workflow/lifecycle/execution/keys/ConditionalKey.scala @@ -1,7 +1,6 @@ package cromwell.engine.workflow.lifecycle.execution.keys import cats.syntax.validated._ -import common.validation.ErrorOr import common.validation.ErrorOr.ErrorOr import cromwell.backend.BackendJobDescriptorKey import cromwell.core.ExecutionIndex.ExecutionIndex @@ -69,10 +68,9 @@ private[execution] case class ConditionalKey(node: ConditionalNode, index: Execu node.conditionalOutputPorts.map(op => ValueKey(op, index) -> op.womType.none).toMap } else Map.empty - populate(b.value) map { populated => - WorkflowExecutionDiff(executionStoreChanges = populated + (this -> conditionalStatus), - valueStoreAdditions = valueStoreAdditions) - } + WorkflowExecutionDiff(executionStoreChanges = populate(b.value) + (this -> conditionalStatus), + valueStoreAdditions = valueStoreAdditions + ).validNel case Some(v: WomValue) => s"'if' condition ${node.conditionExpression.womExpression.sourceString} must evaluate to a boolean but instead got ${v.womType.stableName}".invalidNel case None =>