search

ai2cm / fv3gfs-wrapper

Python wrapper for the FV3-based global climate model
Other
28 stars 3 forks source link

Unit tests are not being run by CI #278

Closed oliverwm1 closed 3 years ago

oliverwm1 commented 3 years ago

See CI logs for build_default on a recent commit, or more specifically see this commit: https://github.com/VulcanClimateModeling/fv3gfs-wrapper/pull/277/commits/85899b07c53473ddabdfaa6bfb14605d4ee7d441

nbren12 commented 3 years ago

CI should trigger any test in tests/image_tests/: https://github.com/VulcanClimateModeling/fv3gfs-wrapper/blob/master/Makefile#L88

Maybe we can rename that folder to tests/pytest and move any non-mpi tests there?

oliverwm1 commented 3 years ago

Why can't we just call pytest tests? That's what I'm trying in #277. Maybe I'm not following the intended testing strategy here.

cc @mcgibbon

oliverwm1 commented 3 years ago

CI should trigger any test in tests/image_tests/

what about the tests in tests?

nbren12 commented 3 years ago

The other tests need to be run in MPI, and must be invoked from here: https://github.com/VulcanClimateModeling/fv3gfs-wrapper/blob/master/tests/test_all.py

@mcgibbon and I have both tried at various times to make the entrypoint to the tests simpler but it is somewhat tricky.

mcgibbon commented 3 years ago

Yeah the complicating factor is that there are a bunch of tests which need to be executed using mpirun. There are different ways to do this, one is to mpirun pytest (which we do for DSL parallel tests), and the other is to run mpi from a pytest test (which we do here).

mcgibbon commented 3 years ago

The DSL approach we came up with after what we have in this repo is perhaps simpler, but it's still not as simple as purely non-MPI tests. With that approach you need to stop after one failure to prevent deadlocks, and put in conditional logic to not run MPI tests if you're running without MPI (and vice-versa for the non-MPI tests).

nbren12 commented 3 years ago

one is to mpirun pytest

I started working on a branch someplace that does this. Other complicating factors are that fv3gfs.wrapper.initialize can only be called once and the run-directory needs to be setup beforehand.

mcgibbon commented 3 years ago

Other complicating factors are that fv3gfs.wrapper.initialize can only be called once and the run-directory needs to be setup beforehand.

Ah yes, that was a huge pain.

oliverwm1 commented 3 years ago

Okay, well anyway I implemented Noah's suggestions in #277 and it actually shows that there are a couple failing unit tests. I'll fix them up, and then ask one of you for review.