make ReadFilter and Annotation packages configurable (#5573)

* changing ReadFilter and Annotations packages be loaded from the config file instead of being hardcoded
* closes #2155
* added a new method to base test runToolInNewJvm which lets you run a gatk tool in a clean jvm so you can alter static state like which config file was used.


Co-authored-by: Louis Bergelson <louisb@broadinstitute.org>

src/main/java/org/broadinstitute/hellbender/cmdline/GATKPlugin/GATKAnnotationPluginDescriptor.java

@@ -11,6 +11,8 @@
 import org.broadinstitute.hellbender.tools.walkers.annotator.Annotation;
 import org.broadinstitute.hellbender.tools.walkers.annotator.PedigreeAnnotation;
 import org.broadinstitute.hellbender.utils.Utils;
+import org.broadinstitute.hellbender.utils.config.ConfigFactory;
+import org.broadinstitute.hellbender.utils.config.GATKConfig;
 
 import java.io.File;
 import java.lang.reflect.Modifier;
@@ -32,10 +34,19 @@
  * NOTE: this class enforces that annotations with required arguments must see their arguments, yet this is not currently tested
  *       as no such annotations exist in the GATK.
  */
-public class GATKAnnotationPluginDescriptor  extends CommandLinePluginDescriptor<Annotation> {
-    //TODO this should be a configurable option or otherwise exposed to the user when configurations are fully supported.
-    public static final String pluginPackageName = "org.broadinstitute.hellbender.tools.walkers.annotator";
-    public static final Class<?> pluginBaseClass = org.broadinstitute.hellbender.tools.walkers.annotator.Annotation.class;
+public class GATKAnnotationPluginDescriptor extends CommandLinePluginDescriptor<Annotation> {
+    /**
+     * At startup, set the plugin package name to the one(s) in the configuration file.
+     */
+    private static final List<String> PLUGIN_PACKAGE_NAMES;
+    static {
+        // Get our configuration:
+        final GATKConfig config = ConfigFactory.getInstance().getGATKConfig();
+        // Exclude abstract classes and interfaces from the list of discovered codec classes
+        PLUGIN_PACKAGE_NAMES = Collections.unmodifiableList(config.annotation_packages());
+    }
+
+    private static final Class<?> PLUGIN_BASE_CLASS = org.broadinstitute.hellbender.tools.walkers.annotator.Annotation.class;
 
     protected transient Logger logger = LogManager.getLogger(this.getClass());
 
@@ -74,7 +85,7 @@ public class GATKAnnotationPluginDescriptor  extends CommandLinePluginDescriptor
      */
     @Override
     public Class<?> getPluginBaseClass() {
-        return pluginBaseClass;
+        return PLUGIN_BASE_CLASS;
     }
 
     /**
@@ -84,7 +95,7 @@ public Class<?> getPluginBaseClass() {
      */
     @Override
     public List<String> getPackageNames() {
-        return Collections.singletonList(pluginPackageName);
+        return PLUGIN_PACKAGE_NAMES;
     }
 
     /**
@@ -125,7 +136,7 @@ public GATKAnnotationPluginDescriptor(final GATKAnnotationArgumentCollection use
     }
     /**
      * Constructor that allows client tools to specify what annotations (optionally with parameters specified) to use as their defaults
-     * before discovery of user specified annotations. Defaults to using an empty GATKAnnotationArgumentCollection object. 
+     * before discovery of user specified annotations. Defaults to using an empty GATKAnnotationArgumentCollection object.
      *
      * @param toolDefaultAnnotations Default annotations that may be supplied with arguments
      *                               on the command line. May be null.
@@ -219,7 +230,7 @@ private void populateAnnotationGroups(final String simpleName, final Annotation
             // extend Annotation, groups are discovered by interrogating annotations for their interfaces and
             // associating the discovered annotations with their defined groups.
             // If a duplicate annotation is added, the group will opt to keep the old instantiation around
-            if ((inter != pluginBaseClass) && (pluginBaseClass.isAssignableFrom(inter))) {
+            if ((inter != PLUGIN_BASE_CLASS) && (PLUGIN_BASE_CLASS.isAssignableFrom(inter))) {
                 Map<String, Annotation> groupIdentity = (discoveredGroups.containsKey(inter.getSimpleName()) ? discoveredGroups.get(inter.getSimpleName()) : new HashMap<>());
                 groupIdentity.putIfAbsent(simpleName, annot);
                 discoveredGroups.put(inter.getSimpleName(), groupIdentity);

src/main/java/org/broadinstitute/hellbender/cmdline/GATKPlugin/GATKReadFilterPluginDescriptor.java

@@ -11,6 +11,8 @@
 import org.broadinstitute.hellbender.engine.filters.CountingReadFilter;
 import org.broadinstitute.hellbender.engine.filters.ReadFilter;
 import org.broadinstitute.hellbender.utils.Utils;
+import org.broadinstitute.hellbender.utils.config.ConfigFactory;
+import org.broadinstitute.hellbender.utils.config.GATKConfig;
 
 import java.io.IOException;
 import java.util.*;
@@ -30,7 +32,17 @@ private void readObject(java.io.ObjectInputStream in)
         logger = LogManager.getLogger(this.getClass()); // Logger is not serializable (even by Kryo)
     }
 
-    private static final String pluginPackageName = "org.broadinstitute.hellbender.engine.filters";
+    /**
+     * At startup, set the plugin package name to the one(s) in the configuration file.
+     */
+    private static final List<String> PLUGIN_PACKAGE_NAMES;
+    static {
+        // Get our configuration:
+        final GATKConfig config = ConfigFactory.getInstance().getGATKConfig();
+        // Exclude abstract classes and interfaces from the list of discovered codec classes
+        PLUGIN_PACKAGE_NAMES = Collections.unmodifiableList(config.read_filter_packages());
+    }
+
     private static final Class<ReadFilter> pluginBaseClass = org.broadinstitute.hellbender.engine.filters.ReadFilter.class;
     private static final String READ_PLUGIN_DISPLAY_NAME = "readFilter";
 
@@ -111,7 +123,7 @@ public String getDisplayName() {
      * @return
      */
     @Override
-    public List<String> getPackageNames() {return Collections.singletonList(pluginPackageName);}
+    public List<String> getPackageNames() {return PLUGIN_PACKAGE_NAMES;}
 
     @Override
     public boolean includePluginClass(Class<?> c) {

src/main/java/org/broadinstitute/hellbender/tools/walkers/annotator/VariantAnnotatorEngine.java

@@ -1,19 +1,17 @@
 package org.broadinstitute.hellbender.tools.walkers.annotator;
 
-import com.google.common.collect.Sets;
 import htsjdk.variant.variantcontext.*;
 import htsjdk.variant.vcf.*;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
-import org.broadinstitute.hellbender.engine.*;
 import org.broadinstitute.hellbender.engine.FeatureContext;
+import org.broadinstitute.hellbender.engine.FeatureDataSource;
 import org.broadinstitute.hellbender.engine.FeatureInput;
 import org.broadinstitute.hellbender.engine.ReferenceContext;
 import org.broadinstitute.hellbender.exceptions.GATKException;
 import org.broadinstitute.hellbender.exceptions.UserException;
 import org.broadinstitute.hellbender.tools.walkers.annotator.allelespecific.ReducibleAnnotation;
 import org.broadinstitute.hellbender.tools.walkers.annotator.allelespecific.ReducibleAnnotationData;
-import org.broadinstitute.hellbender.utils.ClassUtils;
 import org.broadinstitute.hellbender.utils.Utils;
 import org.broadinstitute.hellbender.utils.genotyper.ReadLikelihoods;
 import org.broadinstitute.hellbender.utils.variant.GATKVariantContextUtils;

src/main/java/org/broadinstitute/hellbender/utils/ClassUtils.java

@@ -49,17 +49,33 @@ public static <T> List<T> makeInstancesOfSubclasses(final Class<? extends T> cla
         final List<T> results = new ArrayList<>(classes.size());
 
         for (final Class<?> found: classes){
-            if (canMakeInstances(found)){
-                try {
-                    results.add((T)found.newInstance());
-                } catch (InstantiationException | IllegalAccessException e) {
-                    throw new GATKException("Problem making an instance of " + found + " Do check that the class has a non-arg constructor", e);
-                }
+            final T instance = (T) makeInstanceOf(found);
+            if (instance != null) {
+                results.add(instance);
             }
         }
         return results;
     }
 
+    /**
+     * Create objects of a concrete class.
+     *
+     * The public no-arg constructor is called to create the objects.
+     *
+     * @param clazz class to be instantiated
+     * @return new object or {@code null} if cannot be instantiated.
+     */
+    public static <T> T makeInstanceOf(final Class<T> clazz) {
+        if (canMakeInstances(clazz)) {
+            try {
+                return clazz.newInstance();
+            } catch (final InstantiationException | IllegalAccessException e) {
+                throw new GATKException("Problem making an instance of " + clazz + " Do check that the class has a non-arg constructor", e);
+            }
+        }
+        return null;
+    }
+
     /**
      * Finds sub-interfaces of the given interface (in the same package) and returns their simple names.
      */

src/main/java/org/broadinstitute/hellbender/utils/config/GATKConfig.java

@@ -1,10 +1,11 @@
 package org.broadinstitute.hellbender.utils.config;
 
+import com.google.common.annotations.VisibleForTesting;
 import org.aeonbits.owner.Accessible;
-import org.aeonbits.owner.Mutable;
 import org.aeonbits.owner.Config.LoadPolicy;
 import org.aeonbits.owner.Config.LoadType;
 import org.aeonbits.owner.Config.Sources;
+import org.aeonbits.owner.Mutable;
 
 import java.util.List;
 
@@ -52,6 +53,9 @@ public interface GATKConfig extends Mutable, Accessible {
      */
     String CONFIG_FILE_VARIABLE_CLASS_PATH = "GATKConfig.classPathToGatkConfig";
 
+    @VisibleForTesting
+    public String DEFAULT_ANNOTATION_PACKAGES = "org.broadinstitute.hellbender.tools.walkers.annotator";
+
     // =================================================================================================================
     // =================================================================================================================
     // System Options:
@@ -144,6 +148,12 @@ public interface GATKConfig extends Mutable, Accessible {
     @DefaultValue("htsjdk.variant,htsjdk.tribble,org.broadinstitute.hellbender.utils.codecs")
     List<String> codec_packages();
 
+    @DefaultValue("org.broadinstitute.hellbender.engine.filters")
+    List<String> read_filter_packages();
+
+    @DefaultValue(DEFAULT_ANNOTATION_PACKAGES)
+    List<String> annotation_packages();
+
     // ----------------------------------------------------------
     // GATKTool Options:
     // ----------------------------------------------------------

src/main/resources/org/broadinstitute/hellbender/utils/config/GATKConfig.properties

@@ -73,6 +73,8 @@ spark.executor.extraJavaOptions =
 # ---------------------------------------------------------------------------------
 
 codec_packages = htsjdk.variant, htsjdk.tribble, org.broadinstitute.hellbender.utils.codecs
+read_filter_packages = org.broadinstitute.hellbender.engine.filters
+annotation_packages = org.broadinstitute.hellbender.tools.walkers.annotator
 
 # ---------------------------------------------------------------------------------
 # GATKTool Options:

src/test/java/org/broadinstitute/hellbender/cmdline/GATKPlugin/GATKAnnotationPluginDescriptorUnitTest.java

@@ -1,28 +1,34 @@
 package org.broadinstitute.hellbender.cmdline.GATKPlugin;
 
-import org.broadinstitute.barclay.argparser.ClassFinder;
 import htsjdk.variant.variantcontext.*;
 import org.apache.commons.io.output.NullOutputStream;
-import org.broadinstitute.barclay.argparser.CommandLineArgumentParser;
-import org.broadinstitute.barclay.argparser.CommandLineException;
-import org.broadinstitute.barclay.argparser.CommandLineParser;
+import org.broadinstitute.barclay.argparser.*;
 import org.broadinstitute.hellbender.GATKBaseTest;
+import org.broadinstitute.hellbender.cmdline.GATKPlugin.testpluggables.TestAnnotation;
 import org.broadinstitute.hellbender.cmdline.StandardArgumentDefinitions;
+import org.broadinstitute.hellbender.cmdline.TestProgramGroup;
 import org.broadinstitute.hellbender.engine.FeatureContext;
+import org.broadinstitute.hellbender.engine.GATKTool;
 import org.broadinstitute.hellbender.engine.ReferenceContext;
 import org.broadinstitute.hellbender.exceptions.GATKException;
+import org.broadinstitute.hellbender.testutils.ArgumentsBuilder;
 import org.broadinstitute.hellbender.tools.walkers.annotator.*;
 import org.broadinstitute.hellbender.tools.walkers.annotator.allelespecific.AS_RMSMappingQuality;
 import org.broadinstitute.hellbender.tools.walkers.annotator.allelespecific.AS_StandardAnnotation;
+import org.broadinstitute.hellbender.utils.config.GATKConfig;
 import org.broadinstitute.hellbender.utils.genotyper.ReadLikelihoods;
 import org.broadinstitute.hellbender.utils.variant.GATKVCFConstants;
 import org.mockito.internal.util.collections.Sets;
+import org.mockito.internal.util.io.IOUtil;
 import org.testng.Assert;
 import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
 import org.testng.collections.Lists;
 
+import java.io.File;
+import java.io.IOException;
 import java.io.PrintStream;
+import java.nio.file.Files;
 import java.util.*;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
@@ -456,7 +462,7 @@ public void testUseAllAnnotations(){
         List<Annotation> annots = instantiateAnnotations(clp);
 
         ClassFinder finder = new ClassFinder();
-        finder.find(GATKAnnotationPluginDescriptor.pluginPackageName, Annotation.class);
+        finder.find(GATKConfig.DEFAULT_ANNOTATION_PACKAGES, Annotation.class);
 
         Set<Class<?>> classes = finder.getConcreteClasses();
         Assert.assertFalse(classes.isEmpty());
@@ -477,7 +483,7 @@ public void testUseAllAnnotationsDisableToolDefaultAnnotaiotns(){
         List<Annotation> annots = instantiateAnnotations(clp);
 
         ClassFinder finder = new ClassFinder();
-        finder.find(GATKAnnotationPluginDescriptor.pluginPackageName, Annotation.class);
+        finder.find(GATKConfig.DEFAULT_ANNOTATION_PACKAGES, Annotation.class);
 
         Set<Class<?>> classes = finder.getConcreteClasses();
         Assert.assertFalse(classes.isEmpty());
@@ -502,7 +508,7 @@ public void testIncludeAllExcludeIndividual(){
         Assert.assertFalse(annots.stream().anyMatch(a -> a.getClass()==AS_StandardAnnotation.class));
 
         ClassFinder finder = new ClassFinder();
-        finder.find(GATKAnnotationPluginDescriptor.pluginPackageName, Annotation.class);
+        finder.find(GATKConfig.DEFAULT_ANNOTATION_PACKAGES, Annotation.class);
 
         Set<Class<?>> classes = finder.getConcreteClasses();
         classes.remove(Coverage.class);
@@ -628,6 +634,7 @@ public List<String> getKeyNames() {
             return Collections.singletonList("Test");
         }
     }
+
     static class testParentAnnotation extends InfoFieldAnnotation implements ParentAnnotationGroup  {
         boolean dontAnnotate = false;
 
@@ -651,4 +658,37 @@ public List<String> getKeyNames() {
         }
     }
 
+    @CommandLineProgramProperties(summary="test tool to check annotation loading",
+            oneLineSummary = "test tool to check annotation loading",
+            programGroup = TestProgramGroup.class,
+            omitFromCommandLine = true)
+    public static class TestAnnotationsTool extends GATKTool {
+        @Argument(fullName = StandardArgumentDefinitions.OUTPUT_LONG_NAME)
+        String output;
+
+        @Override
+        public boolean useVariantAnnotations(){
+            return true;
+        }
+
+        @Override
+        public void traverse() {
+            String annotations = makeVariantAnnotations().stream()
+                    .map(a -> a.getClass().getSimpleName())
+                    .collect(Collectors.joining("\n"));
+            IOUtil.writeText(annotations, new File(output));
+        }
+    }
+
+    @Test
+    public void testConfigFileControlsAnnotationPackages() throws IOException {
+        final File output = createTempFile("annotations", "txt");
+        final File configFile = new File(packageRootTestDir + "cmdline/GATKPlugin/changePluginPackages.properties");
+        ArgumentsBuilder args = new ArgumentsBuilder();
+        args.addArgument(StandardArgumentDefinitions.ANNOTATION_LONG_NAME, TestAnnotation.class.getSimpleName())
+                .addFileArgument(StandardArgumentDefinitions.GATK_CONFIG_FILE_OPTION, configFile)
+                .addOutput(output);
+        runToolInNewJVM("TestAnnotationsTool", args);
+        Assert.assertEquals(Files.readAllLines(output.toPath()), Collections.singletonList(TestAnnotation.class.getSimpleName()));
+    }
 }

src/test/java/org/broadinstitute/hellbender/cmdline/GATKPlugin/GATKReadFilterPluginDescriptorTest.java

@@ -4,19 +4,26 @@
 import htsjdk.samtools.SAMFileHeader;
 import htsjdk.samtools.TextCigarCodec;
 import org.apache.commons.io.output.NullOutputStream;
-import org.broadinstitute.barclay.argparser.CommandLineArgumentParser;
-import org.broadinstitute.barclay.argparser.CommandLineException;
-import org.broadinstitute.barclay.argparser.CommandLineParser;
+import org.broadinstitute.barclay.argparser.*;
 import org.broadinstitute.hellbender.GATKBaseTest;
+import org.broadinstitute.hellbender.cmdline.GATKPlugin.testpluggables.TestReadFilter;
 import org.broadinstitute.hellbender.cmdline.ReadFilterArgumentDefinitions;
+import org.broadinstitute.hellbender.cmdline.StandardArgumentDefinitions;
+import org.broadinstitute.hellbender.cmdline.TestProgramGroup;
+import org.broadinstitute.hellbender.engine.GATKTool;
 import org.broadinstitute.hellbender.engine.filters.*;
+import org.broadinstitute.hellbender.testutils.ArgumentsBuilder;
 import org.broadinstitute.hellbender.utils.read.ArtificialReadUtils;
 import org.broadinstitute.hellbender.utils.read.GATKRead;
+import org.mockito.internal.util.io.IOUtil;
 import org.testng.Assert;
 import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
 
+import java.io.File;
+import java.io.IOException;
 import java.io.PrintStream;
+import java.nio.file.Files;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
@@ -786,5 +793,37 @@ private void setupReadNameTest(SetupTest setup) {
     private void setupSampleTest(SetupTest setup) {
         setup.hdr.getReadGroup(setup.read.getReadGroup()).setSample(setup.argValue);
     }
+
+    @CommandLineProgramProperties(summary="test tool to check ReadFilter loading",
+            oneLineSummary = "test tool to check ReadFilter loading",
+            programGroup = TestProgramGroup.class,
+            omitFromCommandLine = true)
+    public static class TestReadFiltersTool extends GATKTool {
+        @Argument(fullName = StandardArgumentDefinitions.OUTPUT_LONG_NAME)
+        String output;
+
+        @Override
+        public void traverse() {
+            String readFilters = getCommandLineParser()
+                    .getPluginDescriptor(GATKReadFilterPluginDescriptor.class)
+                    .getResolvedInstances()
+                    .stream()
+                    .map(a -> a.getClass().getSimpleName())
+                    .collect(Collectors.joining("\n"));
+            IOUtil.writeText(readFilters, new File(output));
+        }
+    }
+
+    @Test
+    public void testConfigFileControlsReadFiltersPackages() throws IOException {
+        final File output = createTempFile("annotations", "txt");
+        final File configFile = new File(packageRootTestDir + "cmdline/GATKPlugin/changePluginPackages.properties");
+        ArgumentsBuilder args = new ArgumentsBuilder();
+        args.addArgument(ReadFilterArgumentDefinitions.READ_FILTER_LONG_NAME, TestReadFilter.class.getSimpleName())
+                .addFileArgument(StandardArgumentDefinitions.GATK_CONFIG_FILE_OPTION, configFile)
+                .addOutput(output);
+        runToolInNewJVM("TestReadFiltersTool", args);
+        Assert.assertEquals(Files.readAllLines(output.toPath()), Arrays.asList(WellformedReadFilter.class.getSimpleName(), TestReadFilter.class.getSimpleName()));
+    }
 
 }