Add WDL for iSNVs/v-phaser #828

tomkinsc · 2018-06-27T23:24:19Z

This PR adds WDLs for running v-phaser on one sample to output iSNVs. It probably makes sense to wait to do multi-sample via scatter-gather until we update everything to WDL 1.0 which has a Pair datatype for gathering multiple files (we're using WDL draft-2 currently).
Also, this should probably be merged via rebase due to all of the dx debugging...

…aser output merge_to_vcf did not previously output correct alternate genotype number for some cases of multi-chr v-phaser output (line intrahost.py:820 of this commit). This corrects that and changes test cases to use the chr "-#" encoding to denote chromosome/segment number. This also changes merge_to_vcf so sample names are guessed from first column of v-phaser output if sample names are not passed in via --samples.

add dummy workflows to support newer cromwell that requires WDL files to have a workflow block; add single-sample isnv workflows

restore dummy workflows to WDL task files; this allows womtool to validate them

…romwell and dxWDL

Note: this is WDL draft-2. Changing to WDL 1.0 will require various changes, starting with a "version 1.0" statement as the first line in each WDL. Also, input {} blocks. See: https://github.com/openwdl/wdl/blob/master/versions/1.0/SPEC.md

add dx default file inputs for new isnv workflows (mostly taxon_filter DBs)

…lowing for the inverted case)

dpark01 · 2018-06-28T15:53:12Z

Okay this is a big one... I think I'll need some deconstructed explanation on each piece. So far, I see a few things in this PR:

A ton of diffs that have to do with updating to the latest dxWDL + Cromwell and all the various changes that imposed.
A new WDL workflow for single sample iSNVs.
Some decent sized changes to intrahost.merge_to_vcf

A few questions and thoughts:

How interconnected are those three changes? Are they somewhat orthogonal/independent or did 2 necessitate both 1 and 3?
I've been watching some of the dxWDL updates from a distance and haven't yet given it a spin myself, but I was concerned about how they dropped support for implicit workflow inputs (percolated up from unlinked task inputs). But it looked like the various new releases of dxWDL have gone back and forth on supporting unlocked workflows.. not sure how related that was. I dunno, if the Cromwell/OpenWDL community more broadly has generally moved away from implicit workflow inputs then I guess we can follow, but I'm not terribly happy with that direction, but if this was only dxWDL we can inquire about it.
What motivated change 3? That particular method is a pretty hairy beast to dive into.. lint has complained about how overly complex it is but I've struggled to find ways to simplify and break it down because it really is managing a ton of edge cases at once (and I'm not absolutely sure that all those cases are well tested).

Anyway, breaking it down a bit will help navigate the actual diffs.

tomkinsc · 2018-07-02T15:54:20Z

The three changes are fairly orthogonal, however w/r/t changes 1&2, in the course of writing the new WDL workflows I wanted to target the newer dxWDL + Cromwell (at least under the draft-2 spec). They're interdependent as included here.
The trend seems to be toward requiring inputs to be explicitly specified. With the combination of versions of dxWDL and Cromwell used in this PR with the draft-2 format WDL files, explicit declaration appears to extend only to required inputs. With the move to WDL 1.0 we may need to also explicitly list the optional inputs from tasks in workflows (not sure yet). It will become apparent when we start updating from WDL draft-2 to 1.0 format. I haven't come across a migration guide, and it is a bit difficult to tell when comparing the specs. In general, we will need to make fairly significant changes to support WDL 1.0...
Change 3 is two things:
- a block that infers sample names from the v-phaser output files in the case where a list of sample names is not passed in. This is to reduce the number of inputs required by intrahost.merge_to_vcf(), with the WDL/DNAnexus implementation in mind.
- a bugfix to encode+report ALT alleles in the VCF GT fields (not sure if anyone was bitten by the bug?). This should have been caught by our tests, except the sample names of the tests were not changed to the sampleName-chr# format when we changed the rest of the code to use the-chr# suffix format for dealing with multiple chromosomes.

dpark01

Okay, went through the diffs in more detail, let me know what you think

dpark01 · 2018-07-03T18:09:17Z

intrahost.py

- """samples, isnvs, and alignments must have the same number of elements
- (plus an extra reference record in the alignment).
- %s does not have the right number of sequences""" % fileName)
+ # for fileName in alignments:


This commented out section of intrahost.merge_to_vcf appears to have been simply a pre-screening sanity check / safety check to make sure the inputs were self consistent. Is this check no longer necessary or is it just that it doesn't function for guessed sample names?

I'll adapt it for the both cases, where sample names are provided, and when they are not.

dpark01 · 2018-07-03T18:43:28Z

intrahost.py

@@ -580,7 +595,10 @@ def merge_to_vcf(
 alignmentFile)

 for s in samplesToUse:
- for row in util.file.read_tabfile(samp_to_isnv[sampleIDMatch(s)]):
+ isnv_filepath = samp_to_isnv[sampleIDMatch(s)]
+ if os.path.getsize(isnv_filepath) == 0:


Why the explicit check for os.path.getsize() == 0? Are there special scenarios where we pass in empty files that semantically mean something we should skip, or is this just a normal situation where vphaser detects no isnvs for that particular file (and thus produces empty output)? util.file.read_tabfile should simply produce an empty iterator, so this check seems unnecessary?

You're right, this should not be necessary.

dpark01 · 2018-07-03T18:45:06Z

intrahost.py

+ isnvs_to_use.append(isnvs_file)
+ continue
+ for row in util.file.read_tabfile(isnvs_file):
+ guessed_sample_ID = sampleIDMatch(row[0])


If I read this correctly, this code is unreachable, because anytime the isnvs_file is non-empty, the above if statement will continue and skip this for loop, but any time it's empty, the for loop won't do anything.

Thanks, that was sloppy of me. I'll fix this and add a unit test where sample names are guessed so this has some test coverage.

dpark01 · 2018-07-03T18:45:37Z

intrahost.py

@@ -800,7 +817,7 @@ def merge_to_vcf(
 continue
 alleleMap = dict((a, i) for i, a in enumerate(alleles))
 # GT col emitted below
- genos = [str(alleleMap.get(consAlleles.get(s), '.')) for s in samples]
+ genos = [str(alleleMap.get(consAlleles.get(s), '.')) for s in samplesToUse]


oops, so this was a bug before?

dpark01 · 2018-07-03T18:46:31Z

test/unit/test_intrahost.py

@@ -152,7 +152,7 @@ def test_single_strand_bias_hard_filter(self):
 self.assertEqual(output[0][7:], expected[7:])


-@unittest.skipIf(tools.is_osx(), "vphaser2 osx binary from bioconda has issues")
+#@unittest.skipIf(tools.is_osx(), "vphaser2 osx binary from bioconda has issues")


did the osx version get fixed?

It seems to be? The test passes on OSX and I can get help text when I call it in isolation. Do you recall what the issue was?

dpark01 · 2018-07-03T18:48:13Z

test/unit/test_intrahost.py

@@ -224,8 +224,10 @@ def add_indel(self, sample, chrom, pos, acounts, libinfo=None):

 def dump_isnv_tmp_file(self, sample):
 fn = util.file.mkstempfname('.txt')
+ print("self.isnvs[sample]",sample,list(map(str,self.isnvs[sample])))


are we sure we want to print all this or was this leftover from a debugging stage?

leftover, whoops!

add unit test for case where sample names are guessed for merge_to_vcf; rework the sectio where they are guessed

dpark01

Looks good, let's make it so!

dpark01 · 2018-07-05T12:52:39Z

travis/install-wdl.sh

-cached_fetch_jar_from_github dnanexus dxWDL dxWDL 0.60.2
+cached_fetch_jar_from_github broadinstitute cromwell womtool 32
+cached_fetch_jar_from_github broadinstitute cromwell cromwell 32
+cached_fetch_jar_from_github dnanexus dxWDL dxWDL 0.66.2


If it doesn't cause much trouble, since you're updating from dxWDL 0.60.2 -> 0.66.2 anyway, want to just try going all the way to 0.68.1 (since some time has elapsed in the course of the development of this branch)?

Let's see what happens. I'll try updating Cromwell too.

…l-ngs into ct-vphaser-wdl

dpark01 · 2018-07-05T15:12:17Z

Actually it might be weird if you update Cromwell because then dxWDL and Cromwell will be out of sync (dxWDL 0.68.1 is based on Cromwell 32, I think).

…l-ngs into ct-vphaser-wdl

specify project for dxWDL compile; this appears to be required, though it is not mentioned in the docs? https://github.com/dnanexus/dxWDL/blob/0.68.1/src/main/scala/dxWDL/Main.scala#L142

tomkinsc added 30 commits June 18, 2018 18:29

add isnvs_per_sample to the end of two new WDL assembly workflows

9ddfb63

WDL isnv/vphaser WIP

36d337f

add dummy workflows to support newer cromwell that requires WDL files to have a workflow block; add single-sample isnv workflows

pass inputs to align_and_plot WDL

753cf40

add commas to WDL

e02c657

hide womtool PATH check stderr

861ab7c

additional debug text for WDL building to DNAnexus

d835f86

debug

411f9ac

rename isnv workflow WDL

e70f514

remove debug

fb2eece

update dx-toolkit

5c3a0e6

debug: comment out exit-on-error in build-dx

ff57066

force compile dxWDL

135b3b1

upgrade cromwell, dxWDL

9768872

move dummy workflow in reports.wdl

3de3d9c

remove dummy workflows from task files

e9083d4

add some missing workflow inputs to assemble_denovo.wdl

c26dfe0

restore dummy workflows to WDL task files

0bda254

restore dummy workflows to WDL task files; this allows womtool to validate them

add compulsory inputs to WDL workflows to satisfy newer versions of c…

e9a68e1

…romwell and dxWDL

use dummyworkflows as needed, remove from task WDLs

4b97674

remap dx default files; rename isnvs_per_sample -> isnvs_one_sample

071945f

remap dx defaults for optional files

02f0efe

add dx default file inputs for new isnv workflows

c968b63

add dx default file inputs for new isnv workflows (mostly taxon_filter DBs)

explicitly pass optional task inputs from workflows

c35a4ac

missed a default DB

750b3d8

dx debug

191ac82

WDL test input update

24c3b5e

correction

096bfb6

restore exit on error / pipefail for tests-dx

2a49f7e

tomkinsc added 10 commits June 27, 2018 16:26

debug

d3c47f2

debug dx workflow inputs

dc265dd

debug dx test inputs

f76a911

debug dx test inputs

f0c3be5

debug

710b596

debug

e8afaf2

remove set -x from dx scripts

3ea5f81

apply CIGAR string filtering only to non-None cigar strings (while al…

5c3f8a2

…lowing for the inverted case)

Merge branch 'master' into ct-vphaser-wdl

68585e3

test correction following merge

47259b9

Merge branch 'master' into ct-vphaser-wdl

165c8f7

dpark01 reviewed Jul 3, 2018

View reviewed changes

tomkinsc and others added 3 commits July 3, 2018 17:27

rework sample guessing for merge_to_vcf

d488eb1

add unit test for case where sample names are guessed for merge_to_vcf; rework the sectio where they are guessed

Merge branch 'master' into ct-vphaser-wdl

b3b16d2

Merge branch 'master' into ct-vphaser-wdl

831a68a

dpark01 approved these changes Jul 5, 2018

View reviewed changes

tomkinsc added 3 commits July 5, 2018 10:44

dxWDL 0.66.2 -> 0.68.1

76f21af

Merge branch 'ct-vphaser-wdl' of ssh://github.com/broadinstitute/vira…

4efe5e4

…l-ngs into ct-vphaser-wdl

update cromwell 32 -> 33.1

c07b793

tomkinsc and others added 6 commits July 5, 2018 11:14

roll back cromwell -> 32

5dc3067

Merge branch 'master' into ct-vphaser-wdl

c4824b8

bump dx-toolkit 240.1 -> 255.0

eb27cdc

Merge branch 'ct-vphaser-wdl' of ssh://github.com/broadinstitute/vira…

b608e8e

…l-ngs into ct-vphaser-wdl

specify project for dxWDL compile

9f0b8f7

specify project for dxWDL compile; this appears to be required, though it is not mentioned in the docs? https://github.com/dnanexus/dxWDL/blob/0.68.1/src/main/scala/dxWDL/Main.scala#L142

Merge branch 'master' into ct-vphaser-wdl

9e6b576

tomkinsc merged commit 567968f into master Jul 6, 2018

tomkinsc deleted the ct-vphaser-wdl branch July 6, 2018 02:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add WDL for iSNVs/v-phaser #828

Add WDL for iSNVs/v-phaser #828

tomkinsc commented Jun 27, 2018

dpark01 commented Jun 28, 2018

tomkinsc commented Jul 2, 2018

dpark01 left a comment

dpark01 Jul 3, 2018

tomkinsc Jul 3, 2018

dpark01 Jul 3, 2018

tomkinsc Jul 3, 2018

dpark01 Jul 3, 2018

tomkinsc Jul 3, 2018

dpark01 Jul 3, 2018

tomkinsc Jul 3, 2018

dpark01 Jul 3, 2018

tomkinsc Jul 3, 2018

dpark01 Jul 3, 2018

tomkinsc Jul 3, 2018

dpark01 left a comment

dpark01 Jul 5, 2018

tomkinsc Jul 5, 2018

dpark01 commented Jul 5, 2018

Add WDL for iSNVs/v-phaser #828

Add WDL for iSNVs/v-phaser #828

Conversation

tomkinsc commented Jun 27, 2018

dpark01 commented Jun 28, 2018

tomkinsc commented Jul 2, 2018

dpark01 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpark01 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpark01 commented Jul 5, 2018