Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Quickstart based integration test [VS-357] #7812

Merged
merged 31 commits into from
May 3, 2022

Conversation

mcovarr
Copy link
Collaborator

@mcovarr mcovarr commented Apr 26, 2022

Fully manual for now, also turns off filtering as that appears to be nondeterministic and this currently only does exact matching of actual to expected results.

@codecov
Copy link

codecov bot commented Apr 26, 2022

Codecov Report

❗ No coverage uploaded for pull request base (ah_var_store@d51a4e5). Click here to learn what that means.
The diff coverage is n/a.

@@               Coverage Diff                @@
##             ah_var_store     #7812   +/-   ##
================================================
  Coverage                ?   86.303%           
  Complexity              ?     35194           
================================================
  Files                   ?      2170           
  Lines                   ?    164837           
  Branches                ?     17774           
================================================
  Hits                    ?    142260           
  Misses                  ?     16254           
  Partials                ?      6323           

input_vcfs = input_vcfs,
input_vcf_indexes = input_vcf_indexes,
filter_set_name = "quickit",
create_filter_set_scatter_count = 20,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we still need these filter params if we are turning filtering off for now (extract_do_not_filter_override) ? (The answer may totally be yes and someplace else needs that updated)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would defer to you on that, though I was hoping filtering wouldn't be off for too long (though maybe it will, I don't know).

# but also for building nightly GATK jars. This wouldn't be that valuable for this current increment of work as
# using a Docker image would save maybe 10 minutes from what is currently a ~4 hour workflow, but a Docker image
# could become more compelling if a scaled down version of this test that could run more frequently was created,
# in addition to the nightly build use case mentioned above.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is GATK doing this for their nightlys?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I should look into that more

fi

echo "All vcfs compared and matched!"
>>>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we are ready to start doing fuzzy comparisons (with filtering), we could potentially use the ah_var_store docker image which has bcftools in it and do some easy selective comparisons that way. eg take two vcfs and compare only their variants as opposed to their bytes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bcftools would also allow us to merge all of the shards into one fairly quickly which could also speed up the comparison

Copy link
Contributor

@RoriCremer RoriCremer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is exciting!

}
}

task Prepare {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can this be something else, like "setup", just to not create any confusion with the PrepareRangesCallset step?

Copy link

@rsasch rsasch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one small nit, otherwise 👍🏻

@mcovarr mcovarr force-pushed the vs_357_quickstart_integration branch from 1b6435a to 3813472 Compare April 29, 2022 16:37
@mcovarr mcovarr merged commit 04aec08 into ah_var_store May 3, 2022
@mcovarr mcovarr deleted the vs_357_quickstart_integration branch May 3, 2022 12:32
This was referenced Mar 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants