search

svalinn / DAGMC

Direct Accelerated Geometry Monte Carlo Toolkit
https://svalinn.github.io/DAGMC
Other
96 stars 61 forks source link

Allow Double Down v1.1.0 Installation in Dockerfile #929

Closed ahnaf-tahmid-chowdhury closed 5 months ago

ahnaf-tahmid-chowdhury commented 7 months ago

Pull Request:

This is the upstream version of #928

Changes Made:

Details:

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 commented 7 months ago

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

gonuke commented 7 months ago

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 commented 7 months ago

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.

pshriwise commented 7 months ago

Seems we'll need this to fix this build error: https://github.com/svalinn/DAGMC/actions/runs/7250886790/job/19752059151

@ahnaf-tahmid-chowdhury we should also require a minimum double-down version of 1.1.0 in the CMake find_package command. Would you mind adding that requirement?

ahnaf-tahmid-chowdhury commented 7 months ago

Seems we'll need this to fix this build error: https://github.com/svalinn/DAGMC/actions/runs/7250886790/job/19752059151

The workflow seems to be old. Where DD is set as ON and using v1.0.0. In my version I am using a specific version 1.1.0. Or should i make a method to use the latest tag version when we set the value ON?

pshriwise commented 7 months ago

The workflow seems to be old. Where DD is set as ON and using v1.0.0. In my version I am using a specific version 1.1.0. Or should i make a method to use the latest tag version when we set the value ON?

that workflow is older than the changes in this PR, yes. It's from merging #913

gonuke commented 7 months ago

I think this PR is mixing a number of things so that it's hard to review. You have changed a lot of variable names that clutter and bury the substantive changes you've made in the workflow logic. Could you submit a separate PR that cleans up the variable names and then we can review the changes in the DD options by themselves?

ahnaf-tahmid-chowdhury commented 7 months ago

In this PR I haven't only added dd version, but also add a method to make a docker image for each version of dd as geant4 do. Also, I have created a method that allows not to create docker image when we set dd version to OFF. Furthermore, I have made the same logic for geant4 as it seems it is also a dependency for DAGMC.

Docker images will be created as

docker-ci-[Ubuntu with version]-[g++/clang]-[hdf5 with version]-[moab with version] # Geant4=OFF DD=OFF
docker-ci-[Ubuntu with version]-[g++/clang]-[hdf5 with version]-[moab with version]-[geant4 with version] # DD=OFF
docker-ci-[Ubuntu with version]-[g++/clang]-[hdf5 with version]-[moab with version]-[dd with version] # Geant4=OFF
docker-ci-[Ubuntu with version]-[g++/clang]-[hdf5 with version]-[moab with version]-[geant4 with version]-[dd with version]

I have also made a method to set build automatically ON for Geant4 and DD if the version is declaired.

gonuke commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 6 months ago

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 commented 5 months ago

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

ahnaf-tahmid-chowdhury commented 5 months ago

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

gonuke commented 5 months ago

Needs a rebase following the recent housekeeping update

gonuke commented 5 months ago

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