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

Proposal for 'after' keyword #162

Merged
merged 2 commits into from
Jan 8, 2019

Conversation

cjllanwarne
Copy link
Contributor

Tidies up the call section, and introduces a proposed new after keyword to provide non-output-based dependencies

@geoffjentry
Copy link
Member

I thought I was going to hate after but I quite like it.

Is there any harm in allowing after in cases where the outputs already provide a dependency?

@vdauwera
Copy link
Member

Ooh I like this idea. I can see this being useful if I want to have a task write to a dB then another read from the db, if there’s no actual file outputs to use as dependencies.

@geoffjentry
Copy link
Member

Yeah this is a fairly common complaint. Our stock answer is to wire in a fake output/input but that is horrible. Left to my own devices I’d probably have gone with the ‘dependsOn’ syntax from zamboni but ‘after’ reads so naturally with the call syntax

@cjllanwarne
Copy link
Contributor Author

@geoffjentry I don't see any harm in allowing both. The after will be redundant if there's already an output-based dependency but it won't break anything. Maybe it even makes things more readable in some cases 🤷‍♂️

@orodeh
Copy link
Contributor

orodeh commented Nov 22, 2017

Looks good to me.

@Horneth
Copy link
Contributor

Horneth commented Nov 25, 2017

Totally ToL and personal opinion: I find something like andThen easier to reason about since the order of execution matches the left to right order of reading / writing.


Calls can be run as soon as their inputs are available. If `call x`'s inputs are based on `call y`'s outputs, this means that `call x` can be run as soon as `call y` has completed.

To add a dependency from x to y that isn't based on outputs, you can use the `after` keyword, such as `call x after y after z`. But note that this is only required if `x` doesn't already depend on an output from `y`.

Choose a reason for hiding this comment

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

I think that if there are multiple dependencies the notation should be less redundant. Perhaps

call x after y and z

or

call x after y,z

@yfarjoun
Copy link

@Horneth your suggestion will make it harder to understand the flow of calls since to determine the dependence of call X one would need to examine all the call statements (to see if any of them have andThen X), while with the current proposal, you only need to examine the call X statement.

@geoffjentry
Copy link
Member

Agree with @yfarjoun. I think the after syntax fits better into the WDL mantra of simplicity & clarity

@orodeh
Copy link
Contributor

orodeh commented Dec 6, 2017

👍

2 similar comments
@vdauwera
Copy link
Member

vdauwera commented Dec 7, 2017

👍

@geoffjentry
Copy link
Member

👍

@geoffjentry
Copy link
Member

This has officially passed on a vote of 3-0 (really 4-0, but I forgot to vote).

As there are no implementations, this will hang out until such time as it actually exists

@abaumann
Copy link

one plug for after with dependencies is this: #183

although i dont think this definition requires after to always run, only guaranteed ordering despite dependencies

@geoffjentry
Copy link
Member

@abaumann At this point this PR is as-is, which is not to say that one couldn't open a subsequent PR

@geoffjentry
Copy link
Member

@cjllanwarne It'd be good to update this to reflect it being against development and not draft-3

@geoffjentry
Copy link
Member

Merging as there is an implementation (Cromwell) and there are no conflicts

@mlin mlin mentioned this pull request Nov 11, 2020
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants