Added the ability for MarkDuplicatesSpark to accept multiple inputs #5430

jamesemery · 2018-11-16T21:16:38Z

I'm not sure how much I like the changes to GATKSparkTool.

Resolves #5398

tomwhite · 2018-11-19T10:49:21Z

src/main/java/org/broadinstitute/hellbender/engine/spark/GATKSparkTool.java

+        doNotMerge,
+        concatMerge,
+        mergeAndSort
+    }


The mergeAndSort policy is not used or implemented, so shouldn't be added yet. The walker version doesn't have such a policy as far as I can see, it effectively implements concatMerge, so just implementing the equivalent would be sufficient, no?

tomwhite · 2018-11-19T10:49:42Z

src/main/java/org/broadinstitute/hellbender/engine/spark/GATKSparkTool.java

@@ -158,6 +160,21 @@ public boolean requiresReads() {
        return false;
    }

+    /**
+     * Does this tool support multiple inputs? Tools that do should


Unfinished sentence.

tomwhite · 2018-11-19T10:50:26Z

src/main/java/org/broadinstitute/hellbender/engine/spark/GATKSparkTool.java

@@ -273,21 +293,36 @@ public SAMFileHeader getHeaderForReads() {
            traversalParameters = null; // no intervals were specified so return all reads (mapped and unmapped)
        }

-        // TODO: This if statement is a temporary hack until #959 gets resolved.
-        if (readInput.endsWith(".adam")) {
+        // TODO: This if statement is a temporary hack until #959 gets resolve


This comment doesn't apply to the next line - it should be moved to the code in getGatkReadJavaRDD.

tomwhite · 2018-11-19T10:53:46Z

src/main/java/org/broadinstitute/hellbender/engine/spark/GATKSparkTool.java

+     * @return doNotMerge by default
+     */
+    public ReadInputMergingPolicy getReadInputMergingPolicy() {
+        return ReadInputMergingPolicy.doNotMerge;


Why not default to concatMerge and support multiple inputs for all Spark tools?

I debated this with @lbergelson. It seems like an undesirable behavior for tools that tailor their behavior to the header sort order to union the RDDs of multiple inputs potentially invalidating any assumptions about input ordering.

I'm in favor of tools opting in.

tomwhite · 2018-11-19T10:55:15Z

src/main/java/org/broadinstitute/hellbender/engine/spark/GATKSparkTool.java

+        headers.addAll(readInputs.values());
+
+        SamFileHeaderMerger headerMerger = new SamFileHeaderMerger(identifySortOrder(headers), headers, true);
+        return headerMerger;


No need for headerMerger variable.

tomwhite · 2018-11-19T10:58:57Z

src/main/java/org/broadinstitute/hellbender/engine/spark/GATKSparkTool.java

+     */
+    private SamFileHeaderMerger createHeaderMerger() {
+        List<SAMFileHeader> headers = new ArrayList<>(readInputs.size());
+        headers.addAll(readInputs.values());


Does this need to be a list? You are using a LinkedHashMap, so you know values() will be ordered correctly. SamFileHeaderMerger takes a Collection, and identifySortOrder could too.

tomwhite · 2018-11-19T11:00:00Z

.../org/broadinstitute/hellbender/tools/spark/pipelines/MarkDuplicatesSparkIntegrationTest.java

+
+    @Test (expectedExceptions = UserException.class)
+    public void testAssertCorrectSortOrderMultipleBams() {
+        // This test asserts that the handling of two read pairs with the same start positions but on different in such a way


Comment wording is a bit unclear.

jamesemery · 2018-11-28T20:33:35Z

@tomwhite Can I get another round of review on this branch?

codecov-io · 2018-11-28T21:34:32Z

Codecov Report

Merging #5430 into master will decrease coverage by 0.003%.
The diff coverage is 86.25%.

@@               Coverage Diff               @@
##              master     #5430       +/-   ##
===============================================
- Coverage     87.085%   87.082%   -0.003%     
- Complexity     31222     31251       +29     
===============================================
  Files           1915      1915               
  Lines         144079    144178       +99     
  Branches       15891     15910       +19     
===============================================
+ Hits          125471    125553       +82     
- Misses         12837     12839        +2     
- Partials        5771      5786       +15

Impacted Files	Coverage Δ	Complexity Δ
...pelines/metrics/QualityScoreDistributionSpark.java	`95.238% <100%> (ø)`	`16 <0> (ø)`	⬇️
...transforms/markduplicates/MarkDuplicatesSpark.java	`94.872% <100%> (+0.427%)`	`38 <9> (+4)`	⬆️
...pipelines/metrics/CollectMultipleMetricsSpark.java	`92.593% <100%> (ø)`	`9 <0> (ø)`	⬇️
...k/pipelines/metrics/MetricsCollectorSparkTool.java	`75% <100%> (ø)`	`3 <0> (ø)`	⬇️
...ark/pipelines/metrics/MeanQualityByCycleSpark.java	`90.816% <100%> (ø)`	`11 <0> (ø)`	⬇️
...s/metrics/CollectBaseDistributionByCycleSpark.java	`87.037% <100%> (ø)`	`9 <0> (ø)`	⬇️
...stitute/hellbender/engine/spark/GATKSparkTool.java	`82.286% <76.923%> (-0.147%)`	`78 <9> (+10)`
.../pipelines/MarkDuplicatesSparkIntegrationTest.java	`91.144% <93.103%> (-0.262%)`	`42 <1> (+6)`
...hellbender/utils/runtime/RuntimeUtilsUnitTest.java	`87.5% <0%> (-12.5%)`	`4% <0%> (+1%)`
...te/hellbender/tools/funcotator/OutputRenderer.java	`92.857% <0%> (-7.143%)`	`4% <0%> (ø)`
... and 17 more

droazen · 2018-12-10T19:13:09Z

@tomwhite Looks like your latest comments have been addressed -- could you have another look at this branch when you get a chance?

lbergelson

@jamesemery Looks good to me. A few nitpicks but feel free to disregard them.

lbergelson · 2018-12-10T21:07:28Z

src/main/java/org/broadinstitute/hellbender/engine/spark/GATKSparkTool.java

@@ -442,7 +474,17 @@ public boolean useVariantAnnotations() {
     * Returns the name of the source of reads data. It can be a file name or URL.
     */
    protected String getReadSourceName(){
-        return readInput;
+        if (readInputs.size() > 1) {
+            throw new GATKException("Multiple ReadsDataSources specificed but a single source requested by the tool");


not ideal maybe but I don't know what else to do about this method... maybe change it to getReadSourceNames and return a list?

Right, I wanted to avoid returning a list because there are a bunch of places in the code where tools expect there to be only one read source kicking around and I didn't want to risk breaking something or having to uproot everything... I agree its pretty gross... Theoretically it shouldn't be a problem for most tools which don't accept multiple inputs anyway

I think we should change it to a list, or even get rid of it. All of the existing consumers except one use it to generate an output name for saving metrics (relatively easy to change); the other one (getRecommendedNumReducers) uses it to determine the number of reducers, which won't work correctly on multiple inputs with this implementation. I think getRecommendedNumReducers should be updated either way.

@cmnbroad Alright, I'll return a list. getRecommendedNumReducers has already been updated in this branch to sum over the all the read input files.

lbergelson · 2018-12-10T21:08:37Z

src/main/java/org/broadinstitute/hellbender/engine/spark/GATKSparkTool.java

+     * @return doNotMerge by default
+     */
+    public ReadInputMergingPolicy getReadInputMergingPolicy() {
+        return ReadInputMergingPolicy.doNotMerge;


I'm in favor of tools opting in.

lbergelson · 2018-12-10T21:09:58Z

src/main/java/org/broadinstitute/hellbender/engine/spark/GATKSparkTool.java

+    }
+    @VisibleForTesting
+    static SAMFileHeader.SortOrder identifySortOrder(final Collection<SAMFileHeader> headers){
+        final Set<SAMFileHeader.SortOrder> sortOrders = headers.stream().map(SAMFileHeader::getSortOrder).collect(Collectors.toSet());


Clever way to check this.

lbergelson · 2018-12-10T21:12:27Z

src/main/java/org/broadinstitute/hellbender/engine/spark/GATKSparkTool.java

+     */
+    private SamFileHeaderMerger createHeaderMerger() {
+        return new SamFileHeaderMerger(identifySortOrder(readInputs.values()), readInputs.values(), true);
+    }


There should be a newline here:

Suggested change

}

}

lbergelson · 2018-12-10T21:13:07Z

src/main/java/org/broadinstitute/hellbender/engine/spark/GATKSparkTool.java

+        readsHeader = createHeaderMerger().getMergedHeader();
+    }
+
+


too many newlines here...

James responded to Tom's review already.

… if they are queryname sorted

…ng test failures

jamesemery requested review from tomwhite and lbergelson November 16, 2018 21:16

tomwhite previously requested changes Nov 19, 2018

View reviewed changes

droazen assigned tomwhite Nov 30, 2018

lbergelson approved these changes Dec 10, 2018

View reviewed changes

lbergelson assigned jamesemery and unassigned tomwhite Dec 10, 2018

tomwhite approved these changes Dec 11, 2018

View reviewed changes

jamesemery added 7 commits January 2, 2019 15:21

some progress?

c123270

Added the ability for MarkDuplicatesSpark to take multiple input bams…

d2ce086

… if they are queryname sorted

responded to comments

27dd6c1

fixing a compiler warning

d6c47f2

Fixed a bug in the recommended number of reducers code that was causi…

7ed7a00

…ng test failures

resplonding to final comments

c4efaab

resovling the conflict of the orders

bf58fd3

jamesemery force-pushed the je_makeMarkDuplicatesSparkAcceptMultipleInputs branch from b558fb2 to bf58fd3 Compare January 2, 2019 20:36

jamesemery added 2 commits January 3, 2019 11:00

fixing the merge conflict failures

3a54fdb

responding to cnormans comment

73155c6

jamesemery merged commit 1fb980c into master Jan 3, 2019

jamesemery deleted the je_makeMarkDuplicatesSparkAcceptMultipleInputs branch January 3, 2019 21:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added the ability for MarkDuplicatesSpark to accept multiple inputs #5430

Added the ability for MarkDuplicatesSpark to accept multiple inputs #5430

jamesemery commented Nov 16, 2018

tomwhite Nov 19, 2018

tomwhite Nov 19, 2018

tomwhite Nov 19, 2018

tomwhite Nov 19, 2018

jamesemery Nov 19, 2018

lbergelson Dec 10, 2018

tomwhite Dec 11, 2018

tomwhite Nov 19, 2018

tomwhite Nov 19, 2018

tomwhite Nov 19, 2018

jamesemery commented Nov 28, 2018

codecov-io commented Nov 28, 2018 •

edited

Loading

droazen commented Dec 10, 2018

lbergelson left a comment

lbergelson Dec 10, 2018

jamesemery Jan 2, 2019

cmnbroad Jan 2, 2019 •

edited

Loading

jamesemery Jan 3, 2019

lbergelson Dec 10, 2018

lbergelson Dec 10, 2018

lbergelson Dec 10, 2018

lbergelson Dec 10, 2018

Added the ability for MarkDuplicatesSpark to accept multiple inputs #5430

Added the ability for MarkDuplicatesSpark to accept multiple inputs #5430

Conversation

jamesemery commented Nov 16, 2018

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

jamesemery commented Nov 28, 2018

codecov-io commented Nov 28, 2018 • edited Loading

Codecov Report

droazen commented Dec 10, 2018

lbergelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmnbroad Jan 2, 2019 • edited Loading

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

codecov-io commented Nov 28, 2018 •

edited

Loading

cmnbroad Jan 2, 2019 •

edited

Loading