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

updating to newer htsjdk snapshot #3588

Merged
merged 1 commit into from
Sep 25, 2017
Merged

Conversation

lbergelson
Copy link
Member

updating to a new htsjdk snapshot and updating our VariantContextWriters to compile

test may fail on this....

Copy link
Contributor

@droazen droazen left a 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

@lbergelson
Copy link
Member Author

@jean-philippe-martin We're seeing the same errors here as in yours pr. So it's something in the htsjdk update...

@cmnbroad
Copy link
Collaborator

cmnbroad commented Sep 19, 2017

@lbergelson @jean-philippe-martin I looked into this it a bit - it can be readily reproduced if the failing test is run through gradle, but not from IntelliJ, which is because when it runs through gradle, its using asyncIO for BAM writing, but when run from IntelliJ its using sync io.

I'm not sure why this issue is showing up now, but in the cases that fail, the async writer thread is being interrupted by this code which is called from the main thread when the PrintReads closeTool method closes the writer (I added a trace statement to verify this). When I run in IntelliJ and force async io to be used, the interrupt call still happens, but there is no test failure.

@cmnbroad
Copy link
Collaborator

cmnbroad commented Sep 19, 2017

One short term option would be to turn off async writing until we figure out the right fix.

@lbergelson
Copy link
Member Author

The regression was introduced in samtools/htsjdk@1ade6c9

@codecov-io
Copy link

Codecov Report

Merging #3588 into master will increase coverage by 0.242%.
The diff coverage is 100%.

@@              Coverage Diff               @@
##              master    #3588       +/-   ##
==============================================
+ Coverage     79.738%   79.98%   +0.242%     
- Complexity     18153    18896      +743     
==============================================
  Files           1221     1222        +1     
  Lines          66623    68248     +1625     
  Branches       10411    10916      +505     
==============================================
+ Hits           53124    54585     +1461     
- Misses          9290     9367       +77     
- Partials        4209     4296       +87
Impacted Files Coverage Δ Complexity Δ
...e/hellbender/utils/variant/writers/GVCFWriter.java 93.258% <100%> (+0.155%) 33 <1> (+1) ⬆️
...ute/hellbender/tools/spark/pathseq/PSTreeNode.java 87.736% <0%> (-0.576%) 38% <0%> (+20%)
...ools/spark/pathseq/PSFilterArgumentCollection.java 79.487% <0%> (-0.513%) 2% <0%> (ø)
...nder/tools/spark/pathseq/PSPathogenTaxonScore.java 78.125% <0%> (-0.057%) 22% <0%> (+3%)
...ecaller/AssemblyBasedCallerArgumentCollection.java 100% <0%> (ø) 2% <0%> (+1%) ⬆️
...tools/spark/pathseq/PSScoreArgumentCollection.java 100% <0%> (ø) 1% <0%> (ø) ⬇️
...r/tools/spark/pathseq/HostAlignmentReadFilter.java 100% <0%> (ø) 17% <0%> (+11%) ⬆️
...umber/allelic/alleliccount/AllelicCountWriter.java 100% <0%> (ø) 2% <0%> (ø) ⬇️
...ender/utils/smithwaterman/SWPairwiseAlignment.java 86.1% <0%> (ø) 65% <0%> (?)
.../copynumber/allelic/alleliccount/AllelicCount.java 65.854% <0%> (+0.336%) 19% <0%> (+8%) ⬆️
... and 15 more

@lbergelson
Copy link
Member Author

the htsjdk regression was fixed in samtools/htsjdk#992

@lbergelson lbergelson merged commit f6cc6fe into master Sep 25, 2017
@lbergelson lbergelson deleted the lb_update_htsjdk_version branch September 25, 2017 17:22
@lbergelson
Copy link
Member Author

@jean-philippe-martin I've updated the htsjdk snapshot, you should be unblocked after a rebase.

@jean-philippe-martin
Copy link
Contributor

Thank you very much @lbergelson !

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.

5 participants