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

Amrplusplus #156

Merged
merged 32 commits into from
Sep 28, 2020
Merged

Amrplusplus #156

merged 32 commits into from
Sep 28, 2020

Conversation

AroArz
Copy link
Collaborator

@AroArz AroArz commented Aug 17, 2020

todo:

  1. slurm settings
  2. edit input in config.yaml
  3. figure out why error message is printed at the end of successful workflow (this is a function of the develop branch and not a result of amrplusplus integration)

@AroArz AroArz requested a review from boulund August 17, 2020 11:45
config.yaml Outdated Show resolved Hide resolved
config.yaml Outdated Show resolved Hide resolved
config.yaml Outdated Show resolved Hide resolved
config.yaml Outdated Show resolved Hide resolved
config.yaml Outdated
db: "scripts/amrplusplus/db/megares_modified_database_v2.00.fasta"
annotation: "scripts/amrplusplus/db/megares_modified_annotations_v2.00.csv"
rarefaction:
script: "scripts/amrplusplus/bin/rarefaction" # path to rarefaction script
Copy link
Member

Choose a reason for hiding this comment

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

We have not utilized this way of referencing script paths in any previous StaG module. I'm not entirely sure that this belongs in the config file, as usage of the script is tightly coupled to the call to the script (in the rule body), so we might as well just hardcode the script path in there... Thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure its easy to change. My main idea with this type of referencing was if we would ever change the structure of scripts folder we would only need to change the path in config.yaml and not touch the rulefile itself.

Copy link
Member

@boulund boulund Aug 17, 2020

Choose a reason for hiding this comment

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

I would like to only have stuff that end-users should be able to modify in the config file. You can still use the params section to define the path to the script for each rule just like you currently have, just that you hardcode it in place there instead of keeping it here in the config.

@boulund
Copy link
Member

boulund commented Aug 17, 2020

We need to add the license text from https://github.com/meglab-metagenomics/amrplusplus_v2 to the subdirectory where we include their scripts (see e.g. how it looks with KrakenTools)

@boulund
Copy link
Member

boulund commented Aug 17, 2020

We also need to write documentation (make a new section in https://github.com/ctmrbio/stag-mwc/blob/amrplusplus/docs/source/modules.rst). We should also make it very clear here that AMRPlusPlus only works with singularity. If a user tries to run this without singularity, the user would likely be presented with strange errors. If possible we should make sure these rules are skipped by Snakemake if the users tries to run without singuarlity (I don't think this is practically possible, however).

@boulund
Copy link
Member

boulund commented Aug 17, 2020

Additionally, we need to update the CircleCI tests so the syntax and dag can be automatically validated: https://github.com/ctmrbio/stag-mwc/blob/amrplusplus/.circleci/config.yml

@boulund
Copy link
Member

boulund commented Aug 17, 2020

The CHANGELOG also needs to be updated:

https://github.com/ctmrbio/stag-mwc/blob/amrplusplus/CHANGELOG.md#041-unreleased

AroArz and others added 4 commits August 17, 2020 14:40
Co-authored-by: Fredrik Boulund <boulund@users.noreply.github.com>
Co-authored-by: Fredrik Boulund <boulund@users.noreply.github.com>
Co-authored-by: Fredrik Boulund <boulund@users.noreply.github.com>
Co-authored-by: Fredrik Boulund <boulund@users.noreply.github.com>
config.yaml Outdated
Comment on lines 128 to 131
amrplusplus:
databases:
megares_fasta: "scripts/amrplusplus/db/megares_modified_database_v2.00.fasta"
megares_annotation: "scripts/amrplusplus/db/megares_modified_annotations_v2.00.csv"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How about

Suggested change
amrplusplus:
databases:
megares_fasta: "scripts/amrplusplus/db/megares_modified_database_v2.00.fasta"
megares_annotation: "scripts/amrplusplus/db/megares_modified_annotations_v2.00.csv"
amrplusplus:
megares:
fasta: "scripts/amrplusplus/db/megares_modified_database_v2.00.fasta"
annotation: "scripts/amrplusplus/db/megares_modified_annotations_v2.00.csv"

@AroArz AroArz marked this pull request as ready for review August 17, 2020 14:44
@AroArz AroArz marked this pull request as draft August 17, 2020 17:57
docs/source/modules.rst Outdated Show resolved Hide resolved
Co-authored-by: Fredrik Boulund <boulund@users.noreply.github.com>
docs/source/modules.rst Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@boulund boulund marked this pull request as ready for review September 28, 2020 08:34
@boulund
Copy link
Member

boulund commented Sep 28, 2020

This is starting to look good. I would just like to confirm with you @AroArz that you've successfully tried running this with --use-conda and --use-singularity separately?

@AroArz
Copy link
Collaborator Author

AroArz commented Sep 28, 2020

This is starting to look good. I would just like to confirm with you @AroArz that you've successfully tried running this with --use-conda and --use-singularity separately?

@AroArz AroArz closed this Sep 28, 2020
@AroArz AroArz reopened this Sep 28, 2020
@AroArz
Copy link
Collaborator Author

AroArz commented Sep 28, 2020

That was not the button I was looking for.. :).

I successfully ran this using --use-singularity and --use-conda on both nas and Uppmax.

@AroArz AroArz marked this pull request as draft September 28, 2020 08:39
@boulund boulund marked this pull request as ready for review September 28, 2020 08:49
@boulund boulund merged commit ae691c5 into develop Sep 28, 2020
boulund added a commit that referenced this pull request Feb 2, 2021
* Update version to 0.4.1-dev

* Update CircleCI badge in README to monitor develop branch

* Add Singularity deffile for stag-mwc env

* Add Singularity image for main env up to kraken2

* Add link to Singularity bind docs

* Add stag-mwc-main singularity image to all rules that require it

* Add Singularity images for biobakery and assembly

* Set rule threads from cluster_config if defined

* Update CHANGELOG

* Docs: Update module intro paragraph, closes #151

* Add {cluster.extra} to slurm-submit.py script call

* Update CHANGELOG

* Fix typo in kraken2 rules

* the host removal script was ignoring it's flag in config.yaml

* Update CHANGELOG

* Update CHANGELOG

* Tweak docs, update changelog, prepare for release

* Amrplusplus (#156)

* amrplusplus

* amrplusplus scripts

* added scripts

* changed input in config.yaml

Co-authored-by: Fredrik Boulund <boulund@users.noreply.github.com>

* default db empty

Co-authored-by: Fredrik Boulund <boulund@users.noreply.github.com>

* db -> megares

Co-authored-by: Fredrik Boulund <boulund@users.noreply.github.com>

* annotation --> megares_annotation

Co-authored-by: Fredrik Boulund <boulund@users.noreply.github.com>

* added amrplusplus to circleci

* updated CHANGELOG.md

* updated amrplusplus in config.yaml

* added amrplusplus to modules.rst

* updated amrplusplus.smk

* added license and removed bin dir

* updated modules.rst

* added align_to_amr

* fixed default database

* amrplusplus db empty in conf

* indentations

* clarification regarding database

* Update docs/source/modules.rst

Co-authored-by: Fredrik Boulund <boulund@users.noreply.github.com>

* Update rules/antibiotic_resistance/amrplusplus.smk

Co-authored-by: Fredrik Boulund <boulund@users.noreply.github.com>

* Update docs/source/modules.rst grammar

Co-authored-by: Fredrik Boulund <boulund@users.noreply.github.com>

* Update CHANGELOG.md

Co-authored-by: Fredrik Boulund <boulund@users.noreply.github.com>

* added bwa to stag-mwc.yaml

* added conda environment

* amrplusplus exectuable with conda

* amrplusplus executable with conda

* samtools version 1.10

* amrplusplus conda executable

* added amrplusplus.yaml to not break stag-mwc.yaml dependencies

* removed amrplusplus dependencies because they broke it

* now using amrplusplus.yaml for conda instead of stag-mwc.yaml

Co-authored-by: Fredrik Boulund <boulund@users.noreply.github.com>
Co-authored-by: Aron Arzoomand <aron@debian.ki.se>

* Change report generation call (#161)

* Change report generation call

Ignore additional arguments to snakemake when creating report,

Intended to avoid shell escaping issue with complex arguments to e.g. Singularity, issue #160

* removed snakemake_call

* added report generation workaround

* report generation

Co-authored-by: Fredrik Boulund <boulund@users.noreply.github.com>

* report generation

Co-authored-by: Fredrik Boulund <boulund@users.noreply.github.com>

* indentation

Co-authored-by: Aron Arzoomand <51719360+AroArz@users.noreply.github.com>

* Slurm profile for CTMR UCP environment (#159)

* First draft for CTMR UCP cluster profile

* Remove references to rackham profile, remove account check

* Updated resource requests for all rules

* Final tweaks

* Rename ctmr_ucp profile to ctmr_gandalf

* Fix yml-yaml typo

* Fix incorrectly named singularity images

* Remove --use prefix for argument to metawrap

* Add missing singularity container to create_kaiju_krona_plot

* added dbdir and threads for align_to_amr

* Disable area plot for Kaiju

* Minor updates to docs (#162)

* Update badge URL in README

Co-authored-by: Kristaps <kbebris@gmail.com>
Co-authored-by: Aron Arzoomand <51719360+AroArz@users.noreply.github.com>
Co-authored-by: Aron Arzoomand <aron@debian.ki.se>
Co-authored-by: AroArz <aron_arzoomand@live.se>
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.

2 participants