UpdateVcfSequenceDictionary handles stdout #741

ronlevine · 2017-01-31T23:01:26Z

Description

Implements the handling of stdout in #507.
If stdout is the output file, set to write to a stream instead of a file.

Checklist (never delete this)

Never delete this, it is our record that procedure was followed. If you find that for whatever reason one of the checklist points doesn't apply to your PR, you can leave it unchecked but please add an explanation below.

Content

Added or modified tests to cover changes and any new functionality
Edited the README / documentation (if applicable)
All tests passing on Travis

Review

Final thumbs-up from reviewer
Rebase, squash and reword as applicable

For more detailed guidelines, see https://github.com/broadinstitute/picard/wiki/Guidelines-for-pull-requests

ronlevine · 2017-01-31T23:02:36Z

@yfarjoun Please review.

coveralls · 2017-01-31T23:09:53Z

Coverage increased (+0.004%) to 73.512% when pulling bb3f99f on rhl_uvsd_stdin_stdout_507 into 2105a1e on master.

nh13 · 2017-01-31T23:12:00Z

src/main/java/picard/vcf/UpdateVcfSequenceDictionary.java

@@ -59,7 +59,7 @@
     @Option(shortName = StandardOptionDefinitions.INPUT_SHORT_NAME, doc = "Input VCF")
    public File INPUT;

-    @Option(shortName = StandardOptionDefinitions.OUTPUT_SHORT_NAME, doc = "Output VCF to be written.")
+    @Option(shortName = StandardOptionDefinitions.OUTPUT_SHORT_NAME, doc = "Output VCF to be written to a file or stdout")


Not needed.

Reverted the change.

nh13 · 2017-01-31T23:22:01Z

src/main/java/picard/vcf/UpdateVcfSequenceDictionary.java

@@ -89,7 +91,15 @@ protected int doWork() {
        if (CREATE_INDEX)
            builder.setOption(Options.INDEX_ON_THE_FLY);

-        final VariantContextWriter vcfWriter = builder.setOutputFile(OUTPUT).build();
+        // handle stdout
+        if ( OUTPUT.getPath().equals(STD_OUT) ) {


Stdout would be specified with O=/dev/stdout. How about:
builder.setOutputStream(new FileOutputStream(OUTPUT, false));
instead of the if/else?

That's cleaner but how can the stdout be redirected to a file for validation by a test? With System.out, it's easy by using System.setOut(PrintStream out).

@yfarjoun I like the code change suggestion but it would it would be tough to validate the output. I can bypass that check in my test.Thoughts?

I don't think you need the if statement here at all, as @nh13 said. please remove it and wrap OUTPUT with a FileOutputStream (and remove the STD_OUT constant)

nh13 · 2017-01-31T23:22:29Z

src/test/java/picard/vcf/UpdateVcfSequenceDictionaryTest.java


 /**
 * @author George Grant
 */
 public class UpdateVcfSequenceDictionaryTest {
    private static final File TEST_DATA_PATH = new File("testdata/picard/vcf/");
    private static final File OUTPUT_DATA_PATH = IOUtil.createTempDir("UpdateVcfSequenceDictionaryTest", null);
+    private static final File STD_OUT_FILE = new File(OUTPUT_DATA_PATH, "stdout.vcf");


I am not so sure about these tests, @yfarjoun

The intent is to redirect stdout into a file, so it could be verified. Do you have a better idea for verification?

Sorry, I meant to say I didn't review the tests, sorry for the confusion.

I think you should be calling System.setOut(PrintStream out) from the test and not modify the code in UpdateVcfSequenceDictionary.

so the test would be something like

File STD_OUT_FILE = new File(OUTPUT_DATA_PATH, "stdout.vcf"); Stream stdout = new OutputStream(STD_OUT_FILE); System.setOut(stdout); //run test //close stream if needed //examine file.

coveralls · 2017-02-03T16:29:17Z

Coverage increased (+0.004%) to 73.512% when pulling 8418079 on rhl_uvsd_stdin_stdout_507 into 2105a1e on master.

coveralls · 2017-02-03T19:44:54Z

Coverage increased (+0.004%) to 73.512% when pulling 9ef506c on rhl_uvsd_stdin_stdout_507 into 2105a1e on master.

coveralls · 2017-02-03T20:13:31Z

Coverage increased (+0.0006%) to 73.508% when pulling f005494 on rhl_uvsd_stdin_stdout_507 into 2105a1e on master.

coveralls · 2017-02-03T20:18:57Z

Coverage increased (+0.0006%) to 73.508% when pulling f005494 on rhl_uvsd_stdin_stdout_507 into 2105a1e on master.

ronlevine · 2017-02-03T20:22:57Z

@nh13 @yfarjoun I cleaned up the test and removed the "if" statement so that the OUTPUT is a stream. Removing that logic makes the code more concise and flexible. Note that setOutputFile is just and adapter, it converts the file to a stream. Let know if you have any more comments.

yfarjoun · 2017-02-03T21:09:34Z

something isn't right.

The value of OUTPUT has to be the string "/dev/stdout"

Then you should be setting the output stream to point to a file using System.setOut()

ronlevine · 2017-02-03T23:37:58Z

@yfarjoun How about this? OUTPUT=/dev/stdout.

coveralls · 2017-02-03T23:43:29Z

Coverage increased (+0.01%) to 73.517% when pulling ef13c61 on rhl_uvsd_stdin_stdout_507 into 2105a1e on master.

yfarjoun · 2017-02-04T06:33:54Z

src/test/java/picard/vcf/UpdateVcfSequenceDictionaryTest.java

        outputFile.deleteOnExit();

+        // Stream standard output a file
+        if ( outputFileName.equals(UpdateVcfSequenceDictionary.STD_OUT) ) {


use a local variable for STD_OUT.

yfarjoun · 2017-02-04T06:34:39Z

src/test/java/picard/vcf/UpdateVcfSequenceDictionaryTest.java

        final UpdateVcfSequenceDictionary updateVcfSequenceDictionary = new UpdateVcfSequenceDictionary();
        updateVcfSequenceDictionary.INPUT = input;
        updateVcfSequenceDictionary.SEQUENCE_DICTIONARY = samSequenceDictionaryVcf;
        updateVcfSequenceDictionary.OUTPUT = outputFile;

        Assert.assertEquals(updateVcfSequenceDictionary.instanceMain(new String[0]), 0);

+        if ( outputFileName.equals(UpdateVcfSequenceDictionary.STD_OUT) ) {


a bit fewer spaces

if (outputFileName.equals(UpdateVcfSequenceDictionary.STD_OUT)) {

yfarjoun · 2017-02-04T06:34:53Z

src/test/java/picard/vcf/UpdateVcfSequenceDictionaryTest.java

        outputFile.deleteOnExit();

+        // Stream standard output a file
+        if ( outputFileName.equals(UpdateVcfSequenceDictionary.STD_OUT) ) {


same here (fewer spaces)

yfarjoun · 2017-02-04T06:36:50Z

back to you @ronlevine

yfarjoun · 2017-02-05T01:49:22Z

@ronlevine I have uploaded a branch with hat I would imagine would be a "clean" solution to the following branch (sorry about the ) in the branch name...typo) : https://github.com/broadinstitute/picard/tree/yf)uvsd_streams

coveralls · 2017-02-05T19:24:28Z

Coverage decreased (-0.03%) to 73.475% when pulling 8a7b965 on rhl_uvsd_stdin_stdout_507 into 2105a1e on master.

ronlevine · 2017-02-05T20:09:27Z

@yfarjoun Please take a look at my changes. I took a look at your branch and incorporated the file stream redirection by file descriptor for stdout. Thanks, that solved the file validation problem. Note that only setOutputStream is needed and the if statement can be removed. I also found that if the test was run with STD_OUT_NAME as the first or only outputFileName, it would fail while writing the header:

htsjdk.samtools.util.RuntimeIOException: Couldn't write file java.io.FileOutputStream@7d907bac

	at htsjdk.variant.variantcontext.writer.VCFWriter.writeHeader(VCFWriter.java:139)

because
final FileOutputStream stream = new FileOutputStream(STD_OUT_FILE) goes out of scope and is destroyed. So, I moved it's declaration outside the if statement.

yfarjoun

a few small comments and we'll be good to go.

yfarjoun · 2017-02-12T03:12:31Z

src/test/java/picard/vcf/UpdateVcfSequenceDictionaryTest.java

        outputFile.deleteOnExit();

        final UpdateVcfSequenceDictionary updateVcfSequenceDictionary = new UpdateVcfSequenceDictionary();
+
+        FileOutputStream stream = null;  // placed outside of if statement so it's not destroyed before executing updateVcfSequenceDictionary


can be final. (just set to null in the else branch)

yfarjoun · 2017-02-12T03:13:00Z

src/test/java/picard/vcf/UpdateVcfSequenceDictionaryTest.java

+
+            stream = new FileOutputStream(STD_OUT_FILE);
+
+            // Ugliness required to write to m a stream given as a string on the commandline.


is that m in my original comment? in any case, it's a typo, please fix.

Nope, just a case of my fat fingers when changing the read from in your original comment to write to.

ronlevine · 2017-02-13T16:31:02Z

Squashed and rebased. Let me know if it's OK to merge.

coveralls · 2017-02-13T16:39:22Z

Coverage decreased (-0.02%) to 73.462% when pulling f196cb1 on rhl_uvsd_stdin_stdout_507 into bce0086 on master.

coveralls · 2017-02-13T16:40:37Z

Coverage decreased (-0.02%) to 73.462% when pulling f196cb1 on rhl_uvsd_stdin_stdout_507 into bce0086 on master.

ronlevine · 2017-02-15T16:11:30Z

@yfarjoun OK to merge?

yfarjoun · 2017-02-15T17:39:30Z

yes. I'm fine with it. 👍 from me.

In the future, please refrain from squashing your commits until the merge (you can squash during the final merge itself), so that your reviewers can have an easier time viewing the latest changes you made.

ronlevine self-assigned this Jan 31, 2017

ronlevine requested a review from yfarjoun January 31, 2017 23:01

nh13 requested changes Jan 31, 2017

View reviewed changes

yfarjoun reviewed Feb 4, 2017

View reviewed changes

yfarjoun suggested changes Feb 12, 2017

View reviewed changes

UpdateVcfSequenceDictionary handles stdout

f196cb1

ronlevine force-pushed the rhl_uvsd_stdin_stdout_507 branch from 4163292 to f196cb1 Compare February 13, 2017 16:28

ronlevine merged commit b58a552 into master Feb 15, 2017

ronlevine deleted the rhl_uvsd_stdin_stdout_507 branch February 15, 2017 17:40

nh13 mentioned this pull request Jul 10, 2017

UpdateVcfSequenceDictionary cannot write index #863

Closed

ronlevine mentioned this pull request Jul 12, 2017

Revert UpdateVcfSequenceDictionary handles stdout logic and repair test - DO NOT MERGE #867

Merged

5 tasks


		stream = new FileOutputStream(STD_OUT_FILE);

		// Ugliness required to write to m a stream given as a string on the commandline.

UpdateVcfSequenceDictionary handles stdout #741

UpdateVcfSequenceDictionary handles stdout #741

Conversation

ronlevine commented Jan 31, 2017

Description

Checklist (never delete this)

Content

Review

ronlevine commented Jan 31, 2017

coveralls commented Jan 31, 2017

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

coveralls commented Feb 3, 2017

coveralls commented Feb 3, 2017

coveralls commented Feb 3, 2017

coveralls commented Feb 3, 2017

ronlevine commented Feb 3, 2017

yfarjoun commented Feb 3, 2017

ronlevine commented Feb 3, 2017

coveralls commented Feb 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yfarjoun commented Feb 4, 2017

yfarjoun commented Feb 5, 2017

coveralls commented Feb 5, 2017

ronlevine commented Feb 5, 2017 • edited Loading

yfarjoun 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

ronlevine Feb 13, 2017 • edited Loading

Choose a reason for hiding this comment

ronlevine commented Feb 13, 2017 • edited Loading

coveralls commented Feb 13, 2017

coveralls commented Feb 13, 2017

ronlevine commented Feb 15, 2017

yfarjoun commented Feb 15, 2017

ronlevine commented Feb 5, 2017 •

edited

Loading

ronlevine Feb 13, 2017 •

edited

Loading

ronlevine commented Feb 13, 2017 •

edited

Loading