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

Make sure the GATK tool python package is present before executing streaming commands. #5819

Merged
merged 3 commits into from
Mar 26, 2019

Conversation

cmnbroad
Copy link
Collaborator

@cmnbroad cmnbroad commented Mar 20, 2019

Fixes #5751 and #4591. Longer term we'll still want to do package version-checking/verification per #4995 as well.

@jamesemery I included tests for this change, but I need the tests to only run when the conda env is NOT activated. Unit tests are always run on the docker image, so thats out. Integration tests are run on both the docker and the travis image, so I throw a skip exception on the docker, which I detect using the "CI" env variable. But that seems fragile and confusing. Is there a better way to do this ?

@jamesemery
Copy link
Collaborator

@cmnbroad I see. The "CI" variable does seem brittle, especially since I'm not strictly sure where it is set. I think a somewhat safer place would be to add some global test flag to the docker image would be to add it to the run_unit_tests.sh script. That way we know it is getting triggered exactly before we run the tests in just the docker environment. Is there some way of detecting what conda environment is active outside of conda.

@codecov-io
Copy link

codecov-io commented Mar 20, 2019

Codecov Report

Merging #5819 into master will decrease coverage by 6.733%.
The diff coverage is 41.667%.

@@               Coverage Diff               @@
##              master     #5819       +/-   ##
===============================================
- Coverage     87.031%   80.298%   -6.733%     
+ Complexity     32107     30526     -1581     
===============================================
  Files           1972      1975        +3     
  Lines         147194    147487      +293     
  Branches       16201     16233       +32     
===============================================
- Hits          128104    118429     -9675     
- Misses         13184     23343    +10159     
+ Partials        5906      5715      -191
Impacted Files Coverage Δ Complexity Δ
.../walkers/vqsr/CNNScoreVariantsIntegrationTest.java 97.386% <0%> (-2.614%) 13 <0> (ø)
...er/utils/python/StreamingPythonScriptExecutor.java 85.47% <100%> (+0.125%) 22 <1> (ø) ⬇️
...va/org/broadinstitute/hellbender/GATKBaseTest.java 96.429% <100%> (-3.571%) 7 <2> (ø)
...python/StreamingPythonExecutorIntegrationTest.java 11.111% <11.111%> (ø) 1 <1> (?)
.../hellbender/utils/python/PythonScriptExecutor.java 66.667% <66.667%> (+3.03%) 10 <0> (ø) ⬇️
...kers/filters/VariantFiltrationIntegrationTest.java 0.826% <0%> (-99.174%) 1% <0%> (-25%)
...dorientation/CollectF1R2CountsIntegrationTest.java 0.917% <0%> (-99.083%) 1% <0%> (-12%)
.../walkers/bqsr/BaseRecalibratorIntegrationTest.java 1.031% <0%> (-98.969%) 1% <0%> (-7%)
...s/variantutils/VariantsToTableIntegrationTest.java 1.042% <0%> (-98.958%) 1% <0%> (-20%)
...ers/vqsr/FilterVariantTranchesIntegrationTest.java 1.053% <0%> (-98.947%) 1% <0%> (-5%)
... and 180 more

@droazen
Copy link
Contributor

droazen commented Mar 25, 2019

@jamesemery @cmnbroad Can we try to get this one in in time for the release?

Copy link
Collaborator

@jamesemery jamesemery left a comment

Choose a reason for hiding this comment

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

This looks fine to me if you have empirically observed the tests (and that you have seen that they run on the non-docker image travis machine).

// skip this test if we're running on the Docker because the Python environment is always
// activated there.
final String isDockerCI = System.getenv("CI");
if (isDockerCI != null && isDockerCI.equalsIgnoreCase("true")) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I still maintain that we should change the "CI" variable and set it ourselves as part of the run-docker script so we are not relying on some feature of the environment that might change. Otherwise i'm not too bothered by this being a skip exception.

// skip this test if we're running on the Docker because the Python environment is always
// activated there.
final String isDockerCI = System.getenv("CI");
if (isDockerCI != null && isDockerCI.equalsIgnoreCase("true")) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

ehh... this could be extracted out into a skipIfNotInDockerImage() since it's copied in two places.

Copy link
Collaborator

@jamesemery jamesemery left a comment

Choose a reason for hiding this comment

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

These changes look good, feel free to merge when tests pass 👍

@cmnbroad cmnbroad merged commit c3a1139 into master Mar 26, 2019
@cmnbroad cmnbroad deleted the cn_cnn_check_deps branch March 26, 2019 11:56
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