Update some methods to accept Path input #1681

takutosato · 2023-08-16T11:37:51Z

Description

This PR makes changes needed for supporting cloud input/output files in some Picard tools. (see broadinstitute/picard#1804)

Things to think about before submitting:

Make sure your changes compile and new tests pass locally.
Add new tests or update existing ones:
- A bug fix should include a test that previously would have failed and passes now.
- New features should come with new tests that exercise and validate the new functionality.
Extended the README / documentation, if necessary
Check your code style.
Write a clear commit title and message
- The commit message should describe what changed and is targeted at htsjdk developers
- Breaking changes should be mentioned in the commit message.

lbergelson

@takutosato Looks good overall. I found a few issues that need to be resolved before we can merge it. (including a bug I seem to have added years ago an no one has noticed I guess).

I'm also not really sure we should deprecate the file methods. In general we've just been keeping them along side the existing ones. Maybe it's time to start flipping them to deprecated, maybe people will complain. I'm unsure...

lbergelson · 2023-08-18T19:12:00Z

src/main/java/htsjdk/samtools/BamFileIoUtils.java

+     */
+    @Deprecated
+    public static void reheaderBamFile(final SAMFileHeader samFileHeader, final File inputFile, final File outputFile, final boolean createMd5, final boolean createIndex) {
+        reheaderBamFile(samFileHeader, inputFile.toPath(), outputFile.toPath(), createMd5, createIndex);


Use IOUtil.toPath() instead of file.toPath() in these cases because it propagates the null in the case of a null input instead of NPE.

lbergelson · 2023-08-18T19:20:27Z

src/main/java/htsjdk/samtools/BamFileIoUtils.java

            final long skipLast = ((term == BlockCompressedInputStream.FileTermination.HAS_TERMINATOR_BLOCK) && skipTerminator) ?
                    BlockCompressedStreamConstants.EMPTY_GZIP_BLOCK.length : 0;
            final long bytesToWrite = length - skipLast - currentPos;

-            IOUtil.transferByStream(in, outputStream, bytesToWrite);
+            IOUtil.transferByStream(in, outputStream, bytesToWrite); // tsato: this method is manually buffered so make sure BufferedReader/Writer is not used (or does that matter?)


Double buffering is a pretty minor issue in most cases. If it wasn't dealing with it before I'm not worried about it here.

Sounds good

lbergelson · 2023-08-18T19:29:33Z

src/main/java/htsjdk/samtools/BamFileIoUtils.java

        IOUtil.assertFileIsReadable(inputFile);
-        IOUtil.assertFileIsWritable(outputFile);
+        // IOUtil.assertFileIsWritable(outputFile); // tsato: what do I do with this...


Good question.. We could presumably adapt that too take a path but maybe that's complicated. There is an existing method assertDirectoryIsWriteable that you could run on the parent of the outputPath (with a null check added to make sure output file isn't null...)

I'm really not a huge fan of these preemptive checks. The good thing is that they produce sane error messages. The bad thing is that they can potentially fail in misleading ways (i.e. transient internet events, weird implementation of directories by google. ), and they have non-negligible performance costs in some cases.

That's a long way to say that it's probably fine to drop it, although I might add a null check.

I see. Added the null check. Another possibility (which I started to adopt in Picard) is to check writability if the Path refers to a local file e.g.

ValidationUtils.nonNull(inputFile); ValidationUtils.nonNull(outputFile); IOUtil.assertFileIsReadable(inputFile); if (outputFile.toUri().getScheme().equals("file")){ IOUtil.assertFileIsWritable(outputFile.toFile()); }

Is this worth doing?

I like that solution. Lets do that for now but wrap it into a utility method so you don't have to keep rewriting that logic.

Sounds good. Done

lbergelson · 2023-08-18T19:30:27Z

src/main/java/htsjdk/samtools/BamFileIoUtils.java

-            final long length = inputFile.length();
+            final long currentPos = in.getCount(); // tsato: assuming currentPos is the position after writing the first block, this should be ok.
+            // final long length = inputFile.length();
+            final long length = Files.size(inputFile); // tsato: rename to size
            final long skipLast = ((term == BlockCompressedInputStream.FileTermination.HAS_TERMINATOR_BLOCK) && skipTerminator) ?


I think length is still perfectly clear.

lbergelson · 2023-08-18T19:48:17Z

src/main/java/htsjdk/samtools/BamFileIoUtils.java

-                final long vOffsetOfFirstRecord = SAMUtils.findVirtualOffsetOfFirstRecordInBam(inputFile);
-                final BlockCompressedInputStream blockIn = new BlockCompressedInputStream(inputFile);
-                blockIn.seek(vOffsetOfFirstRecord);
+            if (skipHeader) { // tsato: this method assumes bam file...should I test cram?


Don't worry about cram here, this method only works for bam files.

lbergelson · 2023-08-18T20:57:25Z

src/test/java/htsjdk/samtools/BamFileIoUtilsUnitTest.java

+
+    @Test
+    public void testReheaderBamFile(){
+        final File originalBam = new File(this.NA12878_8000);


we usually don't use this when referring to a static variable

lbergelson · 2023-08-18T21:00:47Z

src/test/java/htsjdk/samtools/BamFileIoUtilsUnitTest.java

+            // Confirm that the reads are the same as the original
+            Assert.assertTrue(compareBamReads(originalBam, output, DEFAULT_NUM_RECORDS_TO_COMPARE));
+        } catch (IOException e){
+            throw new HtsjdkException("Could not create a temporary output file.", e);


You can just add throws IOException to the test method. It's not clear that this error message will be correct, there are a bunch of IO operations going on during this method so it could be more confusing than helpful.

lbergelson · 2023-08-18T21:02:03Z

src/test/java/htsjdk/samtools/BamFileIoUtilsUnitTest.java

+    public void testBlockCopyBamFile(final boolean skipHeader, final boolean skipTerminator) throws IOException {
+        System.out.println(skipHeader + ", " + skipTerminator);
+        final File output = File.createTempFile("output", ".bam");
+        final OutputStream out = Files.newOutputStream(output.toPath());


try-with-resourcces

lbergelson · 2023-08-18T21:02:08Z

src/test/java/htsjdk/samtools/BamFileIoUtilsUnitTest.java

+        return true;
+    }
+
+    // tsato: kinda ugly?


eh, seems fine

lbergelson · 2023-08-18T21:15:59Z

src/test/java/htsjdk/samtools/BamFileIoUtilsUnitTest.java

+     *
+     * @return true if the first (numRecordsToCompare) reads are equal, else false
+     */
+    private boolean compareBamReads(final File bam1, final File bam2, final int numRecordsToCompare){


This method is subtly wrong. If either iterarator is too short it will fail with an exception instead of returning false. Instead of reimplementing this, you should just use Assert.assertEquals() which includes an overload for comparing two iterables.

It might make sense to keep this method and have it handle opening/closing the SamReaders but renaming it to assertBamRecordsEqual and use the existing comparators.

takutosato · 2023-10-10T11:55:44Z

@lbergelson could you take a look at my revised code? Thanks!

lbergelson

looks google to me!

lbergelson · 2023-10-11T21:25:47Z

src/main/java/htsjdk/samtools/BamFileIoUtils.java

+                    }
+
+                    final long pos = BlockCompressedFilePointerUtil.getBlockAddress(blockIn.getFilePointer());
+                    blockIn.close(); // tsato: why doesn't IntelliJ say this is unnecessary?


You could leave out that close. I don't know why intellij isn't telling you that but it looks like it's redundant to me.

takutosato added 12 commits April 18, 2023 18:28

first draft

0f4f2d5

fix compile errors

72c8b17

file to path

9b7a031

just make a commit

f10b32f

another

a46c9d8

for better versioning

3ad8a4b

test looks good

1c8f57a

one more test

215019c

test done

d0158f3

some investigating bits of code included

a7eaf52

looks good

db71294

ready to open a pr

583e0f3

lbergelson requested changes Aug 18, 2023

View reviewed changes

takutosato added 2 commits August 29, 2023 16:07

louis

4b92e77

cleaning up

1f3dcbc

takutosato force-pushed the ts_nio branch from bd6e979 to 1f3dcbc Compare August 29, 2023 20:09

takutosato mentioned this pull request Oct 10, 2023

Enable use of cloud reference files broadinstitute/picard#1804

Merged

lbergelson approved these changes Oct 11, 2023

View reviewed changes

lbergelson merged commit 6d1a796 into master Oct 11, 2023
4 checks passed

lbergelson deleted the ts_nio branch October 11, 2023 21:27

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update some methods to accept Path input #1681

Update some methods to accept Path input #1681

takutosato commented Aug 16, 2023

lbergelson left a comment

lbergelson Aug 18, 2023

takutosato Aug 19, 2023

lbergelson Aug 18, 2023

takutosato Aug 19, 2023

lbergelson Aug 18, 2023

takutosato Aug 19, 2023

lbergelson Aug 23, 2023

takutosato Aug 23, 2023

lbergelson Aug 18, 2023

lbergelson Aug 18, 2023

lbergelson Aug 18, 2023

takutosato Aug 21, 2023

lbergelson Aug 18, 2023

takutosato Aug 21, 2023

lbergelson Aug 18, 2023

takutosato Aug 21, 2023

lbergelson Aug 18, 2023

lbergelson Aug 18, 2023

takutosato commented Oct 10, 2023

lbergelson left a comment

lbergelson Oct 11, 2023

Update some methods to accept Path input #1681

Update some methods to accept Path input #1681

Conversation

takutosato commented Aug 16, 2023

Description

Things to think about before submitting:

lbergelson 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

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

takutosato commented Oct 10, 2023

lbergelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment