Update the GATK base image to a newer LTS ubuntu release #8610

droazen · 2023-12-08T22:15:11Z

No description provided.

lbergelson

looks ok, I have a question, but the tests are the real validation

lbergelson · 2023-12-08T22:33:26Z

scripts/docker/gatkbase/Dockerfile

-ENV CONDA_PATH /opt/miniconda
+# Following the instructions from https://docs.conda.io/projects/conda/en/latest/user-guide/install/rpm-debian.html
+# Note that this *does* install just miniconda, despite the package being named "conda"
+RUN curl https://repo.anaconda.com/pkgs/misc/gpgkeys/anaconda.asc | gpg --dearmor > /tmp/conda.gpg && \


I hate conda.

lbergelson · 2023-12-08T22:34:14Z

scripts/docker/gatkbase/Dockerfile

+    apt -y autoclean  && \
+    apt -y autoremove && \
+    . /opt/conda/etc/profile.d/conda.sh && \
+    conda -V


Do you need to delete the conda trash that we were removing before?
all the .a and .pyc files it makes when it builds stuff. (including itself)

I've reverted all these changes

gatk-bot · 2023-12-09T00:31:18Z

Github actions tests reported job failures from actions build 7147397849
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
conda	17.0.6+10	7147397849.3	logs

lbergelson · 2023-12-09T02:16:05Z

This h5diff error is the same thing I saw when I tried to just bump the version. We must be either getting a new version of it?

lbergelson · 2023-12-09T02:17:00Z

Oh, it's a python problem.

 @Test(groups = {"python"}) // python environment is required to use h5diff
    public void testRunSystemCommand() {
        runSystemCommand(String.format("h5diff %s/extract.AS.indel.pos.annot.hdf5 %s/extract.AS.indel.pos.annot.hdf5",
                EXPECTED_TEST_FILES_DIR, EXPECTED_TEST_FILES_DIR));
        runSystemCommand(String.format("diff %s/extract.AS.indel.pos.vcf %s/extract.AS.indel.pos.vcf",
                EXPECTED_TEST_FILES_DIR, EXPECTED_TEST_FILES_DIR));
    }

Of course it is.

lbergelson · 2023-12-09T02:19:28Z

conda-forge::h5py=2.10.0 # required by keras 2.2.4

…r changes

gatk-bot · 2023-12-09T21:59:01Z

Github actions tests reported job failures from actions build 7153514898
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
conda	17.0.6+10	7153514898.3	logs

droazen · 2023-12-09T22:16:37Z

@lbergelson So, the test failures are in ScoreVariantAnnotationsIntegrationTest and TrainVariantAnnotationsModelIntegrationTest, when run within the conda environment in the new docker image. I've confirmed that they happen with both Ubuntu 22.04 and 20.04. They are assertion failures from h5diff indicating that the actual outputs did not match the expected outputs, so there are likely actual numerical differences here. Most likely the newer Ubuntu releases are shipping with newer versions of native libraries that HDF5 (or some other GATK dependency) depends on, and there was some breaking change somewhere along the line.

I think our only option is to capture the outputs of these tests, and ask @samuelklee to confirm that the differences do not indicate an actual regression.

gatk-bot · 2023-12-09T23:05:28Z

Github actions tests reported job failures from actions build 7153790937
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
conda	17.0.6+10	7153790937.3	logs

samuelklee · 2023-12-10T00:39:06Z

@droazen I hacked one of the TrainVariantAnnotationsModelIntegrationTest cases to run in your Docker (only necessary because it seems like gradlew test --tests *TrainVariantAnnotationsModelIntegrationTest doesn't recognize tests that use a DataProvider, but perhaps I did something wrong). Here are the differences:

(gatk) root@a87e0994889e:/repo# h5diff -v /repo/src/test/resources/large/org/broadinstitute/hellbender/tools/walkers/vqsr/scalable/train/expected/extract.nonAS.snpIndel.posUn.train.snpIndel.posOnly.IF.snp.trainingScores.hdf5 /repo/extract.nonAS.snpIndel.posUn.train.snpIndel.posOnly.IF.snp.trainingScores.hdf5

file1     file2
---------------------------------------
    x      x    /              
    x      x    /data          
    x      x    /data/scores   

group  : </> and </>
0 differences found
group  : </data> and </data>
0 differences found
dataset: </data/scores> and </data/scores>
size:           [445]           [445]
position        scores          scores          difference          
------------------------------------------------------------
[ 60 ]          -0.419202       -0.419202       5.55112e-17    
1 differences found

Looks pretty negligible to me! 😝 Probably a result of the native code being called by the python/ML packages used in these tools; even minor changes in the compilers across Ubuntu versions might introduce differences like these.

A quick fix might be to replace all system calls to h5diff in these tests with h5diff --use-system-epsilon; seems to do the trick here. But if that doesn't fix all test cases, then perhaps you can relax things with h5diff -p EPSILON, where EPSILON is a relative threshold. Probably OK to pick something like 1E-6. OK if I leave it to you to try this or otherwise check the rest of the cases?

Sorry for the inconvenience! I think the exact-match test worked as intended here, but I probably could've put in better messaging originally. Unfortunately, it's a bit awkward to grab the output of system commands.

And thanks for dealing with conda again (a necessary evil, unless we want to reimplement the entire field of machine learning in Java)! I'll experiment to see if I can't get the more recent version used in #8561 (23.10) working with the current environment---probably just some minor tweak to the pip version is needed to get around the error you're seeing. You could try unpinning it to see what gets pulled in. It would be great if we could get off the old version of conda, since more recent versions using the libmamba solver are MUCH faster and would cut down all of our Docker build times substantially.

droazen · 2023-12-10T21:09:20Z

Thanks @samuelklee for the Saturday night reply -- you rock! I will try running the tests with an epsilon and see if that does the trick. If it does, I might make another attempt at the new conda version to get the fast solver, since the current one is definitely annoyingly slow.

…ationsIntegrationTest and TrainVariantAnnotationsModelIntegrationTest

gatk-bot · 2023-12-10T22:23:23Z

Github actions tests reported job failures from actions build 7160404020
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
conda	17.0.6+10	7160404020.3	logs

gatk-bot · 2023-12-11T10:29:28Z

Github actions tests reported job failures from actions build 7165443572
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
conda	17.0.6+10	7165443572.3	logs

…factoring

samuelklee · 2023-12-11T15:12:07Z

@droazen see how to update to conda 23.10.0 with the correct pip over at #8614. This shaves a couple of minutes off the Docker build, but more importantly, the faster solver should make life much easier for devs doing any updates to the conda environment in the future. (For example, trying to experiment with the environment updates made in #8561 was very painful with conda 23.1.0 currently in the base image---lots of hanging/failed solves.)

23.11.0 was just released, but it seems that there were some hiccups in the associated libmamba release, so let's stick with the last version for now.

droazen · 2023-12-11T17:42:48Z

Thanks @samuelklee , I will incorporate your conda update into this branch, now that we've dealt with the test failures!

I patched the VETS test code to include the h5diff (and diff) output in the exception messages when one of these commands fails, and switched to the existing BaseTest methods for running the process and capturing the output. You can see what the output looks like (when we remove the epsilon tolerance) here:

https://storage.googleapis.com/hellbender-test-logs/build_reports/8610/merge_7165443572.3/tests/testOnPackagedReleaseJar/classes/org.broadinstitute.hellbender.tools.walkers.vqsr.scalable.ScoreVariantAnnotationsIntegrationTest.html

https://storage.googleapis.com/hellbender-test-logs/build_reports/8610/merge_7165443572.3/tests/testOnPackagedReleaseJar/classes/org.broadinstitute.hellbender.tools.walkers.vqsr.scalable.TrainVariantAnnotationsModelIntegrationTest.html

As you suspected/hoped, all the differences were tiny. When you have a chance, could you please review these changes to the VETS tests and let me know if you spot any issues?

samuelklee

Tests and results LGTM, just a few extra comments/questions. Thanks for cleaning up the system-command stuff too!

samuelklee · 2023-12-11T19:26:26Z

scripts/docker/gatkbase/Dockerfile

    python3 \
+    python3-pip \


Is this system pip required for anything?

I thought it might be useful to have a pip in the base environment, but I can remove it if you think it might cause problems?

Should be fine, just curious!

samuelklee · 2023-12-11T19:29:02Z

scripts/docker/gatkbase/stage_docker_base_in_cloud.sh

+    exit 1
+fi
+
+IMAGE_VERSION=$1


The build_docker_base.sh script currently has an (outdated) image version hardcoded in---maybe parametrize it there as well?

Yes, I am definitely going to update the old build script before merging this, and also add a separate script to release a pre-staged base image to the official location

gatk-bot · 2023-12-11T21:41:43Z

Github actions tests reported job failures from actions build 7173178607
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
conda	17.0.6+10	7173178607.3	logs

droazen requested a review from lbergelson December 8, 2023 22:17

lbergelson approved these changes Dec 8, 2023

View reviewed changes

Update the GATK base image to a newer LTS ubuntu release

d8fe231

droazen force-pushed the dr_update_docker_base_image branch from 34f373a to d8fe231 Compare December 8, 2023 23:36

Incorporate base image changes from Sam Lee's PR, fix md5 check, othe…

50cfd0b

…r changes

droazen force-pushed the dr_update_docker_base_image branch from 027a2a9 to 50cfd0b Compare December 9, 2023 19:26

droazen added 2 commits December 9, 2023 15:32

pip

4a4b5c6

Back out update to newer conda, try 20.04 instead of 22.04

59762d5

Back to 22.04

87d6706

Add an h5diff epsilon + reporting of differences to ScoreVariantAnnot…

ccaea7c

…ationsIntegrationTest and TrainVariantAnnotationsModelIntegrationTest

droazen added 2 commits December 11, 2023 03:16

Capture h5diff output in exception messages

2187cf2

Remove epsilon temporarily

5576927

droazen added 2 commits December 11, 2023 05:41

restore epsilon, add debugging statement

636d139

Remove debugging, remove obsolete runSystemCommand() method, final re…

5934691

…factoring

droazen force-pushed the dr_update_docker_base_image branch from f705704 to 5934691 Compare December 11, 2023 13:16

Incorporate Sam L patch to upgrade conda + pip

82e8bfb

samuelklee approved these changes Dec 11, 2023

View reviewed changes

One more test with epsilon off to make sure reporting is still working

312141b

Restore epsilon, switch to release base image, update scripts and docs

92ce8ac

droazen merged commit 85d13d4 into master Dec 12, 2023
20 checks passed

droazen deleted the dr_update_docker_base_image branch December 12, 2023 02:44

droazen mentioned this pull request Dec 12, 2023

Sl dr update docker base image #8614

Closed

samuelklee mentioned this pull request Dec 12, 2023

Updated Python and PyMC, removed TensorFlow, and added PyTorch in conda environment. #8561

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update the GATK base image to a newer LTS ubuntu release #8610

Update the GATK base image to a newer LTS ubuntu release #8610

droazen commented Dec 8, 2023

lbergelson left a comment

lbergelson Dec 8, 2023

lbergelson Dec 8, 2023

droazen Dec 9, 2023

gatk-bot commented Dec 9, 2023

lbergelson commented Dec 9, 2023

lbergelson commented Dec 9, 2023

lbergelson commented Dec 9, 2023

gatk-bot commented Dec 9, 2023

droazen commented Dec 9, 2023

gatk-bot commented Dec 9, 2023

samuelklee commented Dec 10, 2023 •

edited

Loading

droazen commented Dec 10, 2023

gatk-bot commented Dec 10, 2023

gatk-bot commented Dec 11, 2023

samuelklee commented Dec 11, 2023 •

edited

Loading

droazen commented Dec 11, 2023

samuelklee left a comment

samuelklee Dec 11, 2023

droazen Dec 11, 2023

samuelklee Dec 11, 2023

samuelklee Dec 11, 2023

droazen Dec 11, 2023

gatk-bot commented Dec 11, 2023

Update the GATK base image to a newer LTS ubuntu release #8610

Update the GATK base image to a newer LTS ubuntu release #8610

Conversation

droazen commented Dec 8, 2023

lbergelson 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

gatk-bot commented Dec 9, 2023

lbergelson commented Dec 9, 2023

lbergelson commented Dec 9, 2023

lbergelson commented Dec 9, 2023

gatk-bot commented Dec 9, 2023

droazen commented Dec 9, 2023

gatk-bot commented Dec 9, 2023

samuelklee commented Dec 10, 2023 • edited Loading

droazen commented Dec 10, 2023

gatk-bot commented Dec 10, 2023

gatk-bot commented Dec 11, 2023

samuelklee commented Dec 11, 2023 • edited Loading

droazen commented Dec 11, 2023

samuelklee 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gatk-bot commented Dec 11, 2023

samuelklee commented Dec 10, 2023 •

edited

Loading

samuelklee commented Dec 11, 2023 •

edited

Loading