removing all google genomics API dependencies #4266

lbergelson · 2018-01-25T22:44:25Z

the google genomics API has deprecated all the features we were using,
this includes the reference lookup api, and the google Read data types

removing all google genomics related dependencies

replacing com.google.cloud.genomics:gatk-tools-java:1.1 with gov.nist.math.jama:gov.nist.math.jama:1.1.1
we rely on this transitive dependency, making it a direct dependency instead
remove com.google.apis:google-api-services-genomics:v1-rev527-1.22.0
remove com.google.cloud.genomics:google-genomics-utils:v1-0.10
delete ReferenceAPISource and tests
delete GoogleGenomicsReadToGATKReadAdapter and tests
delete CigarConversionUtils and tests
update other classes to remove references to these types
improve an error message

magicDGS · 2018-01-26T10:07:19Z

src/main/java/org/broadinstitute/hellbender/engine/datasources/ReferenceMultiSource.java

@@ -49,8 +49,9 @@ public ReferenceMultiSource(final String referenceURL,
            } else {
                referenceSource = new ReferenceFileSource(referenceURL);


Completely unrelated question, but something that I realized reviewing this: is the separation of ReferenceFileSource and ReferenceHadoopSource really necessary? It looks like the java.nio.Path implementation would take care of Hadoop stuff (just for a different PR, but maybe worthy to open an issue about it)

I think it's probably redundant like you say. We should take a look at it. #4279

magicDGS

Some questions about the tests.

magicDGS · 2018-01-26T10:10:40Z

...test/java/org/broadinstitute/hellbender/engine/spark/JoinReadsWithRefBasesSparkUnitTest.java

@@ -28,7 +27,7 @@
    @DataProvider(name = "bases")
    public Object[][] bases(){
        Object[][] data = new Object[2][];
-        List<Class<?>> classes = Arrays.asList(Read.class, SAMRecord.class);
+        List<Class<?>> classes = Arrays.asList(SAMRecord.class, SAMRecord.class);


This should be a singleton list now, at least until there is no other implementation.

oh, good call, I just replaced read -> samrecord, I wasn't paying close enough attention here

opening a new ticket to simplify these if we're only going to have 1 read type #4318

magicDGS · 2018-01-26T10:10:50Z

...test/java/org/broadinstitute/hellbender/engine/spark/JoinReadsWithVariantsSparkUnitTest.java

@@ -23,7 +22,7 @@
        List<Object[]> testCases = new ArrayList<>();

        for ( JoinStrategy joinStrategy : JoinStrategy.values() ) {
-            for ( Class<?> readImplementation : Arrays.asList(Read.class, SAMRecord.class) ) {
+            for ( Class<?> readImplementation : Arrays.asList(SAMRecord.class, SAMRecord.class) ) {


magicDGS · 2018-01-26T10:13:02Z

...est/java/org/broadinstitute/hellbender/tools/spark/BaseRecalibratorSparkIntegrationTest.java

@@ -149,7 +148,7 @@ public void testBQSRSpark(BQSRTest params) throws IOException {
    public Object[][] createBQSRCloudTestData() {
        final String localResources =  getResourceDir();

-        final String GRCh37RefCloud = ReferenceAPISource.URL_PREFIX + ReferenceAPISource.GRCH37_REF_ID;
+        final String GRCh37RefCloud = GCS_b37_CHR20_21_REFERENCE;


Couldn't you remove the GRCh37RefCloud variable and use the GCS_b37_CHR20_21_REFERENCE directly?

I'm going to keep this because it matches the style of the rest of the test and on the off chance that we want to change this reference path again here.

magicDGS · 2018-01-26T10:13:10Z

...est/java/org/broadinstitute/hellbender/tools/spark/BaseRecalibratorSparkIntegrationTest.java

@@ -211,7 +209,7 @@ public void testBlowUpOnBroadcastIncompatibleReference() throws IOException {
    //This data provider is for tests that use BAM files stored in buckets
    @DataProvider(name = "BQSRTestBucket")
    public Object[][] createBQSRTestDataBucket() {
-        final String GRCh37RefCloud = ReferenceAPISource.URL_PREFIX + ReferenceAPISource.GRCH37_REF_ID;
+        final String GRCh37RefCloud = GCS_b37_CHR20_21_REFERENCE;


Same as before

same as above

magicDGS · 2018-01-26T10:13:34Z

...a/org/broadinstitute/hellbender/tools/spark/BaseRecalibratorSparkShardedIntegrationTest.java

@@ -61,8 +60,7 @@ private String getCloudInputs() {
    @DataProvider(name = "BQSRTest")
    public Object[][] createBQSRTestData() {
        final String localResources =  getResourceDir();
-        final String hg19Ref = ReferenceAPISource.HG19_REF_ID;
-        final String GRCh37Ref = ReferenceAPISource.URL_PREFIX + ReferenceAPISource.GRCH37_REF_ID;
+        final String GRCh37Ref = GCS_b37_CHR20_21_REFERENCE;


Same as before

magicDGS · 2018-01-26T10:13:41Z

...a/org/broadinstitute/hellbender/tools/spark/BaseRecalibratorSparkShardedIntegrationTest.java

@@ -90,7 +88,7 @@ private String getCloudInputs() {

    @DataProvider(name = "BQSRTestBucket")
    public Object[][] createBQSRTestDataBucket() {
-        final String GRCh37Ref = ReferenceAPISource.URL_PREFIX + ReferenceAPISource.GRCH37_REF_ID;
+        final String GRCh37Ref = GCS_b37_CHR20_21_REFERENCE;


Same as before

codecov-io · 2018-02-01T21:39:57Z

Codecov Report

Merging #4266 into master will decrease coverage by 0.062%.
The diff coverage is 33.333%.

@@               Coverage Diff               @@
##              master     #4266       +/-   ##
===============================================
- Coverage     79.088%   79.026%   -0.062%     
+ Complexity     16588     16372      -216     
===============================================
  Files           1048      1045        -3     
  Lines          59506     58982      -524     
  Branches        9718      9634       -84     
===============================================
- Hits           47062     46611      -451     
+ Misses          8666      8628       -38     
+ Partials        3778      3743       -35

Impacted Files	Coverage Δ	Complexity Δ
...institute/hellbender/exceptions/UserException.java	`71.318% <ø> (+0.207%)`	`3 <0> (ø)`	⬇️
...ute/hellbender/utils/read/ArtificialReadUtils.java	`93.478% <ø> (-0.616%)`	`67 <0> (-3)`
...lbender/utils/read/SAMRecordToGATKReadAdapter.java	`92.43% <ø> (-0.03%)`	`135 <0> (-1)`
...broadinstitute/hellbender/utils/read/GATKRead.java	`31.25% <ø> (-37.5%)`	`7 <0> (-6)`
...itute/hellbender/engine/spark/GATKRegistrator.java	`100% <ø> (ø)`	`2 <0> (ø)`	⬇️
...dinstitute/hellbender/utils/pileup/ReadPileup.java	`91.946% <ø> (ø)`	`64 <0> (ø)`	⬇️
...utils/test/ReadsPreprocessingPipelineTestData.java	`0.862% <0%> (+0.015%)`	`1 <1> (ø)`	⬇️
...ender/engine/datasources/ReferenceMultiSource.java	`61.538% <0%> (-15.385%)`	`9 <0> (-2)`
...bender/engine/datasources/ReferenceFileSource.java	`88.462% <100%> (ø)`	`9 <0> (ø)`	⬇️
...titute/hellbender/engine/spark/JsonSerializer.java	`0% <0%> (-63.636%)`	`0% <0%> (-4%)`
... and 6 more

droazen · 2018-02-12T20:23:31Z

src/test/java/org/broadinstitute/hellbender/engine/spark/AddContextDataToReadSparkUnitTest.java

@@ -33,7 +32,7 @@
 public class AddContextDataToReadSparkUnitTest extends GATKBaseTest {
    @DataProvider(name = "bases")
    public Object[][] bases() {
-        List<Class<?>> classes = Arrays.asList(Read.class, SAMRecord.class);
+        List<Class<?>> classes = Arrays.asList(SAMRecord.class, SAMRecord.class);


Specify SAMRecord only once.

droazen · 2018-02-12T20:39:45Z

src/test/java/org/broadinstitute/hellbender/utils/read/GATKReadAdaptersUnitTest.java

-        emptyCigarRead.getAlignment().setCigar(null);
-
-        return new Object[][]{
+          return new Object[][]{


Indentation is off here

droazen · 2018-02-12T20:39:53Z

src/test/java/org/broadinstitute/hellbender/utils/read/GATKReadAdaptersUnitTest.java

-        noRGGoogleRead.setReadGroupId(null);
-
-        return new Object[][] {
+         return new Object[][] {


Here as well

droazen

A few trivial remaining comments, then good to merge 👍

the google genomics API has deprecated all the features we were using, this includes the reference lookup api, and the google Read data types removing all google genomics related dependencies * replacing com.google.cloud.genomics:gatk-tools-java:1.1 with gov.nist.math.jama:gov.nist.math.jama:1.1.1 we rely on this transitive dependency, making it a direct dependency * remove com.google.apis:google-api-services-genomics:v1-rev527-1.22.0 * remove com.google.cloud.genomics:google-genomics-utils:v1-0.10 * delete ReferenceAPISource and tests * delete GoogleGenomicsReadToGATKReadAdapter and tests * delete CigarConversionUtils and tests * update other classes to remove references to these types * improve an error message

magicDGS reviewed Jan 26, 2018

View reviewed changes

droazen self-assigned this Jan 26, 2018

droazen self-requested a review January 26, 2018 13:33

lbergelson force-pushed the lb_remove_genomics_api branch from ca175e4 to 606ed63 Compare January 31, 2018 20:03

droazen reviewed Feb 12, 2018

View reviewed changes

droazen approved these changes Feb 12, 2018

View reviewed changes

droazen assigned lbergelson and unassigned droazen Feb 12, 2018

lbergelson added 3 commits February 12, 2018 16:54

responding to comments

7bb43f3

respond to further comments

1c5b36b

lbergelson force-pushed the lb_remove_genomics_api branch from 606ed63 to 1c5b36b Compare February 12, 2018 21:57

lbergelson merged commit c451470 into master Feb 12, 2018

lbergelson deleted the lb_remove_genomics_api branch February 12, 2018 23:04

lbergelson mentioned this pull request Jul 11, 2018

remove code supporting google Read and Reference API #4166

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

removing all google genomics API dependencies #4266

removing all google genomics API dependencies #4266

lbergelson commented Jan 25, 2018

magicDGS Jan 26, 2018

lbergelson Jan 26, 2018

magicDGS left a comment

magicDGS Jan 26, 2018

lbergelson Jan 31, 2018

lbergelson Jan 31, 2018

magicDGS Jan 26, 2018

lbergelson Jan 31, 2018

magicDGS Jan 26, 2018

lbergelson Jan 31, 2018

magicDGS Jan 26, 2018

lbergelson Jan 31, 2018

magicDGS Jan 26, 2018

magicDGS Jan 26, 2018

codecov-io commented Feb 1, 2018

droazen Feb 12, 2018

droazen Feb 12, 2018

droazen Feb 12, 2018

droazen left a comment

		@@ -49,8 +49,9 @@ public ReferenceMultiSource(final String referenceURL,
		} else {
		referenceSource = new ReferenceFileSource(referenceURL);

removing all google genomics API dependencies #4266

removing all google genomics API dependencies #4266

Conversation

lbergelson commented Jan 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magicDGS left a 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

codecov-io commented Feb 1, 2018

Codecov Report

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