Memory resource definitions #953
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously they would result in mem_mb=0 when the input size in GiB was less than 1, as the int() would truncate that to 0 which then was propagated to the final result by multiplication via the constant scaling factor. This only impacted smaller builds, such as our CI, but good to fix anyway. Use ceil() instead of int() to ensure that we always at least result in mem_mb=1 (although almost every process we run is going to have a larger memory footprint than that in reality, so there's still a bit of inaccuracy…).
While most of our mem_mb definitions are only heuristics for Snakemake's scheduler and the commands themselves aren't limited or aware of the mem_mb defined, IQ-TREE *does* support memory limits. Resolves <#949>.
|
Another approach to setting a memory limit for the rule (rather than basing it off input size and guessing how much IQ-TREE needs) would be slice up the total memory available to the workflow by the number of concurrent jobs. This would avoid having to keep tabs on the current memory usage in benchmarks and update the rule's resources to match over time. |
|
It would be good to merge this, but I'm wary to do so without first completing the review of memory usage correlation with input size (as described under the "Testing" section above). I will likely hold off on merging until that's done (maybe by me eventually, maybe by someone else interested if anyone). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of proposed changes
See commit messages.
Related issue(s)
Fixes #949
Testing
mem_mbas defined forrule treeis still accurate, or if we need to revise the definition to provide more memory. Passing--memto IQ-TREE may make tree building run slower if the current value is insufficient. Graph various input sizes from our builds vs. memory used from benchmark files for this rule?Release checklist
Changes should not be backwards incompatible.
If this pull request introduces new features, complete the following steps:
docs/src/reference/change_log.mdin this pull request to document these changes by the date they were added.