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 all 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
26 changes: 22 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,7 +65,6 @@ 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

ARG NETCDF_C_VERSION=4.8.1
Expand Down Expand Up @@ -291,12 +290,16 @@ FROM rocky-base as rocky-ngen-packaged-deps

ARG ROCKY_NGEN_DEPS_REQUIRED

# Set up pip constraints file for this and descendent stages
COPY constraints.txt ${WORKDIR}/constraints.txt
ENV PIP_CONSTRAINT=${WORKDIR}/constraints.txt

# 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
# 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 \
&& pip install --no-cache-dir pip wheel packaging \
&& if [ "${NGEN_WITH_PYTHON}" == "ON" ]; then pip install --no-cache-dir numpy; 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 @@ -784,6 +786,22 @@ FROM rocky-ngen-deps as rocky_ngen_build_testing
#FROM ${DOCKER_INTERNAL_REGISTRY}/ngen-deps:latest as rocky_ngen_build_testing

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 ; fi

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
5 changes: 5 additions & 0 deletions docker/main/ngen/constraints.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
### Constrain pip
pip>=23.0,<23.1
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is necessary after NOAA-OWP/t-route#804 was merged.

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't have to be addressed here, just putting it on your radar @robertbartel!


### Constrain numpy to < 2.0.0 due to current ngen compatibility issue
numpy~=1.0
Loading