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

Optimize Dockerfile #136

Merged
merged 14 commits into from
Feb 2, 2024
Merged

Optimize Dockerfile #136

merged 14 commits into from
Feb 2, 2024

Conversation

radusuciu
Copy link
Contributor

Hi, I'm submitting this as a draft PR because I think there's room for additional improvements here.

This initial and minimal change was motivated by a build server being down which impacted the ability to build the contrib Docker image since it was downloading sources for coinor and eigen from this build server. Apparently, those specific versions of those libraries aren't really needed and they can be pulled down from apt. Doing so reduces the container build-time significantly (700 -> 140 seconds on my machine).

As long as the team is interested, I would like to continue to optimize this image further, and then can look at these images as well: https://github.com/OpenMS/dockerfiles/, and eventually maybe we can move everything to the main OpenMS repo as suggested in this issue: OpenMS/dockerfiles#28

@radusuciu radusuciu marked this pull request as draft January 11, 2024 19:43
@radusuciu
Copy link
Contributor Author

radusuciu commented Jan 11, 2024

Next improvements:

  • install all deps in one step
  • revisit cmake install method
  • make software.version LABEL dynamic (use build arg)
  • move COPY . /contrib to last step
  • add .dockerignore
  • update action and make use of GHA cache

Also, I'd like to ask... is this even needed as a separate image?

for consistency with Dockerfile in main repo PR
the docker-push-action should handle this for us
just ignoring workflows for now. Ignoring .git might be a good idea but I think some users of this image may rely on that?
@radusuciu radusuciu marked this pull request as ready for review January 22, 2024 22:34
@radusuciu
Copy link
Contributor Author

One thing I haven't changed is the cmake install method. Here's the current:

RUN <<-EOF
    cmake_ubuntu_version=$(lsb_release -cs)
    if ! wget -q --method=HEAD "https://apt.kitware.com/ubuntu/dists/$cmake_ubuntu_version/Release"; then
      bash -c "$(wget -O - https://apt.kitware.com/kitware-archive.sh)"
    else
      wget -qO - https://apt.kitware.com/kitware-archive.sh | bash -s -- --release $cmake_ubuntu_version
    fi
    apt-get -y update
    apt-get install -y cmake
EOF

This requires a few packages including wget, whatever provides lsb_release, maybe ca-certificates and a few others.

Here's an alternative for your consideration:

ARG CMAKE_VERSION="3.28.1"

# installing cmake
WORKDIR /tmp
ADD https://github.com/Kitware/CMake/releases/download/v${CMAKE_VERSION}/cmake-${CMAKE_VERSION}-linux-x86_64.sh cmake.sh
RUN <<-EOF
    mkdir -p /opt/cmake
    sh cmake.sh --skip-license --prefix=/opt/cmake
    ln -s /opt/cmake/bin/cmake /usr/local/bin/cmake
    ln -s /opt/cmake/bin/ctest /usr/local/bin/ctest
    rm -rf /tmp/*
EOF

I like the alternative because the version is explicit and it requires fewer extra packages. However, I'm not really all that experience in this space, so please let me know if you'd like me to implement this change or just stick with what we've got.

@jpfeuffer
Copy link
Contributor

No clear preference. Both should work. We can go with your solution.

dockerfiles/Dockerfile Outdated Show resolved Hide resolved
@radusuciu
Copy link
Contributor Author

Unless there are any other suggestions/comments, I think this is ready for merge.

I guess https://github.com/OpenMS/contrib/blob/master/dockerfiles/pyopenms/manylinux/Dockerfile could be the subject of a separate PR if you're interested in streamlining that too a bit?

@timosachsenberg
Copy link
Contributor

@jpfeuffer looks good to me. What do you think?
I also think streamlining the manylinux one would be great!
Also feel free to add yourself to the AUTHORS file in the OpenMS repo. This work is highly appreciated!

@jpfeuffer
Copy link
Contributor

Yes I think we are at a point that we can try merging and see. Thanks!!!

@timosachsenberg timosachsenberg merged commit 3a9eba4 into OpenMS:master Feb 2, 2024
@radusuciu
Copy link
Contributor Author

Thanks! Feel free to tag me in if there are any issues now or later

I'll take a look at the manylinux Dockerfile, seems it's pretty straightforward to reduce the number of layers at least

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.

3 participants