Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make snakemake a dev dependency #557

Merged
merged 3 commits into from
Aug 12, 2020
Merged

Conversation

huddlej
Copy link
Contributor

@huddlej huddlej commented May 26, 2020

Technically augur does not depend on snakemake to run and including it here can
cause confusion and/or issues installing either augur or snakemake. This commit
removes the snakemake dependency and leaves the installation of snakemake up to
the user.

@codecov
Copy link

codecov bot commented May 26, 2020

Codecov Report

Merging #557 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #557   +/-   ##
=======================================
  Coverage   28.31%   28.31%           
=======================================
  Files          39       39           
  Lines        5332     5332           
  Branches     1306     1306           
=======================================
  Hits         1510     1510           
  Misses       3769     3769           
  Partials       53       53           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b6f950...412ac7b. Read the comment docs.

@tsibley
Copy link
Member

tsibley commented May 26, 2020

100% agree we should remove this hard dep, but I think first we want to

  • Update https://github.com/nextstrain/conda to include Snakemake with the same version spec, so that the environment remains "batteries included"
  • Audit and address Augur's and nextstrain.org's docs for places which assume installing Augur means you have Snakemake.

@huddlej
Copy link
Contributor Author

huddlej commented May 27, 2020

That sounds great to me! I can post a PR for point one tomorrow evening.

The second point is going to take more time, but maybe @eharkins has a better sense of which docs would need to be changed since his recent deep dive into all Nextstrain docs.

@eharkins
Copy link
Contributor

Happy to include

Audit and address Augur's and nextstrain.org's docs for places which assume installing Augur means you have Snakemake.

in my general look at docs and try to come up with a comprehensive list of places where that assumption is made. The tutorials sort of assume this although the zika tutorial does explain that snakemake is necessary and how it is used with augur.

There is also some duplication of explaining how snakemake works with augur, e.g.:

Deduplicating that will involve some design decisions about how we want the tutorials to use snakemake (see this thread on slack).

Copy link

@prakashvishnoi prakashvishnoi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

snakemake dependency removed as desired.

@eharkins
Copy link
Contributor

Here are all of the places I could find in docs where either snakemake is referred to (U) or the use of snakemake is assumed as I interpret it (A):

Augur docs:

  1. (U) https://docs.nextstrain.org/projects/augur/en/stable/faq/augur_snakemake.html?highlight=snakemake and
  2. (A) https://nextstrain-augur.readthedocs.io/en/stable/tutorials/zika_tutorial.html#chaining-of-augur-commands-with-snakemake )

nextstrain.org docs:

While it is instructive to run all of the above commands manually, it is more practical to automate their execution with a single script. Nextstrain implements these automated pathogen builds with Snakemake by defining a Snakefile like the one in the Zika repository you downloaded.

nextstrain cli
Does nextstrain cli get snakemake from augur as a dependency (it seems to be needed for nextstrain cli)? If so, does it need to be added as an explicit dependency of nextstrain cli or is that already addressed by:

Update https://github.com/nextstrain/conda to include Snakemake with the same version spec, so that the environment remains "batteries included"

?

@tsibley
Copy link
Member

tsibley commented Jun 18, 2020

Re: the CLI and Snakemake: I don't think there will be any updates needed to the CLI if Snakemake is removed from Augur's deps, because the Docker image already includes it and we'd update the Conda environment to do so too.

@rneher
Copy link
Member

rneher commented Jun 19, 2020

most of our tests use Snakemake. we would have to add it as a dev dependency.

huddlej added a commit to nextstrain/conda that referenced this pull request Jun 26, 2020
Maintains presence of snakemake executable in the Nextstrain environment in
preparation for removing Snakemake as an augur dependency [1]. Two important
points to note about this change are:

  1. We install Snakemake from PyPI instead of conda to ensure we have the
  latest version and to minimize conda environment conflicts that have been
  issues for many users in the past.

  2. We do not pin Snakemake to a specific earlier version here as we did in
  augur (v5.10.0). This means the latest version of Snakemake will be installed
  by default with whatever breaking changes might occur in Snakemake's own
  API. This allows us to take advantage of the the latest features and bug fixes
  for Snakemake, but it also creates a breaking change for augur that will be
  reflected in a major version release of augur.

[1] nextstrain/augur#557
@huddlej
Copy link
Contributor Author

huddlej commented Jun 26, 2020

Update https://github.com/nextstrain/conda to include Snakemake with the same version spec, so that the environment remains "batteries included"

This is addressed in conda PR 8.

Thank you, @eharkins, for collecting all the relevant docs! I'll look through these and update any mentions of augur/Snakemake that need it.

@huddlej huddlej added this to the Major release 10.0.0 milestone Jul 28, 2020
huddlej added a commit that referenced this pull request Aug 7, 2020
Adds the required `--cores 1` argument to all calls to snakemake in both scripts
and documentation.

This is the first step toward removing Snakemake as an augur dependency [1].

[1] #557
huddlej added a commit to nextstrain/nextstrain.org that referenced this pull request Aug 7, 2020
Adds the required --cores 1 argument to all calls to `snakemake` and `nextstrain
build` in documentation. This is the first step toward removing Snakemake as an
augur dependency [1].

[1] nextstrain/augur#557
@huddlej
Copy link
Contributor Author

huddlej commented Aug 11, 2020

It's getting hard to keep track of all the moving pieces required for this PR to go in, so I've tried to make a comprehensive checklist below.

After reviewing the nextstrain.org docs, I don't think we need to make any additional changes. These docs expect the user to install augur/auspice through conda or Docker. For both of these approaches, Snakemake will already be installed even if it isn't installed as part of augur's installation. The augur PR 600 updates the pip-based instructions to mention that Snakemake must be installed separately.

Technically augur does not depend on Snakemake to run and including it here can
cause confusion and/or issues installing either augur or Snakemake. This commit
removes the snakemake dependency and leaves the installation of Snakemake up to
the user.

Note that functional tests continue to rely on Snakemake, so we need to install
it as a dev dependency. Once functional tests have migrated completely to cram,
we can remove this dependency. We use the latest minor version of the 5.*
Snakemake releases, now that our documentation has been updated to reflect the
required `--cores` argument.

This commit also removes the Snakemake dependency from the conda environment
file, since the version installed from bioconda is ancient (circa 3.*) and gets
replaced by the pip installed version from our dev environment anyway.
@huddlej huddlej force-pushed the update-snakemake-dependency branch from 2eff71c to ddb18c2 Compare August 12, 2020 20:52
I missed this self-referencing Snakemake call, but this commit adds the required
cores argument for later versions of Snakemake.
@huddlej huddlej merged commit 2397f96 into master Aug 12, 2020
@huddlej huddlej deleted the update-snakemake-dependency branch August 12, 2020 22:27
@victorlin victorlin changed the title Remove snakemake as a dependency Make snakemake a dev dependency Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants