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

Separate dockerfile and workflow for conda build [PART 2] #1540

Merged
merged 12 commits into from
Jun 14, 2024

Conversation

bquan0
Copy link
Contributor

@bquan0 bquan0 commented May 31, 2024

Description

This is a continuation of #1527, which was discontinued because we decided that we needed to update the versions of OpenMC, MOAB, and HDF5 for the apt build. This time, for the Conda build, we will not have separate stages for moab, dagmc, openmc, so there will only exist one image with all those dependencies.

Motivation and Context

This allows people to build PyNE using Conda, which is an alternative to using apt.
This PR will also be the new PR to close #1515.

Changes

This PR adds conda_build.dockerfile, which lists all the steps for building PyNE with Conda. It also adds conda_docker.yml, which runs the multistage build action on the dockerfile to build PyNE images with Conda.

Other Information

I'm not sure if the multistage action is relevant anymore because we only have one stage before the pyne stage (this is because we build everything but pyne in the first job, then build pyne in the BuildTest job). I guess it's still necessary for caching purposes in GHCR?

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.

This won't fix your errors, but will be important

docker/conda_build.dockerfile Show resolved Hide resolved
@bquan0
Copy link
Contributor Author

bquan0 commented Jun 7, 2024

I fixed the UnicodeDecodeError in the conda build by changing the Python version from 3.11 --> 3.10 because the apt build uses 3.10. I then re-added the change to test_mesh.py from the previous conda PR to fix another error. Now, both the conda and apt build are failing in the openmc stage due to a FileNotFoundError.

@ahnaf-tahmid-chowdhury
Copy link
Member

It seems alara_inp is missing in the test dir.

@gonuke
Copy link
Contributor

gonuke commented Jun 7, 2024

We need to update the OpenMC statepoint file in tests/files_test_r2s/r2s_examples/openmc_r2s.

Short term: update the statepoint version string in the H5 file and see if it works

Long term: create an OpenMC input that generates that statepoint file - or some replacement

@gonuke
Copy link
Contributor

gonuke commented Jun 7, 2024

@zxkjack123 - any chance you have the OpenMC input that generated the statepoint file for testing R2S? (see more above). We need to regenerate it with a newer version of OpenMC.

@bquan0
Copy link
Contributor Author

bquan0 commented Jun 7, 2024

I tried the short term solution of editing the statepoint file's version using h5py in a python command line. Looks like it worked!

There was another error for the conda build in test_chainsolve.py where dec couldn't be imported from numpy.testing because it was deprecated. I was thinking about downgrading the version of numpy for the conda build, but I looked at test_chainsolve.py and realized that it didn't even use dec, so I removed it.

Looks like all tests are passing now.

Copy link
Member

@ahnaf-tahmid-chowdhury ahnaf-tahmid-chowdhury left a comment

Choose a reason for hiding this comment

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

There are a few changes I would like to request.

docker/conda_build.dockerfile Show resolved Hide resolved
docker/conda_build.dockerfile Outdated Show resolved Hide resolved
tests/test_mesh.py Outdated Show resolved Hide resolved
docker/conda_build.dockerfile Outdated Show resolved Hide resolved
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.

Thanks @bquan0 - I'm glad all the tests pass now. After these changes, we'll need to look carefully through the test logs to make sure we're actually testing what we think and not getting false positives (which has been a problem in the past)

.github/workflows/conda_docker.yml Outdated Show resolved Hide resolved
docker/conda_build.dockerfile Outdated Show resolved Hide resolved
docker/conda_build.dockerfile Outdated Show resolved Hide resolved
docker/conda_build.dockerfile Outdated Show resolved Hide resolved
docker/conda_build.dockerfile Outdated Show resolved Hide resolved
docker/conda_build.dockerfile Outdated Show resolved Hide resolved
Comment on lines 85 to 86
ENV PYNE_MOAB_ARGS "--moab $HOME/opt/moab"
ENV PYNE_DAGMC_ARGS "--dagmc $HOME/opt/dagmc"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think conda installs these in this location. These are locations we use for manual install in the apt version, but conda will put these elsewhere. You might not need to specify a path?

Copy link
Contributor Author

@bquan0 bquan0 Jun 8, 2024

Choose a reason for hiding this comment

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

Originally, I didn't have these args, but in a previous comment you told me to set these args so that the install looks for the MOAB and DAGMC. I can try removing them and also changing them to the respective conda directory.

From what I can tell, the tests for DAGMC and MOAB still run regardless of whether I set them or not, but I'm not 100% sure.

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 you need these args, but either with no path or a different correct path. I think no path should be ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made that change and the tests are still passing (though I'm not sure if the DAGMC and MOAB tests are running)

docker/conda_build.dockerfile Show resolved Hide resolved
bquan0 and others added 2 commits June 8, 2024 15:06
@bquan0
Copy link
Contributor Author

bquan0 commented Jun 8, 2024

I've applied the suggestions except for the one about the MOAB and DAGMC build args for PyNE. @gonuke let me know what I should try for that.
All tests are passing after the changes.

@bquan0 bquan0 requested a review from gonuke June 8, 2024 21:32
@gonuke
Copy link
Contributor

gonuke commented Jun 14, 2024

I can see that this finds MOAB and DAGMC during build and also runs relevant tests, so I think it's all good

Copy link
Member

@ahnaf-tahmid-chowdhury ahnaf-tahmid-chowdhury left a comment

Choose a reason for hiding this comment

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

Thanks, @bquan0.

@ahnaf-tahmid-chowdhury ahnaf-tahmid-chowdhury merged commit 867c504 into develop Jun 14, 2024
27 checks passed
@gonuke
Copy link
Contributor

gonuke commented Jun 14, 2024

@bquan0 - this failed on merge :( with a segfault in Python????

@gonuke
Copy link
Contributor

gonuke commented Jun 14, 2024

@ahnaf-tahmid-chowdhury
Copy link
Member

Important note: Numpy v2 will be released on Jun 16 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.

conda CI build was never actually relying on conda
3 participants