Refactor GATKTool so that more tools can comfortably extend it directly

[droazen]

As discussed in #2471 (comment), we need to refactor GATKTool so that all non-Spark tools can comfortably extend it rather than extending CommandLineProgram directly, as some tools currently do. In particular, we need to:

• Provide a mechanism for subclasses to selectively disable engine-wide arguments such as -I completely (and also the ability to override with their own version of an argument).

• Access necessary datasources outside of the engine package.

• Add the ability to register input metadata such as sequence dictionaries, so that standard validation rules can be enforced across the toolkit.

• Add the ability for each tool to change the defaults for engine arguments such as --interval-merging-rule

[droazen]

@samuelklee Anything else you'd add to this list?

[samuelklee]

This looks like a good start! I'm not quite sure how you are planning to handle dictionary validation, specifically, but you can take a look at the CNV plotting tools (PlotDenoisedCopyRatios and PlotModeledSegments) to see what level of validation we currently do. We can discuss further in person if you like.

(Also, note that those tools take a sequence dictionary as an input to specify which contigs should be plotted; typically, this will be a subset of the full dictionary that excludes alt contigs, etc. Requiring this sequence-dictionary input is somewhat vestigial; previous versions of the pipeline did not include dictionaries in the headers of all CNV data files. Part of making these tools into GATKTools could include switching over to -L to specify regions for plotting.)

Finally, are the changes to -imr mentioned in #2471 going to be addressed in a separate issue?

[droazen]

@samuelklee For -imr, you just need a way to override the default setting, correct? Or is it more than that? I've added a bullet point to the list above that captures what I think you need, but feel free to supplement it with additional requirements.

[samuelklee]

Overriding the defaults would get us most of the way there. Right now, we perform the following check on the IAC and fail if the defaults aren't changed to values that the CNV tools require, which is awkward:

   /**
     * Validate that the interval-argument collection parameters minimally modify the input intervals.
     */
    public static void validateIntervalArgumentCollection(final IntervalArgumentCollection intervalArgumentCollection) {
        Utils.validateArg(intervalArgumentCollection.getIntervalSetRule() == IntervalSetRule.UNION,
                "Interval set rule must be set to UNION.");
        Utils.validateArg(intervalArgumentCollection.getIntervalExclusionPadding() == 0,
                "Interval exclusion padding must be set to 0.");
        Utils.validateArg(intervalArgumentCollection.getIntervalPadding() == 0,
                "Interval padding must be set to 0.");
        Utils.validateArg(intervalArgumentCollection.getIntervalMergingRule() == IntervalMergingRule.OVERLAPPING_ONLY,
                "Interval merging rule must be set to OVERLAPPING_ONLY.");
    }

If we override defaults, we'd still perform the check to make sure the user didn't muck with them, but it'd still be nicer than forcing the user to change the original defaults on their own.

However, there are still two more awkward points: 1) there is no value for -interval-set-rule that does nothing to the incoming intervals (you must either union or intersect), and 2) we have to specify our own padding argument in PreprocessIntervals that is distinct from -interval-padding, since we want to implement our own padding. The first can be easily addressed by adding an option to do nothing to the intervals; I'm not so sure what the best way to handle the second would be, so we can punt on it if it'd be more work than it's worth---these are relatively minor pain points, in the end.

[samuelklee]

Note #4439, which concerns a Picard tool that might also need options for interval merging exposed. Just something to be aware of---I'm guessing that it's probably a bit ambitious to have identical options for all interval inputs to both Picard and GATK tools?

[droazen]

One part of this ticket is done: #4964 added accessors that allow direct descendants of GATKTool to directly access engine datasources, while still forbidding direct access for tools that extend a Walker base class (except for Walker types living in the engine package, which still have access).

[samuelklee]

Not sure if there's a more relevant open issue, but just making note of #6924 here.