Jenkins physics tests #21

elynnwu · 2021-11-17T20:04:09Z

On PR, Jenkins run a separate physics savepoint test
Add physics dependencies on fv3core for now since shared stencils and utils are still in fv3core
This is a temporary change, physics should not depend on fv3core
Update physics API call per changes in fv3core

elynnwu · 2021-11-17T20:06:30Z

launch jenkins

elynnwu · 2021-11-17T22:52:09Z

launch jenkins

elynnwu · 2021-11-18T19:58:46Z

launch jenkins

…into jenkins-physics-tests

elynnwu · 2021-11-19T00:45:49Z

.jenkins/jenkins.sh

+fi
+# If the backend is a GTC backend we fetch the caches
+if [[ $backend != *numpy* ]];then
+    . ${jenkins_dir}/actions/fetch_caches.sh $backend $experiment


We do not currently have a cache plan for physics, but the jenkins test only run numpy backend. Will include this when we set up the caching plan.

mcgibbon

Logging this review I forgot to submit yesterday, it looks like you've mostly fixed the issues raised so I'll re-review now.

mcgibbon · 2021-11-19T01:04:49Z

docker/Makefile

 	  	cd ${GT4PY_DIR} && git checkout ${GT4PY_VERSION};\
 	fi

 ## build production container image
 fv3gfs_image: get_gt4py
-	if [ $(PULL) == True ]; then \
+	if [ $(PULL) = True ]; then \


Is this change correct? I'm not that familiar with bash, but google is saying to use == to check equality.

This one was super weird. It should be ==, and it works on my machine locally. It is also how fv3core has it, and it works on jenkins. However, I'm getting this on jenkins gce: [/bin/sh: 1: : True: unexpected operator. But somehow = worked fine.

My guess is it is because PULL is unset on gce. You can fix this by setting the default to PULL ?= False instead of removing the definition of PULL above altogether.

mcgibbon · 2021-11-19T01:05:15Z

fv3gfs-physics/README.md

@@ -2,7 +2,7 @@
 Python implementation of FV3 GFS physics built using the GT4Py domain-specific language in Python.

 ## Description
-fv3gfs-physics is still under development, more descriptions will be added.
+fv3gfs-physics is under active development. Currently, pace level docker environment should be used for development.


Make sure you remember to update this when it is no logner the case.

mcgibbon · 2021-11-19T17:15:04Z

Makefile

@@ -4,13 +4,39 @@ DOCKER_BUILDKIT=1
 SHELL=/bin/bash
 CWD=$(shell pwd)
 PULL ?=True
+DEV ?=n


Please keep this logic contained in the physics directory. We'll need to be able to develop and test it as a self-contained directory in the long term (not in this PR), and this is a lot of its low level logic to be putting at the top level for now.

mcgibbon · 2021-11-19T17:17:48Z

.jenkins/actions/lint.sh

@@ -0,0 +1,9 @@
+#!/bin/bash


Does this lint action block any tests? If not, we can remove it and rely on CircleCI for linting, since it gets answers in about 10 seconds. I had thought lint checks occur as part of other actions before the tests are run.

This script runs before installing virtual env on daint, so it will block any test runs.

mcgibbon · 2021-11-19T17:21:18Z

.jenkins/actions/run_physics_regression_tests.sh

+    CONTAINER_CMD="srun" make tests physics_savepoint_tests
+else
+    export TEST_ARGS="${TEST_ARGS} --junitxml=/.jenkins/${XML_REPORT}"
+    VOLUMES="-v ${pwd}/.jenkins:/.jenkins" make tests physics_savepoint_tests


This volume shouldn't need to be mounted to run these make targets, should it? The physics Makefile shouldn't be accessing ../.jenkins (and doesn't appear to be), and the tests inside the container shouldn't be reading .jenkins either.

You're right. I'm guessing it was mounted to grab xml report from gce, but it does not actually work. In the current plan, the xml report is always from daint. Removing this.

.jenkins/install_virtualenv.sh

mcgibbon · 2021-11-19T17:24:09Z

dependencies.svg

 -->
 <!-- Pages: 1 -->
-<svg width="287pt" height="260pt"


Interesting how you can actually read svgs (sort of).

mcgibbon · 2021-11-19T17:26:23Z

fv3gfs-physics/Makefile

@@ -0,0 +1,72 @@
+include ../docker/Makefile.image_names
+
+DOCKER_BUILDKIT=1


Not about this PR, but I'm hoping I can refactor the tests so that this logic (duplicated in each subproject with savepoint tests) can be greatly reduced.

Co-authored-by: Jeremy McGibbon <jeremym@allenai.org>

…into jenkins-physics-tests

elynnwu · 2021-11-19T21:55:24Z

launch jenkins

elynnwu · 2021-11-19T22:43:42Z

launch jenkins

mcgibbon

One name to fix but then it should be good to merge! Thanks!

mcgibbon · 2021-11-19T22:47:16Z

.jenkins/jenkins.sh

@@ -185,7 +185,7 @@ if [ ${python_env} == "virtualenv" ]; then
    if grep -q "parallel" <<< "${script}"; then
 	export MPIRUN_CALL="srun"
    fi
-    export pace_PATH="${envloc}/../"
+    export pace_PATH="${envloc}/../fv3gfs-physics/"


This is no longer the path of PACE, can you either revert the path and append fv3gfs-physics later or rename the variable?

Also, if PATH is allcaps then PACE should be allcaps in this name, as much as I don't like allcaps PACE...

mcgibbon · 2021-11-19T22:47:24Z

Makefile

@@ -1,3 +1,4 @@
+SHELL := /bin/bash


Glad this worked!

elynnwu added 9 commits November 16, 2021 16:25

update to use latest fv3core in physcis

22aa986

physics tests run from make

16984df

add jenkins file at top level

21d9f12

lint

db2f01b

exeucte permission

ca54b32

including test_args

b936243

physics depend on fv3core for now

c15e3e3

typo in pull image

654711a

trigger physics test

b40e839

jenkins daint

19a5675

elynnwu added 3 commits November 17, 2021 16:08

jenkins update

ed68398

update physics setup

47b28c8

Merge branch 'main' into jenkins-physics-tests

07e8f10

elynnwu added 2 commits November 18, 2021 11:58

Merge branch 'main' into jenkins-physics-tests

74a4c71

Merge branch 'jenkins-physics-tests' of https://github.com/ai2cm/pace …

39e6e18

…into jenkins-physics-tests

elynnwu requested a review from mcgibbon November 19, 2021 00:38

elynnwu commented Nov 19, 2021

View reviewed changes

move physics test details to physics

d446adc

mcgibbon reviewed Nov 19, 2021

View reviewed changes

elynnwu and others added 7 commits November 19, 2021 09:39

fixed jenkins get data

9949cf4

Update .jenkins/install_virtualenv.sh

649590c

Co-authored-by: Jeremy McGibbon <jeremym@allenai.org>

Merge branch 'jenkins-physics-tests' of https://github.com/ai2cm/pace …

7a71690

…into jenkins-physics-tests

jenkins update

3e40630

fix jenkins

aea26ad

Merge branch 'main' into jenkins-physics-tests

86eb913

Merge branch 'main' into jenkins-physics-tests

6d22aa3

elynnwu enabled auto-merge (squash) November 19, 2021 21:49

makefile deafult to bash

8922418

mcgibbon approved these changes Nov 19, 2021

View reviewed changes

elynnwu merged commit ba5380b into main Nov 19, 2021

elynnwu deleted the jenkins-physics-tests branch November 19, 2021 22:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jenkins physics tests #21

Jenkins physics tests #21

elynnwu commented Nov 17, 2021

elynnwu commented Nov 17, 2021

elynnwu commented Nov 17, 2021

elynnwu commented Nov 18, 2021

elynnwu Nov 19, 2021

mcgibbon left a comment

mcgibbon Nov 19, 2021

elynnwu Nov 19, 2021

mcgibbon Nov 19, 2021

mcgibbon Nov 19, 2021

mcgibbon Nov 19, 2021

mcgibbon Nov 19, 2021

elynnwu Nov 19, 2021

mcgibbon Nov 19, 2021

elynnwu Nov 19, 2021

mcgibbon Nov 19, 2021

mcgibbon Nov 19, 2021

elynnwu commented Nov 19, 2021

elynnwu commented Nov 19, 2021

mcgibbon left a comment

mcgibbon Nov 19, 2021

mcgibbon Nov 19, 2021

		@@ -0,0 +1,72 @@
		include ../docker/Makefile.image_names

		DOCKER_BUILDKIT=1

Jenkins physics tests #21

Jenkins physics tests #21

Conversation

elynnwu commented Nov 17, 2021

elynnwu commented Nov 17, 2021

elynnwu commented Nov 17, 2021

elynnwu commented Nov 18, 2021

Choose a reason for hiding this comment

mcgibbon left a 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

elynnwu commented Nov 19, 2021

elynnwu commented Nov 19, 2021

mcgibbon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment