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

Streamline and simplify CI building #880

Merged
merged 33 commits into from
Aug 15, 2023
Merged

Conversation

gonuke
Copy link
Member

@gonuke gonuke commented May 8, 2023

With the success of #863, I started looking at updating our CI tests to take advantage of the new images. However, it occurred to me that with the effective caching of the multistage-docker-build-action, we don't need to generate images in a separate step. We can just do that always and have it take almost no time if the cached images are still valid.

This then also allows us to use some of the features of that action to just update the image for deployment when successful.

Hopefully this PR lets us test this a little... I'm testing it in my own repo to avoid polluting our images.

If successful, we'll be able to get rid of the deploy workflow entirely.

Expected behavior: Once successful images have been built/pushed to GHCR, this workflow should try to build every stage after first pulling and caching the previously build images. If the Dockerfile has not changed in the earlier stages, they should not require any action and will rely on the pulled/cached images. The dagmc build stage in the Dockerfile will force a full build when there is a change to the repo and then the testing stage will be run. If successful, it will push the dagmc_test stage to the repo and label it as latest.

If there are changes to earlier stages in the Dockerfile, it will execute those stages as well and continue until the tests, also.

@gonuke gonuke marked this pull request as draft May 8, 2023 12:01
@gonuke
Copy link
Member Author

gonuke commented May 8, 2023

Converted to draft to consider which GHCR repo is the right one for doing this testing...??? In its current form, this will pull/push directly to svalinn during testing and before merge, including overwriting the existing CI images. I'm not sure that's a good plan...

@gonuke gonuke force-pushed the update_build_test branch from 5d6f807 to bb054bc Compare July 20, 2023 13:45
@ahnaf-tahmid-chowdhury
Copy link
Member

Converted to draft to consider which GHCR repo is the right one for doing this testing...??? In its current form, this will pull/push directly to svalinn during testing and before merge, including overwriting the existing CI images. I'm not sure that's a good plan...

I think, it's a good practice to consider the appropriate branch (testing-ci-update) for testing changes, especially when it involves pushing docker images to GHCR. For this, we need to modify the workflow to push docker images to the appropriate GHCR repo, ensuring it doesn't overwrite the existing CI images used in svalinn.

@gonuke
Copy link
Member Author

gonuke commented Jul 22, 2023

Converted to draft to consider which GHCR repo is the right one for doing this testing...??? In its current form, this will pull/push directly to svalinn during testing and before merge, including overwriting the existing CI images. I'm not sure that's a good plan...

I think, it's a good practice to consider the appropriate branch (testing-ci-update) for testing changes, especially when it involves pushing docker images to GHCR. For this, we need to modify the workflow to push docker images to the appropriate GHCR repo, ensuring it doesn't overwrite the existing CI images used in svalinn.

We are somewhat constrained by the github action we are using. It is immensely valuable, but the author is open to our suggestions. I'm still feeling out the behavior to see what best to recommend for changes to that action

@gonuke
Copy link
Member Author

gonuke commented Aug 6, 2023

This is now waiting on the merge of #893 so that full testing of a revised workflow can be performed.

@gonuke gonuke force-pushed the update_build_test branch 2 times, most recently from 64512f1 to 9790dd0 Compare August 13, 2023 16:44
@gonuke
Copy link
Member Author

gonuke commented Aug 13, 2023

This will be ready for a full test after #894 is merged.

@gonuke
Copy link
Member Author

gonuke commented Aug 13, 2023

The linux tests passing here are building and testing against the newly generated docker images that will be more firmly codified by #894

@gonuke gonuke force-pushed the update_build_test branch from b7a24d0 to 14fab28 Compare August 13, 2023 20:47
@gonuke gonuke marked this pull request as ready for review August 13, 2023 20:52
@gonuke gonuke requested review from shimwell, bam241 and bquan0 August 13, 2023 21:03
@gonuke
Copy link
Member Author

gonuke commented Aug 13, 2023

For proof that this build process is using the new docker images, see the detailed test logs:
Under step "Initialize containers", substep "Starting job container", on line 16 you'll see reference to: ghcr.io/svalinn/dagmc-ci-ubuntu-20.04-gcc-ext-hdf5_1.10.4-moab_5.3.0/moab:latest which is identical to the image pushed by the most recent merge of the docker deployment system.

@gonuke
Copy link
Member Author

gonuke commented Aug 14, 2023

I updated the make parallelization per @shimwell 's suggestion. It did make the Ubuntu 20.04 clang build substantially faster, but the others were about the same. Ready for re-review (& merge?)

-DDOUBLE_DOWN=${double_down} \
-Ddd_ROOT=${double_down_install_dir} && \
make -j2 && \
make install
Copy link
Member

@shimwell shimwell Aug 14, 2023

Choose a reason for hiding this comment

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

Suggested change
make install
make install
PATH=${install_dir}/dagmc/bin:${PATH} CTEST_OUTPUT_ON_FAILURE=1 make test

is it worth testing here

Copy link
Member Author

Choose a reason for hiding this comment

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

It's in the next step of this job - do you think there is a reason to move it?

Copy link
Member

@shimwell shimwell Aug 15, 2023

Choose a reason for hiding this comment

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

I was thinking it could save a few lines of yaml, in particular that env varible called $GITHUB_WORKSPACE would not be needed

Copy link
Member

@shimwell shimwell 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, a few optional comments.
I like the idea of moving .sh scripts into the yaml files
left some optional comments but I think this is ready to merge

@shimwell
Copy link
Member

shimwell commented Aug 15, 2023

ok I've poked around a bit, but can't see anything to improve on this PR. Thanks for running through that with me Paul.

@shimwell shimwell merged commit 73ed77b into svalinn:develop Aug 15, 2023
@gonuke gonuke deleted the update_build_test branch August 21, 2023 00:11
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