Optionally auto-scale the number of threads used for alignment and tree building #222
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
16 tasks
If the allowed values for --method are updated without the case-by-case conditional being updated to match, an error will now be thrown instead of the command running with the wrong tree builder (FastTree). This type of defensive or comprehensive check is a common best practice.
It's leftover from when "refine" (née "treetime") and "tree" were the same command, before commit 6832839.
By default, FastTree with OpenMP support uses all available cores. This mismatches the other tree builders we use and the interface presented by the "tree" command (which allows a specific number of threads to be set).
|
For posterity, @tsibley and I talked about this yesterday with respect to how to make it work on OS X and Windows environments that don't have the same "affinity state" methods. We agreed on two complementary approaches: to default to |
…ee building Passing "--nthreads auto" to the "align" or "tree" commands will use all the available CPU cores. The default is reduced from 2 to 1, which is the only safe assumption. Auto-scaling is not the default because it's not necessarily reasonable to assume that all cores should be used without more information about the system's workload. For example, someone running two simple `augur tree` commands may not reasonably expect them to conflict with each other, but they would with auto-scaling enabled by default. Parallelism requires co-operation and thus an explicit opt-in.
tsibley
force-pushed
the
auto-scale-threads
branch
from
c029d8f
to
40040eb
Compare
|
Re: our discussion, I've just re-pushed this changing the default and implementing a better fallback cascade for CPU count. |
tsibley
changed the title
huddlej
approved these changes
Sep 27, 2018
|
Thanks for the review! |
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.
Uses all the available CPU cores by default instead of just 2. It is still possible to limit the number of threads to a certain number using the
--nthreadsoptions to "align" and "tree".Auto-scaling by default is particularly nice because it means the same command invocations in Snakefile can make the most of the system they're run on, whether that's a laptop or the cluster or a beefy AWS instance.
This PR builds off PR #217 (the
command-architecturebranch), so should wait to merge until that is merged.