Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow nested inputs when corresponding meta setting is set to true #5562

Merged
merged 1 commit into from
Jul 9, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,20 @@ import cats.syntax.validated._
import common.validation.ErrorOr.{ErrorOr, _}
import wdl.model.draft3.elements.ExpressionElement.{ArrayLiteral, IdentifierLookup, SelectFirst}
import wdl.model.draft3.elements._
import wdl.model.draft3.graph.ExpressionValueConsumer.ops._
import wdl.model.draft3.graph.expression.{FileEvaluator, TypeEvaluator, ValueEvaluator}
import wdl.model.draft3.graph.{ExpressionValueConsumer, GeneratedCallFinishedHandle, GeneratedValueHandle, LinkedGraph}
import wdl.shared.transforms.wdlom2wom.WomGraphMakerTools
import wdl.transforms.base.linking.graph.LinkedGraphMaker
import wdl.transforms.base.wdlom2wom.graph.renaming.GraphIdentifierLookupRenamer.ops._
import wdl.transforms.base.wdlom2wom.graph.renaming._
import wdl.transforms.base.wdlom2wom.graph.{GraphNodeMakerInputs, WorkflowGraphElementToGraphNode}
import wom.callable.MetaValueElement.MetaValueElementBoolean
import wom.callable.{Callable, WorkflowDefinition}
import wom.graph.expression.AnonymousExpressionNode
import wom.graph.GraphNodePort.OutputPort
import wom.graph.expression.AnonymousExpressionNode
import wom.graph.{CallNode, GraphNode, WomIdentifier, Graph => WomGraph}
import wom.types.WomType
import wdl.model.draft3.graph.ExpressionValueConsumer.ops._
import wdl.model.draft3.graph.expression.{FileEvaluator, TypeEvaluator, ValueEvaluator}
import wdl.transforms.base.wdlom2wom.graph.renaming.GraphIdentifierLookupRenamer.ops._
import wdl.transforms.base.wdlom2wom.graph.renaming._

object WorkflowDefinitionElementToWomWorkflowDefinition extends Util {

Expand All @@ -33,6 +34,14 @@ object WorkflowDefinitionElementToWomWorkflowDefinition extends Util {

val a: WorkflowDefinitionConvertInputs = eliminateInputDependencies(b)

val (meta, parameterMeta) = processMetaSections(a.definitionElement.metaSection, a.definitionElement.parameterMetaSection)

val allowNestedInputs: Boolean = {
meta.get("allowNestedInputs").flatMap {
case x: MetaValueElementBoolean => Option(x.value)
case _ => None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TOL (Talking Out Loud, not gating approval of this PR): Is it the case that specifying

   meta {allowNestedInputs: "I like turtles"}

would silently set allowNestedInputs to false? Should there be a warning or error if the wrong type (something other than MetaValueElementBoolean) was specified?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

meta {allowNestedInputs: "I like turtles"} Will indeed set allowNestedInputs to false. I see no problem here, as it is clearly not the intention of the WDL writer to enable a feature.
Booleans are always true or false. I do not think extra code is needed to evaluate cases such as "true", "false", 0, 1 etc. If people start complaining at this behavior we can easily change it later. For now, I see no reason for the extra effort and code complexity (also I am lazy 😉).

}}.getOrElse(false)

// Make the set of workflow graph elements, including:
// - Top-level graph elements
// - Declarations in the inputs section
Expand All @@ -45,6 +54,7 @@ object WorkflowDefinitionElementToWomWorkflowDefinition extends Util {
val innerGraph: ErrorOr[WomGraph] = convertGraphElements(GraphLikeConvertInputs(graphNodeElements, Set.empty, Map.empty, a.typeAliases, a.definitionElement.name,
insideAScatter = false,
convertNestedScatterToSubworkflow = b.convertNestedScatterToSubworkflow,
allowNestedInputs = allowNestedInputs,
a.callables))
// NB: isEmpty means "not isDefined". We specifically do NOT add defaults if the output section is defined but empty.
val withDefaultOutputs: ErrorOr[WomGraph] = if (a.definitionElement.outputsSection.isEmpty) {
Expand All @@ -53,8 +63,6 @@ object WorkflowDefinitionElementToWomWorkflowDefinition extends Util {
innerGraph
}

val (meta, parameterMeta) = processMetaSections(a.definitionElement.metaSection, a.definitionElement.parameterMetaSection)

(withDefaultOutputs map {
ig => WorkflowDefinition(a.definitionElement.name, ig, meta, parameterMeta, b.definitionElement.sourceLocation)
}).contextualizeErrors(s"process workflow definition '${a.definitionElement.name}'")
Expand All @@ -67,6 +75,7 @@ object WorkflowDefinitionElementToWomWorkflowDefinition extends Util {
workflowName: String,
insideAScatter: Boolean,
convertNestedScatterToSubworkflow: Boolean,
allowNestedInputs: Boolean,
callables: Map[String, Callable])

def convertGraphElements(a: GraphLikeConvertInputs)
Expand All @@ -84,7 +93,7 @@ object WorkflowDefinitionElementToWomWorkflowDefinition extends Util {

for {
linkedGraph <- LinkedGraphMaker.make(nodes = a.graphElements, seedGeneratedValueHandles ++ finished, typeAliases = a.typeAliases, callables = a.callables)
womGraph <- makeWomGraph(linkedGraph, a.seedNodes, a.externalUpstreamCalls, a.workflowName, a.insideAScatter, a.convertNestedScatterToSubworkflow, a.callables)
womGraph <- makeWomGraph(linkedGraph, a.seedNodes, a.externalUpstreamCalls, a.workflowName, a.insideAScatter, a.convertNestedScatterToSubworkflow, a.allowNestedInputs, a.callables)
} yield womGraph
}

Expand All @@ -94,6 +103,7 @@ object WorkflowDefinitionElementToWomWorkflowDefinition extends Util {
workflowName: String,
insideAScatter: Boolean,
convertNestedScatterToSubworkflow : Boolean,
allowNestedInputs: Boolean,
callables: Map[String, Callable])
(implicit expressionValueConsumer: ExpressionValueConsumer[ExpressionElement],
fileEvaluator: FileEvaluator[ExpressionElement],
Expand All @@ -119,7 +129,7 @@ object WorkflowDefinitionElementToWomWorkflowDefinition extends Util {

val generatedGraphNodesValidation: ErrorOr[Set[GraphNode]] =
WorkflowGraphElementToGraphNode.convert(
GraphNodeMakerInputs(next, upstreamCallNodes, linkedGraph.consumedValueLookup, availableValues, linkedGraph.typeAliases, workflowName, insideAScatter, convertNestedScatterToSubworkflow, callables))
GraphNodeMakerInputs(next, upstreamCallNodes, linkedGraph.consumedValueLookup, availableValues, linkedGraph.typeAliases, workflowName, insideAScatter, convertNestedScatterToSubworkflow, allowNestedInputs, callables))
generatedGraphNodesValidation map { nextGraphNodes: Set[GraphNode] => currentList ++ nextGraphNodes }
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ object CallElementToGraphNode {

def supplyableInput(definition: Callable.InputDefinition): Boolean = {
!definition.isInstanceOf[FixedInputDefinitionWithDefault] &&
!definition.name.contains(".") // NB: Remove this check when sub-workflows allow pass-through task inputs
(!definition.name.contains(".") || a.allowNestedInputs)
}

def validInput(name: String, definition: Callable.InputDefinition): Boolean = {
Expand Down Expand Up @@ -224,4 +224,5 @@ case class CallNodeMakerInputs(node: CallElement,
availableTypeAliases: Map[String, WomType],
workflowName: String,
insideAnotherScatter: Boolean,
allowNestedInputs: Boolean,
callables: Map[String, Callable])
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ object IfElementToGraphNode {
val graphLikeConvertInputs = GraphLikeConvertInputs(graphElements.toSet, ogins, foundOuterGenerators.completionPorts, a.availableTypeAliases, a.workflowName,
insideAScatter = a.insideAnotherScatter,
convertNestedScatterToSubworkflow = a.convertNestedScatterToSubworkflow,
allowNestedInputs = a.allowNestedInputs,
a.callables)
val innerGraph: ErrorOr[Graph] = WorkflowDefinitionElementToWomWorkflowDefinition.convertGraphElements(graphLikeConvertInputs)

Expand All @@ -96,4 +97,5 @@ final case class ConditionalNodeMakerInputs(node: IfElement,
workflowName: String,
insideAnotherScatter: Boolean,
convertNestedScatterToSubworkflow : Boolean,
allowNestedInputs: Boolean,
callables: Map[String, Callable])
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ object ScatterElementToGraphNode {
val graphLikeConvertInputs = GraphLikeConvertInputs(graphElements.toSet, ogins ++ Set(womInnerGraphScatterVariableInput), foundOuterGenerators.completionPorts, a.availableTypeAliases, a.workflowName,
insideAScatter = true,
convertNestedScatterToSubworkflow = a.convertNestedScatterToSubworkflow,
allowNestedInputs = a.allowNestedInputs,
a.callables)
val innerGraph: ErrorOr[Graph] = WorkflowDefinitionElementToWomWorkflowDefinition.convertGraphElements(graphLikeConvertInputs)

Expand Down Expand Up @@ -131,6 +132,7 @@ object ScatterElementToGraphNode {
val graphLikeConvertInputs = GraphLikeConvertInputs(Set(a.node), subWorkflowInputs, Map.empty, a.availableTypeAliases, a.workflowName,
insideAScatter = false,
convertNestedScatterToSubworkflow = a.convertNestedScatterToSubworkflow,
allowNestedInputs = a.allowNestedInputs,
a.callables)
val subWorkflowGraph = WorkflowDefinitionElementToWomWorkflowDefinition.convertGraphElements(graphLikeConvertInputs)
subWorkflowGraph map { WomGraphMakerTools.addDefaultOutputs(_) }
Expand Down Expand Up @@ -188,4 +190,5 @@ final case class ScatterNodeMakerInputs(node: ScatterElement,
workflowName: String,
insideAnotherScatter: Boolean,
convertNestedScatterToSubworkflow: Boolean,
allowNestedInputs: Boolean,
callables: Map[String, Callable])
Original file line number Diff line number Diff line change
Expand Up @@ -50,15 +50,15 @@ object WorkflowGraphElementToGraphNode {
result.contextualizeErrors(s"process declaration '${typeElement.toWdlV1} $name = ${expr.toWdlV1}'")

case se: ScatterElement =>
val scatterMakerInputs = ScatterNodeMakerInputs(se, a.upstreamCalls, a.linkableValues, a.linkablePorts, a.availableTypeAliases, a.workflowName, a.insideAScatter, a.convertNestedScatterToSubworkflow, a.callables)
val scatterMakerInputs = ScatterNodeMakerInputs(se, a.upstreamCalls, a.linkableValues, a.linkablePorts, a.availableTypeAliases, a.workflowName, a.insideAScatter, a.convertNestedScatterToSubworkflow, a.allowNestedInputs, a.callables)
ScatterElementToGraphNode.convert(scatterMakerInputs)

case ie: IfElement =>
val ifMakerInputs = ConditionalNodeMakerInputs(ie, a.upstreamCalls, a.linkableValues, a.linkablePorts, a.availableTypeAliases, a.workflowName, a.insideAScatter, a.convertNestedScatterToSubworkflow, a.callables)
val ifMakerInputs = ConditionalNodeMakerInputs(ie, a.upstreamCalls, a.linkableValues, a.linkablePorts, a.availableTypeAliases, a.workflowName, a.insideAScatter, a.convertNestedScatterToSubworkflow, a.allowNestedInputs, a.callables)
IfElementToGraphNode.convert(ifMakerInputs)

case ce: CallElement =>
val callNodeMakerInputs = CallNodeMakerInputs(ce, a.upstreamCalls, a.linkableValues, a.linkablePorts, a.availableTypeAliases, a.workflowName, a.insideAScatter, a.callables)
val callNodeMakerInputs = CallNodeMakerInputs(ce, a.upstreamCalls, a.linkableValues, a.linkablePorts, a.availableTypeAliases, a.workflowName, a.insideAScatter, a.allowNestedInputs, a.callables)
CallElementToGraphNode.convert(callNodeMakerInputs)
}

Expand All @@ -81,4 +81,5 @@ final case class GraphNodeMakerInputs(node: WorkflowGraphElement,
workflowName: String,
insideAScatter: Boolean,
convertNestedScatterToSubworkflow : Boolean,
allowNestedInputs: Boolean,
callables: Map[String, Callable])
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
version 1.0

workflow sub_wf {
input {
Int y
}
# Calls foo but doesn't provide an 'x'. That's fine because the input was optional, but now the outer WF cannot override it.
call foo { input: y = y }

# No outputs, but we don't want that to be the error:
output { }
meta {allowNestedInputs: true}
}

task foo {
input {
Int? x
Int y
}
command {
}
output {
Int z = y
}
runtime {
docker: "ubuntu:latest"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"supplied_optional_subwf_sub_inputs.y": 5,
"supplied_optional_subwf_sub_inputs.sub_wf.foo.x": 5
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
version 1.0

import "import_me.wdl"

workflow supplied_optional_subwf_sub_inputs {
input {
Int y
}
# Shouldn't (strictly) be able to call this sub-workflow because the inputs are not passed through
call import_me.sub_wf {input: y=y}
meta {
allowNestedInputs: true
}
}