Reduced number of Dockerfiles by shimwell · Pull Request #813 · svalinn/DAGMC

shimwell · 2022-07-07T14:30:30Z

Description

This PR aims to simplify the CI process from many dockerfiles and scripts to less dockerfiles and scripts, carrying on the good work @bam241 made migrating from Dockerhub and Circle to GitHub Actions.

This is the first part of simplifying the CI, if this PR is successful then the next stage would be to move .sh scripts inside the Docker container

Motivation and Context

Simple is nice

Changes

Multiple Dockerfiles replaced with one

Behavior

Same end point, we should just have an easier system for the CI

shimwell · 2022-07-07T15:20:09Z

Tests are passing 🎉 and I think this is ready for a review

What do people think is this a reasonable first step for simplifying the dockerfiles.

There is more to be done but I am keen to keep the PR easy to review

gonuke

This all looks fine to me semantically/syntactically. However, it appears that the current workflow may (???) duplicate effort. In the original, each Dockerfile is based on a docker image created and pushed at an earlier step, referred to by name. In this case, we always start with the same base image, and it's unclear to me how much is cached within the workflow.

This may be a known issue with plans to resolve as this progresses? Or may be something we should discuss.

Discussion:

Since it only happens in the process of building CI images - hopefully relatively rare - we don't care? Especially if it gives simplicity for maintenance and developer onboarding.
We can fix it by careful/clever use of build_args

gonuke · 2022-07-08T10:26:18Z

Maybe we could temporarily add a master high-level TODO list as a comment in one of these files so that everyone sees the end goal/vision?

shimwell · 2022-07-08T10:47:10Z

Yep good point, I shall keep an eye on efficiency of the building, looking at the action logs this PR build takes about the same amount of time as previously, ~23mins

For the high level TODO list I was thinking it could be something like this

combine dockerfiles
move dockerfile location
move ARGS and ENV to top of dockerfile
move .sh scripts into dockerfile
add code space developer environment
remove old files include circleci
update setup-qemu-action to v2

gonuke · 2022-07-08T11:00:14Z

Oooo... you could make a GitHub project with these steps defined there... 😀 rather than in a single PR or comment in a file.

gonuke · 2022-07-08T11:01:15Z

As for efficiency, if it is just as fast, I'd like to make sure we understand why so that we don't accidentally break it in the future.

shimwell · 2022-07-08T11:24:25Z

As for efficiency, if it is just as fast, I'd like to make sure we understand why so that we don't accidentally break it in the future.

The current builds use -f for file and that file has a few docker files below which also needed to be built. If the images are built and in the local docker image collection then they are used, if they are not found then are downloaded.

This build uses --target which also needs the stages below to have previously been built. If they are in the local collection then they are used and if they are not found then they built from scratch. However the images are found in the cache as the building route starts from the base, tagging each build as it goes and works up the stages.

Each docker image build results in a tagged image and if the same dockerfile is built with the same build args values then the previously tagged image will be used. This happens automatically for modern docker installs on your desktop (buildkit required). Enabling Docker Layer Caching on GitHub requires a bit of extra lines in the existing yml scripts using the setup-qemu-action to get docker layer caching working for the current build scripts and this also applies to the proposed layout.

shimwell · 2022-07-08T11:34:09Z

Oooo... you could make a GitHub project with these steps defined there... grinning rather than in a single PR or comment in a file.

Yes certainly, I have added the tasks to a project here https://github.com/svalinn/DAGMC/projects/6

gonuke · 2022-07-08T15:50:32Z

As for efficiency, if it is just as fast, I'd like to make sure we understand why so that we don't accidentally break it in the future.

The current builds use -f for file and that file has a few docker files below which also needed to be built. If the images are built and in the local docker image collection then they are used, if they are not found then are downloaded.

This build uses --target which also needs the stages below to have previously been built. If they are in the local collection then they are used and if they are not found then they built from scratch. However the images are found in the cache as the building route starts from the base, tagging each build as it goes and works up the stages.

Each docker image build results in a tagged image and if the same dockerfile is built with the same build args values then the previously tagged image will be used. This happens automatically for modern docker installs on your desktop (buildkit required). Enabling Docker Layer Caching on GitHub requires a bit of extra lines in the existing yml scripts using the setup-qemu-action to get docker layer caching working for the current build scripts and this also applies to the proposed layout.

If I understand correctly, then, it's the setup-qemu-action part that is key, because we push these images with varying tag names and I'm not convinced that relying on fetching them would work. Instead, we have to rely on the cache.

shimwell · 2022-07-08T16:08:31Z

If I understand correctly, then, it's the setup-qemu-action part that is key, because we push these images with varying tag names and I'm not convinced that relying on fetching them would work. Instead, we have to rely on the cache.

I agree

bam241

few comments I didn't had the time to post,

I was doing a review and got interrupted...

Pushing this for future info

bam241 · 2022-07-08T11:57:32Z

+#TODO move sh file contents into this Dockerfile
+RUN /root/etc/CI/docker/build_embree.sh
+
+FROM external_deps AS hdf5


I would put 2 line breaks between stages

maybe also add a comment before the FROM to explain what the stage is about

pshriwise · 2022-07-09T21:57:37Z

Just returned from vacation so I'll keep an eye out for the next PR that continues this work. Thanks for diving into it @shimwell!

shimwell added 3 commits June 9, 2022 16:34

multi layer docker draft

4156805

upper case

4edd378

updated scripts to work with single dockerfile

65a5810

shimwell changed the title ~~Single dockerfile~~ Reduced number of Dockerfiles Jul 7, 2022

shimwell added 2 commits July 7, 2022 15:45

[skip ci] using housekeeping name for docs image

b55976b

added entry for 813

0f4e64d

shimwell mentioned this pull request Jul 7, 2022

request to moving location of Dockerfile #814

Open

shimwell requested review from bam241, gonuke and pshriwise July 8, 2022 09:07

gonuke reviewed Jul 8, 2022

View reviewed changes

gonuke merged commit 7f64b42 into svalinn:develop Jul 8, 2022

bam241 reviewed Jul 9, 2022

View reviewed changes

shimwell deleted the single_dockerfile branch July 15, 2022 07:41

gonuke mentioned this pull request Feb 23, 2023

Putting install scripts into dockerfile #822

Closed

Conversation

shimwell commented Jul 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Changes

Behavior

Uh oh!

shimwell commented Jul 7, 2022

Uh oh!

gonuke left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gonuke commented Jul 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shimwell commented Jul 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gonuke commented Jul 8, 2022

Uh oh!

gonuke commented Jul 8, 2022

Uh oh!

shimwell commented Jul 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shimwell commented Jul 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gonuke commented Jul 8, 2022

Uh oh!

shimwell commented Jul 8, 2022

Uh oh!

bam241 left a comment

Choose a reason for hiding this comment

Uh oh!

bam241 Jul 8, 2022

Choose a reason for hiding this comment

Uh oh!

bam241 Jul 8, 2022

Choose a reason for hiding this comment

Uh oh!

pshriwise commented Jul 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shimwell commented Jul 7, 2022 •

edited

Loading

gonuke left a comment •

edited

Loading

gonuke commented Jul 8, 2022 •

edited

Loading

shimwell commented Jul 8, 2022 •

edited

Loading

shimwell commented Jul 8, 2022 •

edited

Loading

shimwell commented Jul 8, 2022 •

edited

Loading