feat: deploy builds eagerly, not wait until all done#1025
feat: deploy builds eagerly, not wait until all done#1025corneliusroemer merged 1 commit intomasterfrom
Conversation
victorlin
left a comment
There was a problem hiding this comment.
I don't know enough about Snakemake to give a hard approval, but looks like you've tested successfully so consider this a soft approval. I just have a few non-blocking comments.
| rule deploy: | ||
| input: | ||
| *rules.all_regions.input | ||
| expand("auspice/{build_name}.deployed", build_name=BUILD_NAMES), |
There was a problem hiding this comment.
How does this work? There is a separate deploy for every item in BUILD_NAMES?
There was a problem hiding this comment.
The rule deploy is the one that requests deployment of all builds.
Previously it asked for all files, and then deployed them itself.
Now it asks for a touchfile auspice/{build_name}.deployed instead, which causes the rule deploy_single to deploy one build at a time.
The speed up happens because previously deploy only happened once all input files = all builds were ready.
Now the actual deployments can happen as soon as all build files for a single build are done.
tsibley
left a comment
There was a problem hiding this comment.
Code looks good by inspection to me.
The branch history is one primary commit and then four fixup commits + a changelog update commit. Would be nice to rebase those fixups away into the first commit before merging.
|
I'll rebase - never done this before so bear with me 🙃 |
|
Looks like there's a small changelog conflict too, so you'll have to update with latest master to resolve the conflict. Something like this should do it: # Update your local master branch
git fetch origin master:master
# Switch to the working branch if you're not there already
git switch eager-deploy
# This will open a rebase prompt, and you can mark fixup commits accordingly.
git rebase -i master
# Resolve merge conflicts (note the date format consistency fix in changelog)
git push --force-with-lease |
Currently we only deploy builds once all builds are done. Especially when testing, this causes unnecessarily large delays: some builds can be done as soon as 1.5hr after a build is started, but others may take 12hr. This PR adds an intermediate rule that deploys a single build as soon as all files are ready. This was discussed briefly on Slack and @trvrb gave green light for this change: https://bedfordlab.slack.com/archives/CSKMU6YUC/p1666885515151009 Changes are minimal, I tested this using dry run, will make a test run, this should work well and help test things faster in the future. To make it easier to check builds that have finished, I've added Slack messages including the URL for each successful deployment. We already output a lot of files anyways and having clickable URLs is quite useful. The rule deploy is the one that requests deployment of all builds. Previously it asked for all files, and then deployed them itself. Now it asks for a touchfile auspice/{build_name}.deployed instead, which causes the rule deploy_single to deploy one build at a time. The speed up happens because previously deploy only happened once all input files = all builds were ready. Now the actual deployments can happen as soon as all build files for a single build are done.
f43d2be to
b915867
Compare
|
Thanks @victorlin - I used fork in the end, hope it's good now. Looks good to me at least :) |
Description of proposed changes
Currently we only deploy builds once all builds are done.
Especially when testing, this causes unnecessarily large delays: some builds can be done as soon as 1.5hr after a build is started, but others may take 12hr.
This PR adds an intermediate rule that deploys a single build as soon as all files are ready. This was discussed briefly on Slack and @trvrb gave green light for this change: https://bedfordlab.slack.com/archives/CSKMU6YUC/p1666885515151009
Changes are minimal, I tested this using dry run, will make a test run, this should work well and help test things faster in the future.
To make it easier to check builds that have finished, I've added Slack messages including the URL for each successful deployment. We already output a lot of files anyways and having clickable URLs is quite useful.
Testing
Test builds:
https://nextstrain.org/staging/ncov/gisaid/trial/eager/global/all-time
https://nextstrain.org/staging/ncov/open/trial/eager/global/all-time
The first build was already deployed after less than 90min (reference, open)