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

Ci rework gcc builds #3631

Merged
merged 2 commits into from Jul 11, 2023

Conversation

scottwittenburg
Copy link
Collaborator

@scottwittenburg scottwittenburg commented May 24, 2023

This PR reworks how docker images are built for gcc, clang, and intel builds done in github actions. To dramatically reduce build times for the gcc builds, the plan going forward is to use pre-built Docker images from the E4S project (see "Minimal Images", described here) as a base image. These images come with some spack packages already installed, but more importantly, installing ADIOS2 dependencies using the spack shipped in those images results in most dependencies getting installed from a release-specific E4S buildcache. This is far faster than building those dependencies from sources.

For intel builds run in el8, since no buildcache is available to install ADIOS2 dependencies for that OS, the dependencies that can be installed from the package manager are installed, and options for missing dependencies are just turned off.

After this PR is merged, there are only two shell scripts used to build, tag, and push all the images used in gcc and intel builds on github actions. To build gcc images (currently including gcc8, gcc9, gcc10, gcc11, and clang10), simply run scripts/ci/images-v2/build-ubuntu.sh. On a fairly old linux machine with 32 hyperthreads, but with slow spinning disks, and with no caching, this now takes around 30 minutes. To build, tag, and push images for icc and oneapi testing, run scripts/ci/images-v2/build-el8.sh. This takes around 20 minutes, under the same conditions mentioned above.

This PR also renders some of the existing dockerfiles, scripts, cmake files, etc unused, and thus removes them from the repo.

fixes: #3611
fixes: #3612
fixes: #3613
fixes: #3614

@scottwittenburg scottwittenburg force-pushed the ci-rework-gcc-builds branch 5 times, most recently from fe48235 to 16fcd4d Compare May 24, 2023 19:58
@scottwittenburg scottwittenburg force-pushed the ci-rework-gcc-builds branch 2 times, most recently from bdfaebf to 0a7569a Compare June 20, 2023 22:24
@scottwittenburg
Copy link
Collaborator Author

@vicentebolea The two failing tests on ubuntu/gcc10/mpich seem to have failed on master recently as well.

There is also 1 configure warning on each of the four intel builds. I think it's because those images are using the latest nightly version of cmake, which is warning about kwsys having too old of a minimum cmake version requirement.

Other than those issues, this is probably ready for you to review.

Copy link
Collaborator

@vicentebolea vicentebolea left a comment

Choose a reason for hiding this comment

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

Review from the skies, I will review the rest tomorrow, running out of battery in the fly.

scripts/ci/images-v2/Dockerfile.ci-spack-ubuntu20.04-leaf Outdated Show resolved Hide resolved
scripts/ci/setup-run/ci-ubuntu20.04-gcc10.sh Outdated Show resolved Hide resolved
.shellcheck_exclude_paths Outdated Show resolved Hide resolved
.github/workflows/everything.yml Outdated Show resolved Hide resolved
scripts/ci/cmake-v2/ci-ubuntu20.04-clang10-mpi.cmake Outdated Show resolved Hide resolved
@scottwittenburg
Copy link
Collaborator Author

@vicentebolea Since a lot of the work that went into this PR was exploratory at first, the commit history is far from clean. I'm not sure it's worth the effort to try to break it up into logical chunks now, but I could easily collapse it into a single commit if you want.

Copy link
Collaborator

@vicentebolea vicentebolea left a comment

Choose a reason for hiding this comment

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

Great job Scott, only cosmetic changes requested here. Sorry for me taking this long to review this PR, the reason is due to the size of changes and me being distracted with the AHM this week. Let me know

scripts/ci/cmake-v2/ci-el8-icc-mpi.cmake Outdated Show resolved Hide resolved
scripts/ci/cmake-v2/ci-el8-icc-serial.cmake Outdated Show resolved Hide resolved
scripts/ci/images-v2/Dockerfile.ci-spack-ubuntu20.04-leaf Outdated Show resolved Hide resolved
scripts/ci/cmake-v2/ci-ubuntu20.04-gcc10-mpich.cmake Outdated Show resolved Hide resolved
@@ -28,6 +35,7 @@ MPIEXEC_EXTRA_FLAGS:STRING=--allow-run-as-root --oversubscribe
MPIEXEC_MAX_NUMPROCS:STRING=${N2CPUS}
")

set(CTEST_CMAKE_GENERATOR "Ninja")
set(CTEST_TEST_ARGS PARALLEL_LEVEL 1)
set(CTEST_CMAKE_GENERATOR "Unix Makefiles")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above

scripts/ci/setup-run/ci-ubuntu20.04-gcc10.sh Show resolved Hide resolved
scripts/ci/setup-run/ci-ubuntu20.04-gcc11.sh Show resolved Hide resolved
scripts/ci/setup-run/ci-ubuntu20.04-gcc8.sh Show resolved Hide resolved
scripts/ci/setup-run/ci-ubuntu20.04-gcc9.sh Show resolved Hide resolved
source/adios2/engine/bp5/BP5Reader.cpp Outdated Show resolved Hide resolved
@vicentebolea
Copy link
Collaborator

We also are missing the clang6 build but we can provide it in a later PR

@vicentebolea
Copy link
Collaborator

I'm not sure it's worth the effort to try to break it up into logical chunks now, but I could easily collapse it into a single commit if you want.

Its up to you, but due to the large amount of changes you might as well make a single commit and be done with it

@scottwittenburg
Copy link
Collaborator Author

You mentioned you tried using the slave approach and it worked for you:

I Tried update-alternatives --install /usr/bin/gcc gcc /usr/bin/gcc-11 100 --slave /usr/bin/g++ g++ /usr/bin/g++-11 in the ubuntu image and it seems to work.

Here's what I see:

$ docker run --rm -ti ecpe4s/ubuntu20.04:23.02
root@9bfae9858561:/# apt-get update
Hit:1 http://archive.ubuntu.com/ubuntu focal InRelease
Get:2 http://archive.ubuntu.com/ubuntu focal-updates InRelease [114 kB]
Get:3 http://security.ubuntu.com/ubuntu focal-security InRelease [114 kB]
Get:4 http://archive.ubuntu.com/ubuntu focal-backports InRelease [108 kB]
Get:5 http://security.ubuntu.com/ubuntu focal-security/multiverse amd64 Packages [37.5 kB]
Get:6 http://security.ubuntu.com/ubuntu focal-security/restricted amd64 Packages [2,529 kB]
Get:7 http://archive.ubuntu.com/ubuntu focal-updates/main amd64 Packages [3,346 kB]
Get:8 http://security.ubuntu.com/ubuntu focal-security/main amd64 Packages [2,866 kB]
Get:9 http://security.ubuntu.com/ubuntu focal-security/universe amd64 Packages [1,070 kB]   
Get:10 http://archive.ubuntu.com/ubuntu focal-updates/restricted amd64 Packages [2,671 kB]    
Get:11 http://archive.ubuntu.com/ubuntu focal-updates/universe amd64 Packages [1,369 kB]
Get:12 http://archive.ubuntu.com/ubuntu focal-updates/multiverse amd64 Packages [40.3 kB]
Get:13 http://archive.ubuntu.com/ubuntu focal-backports/main amd64 Packages [55.2 kB]
Get:14 http://archive.ubuntu.com/ubuntu focal-backports/universe amd64 Packages [28.6 kB]
Fetched 14.3 MB in 2s (6,591 kB/s)                           
Reading package lists... Done
root@9bfae9858561:/# update-alternatives --install /usr/bin/gcc gcc /usr/bin/gcc-11 100 --slave /usr/bin/g++ g++ /usr/bin/g++-11
update-alternatives: error: alternative g++ can't be slave of gcc: it is a master alternative

Let me know if it's obvious to you what I'm doing wrong here, but I just copy-pasted what you said worked for you.

@scottwittenburg
Copy link
Collaborator Author

I'm not sure it's worth the effort to try to break it up into logical chunks now, but I could easily collapse it into a single commit if you want.

Its up to you, but due to the large amount of changes you might as well make a single commit and be done with it

Yeah ok, once you're happy with the changes (and everything is still working), I'll squash to a single commit and push one last time.

@scottwittenburg
Copy link
Collaborator Author

5b4dc07 adds a clang6 build, since it was easy to add, so we'll see how it fares in CI. But if you prefer I can also save that commit for a follow-on PR.

@vicentebolea
Copy link
Collaborator

Let me know if it's obvious to you what I'm doing wrong here, but I just copy-pasted what you said worked for you.

I see, it appears that g++ is at the same time another master alternative in this E4s image, thus update-alternatives is declining the instruction. In vanilla ubuntu20.04 is possible but not in this image, we tried but it is all right is not super important. Lets move on with this

Copy link
Collaborator

@vicentebolea vicentebolea left a comment

Choose a reason for hiding this comment

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

Looks great, before squashing everything into a single commit add a commit with the changes for source/adios2/engine/bp5/BP5Reader.cpp.

scripts/ci/cmake-v2/ci-ubuntu20.04-gcc11-mpi.cmake Outdated Show resolved Hide resolved
@@ -28,6 +35,7 @@ MPIEXEC_EXTRA_FLAGS:STRING=--allow-run-as-root --oversubscribe
MPIEXEC_MAX_NUMPROCS:STRING=${N2CPUS}
")

set(CTEST_TEST_ARGS PARALLEL_LEVEL 1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

same

scripts/ci/cmake-v2/ci-ubuntu20.04-gcc9-mpi.cmake Outdated Show resolved Hide resolved
@scottwittenburg
Copy link
Collaborator Author

before squashing everything into a single commit add a commit with the changes for source/adios2/engine/bp5/BP5Reader.cpp

Should I separate out the change to DataManWriter.cpp in that same commit as well?

@vicentebolea
Copy link
Collaborator

before squashing everything into a single commit add a commit with the changes for source/adios2/engine/bp5/BP5Reader.cpp

Should I separate out the change to DataManWriter.cpp in that same commit as well?

Yep

Copy link
Collaborator

@vicentebolea vicentebolea left a comment

Choose a reason for hiding this comment

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

The two failing test for mpich are not related to this PR.

Copy link
Collaborator

@vicentebolea vicentebolea left a comment

Choose a reason for hiding this comment

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

followup from previous review; everything else good; lets try ghcr instead of docker.

scripts/ci/images-v2/Dockerfile.ci-spack-ubuntu20.04-base Outdated Show resolved Hide resolved
@scottwittenburg scottwittenburg force-pushed the ci-rework-gcc-builds branch 4 times, most recently from 7b7e8b1 to e911aa7 Compare July 10, 2023 23:12
@scottwittenburg
Copy link
Collaborator Author

@vicentebolea Do you want to take one last look before merging? I think I've addressed everything we discussed.

Copy link
Collaborator

@vicentebolea vicentebolea left a comment

Choose a reason for hiding this comment

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

Lookgs good small last detail

docker build --rm --build-arg CLANG_VERSION=6.0 -f ./Dockerfile.ci-spack-ubuntu20.04-clang -t adios2:ci-spack-ubuntu20.04-clang6 .
docker build --rm --build-arg CLANG_VERSION=10 -f ./Dockerfile.ci-spack-ubuntu20.04-clang -t adios2:ci-spack-ubuntu20.04-clang10 .

# Tag test images for pushing (TODO: change organization before merge)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# Tag test images for pushing (TODO: change organization before merge)

No need as we will use GHCR

Copy link
Collaborator

@vicentebolea vicentebolea left a comment

Choose a reason for hiding this comment

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

Ready to merge; If you cant merge let me know.

Good job there :)

@scottwittenburg
Copy link
Collaborator Author

Thanks @vicentebolea! It seems that I cannot merge, but I don't think it's about permissions, as it seems I could push the "Enable auto-merge" button to "Automatically merge when all requirements are met". Is it possible the failing mpich test is preventing merge?

@vicentebolea
Copy link
Collaborator

@scottwittenburg yeah, I have also rebase it, lets merge it after passing CI

@vicentebolea vicentebolea merged commit 9965921 into ornladios:master Jul 11, 2023
28 of 29 checks passed
@scottwittenburg scottwittenburg deleted the ci-rework-gcc-builds branch July 11, 2023 21:49
vicentebolea added a commit to vicentebolea/ADIOS2 that referenced this pull request Jul 12, 2023
vicentebolea added a commit to vicentebolea/ADIOS2 that referenced this pull request Jul 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants