Docker image for top level directory #4

elynnwu · 2021-08-26T17:18:17Z

Added docker image from the top level that includes fv3core, fv3gfs-physics, and fv3gfs-util.

Copied most top level files in fv3core to fv3gfs-integration
Changed docker image name to fv3gfs-integration
Generated new constraints file based on the requirements in all three folders
Added test data symbol link to allow fv3core pytest to run with make savepoint_tests or manually through make dev && pytest -v -s --data_path=/test_data/ /port_dev/tests

Makefile

mcgibbon · 2021-08-30T22:00:57Z

Makefile

+constraints.txt: requirements.txt requirements/requirements_wrapper.txt requirements/requirements_lint.txt
+	pip-compile $^ --output-file constraints.txt


This should probably look at requirements files in its subdirectories, rather than having copies of those requirements files, to avoid duplicating data. It can include requirements not just from fv3core, but also from fv3gfs-util and fv3gfs-physics.

This isn't to say there shouldn't be any requirements files at the top level (esp for linting or for running the top-level integration tests), it's just that if requirements get updated for fv3core then ideally we should only need to update the fv3core requirements in the fv3core directory.

This is a good point. It now reads the requirement data for the individual repository.

Makefile

README.md

docker/Makefile.image_names

mcgibbon · 2021-08-30T22:13:59Z

docker/entrypoint.sh

+pip install -r requirements.txt
+pip install -e /fv3gfs-util
+pip install -e /fv3core
+pip install -e /fv3gfs-physics


Do these need a -c constraints.txt?

I don't think so because the docker image already installed the requirements with constraints.

I'd ask to add -c constraints.txt. I'd expect this wouldn't update the packages, but I'm not confident that pip install package==version followed by pip install package will never upgrade it. And it's possible for an error or refactor to cause the requirements to no longer be installed during the docker build.

mcgibbon · 2021-08-30T22:14:04Z

docker/dependencies.Dockerfile

+
+## Build FMS
+##---------------------------------------------------------------------------------
+FROM fv3gfs-environment AS fv3gfs-fms-install


Note that if you remove fv3gfs-wrapper from this level as I suggested in another comment, it will also involve removing the FMS and ESMF stages, and fv3gfs-build, and maybe also fv3gfs-environment.

FMS, ESMF, fv3gfs-build, and fv3gfs-environment are removed.

mcgibbon

So much cleaner now! We also talked about removing the test data link from the top level, since you will already have access to the test data through the fv3core/fv3gfs-physics mounts. You may want to add the test arguments you run inside the dev image to the README so others can conveniently copy-paste them as needed.

mcgibbon · 2021-09-01T18:27:26Z

GT4PY_VERSION.txt

@@ -0,0 +1 @@
+v30


I think this is a good design, but you should communicate to the team that the integration, physics, and dycore have separate pins of the gt4py version.

mcgibbon · 2021-09-01T18:28:08Z

Makefile

+FV3=fv3core
+FV3_PATH ?=/$(FV3)


The $(FV3) variable is only used here.

Suggested change

FV3=fv3core

FV3_PATH ?=/$(FV3)

FV3_PATH ?=/fv3core

Actually we don't need these variables anymore, deleted

mcgibbon · 2021-09-01T18:28:31Z

Makefile

+PHY=fv3gfs-physics
+PHY_PATH ?=/$(PHY)


The $(PHY) variable is only used here.

Suggested change

PHY=fv3gfs-physics

PHY_PATH ?=/$(PHY)

PHY_PATH ?=/fv3gfs-physics

Same as above

mcgibbon · 2021-09-01T18:28:56Z

Makefile

+PULL ?=True
+CONTAINER_ENGINE ?=docker
+TEST_DATA_HOST ?=$(CWD)/test_data/$(EXPERIMENT)
+FV3UTIL_DIR=$(CWD)/fv3gfs-util


This is unused.

Suggested change

FV3UTIL_DIR=$(CWD)/fv3gfs-util

Same as before

mcgibbon · 2021-09-01T18:33:59Z

Makefile

+dev:
+	docker run --rm -it \
+		--network host \
+		-v $(TEST_DATA_HOST):$(TEST_DATA_RUN_LOC) \


I would suggest removing the test data link from the integration dev entrypoint, to avoid adding a third place (physics, dynamics and integration) that depends on this test data. This entrypoint is the only place where you still have a dependency.

We can always specify an additional -v flag manually to mount this data if we want to use it inside the integrations image while developing.

On that note, I think you should add a RUN_ARGS ?= --rm flag to this file and use it in this make target, to give a clear way to add additional docker run arguments.

Test data mount is removed, updated documentation to point to test data. RUN_ARGS added.

mcgibbon · 2021-09-01T18:44:30Z

docker/Makefile

+build_core_deps: ## build container images of dependencies
+	docker build -f $(DEPENDENCIES_DOCKERFILE) -t $(MPI_IMAGE) $(BUILD_ARGS) --target fv3gfs-mpi ../external/fv3gfs-fortran
+	$(MAKE) build_core_env
+	docker build -f $(DEPENDENCIES_DOCKERFILE) -t $(SERIALBOX_IMAGE) $(BUILD_ARGS) --target fv3gfs-environment-serialbox .
+
+build_core_env:
+	 docker build -f $(DEPENDENCIES_DOCKERFILE) -t $(ENVIRONMENT_IMAGE) $(BUILD_ARGS) --target fv3gfs-environment .


It looks like build_core_env only appears here, this would make the lines a bit more uniform.

nit: could you rename each of these to remove core? I believe this was meant to refer to fv3core. As mentioned in the other comment, it doesn't look like there's a need to separate dependencies into two groups here.

Suggested change

build_core_deps: ## build container images of dependencies

docker build -f $(DEPENDENCIES_DOCKERFILE) -t $(MPI_IMAGE) $(BUILD_ARGS) --target fv3gfs-mpi ../external/fv3gfs-fortran

$(MAKE) build_core_env

docker build -f $(DEPENDENCIES_DOCKERFILE) -t $(SERIALBOX_IMAGE) $(BUILD_ARGS) --target fv3gfs-environment-serialbox .

build_core_env:

docker build -f $(DEPENDENCIES_DOCKERFILE) -t $(ENVIRONMENT_IMAGE) $(BUILD_ARGS) --target fv3gfs-environment .

build_core_deps: ## build container images of dependencies

docker build -f $(DEPENDENCIES_DOCKERFILE) -t $(MPI_IMAGE) $(BUILD_ARGS) --target fv3gfs-mpi ../external/fv3gfs-fortran

docker build -f $(DEPENDENCIES_DOCKERFILE) -t $(ENVIRONMENT_IMAGE) $(BUILD_ARGS) --target fv3gfs-environment .

docker build -f $(DEPENDENCIES_DOCKERFILE) -t $(SERIALBOX_IMAGE) $(BUILD_ARGS) --target fv3gfs-environment-serialbox .

Consolidated to deps, no more reference to core_deps

mcgibbon · 2021-09-01T18:45:12Z

docker/dependencies.Dockerfile

+#RUN update-alternatives --install /usr/bin/gcc gcc /usr/bin/gcc-8 8 && \
+#    update-alternatives --install /usr/bin/g++ g++ /usr/bin/g++-8 8 && \
+#    update-alternatives --install /usr/bin/gfortran gfortran /usr/bin/gfortran-8 8


Please delete these commented lines.

mcgibbon · 2021-09-01T18:45:26Z

docker/dependencies.Dockerfile

+#RUN update-alternatives --install /usr/bin/gcc gcc /usr/bin/gcc-8 8 && \
+#    update-alternatives --install /usr/bin/g++ g++ /usr/bin/g++-8 8 && \
+#    update-alternatives --install /usr/bin/gfortran gfortran /usr/bin/gfortran-8 8


mcgibbon · 2021-09-01T18:47:58Z

docker/entrypoint.sh

+pip install -r requirements.txt
+pip install -e /fv3gfs-util
+pip install -e /fv3core
+pip install -e /fv3gfs-physics


I'd ask to add -c constraints.txt. I'd expect this wouldn't update the packages, but I'm not confident that pip install package==version followed by pip install package will never upgrade it. And it's possible for an error or refactor to cause the requirements to no longer be installed during the docker build.

requirements.txt

mcgibbon

LGTM!

Update dwind phys is an object

elynnwu added 3 commits August 25, 2021 21:45

move docker to top level and include physics

081ea75

get data from top level

ac59264

add documentation

ac0c7f4

elynnwu requested a review from mcgibbon August 26, 2021 17:19

fix syntax

7364655

mcgibbon reviewed Aug 30, 2021

View reviewed changes

change test data location and clean up

85f8fb0

mcgibbon reviewed Sep 1, 2021

View reviewed changes

elynnwu added 3 commits September 1, 2021 12:32

update test data path and core deps

64c8bae

more cleanup and requirement update

6e9a98e

changing fv3core to fv3gfs

18ce1d3

mcgibbon approved these changes Sep 1, 2021

View reviewed changes

elynnwu merged commit 669a616 into main Sep 2, 2021

elynnwu deleted the add-docker branch September 2, 2021 02:00

mcgibbon pushed a commit that referenced this pull request Nov 12, 2021

Merge pull request #4 from ai2cm/update-dwind-phys

280ed4f

Update dwind phys is an object

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Docker image for top level directory #4

Docker image for top level directory #4

elynnwu commented Aug 26, 2021

mcgibbon Aug 30, 2021

elynnwu Sep 1, 2021

mcgibbon Aug 30, 2021

elynnwu Sep 1, 2021

mcgibbon Sep 1, 2021

mcgibbon Aug 30, 2021

elynnwu Sep 1, 2021

mcgibbon left a comment •

edited

Loading

mcgibbon Sep 1, 2021

mcgibbon Sep 1, 2021

elynnwu Sep 1, 2021

mcgibbon Sep 1, 2021

elynnwu Sep 1, 2021

mcgibbon Sep 1, 2021

elynnwu Sep 1, 2021

mcgibbon Sep 1, 2021

elynnwu Sep 1, 2021

mcgibbon Sep 1, 2021

elynnwu Sep 1, 2021

mcgibbon Sep 1, 2021

mcgibbon Sep 1, 2021

mcgibbon Sep 1, 2021

mcgibbon left a comment

		constraints.txt: requirements.txt requirements/requirements_wrapper.txt requirements/requirements_lint.txt
		pip-compile $^ --output-file constraints.txt

	PHY=fv3gfs-physics
	PHY_PATH ?=/$(PHY)
	PHY_PATH ?=/fv3gfs-physics

		@@ -0,0 +1 @@
		v30

Docker image for top level directory #4

Docker image for top level directory #4

Conversation

elynnwu commented Aug 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcgibbon left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcgibbon left a comment

Choose a reason for hiding this comment

mcgibbon left a comment •

edited

Loading