[WDL Biscayne] 'after' keyword #4483
Conversation
| @@ -79,6 +79,14 @@ object GraphNodePort { | |||
| lazy val conditionalNode: ConditionalNode = g(()) | |||
| } | |||
|
|
|||
| final case class NodeCompletionPort(g: Unit => GraphNode) extends OutputPort with DelayedGraphNodePort { | |||
| override lazy val identifier: WomIdentifier = { | |||
| val name = "__after" // + graphNode.localName | |||
| @@ -44,7 +47,14 @@ object ExecutionStore { | |||
| val upstreamPort = conditionalCollector.outputNodeToCollect.singleUpstreamPort | |||
| upstreamPort.executionNode.isInStatus(chooseIndex(upstreamPort), statusTable) | |||
| // In the general case, the dependencies are held by the upstreamPorts | |||
| case _ => key.node.upstreamPorts forall { p => p.executionNode.isInStatus(chooseIndex(p), statusTable) } | |||
| case _ => | |||
| println(s"***** Checking completion of ${key.node.localName}'s upstreams: ${key.node.upstreamPorts.map(p => { | |||
There was a problem hiding this comment.
Remove or convert to logging call
| @@ -25,6 +25,9 @@ sealed abstract class CallNode extends GraphNode { | |||
| def callType: String | |||
|
|
|||
| def inputDefinitionMappings: InputDefinitionMappings | |||
| def mustFollow: Set[GraphNode] | |||
There was a problem hiding this comment.
Not super obvious whether this means
"the
Setof nodes that must follow this one"
or
"this node must follow the
Setof nodes"
I like the upstream/downstream naming you use elsewhere, any reason not to apply that here? Or prerequisites?
There was a problem hiding this comment.
I tried to avoid using a generic name like upstream because this is not a complete set, it's just the afters (eg to get a complete set of upstreams you add "input upstreams" and "non-input upstreams".
But I agree this is ambiguous - I'll change it while trying to preserve that distinction.
| /** | ||
| * A set of prerequisites for this call which are defined *in addition to* any input-based upstream requirements. | ||
| */ | ||
| def nonInputBasedPrerequisites: Set[GraphNode] |
| @@ -52,7 +52,8 @@ class ExpressionAsCallInputSpec extends FlatSpec with Matchers { | |||
| val callNodeWithInputs = callNodeBuilder.build( | |||
| WomIdentifier("foo"), | |||
| CommandTaskDefinitionSpec.oneInputTask, | |||
| inputDefinitionFold | |||
| inputDefinitionFold, | |||
| Set.empty | |||
There was a problem hiding this comment.
[ToL] There are a lot of Set.empty's in this PR, any reason not to make it the default value?
One possible counter-reason: that would make it rather easier to forget it somewhere where it's required.
There was a problem hiding this comment.
You're right, I left this like this to avoid forgetting to set it in the appropriate places during implementation.
FWIW (!SPOILER ALERT!) I think this is all going away in an upcoming PR so probably not worth worrying too much about in this one.
| Int i | ||
| } | ||
| command <<< | ||
| sleep 10 |
There was a problem hiding this comment.
[ToL] Is there a particular reason for all the sleep 10s? We're already fighting long test times...
There was a problem hiding this comment.
It's to make sure that if read and write are (incorrectly!) started concurrently, the read will get whatever was in the file before the write has a chance to overwrite it.
That said, I could probably shrink this down to something less offensively large (2 seconds?)
There was a problem hiding this comment.
(I'll only change the centaur ones though - this one is just in a validate test)
b57d2e9 to
2330ff9
Compare
| after foo2 | ||
| { input: | ||
| where = where | ||
| # , ready = foo2.done[0] |
| case _ => key.index | ||
| } | ||
|
|
||
| key match { | ||
| case scatteredCallCompletion: ScatteredCallCompletionKey => | ||
| println(s"${statusTable.row(scatteredCallCompletion.node).size} == ${scatteredCallCompletion.totalIndices}?") |
| def keyForNode(node: GraphNode): Option[JobKey] = { | ||
| statusStore.keys collectFirst { case k if k.node eq node => k } | ||
| def keysForNode(node: GraphNode): Iterable[JobKey] = { | ||
| statusStore.keys collect { case k if k.node eq node => k } |
There was a problem hiding this comment.
more than one is now possible because of afters, I'm just not 💯 why 🙂
| @@ -2,6 +2,7 @@ package wdl.transforms.biscayne.ast2wdlom | |||
|
|
|||
| import better.files.File | |||
| import org.scalatest.{FlatSpec, Matchers} | |||
| import wdl.model.draft3.elements.{ExpressionElement, _} | |||
2330ff9 to
8c30a3c
Compare
Redo of #4450 but Including interaction with
scatterandifblocks.