Add WDL --badWorker/--restart test (#5247) - #5547
Conversation
annagiroti
changed the title
adamnovak
left a comment
adamnovak
left a comment
There was a problem hiding this comment.
I think the test could be faster and more or less guaranteed to both test the thing we want and to succeed, if we use different worker failure rates to set up the restart-able workflow and to finish it.
| "--badWorker=0.5", | ||
| "--badWorkerFailInterval=0.01", | ||
| ] | ||
|
|
||
| result_json = None | ||
| try: | ||
| # With --badWorker, the first attempt will usually fail due to a simulated worker death, but not always | ||
| result_json = subprocess.check_output(command + ["--logCritical"]) | ||
| except subprocess.CalledProcessError: | ||
| pass |
There was a problem hiding this comment.
Why not just use a workflow that sleeps for longer than the bad worker interval, and --badWorker=1.0, to always fail? If we don't get a failure here the test doesn't really test what we want it to test.
| # Keep restarting until the workflow actually finishes, | ||
| # or give up after a bounded number of attempts so a hang fails | ||
| # loudly instead of looping forever | ||
| max_attempts = 10 | ||
| for attempt in range(max_attempts): | ||
| try: | ||
| result_json = subprocess.check_output( | ||
| command + ["--restart", "--logCritical"] | ||
| ) | ||
| break | ||
| except subprocess.CalledProcessError: | ||
| if attempt == max_attempts - 1: | ||
| raise |
There was a problem hiding this comment.
Once we've prepared a workflow that we can restart, we should turn off --badWorker and finish the run in a way that we know should succeed, rather than throwing more attempts at it with the same failure settings.
| # Since we were catching | ||
| # <https://github.com/DataBiosphere/toil/issues/5247> at file | ||
| # export, make sure we actually exported a file, even after | ||
| # restarting from a --badWorker-induced failure |
There was a problem hiding this comment.
This could be simplified.
| # Since we were catching | |
| # <https://github.com/DataBiosphere/toil/issues/5247> at file | |
| # export, make sure we actually exported a file, even after | |
| # restarting from a --badWorker-induced failure | |
| # Make sure we actually exported a file, even after restarting |
…ngle attempt without --badWorker, add assert result_json is not None
adamnovak
left a comment
adamnovak
left a comment
There was a problem hiding this comment.
This looks pretty good to me.
|
There's something wrong with CI here. It is running |
|
I think whatever's going on with the missing tests is a preexisting problem. |
|
OK what's going on is those tests (and the new test) end up in |
2 participants
Changelog Entry
Added a test exercising
toil-wdl-runnerwith--badWorkerand--restart, covering the restart export crash where the--restartcodepath previously didn't construct theWDLContextneeded to export files. The underlying fix was already merged in #5248; this closes out the remaining test coverage for #5247.Resolves #5247
To be copied to the draft changelog by merger:
Reviewer Checklist
issues/XXXX-fix-the-thingin the Toil repo, or from an external repo.camelCasethat want to be insnake_case.docs/running/{cliOptions,cwl,wdl}.rstMerger Checklist