-
Notifications
You must be signed in to change notification settings - Fork 596
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
Vs 1416 modify ingest to correctly handle ploidy differences in dragen 3 7 8 samples #8994
Vs 1416 modify ingest to correctly handle ploidy differences in dragen 3 7 8 samples #8994
Conversation
…andle-ploidy-differences-in-dragen-3-7-8-samples
Github actions tests reported job failures from actions build 11256533054
|
src/main/java/org/broadinstitute/hellbender/tools/gvs/common/PloidyUtils.java
Outdated
Show resolved
Hide resolved
src/main/java/org/broadinstitute/hellbender/tools/gvs/common/PloidyUtils.java
Outdated
Show resolved
Hide resolved
src/main/java/org/broadinstitute/hellbender/tools/gvs/common/PloidyUtils.java
Outdated
Show resolved
Hide resolved
src/main/java/org/broadinstitute/hellbender/tools/gvs/common/PloidyUtils.java
Outdated
Show resolved
Hide resolved
src/main/java/org/broadinstitute/hellbender/tools/gvs/common/PloidyUtils.java
Outdated
Show resolved
Hide resolved
src/main/java/org/broadinstitute/hellbender/tools/gvs/common/PloidyUtils.java
Show resolved
Hide resolved
src/main/java/org/broadinstitute/hellbender/tools/gvs/extract/ExtractCohortEngine.java
Outdated
Show resolved
Hide resolved
src/main/java/org/broadinstitute/hellbender/tools/gvs/ingest/CreateVariantIngestFiles.java
Outdated
Show resolved
Hide resolved
src/main/java/org/broadinstitute/hellbender/tools/gvs/ingest/SamplePloidyCreator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/broadinstitute/hellbender/tools/gvs/ingest/SamplePloidyCreator.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thumbs up, conditional on:
- Miguel's comments are addressed.
- Passing integration test (ideally on ALL chromosomes).
…andle-ploidy-differences-in-dragen-3-7-8-samples
Github actions tests reported job failures from actions build 11502878660
|
Github actions tests reported job failures from actions build 11506462963
|
Github actions tests reported job failures from actions build 11506500757
|
Github actions tests reported job failures from actions build 11519677920
|
…iscovered as part of ticket
Github actions tests reported job failures from actions build 11580320242
|
CreateVariantIngestFilesIntegrationTest test is failing now. |
…andle-ploidy-differences-in-dragen-3-7-8-samples
Github actions tests reported job failures from actions build 11619790404
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor nit only 👍
src/main/java/org/broadinstitute/hellbender/tools/gvs/ingest/SamplePloidyCreator.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually it looks like there are legit failing tests
…xisting tests that go through the tsv pathway don't break
Yup, it looks like a hunk of the integration tests are operating in TSV mode (which we don't officially support any longer... but I suppose they can stay). So in order to make those pass, I had to put some things behind an explicit check for BQ being set as the output type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Three major changes here.
Successful run against tiny sample set "PLOIDY_TEST" in echo callset project:
https://app.terra.bio/#workspaces/allofus-drc-wgs-dev/GVS%20AoU%20WGS%20Echo%20Callset%20v2/job_history/a93aa2ef-9cef-451d-8cf8-b31f1c6a8407
You'll need your aou credentials to see the results.
Successful integration run on XY:
https://app.terra.bio/#workspaces/gvs-dev/GVS%20Integration/job_history/6a9a5fdf-ffaa-4dcb-af73-56a4b25e69a4
This run shows all of the OTHER integration tests running successfully except BGE, due to the test data needing an updates for BGE X and Y:
https://app.terra.bio/#workspaces/gvs-dev/GVS%20Integration/job_history/21664810-7516-49f2-a60c-51b2e05faf06
The only difference between those two tests running was an update to the expected values for integration tests
Successful integration run after pr changes:
https://app.terra.bio/#workspaces/gvs-dev/GVS%20Integration/job_history/ec823390-dab1-4211-a1a0-ab0485615970