Avro test #7192

RoriCremer · 2021-04-09T06:38:12Z

No description provided.

gatk-bot · 2021-04-09T07:15:28Z

Travis reported job failures from build 33622
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
unit	openjdk11	33622.13	logs
unit	openjdk8	33622.3	logs

src/main/java/org/broadinstitute/hellbender/tools/variantdb/nextgen/ExtractCohort.java

mmorgantaylor · 2021-04-09T14:29:30Z

.../broadinstitute/hellbender/tools/variantdb/nextgen/AnVIL_WGS_100_exported_cohort/sample_list

@@ -0,0 +1,100 @@
+30,HG00561


probably could use just a few samples, and use a subsetted interval list, so that the test files can be small. there's a small interval list in the CreateVariantIngestFiles integration test resources, if that helps!

Yes please! Just take a few ( < 5 ) samples and just a few sites (100s)

mmorgantaylor · 2021-04-09T14:37:29Z

...test/java/org/broadinstitute/hellbender/tools/variantdb/nextgen/ExtractCohortEngineTest.java

+public class ExtractCohortEngineTest extends CommandLineProgramTest {
+
+  private static final String cohortAvroFileName = // is it okay to just grab one?
+      "src/test/java/org/broadinstitute/hellbender/tools/variantdb/nextgen/AnVIL_WGS_100_exported_cohort/000000000000";


can use getToolTestDataDir() to get the path

gatk-bot · 2021-04-12T03:12:39Z

Travis reported job failures from build 33661
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
unit	openjdk11	33661.13	logs
unit	openjdk8	33661.3	logs

gatk-bot · 2021-04-12T20:04:28Z

Travis reported job failures from build 33681
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
unit	openjdk11	33681.13	logs
unit	openjdk8	33681.3	logs

gatk-bot · 2021-04-14T16:22:43Z

Travis reported job failures from build 33742
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
unit	openjdk11	33742.13	logs
unit	openjdk8	33742.3	logs

gatk-bot · 2021-04-14T22:07:56Z

Travis reported job failures from build 33764
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
unit	openjdk11	33764.13	logs
integration	openjdk11	33764.12	logs

kcibul · 2021-04-16T14:54:32Z

One concern I have is the maintainability of the test (having been burned by this in other places myself). When we add a new output field, etc we need a very easy way to update/generate these results. At the very least some instructions would be helpful (and imagine someone to follow those as part of a PR)

mmorgantaylor

Looks good! Some small comments, and I think we shouldn't have a 23 MB test file, but other than that 👍

mmorgantaylor · 2021-04-16T14:53:53Z

src/main/java/org/broadinstitute/hellbender/tools/variantdb/nextgen/ExtractCohort.java

+        // if there is a avro file, the BQ specific parameters are unnecessary,
+        // but they all are required if there is no avro file
+        if (cohortAvroFileName == null && (projectID == null || cohortTable == null)) {
+            throw new UserException("a project id and cohort table are required if no avro file is provided");


super small, but for ease of the user, could you provide the argument flags corresponding to project id and cohort table? e.g. Project id (--project-id) and cohort table (--cohort-extract-table) are required if no avro file is provided.

mmorgantaylor · 2021-04-16T14:55:08Z

src/test/java/org/broadinstitute/hellbender/tools/variantdb/nextgen/ExtractCohortTest.java

+
+class ExtractCohortTest extends CommandLineProgramTest {
+  private final String prefix = getToolTestDataDir();
+  private final String cohortAvroFileName = prefix +"000000000000.avro";


this file is 22.8 MB - can we make it smaller?

mmorgantaylor · 2021-04-16T14:56:32Z

src/main/java/org/broadinstitute/hellbender/tools/variantdb/nextgen/ExtractCohort.java

    )
    private String cohortTable = null;

    @Argument(
            fullName = "cohort-avro-file-name",
            doc = "Path of the cohort avro file",
+            mutex={"cohort-extract-table"},


i'm so sorry to be this person but can you put spaces before and after the = on all these mutex sets so it's consistent?

src/main/java/org/broadinstitute/hellbender/tools/variantdb/nextgen/ExtractCohort.java

mmorgantaylor · 2021-04-20T21:20:11Z

src/main/java/org/broadinstitute/hellbender/tools/variantdb/nextgen/ExtractCohort.java

+        // but they all are required if there is no avro file
+        if (cohortAvroFileName == null && (projectID == null || cohortTable == null)) {
+            throw new UserException("Project id (--project-id) " +
+                "and cohort table (--cohort-extract-table) are required if no avro file is provided.");


this is awesome -- could you also add a similar check for for the sample-file, sample-table parameters? I think it's the same logic (if you don't give a sample file, you need both a sample-table and a project-id)

kcibul

Looks great Rori! Just make sure that after removing output.vcf from your commit/PR that the test still pass.

kcibul · 2021-04-21T00:59:05Z

src/main/java/org/broadinstitute/hellbender/tools/variantdb/nextgen/ExtractCohort.java

+        // but they all are required if there is no avro file
+        if (cohortAvroFileName == null && (projectID == null || cohortTable == null)) {
+            throw new UserException("Project id (--project-id) " +
+                "and cohort table (--cohort-extract-table) are required if no avro file is provided.");


this is awesome -- could you also add a similar check for for the sample-file, sample-table parameters? I think it's the same logic (if you don't give a sample file, you need both a sample-table and a project-id)

kcibul · 2021-04-21T01:00:24Z

src/test/java/org/broadinstitute/hellbender/tools/variantdb/nextgen/ExtractCohortTest.java

+
+  @Test
+  public void testFinalVCFfromAvro() throws Exception {
+    // To create the expected output file--create a temp table in BQ with the folowing query


kcibul · 2021-04-21T01:01:20Z

...st/resources/org/broadinstitute/hellbender/tools/variantdb/nextgen/ExtractCohort/sample_list

@@ -0,0 +1,3 @@
+2,HG00408


nit: can this be sorted?

kcibul · 2021-04-21T01:03:50Z

src/test/java/org/broadinstitute/hellbender/tools/variantdb/nextgen/ExtractCohortTest.java

+    runCommandLine(args);
+
+    final File expectedVCF = getTestFile("expected.vcf");
+    final File outputVCF = getTestFile("output.vcf");


Is this going to get the output.vcf that you checked in as part of this commit? It should be the output of the run above (which I think won't live in the same place as getTestFile(). You can tell for sure by removing src/test/resources/org/broadinstitute/hellbender/tools/variantdb/nextgen/ExtractCohort/output.vcf from your PR (which I think shouldn't be commited anyway).

…t has run

mmorgantaylor · 2021-04-21T18:07:34Z

src/test/java/org/broadinstitute/hellbender/tools/variantdb/nextgen/ExtractCohortTest.java

+        .add("sample-file", sampleFile);
+
+    runCommandLine(args);
+    IntegrationTestSpec.assertEqualTextFiles(outputVCF, expectedVCF);


do you also check the index file?

this feels like it would be outside the scope of the test no? a GATK issue?

yeah, totally - i just wasn't sure why you were checking in the expected index file in that case

mmorgantaylor reviewed Apr 9, 2021

View reviewed changes

src/main/java/org/broadinstitute/hellbender/tools/variantdb/nextgen/ExtractCohort.java Show resolved Hide resolved

mmorgantaylor reviewed Apr 9, 2021

View reviewed changes

RoriCremer added 6 commits April 12, 2021 15:22

initial outline of test

1657e86

add static test files

3729c17

does this need a safety gate?

4ebc4ef

update test

1b57820

cleaner path

cad465e

add mutually exclusive tags

988930a

RoriCremer force-pushed the rc-avro-test branch from 897682f to 988930a Compare April 12, 2021 19:24

RoriCremer added 4 commits April 14, 2021 11:12

rename test

8380d6f

clean up for nulls

0f94751

test fixup

403bb2d

move test files

f000045

RoriCremer added 2 commits April 14, 2021 17:27

update test

a56b86e

clean up test files

f1cbee4

RoriCremer marked this pull request as ready for review April 14, 2021 21:28

make projectid optional, but add validation

b462aa4

mmorgantaylor approved these changes Apr 16, 2021

View reviewed changes

RoriCremer added 3 commits April 16, 2021 11:14

cleanup

6f0b9db

use a smaller test file

ecf5efe

typo

98faacd

mmorgantaylor reviewed Apr 20, 2021

View reviewed changes

kcibul approved these changes Apr 21, 2021

View reviewed changes

RoriCremer added 4 commits April 21, 2021 12:22

remove output files

6a9eed8

add testing for sample-file, sample-table parameters-

669985f

sort sample list

bd909dc

rework the output file so it wont be stored permanently after the tes…

3c0e6ae

…t has run

mmorgantaylor reviewed Apr 21, 2021

View reviewed changes

RoriCremer merged commit d3affb1 into ah_var_store Apr 22, 2021

RoriCremer deleted the rc-avro-test branch April 22, 2021 04:15

This was referenced Mar 17, 2023

lb merge gvs branch #8248

Closed

testing something, please ignore #8251

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avro test #7192

Avro test #7192

RoriCremer commented Apr 9, 2021

gatk-bot commented Apr 9, 2021 •

edited

Loading

mmorgantaylor Apr 9, 2021

kcibul Apr 16, 2021

mmorgantaylor Apr 9, 2021

gatk-bot commented Apr 12, 2021 •

edited

Loading

gatk-bot commented Apr 12, 2021 •

edited

Loading

gatk-bot commented Apr 14, 2021 •

edited

Loading

gatk-bot commented Apr 14, 2021 •

edited

Loading

kcibul commented Apr 16, 2021

mmorgantaylor left a comment

mmorgantaylor Apr 16, 2021

mmorgantaylor Apr 16, 2021

mmorgantaylor Apr 16, 2021

mmorgantaylor Apr 20, 2021

kcibul Apr 21, 2021

kcibul left a comment

kcibul Apr 21, 2021

kcibul Apr 21, 2021

kcibul Apr 21, 2021

kcibul Apr 21, 2021

mmorgantaylor Apr 21, 2021

RoriCremer Apr 22, 2021

mmorgantaylor Apr 22, 2021

Avro test #7192

Avro test #7192

Conversation

RoriCremer commented Apr 9, 2021

gatk-bot commented Apr 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gatk-bot commented Apr 12, 2021 • edited Loading

gatk-bot commented Apr 12, 2021 • edited Loading

gatk-bot commented Apr 14, 2021 • edited Loading

gatk-bot commented Apr 14, 2021 • edited Loading

kcibul commented Apr 16, 2021

mmorgantaylor 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

kcibul 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

gatk-bot commented Apr 9, 2021 •

edited

Loading

gatk-bot commented Apr 12, 2021 •

edited

Loading

gatk-bot commented Apr 12, 2021 •

edited

Loading

gatk-bot commented Apr 14, 2021 •

edited

Loading

gatk-bot commented Apr 14, 2021 •

edited

Loading