GVS walker to master [VS-964] #8355

mcovarr · 2023-06-08T22:52:35Z

Ready for review!

gatk-bot · 2023-06-28T20:56:06Z

Github actions tests reported job failures from actions build 5405388563
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
conda	17.0.6+10	5405388563.3	logs

…elds [VS-860]

* Fix a bug where there were NO variants in a range.

KevinCLydon

This looks just about good to me. I left a couple comments, mostly questions and some very minor changes. Beyond those, I have another question and a couple more suggestions:

Codecov claims a lot of this code doesn't have tests. Am I correct to assume that is because it mostly relies on BigQuery and is therefore prohibitively difficult to write tests for?
The code is mostly stylistically good, but there are a lot of places where variables, especially method parameters, that should be final are not, so it would be good to do a quick pass and update those if possible.
Most of this code could benefit from more documentation. Some of it is very well documented, but some of the files don't seem to have much at all. It would be good to at least have doc comments for the public classes and any public attributes and methods that are not immediately self-explanatory by their names.

Thank you for doing the work to get this merged into master.

KevinCLydon · 2023-07-07T17:48:46Z

src/main/java/org/broadinstitute/hellbender/tools/gvs/common/ExtractTool.java

+    @Argument(
+            fullName = "dataset-id",
+            doc = "ID of the Google Cloud dataset to use when executing queries",
+            optional = true // I guess, but won't it break otherwise or require that a dataset be created with the name temp_tables?


Should this be optional or required?

KevinCLydon · 2023-07-07T17:50:50Z

src/main/java/org/broadinstitute/hellbender/tools/gvs/common/ExtractTool.java

+    @Argument(
+            fullName = "sample-file",
+            doc = "Alternative to `sample-table`. Pass in a (sample_id,sample_name) CSV that describes the full list of samples to extract. No header",
+            optional = true,
+            mutex={"sample-table"}
+
+    )


Suggested change

@Argument(

fullName = "sample-file",

doc = "Alternative to `sample-table`. Pass in a (sample_id,sample_name) CSV that describes the full list of samples to extract. No header",

optional = true,

mutex={"sample-table"}

)

@Argument(

fullName = "sample-file",

doc = "Alternative to `sample-table`. Pass in a (sample_id,sample_name) CSV that describes the full list of samples to extract. No header",

optional = true,

mutex={"sample-table"}

)

KevinCLydon · 2023-07-07T17:51:59Z

src/main/java/org/broadinstitute/hellbender/tools/gvs/common/ExtractTool.java

+    @Argument(
+            fullName = "print-debug-information",
+            doc = "If true, print extra debugging output",
+            optional = true)


Suggested change

@Argument(

fullName = "print-debug-information",

doc = "If true, print extra debugging output",

optional = true)

@Argument(

fullName = "print-debug-information",

doc = "If true, print extra debugging output",

optional = true

)

KevinCLydon · 2023-07-07T18:03:38Z

src/main/java/org/broadinstitute/hellbender/tools/gvs/common/ExtractTool.java

+        //TODO verify what we really need here
+        annotationEngine = new VariantAnnotatorEngine(makeVariantAnnotations(), null, Collections.emptyList(), false, false);


What do we really need here?

KevinCLydon · 2023-07-07T18:19:51Z

src/main/java/org/broadinstitute/hellbender/tools/gvs/common/FilterSensitivityTools.java

+        // We want to find the tranche with the smallest target_truth_sensitivity that is
+        // equal to or greater than our truthSensitivityThreshold.
+        // e.g. if truthSensitivitySNPThreshold is 99.8 and we have tranches with target_truth_sensitivities
+        // of 99.5, 99.7, 99.9, and 100.0, we want the 99.9 sensitivity tranche.


Would it make sense to move this to a javadoc comment for this method?

KevinCLydon · 2023-07-11T15:11:58Z

src/main/java/org/broadinstitute/hellbender/tools/gvs/extract/ExtractCohort.java

+    @Argument(
+            fullName = "vet-avro-file-name",
+            doc = "Path to data from Vet table in Avro format",
+            optional = true
+    )
+    private GATKPath vetAvroFileName = null;
+
+    @Argument(
+            fullName = "ref-ranges-avro-file-name",
+            doc = "Path to data from Vet table in Avro format",
+            optional = true
+    )
+    private GATKPath refRangesAvroFileName = null;


These have the same description so is one of them inaccurate?

KevinCLydon · 2023-07-11T15:27:11Z

src/main/java/org/broadinstitute/hellbender/tools/gvs/extract/ExtractCohort.java

+    /**
+     * Enforce that if cost information is being recorded to the cost-observability-tablename then *all* recorded
+     * parameters are set
+     */


This comment seems to not fully describe what this method does.

KevinCLydon · 2023-07-11T15:31:50Z

src/main/java/org/broadinstitute/hellbender/tools/gvs/extract/ExtractCohort.java

+        // if there is an avro file, the BQ specific parameters are unnecessary,
+        // but they all are required if there is no avro file
+        // KCIBUL: revisit!!!
+//        if ((cohortAvroFileName == null && vetAvroFileName == null && refRangesAvroFileName == null) && (projectID == null || (cohortTable == null && vetRangesFQDataSet == null))) {
+//            throw new UserException("Project id (--project-id) and cohort table (--cohort-extract-table) are required " +
+//                "if no avro file (--cohort-avro-file-name or --vet-avro-file-name and --ref-ranges-avro-file-name) is provided.");
+//        }


Should this be removed?

KevinCLydon · 2023-07-11T17:40:29Z

src/main/java/org/broadinstitute/hellbender/tools/gvs/extract/ExtractCohortRecord.java

+    // COHORT_FIELDS
+//    public static final ImmutableSet<String> COHORT_FIELDS = ImmutableSet.of(
+//            SchemaUtils.LOCATION_FIELD_NAME,
+//            SchemaUtils.SAMPLE_ID_FIELD_NAME,
+//            SchemaUtils.STATE_FIELD_NAME,
+//            SchemaUtils.REF_ALLELE_FIELD_NAME,
+//            SchemaUtils.ALT_ALLELE_FIELD_NAME,
+//            SchemaUtils.CALL_GT,
+//            SchemaUtils.CALL_GQ,
+//            SchemaUtils.CALL_RGQ,
+//            SchemaUtils.QUALapprox,
+//            SchemaUtils.AS_QUALapprox,
+//            SchemaUtils.CALL_PL);//, AS_VarDP);


Does this need to be here?

KevinCLydon · 2023-07-11T19:22:52Z

src/main/java/org/broadinstitute/hellbender/tools/gvs/extract/ExtractCohortToVcf.java

+            fullName = StandardArgumentDefinitions.OUTPUT_LONG_NAME,
+            doc = "Output VCF file to which annotated variants should be written."
+    )
+    protected String outputVcfPathString = null;


The preferred type for file arguments in GATK is GATKPath, so can this be switched to that?

koncheto-broad

LGTM. I am not overwhelmingly happy with needing to drop this many files for our extract process into GATK in order to make this happen, but for now we're going to move these files over and we can consider separating them back out again later when we move to another repo and have things nice and cleanly carved up. This little island of GVS in core GATK should likely be moved back home then

KevinCLydon · 2023-07-13T18:34:06Z

LGTM. I am not overwhelmingly happy with needing to drop this many files for our extract process into GATK in order to make this happen, but for now we're going to move these files over and we can consider separating them back out again later when we move to another repo and have things nice and cleanly carved up. This little island of GVS in core GATK should likely be moved back home then

@mcovarr @koncheto-broad Do all these files exist as they are here in the ah_var_store branch currently?

mcovarr · 2023-07-13T18:57:45Z

@mcovarr @koncheto-broad Do all these files exist as they are here in the ah_var_store branch currently?

~~Yes, delta the refactoring required to separate the VCF writing into a subclass.~~
Yes!

KevinCLydon · 2023-07-13T19:07:38Z

@mcovarr @koncheto-broad Do all these files exist as they are here in the ah_var_store branch currently?

Yes, delta the refactoring required to separate the VCF writing into a subclass.

Would it be easier/preferable for you all to move those changes into the var store branch and I'll work on the PGEN stuff there instead of in master?

mcovarr · 2023-07-13T19:31:43Z

Hi @KevinCLydon, yes actually all of this is on ah_var_store already per an earlier PR. Sorry my earlier comment was incorrect, this was > 1 month ago and I forgot 😅

droazen · 2023-07-13T20:16:39Z

@mcovarr @koncheto-broad Had a talk today with @KevinCLydon about this PR, and he made a convincing case that the pgen project is more likely to require additional stuff from the GVS branch than from gatk/master. To avoid a nightmare scenario of constantly having to request additional transplants from the GVS branch, I'm on board with the idea of having Kevin try to work off of the GVS branch for the pgen project for now, provided that we can get timely rebases of ah_var_store onto master if we need them.

mcovarr · 2023-07-14T18:19:07Z

Thanks @droazen, I'll go ahead and close this PR. The equivalent GVS walker code is already on ah_var_store, but the syncing up of ah_var_store to master is still on the TODO list.

mcovarr force-pushed the vs_964_gvs_walker_to_master branch from 6678efe to 87a2a12 Compare June 28, 2023 20:07

mcovarr force-pushed the vs_964_gvs_walker_to_master branch from 87a2a12 to ec0ccd8 Compare June 29, 2023 17:37

mcovarr and others added 11 commits July 5, 2023 16:39

copy over required files from ah_var_store

739c91c

attempt to resolve ReferenceConfidenceVariantContextMerger conflicts

f9899a2

Fix reference reference

0cc3cb0

Annotation adjustments for ExtractTool

f770c36

Call new mergeWithRemapping method

22da04b

Adapt for Avro 1.11 behavior of throwing on get()s of non-existent fi…

e7b7aba

…elds [VS-860]

non clp vcf extract tests

af6c5b0

clp vcf extract test

bd39e8e

fix imports

013b11b

VS-942. Fix an error where NO variants were found (#8388)

dab2446

* Fix a bug where there were NO variants in a range.

BigQuery storage v1beta2 to v1 [VS-1001] (#8399)

fa74fc7

mcovarr force-pushed the vs_964_gvs_walker_to_master branch from ec0ccd8 to fa74fc7 Compare July 5, 2023 20:51

mcovarr changed the title ~~[DRAFT] GVS walker to master [VS-964]~~ GVS walker to master [VS-964] Jul 6, 2023

mcovarr marked this pull request as ready for review July 6, 2023 14:29

droazen requested a review from KevinCLydon July 7, 2023 15:50

droazen assigned KevinCLydon Jul 7, 2023

KevinCLydon requested changes Jul 11, 2023

View reviewed changes

koncheto-broad approved these changes Jul 13, 2023

View reviewed changes

mcovarr closed this Jul 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GVS walker to master [VS-964] #8355

GVS walker to master [VS-964] #8355

mcovarr commented Jun 8, 2023 •

edited

Loading

gatk-bot commented Jun 28, 2023

KevinCLydon left a comment

KevinCLydon Jul 7, 2023

KevinCLydon Jul 7, 2023

KevinCLydon Jul 7, 2023

KevinCLydon Jul 7, 2023

KevinCLydon Jul 7, 2023

KevinCLydon Jul 11, 2023

KevinCLydon Jul 11, 2023

KevinCLydon Jul 11, 2023

KevinCLydon Jul 11, 2023

KevinCLydon Jul 11, 2023

koncheto-broad left a comment

KevinCLydon commented Jul 13, 2023

mcovarr commented Jul 13, 2023 •

edited

Loading

KevinCLydon commented Jul 13, 2023

mcovarr commented Jul 13, 2023

droazen commented Jul 13, 2023

mcovarr commented Jul 14, 2023

		//TODO verify what we really need here
		annotationEngine = new VariantAnnotatorEngine(makeVariantAnnotations(), null, Collections.emptyList(), false, false);

GVS walker to master [VS-964] #8355

GVS walker to master [VS-964] #8355

Conversation

mcovarr commented Jun 8, 2023 • edited Loading

gatk-bot commented Jun 28, 2023

KevinCLydon 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

koncheto-broad left a comment

Choose a reason for hiding this comment

KevinCLydon commented Jul 13, 2023

mcovarr commented Jul 13, 2023 • edited Loading

KevinCLydon commented Jul 13, 2023

mcovarr commented Jul 13, 2023

droazen commented Jul 13, 2023

mcovarr commented Jul 14, 2023

mcovarr commented Jun 8, 2023 •

edited

Loading

mcovarr commented Jul 13, 2023 •

edited

Loading