Skip to content

Conversation

Edgar-21
Copy link
Contributor

I find myself sharing this dockerfile around, and figured it might be good to include it in a repository somewhere. It builds DAGMC, OpenMC, and PyNE together which may be useful for some folks. Additionally it includes the fusion-material-db since that's the reason I needed to have PyNE.

Copy link
Member

@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 looks good - might be good to add some documentation somewhere to describe why you are using this base image and not using conda :)

ENV EMBREE_INSTALL_DIR=$HOME/EMBREE/

# MOAB variables
ENV MOAB_TAG='5.3.0'
Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure we should be using a newer MOAB, no?

ENV DD_INSTALL_DIR=$HOME/Double_down

# DAGMC variables
ENV DAGMC_BRANCH='v3.2.3'
Copy link
Member

Choose a reason for hiding this comment

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

Probably want a newer DAGMC 3.2.4


# HDF5 stuff
ENV HDF5_BUILD_DIR=/root/build/hdf5
ENV HDF5_INSTALL_DIR=/opt/hdf5
Copy link
Member

Choose a reason for hiding this comment

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

Add an HDF5 version string as a variable as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated to use package manager install

ARG openmc_branch=develop
ENV OPENMC_REPO='https://github.com/openmc-dev/openmc'

ENV DAGMC_INSTALL_DIR=$HOME/DAGMC/
Copy link
Member

Choose a reason for hiding this comment

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

This is already defined above

ENV DAGMC_BRANCH='v3.2.3'
ENV DAGMC_REPO='https://github.com/svalinn/DAGMC'
ENV DAGMC_INSTALL_DIR=$HOME/DAGMC/

Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to add a block for OpenMC and PyNE here with relevant version strings, etc?

@Edgar-21
Copy link
Contributor Author

@anu1217 pointed out that mbconvert didn't work in this previously, updated with inspiration from @connoramoreno to get HDF5 from the debian package manager.

&& make 2>/dev/null -j${cores} install \
&& rm -rf ${DAGMC_INSTALL_DIR}/DAGMC ${DAGMC_INSTALL_DIR}/build

# clone and install openmc

Choose a reason for hiding this comment

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

Incredibly pedantic point: All the other comments are "Clone", but this one is "clone"

Copy link

@connoramoreno connoramoreno left a comment

Choose a reason for hiding this comment

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

Some general comments about documentation and organization

@@ -0,0 +1,142 @@
FROM debian:bookworm-slim

ENV cores=18

Choose a reason for hiding this comment

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

Should this be a build argument instead? Not guaranteed that all users will have 18 cores available

&& cd pymoab && bash install.sh \
&& python setup.py install \
&& python -c "import pymoab" \
&& ldconfig

Choose a reason for hiding this comment

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

Might be helpful to add a comment for what this line is useful for (for posterity)

Comment on lines 119 to 130
RUN mkdir -p ${HOME}/OpenMC && cd ${HOME}/OpenMC \
&& git clone --shallow-submodules --recurse-submodules --single-branch -b ${OPENMC_BRANCH} --depth=1 ${OPENMC_REPO} \
&& mkdir build && cd build \
&& cmake ../openmc \
-DCMAKE_CXX_COMPILER=mpicxx \
-DOPENMC_USE_MPI=on \
-DHDF5_PREFER_PARALLEL=on \
-DOPENMC_USE_DAGMC=ON \
-DCMAKE_PREFIX_PATH=${DAGMC_INSTALL_DIR} \
&& make 2>/dev/null -j${cores} install \
&& cd ../openmc && pip install .[test,depletion-mpi] \
&& python -c "import openmc"

Choose a reason for hiding this comment

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

Can help with organization to have multiple build layers, separating dependencies from core software

RUN ./pyne/scripts/nuc_data_make

RUN git clone https://github.com/svalinn/fusion-material-db.git
ENV PYTHONPATH=$PYTHONPATH:$HOME/opt/fusion-material-db/material-db-tools

Choose a reason for hiding this comment

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

Suggested change
ENV PYTHONPATH=$PYTHONPATH:$HOME/opt/fusion-material-db/material-db-tools
ENV PYTHONPATH=""
ENV PYTHONPATH=$PYTHONPATH:$HOME/opt/fusion-material-db/material-db-tools

Either define the variable before or get rid of the line

Copy link
Member

Choose a reason for hiding this comment

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

Yikes! Won't this remove default values of PYTHONPATH?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The PYTHONPATH is empty by default, docker gives a build warning about using an undefined variable as written, this suggestion is to resolve the warning. Another alternative would be to just do

ENV PYTHONPATH=$HOME/opt/fusion-material-db/material-db-tools

But I think I like defining PYTHONPATH as an empty string elsewhere so we can maintain the convention of appending all additions to PYTHONPATH to itself. Not a big deal since we only add one thing to it in this dockerfile, but if more things needed to be added it would be convenient to not worry about whether you are changing the first place it is declared.

Or we could just ignore the warning.

@FusionSandwich
Copy link

Just checking but we don't want to build the docker file with any cross sections?

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.

5 participants