Drop backward compatibility

[huddlej]

Description of proposed changes

This is a proof-of-concept PR showing components of the ncov workflow that I think could be removed or simplified.

Most of these changes rely on the idea of making a major backward incompatible release that removes much of our backward-compatibility that has survived at the expense of a more complicated workflow.

Each commit represents a single simplification step that could be cherry-picked or rebased onto master, if we choose.

Testing

• Tested by CI
• Tested on SLURM cluster with full Nextstrain profile
• Tested with a trial build through GitHub Actions

Release checklist

If this pull request introduces backward incompatible changes, complete the following steps for a new release of the workflow:

• Determine the version number for the new release by incrementing the most recent release: v5
• Update docs/change_log.md in this pull request to document these changes and the new version number.
• After merging, create a new GitHub release with the new version number as the tag and release title.

[jameshadfield]

Thanks for this @huddlej! Enforcing the new style of inputs allows the snakefile to be much nicer. Apart from the comment on trimmed_origin, everything looks good as is.

Given that this will be a "breaking change", I would favour adding a few more changes to this major version bump. (Equally some could go in the next one).

• Remove usage of "exposure" - removes the need for a lot of "getter" functions, removes the rule incorporate_travel_history entirely, and generally makes auspice easier to understand. Edit: this might be quite a lot of work...
• Add nice error messages if one supplies an old-style sequence/metadata input.

[huddlej]

Thank you for checking this out, @jameshadfield! I'll work on cutting out the trim_origin bits.

Add nice error messages if one supplies an old-style sequence/metadata input.

This seems really important, since we're dropping support for that interface in this PR. We also need to provide nice errors when no inputs are defined.

Remove usage of "exposure"

If this will be a lot of work, we could handle it in a separate PR. Some of the proposals in the Google Doc for making things simpler won't be simple to implement themselves. :)

[huddlej]

Commit 6b32071 adds helpful error messages for users with the old input config or no input configs and also removes all references to the trimmed origin, underscores in origins, etc. from the Snakemake files and multiple inputs tutorial.

I think I caught every reference to the old config style and origin wildcards with underscores, but would you mind giving this another look, @jameshadfield, just to verify?

[jameshadfield]

This currently fails, as the compressed fasta is handed to nextalign which results in an empty results/aligned_example-data.fasta (no error raised). The pipeline eventually fails at a downstream filter step (which is slightly confusing).

We could add a check to _get_path_for_input which throws a nice error if the input looks like it's compressed.

[jameshadfield]

e.g.

diff --git a/workflow/snakemake_rules/common.smk b/workflow/snakemake_rules/common.smk
index d4a09568..105d16b2 100644
--- a/workflow/snakemake_rules/common.smk
+++ b/workflow/snakemake_rules/common.smk
@@ -53,6 +53,10 @@ def _get_path_for_input(stage, origin_wildcard):
     if path_or_url=="" and stage in ["metadata", "sequences"]:
         raise Exception(f"ERROR: config->input->{origin_wildcard}->{stage} is not defined.")
 
+    ## Currently only metadata can be compressed!
+    if stage!="metadata" and (path_or_url.endswith(".xz") or path_or_url.endswith(".gz")):
+        raise Exception(f"Compressed sequence input is not yet supported. Please check {path_or_url}")
+
     if stage=="metadata":
         return f"data/downloaded_{origin_wildcard}.tsv" if remote else path_or_url
     if stage=="sequences":

[jameshadfield]

(And we should update my_profiles/example/builds.yaml to use data/example_sequences.fasta. The docs already instruct you to decompress the sequences file before running.)

[huddlej]

Good catch! It turns out that the example profile has been broken on master for a while now (since the proximity script started ignoring the reference strains), so we'll need to fix that in a separate PR.

In the meantime, I switched the starting sequences for the example profile back to the uncompressed file. This compressed vs uncompressed issue should be mostly handled by #605. I was hoping to merge that PR first, so this PR won't need to add any checks for compression state.

[jameshadfield]

Good choice to move to nextalign for everything. If nextalign is missing from the environment we get the error

/usr/local/bin/bash: line 1: nextalign: command not found

which is probably good enough. I considered adding something simple to the Snakefile like the following, but it may be more trouble than it's worth.

# Ensure `nextalign` is available, as we no longer support `mafft`
import shutil
if shutil.which("nextalign") is None:
    logger.error("ERROR: `nextalign` does not seem to be installed, which is needed for this pipeline.")
    sys.exit(1)

[huddlej]

Yeah, I think we can't handle this much better than recommending users add the --use-conda to their Snakemake commands or use the CLI and Docker image.

[jameshadfield]

Nice one @huddlej! Apart from some minor fixes this looks good to go 👍

[rneher]

one issue with dropping backwards compatibility is that users using previous versions can no longer pull in the updated exclude files etc...

[huddlej]

one issue with dropping backwards compatibility is that users using previous versions can no longer pull in the updated exclude files etc...

That's a good point. We've been treating these workflows and the data they use as parts of a single package, but the workflow is software and the data are data. If we want to avoid this issue generally in the future, we should probably think about how to decouple the software from the data it consumes.

For example, we could continue to version the excludes and other curated files in this repo, but we could host the latest files for others to use on data.nextstrain.org. This approach would treat those data more like the files people already download from GISAID, but we could allow users to provide paths to data.nextstrain.org in their config files so they don't have to manually download anything. This solution could lead to default/parameters.yaml section like this (I don't know all of the files people would want, so I updated a couple of the obvious ones):

files:
  include: "https://data.nextstrain.org/ncov_include.txt"
  exclude: "https://data.nextstrain.org/ncov_exclude.txt"
  reference: "defaults/reference_seq.gb"
  alignment_reference: "defaults/reference_seq.fasta"
  annotation: "defaults/annotation.gff"
  outgroup: "defaults/outgroup.fasta" # is this arg still used? file doesn't exist in gh repo
  ordering: "defaults/color_ordering.tsv"
  color_schemes: "defaults/color_schemes.tsv"
  auspice_config: "defaults/auspice_config.json"
  lat_longs: "https://data.nextstrain.org/ncov_lat_longs.tsv"
  description: "defaults/description.md"
  clades: "defaults/clades.tsv"
  emerging_lineages: "defaults/emerging_lineages.tsv"

[huddlej]

I successfully ran through the Nextstrain profile on both AWS (through a trial build) and our SLURM cluster, so this is ready to merge. If the nextalign change ends up being too problematic for folks, it will be easy to revert that specific commit and try something else like keeping mafft but making nextalign the default.

[sidneymbell]

This is very 'neat', thanks for sharing @huddlej!

Looking at the changelog, I had a few questions (apologies if these are already documented elsewhere, happy to be redirected :)

Major changes

• Drop support for old sequence/metadata inputs

What do you mean by "old...inputs"? I.e., what is the requirement and how is it different?

• Use nextalign for alignment instead of mafft

Is this now the only alignment algo available?

Minor changes

• Drop unused haplotype status rule and script
• Remove unused nucleotide mutation frequencies rule
• Use augur distance for mutation counts

What does this last bullet mean?

cc @ttung

[huddlej]

Thank you for these clarifying questions, @sidneymbell; I was a little too terse in the change log! I've proposed some revisions to the change log that should answer these questions. I want to include a link to the new configuration parameters docs from the revised change log, but these new docs haven't been merged yet so I can't quite yet.

[sidneymbell]

Thank you!! Super helpful.

…

On Wed, May 12, 2021 at 12:03 PM John Huddleston ***@***.***> wrote: Thank you for these clarifying questions, @sidneymbell <https://github.com/sidneymbell>; I was a little too terse in the change log! I've proposed some revisions to the change log that should answer these questions <#634>. I want to include a link to the new configuration parameters docs <https://github.com/nextstrain/ncov/blob/add-config-docs/docs/configuration.md> from the revised change log, but these new docs haven't been merged yet so I can't quite yet. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#615 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADAIYX3VXPERF5IT4FMAUETTNLGITANCNFSM43LJNAFQ> .