Consolidate code in PedigreeAnnotation

[bbimber]

Consolidate code in PedigreeAnnotation, provide getters, and allow PedigreeValidationType to be set

[bbimber]

@cmnbroad The use case behind this PR is custom PedigreeAnnotations outside GATK. Many of my proposed changes are just fixing typos or code-consolidation. The only other changes are adding protected getters for pedigreeFile, pedigreeValidationType, and sampleDB. I add a setter for pedigreeValidationType. Overall these should be pretty contained changes.

[bbimber]

@cmnbroad Sorry to bug here, but I am wondering if it would be possible for someone to review. This is a limited change that basically consolidates some internal code in PedigreeAnnotation and exposes a couple protected getters. Tests are passing. Thanks in advance.

[cmnbroad]

@bbimber The existing implementation of the Pedigree annotation hierarchy is pretty cumbersome, and the changes proposed here somewhat exacerbate that. We really want to go in the direction of separating out Founder annotations from base Pedigree annotaionts as started here, but nobody has had the the bandwidth to finish that. I suggested a simpler alternative for the interim that hopefully addresses the issue (assuming the issue is just being able to control the validation param).

[bbimber]

OK - PedigreeValidationType is now set in the constructor and is final. This does not separate the two intertwined codepaths around PedigreeFile vs. FounderIds, but that was a pre-existing problem. It doesnt doesnt change the pre-existing weirdness around the timing of setting pedigreeFile and/or founderIds within GATKAnnotationPluginDescriptor, where PedigreeAnnotation gets special treatment. I dont think this makes that situation any worse.

if you still have concerns on this proposal, I actually think I could make our code work if you simply exposed a protected getPedigreeFile() method on PedigreeAnnotation. I can make the SampleDB instance in my code without needed to share code here. It seemed useful to expose some of that code to avoid duplication, but if it's going to over-complicate we can remove it.

Also: that one test failure seems potentially unrelated (https://travis-ci.com/github/broadinstitute/gatk/jobs/510624560)? A compile issue with javadoc?

[cmnbroad]

@bbimber I was hoping this could be reduced to a single new File, PedigreeValidationType constructor overload, and the new File getter (and without any changes to the existing subclasses). Its also not a perfect solution, but I'd prefer to minimize addition of any new methods that expose founderIDs or SampleDB, since we aspire to factor out the existing code that uses those from this class completely. As for the failed test, it looks like the tests timed out for some reason, hopefully transient, but it I'm guessing its unrelated.

[bbimber]

@cmnbroad OK - what about this proposal? I just added a protected getter and fixed the typo in 'annotation'? We could expose a constructor based way to set PedigreeValidationType, but if you dont really want to expose more of the guts of PedigreeAnnotation to subclasses prior to splitting apart founderIds and pedigree, what about keeping this as simple and minimal as possible?

[cmnbroad]

If that solves your issue, then thats even better. The one change I'd request then is that you add javadoc saying that the getter can return null, since its possible to construct a PedigreeAnnotation without a File. Then once tests pass it should be good.

[bbimber]

@cmnbroad OK, added. travisci doesnt seem to have been kicked off - is that b/c github thinks there are unresolved change requests?

[cmnbroad]

LGTM once tests run and pass. @bbimber The issue with CI tests not running is being looked into. It may take a day or two to resolve.

[bbimber]

@cmnbroad OK. so will you be able to squash/merge on github or do you want me to do it and push here?

[cmnbroad]

I can squash when I merge - thanks.

[cmnbroad]

@bbimber this should run on travis now, but you probably have to push the branch again to trigger it to start.

[bbimber]

@cmnbroad is seems there's a docker rate-limit issue again: https://travis-ci.com/github/broadinstitute/gatk/jobs/512098673