-
Notifications
You must be signed in to change notification settings - Fork 587
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 741 fix indefinite freeze in split intervals task when using exome data #8113
Vs 741 fix indefinite freeze in split intervals task when using exome data #8113
Conversation
…g an explicit parameter use_interval_weights that defaults to true (the current behavior)
…g an explicit parameter use_interval_weights that defaults to true (the current behavior)
…ntervals because it tried to pass --weight-bed-file anyway, which causes SplitIntervals to fail.
…region close to the start of a genome
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## ah_var_store #8113 +/- ##
================================================
Coverage ? 86.170%
Complexity ? 35132
================================================
Files ? 2173
Lines ? 165045
Branches ? 17794
================================================
Hits ? 142220
Misses ? 16479
Partials ? 6346 |
Github actions tests reported job failures from actions build 3577955591
|
Github actions tests reported job failures from actions build 3577966142
|
src/main/java/org/broadinstitute/hellbender/tools/gvs/extract/ExtractCohortEngine.java
Outdated
Show resolved
Hide resolved
src/main/java/org/broadinstitute/hellbender/tools/gvs/extract/ExtractCohortEngine.java
Outdated
Show resolved
Hide resolved
…it against unset variable issues
…it against unset variable issues Also adding branch back to dockstore so I can run an extract one last time just to verify the changes
Github actions tests reported job failures from actions build 3578304201
|
Github actions tests reported job failures from actions build 3578374711
|
…ExtractCohortEngine.java Sure, this is more concise Co-authored-by: Miguel Covarrubias <[email protected]>
…earer, but it's fewer lines and the comment should make clear what it's doing
Github actions tests reported job failures from actions build 3584250201
|
…undefined value along I feel dirty.
Github actions tests reported job failures from actions build 3586714857
|
Github actions tests reported job failures from actions build 3587103510
|
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!
command <<< | ||
# Updating to use standard shell boilerplate | ||
PS4='\D{+%F %T} \w $ ' | ||
set -o errexit -o nounset -o pipefail -o xtrace | ||
set -e |
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.
this line can go now as it's the terse form of the set -o errexit
above
This PR fixes two bugs.
First, the SplitIntervals task would enter WeightedSplitIntervals and hang. I added an extra boolean argument to extract so you can specify that no, you really don't want to use a weighted bed. Relatedly, the code branch for running the original GATK SplitIntervals code wasn't correct, as passing weight-bed-file to it as an argument caused a failure. It uses a slightly hacky method of defining a string in WDL to be empty or not depending on if we use weighted beds, interpolating that string into the bash, then checking to see if it's empty there to transmit that state. There is likely a cleaner way to do this, and in the next revision I will likely rewrite this part cleaner.
Second, after SplitIntervals passed we hit an error during ExtractTask. The way it expanded intervals to handle large deletions could sometimes subtract past the start of a chromosome, so that logic needed to be patched in a few separate places to handle the interval for the mitochondrial dna that started much closer to the beginning (instead of having a 10k base pair buffer). This PR has those changes too.
Successful run here: https://app.terra.bio/#workspaces/gvs-dev/GVS%20Exome%20Test/job_history/a006a959-9300-42cf-84a7-38c70a35ee21
Successful run after incorporating PR changes: https://app.terra.bio/#workspaces/gvs-dev/GVS%20Exome%20Test/job_history/e2ee3abd-288e-4f1d-b5be-f78cf5400ce9
Successful run after last PR refactoring that allowed me to revert almost all changes to GvsUtils.SplitIntervals: https://app.terra.bio/#workspaces/gvs-dev/GVS%20Exome%20Test/job_history/94fed63a-98ca-466e-8d4c-ac97f24adf37