-
Notifications
You must be signed in to change notification settings - Fork 585
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
Add reference broadcasting to BQSR #928
Conversation
(Also note this adds .2bit chr20/21 reference to LFS which is about 27 MB) |
Will do a review pass first thing Monday -- @tomwhite and/or @davidadamsphd should also have a look. |
public class BroadcastJoinReadsWithRefBases { | ||
public static JavaPairRDD<GATKRead, ReferenceBases> addBases(final ReferenceDataflowSource referenceDataflowSource, | ||
final JavaRDD<GATKRead> reads) { | ||
JavaSparkContext ctx = new JavaSparkContext(reads.context()); |
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.
It's odd that reads.context()
doesn't return a JavaSparkContext
. This is fine though since it's reusing the underlying context.
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.
I agree. Strange API choice.
I added a few fairly minor comments. Curious to see the performance implications. |
|
||
} | ||
|
||
private static void assertSameReads(final JavaRDD<GATKRead> reads, |
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.
We'd like to keep this function around to verify we're not losing reads.
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.
It's no longer being used. If anything, this should be moved into a test somewhere.
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.
Moving to test sounds good to me.
@@ -35,7 +34,13 @@ | |||
public ReferenceDataflowSource(final PipelineOptions pipelineOptions, final String referenceURL, | |||
final SerializableFunction<GATKRead, SimpleInterval> referenceWindowFunction) { | |||
Utils.nonNull(referenceWindowFunction); | |||
if (isFasta(referenceURL)) { | |||
if (referenceURL.endsWith(".2bit")) { |
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.
Declare this extension as a named constant at the top of the class.
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.
See note above about similar issue.
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.
Answered above
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.
done.
@@ -14,6 +14,7 @@ | |||
import org.broadinstitute.hellbender.engine.dataflow.datasources.ReferenceDataflowSource; | |||
import org.broadinstitute.hellbender.utils.SimpleInterval; | |||
import org.broadinstitute.hellbender.utils.read.GATKRead; | |||
import org.broadinstitute.hellbender.utils.spark.JoinStrategy; | |||
import org.broadinstitute.hellbender.utils.test.BaseTest; | |||
import org.broadinstitute.hellbender.utils.test.FakeReferenceSource; | |||
import org.broadinstitute.hellbender.utils.variant.Variant; |
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.
Add a new unit test to AddContextDataToReadSparkUnitTest
that tests the BROADCAST
case.
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.
Made the DataProvider
simply add a case for BROADCAST
strategy as well. It will probably be inefficient, but for small data sets, all the ReferenceSources
should work in a broadcast strategy as well I think.
Have a couple of test failures still -- do we understand what's causing them?
|
Review complete -- back to @laserson |
3b39dfa
to
9e1a222
Compare
Ok, @droazen, I think just a few comments for you to address, and we're almost finished. |
9e1a222
to
bf363ad
Compare
@laserson Ok, responded to the latest round of comments. Ping me when this is rebased/finalized and passing tests, and I'll take a last look and merge. |
Thanks! One issue is that the tests here will fail until the newest ADAM is released later this week :-/ I don't mind waiting if you don't. Otherwise, I could disable the tests for now. |
I vote to disable the test(s) in question in the interests of getting this merged (@akiezun feel free to chime in if you disagree) -- let's open a ticket to remind us to turn them back on when we rev ADAM, though. |
fine, disable and enter a ticket to reenable. |
bf363ad
to
9713727
Compare
Adds a new file to LFS: human_g1k_v37.20.21.2bit. It was by applying the faToTwoBit binary from the "kent" UCSC utilities package on the similarly named fasta reference already in the repo.
9713727
to
ac321f4
Compare
I anticipate this should pass, and I believe all issues have been addressed. |
👍 merging! |
Add reference broadcasting to BQSR
hallelujah. sorry that took so long |
please review any of @droazen @tomwhite @davidadamsphd