Skip to content

README: Add --docker and --cpus 1 to the example nextstrain build invocation#10

Merged
rneher merged 2 commits intomasterfrom
trs/nextstrain-build-invocation
May 25, 2022
Merged

README: Add --docker and --cpus 1 to the example nextstrain build invocation#10
rneher merged 2 commits intomasterfrom
trs/nextstrain-build-invocation

Conversation

@tsibley
Copy link
Copy Markdown
Contributor

@tsibley tsibley commented May 24, 2022

The --image argument is ignored unless a containerized runtime is used
(Docker or AWS Batch currently). Under the "native"/ambient runtime,
which may be the default for some installations, a warning will be
emitted but ultimately --image will be ignored. Specifying --docker
explicitly means a more obvious (although still not good) error occurs
earlier. These two arguments are temporary until Nextalign v2 has a
stable release.

Relatedly, specifying --cpus 1 now heads off issues with
"native"/ambient environments with newer Snakemake's until Nextstrain
CLI accounts for that automatically. While this invocation doesn't use
the "native"/ambient runtime now, including --cpus now means that when
--image and --docker are no longer necessary we can simply remove them.
Before that point, it also provides a guidance point for people using
variants of this example invocation.

Ultimately, given further work within our ecosystem (Nextalign,
Nextstrain CLI), this whole invocation should revert back to just
nextstrain build ..

Related issue(s)

Related to #8

Testing

Ran new example invocation under environments where the

  • Docker runtime is default
  • Native ("ambient') runtime is default, but Docker runtime is supported
  • Docker runtime is not supported

…nvocation

The --image argument is ignored unless a containerized runtime is used
(Docker or AWS Batch currently).  Under the "native"/ambient runtime,
which may be the default for some installations, a warning will be
emitted but ultimately --image will be ignored.  Specifying --docker
explicitly means a more obvious (although still not good) error occurs
earlier.  These two arguments are temporary until Nextalign v2 has a
stable release.

Relatedly, specifying --cpus 1 now heads off issues with
"native"/ambient environments with newer Snakemake's until Nextstrain
CLI accounts for that automatically.  While this invocation doesn't use
the "native"/ambient runtime now, including --cpus now means that when
--image and --docker are no longer necessary we can simply remove them.
Before that point, it also provides a guidance point for people using
variants of this example invocation.

Ultimately, given further work within our ecosystem (Nextalign,
Nextstrain CLI), this whole invocation should revert back to just
`nextstrain build .`.
@tsibley tsibley requested a review from corneliusroemer May 24, 2022 22:25
Use all the CPUs available for Nextalign, under the assumptions that a)
this workflow currently produces a single build at a time and b)
Nextalign doesn't have significant upper limits on useful parallelism
(number of input sequences not withstanding, which is awkward to
calculate here).  These assumptions might be incorrect?  They certainly
might _become_ incorrect later, so we should remember to reconsider them
in the future.

Use up to 8 CPUs for `augur tree`, as I dimly recall diminishing returns
from IQ-TREE above a number like 8.  This might also be wrong, though!
@tsibley
Copy link
Copy Markdown
Contributor Author

tsibley commented May 25, 2022

New commit (1088ed1) on top:

Enable parallelism for Nextalign and tree building

Use all the CPUs available for Nextalign, under the assumptions that a) this workflow currently produces a single build at a time and b) Nextalign doesn't have significant upper limits on useful parallelism (number of input sequences not withstanding, which is awkward to calculate here). These assumptions might be incorrect? They certainly might become incorrect later, so we should remember to reconsider them in the future.

Use up to 8 CPUs for augur tree, as I dimly recall diminishing returns from IQ-TREE above a number like 8. This might also be wrong, though!

Thoughts?

@tsibley tsibley requested a review from a team May 25, 2022 17:59
@rneher rneher merged commit d6d1936 into master May 25, 2022
@rneher rneher deleted the trs/nextstrain-build-invocation branch May 25, 2022 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects

Development

Successfully merging this pull request may close these issues.

2 participants