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

add workflows for isnvs_merge_to_vcf #864

Merged
merged 50 commits into from
Jul 25, 2018
Merged

add workflows for isnvs_merge_to_vcf #864

merged 50 commits into from
Jul 25, 2018

Conversation

tomkinsc
Copy link
Member

No description provided.

@dpark01
Copy link
Member

dpark01 commented Jul 23, 2018

Hm.. just realized looking at this: what’s the difference between the two tasks besides the boolean input? If there’s no difference perhaps they should be consolidated to one task? The Boolean input could even be optional if we want it to be.

Separately, should we rename all our tasks/xxx.wdl import files to tasks_xxx.wdl ?

@tomkinsc
Copy link
Member Author

Nice catch; the two had carried over from the Snakemake rules. I'll consolidate them.

We could rename the task files to avoid collisions. I can submit that as a separate PR.

Copy link
Member

@dpark01 dpark01 left a comment

Choose a reason for hiding this comment

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

Probably worth manually testing on dx just to be sure

@tomkinsc tomkinsc changed the title add workflows for isnvs_merge_to_vcf and isnvs_merge_to_vcf_filtered add workflows for isnvs_merge_to_vcf Jul 24, 2018
--parse_accession

$naive_filter \
--parse_accession && \
Copy link
Member

Choose a reason for hiding this comment

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

Actually, instead of stringing everything in the WDL command section together with &&, I've found it easier just to set -ex -o pipefail in any WDL task that's more than a one-liner.

dpark01
dpark01 previously approved these changes Jul 24, 2018
Copy link
Member

@dpark01 dpark01 left a comment

Choose a reason for hiding this comment

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

looks good, assuming everything's working

@tomkinsc
Copy link
Member Author

set -ex -o pipefail works within WDL bash, that's great.
I'm still kicking the tires on this one. I'm getting VCFs with headers but not rows, and still checking whether the cause is the data (v-phaser does emit some variants) or something with snpEff/our process.

@@ -3,7 +3,7 @@
# A wrapper script to run viral-ngs docker images
# The following paths have to be modified according to end-user environment
NOVOALIGN_PATH="/opt/novocraft" # Directory where novoalign.lic license file can befound
GATK_PATH="/opt/GenomeAnalysisTK-3.6" # Directory where the correct GATK jar file can be found
GATK_PATH="/opt/GenomeAnalysisTK-3.8" # Directory where the correct GATK jar file can be found
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

done, thanks

Copy link
Member

Choose a reason for hiding this comment

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

Has anyone used rundocker.sh recently? It doesn’t get used for anything more pipelined, but for ad hoc command line work, is it useful beyond what you get from the usual “docker run -it —rm”?

Copy link
Member Author

Choose a reason for hiding this comment

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

I haven't used it recently.

@tomkinsc tomkinsc dismissed dpark01’s stale review July 25, 2018 03:27

this also needs a fix related to sample name inference

@@ -59,7 +59,7 @@ def execute(self, command, args, JVMmemory=None, stdin=None, stdout=None): #
] + args

_log.debug(' '.join(tool_cmd))
return util.misc.run_and_print(tool_cmd, stdin=stdin, buffered=True, silent=("databases" in command), check=True)
return util.misc.run_and_print(tool_cmd, stdin=stdin, stderr=stderr, buffered=True, silent=("databases" in command), check=True)
Copy link
Member

Choose a reason for hiding this comment

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

Any reason we can just revert this to subprocess.check_call?

Copy link
Member Author

Choose a reason for hiding this comment

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

We probably should revert. Let's save it for a separate PR? This one is already larger than it should be.

@tomkinsc tomkinsc merged commit 03b8368 into master Jul 25, 2018
@tomkinsc tomkinsc deleted the ct-add-merge-to-vcf branch July 25, 2018 16:02
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.

3 participants