Skip to content

Commit

Permalink
Code review comments.
Browse files Browse the repository at this point in the history
  • Loading branch information
cmnbroad committed May 22, 2018
1 parent 270f70c commit 1a33c3f
Show file tree
Hide file tree
Showing 7 changed files with 50 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import java.util.concurrent.TimeUnit;
import java.util.stream.StreamSupport;

import static org.broadinstitute.hellbender.utils.io.Resource.LARGE_RUNTIME_RESOURCES_PATH;


/**
Expand Down Expand Up @@ -176,8 +177,8 @@ public class CNNScoreVariants extends VariantWalker {

private String scoreKey;

private static String resourcePathReadTensor = "large" + File.separator + "cnn_score_variants" + File.separator + "small_2d.json";
private static String resourcePathReferenceTensor = "large" + File.separator + "cnn_score_variants" + File.separator + "1d_cnn_mix_train_full_bn.json";
private static String resourcePathReadTensor = LARGE_RUNTIME_RESOURCES_PATH + "/cnn_score_variants/small_2d.json";
private static String resourcePathReferenceTensor = LARGE_RUNTIME_RESOURCES_PATH + "/cnn_score_variants/1d_cnn_mix_train_full_bn.json";

@Override
protected String[] customCommandLineValidation() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ public boolean exec() {
if (!this.libraries.isEmpty()) {
List<String> tempLibraryPaths = new ArrayList<>();
for (RScriptLibrary library: this.libraries) {
final File tempLibrary = library.writeLibrary(tempLibSourceDir);
final File tempLibrary = library.writeLibraryToTempFile(tempLibSourceDir);
tempLibraryPaths.add(tempLibrary.getAbsolutePath());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,13 @@ public File writeTemp() {
}

/**
* Retrieve the resource for this library and write it to a File in {@code targetDir}. The File is not
* automatically scheduled for deletion and must be cleaned up by the caller.
* Retrieve the resource for this library and write it to a temp File in {@code targetDir}. The File is
* automatically scheduled for deletion on exit.
* @param targetDir target directory where the File should be written
* @return the newly created File containing the library
* @return the newly created temporary File containing the library
*/
public File writeLibrary(final File targetDir) {
final File libraryFile = new File(targetDir, getLibraryName());
public File writeLibraryToTempFile(final File targetDir) {
final File libraryFile = IOUtils.createTempFileInDirectory(getLibraryName(), R_LIBRARY_SUFFIX, targetDir);
// Note that the temporary filename generated by this ends with the resource path suffix containing
// embedded digits: ".tar.dddddddddd.gz".
IOUtils.writeResource(new Resource(getResourcePath(), RScriptLibrary.class), libraryFile);
Expand Down
16 changes: 14 additions & 2 deletions src/main/java/org/broadinstitute/hellbender/utils/io/IOUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -402,13 +402,26 @@ public static PrintStream makePrintStreamMaybeGzipped(File file) throws IOExcept
* @return A file in the temporary directory starting with name, ending with extension, which will be deleted after the program exits.
*/
public static File createTempFile(String name, String extension) {
return createTempFileInDirectory(name, extension, null);
}

/**
* Creates a temp file in a target directory that will be deleted on exit
*
* This will also mark the corresponding Tribble/Tabix/BAM indices matching the temp file for deletion.
* @param name Prefix of the file.
* @param extension Extension to concat to the end of the file name.
* @param targetDir Directory in which to create the temp file. If null, the default temp directory is used.
* @return A file in the temporary directory starting with name, ending with extension, which will be deleted after the program exits.
*/
public static File createTempFileInDirectory(final String name, String extension, final File targetDir) {
try {

if ( !extension.startsWith(".") ) {
extension = "." + extension;
}

final File file = File.createTempFile(name, extension);
final File file = File.createTempFile(name, extension, targetDir);
file.deleteOnExit();

// Mark corresponding indices for deletion on exit as well just in case an index is created for the temp file:
Expand All @@ -424,7 +437,6 @@ public static File createTempFile(String name, String extension) {
}
}


/**
* @param extension a file extension, may include 0 or more leading dots which will be replaced with a single dot
* @return replace the final extension on a path with the given extension
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,11 @@ public boolean executeScript(final Resource scriptResource, final List<String> p
Utils.nonNull(scriptResource, "script resource cannot be null");
// this File is automatically scheduled for deletion on exit
final File tempResourceFile = IOUtils.writeTempResource(scriptResource);
return executeScript(tempResourceFile.getAbsolutePath(), pythonProcessArgs, scriptArgs);
try {
return executeScript(tempResourceFile.getAbsolutePath(), pythonProcessArgs, scriptArgs);
} finally {
FileUtils.deleteQuietly(tempResourceFile);
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
package org.broadinstitute.hellbender.utils.R;

import org.broadinstitute.hellbender.utils.test.BaseTest;
import org.testng.Assert;
import org.testng.annotations.Test;

import java.io.File;

public final class RScriptLibraryUnitTest {
public final class RScriptLibraryUnitTest extends BaseTest {
@Test(groups = {"R"})
public void testProperties() {
Assert.assertEquals(RScriptLibrary.GSALIB.getLibraryName(), "gsalib");
Expand All @@ -17,4 +18,15 @@ public void testWriteTemp() {
final File file = RScriptLibrary.GSALIB.writeTemp();
Assert.assertTrue(file.exists(), "R library was not written to temp file: " + file);
}

@Test(groups = {"R"})
public void testWriteLibraryToTempFileInDir() {
final RScriptLibrary library = RScriptLibrary.GSALIB;
final File tempLibSourceDir = createTempDir("testWriteLibraryToTempFileInDir");
final File tempLibraryFile = library.writeLibraryToTempFile(tempLibSourceDir);
Assert.assertTrue(tempLibraryFile.exists(), "R library was not written to temp file: " + tempLibraryFile);
Assert.assertEquals(tempLibraryFile.getParentFile().getAbsolutePath(), (tempLibSourceDir.getAbsolutePath()),
"R library was not written to temp file: " + tempLibraryFile + " in dir: " + tempLibSourceDir);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,15 @@ public void testWriteTempResourceFromPath(
Assert.assertEquals(resourceContentsFirstLine, expectedFirstLine);
}

@Test
public void testCreateTempFileInDirectory() {
final File tempDir = createTempDir("testCreateTempFileInDirectory");
final File tempFile = IOUtils.createTempFileInDirectory("testCreateTempFileInDirectory", ".txt", tempDir);
Assert.assertTrue(tempFile.exists(), "file was not written to temp file: " + tempFile);
Assert.assertEquals(tempFile.getParentFile().getAbsolutePath(), (tempDir.getAbsolutePath()),
"file was not written to temp file: " + tempFile + " in dir: " + tempDir);
}

private String getFirstLineAndDeleteTempFile(final File tempResourceFile) throws IOException {
String resourceContentsFirstLine = "";
try (final FileReader fr = new FileReader(tempResourceFile);
Expand Down

0 comments on commit 1a33c3f

Please sign in to comment.