Use resourceLimits and add test runs #53
Conversation
requires higher version of nextflow, but that is set
and normalize tokenization
| push: | ||
| branches: | ||
| - jashapiro/resourceLimits |
There was a problem hiding this comment.
Just a note that we should remove this before merging?
There was a problem hiding this comment.
Thanks for catching this & the lingering params.max_memory
Should be ready to go now.
| withLabel: mem_max { | ||
| // max memory for tasks that have failed more than twice for OOM | ||
| // set to 2.TB, but will be reduced by process.resourceLimits | ||
| memory = {(task.attempt > 2 && task.exitStatus in 137..140) ? 2.TB : 64.GB * task.attempt } |
There was a problem hiding this comment.
Just noting that I think it's fine to keep this set at 2 TB. Although if you are going to have to actually change the config files to change the resource limits anyways, why not just set this to whatever the resource limit is?
There was a problem hiding this comment.
Yeah, I figured it was easier to have only one place to change/update. There is a small risk we will forget that there is an implicit 2TB max, but if we are running into that, we probably have other problems as well!
nextflow.config
Outdated
| max_cpus = 8 | ||
| max_memory = 32.GB | ||
| } | ||
| params.max_memory = 16.GB |
There was a problem hiding this comment.
I don't think you are actually using this anymore?
This started with just changing to using the new
process.resourceLimitsto replace our functions for checking memory and cpus against the maximum allowances.One thing that was a bit non-obvious was how to handle the
mem_maxlabel: I ended up just picking a memory amount larger than any max value we are likely to use in the directive, but this is automatically scaled down to whatever is set in theprocess.resourceLimitssetting.The one disadvantage of this setup is that it is a bit harder to change limits on the fly; you can't just set a
paramand have that propagate in as we had before, but I don't expect that to be a common use case.After I added the resourceLimits changes, I thought this was a good chance to add some more testing, so I included a new GitHub action for stub runs of the workflow.