Conversation
ffc3905 to
0138093
Compare
05b0997 to
c10b30a
Compare
f9fc997 to
1d8d61f
Compare
GCC was updated from 13.x to 14.x to allow for passing a factory procedure with a polymorphic function result to each test case subroutine. See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118372 for more details.
1d8d61f to
7f8d450
Compare
Whyborn
left a comment
There was a problem hiding this comment.
A lot of the internal functions are missing any commentary. I found a lot of the routines not particularly intuitive, so some commentary explaining human-readably what the routines are doing would be a big help I think. Doesn't have to be public facing documentation, more like internal developer comments.
I find the tests not very "human"- a lot of abstractions, that I didn't find very easy to reason about exactly what they're doing. This could probably be solved by some commentary as well.
| if [ $do_tests -eq 1 ]; then | ||
| # This is required to allow for passing a factory procedure | ||
| # with a polymorphic function result to various test cases. See | ||
| # https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118372 for more | ||
| # details. | ||
| module add gcc/14.1.0 | ||
| else | ||
| module add gcc/13.2.0 | ||
| fi |
There was a problem hiding this comment.
What's the reason for keeping gcc/13.2.0 at all? Seems a bit redundant to do the tests with one compiler, but do production compilation with another?
There was a problem hiding this comment.
I think it's important to know why a compiler constraint is needed and to not impose them if it isn't necessary as this could hurt portability in general. So I guess I wanted to highlight here that the compiler bump here is needed only to build the tests and not necessarily to build the model.
There was a problem hiding this comment.
I'm personally not in favour of this- I'd much prefer to just up the default version of gcc to the higher version. I don't think updating the gcc compiler is a difficult barrier to get over. I like keeping the explanation as to why the version was bumped. This opens the potential for a development to pass the unit tests, but fail in production (although quite unlikely)
There was a problem hiding this comment.
Good point, I overlooked the possibility that tests will pass for the more recent compiler, but fail for older compilers.
Happy to keep the compiler versions consistent between test and non-test builds.
I realised now that this is a bigger issue for the Fortuno dependency itself, which requires the more recent intel compilers when building from the ground up via FetchContent, which is an even bigger compiler jump.
Lines 108 to 114 in 7f57306
Intel Fortran compilers should have compatible ABIs with each other, so Fortuno can be built with a more recent compiler, but not necessarily have this enforced on the CABLE test build. I've managed to write a spack package recipe for Fortuno, so I'll try install fortuno via spack with the more recent intel compiler, and use find_package to link against it, and try building the CABLE tests with CABLE's default compiler.
There was a problem hiding this comment.
Just did some tests building the tests for various intel compilers using Fortuno built with Intel (ifx) 2025.2.0:
- intel-compiler/2019.5.281 FAILED
- intel-compiler/2020.3.304 FAILED
- intel-compiler/2020.3.304 FAILED
- intel-compiler/2021.4.0 FAILED
- intel-compiler/2021.8.0 FAILED
- intel-compiler/2021.10.0 SUCCESS
Looks like, for the most part, ifx and ifort modules are incompatible.
There was a problem hiding this comment.
Just did some tests building the tests for various intel compilers using Fortuno built with Intel (ifx) 2025.2.0:
Even some older ifx compilers are not compatible:
- intel-compiler-llvm/2022.0.0 FAILED
- intel-compiler-llvm/2023.0.0 FAILED
- intel-compiler-llvm/2023.2.0 SUCCESS
So in summary, we need at least ifort 2021.10.0 or ifx 2023.2.0 to link against Fortuno built with Intel (ifx) 2025.2.0
| integer :: i, tmp | ||
| do i = 1, size(dim_names) | ||
| if (dim_lens(i) == CABLE_NETCDF_UNLIMITED) then | ||
| call check_pio(pio_def_dim(this%pio_file_desc, dim_names(i), PIO_UNLIMITED, tmp)) |
There was a problem hiding this comment.
Can we assign CABLE_NETCDF_UNLIMITED = PIO_UNLIMITED, so that we can drop the if (dim_lens(i) == CABLE_NETCDF_UNLIMITED) then clause?
There was a problem hiding this comment.
I'm gonna say no on this one, CABLE_NETCDF_UNLIMITED is a constant and I don't think each implementation should overwrite constants in cable_netcdf_mod in general.
There was a problem hiding this comment.
PIO and NetCDF use different values for UNLIMITED?
There was a problem hiding this comment.
PIO and NetCDF use different values for
UNLIMITED?
They could be, it depends on what the PIO developers use, and whether that's consistent with NetCDF Fortran.
Having a separate constant in cable_netcdf_mod means we don't need to worry about this.
d1767af to
87f98fd
Compare
|
Thanks for taking a look Lachlan, I forgot to add docs to cable_netcdf_internal.F90. I realised cable_netcdf_internal.F90 was only really necessary for initialising the I/O handler, so I've trimmed down that file to only include the I/O handler initialisation and renamed it to cable_netcdf_init.F90 in 0f30366. |
This change brings in the interface layer for working with the NetCDF Fortran and ParallelIO (PIO) libraries in CABLE. PIO allows for each MPI rank to participate in I/O operations collectively and is a first step in adding MPI support to the serial offline driver, and eventually, to replace the legacy MPI implementation (#358).
To keep CABLE dependencies as minimal as possible for running in serial mode (without MPI), the interface layer is designed such that PIO support is optional.
To build CABLE with PIO, PIO version 2.6.8 or greater is required.
Type of change
Please delete options that are not relevant.
Checklist
Testing
📚 Documentation preview 📚: https://cable--706.org.readthedocs.build/en/706/