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

Update current apt build #1529

Merged
merged 20 commits into from
Apr 29, 2024
Merged

Update current apt build #1529

merged 20 commits into from
Apr 29, 2024

Conversation

bquan0
Copy link
Contributor

@bquan0 bquan0 commented Apr 11, 2024

Description

This PR updates the versions of OpenMC, MOAB, and hdf5 that are built in docker_publish.yml and ubuntu_22.04-dev.dockerfile. It also removes the build without hdf5 so that we always build hdf5. It also doesn't download the apt dependencies libhdf5-dev and hdf5-tools so that they come directly from the hdf5 build and are the most up-to-date version.

Motivation and Context

There are new versions of MOAB and OPENMC that we should use. Also, linux is slow to update hdf5, so we should always build it so we have the most recent version.

Behavior

Previously, we had one build with hdf5 and one build without. Now, we will always build with hdf5. This means removing a bunch of the matrices in docker_publish.yml used for having an hdf5 build and a no-hdf5 build. I also removed the build arg that allows docker_publish.yml to specify which version of hdf5 to build. Now, it's just hardcoded into the dockerfile in the hdf5 build section.

The version changes are:

  • hdf5 v1.12.0 --> v1.14.3
  • MOAB v5.3.0 --> v5.5.1
  • OpenMC v0.13.0 --> v0.14.0

Copy link
Contributor

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

I think this comment will fix your failed runs.

docker/ubuntu_22.04-dev.dockerfile Outdated Show resolved Hide resolved
@bquan0
Copy link
Contributor Author

bquan0 commented Apr 12, 2024

@gonuke now that we are using OpenMC v0.14.0, we need to regenerate the statepoint file. (see forum post) How should I go about this? This is currently causing the OpenMC file to fail.

Also, we had to go back to installing MOAB v5.3.0 because v5.5.0 uses Cython3, and we don't have the Cython3 compatibility fix in PyNE yet. (This was causing the MOAB, DAGMC, and OpenMC tests to fail). We will have to update MOAB in the same (future) PR where we update to Cython3 in PyNE.

@gonuke
Copy link
Contributor

gonuke commented Apr 14, 2024

What's the prospect of updating everything to Cython3 as part of this process, rather than the other way around?

@ahnaf-tahmid-chowdhury
Copy link
Member

What's the prospect of updating everything to Cython3 as part of this process, rather than the other way around?

Sure, we can. Only thing we need to do is merging cython3-upgrade branch into our current setup.

P.S. Apologies for my recent inactivity; I'm currently on leave.

@bquan0
Copy link
Contributor Author

bquan0 commented Apr 15, 2024

I rebased my branch onto the cython3-update branch.

There were tests that were failing in test_material.py related to how Cython3 updates operator overloading to be more like Python. I have fixed them by adding radd and rmul in material.pyx. Also, I would like to note that there is no rdiv test.

As for the moab, dagmc, openmc, tests, they were failing due to the same ValueError: Cannot create cython.array from NULL pointer in test_response_hdf5_to_mesh() that we've seen quite a few times now.

RUN cd $HOME/opt \
&& mkdir hdf5 \
&& cd hdf5 \
&& git clone --single-branch --branch hdf5-1_14_3 https://github.com/HDFGroup/hdf5.git \
Copy link
Contributor

Choose a reason for hiding this comment

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

shouild this line make use of the ARG build_hdf5 value

Suggested change
&& git clone --single-branch --branch hdf5-1_14_3 https://github.com/HDFGroup/hdf5.git \
&& git clone --single-branch --branch $build_hdf5 https://github.com/HDFGroup/hdf5.git \

&& mkdir moab \
&& cd moab \
&& git clone --depth 1 --single-branch -b 5.3.0 https://bitbucket.org/fathomteam/moab \
&& git clone --depth 1 --single-branch -b 5.5.1 https://bitbucket.org/fathomteam/moab \
Copy link
Contributor

Choose a reason for hiding this comment

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

we could make the moab version a ARG which defaults to 5.5.1

git clone https://github.com/openmc-dev/openmc.git $HOME/opt/openmc \
&& cd $HOME/opt/openmc \
&& git checkout tags/v0.13.0 \
&& git checkout tags/v0.14.0 \
Copy link
Contributor

Choose a reason for hiding this comment

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

We could make the HDF5 version an ARG which defaults to 0.14.0

Copy link
Contributor

Choose a reason for hiding this comment

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

This is an openmc version, but yes, your point is well-taken

RUN if [ "$build_hdf5" != "NO" ]; then \
export HDF5_ROOT="$HDF5_INSTALL_PATH" ; \
fi ;\
RUN export HDF5_ROOT="$HDF5_INSTALL_PATH" ; \
git clone https://github.com/openmc-dev/openmc.git $HOME/opt/openmc \
Copy link
Contributor

Choose a reason for hiding this comment

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

we can clone the specific tag release instead of cloning the repo and checking out the tag

Copy link
Contributor

Choose a reason for hiding this comment

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

git clone --depth 1 --branch ${openmc_version} https://github.com/openmc-dev/openmc.git $HOME/opt/openmc \

@gonuke
Copy link
Contributor

gonuke commented Apr 25, 2024

I've issued a PR in MOAB to hopefully fix this problem.

@gonuke
Copy link
Contributor

gonuke commented Apr 25, 2024

Alternatively, we may need to try and catch an exception at line 434 in pyne/mesh.py. It seems that this method will throw an exception if the default value is None/NULL. I'm not sure why we aren't seeing that instead of this error.

@gonuke
Copy link
Contributor

gonuke commented Apr 25, 2024

Alternatively, we may need to try and catch an exception at line 434 in pyne/mesh.py. It seems that this method will throw an exception if the default value is None/NULL. I'm not sure why we aren't seeing that instead of this error.

But first... let's update to the newest MOAB (5.5.1) to see if/how this error presents itself.

@bquan0
Copy link
Contributor Author

bquan0 commented Apr 26, 2024

But we were already building and testing moab 5.5.1, ever since this commit. I'll test the try catch solution you mentioned

@bquan0
Copy link
Contributor Author

bquan0 commented Apr 26, 2024

Seems like the try except has fixed the moab and dagmc tests. The openmc tests are still failing because the statepoint file needs to be regenerated.

@shimwell
Copy link
Contributor

I see the 5 year old statepoint file on the repo but I don't see any scripts that generated it. I also looked for (but couldn't see) a summary.h5 file in case it was present as I think that contains the openmc model so could e helpful to regenerate the statepoint.

@gonuke
Copy link
Contributor

gonuke commented Apr 26, 2024

But we were already building and testing moab 5.5.1, ever since this commit. I'll test the try catch solution you mentioned

Sorry - my bad - I was looking at the comment by @shimwell and misinterpreted it as suggested change.

Copy link
Contributor

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

Clarification on value of mesh tag default when ValueErrror

pyne/mesh.py Outdated
try:
self.default = self.tag.get_default_value()
except ValueError:
self.default = int
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be None

@gonuke
Copy link
Contributor

gonuke commented Apr 26, 2024

I see the 5 year old statepoint file on the repo but I don't see any scripts that generated it. I also looked for (but couldn't see) a summary.h5 file in case it was present as I think that contains the openmc model so could e helpful to regenerate the statepoint.

We may need to work backwards from the things that are tested in tests/test_openmc_utils.py:
Tally ID=1

  • has ebounds = [0.0, 1.0, 2.0] * 1e6
  • is a structured mesh tally of size 3x2x1 on domain (-40, -12.5, -2.5) to (80, 25, 5) that scores flux
  • the values of the results don't really matter since the test uses openmc directly to evaluate the tally and compares to PyNE's evaluation

I think those are the only things that are true.

We should make an openmc python script that will generate something like this and add it to the repo. That sounds a like something that @shimwell would be great at :)

@shimwell
Copy link
Contributor

I see the 5 year old statepoint file on the repo but I don't see any scripts that generated it. I also looked for (but couldn't see) a summary.h5 file in case it was present as I think that contains the openmc model so could e helpful to regenerate the statepoint.

We may need to work backwards from the things that are tested in tests/test_openmc_utils.py: Tally ID=1

  • has ebounds = [0.0, 1.0, 2.0] * 1e6
  • is a structured mesh tally of size 3x2x1 on domain (-40, -12.5, -2.5) to (80, 25, 5) that scores flux
  • the values of the results don't really matter since the test uses openmc directly to evaluate the tally and compares to PyNE's evaluation

I think those are the only things that are true.

We should make an openmc python script that will generate something like this and add it to the repo. That sounds a like something that @shimwell would be great at :)

I have had a go at this in PR #1533

@bquan0
Copy link
Contributor Author

bquan0 commented Apr 28, 2024

After making these 2 changes to generate_statepoint.py:

  • change the upper bounds of the rectangular prism from (80, 25, 5), to (40, 12.5, 5) to match what the tests were expecting. This is because (80, 25, 5) are added to the lower bounds of (-40, -12.5, -2.5).
  • change 2e6 in energy_bins to 2e7 because the test expects 20e6 and not 2e6.

and regenerating the statepoint file, the openmc test is passing. There are 4 openmc BuildTests which are not passing but that's to be expected since they are testing on images built with version 0.13.0, and we have updated to version 0.14.0

@gonuke I think this is ready to be merged now since all the relevant tests are passing.

@gonuke
Copy link
Contributor

gonuke commented Apr 28, 2024

Just to summarize my understanding of why the remaining openmc build tests are failing, and for posterity:

  • this PR is largely designed to update the CI image which normally would not run build tests on old images
  • because of the Cython updates, it also includes code changes which cause the build tests to run, and run on old images
  • the new images work with the newer OpenMC statepoint file
  • we expect the build tests to pass once the CI images have been updated

Copy link
Contributor

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

Please incorporate @shimwell 's suggestions of using more build-args w/ default values for the Dockerfile, and cloning single branches.

@bquan0
Copy link
Contributor Author

bquan0 commented Apr 29, 2024

@gonuke I just applied @shimwell's suggestions, though I'm not sure if they are necessary. The build-args for moab and openmc are each used in one spot for the dockerfile, and right now we aren't using the github workflow to build images with different versions of moab and openmc. If we want to in the future, this change could be added then, but as of right now it's doing nothing.

I also updated build_test.yml and its composite action action.yml to test the new images that we are building.

@gonuke
Copy link
Contributor

gonuke commented Apr 29, 2024

Thanks @bquan0 - let's go forward with this and see how it all goes once the images are updated

@gonuke gonuke merged commit d334cbd into develop Apr 29, 2024
19 of 21 checks passed
@gonuke gonuke mentioned this pull request May 5, 2024
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.

None yet

6 participants