Add registry for deprecated tools and annotations. #4505

cmnbroad · 2018-03-07T16:22:37Z

We should be able to use this single implementation to satisfy both #4347 and #4351.

Questions: @vdauwera What other tools that were dropped from GATK3 should be added to the list now? GATK3 also has a deprecated annotations list, which is included here, but is empty. Are there any annotations that should be listed ? I can't really implement/test that part unless I populate it with something.

Also, trying to run a missing tool is currently handled as an error, and surfaces in the context of a usage message. Perhaps that hides it too much:

codecov-io · 2018-03-07T17:14:19Z

Codecov Report

Merging #4505 into master will decrease coverage by 0.229%.
The diff coverage is 87.5%.

@@               Coverage Diff               @@
##              master     #4505       +/-   ##
===============================================
- Coverage     79.119%   78.891%   -0.229%     
- Complexity     16677     16775       +98     
===============================================
  Files           1051      1051               
  Lines          60285     60916      +631     
  Branches        9875     10060      +185     
===============================================
+ Hits           47697     48057      +360     
- Misses          8760      9014      +254     
- Partials        3828      3845       +17

Impacted Files	Coverage Δ	Complexity Δ
...stitute/hellbender/cmdline/CommandLineProgram.java	`82.432% <ø> (ø)`	`43 <0> (ø)`	⬇️
.../main/java/org/broadinstitute/hellbender/Main.java	`72.637% <100%> (+0.698%)`	`48 <3> (+3)`	⬆️
...ellbender/cmdline/DeprecatedArtifactsRegistry.java	`81.818% <81.818%> (ø)`	`3 <3> (?)`
...ignment/AssemblyContigWithFineTunedAlignments.java	`53.595% <0%> (-1.961%)`	`34% <0%> (+11%)`
...der/engine/spark/datasources/ReadsSparkSource.java	`80.198% <0%> (-1.853%)`	`38% <0%> (-6%)`
...der/tools/spark/sv/discovery/SvDiscoveryUtils.java	`7.143% <0%> (-1.19%)`	`3% <0%> (+1%)`
...r/tools/walkers/mutect/Mutect2FilteringEngine.java	`81.6% <0%> (-1.159%)`	`36% <0%> (-4%)`
.../walkers/mutect/GermlineProbabilityCalculator.java	`89.286% <0%> (-1.037%)`	`9% <0%> (-3%)`
...lbender/utils/variant/GATKVariantContextUtils.java	`83.52% <0%> (-0.8%)`	`215% <0%> (-1%)`
...ark/sv/discovery/inference/CpxVariantDetector.java	`1.835% <0%> (-0.769%)`	`2% <0%> (ø)`
... and 29 more

magicDGS · 2018-03-07T17:14:15Z

src/main/java/org/broadinstitute/hellbender/Main.java

@@ -480,6 +481,10 @@ public static String getUnknownCommandMessage(final Set<Class<?>> classes, final
        int bestDistance = Integer.MAX_VALUE;
        int bestN = 0;

+        final String deprecationMessage = DeprecatedArtifactsRegistry.getToolDeprecationInfo(command);


Please, make this configurable outside the getUnknownCommandMessage to override by downstream toolkits (if someone is trying to run IndelRealignment on my toolkit, I would like to throw an error saying that it does not exists but not that it is not included in GATK4 but I would like still to have the code for searching for similar commands...

magicDGS · 2018-03-07T17:20:29Z

src/main/java/org/broadinstitute/hellbender/cmdline/DeprecatedArtifactsRegistry.java

+    static {
+        // Indicate version in which the tool disappeared, and recommended replacement in parentheses if applicable
+        deprecatedTools.put("IndelRealigner", new Tuple2<>("4.0.0.0", "Please use GATK3 to run this tool"));
+        deprecatedTools.put("RealignerTargetCreator", new Tuple2<>("4.0.0.0", "Please use GATK3 to run this tool"));


If these tools are going to be included after I implement them, saying that they are deprecated might be a strong statement: maybe a distinction between "not supported yet (but probably future support)" and "deprecated" is a better idea. Otherwise, if they are included again after version x.x.x.x, this message in previous version will be weird.

An idea might be to include a placeholder command line programs that just throw an "unsupported in the current version" for that kind of tools and annotated with @ExperimentalFeature, for example. Anyway, I hope to have IndelRealignment sooner than later...

I think these messages should reflect the current state, rather than speculate about the future. Hopefully we can get these tools ported; at that point we can remove them from this list.

Fair enough.

magicDGS · 2018-03-07T17:22:35Z

src/main/java/org/broadinstitute/hellbender/cmdline/DeprecatedArtifactsRegistry.java

+     *
+     * @param toolName   the tool class name (not the full package) to check
+     */
+    public static String getToolDeprecationInfo(final String toolName) {


Maybe instead of handle this here, a method to throw a new UserException in main if it is in the deprecation registry may be more informative and will handle common message....

magicDGS · 2018-03-07T17:24:31Z

src/main/java/org/broadinstitute/hellbender/cmdline/DeprecatedArtifactsRegistry.java

+ * be added to this list to issue a message when the user tries to run that tool.
+ *
+ * NOTE: Picard tools should be listed here as well, since by definition such tools will not be found in
+ * the Picard jar.


Maybe a registry in Picard that can add on runtime values will be helpful. The same for dowsntream tools...

Unfortunately we'll probably have to duplicate this in picard as well. If and when picard has such a thing we can consider integrating it here. for downstream toolkits, getUnknownCommand is overridable now, which I think will allow you the flexibility to do whatever integration you want.

droazen · 2018-03-12T19:52:39Z

src/main/java/org/broadinstitute/hellbender/cmdline/DeprecatedArtifactsRegistry.java

+
+    // Mapping from tool name to string describing the major version number where the tool first disappeared and
+    // optional recommended alternatives
+    private static Map<String, Tuple2<String, String>> deprecatedTools = new HashMap<>();


Use org.apache.commons.lang3.tuple.Pair instead of Scala Tuple2

droazen · 2018-03-12T19:55:04Z

src/main/java/org/broadinstitute/hellbender/cmdline/DeprecatedArtifactsRegistry.java

+        deprecatedTools.put("RealignerTargetCreator", new Tuple2<>("4.0.0.0", "Please use GATK3 to run this tool"));
+    }
+
+    // Mapping from walker name to major version number where the walker first disappeared and optional replacement options


walker -> annotation

Removed completely.

droazen · 2018-03-12T19:56:01Z

src/main/java/org/broadinstitute/hellbender/cmdline/DeprecatedArtifactsRegistry.java

+    }
+
+    // Mapping from walker name to major version number where the walker first disappeared and optional replacement options
+    private static Map<String, String> deprecatedAnnotations = new HashMap<>();


Remove deprecatedAnnotations for now until there's a formal request for this feature.

droazen · 2018-03-12T19:56:39Z

src/main/java/org/broadinstitute/hellbender/cmdline/DeprecatedArtifactsRegistry.java

+     */
+    public static String getToolDeprecationInfo(final String toolName) {
+        return deprecatedTools.containsKey(toolName) ?
+                String.format("%s is no longer included in GATK as of version %s. (%s)",


(%s) -> %s

droazen · 2018-03-12T19:57:51Z

src/test/java/org/broadinstitute/hellbender/cmdline/DeprecatedArtifactsRegistryTest.java

+                () -> new Main().instanceMain( new String[] {missingTool} )
+        );
+        Assert.assertTrue(e.getMessage().contains(DeprecatedArtifactsRegistry.getToolDeprecationInfo(missingTool)));
+    }


Add a second test for a tool that doesn't exist and is not in the registry.

droazen · 2018-03-12T19:58:10Z

src/main/java/org/broadinstitute/hellbender/cmdline/DeprecatedArtifactsRegistry.java

+ * NOTE: Picard tools should be listed here as well, since by definition such tools will not be found in
+ * the Picard jar.
+ */
+public class DeprecatedArtifactsRegistry {


MissingToolsRegistry ?

droazen

Review complete, back to @cmnbroad. Merge after addressing comments.

cmnbroad · 2018-03-13T13:58:51Z

src/main/java/org/broadinstitute/hellbender/cmdline/DeprecatedToolsRegistry.java

+    static {
+        // Indicate version in which the tool disappeared, and recommended replacement in parentheses if applicable
+        deprecatedTools.put("IndelRealigner", Pair.of("4.0.0.0", "Please use GATK3 to run this tool"));
+        deprecatedTools.put("RealignerTargetCreator", Pair.of("4.0.0.0", "Please use GATK3 to run this tool"));


@vdauwera Anything else that you want on this list before I merge this PR ?

magicDGS reviewed Mar 7, 2018

View reviewed changes

droazen requested a review from vdauwera March 7, 2018 18:44

droazen reviewed Mar 12, 2018

View reviewed changes

droazen suggested changes Mar 12, 2018

View reviewed changes

droazen assigned cmnbroad Mar 12, 2018

cmnbroad force-pushed the cn_tool_registry branch from 675b4aa to d789364 Compare March 13, 2018 13:57

cmnbroad commented Mar 13, 2018

View reviewed changes

Add registry for deprecated tools.

9bb5e39

cmnbroad force-pushed the cn_tool_registry branch from d789364 to 9bb5e39 Compare March 13, 2018 18:44

cmnbroad merged commit 5127367 into master Mar 20, 2018

cmnbroad deleted the cn_tool_registry branch March 20, 2018 18:55

cmnbroad mentioned this pull request Mar 20, 2018

Implement registry of deprecated tools #4347

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add registry for deprecated tools and annotations. #4505

Add registry for deprecated tools and annotations. #4505

cmnbroad commented Mar 7, 2018 •

edited

Loading

codecov-io commented Mar 7, 2018 •

edited

Loading

magicDGS Mar 7, 2018

cmnbroad Mar 8, 2018

magicDGS Mar 9, 2018

magicDGS Mar 7, 2018

cmnbroad Mar 8, 2018

magicDGS Mar 9, 2018

magicDGS Mar 7, 2018

magicDGS Mar 7, 2018

cmnbroad Mar 8, 2018 •

edited

Loading

droazen Mar 12, 2018

cmnbroad Mar 13, 2018

droazen Mar 12, 2018

cmnbroad Mar 13, 2018

droazen Mar 12, 2018

cmnbroad Mar 13, 2018

droazen Mar 12, 2018

cmnbroad Mar 13, 2018

droazen Mar 12, 2018

cmnbroad Mar 13, 2018

droazen Mar 12, 2018

cmnbroad Mar 13, 2018

droazen left a comment

cmnbroad Mar 13, 2018

Add registry for deprecated tools and annotations. #4505

Add registry for deprecated tools and annotations. #4505

Conversation

cmnbroad commented Mar 7, 2018 • edited Loading

codecov-io commented Mar 7, 2018 • edited Loading

Codecov Report

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

cmnbroad Mar 8, 2018 • 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

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

droazen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmnbroad commented Mar 7, 2018 •

edited

Loading

codecov-io commented Mar 7, 2018 •

edited

Loading

cmnbroad Mar 8, 2018 •

edited

Loading