Add controls for numpy version #678

robertbartel · 2024-07-17T13:47:24Z

~~Add support for environment-config-controlled constraints on the version of numpy installed by the image build process for ngen-related Docker images.~~ (Note: scrapping original idea in favor of a different approach.)

Add support for pip-constraints-file based control over the version of numpy installed by the image build process for ngen-related Docker images.

Slightly related to NOAA-OWP/ngen#837.

Note that this blocks #675, as that revision of the image can't be built for testing due to the problem being fixed here.

Additions

New environment config variable for numpy constraint, with default value and comments in example.env
New constraints.txt file defining pip constraints for ngen (and related) image builds.

Removals

Removes a deprecated and unused Dockerfile ARG MIN_NUMPY for clarity.

Changes

~~New Dockerfile ARG for numpy constraint, applied appropriately where numpy gets installed by image build~~
~~Setting of aforementioned ARG in Compose build yaml configs, for all ngen-based images/services (e.g., model exec, calibration, etc.)~~
COPY and ENV statements in ngen image Dockerfile to bring in new pip constraint and then set PIP_CONSTRAINT environment variable so that pip uses it.

Testing

ngen worker image builds successfully, whereas previously it would fail due to a failed ngen unit test testing part of the Python BMI integration

Updating Dockerfile and Compose build config file for ngen-based worker images (calibration, partitioning, testing, etc.) to utilize ARG sourced from environment config to control the version of numpy that is installed.

Updating example for DMOD deployment environment config with section for numpy, new numpy constraint environmental variable used by worker image builds, safe default value for new variable, and comments explaining.

Updating project requirements.txt and GUI service requirements file (python/gui/dependencies.txt) to constrain numpy version consistent with default used by worker image builds set up in example.env.

docker/main/docker-build.yml

christophertubbs · 2024-07-17T14:02:17Z

docker/main/docker-build.yml

@@ -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}


This is an abuse of the args parameter. Do not use this to inject python versions. Stick to setup.py and requirements.txt.

docker/main/docker-build.yml

docker/main/ngen/Dockerfile

example.env

aaraney

I think was should instead add a requirements.txt for the ngen image and put things like this there. I think that will easiest to maintain and understand in the long run. I think it also results in a simpler solution. Thoughts?

python/gui/dependencies.txt

requirements.txt

robertbartel · 2024-07-17T16:03:13Z

I think was should instead add a requirements.txt for the ngen image and put things like this there. I think that will easiest to maintain and understand in the long run. I think it also results in a simpler solution. Thoughts?

@aaraney, @christophertubbs , there are subtle implications to this in the context of building a Docker image. While there may be others as well, at minimum:

Combining all the installed Python packages in the Dockerfile into a single requirements file is going to inevitably mess up build caching for some earlier stages and make the build less efficient
Combining everything inside the Dockerfile into a single requirements file would force things to be installed into stages (and therefore, images) when they aren't actually needed; e.g., ngen-cal

We can, of course, add subtle complexity to the way we design what goes into to requirement files, whether we use different ones for different build stages, etc. But if we start going down that road, I think what we end up with is not any better of a solution.

Honestly, if allowing configurable control of the version really seems like a problem, what are both of your thoughts on just hard-coding the numpy version constraints into the Dockerfile in the prior places where it was being installed?

christophertubbs · 2024-07-17T16:32:29Z

If the issue is with a single requirements file... there's nothing keeping us from having a single requirements file. We can also produce new build 'hooks' where extra files may be COPYed in if behavior needs to be altered in different stages.

What we have, right now, is already too complicated. We need to start moving in another direction to find an easier to follow build pipeline. If not making something configurable makes the build process prohibitively resource intensive, something is wrong and we need to take a step back and evaluate our process(es).

For now, hard code the value since it would be global anyways. If a user needs to color outside the lines and use a different value, they may change it themselves.

Ideally, DMOD needs to get out of the game of setting up compilation rules for another product. We don't set compilation flags or environment variables just to prepare pandas for use. DMOD is a consumer of NextGen and compatible formulations. Instructions and constraints for use need to be provided from them, not developed by us. If the DMOD image creation process is responsible for setting this up we're putting ourselves in a position of eternal catch up.

robertbartel · 2024-07-17T17:23:46Z

What we have, right now, is already too complicated. We need to start moving in another direction to find an easier to follow build pipeline.

You aren't wrong, but that's out of scope for what is essentially a bug fix.

If not making something configurable makes the build process prohibitively resource intensive, something is wrong and we need to take a step back and evaluate our process(es).

Just to be clear, the lack of configurability doesn't make the builds less efficient. But artificially tethering together the installation of all the Python packages used throughout the worker images would.

For now, hard code the value since it would be global anyways. If a user needs to color outside the lines and use a different value, they may change it themselves.

Sure, I can make that change for here, and we can separate off the idea of adding (or not) configurability to numpy versioning for workers.

Ideally, DMOD needs to get out of the game of setting up compilation rules for another product. We don't set compilation flags or environment variables just to prepare pandas for use. DMOD is a consumer of NextGen and compatible formulations.

Providing compatible, consistent, reproducible compute environments (whether these be Docker images/containers or some other mechanism) for model execution is a fundamental feature of DMOD, and that means it has to be concerned with compilation flags and environment details and anything else users would have to deal with to set up an environment if they were just trying to use ngen directly. There is certainly room to discuss which of these DMOD opens up via control/configure mechanisms versus what DMOD just handles automagically behind the scenes. And if/when ngen can add its own automation for installing its dependencies like numpy or begins distributing installation packages, great; DMOD can leverage those instead. Until it does though, for DMOD to be able to run ngen, there’s just no way around dealing with this kind of stuff.

aaraney · 2024-07-17T17:43:03Z

I think NOAA-OWP/ngen#856 will solve our issues an obsolete this PR. We will still at some point need to revisit how we handle external bmi module dependency versioning, but that is a whole can of worms that I think we should kick down the road!

robertbartel · 2024-07-17T17:48:22Z

@aaraney, no, I think NOAA-OWP/ngen#856 will just cause DMOD image builds to fail for a different reason (the CMake error instead of the unit test failure). Unless ngen actually handles installing numpy itself, we are going to have to address what version of numpy gets installed in the image.

As discussed above, I'll make changes so that we hard code this for now to the latest 1.x.

aaraney · 2024-07-17T17:48:43Z

Extra context, there is already a check in ngen that if the version of numpy that was used at build time does not match the one that is used at runtime, an exception is thrown. So, if ngen is built with numpy<=2 and then numpy in the environment gets updated, we are still covered.

aaraney · 2024-07-17T19:16:46Z

Alright, so after a side channel conversation with @robertbartel, at least IMO, I think we can achieve the goals of this PR using a pip constraints.txt file (very similar to requirements.txt). More specifically, the path to the constraints.txt file would be set to the PIP_CONSTRAINT environment variable in any build stages that build / install python dependencies. In doing so, any packages that are installed via pip will be constrained to the version specifiers in constraints.txt.

In doing so, this lets us control what version or version ranges of certain packages are installed in the image. Likewise, incompatibilities will results in a build failure.

aaraney · 2024-07-17T19:28:27Z

docker/main/ngen/Dockerfile

@@ -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

 # 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


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

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.

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.

@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.

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.

aaraney · 2024-07-17T19:30:07Z

docker/main/ngen/Dockerfile


 # 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
 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


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

Rolling back initial changes in favor of a different approach.

Adding constraints file to ngen (and related) image build to control numpy and pip versions as needed for compatibility, using this instead of doing directly within the Dockerfile itself in pip commands.

aaraney · 2024-07-18T17:32:51Z

docker/main/ngen/constraints.txt

@@ -0,0 +1,5 @@
+### Constrain pip
+pip>=23.0,<23.1


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

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

aaraney

This looks good to me! Thanks, @robertbartel!

robertbartel · 2024-07-18T18:12:08Z

@christophertubbs, any other concerns before this moves forward? I might either need explicit approval from you or to dismiss your previous review, in order to comply with branch protections (the latter of which I don't want to do until you've had a chance to raise any remaining discussion items).

Changes reengineered using different approach based on previous review discussion.

Rolling back initial changes in favor of a different approach.

robertbartel added 3 commits July 17, 2024 09:17

Control the version of numpy used in workers.

6d02a07

Updating Dockerfile and Compose build config file for ngen-based worker images (calibration, partitioning, testing, etc.) to utilize ARG sourced from environment config to control the version of numpy that is installed.

Update example.env with numpy version constraint.

22b308b

Updating example for DMOD deployment environment config with section for numpy, new numpy constraint environmental variable used by worker image builds, safe default value for new variable, and comments explaining.

Update requirements files for numpy constraint.

318afe5

Updating project requirements.txt and GUI service requirements file (python/gui/dependencies.txt) to constrain numpy version consistent with default used by worker image builds set up in example.env.

robertbartel added enhancement New feature or request maas MaaS Workstream Blocks Another issue is blocked until this is resolved labels Jul 17, 2024

robertbartel requested review from hellkite500, christophertubbs and aaraney July 17, 2024 13:47

robertbartel mentioned this pull request Jul 17, 2024

Update worker images to optimize IO performance using local data #675

Open

christophertubbs previously requested changes Jul 17, 2024

View reviewed changes

aaraney requested changes Jul 17, 2024

View reviewed changes

python/gui/dependencies.txt Outdated Show resolved Hide resolved

requirements.txt Outdated Show resolved Hide resolved

aaraney reviewed Jul 17, 2024

View reviewed changes

robertbartel added 2 commits July 18, 2024 10:17

Roll back initial NOAA-OWP#678 changes.

d302fc3

Rolling back initial changes in favor of a different approach.

Use pip constraints file in ngen Dockerfile.

74f7759

Adding constraints file to ngen (and related) image build to control numpy and pip versions as needed for compatibility, using this instead of doing directly within the Dockerfile itself in pip commands.

robertbartel requested review from aaraney and christophertubbs July 18, 2024 14:44

aaraney reviewed Jul 18, 2024

View reviewed changes

aaraney approved these changes Jul 18, 2024

View reviewed changes

robertbartel merged commit b8dcc03 into NOAA-OWP:master Jul 29, 2024
4 of 8 checks passed

robertbartel added a commit that referenced this pull request Jul 29, 2024

Roll back initial #678 changes.

3018f02

Rolling back initial changes in favor of a different approach.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add controls for numpy version #678

Add controls for numpy version #678

robertbartel commented Jul 17, 2024 •

edited

Loading

christophertubbs Jul 17, 2024

aaraney left a comment

robertbartel commented Jul 17, 2024

christophertubbs commented Jul 17, 2024

robertbartel commented Jul 17, 2024

aaraney commented Jul 17, 2024

robertbartel commented Jul 17, 2024 •

edited

Loading

aaraney commented Jul 17, 2024

aaraney commented Jul 17, 2024

aaraney Jul 17, 2024

christophertubbs Jul 17, 2024

robertbartel Jul 17, 2024

robertbartel Jul 17, 2024 •

edited

Loading

aaraney Jul 18, 2024 •

edited

Loading

aaraney Jul 17, 2024

aaraney Jul 18, 2024

aaraney Jul 18, 2024

aaraney left a comment

robertbartel commented Jul 18, 2024

	&& 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

Add controls for numpy version #678

Add controls for numpy version #678

Conversation

robertbartel commented Jul 17, 2024 • edited Loading

Additions

Removals

Changes

Testing

christophertubbs Jul 17, 2024

Choose a reason for hiding this comment

aaraney left a comment

Choose a reason for hiding this comment

robertbartel commented Jul 17, 2024

christophertubbs commented Jul 17, 2024

robertbartel commented Jul 17, 2024

aaraney commented Jul 17, 2024

robertbartel commented Jul 17, 2024 • edited Loading

aaraney commented Jul 17, 2024

aaraney commented Jul 17, 2024

aaraney Jul 17, 2024

Choose a reason for hiding this comment

christophertubbs Jul 17, 2024

Choose a reason for hiding this comment

robertbartel Jul 17, 2024

Choose a reason for hiding this comment

robertbartel Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

aaraney Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

aaraney Jul 17, 2024

Choose a reason for hiding this comment

aaraney Jul 18, 2024

Choose a reason for hiding this comment

aaraney Jul 18, 2024

Choose a reason for hiding this comment

aaraney left a comment

Choose a reason for hiding this comment

robertbartel commented Jul 18, 2024

robertbartel commented Jul 17, 2024 •

edited

Loading

robertbartel commented Jul 17, 2024 •

edited

Loading

robertbartel Jul 17, 2024 •

edited

Loading

aaraney Jul 18, 2024 •

edited

Loading