Allow Double Down v1.1.0 Installation in Dockerfile #929

ahnaf-tahmid-chowdhury · 2023-12-09T02:03:52Z

Pull Request:

This is the upstream version of #928

Changes Made:

Enabled Double Down v1.1.0 to be installed in the Dockerfile.
Removed double_down from the action and added double_down_version. In the past, the build process used double_down=ON/OFF to build DAGMC, but now DAGMC will default to using Double Down. The version can be specified using double_down_version in the actions.

Details:

The Dockerfile has been updated to support the installation of Double Down v1.1.0.
Removed the double_down variable from the action and introduced double_down_version for specifying the version during the actions.
Previously, DAGMC was built using double_down=ON/OFF, but now it defaults to using Double Down.
Various variables have been adjusted for consistency and improved readability.

This PR streamlines the installation process, makes the Dockerfile compatible with Double Down v1.1.0, and provides flexibility in choosing the version in the actions.

ahnaf-tahmid-chowdhury · 2023-12-09T06:33:33Z

Not sure why Linux Build/Test is failing. It should use the latest docker image.

pshriwise

A couple of really tiny nit picks here @ahnaf-tahmid-chowdhury. Thanks for updating the CI variables/Docker build args to use a consistent syntax. I think that's a nice improvement.

I'll let @gonuke chime in on using double-down by default. My take is that we'd want to leave the default set to MOAB since double-down is an optional dependency of DAGMC.

.github/workflows/linux_upstream_test_double_down.yml

.github/workflows/linux_upstream_test_moab.yml

Co-authored-by: Patrick Shriwise <[email protected]>

gonuke

A couple of additional changes requested.

CI/Dockerfile

gonuke · 2023-12-11T03:29:10Z

I'm happy to revisit the plan for testing with Double Down. As @pshriwise says above, it is an optional dependency. Therefore, we should definitely be testing without it. What is not clear is whether we want to require that every change builds successfully with DD. We could, but perhaps not the full matrix - just add it as one or two special cases.

ahnaf-tahmid-chowdhury · 2023-12-11T08:09:03Z

Hi @gonuke and @pshriwise,

It seems that both Double Down and Geant4 are now optional for DAGMC. I have made some changes in this version. If the geant4_version or double_down_version is not set, workflow will automatically disable their build. Additionally, Docker images will be created with their names appended to the version string.

Since I've made several changes, please review from the beginning.

gonuke · 2023-12-23T15:01:11Z

I've just noticed a couple of concepts that have been forgotten in this update.

Even though GEANT4 is an optional dependency, we always want to test with GEANT4 in CI. There is no reason not to. It should never be off. It also leads to needing fewer Docker images in our container registry.

Also, Embree was always installed as part of the external dependencies for a few reasons, and independent of whether or not Double Down is being tested. Again, since this is for CI, there is reason not to have Embree always installed. If always installing it, it is best to do it early in the build since it changes least often and then the Docker cache doesn't have to rebuild it if/when MOAB changes.

It is easy to forget these things or not realize why they are true, so I suppose I should leave more comments for some of these subtle decisions.

ahnaf-tahmid-chowdhury · 2023-12-23T15:25:00Z

If it is necessary to build Geant4 every time, we can keep this always on. Moreover, with this update we can use both mode with matrix. Same rule for Double Down.

If Embree is always required to be installed (optional part of DAGMC) we can make remove the current condisions, also, we can make similar method for Embree as we have done for Geant4 and Double Down.

gonuke · 2023-12-23T18:59:18Z

I think it makes sense to always build GEANT4, Embree & Double Down, but we don't always need to test with them. For CI purposes, there is no down side to having the maximum number of dependencies installed, and then use them in different ways during testing.

ahnaf-tahmid-chowdhury · 2023-12-23T19:17:47Z

As I have already mentioned, the current update can let us do both. Yes, this approach is using some extra conditions, but it may cost an unnoticeable time to process, indeed it may speed up the build process in some cases. However, if you are satisfied with extra builds, I can remove all the conditions and may be hardcoded the OFF value to the DAGMC build.

gonuke

I think this still me need a rebase at least for the CHANGELOG?

gonuke · 2023-12-23T20:35:16Z

doc/CHANGELOG.rst

@@ -7,9 +7,13 @@ DAGMC Changelog
 Next version
 ====================

+v3.2.3


Something looks wrong here relative to the released version v3.2.3 - maybe needs a rebase?

gonuke · 2023-12-23T20:37:39Z

As I have already mentioned, the current update can let us do both. Yes, this approach is using some extra conditions, but it may cost an unnoticeable time to process, indeed it may speed up the build process in some cases. However, if you are satisfied with extra builds, I can remove all the conditions and may be hardcoded the OFF value to the DAGMC build.

Maybe it is OK in its current form. The only issue I'm not sure about is that we only build Embree when DD is on, and we do so after MOAB. If we did it always (whether or not DD is on) as part of the external dependencies (e.g. near GEANT or HDF5) it would make the test for DD faster because it would not always have to also build Embree.

ahnaf-tahmid-chowdhury · 2023-12-24T13:26:07Z

Actually, Embree will be built only once for the docker tag that contains double_down_[version] (different tag). It will not be built every time if we do not modify anything in the Docker file. Even if we change the version of Double Down, it will not be built, as it comes before Double Down. When considering the order of dependencies we build first, we may need to take into account which of them generally gets updated often. So, let me know how do I reorder them.

ahnaf-tahmid-chowdhury · 2024-01-11T15:16:24Z

Is there anything more to change on this? Or, I think @gonuke is awaiting @pshriwise's response on using find_package through CMake to specify the minimum version of DD.

ahnaf-tahmid-chowdhury · 2024-02-14T15:01:07Z

@gonuke, do you like to enable testing for DD v1.1.0 as you have mentioned earlier?

gonuke

One more change and still waiting on @pshriwise to chime in

CI/Dockerfile

gonuke · 2024-02-16T17:44:42Z

cmake/DAGMC_macros.cmake

@@ -90,6 +90,10 @@ macro (dagmc_setup_options)

 if (DOUBLE_DOWN)
  find_package(DOUBLE_DOWN REQUIRED)


ping @pshriwise

ahnaf-tahmid-chowdhury · 2024-02-16T18:23:59Z

@gonuke, Please note DD tests are turned off. Let me know, if you like to turn this on to v1.1.0

gonuke

We've been churning away at this for a while, so it probably makes sense to recap what the goals of this PR are:

Building CI testing images
- install a valid version of Double Down in all CI testing images - currently v1.1.0 but NOT v1.0.0
- test the CI testing images with DAGMC both with DD and without
testing for each PR
- test with DD and without for each PR
testing for merge
- test DD develop branch upon merge as advisory

In addition, there is a lot of cleanup of build-arg variables and other variables.

Given all this, in addition to the code comments, I think we should turn on DD testing in linux_build_test.yml

gonuke · 2024-02-21T14:44:48Z

.github/workflows/docker_publish.yml

          10.7.4,
          11.1.2
        ]
+        double_down_version : [
+          v1.0.0,


We know this version fails, so we should remove it.

We may also want to add off to this list so we can test without DD.

I have added this to check if CMake 3.16 shows any error messages since it is lower than 1.1.0, but it just passed. That means, CMake will also get failed for Geant4 as well.

gonuke · 2024-02-21T14:45:22Z

.github/workflows/docker_publish.yml

          10.7.4,
          11.1.2
        ]
+        double_down_version : [
+          off


This matrix should probably match the matrix above to make sure all the right images exist.

…ansion

gonuke · 2024-02-22T11:46:48Z

Needs a rebase following the recent housekeeping update

gonuke · 2024-02-22T12:11:51Z

I think we've addressed your comment @pshriwise and are ready to merge

pshriwise

Thanks for your effort here @ahnaf-tahmid-chowdhury! And thanks to @gonuke for the thorough discussion.

gonuke · 2024-02-22T13:53:19Z

.github/actions/upstream-test/action.yml

@@ -43,11 +43,24 @@ runs:
      - name: Installing Dependencies in Docker image
        uses: firehed/multistage-docker-build-action@v1
        with:
-          repository: ghcr.io/${{ github.repository_owner }}/dagmc-ci-ubuntu-${{ inputs.ubuntu_version }}-${{ inputs.compiler}}-geant4_${{ inputs.geant_version }}-hdf5_${{ inputs.hdf5_version }}-moab_${{ inputs.moab_version }}
+          repository: >


@ahnaf-tahmid-chowdhury : This change introduced a failure - you can’t split this across multiple lines - that we didn’t review carefully enough in the PR. Please submit an update to fix this.

Let's wait for all the new Docker images to finish building, then we can rerun the Linux tests to see what happens.

I looked at the failed logs and saw that the failure is because this multi-line container name is resolving with spaces in the name, so it definitely needs to be fixed.

ahnaf-tahmid-chowdhury added 4 commits December 8, 2023 23:38

dd fix

6166e4f

fix Geant4_VERSION

d057970

typo fix

3cb814f

dd version 1.1.0

0b12126

ahnaf-tahmid-chowdhury added the build & CI label Dec 9, 2023

ahnaf-tahmid-chowdhury added 4 commits December 9, 2023 08:07

Double Down v1.1.0 Installation in Dockerfile

7d8de72

DOUBLE_DOWN_VERSION string fix

91245ea

add space

b9c88b6

fix DOUBLE_DOWN_VERSION branch for develop

26ad7e3

ahnaf-tahmid-chowdhury requested review from gonuke and pshriwise December 10, 2023 15:32

pshriwise requested changes Dec 10, 2023

View reviewed changes

.github/workflows/linux_upstream_test_double_down.yml Outdated Show resolved Hide resolved

.github/workflows/linux_upstream_test_moab.yml Outdated Show resolved Hide resolved

pshriwise mentioned this pull request Dec 10, 2023

Double Down ci fix #928

Closed

ahnaf-tahmid-chowdhury and others added 2 commits December 11, 2023 09:15

Update name string

2dfa97b

Co-authored-by: Patrick Shriwise <[email protected]>

Update name string

2a1133e

Co-authored-by: Patrick Shriwise <[email protected]>

gonuke requested changes Dec 11, 2023

View reviewed changes

CI/Dockerfile Show resolved Hide resolved

CI/Dockerfile Outdated Show resolved Hide resolved

allow to chose building geant4 and dd from actions

fca55ad

double_down default OFF

13efae6

ahnaf-tahmid-chowdhury marked this pull request as draft December 11, 2023 09:46

ahnaf-tahmid-chowdhury added 4 commits December 11, 2023 21:55

build Geant4 or DD if enabled

ff3ea55

fix ${{ env.image_base_tag }}

78c2ac2

fix syntax error

91d51b0

fix syntex

037e6fc

ahnaf-tahmid-chowdhury marked this pull request as ready for review December 11, 2023 17:21

ahnaf-tahmid-chowdhury requested a review from gonuke December 12, 2023 04:51

ahnaf-tahmid-chowdhury added 2 commits December 12, 2023 11:34

Merge branch 'develop' into dd-ci-fix

c3136a9

add more conditions to container

42a6ab5

gonuke requested changes Dec 23, 2023

View reviewed changes

ahnaf-tahmid-chowdhury mentioned this pull request Feb 6, 2024

Update Double Down to 1.1.0 #932

Closed

Merge remote-tracking branch 'upstream/develop' into dd-ci-fix

36adea7

gonuke requested changes Feb 16, 2024

View reviewed changes

move embree to external_deps stage

5f844bc

ahnaf-tahmid-chowdhury added 2 commits February 21, 2024 10:54

find_package to set min version

7bd408c

embre 3.6.1

b2609ff

gonuke requested changes Feb 21, 2024

View reviewed changes

ahnaf-tahmid-chowdhury added 3 commits February 22, 2024 00:14

set min version of DD and Geant4 in legacy way

61dff56

if statements in CMake do not require the use of ${} for variable exp…

abe00a4

…ansion

Merge branch 'develop' into dd-ci-fix

0097561

Merge branch 'develop' into dd-ci-fix

adcebb2

gonuke approved these changes Feb 22, 2024

View reviewed changes

pshriwise approved these changes Feb 22, 2024

View reviewed changes

gonuke merged commit bdd5b79 into develop Feb 22, 2024
20 checks passed

ahnaf-tahmid-chowdhury deleted the dd-ci-fix branch February 22, 2024 13:41

gonuke reviewed Feb 22, 2024

View reviewed changes

ahnaf-tahmid-chowdhury restored the dd-ci-fix branch February 22, 2024 15:05

ahnaf-tahmid-chowdhury added a commit that referenced this pull request Feb 22, 2024

Allow Double Down v1.1.0 Installation in Dockerfile (#929 #944)

0c6d98a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Double Down v1.1.0 Installation in Dockerfile #929

Allow Double Down v1.1.0 Installation in Dockerfile #929

ahnaf-tahmid-chowdhury commented Dec 9, 2023

ahnaf-tahmid-chowdhury commented Dec 9, 2023

pshriwise left a comment

gonuke left a comment

gonuke commented Dec 11, 2023

ahnaf-tahmid-chowdhury commented Dec 11, 2023

gonuke commented Dec 23, 2023

ahnaf-tahmid-chowdhury commented Dec 23, 2023

gonuke commented Dec 23, 2023

ahnaf-tahmid-chowdhury commented Dec 23, 2023

gonuke left a comment

gonuke Dec 23, 2023

gonuke commented Dec 23, 2023

ahnaf-tahmid-chowdhury commented Dec 24, 2023

ahnaf-tahmid-chowdhury commented Jan 11, 2024

ahnaf-tahmid-chowdhury commented Feb 14, 2024

gonuke left a comment

gonuke Feb 16, 2024

ahnaf-tahmid-chowdhury commented Feb 16, 2024

gonuke left a comment

gonuke Feb 21, 2024

ahnaf-tahmid-chowdhury Feb 21, 2024

gonuke Feb 21, 2024

gonuke commented Feb 22, 2024

gonuke commented Feb 22, 2024

pshriwise left a comment

gonuke Feb 22, 2024

ahnaf-tahmid-chowdhury Feb 22, 2024

gonuke Feb 22, 2024

		@@ -90,6 +90,10 @@ macro (dagmc_setup_options)

		if (DOUBLE_DOWN)
		find_package(DOUBLE_DOWN REQUIRED)

Allow Double Down v1.1.0 Installation in Dockerfile #929

Allow Double Down v1.1.0 Installation in Dockerfile #929

Conversation

ahnaf-tahmid-chowdhury commented Dec 9, 2023

Pull Request:

Changes Made:

Details:

ahnaf-tahmid-chowdhury commented Dec 9, 2023

pshriwise left a comment

Choose a reason for hiding this comment

gonuke left a comment

Choose a reason for hiding this comment

gonuke commented Dec 11, 2023

ahnaf-tahmid-chowdhury commented Dec 11, 2023

gonuke commented Dec 23, 2023

ahnaf-tahmid-chowdhury commented Dec 23, 2023

gonuke commented Dec 23, 2023

ahnaf-tahmid-chowdhury commented Dec 23, 2023

gonuke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gonuke commented Dec 23, 2023

ahnaf-tahmid-chowdhury commented Dec 24, 2023

ahnaf-tahmid-chowdhury commented Jan 11, 2024

ahnaf-tahmid-chowdhury commented Feb 14, 2024

gonuke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahnaf-tahmid-chowdhury commented Feb 16, 2024

gonuke 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

gonuke commented Feb 22, 2024

gonuke commented Feb 22, 2024

pshriwise 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