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

Updated install_R_packages.R to fix broken ggplot2 dependency in gatkbase Docker image. #5040

Merged
merged 3 commits into from
Jul 27, 2018

Conversation

samuelklee
Copy link
Contributor

@samuelklee samuelklee commented Jul 20, 2018

The ggplot2 R dependency was not installed correctly in the gatkbase-2.0.0 Docker image. It appears that this resulted from a recent ggplot2 update that has broken dependencies (perhaps for the version of R that we use). This missing ggplot2 dependency was the root cause of #5022.

I updated the install_R_packages.R script, which should now fail if any package fails to install, and pushed an updated gatkbase-2.0.1 image. The second commit addresses #5022.

This should be considered a temporary fix until #5026 is in.

Closes #5022.

@codecov-io
Copy link

codecov-io commented Jul 20, 2018

Codecov Report

Merging #5040 into master will increase coverage by 0.104%.
The diff coverage is 100%.

@@              Coverage Diff               @@
##             master     #5040       +/-   ##
==============================================
+ Coverage     86.38%   86.485%   +0.104%     
- Complexity    28640     29299      +659     
==============================================
  Files          1782      1791        +9     
  Lines        132603    135334     +2731     
  Branches      14761     15341      +580     
==============================================
+ Hits         114543    117043     +2500     
- Misses        12740     12841      +101     
- Partials       5320      5450      +130
Impacted Files Coverage Δ Complexity Δ
...walkers/bqsr/AnalyzeCovariatesIntegrationTest.java 93.846% <100%> (+1.783%) 24 <0> (+2) ⬆️
.../sv/integration/SVIntegrationTestDataProvider.java 90.909% <0%> (-1.399%) 2% <0%> (+1%)
...lignment/AssemblyContigAlignmentsConfigPicker.java 92.857% <0%> (-0.658%) 125% <0%> (+30%)
.../AssemblyContigAlignmentsConfigPickerUnitTest.java 99.279% <0%> (-0.326%) 42% <0%> (+22%)
...on/FindBreakpointEvidenceSparkIntegrationTest.java 100% <0%> (ø) 13% <0%> (+7%) ⬆️
...dateBasicSomaticShortMutationsIntegrationTest.java 100% <0%> (ø) 6% <0%> (+1%) ⬆️
...formers/PalindromeArtifactClipReadTransformer.java 100% <0%> (ø) 23% <0%> (+6%) ⬆️
...spark/sv/evidence/BreakpointDensityFilterTest.java 100% <0%> (ø) 22% <0%> (+5%) ⬆️
...tools/spark/sv/evidence/XGBoostEvidenceFilter.java 88.95% <0%> (ø) 45% <0%> (?)
...nder/tools/spark/sv/evidence/EvidenceFeatures.java 71.429% <0%> (ø) 5% <0%> (?)
... and 34 more

@samuelklee samuelklee requested a review from jamesemery July 20, 2018 16:15
@samuelklee
Copy link
Contributor Author

@cmnbroad Just saw your email about releasing---I think this definitely needs to go in before then. Can you review?

@cmnbroad cmnbroad self-requested a review July 27, 2018 12:50
}

dependencies = c("gplots",
"digest", "gtable", "MASS", "plyr", "reshape2", "scales", "tibble", "lazyeval") # ggplot2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just realized the comment might not be very helpful. I'll change it from "ggplot2" to "for ggplot2".

Copy link
Collaborator

@cmnbroad cmnbroad left a comment

Choose a reason for hiding this comment

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

@samuelklee Looks good - a couple of questions though, just to confirm that I understand the changes:

  • I pulled the 2.0.1 base image and the R dependencies look right to me, but there is a gatkbase image published as 2.1.0 that looks older. Presumably that was from the first solution you tried for this, and we don't want that now (or rather yet) ?
  • BQSR.R doesn't have an explicit dependency on reshape, but reshape2 is still needed for ggplot2 ?
  • When this branch ran on travis, was the travis cache cleared first ? If not, we should probably force it to re-run with the cache cleared just to be sure.

@samuelklee
Copy link
Contributor Author

Thanks, @cmnbroad!

  • You're right about gatkbase-2.1.0, that image is coming from Moved R dependencies to conda environment, cleaned up R/python dependencies, and updated base Docker/Travis configuration. #5026, which needs some more work. We can delete it for the time being if you think it'll cause confusion.
  • Correct, I think the import statement for reshape in BQSR.R was always incorrect/extraneous. reshape2 is the correct dependency for ggplot2 (which is itself imported), and reshape is not explicitly used in BQSR.R. So to recap: I removed the installation of this unnecessary package, but failed to remove an unnecessary import statement since it was in an untested code path, which was then caught when users tried to run the tool. Investigation of this issue then revealed that ggplot2 was not installed correctly in the current base image, due to a completely unrelated dependency issue.
  • Good call on clearing the Travis cache. Not actually sure how to do that, do I just delete the cache at https://travis-ci.org/broadinstitute/gatk/caches for this particular branch?

@cmnbroad
Copy link
Collaborator

@samuelklee I'd say lets leave 2.1 base image up there for now, and yes on the cache clearing. Once tests pass with the cache cleared it should be good to merge. Feel free to squash and rebase if you like.

Copy link
Collaborator

@cmnbroad cmnbroad left a comment

Choose a reason for hiding this comment

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

Once tests pass on travis with the cache cleared.

@samuelklee
Copy link
Contributor Author

OK, done, thanks! Fingers crossed that everything is fixed now...!

@samuelklee samuelklee merged commit 853b53e into master Jul 27, 2018
@samuelklee samuelklee deleted the sl_base_2.0.1 branch July 27, 2018 16:47
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.

Restore reshape R dependency and add test.
3 participants