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.

Sign up for GitHub

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

Jump to bottom

Add controls for numpy version #678

Merged
merged 5 commits into from
Jul 29, 2024
Merged

Add controls for numpy version #678

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6d02a07
Control the version of numpy used in workers.
robertbartel Jul 17, 2024
22b308b
Update example.env with numpy version constraint.
robertbartel Jul 17, 2024
318afe5
Update requirements files for numpy constraint.
robertbartel Jul 17, 2024
d302fc3
Roll back initial #678 changes.
robertbartel Jul 18, 2024
74f7759
Use pip constraints file in ngen Dockerfile.
robertbartel Jul 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions docker/main/docker-build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ services:
context: ./ngen
target: rocky-ngen-deps
args:
NUMPY_CONSTRAINT: ${NUMPY_INSTALL_CONSTRAINT:?Please set global NUMPY_INSTALL_CONSTRAINT in environment config}
robertbartel marked this conversation as resolved.
Show resolved Hide resolved
DOCKER_INTERNAL_REGISTRY: ${DOCKER_INTERNAL_REGISTRY:?}
#depends_on:
# - base
Expand All @@ -59,6 +60,7 @@ services:
context: ./ngen
target: rocky_ngen_build_testing
args:
NUMPY_CONSTRAINT: ${NUMPY_INSTALL_CONSTRAINT:?Please set global NUMPY_INSTALL_CONSTRAINT in environment config}
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 abuse of the args parameter. Do not use this to inject python versions. Stick to setup.py and requirements.txt.

REPO_URL: ${NGEN_REPO_URL?No NGen repo url configured}
BRANCH: ${NGEN_BRANCH?No NGen branch configured}
COMMIT: ${NGEN_COMMIT}
Expand All @@ -77,6 +79,7 @@ services:
context: ./ngen
target: ngen_worker
args:
NUMPY_CONSTRAINT: ${NUMPY_INSTALL_CONSTRAINT:?Please set global NUMPY_INSTALL_CONSTRAINT in environment config}
robertbartel marked this conversation as resolved.
Show resolved Hide resolved
REPO_URL: ${NGEN_REPO_URL?No NGen repo url configured}
BRANCH: ${NGEN_BRANCH?No NGen branch configured}
COMMIT: ${NGEN_COMMIT}
Expand All @@ -98,6 +101,7 @@ services:
context: ./ngen
target: partitioner_image
args:
NUMPY_CONSTRAINT: ${NUMPY_INSTALL_CONSTRAINT:?Please set global NUMPY_INSTALL_CONSTRAINT in environment config}
robertbartel marked this conversation as resolved.
Show resolved Hide resolved
REPO_URL: ${NGEN_REPO_URL?No NGen repo url configured}
BRANCH: ${NGEN_BRANCH?No NGen branch configured}
COMMIT: ${NGEN_COMMIT}
Expand All @@ -114,6 +118,7 @@ services:
context: ./ngen
target: ngen_cal_worker
args:
NUMPY_CONSTRAINT: ${NUMPY_INSTALL_CONSTRAINT:?Please set global NUMPY_INSTALL_CONSTRAINT in environment config}
robertbartel marked this conversation as resolved.
Show resolved Hide resolved
REPO_URL: ${NGEN_REPO_URL?No NGen repo url configured}
BRANCH: ${NGEN_BRANCH?No NGen branch configured}
COMMIT: ${NGEN_COMMIT}
Expand Down
31 changes: 27 additions & 4 deletions docker/main/ngen/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,11 @@ ARG BOOST_VERSION=1.82.0
#mpich 3.2 doesn't work well gfortran 11 it seems, an alignment error crops up, but 3.3.2 seems to work...
ARG MPICH_VERSION="3.3.2"
ARG MIN_PYTHON="3.8.0"
ARG MIN_NUMPY="1.18.0"
ARG BLOSC2_VERSION

# Necessry to control specific numpy version after release of 2.0 and prior to fully support by all NextGen ecosystem
ARG NUMPY_CONSTRAINT

robertbartel marked this conversation as resolved.
Show resolved Hide resolved
ARG NETCDF_C_VERSION=4.8.1
ARG NETCDF_CXX_VERSION=4.3.1
ARG NETCDF_FORTRAN_VERSION=4.6.0
Expand Down Expand Up @@ -290,14 +292,15 @@ RUN if [[ "${NETCDF_CXX_VERSION}" == "latest" ]]; then \
FROM rocky-base as rocky-ngen-packaged-deps

ARG ROCKY_NGEN_DEPS_REQUIRED
ARG NUMPY_CONSTRAINT
robertbartel marked this conversation as resolved.
Show resolved Hide resolved

# TODO: later, go back and change all pip3/python3 to just pip/python (but leave for now to limit scope)
# Note that this includes numpy, which is needed for Python BMI support, regardless of BMI module
USER root
Copy link
Member

Choose a reason for hiding this comment

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

I figure we could move the required python packages to a requirements.txt file (so, pip, wheel, packaging, and numpy) and then create a constraints.txt that we use to enforce the allowed versions of the required and other packages in constraints.txt. Thoughts?

Suggested change
USER root
USER root
COPY requirements.txt ${WORKDIR}/requirements.txt
COPY constraints.txt ${WORKDIR}/constraints.txt

Copy link
Contributor

Choose a reason for hiding this comment

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

Good find! I think it's a good solution.

It's been a while since I've had to worry about this, but I believe that editing the constraints.txt and creating a new hash for the file would cause a new docker build to start at the line for the constraints.txt file , so I think we can be sure that a change there will just about always be reflected after a build.

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 still don't like the idea of coupling all the Python packages together in a requirements.txt, as I don't think that fits with how the Dockerfile is set up. It's totally fair to discuss whether that needs to change - e.g., should we really support building the image both with or without Python support (we don't really need numpy if in the without option)) - but it needs to be discussed separately.

But the PIP_CONSTRAINT and constraints.txt settings sound fine. Those alone should be sufficient to resolve the immediate issue.

Copy link
Contributor Author

@robertbartel robertbartel Jul 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@christophertubbs, you raise a good point I hadn't consider: depending on when the constraints.txt file is setup in the image that will once again have implications on build caching and efficiency. That isn't an absolute blocker, but I want to think on it a little more to see if there's anyway to avoid that tradeoff. For the sake of getting the immediate issue fixed, I'm going to go ahead and prepare the changes to go this route, but I'm not sure I want it to be permanent just yet. But that can be spun off to a separate issue and dealt with later.

Copy link
Member

@aaraney aaraney Jul 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I still don't like the idea of coupling all the Python packages together in a requirements.txt, as I don't think that fits with how the Dockerfile is set up.

Yeah, I agree. Im just talking about the required packages to build ngen with python support. So, numpy, pip, wheel and packaging for now. It just seems like a nice place to keep those baseline required packages rather than inlining them in the Dockerfile.

RUN dnf update -y && dnf install -y ${ROCKY_NGEN_DEPS_REQUIRED} && dnf clean -y all \
&& ln -s $(which python3) $(which python3 | sed 's/python3/python/') \
&& pip install --no-cache-dir "pip>=23.0,<23.1" wheel packaging \
&& if [ "${NGEN_WITH_PYTHON}" == "ON" ]; then pip install --no-cache-dir numpy; fi
&& if [ "${NGEN_WITH_PYTHON}" == "ON" ]; then pip install --no-cache-dir numpy${NUMPY_CONSTRAINT}; fi
robertbartel marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

This would become:

Suggested change
&& if [ "${NGEN_WITH_PYTHON}" == "ON" ]; then pip install --no-cache-dir numpy${NUMPY_CONSTRAINT}; fi
&& if [ "${NGEN_WITH_PYTHON}" == "ON" ]; then pip install --no-cache-dir --requirement requirements.txt --constraint constraints.txt ; fi

USER ${USER}

################################################################################################################
Expand Down Expand Up @@ -332,7 +335,6 @@ COPY --chown=${USER} --from=download_boost /boost ${WORKDIR}/boost

ARG MPICH_VERSION
ARG MIN_PYTHON
ARG MIN_NUMPY
ARG BLOSC2_VERSION
ARG ROCKY_NGEN_DEPS_REQUIRED

Expand Down Expand Up @@ -652,6 +654,8 @@ ARG BUILD_TOPMODEL
ARG BUILD_PET
ARG BUILD_SLOTH

ARG NUMPY_CONSTRAINT
robertbartel marked this conversation as resolved.
Show resolved Hide resolved

COPY --chown=${USER} --from=rocky_init_repo ${WORKDIR}/ngen ${WORKDIR}/ngen
COPY --chown=${USER} --from=rocky_build_troute ${WORKDIR}/t-route/wheels /tmp/t-route-wheels
COPY --chown=${USER} --from=rocky_build_troute ${WORKDIR}/t-route/src/kernel /tmp/t-route-kernels
Expand All @@ -666,8 +670,9 @@ RUN if [ "${NGEN_WITH_PYTHON}" == "ON" ]; then \
USER ${USER}

ENV VIRTUAL_ENV=/dmod/venv
RUN python3 -m venv $VIRTUAL_ENV && pip3 install numpy
RUN python3 -m venv $VIRTUAL_ENV
ENV PATH="$VIRTUAL_ENV/bin:$PATH"
RUN if [ "${NGEN_WITH_PYTHON}" == "ON" ]; then pip install numpy${NUMPY_CONSTRAINT} ; fi
robertbartel marked this conversation as resolved.
Show resolved Hide resolved

RUN cd ${BOOST_ROOT} \
&& tar -xf boost_tarball.blob --strip 1 \
Expand Down Expand Up @@ -783,7 +788,25 @@ FROM rocky-ngen-deps as rocky_ngen_build_testing
#FROM rocky_build_ngen as rocky_ngen_build_testing
#FROM ${DOCKER_INTERNAL_REGISTRY}/ngen-deps:latest as rocky_ngen_build_testing

ARG NUMPY_CONSTRAINT
robertbartel marked this conversation as resolved.
Show resolved Hide resolved

COPY --chown=${USER} --from=rocky_init_repo ${WORKDIR}/ngen ${WORKDIR}/ngen

ENV BOOST_ROOT=${WORKDIR}/boost

USER root
RUN if [ "${NGEN_WITH_PYTHON}" == "ON" ]; then \
chgrp -R ${USER} /usr/local/lib*/python3.* ; \
chmod -R g+sw /usr/local/lib*/python3.* ; \
fi
USER ${USER}

ENV VIRTUAL_ENV=/dmod/venv
RUN if [ "${NGEN_WITH_PYTHON}" == "ON" ]; then python3 -m venv ${VIRTUAL_ENV} ; fi
ENV PATH="$VIRTUAL_ENV/bin:$PATH"
RUN if [ "${NGEN_WITH_PYTHON}" == "ON" ]; then pip install numpy${NUMPY_CONSTRAINT} ; fi
robertbartel marked this conversation as resolved.
Show resolved Hide resolved

RUN cd ${BOOST_ROOT} && tar -xf boost_tarball.blob --strip 1 && rm boost_tarball.blob
ENV BOOST_ROOT=${WORKDIR}/boost
#COPY --chown=${USER} --from=rocky_build_troute ${WORKDIR}/t-route/wheels /tmp/t-route-wheels
#COPY --chown=${USER} --from=rocky_build_troute ${WORKDIR}/t-route/requirements.txt /tmp/t-route-requirements.txt
Expand Down
15 changes: 15 additions & 0 deletions example.env
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,21 @@ PYCHARM_REMOTE_DEBUG_SERVER_PORT_GUI=55875
## Path string for Pycharm remote debug package egg
#PYCHARM_REMOTE_DEBUG_EGG_PATH=

################################################################################
################################################################################
## Numpy Settings ##
################################################################################
################################################################################

## Version of Numpy to select when installing (e.g., within Docker images)
## Note that this probably needs to stay in sync with what is in a few other places
## that don't read directly from this file:
## - requirement.txt
## - python/gui/dependencies.txt
# Note: mind the consecutive equal signs; the value being set here is '==1.26.4'
NUMPY_INSTALL_CONSTRAINT===1.26.4


robertbartel marked this conversation as resolved.
Show resolved Hide resolved
################################################################################
################################################################################
## Docker Settings ##
Expand Down
2 changes: 1 addition & 1 deletion python/gui/dependencies.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,4 @@ channels
channels-redis
djangorestframework
psycopg2-binary # TODO: get source package in future. Note that psycopg2 cannot be used on Mac; psycopg2-binary must be used
numpy
numpy==1.26.4
robertbartel marked this conversation as resolved.
Show resolved Hide resolved
2 changes: 1 addition & 1 deletion requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ python-dotenv
djangorestframework
git+https://github.com/noaa-owp/hypy@master#egg=hypy&subdirectory=python
hydrotools.nwis-client
numpy
numpy==1.26.4
aaraney marked this conversation as resolved.
Show resolved Hide resolved
scikit-learn
minio
uri
Expand Down
Loading