From 70e1aef5508e911a40ba96071125ab1d37c32425 Mon Sep 17 00:00:00 2001 From: hanna Date: Tue, 29 Sep 2009 22:23:19 +0000 Subject: [PATCH] Better integrate the @ArgumentCollection into the command-line argument parser. Walkers can now specify their own @ArgumentCollections. Also cleaned up a bit of the CommandLineProgram template method pattern to minimize duplicate code. git-svn-id: file:///humgen/gsa-scr1/gsa-engineering/svn_contents/trunk@1746 348d0f76-0448-11de-a6fe-93d51630548a --- .../sting/gatk/CommandLineExecutable.java | 6 +- .../sting/gatk/GATKArgumentCollection.java | 6 -- .../sting/gatk/GenomeAnalysisEngine.java | 4 +- .../sting/gatk/io/OutputTracker.java | 4 +- .../GenotypeCalculationModelFactory.java | 25 ++++++++ .../genotyper/UnifiedArgumentCollection.java | 25 ++++++++ .../walkers/genotyper/UnifiedGenotyper.java | 25 ++++++++ .../broadinstitute/sting/utils/JVMUtils.java | 18 +++++- .../sting/utils/cmdLine/ArgumentMatches.java | 10 ++- .../sting/utils/cmdLine/ArgumentSource.java | 17 ++++-- .../sting/utils/cmdLine/ParsingEngine.java | 61 ++++++++++++++----- .../gatk/GATKArgumentCollectionTest.java | 2 - .../utils/cmdLine/ParsingEngineTest.java | 33 +++++++++- 13 files changed, 198 insertions(+), 38 deletions(-) diff --git a/java/src/org/broadinstitute/sting/gatk/CommandLineExecutable.java b/java/src/org/broadinstitute/sting/gatk/CommandLineExecutable.java index 55ac87b03..9407d9d8f 100644 --- a/java/src/org/broadinstitute/sting/gatk/CommandLineExecutable.java +++ b/java/src/org/broadinstitute/sting/gatk/CommandLineExecutable.java @@ -47,7 +47,6 @@ import java.util.Arrays; public abstract class CommandLineExecutable extends CommandLineProgram { public Object GATKResult = null; - /** * The actual engine which performs the analysis. */ @@ -77,19 +76,16 @@ public abstract class CommandLineExecutable extends CommandLineProgram { } protected Object executeGATK() { - GATKArgumentCollection arguments = getArgumentCollection(); - Walker mWalker = GATKEngine.getWalkerByName(getAnalysisName()); // load the arguments into the walkers - loadArgumentsIntoObject(arguments); loadArgumentsIntoObject(mWalker); // process any arguments that need a second pass + GATKArgumentCollection arguments = getArgumentCollection(); processArguments(arguments); // set the analysis name in the argument collection - arguments.analysisName = getAnalysisName(); return GATKEngine.execute(arguments, mWalker); } diff --git a/java/src/org/broadinstitute/sting/gatk/GATKArgumentCollection.java b/java/src/org/broadinstitute/sting/gatk/GATKArgumentCollection.java index 954a254f1..479520bc7 100755 --- a/java/src/org/broadinstitute/sting/gatk/GATKArgumentCollection.java +++ b/java/src/org/broadinstitute/sting/gatk/GATKArgumentCollection.java @@ -47,9 +47,6 @@ public class GATKArgumentCollection { public GATKArgumentCollection() { } - @Element(required = false) - public String analysisName = null; - @ElementMap(entry = "analysis_argument", key = "key", attribute = true, inline = true, required = false) public Map walkerArgs = new HashMap(); @@ -235,9 +232,6 @@ public class GATKArgumentCollection { if (!other.intervals.equals(this.intervals)) { return false; } - if (!other.analysisName.equals(this.analysisName)) { - return false; - } if (!other.DBSNPFile.equals(this.DBSNPFile)) { return false; } diff --git a/java/src/org/broadinstitute/sting/gatk/GenomeAnalysisEngine.java b/java/src/org/broadinstitute/sting/gatk/GenomeAnalysisEngine.java index 4707a5060..37af077a4 100755 --- a/java/src/org/broadinstitute/sting/gatk/GenomeAnalysisEngine.java +++ b/java/src/org/broadinstitute/sting/gatk/GenomeAnalysisEngine.java @@ -105,6 +105,7 @@ public class GenomeAnalysisEngine { * Actually run the GATK with the specified walker. * @param args the argument collection, where we get all our setup information from * @param my_walker Walker to run over the dataset. Must not be null. + * @return the value of this traversal. */ public Object execute(GATKArgumentCollection args, Walker my_walker) { // validate our parameters @@ -225,7 +226,7 @@ public class GenomeAnalysisEngine { Utils.scareUser(String.format("Read-based traversals require a reference file but none was given")); microScheduler = MicroScheduler.create(my_walker, readsDataSource, referenceDataSource, rodDataSources, argCollection.numberOfThreads); } else { - Utils.scareUser(String.format("Unable to create the appropriate TraversalEngine for analysis type " + argCollection.analysisName)); + Utils.scareUser(String.format("Unable to create the appropriate TraversalEngine for analysis type %s", WalkerManager.getWalkerName(my_walker.getClass()))); } return microScheduler; @@ -615,6 +616,7 @@ public class GenomeAnalysisEngine { * Initialize the output streams as specified by the user. * * @param walker the walker to initialize output streams for + * @param outputTracker the tracker supplying the initialization data. */ private void initializeOutputStreams(Walker walker, OutputTracker outputTracker) { if( argCollection.outErrFileName != null ) diff --git a/java/src/org/broadinstitute/sting/gatk/io/OutputTracker.java b/java/src/org/broadinstitute/sting/gatk/io/OutputTracker.java index cbf1f8004..a79c02261 100755 --- a/java/src/org/broadinstitute/sting/gatk/io/OutputTracker.java +++ b/java/src/org/broadinstitute/sting/gatk/io/OutputTracker.java @@ -103,7 +103,7 @@ public abstract class OutputTracker { targetValue = builder.build(); } - JVMUtils.setField( targetField.field, walker, targetValue ); + JVMUtils.setFieldValue( targetField.field, walker, targetValue ); } } @@ -171,6 +171,6 @@ public abstract class OutputTracker { */ private void installStub( Walker walker, String fieldName, OutputStream outputStream ) { Field field = JVMUtils.findField( walker.getClass(), fieldName ); - JVMUtils.setField( field, walker, outputStream ); + JVMUtils.setFieldValue( field, walker, outputStream ); } } diff --git a/java/src/org/broadinstitute/sting/gatk/walkers/genotyper/GenotypeCalculationModelFactory.java b/java/src/org/broadinstitute/sting/gatk/walkers/genotyper/GenotypeCalculationModelFactory.java index de3e717d9..9a0b52f30 100755 --- a/java/src/org/broadinstitute/sting/gatk/walkers/genotyper/GenotypeCalculationModelFactory.java +++ b/java/src/org/broadinstitute/sting/gatk/walkers/genotyper/GenotypeCalculationModelFactory.java @@ -1,3 +1,28 @@ +/* + * Copyright (c) 2009 The Broad Institute + * + * Permission is hereby granted, free of charge, to any person + * obtaining a copy of this software and associated documentation + * files (the "Software"), to deal in the Software without + * restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following + * conditions: + * + * The above copyright notice and this permission notice shall be + * included in all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES + * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT + * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, + * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR + * OTHER DEALINGS IN THE SOFTWARE. + */ + package org.broadinstitute.sting.gatk.walkers.genotyper; import static org.broadinstitute.sting.gatk.walkers.genotyper.GenotypeCalculationModel.Model.*; diff --git a/java/src/org/broadinstitute/sting/gatk/walkers/genotyper/UnifiedArgumentCollection.java b/java/src/org/broadinstitute/sting/gatk/walkers/genotyper/UnifiedArgumentCollection.java index 5c7428aba..998b1b170 100755 --- a/java/src/org/broadinstitute/sting/gatk/walkers/genotyper/UnifiedArgumentCollection.java +++ b/java/src/org/broadinstitute/sting/gatk/walkers/genotyper/UnifiedArgumentCollection.java @@ -1,3 +1,28 @@ +/* + * Copyright (c) 2009 The Broad Institute + * + * Permission is hereby granted, free of charge, to any person + * obtaining a copy of this software and associated documentation + * files (the "Software"), to deal in the Software without + * restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following + * conditions: + * + * The above copyright notice and this permission notice shall be + * included in all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES + * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT + * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, + * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR + * OTHER DEALINGS IN THE SOFTWARE. + */ + package org.broadinstitute.sting.gatk.walkers.genotyper; import org.broadinstitute.sting.utils.cmdLine.Argument; diff --git a/java/src/org/broadinstitute/sting/gatk/walkers/genotyper/UnifiedGenotyper.java b/java/src/org/broadinstitute/sting/gatk/walkers/genotyper/UnifiedGenotyper.java index 62b64d9e4..2db39342b 100755 --- a/java/src/org/broadinstitute/sting/gatk/walkers/genotyper/UnifiedGenotyper.java +++ b/java/src/org/broadinstitute/sting/gatk/walkers/genotyper/UnifiedGenotyper.java @@ -1,3 +1,28 @@ +/* + * Copyright (c) 2009 The Broad Institute + * + * Permission is hereby granted, free of charge, to any person + * obtaining a copy of this software and associated documentation + * files (the "Software"), to deal in the Software without + * restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following + * conditions: + * + * The above copyright notice and this permission notice shall be + * included in all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES + * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT + * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, + * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR + * OTHER DEALINGS IN THE SOFTWARE. + */ + package org.broadinstitute.sting.gatk.walkers.genotyper; import org.broadinstitute.sting.gatk.GenomeAnalysisEngine; diff --git a/java/src/org/broadinstitute/sting/utils/JVMUtils.java b/java/src/org/broadinstitute/sting/utils/JVMUtils.java index cdfed4c4a..7ba576a6c 100755 --- a/java/src/org/broadinstitute/sting/utils/JVMUtils.java +++ b/java/src/org/broadinstitute/sting/utils/JVMUtils.java @@ -75,7 +75,7 @@ public class JVMUtils { * @param instance Instance in which to set the field. * @param value The value to which to set the given field in the given instance. */ - public static void setField( Field field, Object instance, Object value ) { + public static void setFieldValue( Field field, Object instance, Object value ) { try { field.setAccessible(true); field.set(instance, value); @@ -85,4 +85,20 @@ public class JVMUtils { } } + /** + * Gets the value stored in the provided field in the given instance. + * @param field Field to set in the given object. + * @param instance Instance in which to set the field. + * @return Value currently stored in the given field. + */ + public static Object getFieldValue( Field field, Object instance ) { + try { + field.setAccessible(true); + return field.get(instance); + } + catch( IllegalAccessException ex ) { + throw new StingException(String.format("Could not retrieve %s in instance %s",field.getName(),instance.getClass().getName())); + } + } + } diff --git a/java/src/org/broadinstitute/sting/utils/cmdLine/ArgumentMatches.java b/java/src/org/broadinstitute/sting/utils/cmdLine/ArgumentMatches.java index 985350744..134f9dd80 100755 --- a/java/src/org/broadinstitute/sting/utils/cmdLine/ArgumentMatches.java +++ b/java/src/org/broadinstitute/sting/utils/cmdLine/ArgumentMatches.java @@ -124,6 +124,7 @@ public class ArgumentMatches implements Iterable { /** * Find all successful matches (a 'successful' match is one paired with a definition). + * @return All successful matches. */ ArgumentMatches findSuccessfulMatches() { ArgumentMatches matches = new ArgumentMatches(); @@ -151,7 +152,6 @@ public class ArgumentMatches implements Iterable { * Merges the given argument match into the set of existing argument matches. * If multiple arguments are present, those arguments will end up grouped. * @param match The match to merge into. - * @return The merged match. */ void mergeInto( ArgumentMatch match ) { boolean definitionExists = false; @@ -228,6 +228,14 @@ class ArgumentMatch implements Iterable { indices.put(index,values ); } + /** + * Return a string representation of the given argument match, for debugging purposes. + * @return String representation of the match. + */ + public String toString() { + return label; + } + /** * Creates an iterator that walks over each individual match at each position of a given argument. * @return An iterator over the individual matches in this argument. Will not be null. diff --git a/java/src/org/broadinstitute/sting/utils/cmdLine/ArgumentSource.java b/java/src/org/broadinstitute/sting/utils/cmdLine/ArgumentSource.java index fc3a0f6bf..e766130b9 100644 --- a/java/src/org/broadinstitute/sting/utils/cmdLine/ArgumentSource.java +++ b/java/src/org/broadinstitute/sting/utils/cmdLine/ArgumentSource.java @@ -25,8 +25,6 @@ package org.broadinstitute.sting.utils.cmdLine; -import org.broadinstitute.sting.utils.StingException; - import java.lang.reflect.Field; import java.util.Collection; import java.util.List; @@ -96,11 +94,12 @@ public class ArgumentSource { } /** - * Injects the specified value into the selected field of the instance. - * @param targetInstance Instance into which to inject the parsed value. + * Parses the specified value based on the specified type. + * @param source The type of value to be parsed. * @param values String representation of all values passed. + * @return the parsed value of the object. */ - public Object parse( ArgumentSource source, Object targetInstance, ArgumentMatches values ) { + public Object parse( ArgumentSource source, ArgumentMatches values ) { Object value = null; if( !isFlag() ) { ArgumentTypeDescriptor typeDescriptor = ArgumentTypeDescriptor.create( field.getType() ); @@ -128,4 +127,12 @@ public class ArgumentSource { Class argumentType = field.getType(); return Collection.class.isAssignableFrom(argumentType) || field.getType().isArray(); } + + /** + * Gets a string representation of the argument source for debugging. + * @return String representation of the argument source. + */ + public String toString() { + return clazz.getSimpleName() + ": " + field.getName(); + } } diff --git a/java/src/org/broadinstitute/sting/utils/cmdLine/ParsingEngine.java b/java/src/org/broadinstitute/sting/utils/cmdLine/ParsingEngine.java index 772fb7b7d..eadef98d4 100755 --- a/java/src/org/broadinstitute/sting/utils/cmdLine/ParsingEngine.java +++ b/java/src/org/broadinstitute/sting/utils/cmdLine/ParsingEngine.java @@ -88,7 +88,7 @@ public class ParsingEngine { */ public void addArgumentSource( String sourceName, Class sourceClass ) { List argumentsFromSource = new ArrayList(); - for( ArgumentSource argumentSource: extractArgumentSources(sourceClass,true) ) + for( ArgumentSource argumentSource: extractArgumentSources(sourceClass) ) argumentsFromSource.addAll( argumentSource.createArgumentDefinitions() ); argumentDefinitions.add( new ArgumentDefinitionGroup(sourceName, argumentsFromSource) ); } @@ -246,31 +246,46 @@ public class ParsingEngine { * @param object Object into which to add arguments. */ public void loadArgumentsIntoObject( Object object ) { - // Get a list of argument sources, not including the children of this argument. For now, skip loading - // arguments into the object recursively. - List argumentSources = extractArgumentSources( object.getClass(), false ); + List argumentSources = extractArgumentSources(object.getClass()); for( ArgumentSource argumentSource: argumentSources ) loadMatchesIntoObject( argumentSource, object, argumentMatches.findMatches(argumentSource) ); } /** - * Loads a single argument into the object. + * Loads a single argument into the object and that objects children. * @param argumentMatches Argument matches to load into the object. - * @param target + * @param source Argument source to load into the object. + * @param instance Object into which to inject the value. The target might be in a container within the instance. */ - private void loadMatchesIntoObject( ArgumentSource source, Object target, ArgumentMatches argumentMatches ) { + private void loadMatchesIntoObject( ArgumentSource source, Object instance, ArgumentMatches argumentMatches ) { // Nothing to load if( argumentMatches.size() == 0 ) return; - if( source.clazz.isAssignableFrom(target.getClass()) ) { - Object value = source.parse( source, target, argumentMatches ); - JVMUtils.setField( source.field, target, value ); + // Target instance into which to inject the value. + List targets = new ArrayList(); + + // Check to see whether the instance itself can be the target. + if( source.clazz.isAssignableFrom(instance.getClass()) ) { + targets.add(instance); + } + + // Check to see whether a contained class can be the target. + targets.addAll(getContainersMatching(instance,source.clazz)); + + // Abort if no home is found for the object. + if( targets.size() == 0 ) + throw new StingException("Internal command-line parser error: unable to find a home for argument matches " + argumentMatches); + + for( Object target: targets ) { + Object value = source.parse( source, argumentMatches ); + JVMUtils.setFieldValue( source.field, target, value ); } } /** * Prints out the help associated with these command-line argument definitions. + * @param applicationDetails Details about the specific GATK-based application being run. */ public void printHelp( ApplicationDetails applicationDetails ) { new HelpFormatter().printHelp(applicationDetails,argumentDefinitions); @@ -279,18 +294,17 @@ public class ParsingEngine { /** * Extract all the argument sources from a given object. * @param sourceClass class to act as sources for other arguments. - * @param recursive Whether to recursively look for argument collections and add their contents. * @return A list of sources associated with this object and its aggregated objects. */ - private List extractArgumentSources( Class sourceClass, boolean recursive ) { + private List extractArgumentSources(Class sourceClass) { List argumentSources = new ArrayList(); while( sourceClass != null ) { Field[] fields = sourceClass.getDeclaredFields(); for( Field field: fields ) { if( field.isAnnotationPresent(Argument.class) ) argumentSources.add( new ArgumentSource(sourceClass,field) ); - if( field.isAnnotationPresent(ArgumentCollection.class) && recursive ) - argumentSources.addAll( extractArgumentSources(field.getType(),recursive) ); + if( field.isAnnotationPresent(ArgumentCollection.class) ) + argumentSources.addAll( extractArgumentSources(field.getType()) ); } sourceClass = sourceClass.getSuperclass(); } @@ -314,6 +328,7 @@ public class ParsingEngine { /** * Parse a short name into an ArgumentMatch. * @param token The token to parse. The token should pass the isLongArgumentForm test. + * @param position The position of the token in question. * @return ArgumentMatch associated with this token, or null if no match exists. */ private ArgumentMatch parseArgument( String token, int position ) { @@ -328,6 +343,24 @@ public class ParsingEngine { // No parse results found. return null; } + + /** + * Gets a list of the container instances of the given type stored within the given target. + * @param target Class holding the container. + * @param type Container type. + * @return A list of containers matching the given type. + */ + private List getContainersMatching(Object target, Class type) { + List containers = new ArrayList(); + + Field[] fields = target.getClass().getDeclaredFields(); + for( Field field: fields ) { + if( field.isAnnotationPresent(ArgumentCollection.class) && type.isAssignableFrom(field.getType()) ) + containers.add(JVMUtils.getFieldValue(field,target)); + } + + return containers; + } } /** diff --git a/java/test/org/broadinstitute/sting/gatk/GATKArgumentCollectionTest.java b/java/test/org/broadinstitute/sting/gatk/GATKArgumentCollectionTest.java index 61772462f..945db5180 100755 --- a/java/test/org/broadinstitute/sting/gatk/GATKArgumentCollectionTest.java +++ b/java/test/org/broadinstitute/sting/gatk/GATKArgumentCollectionTest.java @@ -1,7 +1,6 @@ package org.broadinstitute.sting.gatk; import org.broadinstitute.sting.BaseTest; -import org.broadinstitute.sting.gatk.GATKArgumentCollection; import org.junit.After; import static org.junit.Assert.fail; import org.junit.Before; @@ -83,7 +82,6 @@ public class GATKArgumentCollectionTest extends BaseTest { collect.maximumEngineIterations = -1; collect.strictnessLevel = SAMFileReader.ValidationStringency.STRICT; collect.referenceFile = new File("referenceFile".toLowerCase()); - collect.analysisName = "analysisName".toLowerCase(); collect.DBSNPFile = "DBSNPFile".toLowerCase(); collect.HAPMAPFile = "HAPMAPFile".toLowerCase(); collect.HAPMAPChipFile = "HAPMAPChipFile".toLowerCase(); diff --git a/java/test/org/broadinstitute/sting/utils/cmdLine/ParsingEngineTest.java b/java/test/org/broadinstitute/sting/utils/cmdLine/ParsingEngineTest.java index ce0e0cf3c..e3509bac6 100755 --- a/java/test/org/broadinstitute/sting/utils/cmdLine/ParsingEngineTest.java +++ b/java/test/org/broadinstitute/sting/utils/cmdLine/ParsingEngineTest.java @@ -201,7 +201,7 @@ public class ParsingEngineTest extends BaseTest { Assert.assertEquals("Enum value is not correct", TestEnum.THREE, argProvider.testEnum); } - public enum TestEnum { ONE, TWO, THREE }; + public enum TestEnum { ONE, TWO, THREE } private class EnumArgProvider { @Argument(fullName="test_enum",shortName="ti",doc="test enum",required=false) @@ -562,4 +562,35 @@ public class ParsingEngineTest extends BaseTest { @Argument(doc="value",validation="\\d+") Integer value; } + + @Test + public void argumentCollectionTest() { + String[] commandLine = new String[] { "--value", "5" }; + + parsingEngine.addArgumentSource( ArgumentCollectionProvider.class ); + parsingEngine.parse( commandLine ); + parsingEngine.validate(); + + ArgumentCollectionProvider argProvider = new ArgumentCollectionProvider(); + parsingEngine.loadArgumentsIntoObject(argProvider); + + Assert.assertEquals("Argument is not correctly initialized", 5, argProvider.rap.value.intValue() ); + } + + private class ArgumentCollectionProvider { + @ArgumentCollection + RequiredArgProvider rap = new RequiredArgProvider(); + } + + @Test(expected=StingException.class) + public void multipleArgumentCollectionTest() { + parsingEngine.addArgumentSource( MultipleArgumentCollectionProvider.class ); + } + + private class MultipleArgumentCollectionProvider { + @ArgumentCollection + RequiredArgProvider rap1 = new RequiredArgProvider(); + @ArgumentCollection + RequiredArgProvider rap2 = new RequiredArgProvider(); + } }