Incorporating changes from GVS to existing files #8256

lbergelson · 2023-03-22T17:13:37Z

Absorbing the changes to core GATK files from the long running GVS work.
Based on the vs_834_deletions branch where code was factored to separate the gvs
specific classes from the shared gatk code
There were fairly minimal conflicts between the two except for in the new VQSR package.
In this case we've taken the master version of those files since the GVS version is out
of date.
The dockstore file was also not included because I don't understand it and someone familiar with it will have to take a look.

lbergelson · 2023-03-22T17:16:17Z

@droazen @mcovarr
This incorporates the code from https://github.com/broadinstitute/gatk/pull/8231/files into gatk. There are two caveats that I've described above which may need input from the GVS team.

If tests are passing we should probably do a light review to make sure nothing is obviously bonkers in unintentional ways from the merge.

lbergelson · 2023-03-22T17:36:20Z

We're failing with:
stderr: write /opt/miniconda/envs/gatk/libexec/gcc/x86_64-conda-linux-gnu/7.5.0/lto1: no space left on device

mcovarr · 2023-03-22T17:53:08Z

@lbergelson we're not sure why we'd be running out of space, we'll have a look.

mcovarr · 2023-03-22T18:30:36Z

Per team discussion:

Taking the new VQSR package from master is a good thing, so yay to that. 🙂
.dockstore.yml is used for making the GVS WDLs available in Terra from Dockstore, so this is something that can / should live in a GVS-specific repo and doesn't need to be in GATK.

mcovarr · 2023-03-22T20:13:34Z

@lbergelson The ~370 MB of files added under src/test/resources/large/filteringJointVcf appear to be test data from an older version of VQSR Lite that are no longer referenced. I deleted these files and that seems to have been enough to bring disk usage back under the limit, with no tests upset by their absence.

lbergelson · 2023-03-22T20:28:21Z

@mcovarr Happy to remove these then! We're dangerously close to the vm disk limit it seems.

* Absorbing the changes to core GATK files from the long running GVS work. * Based on the vs_834_deletions branch where code was factored to separate the gvs specific classes from the shared gatk code * There were fairly minimal conflicts between the two except for in the new VQSR package. In this case we've taken the master version of those files since the GVS version is out of date. remove some large files that don't seem to be referenced

mcovarr · 2023-03-22T20:59:07Z

Yeah disk space definitely seems pretty tight in these builds right now...

I'm not sure why those files are still in our branch. I did a cleanup pass on our branch a couple of weeks ago that was intended to identify files like this; I'll see if I can figure out how they were missed.

lbergelson · 2023-03-22T21:01:23Z

Maybe they were referenced in the vqsr stuff that I took the newer version of?

mcovarr · 2023-03-22T21:43:31Z

OK so the script I wrote earlier was really about figuring out which files in ah_var_store were candidates to be moved to a new variantstore repo and not explicitly about identifying cruft in our ah_var_store branch (though I did incidentally identify some cruft in the process of writing the script). However a couple of functions from this can be repurposed to find cruft:

% comm -1 -2 <(files_deleted_from_master) <(files_added_on_ah_var_store)              
scripts/vcf_site_level_filtering_cromwell_tests/vcf_site_level_filtering_travis.json
src/test/resources/large/filteringJointVcf/test_10_samples.22.avg.vcf.gz
src/test/resources/large/filteringJointVcf/test_10_samples.22.avg.vcf.gz.tbi
src/test/resources/large/filteringJointVcf/test_10_samples.23.avg.vcf.gz
src/test/resources/large/filteringJointVcf/test_10_samples.23.avg.vcf.gz.tbi
src/test/resources/large/filteringJointVcf/test_10_samples.sites_only.vcf.gz
src/test/resources/large/filteringJointVcf/test_10_samples.sites_only.vcf.gz.tbi
%

Which are the 6 files we already knew about plus a bonus JSON. I'll make a PR against ah_var_store to clean this out and hopefully prevent anyone else from having to think about these files again. 🙂

mcovarr · 2023-03-22T23:16:22Z

Actually it turns out all of these files are still being used in the version of VQSR Lite code on ah_var_store, even the JSON file with 'Travis' in its name. So at least from the ah_var_store perspective this wasn't cruft, even if it is on master.

lbergelson · 2023-03-23T14:13:43Z

@mcovarr Interesting. Maybe they were additional test files added when you incorporated the vqsr lite changes? It was very unclear to me if there were intentional changes / additions in the variant store branch or if things were just out of date / slightly different than master. Maybe we should have someone who was involved in that take a look so we can be sure to incorporate any positive changes that were made.

mcovarr · 2023-03-23T18:03:35Z

Hi Louis, we talked this over and the Variants folks who have done more VQSR Lite than myself say this looks fine as they expected the version of VQSR Lite on master to be more up to date than what's on ah_var_store.

So basically this looks good to us. Thanks again for taking this on! 🙏

mcovarr · 2023-03-23T19:23:30Z

So I tried adding in the GVS files to this branch and found two compilation issues:

One can apparently be resolved by adding this constant.
The other appears to be related to the absence of this commit. Unfortunately I get a merge conflict when I try to cherry pick this onto my branch, and I don't understand the code well enough to resolve it.

droazen

@lbergelson Just a few minor issues / questions

Can you also comment on what happened to the changes in 95b1548 during conflict resolution?

droazen · 2023-03-23T19:31:33Z

src/main/java/org/broadinstitute/hellbender/engine/GATKTool.java

@@ -145,7 +145,7 @@ public abstract class GATKTool extends CommandLineProgram {
    /**
     * Our source of reference data (null if no reference was provided)
     */
-    ReferenceDataSource reference;
+    protected ReferenceDataSource reference;


Ideally we would keep this package-protected. It's meant to be encapsulated for walker traversals (descendants of WalkerBase), and exposed for direct descendants of GATKTool (ie., non-walkers). Non-walker tools can access this field via the inherited directlyAccessEngineReferenceDataSource() method. @mcovarr / @lbergelson , would it be a significant hardship for the GVS branch to migrate to using that method?

Would it be okay if we do the following in ExtractTool.java? Only the three subclasses of ExtractTool.java reference reference.

protected ReferenceDataSource reference = directlyAccessEngineReferenceDataSource();

@mcovarr Of course, that's totally fine! If those are the only usages in the GVS branch, then, would you be ok with @lbergelson changing this field back to package-accessible?

Sure that would be fine, thanks!

droazen · 2023-03-23T19:34:00Z

.../java/org/broadinstitute/hellbender/tools/walkers/gnarlyGenotyper/GnarlyGenotyperEngine.java

    public GnarlyGenotyperEngine(final boolean keepAllSites, final int maxAltAllelesToOutput, final boolean stripASAnnotations) {
+        this(keepAllSites, maxAltAllelesToOutput, true, stripASAnnotations);


Would be good for @ldgauthier to have a quick look at the GnarlyGenotyper changes here.

Actually on second look these GnarlyGenotyper changes are trivial (just a new emitPls flag), and so don't require extra scrutiny

droazen · 2023-03-23T19:38:08Z

src/main/java/org/broadinstitute/hellbender/utils/tsv/SimpleXSVWriter.java

@@ -200,7 +200,7 @@ public LineBuilder fill(final String filling) {
         * Constructs the line and writes it out to the output
         */
        public void write() {
-            Utils.validate(!Arrays.stream(lineToBuild).anyMatch(Objects::isNull), "Attempted to construct an incomplete line, make sure all columns are filled");
+            Utils.validate(!Arrays.stream(lineToBuild).anyMatch(Objects::isNull), "Attempted to construct an incomplete line, make sure all columns are filled: " + Arrays.toString(lineToBuild));


Perhaps this String should be lazily constructed (ie., only when the validation check fails) instead of unconditionally on every call to write()

droazen · 2023-03-23T19:39:57Z

src/test/java/org/broadinstitute/hellbender/utils/bigquery/BigQueryUtilsUnitTest.java

@@ -38,28 +36,42 @@ public void testExecuteQueryWithWhereClause() {
        expectedNamesAndAges.put("Fred", "35");

        final String query = String.format("SELECT * FROM `%s` WHERE name = 'Fred'", BIGQUERY_FULLY_QUALIFIED_TABLE);
+        Map<String, String> labels = new HashMap<String, String>();
+        labels.put("gatktestquery", "testwhereclause" + runUuid);
+        System.out.print("testwhereclause" + runUuid);


Use the logger, not System.out.print()

I think they're more commonly called lumberjacks?

mcovarr

LGTM delta David's requested changes. Looking forward to having GVS synced up with master! 😄

lbergelson · 2023-04-10T23:22:05Z

@droazen I've responded to your comments.

droazen self-requested a review March 22, 2023 17:17

droazen self-assigned this Mar 22, 2023

lbergelson force-pushed the lb_merge_gvs_conflicts branch from 19f90bc to 3bee3c1 Compare March 22, 2023 20:30

droazen suggested changes Mar 23, 2023

View reviewed changes

mcovarr approved these changes Apr 10, 2023

View reviewed changes

responding to comments

e4ca86a

droazen approved these changes Apr 11, 2023

View reviewed changes

droazen merged commit 0374937 into master Apr 11, 2023

droazen deleted the lb_merge_gvs_conflicts branch April 11, 2023 14:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Incorporating changes from GVS to existing files #8256

Incorporating changes from GVS to existing files #8256

lbergelson commented Mar 22, 2023

lbergelson commented Mar 22, 2023

lbergelson commented Mar 22, 2023

mcovarr commented Mar 22, 2023

mcovarr commented Mar 22, 2023 •

edited

Loading

mcovarr commented Mar 22, 2023

lbergelson commented Mar 22, 2023

mcovarr commented Mar 22, 2023

lbergelson commented Mar 22, 2023

mcovarr commented Mar 22, 2023

mcovarr commented Mar 22, 2023

lbergelson commented Mar 23, 2023

mcovarr commented Mar 23, 2023

mcovarr commented Mar 23, 2023

droazen left a comment •

edited

Loading

droazen Mar 23, 2023

mcovarr Mar 23, 2023

droazen Mar 23, 2023

mcovarr Mar 23, 2023

droazen Mar 23, 2023

droazen Apr 11, 2023

droazen Mar 23, 2023

lbergelson Apr 10, 2023

droazen Mar 23, 2023

lbergelson Apr 10, 2023

mcovarr left a comment

lbergelson commented Apr 10, 2023

		public GnarlyGenotyperEngine(final boolean keepAllSites, final int maxAltAllelesToOutput, final boolean stripASAnnotations) {
		this(keepAllSites, maxAltAllelesToOutput, true, stripASAnnotations);

Incorporating changes from GVS to existing files #8256

Incorporating changes from GVS to existing files #8256

Conversation

lbergelson commented Mar 22, 2023

lbergelson commented Mar 22, 2023

lbergelson commented Mar 22, 2023

mcovarr commented Mar 22, 2023

mcovarr commented Mar 22, 2023 • edited Loading

mcovarr commented Mar 22, 2023

lbergelson commented Mar 22, 2023

mcovarr commented Mar 22, 2023

lbergelson commented Mar 22, 2023

mcovarr commented Mar 22, 2023

mcovarr commented Mar 22, 2023

lbergelson commented Mar 23, 2023

mcovarr commented Mar 23, 2023

mcovarr commented Mar 23, 2023

droazen left a comment • 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

mcovarr left a comment

Choose a reason for hiding this comment

lbergelson commented Apr 10, 2023

mcovarr commented Mar 22, 2023 •

edited

Loading

droazen left a comment •

edited

Loading