Cram implementation reports validation errors at container granularity #1091

SathemBite · 2018-02-20T10:08:21Z

Description

Associated issue: #1076

Motivation

CRAM implementation is different then BAM implementation and validates all records in container when it first opened, but BAM implementation validated each record as its retrieved
CRAM iterator contained useless memory allocation for 10000 elements of ArrayList<SAMRecord> records;

Solution

The first problem was solved by deletion of validation from cramIterator.nextContainer() method, and addition it in cramIterator.next(), for validation records when they retrieves.
And the second problem was solved by deletion of objects reservation.

Checklist

Code compiles correctly
New tests covering changes and new functionality
All tests passing
Extended the README / documentation, if necessary
Is not backward compatible (breaks binary or source compatibility)

codecov-io · 2018-02-20T11:58:34Z

Codecov Report

Merging #1091 into master will increase coverage by 0.003%.
The diff coverage is 100%.

@@              Coverage Diff               @@
##             master     #1091       +/-   ##
==============================================
+ Coverage      68.7%   68.703%   +0.003%     
- Complexity     8060      8061        +1     
==============================================
  Files           542       542               
  Lines         32728     32725        -3     
  Branches       5537      5536        -1     
==============================================
- Hits          22484     22483        -1     
  Misses         8043      8043               
+ Partials       2201      2199        -2

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/htsjdk/samtools/CRAMIterator.java	`79.72% <100%> (+0.953%)`	`33 <0> (ø)`	⬇️
...samtools/util/AsyncBlockCompressedInputStream.java	`72% <0%> (-4%)`	`12% <0%> (-1%)`
src/main/java/htsjdk/samtools/SAMRecord.java	`67.795% <0%> (+0.117%)`	`315% <0%> (+1%)`	⬆️
src/main/java/htsjdk/samtools/SAMUtils.java	`59.848% <0%> (+0.505%)`	`126% <0%> (+1%)`	⬆️

magicDGS · 2018-02-20T13:26:47Z

src/main/java/htsjdk/samtools/CRAMIterator.java

@@ -106,7 +106,7 @@ public CRAMIterator(final SeekableStream seekableStream, final CRAMReferenceSour
        this.containerIterator = containerIterator;

        firstContainerOffset = containerIterator.getFirstContainerOffset();
-        records = new ArrayList<SAMRecord>(10000);
+        records = new ArrayList<SAMRecord>();


How many records are expected in each container? This could have some performance effect, but I understand that there is no explanation for the magic number. Should it be extracted as an default constant at the class level or maybe to an argument to the iterator?

@magicDGS hello, thanks for your review, I've extracted it as constant at the class level

vadimzalunin · 2018-02-20T14:08:48Z

10k is/used to be container default size but you are right of course.

…

On Tue, 20 Feb 2018, 08:26 Daniel Gómez-Sánchez, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/main/java/htsjdk/samtools/CRAMIterator.java <#1091 (comment)>: > @@ -106,7 +106,7 @@ public CRAMIterator(final SeekableStream seekableStream, final CRAMReferenceSour this.containerIterator = containerIterator; firstContainerOffset = containerIterator.getFirstContainerOffset(); - records = new ArrayList<SAMRecord>(10000); + records = new ArrayList<SAMRecord>(); How many records are expected in each container? This could have some performance effect, but I understand that there is no explanation for the magic number. Should it be extracted as an default constant at the class level or maybe to an argument to the iterator? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1091 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAoz5FYaGogrKr0Jfg5LVsc1I4V_R8ROks5tWsgXgaJpZM4SLsja> .

…erator.next() -Added corresponded tests

cmnbroad

Did a first pass, with a few requests.

cmnbroad · 2018-02-21T15:05:55Z

src/main/java/htsjdk/samtools/CRAMIterator.java

@@ -43,6 +43,7 @@

 public class CRAMIterator implements SAMRecordIterator {
    private static final Log log = Log.getInstance(CRAMIterator.class);
+    private static final int DEFAULT_CONTAINER_CAPACITY = 10000;


There are existing constants named DEFAULT_RECORDS_PER_SLICE and DEFAULT_SLICES_PER_CONTAINER (defined in CRAMContainerStreamWriter!). We're planning to do a refactoring of this code soon to fix a lot of this (i.e., those constants and much of this code should live in the Container and Slice classes, and since its reading an existing container, the size of the list can be based on the size of the container being read rather than constant). For now, if anything, I'd say to just use the existing DEFAULT_RECORDS_PER_SLICE.

@cmnbroad hello, thanks for your review, all requested changes are done

cmnbroad · 2018-02-21T15:18:14Z

src/main/java/htsjdk/samtools/CRAMIterator.java

+     *
+     * @param samRecord - validated record
+     */
+    public void validateRecord(SAMRecord samRecord){


This doesn't need to be public, or even a separate method at all since its only called once.

cmnbroad · 2018-02-21T15:21:08Z

src/main/java/htsjdk/samtools/CRAMIterator.java

+     */
+    public void validateRecord(SAMRecord samRecord){
+        final List<SAMValidationError> validationErrors = samRecord.isValid();
+        SAMUtils.processValidationErrors(validationErrors, samRecordIndex, validationStringency);


I don't think that in this context, samRecordIndex bears any relationship to the record being processed.

cmnbroad · 2018-02-21T16:10:23Z

src/test/java/htsjdk/samtools/CRAMIteratorTest.java

+
+
+/**
+ * @author [email protected], EPAM Systems, Inc.


This is a test file, so it doesn't matter, but block comments like "author" need to go after the main javadoc or else the entire javadoc text becomes part of the author block.

cmnbroad · 2018-02-21T16:11:57Z

src/test/java/htsjdk/samtools/CRAMIteratorTest.java

+
+public class CRAMIteratorTest extends HtsjdkTest {
+    private static final File refFile = new File("src/test/resources/htsjdk/samtools/cram/ce.fa");
+    private static final File cramFile = new File("src/test/resources/htsjdk/samtools/cram/ce#supp.3.0.cram");


Presumably this file has some records that fail validation. It would be better to make a copy of it with a name that indicates that, and reinforce it by using a variable name here that reflects it.

Also, although many of the test are written this same way, it would be preferable for each test to do its own setup rather than relying on class-level state.

cmnbroad · 2018-02-21T16:12:35Z

src/test/java/htsjdk/samtools/CRAMIteratorTest.java

+public class CRAMIteratorTest extends HtsjdkTest {
+    private static final File refFile = new File("src/test/resources/htsjdk/samtools/cram/ce.fa");
+    private static final File cramFile = new File("src/test/resources/htsjdk/samtools/cram/ce#supp.3.0.cram");
+    ReferenceSource source = new ReferenceSource(refFile);


We don't publish a style guide, but for new code we try to use "final" everywhere possible (paramaters, locals, etc.).

cmnbroad · 2018-02-21T16:15:09Z

src/test/java/htsjdk/samtools/CRAMIteratorTest.java

+                getCramFileIterator(cramFile, source, ValidationStringency.STRICT);
+
+        while (cramIter.hasNext())
+        {


We don't publish a style guide, but we generally try to put opening braces on the line with the previous statement.

One other comment I forgot to make; since this test is expected to throw an exception, it would be a good idea to use a try/finally block to close the reader.

cmnbroad · 2018-02-21T16:36:31Z

src/test/java/htsjdk/samtools/CRAMIteratorTest.java

+
+    private SAMRecordIterator getCramFileIterator(File cramFile,
+                                                  ReferenceSource source,
+                                                  ValidationStringency valStrigency) {


valStrigency -> valStringency

cmnbroad · 2018-02-21T16:37:40Z

src/test/java/htsjdk/samtools/CRAMIteratorTest.java

+    }
+
+    @Test(expectedExceptions = SAMException.class)
+    public void shouldThrowExceptionIfCRAMFileContainsInvalidRecods() {


maybe rename to throwOnRecordValidationFailure.

src/test/java/htsjdk/samtools/CRAMIteratorTest.java

cmnbroad

Sorry for the delay on reviewing this. The changes are pretty nicely simplified now - just a couple of test cleanup comments.

cmnbroad · 2018-06-18T18:49:18Z

src/test/java/htsjdk/samtools/CRAMIteratorTest.java

+        final ReferenceSource source = new ReferenceSource(refFile);
+        final SAMRecordIterator cramIteratorOverInvalidRecords =
+                getCramFileIterator(cramFileWithInvalidRecs, source, ValidationStringency.STRICT);
+        try{


try-with-resources

cmnbroad · 2018-06-18T18:50:26Z

src/test/java/htsjdk/samtools/CRAMIteratorTest.java

+        final File cramFileWithInvalidRecs = new File("src/test/resources/htsjdk/samtools/cram/ce#containsInvalidRecords.3.0.cram");
+        final ReferenceSource source = new ReferenceSource(refFile);
+        final SAMRecordIterator cramIteratorOverInvalidRecords =
+                getCramFileIterator(cramFileWithInvalidRecs, source, ValidationStringency.STRICT);


Most of the code up to this point is duplicated in both test methods. Can you factor that out (maybe into getCramFileIterator).

cmnbroad · 2018-06-18T18:54:41Z

src/main/java/htsjdk/samtools/CRAMIterator.java

-        return iterator.next();
+        SAMRecord samRecord = iterator.next();
+        if (validationStringency != ValidationStringency.SILENT) {
+            SAMUtils.processValidationErrors(samRecord.isValid(), samRecordIndex++, validationStringency);


Since samRecordIndex is a class not a local variable, can you add a comment to the declaration saying that it is only used when Validation Stringency is not SILENT, and otherwise it isn't valid. (this was always true even before this PR, but it would be good to make that clear).

cmnbroad · 2018-06-18T18:56:49Z

src/test/java/htsjdk/samtools/CRAMIteratorTest.java

+public class CRAMIteratorTest extends HtsjdkTest {
+
+    @Test(description = "This test checks that records validation is deferred until they are retrieved")
+    public void notThrowOnOpeningContainerWithInvalidRecords() {


Maybe rename this to "noValidationFailureOnContainerOpen".

cmnbroad · 2018-06-18T18:58:43Z

src/test/java/htsjdk/samtools/CRAMIteratorTest.java

+                getCramFileIterator(cramFileWithInvalidRecs, source, ValidationStringency.STRICT);
+
+        Assert.assertTrue(cramIteratorOverInvalidRecords.hasNext());
+        cramIteratorOverInvalidRecords.close();


SAMRecordIterator is closable, so you should be able to use try-with-resources here.

@cmnbroad Hello! Thank you for review, requested changes are done

cmnbroad · 2018-07-26T14:24:19Z

@AntonMazur I think this is pretty close now, but it fails to compile on travis. CRAMIteratorTest.java imports a junit package, but we use testng. I suspect if you replace that import it will compile, and then I can do a final review.

cmnbroad

@AntonMazur Sorry again for the long delay on getting this branch reviewed. It looks pretty good with the last set of changes, however there is one comment thats a bit awkwardly written. Since the tests didn't run last time this was submitted, it would be good to update that, and then rebase onto current master since this branch is pretty far behind. This will rerun the CI tests to make sure everything passes, then we can get this merged. Alternatively, I'd be happy to make that last change for you if you prefer - just let me know. Thanks again for doing this.

src/main/java/htsjdk/samtools/CRAMIterator.java

cmnbroad · 2018-10-22T18:46:13Z

@AntonMazur Just pinging you one more time on this. I'll plan to update and rebase this branch if I don't hear back from you soon, since I thinks pretty close, but has conflicts that need to be resolved.

…ation_errors_at_container_granularity # Conflicts: # src/main/java/htsjdk/samtools/CRAMIterator.java

lbergelson

👍 @cmnbroad Can you make the fields we talked about final in a follow up?

SathemBite force-pushed the TASK-298-CRAM_implementation_reports_validation_errors_at_container_granularity branch 2 times, most recently from 17dd6f2 to b3e4f77 Compare February 20, 2018 11:45

magicDGS reviewed Feb 20, 2018

View reviewed changes

-Moved records validation from cramIterator.nextContainer() to cramIt…

410011b

…erator.next() -Added corresponded tests

SathemBite force-pushed the TASK-298-CRAM_implementation_reports_validation_errors_at_container_granularity branch from ea5521b to 410011b Compare February 21, 2018 15:35

cmnbroad requested changes Feb 21, 2018

View reviewed changes

Performed some refactoring

92c7ccd

yfarjoun assigned cmnbroad Mar 20, 2018

yfarjoun added the cram label Jun 4, 2018

cmnbroad requested changes Jun 18, 2018

View reviewed changes

Made requested changes

c672c65

cmnbroad requested changes Oct 2, 2018

View reviewed changes

src/main/java/htsjdk/samtools/CRAMIterator.java Show resolved Hide resolved

cmnbroad added the Waiting for revisions This PR has received comments from reviewers and is waiting for the Author to respond label Oct 22, 2018

Fixed review comment: corrected comment before samRecordIndex

cba9220

SathemBite force-pushed the TASK-298-CRAM_implementation_reports_validation_errors_at_container_granularity branch from f80ba81 to cba9220 Compare October 30, 2018 08:39

Merge branch 'master' into TASK-298-CRAM_implementation_reports_valid…

cb13b65

…ation_errors_at_container_granularity # Conflicts: # src/main/java/htsjdk/samtools/CRAMIterator.java

SathemBite force-pushed the TASK-298-CRAM_implementation_reports_validation_errors_at_container_granularity branch from 6439592 to cb13b65 Compare October 30, 2018 09:06

Corrected CRAMIteratorTest import

c77ba65

lbergelson approved these changes Dec 10, 2018

View reviewed changes

cmnbroad approved these changes Dec 10, 2018

View reviewed changes

cmnbroad merged commit 6ac7a60 into samtools:master Dec 10, 2018

cmnbroad mentioned this pull request Dec 10, 2018

Add finals for issues raised during review of PR 1091. #1240

Merged

cmnbroad mentioned this pull request Dec 20, 2018

CRAM implementation reports validation errors at container granularity #1076

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cram implementation reports validation errors at container granularity #1091

Cram implementation reports validation errors at container granularity #1091

SathemBite commented Feb 20, 2018 •

edited

Loading

codecov-io commented Feb 20, 2018 •

edited

Loading

magicDGS Feb 20, 2018

SathemBite Feb 21, 2018

vadimzalunin commented Feb 20, 2018 via email

cmnbroad left a comment

cmnbroad Feb 21, 2018

SathemBite Feb 23, 2018

cmnbroad Feb 21, 2018

cmnbroad Feb 21, 2018

cmnbroad Feb 21, 2018

cmnbroad Feb 21, 2018

cmnbroad Feb 21, 2018

cmnbroad Feb 21, 2018

cmnbroad Feb 21, 2018

cmnbroad Feb 21, 2018

cmnbroad Feb 21, 2018

cmnbroad Feb 21, 2018

cmnbroad left a comment

cmnbroad Jun 18, 2018

cmnbroad Jun 18, 2018

cmnbroad Jun 18, 2018

cmnbroad Jun 18, 2018

cmnbroad Jun 18, 2018

SathemBite Jun 24, 2018

cmnbroad commented Jul 26, 2018

cmnbroad left a comment

cmnbroad commented Oct 22, 2018 •

edited

Loading

lbergelson left a comment

Cram implementation reports validation errors at container granularity #1091

Cram implementation reports validation errors at container granularity #1091

Conversation

SathemBite commented Feb 20, 2018 • edited Loading

Description

Motivation

Solution

Checklist

codecov-io commented Feb 20, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadimzalunin commented Feb 20, 2018 via email

cmnbroad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmnbroad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmnbroad commented Jul 26, 2018

cmnbroad left a comment

Choose a reason for hiding this comment

cmnbroad commented Oct 22, 2018 • edited Loading

lbergelson left a comment

Choose a reason for hiding this comment

SathemBite commented Feb 20, 2018 •

edited

Loading

codecov-io commented Feb 20, 2018 •

edited

Loading

cmnbroad commented Oct 22, 2018 •

edited

Loading