PSyclone icon indicating copy to clipboard operation
PSyclone copied to clipboard

1735 simplify using lfric infrastructure

Open hiker opened this issue 2 years ago • 4 comments

Move the LFRic infrastructure files into subdirectories that are used in LFRic. This makes it easier to replace the included infrastructure with original LFRic files. This code removed the list of files for compilation testing, instead the new Makefile is used to compile the infrastructure during testing if requested. All Makefiles use a target in the PSycline LFRic infrastructure to get the list if include directories to use, but in tests/lfric_build it was easier to use Python to query the list of directories. There should be no more hard-coded path.

hiker avatar Aug 09 '22 08:08 hiker

Codecov Report

Base: 99.42% // Head: 99.50% // Increases project coverage by +0.08% :tada:

Coverage data is based on head (bacb2e0) compared to base (1e64c4b). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1831      +/-   ##
==========================================
+ Coverage   99.42%   99.50%   +0.08%     
==========================================
  Files         262      262              
  Lines       39043    39054      +11     
==========================================
+ Hits        38818    38862      +44     
+ Misses        225      192      -33     
Impacted Files Coverage Δ
src/psyclone/tests/conftest.py 98.55% <100.00%> (ø)
src/psyclone/tests/gocean1p0_stencil_test.py 100.00% <100.00%> (ø)
src/psyclone/tests/gocean1p0_test.py 100.00% <100.00%> (ø)
src/psyclone/tests/gocean_build.py 100.00% <100.00%> (ø)
src/psyclone/tests/lfric_build.py 100.00% <100.00%> (+3.44%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov[bot] avatar Aug 09 '22 14:08 codecov[bot]

This PR:

  1. Moves the infrastructure files into the same subdirectories as they are in LFRic, making it easier to replace our stub with the real LFRic files.
  2. It improves the Makefile to find all source files in all directories automatically and compile them. The files are like LFRic compiled into the same directories, which means many include paths are now required to compile (why on earth does LFRic do that??? The 'base' name of all files must be unique due to the dependency analysis anyway, so all .o and .mod files could be stored in one flat directory).
  3. It provides an include makefile that other makefiles can use to get the list of include paths to provide (i.e. we can anytime add or remove files and directories without any change anywhere in PSyclone).
  4. The lfric_build class uses this makefile to build the infrastructure files (instead of a pre-defined list of files).
  5. The lfric_build class automatically checks for all subdirectories to get the required include list. RE-using the makefile (i.e. calling make from python to get the list of include paths this way was really much longer compared with quickly getting all directories).
  6. All other makesfiles (examples, psydata, tutorial) use the include makefile to query the include paths.
  7. I added tests for gocean_build and lfric_build by allowing the tests to replace the 'make' command with something else (true or false). Now all the build classes are 100% covered, even in non-compilation tests.
  8. I renamed gocean1p0_build to gocean_build ... 1p0 in the name was just wrong if you ask me :) There was one minor change to this file (making the 'make' command configurable ... a bit annoying, in hindsight I should not have renamed it). But I already had changed dozens of tests to use the new name :(

There are two or three things that I would recommend to make separate PRs for, since this PR is already quite big (though mostly moving files, and minor modifications to many many makefiles):

  1. Now that gocean and lfric build use make, some duplicated code should be moved into the base class Compile (i.e. triggering make, and checking for errors). I didn't do this here because I renamed gocean_build.
  2. We should consider to support $F90 and $F90FLAGS in pytest as well. I had problems with our jenkins builds ... as it turns out in our 'intel' pytests, my team member didn't provide --f90 and --f90flags to pytest ... as a result intel pytest on jenkins was actually using gfortran, and it all worked because pytest uses the infrastructure files compiled by python calling the compiler ... which was gfortran. With this PR pytest calls make, which picked up the settings in the environment and then compiled the infrastructure with ifort, but all other tests with gfortran 😱 If we change the default for the pytests command line options (f90/f90flags) to be the same as the environment variables if available, we could just modify the compiler with one environment variable consistently everywhere. I've fixed our jenkins, and my PR passed the full compilation test now.
  3. I think with my trick of changing the compilation command from make to true and false, it should be possible to 100% cover tests/utilities.py, but I suggest leaving this to a separate PR to avoid making this even bigger.

Ready for first review.

hiker avatar Aug 11 '22 03:08 hiker

One thing I forgot to mention: Ideally we would also replace the hard-coded dependency file with a rule that recreates this file. But this needs an update to the latest version of fparser (since we need support for dependencies in subdirectories)

hiker avatar Aug 11 '22 09:08 hiker

Branches passed all tests here as well as our jenkins compilation tests. Ready for next review.

hiker avatar Aug 16 '22 07:08 hiker

Thanks for the review and debugging. It turns out that by sheer coincidence I always had the infrastructure library inside of PSyclone built, so the incorrect paths that were used inside of the compilation in pytest --compile never mattered. While trying to fix this nicely, I found a nice solution to avoid that each process compiles its own infrastructure library (#1849), but I'll leave this for a separate PR, and implement the not-so-nice solution (a conditional setting of a variable in the include file).

hiker avatar Sep 01 '22 08:09 hiker