Addition of `coverage` tool to unit tests in workflow #21

fmalatino · 2024-05-23T14:38:24Z

Description
Addition of coverage to unit tests in workflow to determine the scope of code touched by testing.

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
Any dependent changes have been merged and published in downstream modules
New check tests, if applicable, are included
Targeted model of this changed was triggered by a model need/shortcoming

xyuan

could we use more openmp threads to speed up the test?

mlee03 · 2024-05-28T21:14:47Z

.github/workflows/translate.yml

@@ -73,7 +74,7 @@ jobs:
            export FV3_DACEMODE=BuildAndRun
            export PACE_FLOAT_PRECISION=64
            export PACE_TEST_N_THRESHOLD_SAMPLES=0
-            export OMP_NUM_THREADS=1
+            export OMP_NUM_THREADS=10


does it work with 10 threads? https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners/about-github-hosted-runners#standard-github-hosted-runners-for-public-repositories

The OMP threading has been left untested for a great while. While I expect it will work, I'd rather have it done in another PR safety.

I will roll back to OMP_NUM_THREADS=1, thank you both for pointing this out.

FlorianDeconinck · 2024-05-29T13:01:20Z

setup.py

@@ -16,7 +16,7 @@
 ]

 test_requirements = ["pytest", "pytest-subtests", "serialbox"]
-ndsl_requirements = ["ndsl @ git+https://github.com/NOAA-GFDL/NDSL.git@2024.04.00"]
+ndsl_requirements = ["ndsl @ git+https://github.com/NOAA-GFDL/NDSL.git@develop"]


Bad reference version.

FlorianDeconinck

Good start, we need to think further on what our time threshold is for those PR tests. Can you time how long this takes and we will add tests to see if we can cover a bigger section of the code.

Alternatively, we could run codecov to see how much is getting touch in pyFV3 and try to have stencil code coverage somewhere above the ~75% mark

FlorianDeconinck · 2024-05-29T13:01:28Z

.github/workflows/translate.yml

@@ -73,7 +74,7 @@ jobs:
            export FV3_DACEMODE=BuildAndRun
            export PACE_FLOAT_PRECISION=64
            export PACE_TEST_N_THRESHOLD_SAMPLES=0
-            export OMP_NUM_THREADS=1
+            export OMP_NUM_THREADS=10


The OMP threading has been left untested for a great while. While I expect it will work, I'd rather have it done in another PR safety.

FlorianDeconinck · 2024-05-29T13:03:12Z

.github/workflows/translate.yml

+              --which_modules=D_SW \
+              --threshold_overrides_file=./tests/savepoint/translate/overrides/standard.yaml \
+              ./tests/savepoint
+        - name: Orchestrated dace:cpu Remapping


This is the bare minimum for orchestration. We should probably run some pySHiELD code and the full FVDynamics case as well

As of commit 453e6b7 the omp threads have been rolled back to 1, the NDSL version is back at 2024.04.00, and added coverage to the test suite. I timed the current test suite to take 5601.23 s (~93.35 mins). Would it make sense to add in the pySHiELD code and FVDynamics case in another PR?

Coverage result:

Name Stmts Miss Branch BrPart Cover -------------------------------------------------------------------------------------------- pyFV3/_config.py 281 42 84 2 80% pyFV3/dycore_state.py 120 39 52 1 64% pyFV3/initialization/analytic_init.py 20 11 6 0 35% pyFV3/initialization/init_utils.py 139 92 12 0 31% pyFV3/initialization/test_cases/initialize_baroclinic.py 89 67 4 0 24% pyFV3/initialization/test_cases/initialize_tc.py 164 164 2 0 0% pyFV3/stencils/a2b_ord4.py 252 125 98 12 47% pyFV3/stencils/c_sw.py 174 120 68 3 24% pyFV3/stencils/d2a2c_vect.py 252 136 112 8 39% pyFV3/stencils/d_sw.py 323 132 140 18 51% pyFV3/stencils/del2cubed.py 90 45 40 0 36% pyFV3/stencils/delnflux.py 252 129 155 8 37% pyFV3/stencils/divergence_damping.py 201 68 74 7 57% pyFV3/stencils/dyn_core.py 277 122 114 11 46% pyFV3/stencils/fillz.py 78 57 50 0 21% pyFV3/stencils/fv_dynamics.py 188 145 80 0 18% pyFV3/stencils/fv_subgridz.py 377 331 90 0 14% pyFV3/stencils/fvtp2d.py 83 16 24 2 74% pyFV3/stencils/fxadv.py 141 81 66 3 30% pyFV3/stencils/map_single.py 59 21 16 0 59% pyFV3/stencils/mapn_tracer.py 24 1 8 2 91% pyFV3/stencils/moist_cv.py 52 33 20 0 43% pyFV3/stencils/neg_adj3.py 188 169 106 0 8% pyFV3/stencils/nh_p_grad.py 51 22 10 0 48% pyFV3/stencils/pe_halo.py 11 8 10 0 14% pyFV3/stencils/pk3_halo.py 24 10 10 0 41% pyFV3/stencils/ppm.py 40 26 20 0 30% pyFV3/stencils/ray_fast.py 94 67 84 0 21% pyFV3/stencils/remap_profile.py 318 276 220 2 11% pyFV3/stencils/remapping.py 173 77 88 8 41% pyFV3/stencils/riem_solver3.py 74 45 30 1 31% pyFV3/stencils/riem_solver_c.py 57 31 28 0 33% pyFV3/stencils/saturation_adjustment.py 525 400 200 1 30% pyFV3/stencils/sim1_solver.py 77 64 58 0 11% pyFV3/stencils/temperature_adjust.py 18 13 10 0 18% pyFV3/stencils/tracer_2d_1l.py 94 71 36 0 21% pyFV3/stencils/updatedzc.py 72 36 28 1 45% pyFV3/stencils/updatedzd.py 84 31 24 1 54% pyFV3/stencils/xppm.py 169 122 78 3 32% pyFV3/stencils/xtp_u.py 35 27 16 0 24% pyFV3/stencils/yppm.py 170 122 78 3 33% pyFV3/stencils/ytp_v.py 35 27 16 0 24% pyFV3/testing/map_single.py 15 15 2 0 0% pyFV3/testing/translate_dyncore.py 55 3 16 1 94% pyFV3/testing/translate_fvdynamics.py 81 52 24 0 30% pyFV3/testing/validation.py 89 62 20 0 27% pyFV3/utils/functional_validation.py 31 25 4 0 17% pyFV3/version.py 2 0 0 0 100% pyFV3/wrappers/geos_wrapper.py 275 275 32 0 0% -------------------------------------------------------------------------------------------- TOTAL 6493 4053 2563 98 33%

93 mins and we are only touching 33% of the code? That's... disappointing. Is the coverage report aggregating everything correctly since all the test are done one by one?

yeah, this is very slow, could you try using more openmp threads?

OpenMP won't help, the execution is not slow part, the build is. Frank restricted to one rank test (e.g. testing the data generated on rank 0 instead of the entire cube) and that should take care of the slowness of the test overall

FlorianDeconinck · 2024-05-30T13:18:41Z

This will require a release of NDSL with a higher DaCe, which would solve the few lasting issues around enumerate

fmalatino · 2024-05-31T19:23:32Z

Issue regarding code coverage logged in 22

FlorianDeconinck · 2024-07-09T19:15:48Z

This should b renamed right?

bensonr · 2024-07-09T19:23:08Z

setup.py

@@ -27,7 +27,7 @@

 setup(
    author="The Allen Institute for Artificial Intelligence",
-    author_email="[email protected]",
+    author_email="[email protected]",


Do we need to change any more of the information in the setup block such as python-requires and some of the other classifiers?

Updated the python-requires will wait for opinions of others to make subsequent changes.

If we don't expect to release the code on pypi, this has minimal impact since most of that metadata is made for pypi and other tools that automate gathering informations on the packages.

That said, no reason to not change the other field to shift things to NOAA, correct license and Py 3.11 as a base py

If we're going to change this (we should) we should change it everywhere so there is no inconsistency

oelbert

The Change to setup.py probably belongs in another PR where we revise all author info, but otherwise good to go

fmalatino added 8 commits May 21, 2024 15:10

Added orchestrated tests to workflow

f337fcf

Linting

a137551

Adding FV3_DACEMODE variable to tests

63ddb26

Added data download and environment variables to translate workflow

ebf24de

Updated setup.py to point to updated NDSL

d2cdc8b

Updates to translate test to remove gt_cache between tests

cb94382

Changed setup.py back to track 2024.04.00 release of NDSL

6675f76

Moving NDSL reference back to develop

5a3c1e7

fmalatino marked this pull request as ready for review May 23, 2024 15:25

fmalatino requested review from xyuan, FlorianDeconinck, oelbert, bensonr and mlee03 May 23, 2024 15:27

bensonr previously approved these changes May 24, 2024

View reviewed changes

Merge branch 'develop' into update/workflow

918ff32

oelbert previously approved these changes May 25, 2024

View reviewed changes

xyuan previously approved these changes May 25, 2024

View reviewed changes

mlee03 previously approved these changes May 28, 2024

View reviewed changes

Increased number of omp threads for testing

5dced20

fmalatino dismissed stale reviews from mlee03, xyuan, oelbert, and bensonr via 5dced20 May 28, 2024 17:54

Adding in dace mode and loglevel environment variables for tests

4ae14e1

fmalatino requested review from mlee03, bensonr, oelbert and xyuan May 28, 2024 18:19

Addition of environment variables to have settings be explicit

a74ee82

bensonr previously approved these changes May 28, 2024

View reviewed changes

mlee03 reviewed May 28, 2024

View reviewed changes

FlorianDeconinck reviewed May 29, 2024

View reviewed changes

fmalatino added 4 commits May 29, 2024 10:27

Changed translate tests in translate.yml to use 1 omp thread

ecd8d2a

Changed reference to NDSL in setup.py to latest release version

0828d72

Added coverage package to testing workflow

10ed431

Linting

453e6b7

fmalatino dismissed bensonr’s stale review via 453e6b7 May 29, 2024 17:49

Re-linting

614aa47

Fixed translate tests to use one rank

0f45171

fmalatino marked this pull request as draft July 1, 2024 19:07

fmalatino added 2 commits July 9, 2024 14:25

Merged develop into update/workflow

9d3dc6b

Removed translate.yml

c670a1b

fmalatino requested review from FlorianDeconinck, bensonr and mlee03 July 9, 2024 19:10

fmalatino marked this pull request as ready for review July 9, 2024 19:10

fmalatino changed the title ~~Addition of dace:cpu orchestrated versions of unit tests in workflow~~ Addition of coverage tool to unit tests in workflow Jul 9, 2024

Updated minimum python version requirement in doc of setup.py

c8e244a

bensonr reviewed Jul 9, 2024

View reviewed changes

oelbert approved these changes Jul 12, 2024

View reviewed changes

mlee03 approved these changes Jul 12, 2024

View reviewed changes

fmalatino merged commit 595c3c8 into NOAA-GFDL:develop Jul 12, 2024
4 checks passed

romanc mentioned this pull request Nov 12, 2024

Cleanup pass in setup.py #28

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Addition of `coverage` tool to unit tests in workflow #21

Addition of `coverage` tool to unit tests in workflow #21

fmalatino commented May 23, 2024 •

edited

Loading

xyuan left a comment

mlee03 May 28, 2024

FlorianDeconinck May 29, 2024

fmalatino May 29, 2024

FlorianDeconinck May 29, 2024

FlorianDeconinck left a comment

FlorianDeconinck May 29, 2024

FlorianDeconinck May 29, 2024

fmalatino May 29, 2024 •

edited

Loading

FlorianDeconinck May 29, 2024

xyuan Jun 3, 2024

FlorianDeconinck Jun 3, 2024

FlorianDeconinck commented May 30, 2024

fmalatino commented May 31, 2024

FlorianDeconinck commented Jul 9, 2024

bensonr Jul 9, 2024

fmalatino Jul 9, 2024

FlorianDeconinck Jul 9, 2024

oelbert Jul 12, 2024

oelbert left a comment

Addition of coverage tool to unit tests in workflow #21

Addition of coverage tool to unit tests in workflow #21

Conversation

fmalatino commented May 23, 2024 • edited Loading

xyuan 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

FlorianDeconinck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmalatino May 29, 2024 • 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

FlorianDeconinck commented May 30, 2024

fmalatino commented May 31, 2024

FlorianDeconinck commented Jul 9, 2024

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

oelbert left a comment

Choose a reason for hiding this comment

Addition of `coverage` tool to unit tests in workflow #21

Addition of `coverage` tool to unit tests in workflow #21

fmalatino commented May 23, 2024 •

edited

Loading

fmalatino May 29, 2024 •

edited

Loading