Skip to content
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 large runtime resource directory to lfs, and expose it to the Docker build. #4530

Merged
merged 1 commit into from
Mar 27, 2018

Conversation

cmnbroad
Copy link
Collaborator

@cmnbroad cmnbroad commented Mar 14, 2018

This adds a new folder for large runtime resources to git-lfs. Unlike the large test resources, which are only accessed via a volume mount when tests are run on the Docker image, the runtime resources need to be accessible to the GATK build during the Docker build process, since they're included in the jar. AFAICT there is no docker build equivalent to docker run -v. So for now the runtime resources are git pulled into the Docker staging area, and thus onto the Docker image. We need this for @lucidtronix 's CNN branch (and possibly for @TedBrookings) if we're going to load models for resources, but longer term, we need a better solution.

@cmnbroad
Copy link
Collaborator Author

@jamesemery @lbergelson Any alternate suggestions ?

@codecov-io
Copy link

codecov-io commented Mar 14, 2018

Codecov Report

Merging #4530 into master will decrease coverage by 0.224%.
The diff coverage is 81.818%.

@@              Coverage Diff               @@
##             master     #4530       +/-   ##
==============================================
- Coverage     79.84%   79.616%   -0.224%     
- Complexity    16958     17758      +800     
==============================================
  Files          1062      1068        +6     
  Lines         61677     65188     +3511     
  Branches       9983     10738      +755     
==============================================
+ Hits          49243     51900     +2657     
- Misses         8539      9232      +693     
- Partials       3895      4056      +161
Impacted Files Coverage Δ Complexity Δ
...g/broadinstitute/hellbender/utils/io/Resource.java 52.381% <ø> (+9.524%) 5 <0> (+1) ⬆️
...g/broadinstitute/hellbender/utils/NativeUtils.java 25% <0%> (ø) 3 <0> (ø) ⬇️
...adinstitute/hellbender/utils/R/RScriptLibrary.java 100% <100%> (ø) 6 <1> (ø) ⬇️
...rg/broadinstitute/hellbender/utils/io/IOUtils.java 68.358% <88.889%> (+6.549%) 100 <3> (+49) ⬆️
...itute/hellbender/tools/funcotator/Funcotation.java 45.455% <0%> (-4.545%) 7% <0%> (+3%)
.../discovery/alignment/ContigAlignmentsModifier.java 81.25% <0%> (-4.285%) 70% <0%> (+29%)
...ignment/AssemblyContigWithFineTunedAlignments.java 66.234% <0%> (-3.669%) 41% <0%> (+9%)
.../tools/copynumber/PostprocessGermlineCNVCalls.java 87.931% <0%> (-3.14%) 30% <0%> (+7%)
...ools/funcotator/FuncotatorArgumentDefinitions.java 85.075% <0%> (-1.592%) 1% <0%> (ø)
...ools/copynumber/DetermineGermlineContigPloidy.java 95.041% <0%> (-1.429%) 19% <0%> (+5%)
... and 49 more

@@ -270,5 +267,21 @@ public void testUrlEncodeDecode(final String string, final String encoded) {
Assert.assertEquals(string, IOUtils.urlDecode(encoded));
}

@Test
public void testLoadLargeRuntimeResource() throws IOException {
final String largeResourcePath = "large/largeResourceTest.txt";
Copy link
Member

@lbergelson lbergelson Mar 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this path should be a constant somewhere (the "large" I mean)

Copy link
Member

@lbergelson lbergelson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems sane to me. I think we're going to need to update the readme to explain that some large files are required to build a functional gatk now though. Maybe we can wait until a tool that requires them though.

We might want to add a check and warning to the gradle build that the large runtime resources are available as well so that it doesn't blindly build without them and then people get surprised. (also the tools that require them need to have good error checking around their existence...)

@cmnbroad cmnbroad force-pushed the cn_large_runtime_resources branch 2 times, most recently from 109c9f1 to 2a09e01 Compare March 16, 2018 20:09
@cmnbroad
Copy link
Collaborator Author

I added a check to build.gradle to verify that the large (runtime) resources are not lfs pointer files. I also added a test to verify that there is a .git folder, and non-null ToolProvider classLoader (fixes #4532). To test the pointer file check, I created a branch of this branch with a temporary commit that reverts the addition of the large resources to the Docker staging area, and submitted it to Travis to verify that the build fails when they're not present.

README.md Outdated
@@ -55,6 +55,8 @@ releases of the toolkit.
* Optional, but recommended:
* Gradle 3.1 or greater, needed for building the GATK. We recommend using the `./gradlew` script which will
download and use an appropriate gradle version automatically (see examples below).
* [git-lfs](https://git-lfs.github.com/) 1.1.0 or greater, needed for building the GATK. Used to download the large files required to run the build and to run the test suite.
Run `git lfs install` after downloading, followed by `git lfs pull` from the root of your git clone to download the large files. The download is several hundred megabytes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should split into "build" and "runtime" requirements?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, can you check whether git lfs pull still works?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I split this into separate sections for runtime and build. Updated to state that the lfs download is around 2GB. And git lfs pull does work.

README.md Outdated
@@ -55,6 +55,8 @@ releases of the toolkit.
* Optional, but recommended:
* Gradle 3.1 or greater, needed for building the GATK. We recommend using the `./gradlew` script which will
download and use an appropriate gradle version automatically (see examples below).
* [git-lfs](https://git-lfs.github.com/) 1.1.0 or greater, needed for building the GATK. Used to download the large files required to run the build and to run the test suite.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"run the build" -> "build GATK"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

README.md Outdated
@@ -102,7 +102,7 @@ You can download and run pre-built versions of GATK4 from the following places:

* This creates a zip archive in the `build/` directory with a name like `gatk-VERSION.zip` containing a complete standalone GATK distribution, including our launcher `gatk`, both the local and spark jars, and this README.
* You can also run GATK commands directly from the root of your git clone after running this command.
* Note that you *must* have a full git clone in order to build GATK. The zipped source code alone is not buildable.
* Note that you *must* have a full git clone in order to build GATK, incuding git-lfs files. The zipped source code alone is not buildable.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

incuding -> including

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"including the git-lfs files in src/main/resources" ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

build.gradle Outdated
task validateBuildPrerequisites {
if (ToolProvider.getSystemToolClassLoader() == null) {
throw new RuntimeException(
String.format(buildPrerequisitesErrorFormatString, "A (version 8) JDK must be installed."));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extract required Java version as a named constant at the top

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

build.gradle Outdated
if (!file(".git").isDirectory()) {
throw new RuntimeException(
String.format(buildPrerequisitesErrorFormatString,
"The GATK Github repository must be cloned to run the build."));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add "using git clone"

Also provide a link to github tutorial

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are all switched to use string interpolation now. The embedded buildPrerequisites message directs the user to the gatk github repo.

build.gradle Outdated
"The GATK Github repository must be cloned to run the build."));
}
// Next, ensure that the git-lfs files required to run the build are present
final FileTree largeResources = fileTree(dir: "src/main/resources/large")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declare src/main/resources/large as a named constant at the top

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

build.gradle Outdated
def validateLargeResourceFile(final File largeResourceFile, buildPrerequisitesErrorFormatString) {
largeResourceFile.withReader("UTF-8") { reader ->
final String firstLine = reader.readLine();
if (firstLine != null && firstLine.startsWith("version https://git-lfs.github.com/spec/v1")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declare this string as a named constant somewhere

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

build.gradle Outdated
// that the lfs files have been fetched and that the directory does not contain the git-lfs pointer files
def validateLargeResourceFile(final File largeResourceFile, buildPrerequisitesErrorFormatString) {
largeResourceFile.withReader("UTF-8") { reader ->
final String firstLine = reader.readLine();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read first X bytes instead of a full line in case the first line is very large.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

build_docker.sh Outdated
@@ -98,6 +98,11 @@ if [ -n "$STAGING_DIR" ]; then
GIT_CHECKOUT_COMMAND="git checkout ${GITHUB_DIR}${GITHUB_TAG}"
echo "${GIT_CHECKOUT_COMMAND}"
${GIT_CHECKOUT_COMMAND}
# pull just the large runtime resources into the staging area so they are available to the build
# when its done as part of the Docker build so the resources can be compiled into the jar as resources
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its -> it's

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

build_docker.sh Outdated
@@ -98,6 +98,11 @@ if [ -n "$STAGING_DIR" ]; then
GIT_CHECKOUT_COMMAND="git checkout ${GITHUB_DIR}${GITHUB_TAG}"
echo "${GIT_CHECKOUT_COMMAND}"
${GIT_CHECKOUT_COMMAND}
# pull just the large runtime resources into the staging area so they are available to the build
# when its done as part of the Docker build so the resources can be compiled into the jar as resources
GIT_PULL_LARGE_COMMAND="git lfs pull --include src/main/resources/large/"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be good to mention this command in the README for users who want to build GATK without downloading all of the large test data.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -9,6 +9,7 @@
public final class Resource {
private final String path;
private final Class<?> relativeClass;
public final static String LARGE_RESOURCES_PATH = "large";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment here explaining what this is

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@Test
public void testLoadLargeRuntimeResource() throws IOException {
final Resource largeResource = new Resource(Resource.LARGE_RESOURCES_PATH + "/largeResourceTest.txt", null);
final File resourceFile = IOUtils.writeTempResource(largeResource);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might want to extract a simple utility method for loading a resource and writing to a temp location (would return a File)

@cmnbroad cmnbroad force-pushed the cn_large_runtime_resources branch from d00d04f to 485ef3c Compare March 20, 2018 18:46
@cmnbroad
Copy link
Collaborator Author

@droazen please take a look at the updated README.md in this PR.

@droazen droazen self-assigned this Mar 21, 2018
Copy link
Contributor

@droazen droazen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second-pass review complete, back to @cmnbroad for more changes. A bunch of little comments, but also a request to have the build script run git lfs pull --include src/main/resources/large/ as needed during a build.

build.gradle Outdated
final pythonPackageArchiveName = 'gatkPythonPackageArchive.zip'
final largeResourcesFolder = "src/main/resources/large"
final lfsFileSentinel = "version https://git-lfs.github.com/spec/v1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment documenting what lfsFileSentinel is, since it's not immediately obvious.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, recommend renaming to lfsStubFileHeader for clarity

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed all of these names to use "stub", but I'm not sure thats better, since AFACIT git-lfs refers to these as pointer files in all of the help and doc.

build.gradle Outdated
final byte[] actualBytes = readBytesFromFile(largeResourceFile, lfsSentinel.length());
if (new String(actualBytes, "UTF-8") == lfsSentinel) {
throw new GradleException(
"$largeResourceFile appears to be a git-lfs pointer file." +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pointer file -> stub file

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

build.gradle Outdated
if (new String(actualBytes, "UTF-8") == lfsSentinel) {
throw new GradleException(
"$largeResourceFile appears to be a git-lfs pointer file." +
" The resource files in src/main/resources/large must be downloaded using git-lfs. " +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the largeResourcesFolder constant here

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

build.gradle Outdated
final buildPrerequisitesMessage = "See https://github.com/broadinstitute/gatk#building for information on how to build GATK."

// Large runtime resource files must be present at build time to be compiled into the jar, so validate
// that the lfs files have been fetched and that the directory does not contain the git-lfs pointer files
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pointer files -> stub files

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

build.gradle Outdated
// Validate that we have a clone of the repository and that any required git-lfs files have been resolved
def validateBuildPrerequisites(javaVersion, largeResourcesFolder, lfsSentinel, buildPrerequisitesMessage) {
if (ToolProvider.getSystemToolClassLoader() == null) {
throw new GradleException("The SystemToolClassLoader obtained from the ToolProvider is null. A Java $RequiredJavaVersion JDK must be installed. $buildPrerequisitesMessage")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When can this happen?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it happens when you have a JRE rather than a JDK (see #4532). Also, although I haven't tried it I suspect it will happen with JDK9.

finally {
resourceFile.delete();
}
Assert.assertEquals(resourceContents, "line in large resource file");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declare expected text as a constant, and use it in both test methods.

@DataProvider(name="resourcePaths")
public Object[][] getResourcePaths() {
return new Object[][] {
{ Resource.LARGE_RUNTIME_RESOURCES_PATH + "/largeResourceTest.txt", null }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test case involving a non-null relative class as well?

README.md Outdated
* [git-lfs](https://git-lfs.github.com/) 1.1.0 or greater. Required to download the large files used to build GATK and
run the test suite. Run `git lfs install` after downloading, followed by `git lfs pull` from the root of your git clone
to download all of the large files. The full download is approximately 2 gigabytes. Alternatively, to pull the minimal
set of large `lfs` resource files required to only build ()without running tests), the test resources can be excluded
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

()without running tests) -> (without running tests)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

README.md Outdated
run the test suite. Run `git lfs install` after downloading, followed by `git lfs pull` from the root of your git clone
to download all of the large files. The full download is approximately 2 gigabytes. Alternatively, to pull the minimal
set of large `lfs` resource files required to only build ()without running tests), the test resources can be excluded
from the download by running `git lfs pull --include src/main/resources/large/` instead of `git lfs pull`. This greatly
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we modify build.gradle to run this command automatically at build time, if lfs stubs are detected? Let's do that (if possible) -- then this set of instructions can be simplified to just "install git lfs"

Copy link
Collaborator Author

@cmnbroad cmnbroad Mar 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the build to check for stubs, and to only run git lfs pull --include src/main/resources/large/ if it finds any, then validate again. Also, we still have to manually download the files into the staging area, since the Docker doesn't have git-lfs installed. However, that doesn't pull the test files. So the readme still needs to reflect that difference. Updated accordingly. See what you think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done see above note.

README.md Outdated
* [git-lfs](https://git-lfs.github.com/) 1.1.0 or greater (needed to download large files for the complete test suite).
Run `git lfs install` after downloading, followed by `git lfs pull` from the root of your git clone to download the large files. The download is several hundred megabytes.
* Alternatively, pre-packaged images with all needed dependencies installed can be found on [our dockerhub repository](https://hub.docker.com/r/broadinstitute/gatk/). This requires a recent version of the docker client, which can be found on the [docker website](https://www.docker.com/get-docker).
* To build GATK:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is R a build-time dependency as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only to run tests, AFAIK. I added it to this section as well saying its needed for running tests.

@droazen droazen assigned cmnbroad and unassigned droazen Mar 21, 2018
@cmnbroad cmnbroad force-pushed the cn_large_runtime_resources branch 2 times, most recently from 700667a to f5fed00 Compare March 23, 2018 00:22
@cmnbroad cmnbroad assigned droazen and unassigned cmnbroad Mar 23, 2018
@cmnbroad
Copy link
Collaborator Author

Back to @droazen. Responded to all of the comments. I did an interim commit to test the lfs changes on docker/travis so there are two commits for this round.

Copy link
Contributor

@droazen droazen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Final review complete, back to @cmnbroad. Merge after addressing remaining comments.

build.gradle Outdated
println "Fetching large runtime resources via: $execLFSPull"
def retCode = execLFSPull.execute().waitFor()
if (retCode.intValue() != 0) {
throw new GradleException("git-lfs pull execution failed: $retCode");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"failed" -> "failed with exit code: "

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

build.gradle Outdated
}
return retCode
} catch (IOException e) {
throw new GradleException("git-lfs must be installed to run the GATK build", e)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is git-lfs not being installed the only possible cause of an IOException here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't be sure. I changed the message to report the IOException and suggest checking git-lfs.

build.gradle Outdated
if (checkLargeResourcesForStubFiles(largeResourcesFolder, lfsStubFileHeader, buildPrerequisitesMessage)) {
throw new GradleException(
"$largeResourcesFolder contains git-lfs stub files." +
" The resource files in $largeResourcesFolder must be downloaded using git-lfs. " +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Provide the actual git-lfs command in this error message.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

build.gradle Outdated
}

// Validate that we have a clone of the repository and that any required git-lfs files have been resolved
def validateBuildPrerequisites(javaVersion, largeResourcesFolder, lfsStubFileHeader, buildPrerequisitesMessage) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now does more than validation (it also potentially actually downloads prerequisites). Update the comment (and possible the name as well) to reflect this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@@ -262,6 +321,8 @@ processResources {
processTestResources {
//Don't waste time packaging unnecessary test data into the test resources:
include "org/broadinstitute/hellbender/utils/config/*"
//Required for IOUtils resource tests
include "org/broadinstitute/hellbender/utils/io/*"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this block need to be modified every time a resource is added?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only if you add a resource that needs to be packaged into the test jar as part of the test resources.

{
final Resource largeResource = new Resource(resourcePath, relativeClass);
final File resourceFile = IOUtils.writeTempResource(largeResource);
final String resourceContentsFirstLine = getFirstLineAndDeleteTempFile(resourceFile);
Copy link
Contributor

@droazen droazen Mar 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the IOUtils.writeTempResource() methods always set the extracted resource to be deleted on JVM exit? It seems like we will end up leaving garbage around if we don't do that in the methods themselves.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably should. I'm going to do that in a separate PR though since all of the call sites should be cleaned up as well.

README.md Outdated
* [git-lfs](https://git-lfs.github.com/) 1.1.0 or greater. Required to download the large files used to build GATK, and
test files required to run the test suite. Run `git lfs install` after downloading, followed by `git lfs pull` from
the root of your git clone to download all of the large files, including those required to run the test suite. The
full download is approximately 2 gigabytes. Alternatively, you can skip this step since the build will itself use
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Alternatively, " -> "Alternatively, if you are just building GATK and not running the test suite, "

@droazen droazen assigned cmnbroad and unassigned droazen Mar 23, 2018
@cmnbroad cmnbroad force-pushed the cn_large_runtime_resources branch from 72e5adb to 1fb2207 Compare March 26, 2018 13:27
Copy link
Contributor

@droazen droazen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 One or two last comments, then this is good to merge.

Can you create a ticket for the task of moving to delete-on-exit for extracted resources, so we don't forget to do it?

README.md Outdated
the root of your git clone to download all of the large files, including those required to run the test suite. The
full download is approximately 2 gigabytes. Alternatively, if you are just building GATK and not running the test
suite, you can skip this step since the build itself will use git-lfs to download the minimal set of large `lfs`
resource files required to complete the build. The tests resources will not be downloaded, but this greatly reduces
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"tests resources" -> "test resources"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

final lfsStubFileHeader = "version https://git-lfs.github.com/spec/v1" // first line of a git-lfs stub file
def readBytesFromFile = { largeFile, n ->
final byte[] bytes = new byte[n]
largeFile.withInputStream { stream -> stream.read(bytes, 0, bytes.length) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the input stream here get closed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, its equivalent to try-wth-resources.

@cmnbroad cmnbroad force-pushed the cn_large_runtime_resources branch from 1fb2207 to 9c646ca Compare March 26, 2018 19:56
@cmnbroad cmnbroad merged commit b572047 into master Mar 27, 2018
@cmnbroad cmnbroad deleted the cn_large_runtime_resources branch April 26, 2018 13:19
cwhelan pushed a commit to cwhelan/gatk-linked-reads that referenced this pull request May 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants