Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add cmake nvhpc support #1286

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

jim-p-w
Copy link
Contributor

@jim-p-w jim-p-w commented Feb 7, 2025

This PR adds support for using the nvhpc toolchain when building with cmake.

The primary changes include the following when the nvhpc toolchain is detected:

  1. setting the MPI_Fortran_COMPILER variable to mpifort prior to searching for the MPI package.
  2. adding various defines and compiler switches for the compilation stage.

For simplicity, the dependencies on netcdf are removed. MPAS-Model only depends on the parallel netcdf packages.

To test the nvhpc toolchain, a regional test case was submitted to gpu nodes, and the output
was compared to the output from running it against an nvhpc build built with make.
The test output was the same.

To ensure the removal of the netcdf dependencies is backward compatible, the mpas-bundle software was built with the gnu toolchain. Everything built properly.
The result of the build was used to run the panda-c cylc test suite, all tests passed.

This includes conditionally setting the MPI_Fortran_COMPILER prior to
searching for the MPI package.

This also includes adding a number of defines and compiler switches
when the nvhpc toolchain is detected.

For simplification the NetCDF dependencies were removed
(only the parallel netcdf packages are needed).
@mgduda mgduda requested review from amstokely, islas and mgduda February 7, 2025 22:52
@mgduda mgduda added the feature label Feb 7, 2025
Copy link
Contributor

@amstokely amstokely left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the FindNetCDF.cmake file is no longer used, you may want to delete it. Other than that, I was able to build this branch on my personal Linux desktop and on Aspire2a (both use GNU and OpenMPI) without any problems.

@mgduda
Copy link
Contributor

mgduda commented Feb 8, 2025

Since the FindNetCDF.cmake file is no longer used, you may want to delete it. Other than that, I was able to build this branch on my personal Linux desktop and on Aspire2a (both use GNU and OpenMPI) without any problems.

It's possible that PIO would require netCDF, however. So perhaps the find_package invocation for NetCDF should depend on PIO being requested?

@jim-p-w
Copy link
Contributor Author

jim-p-w commented Feb 8, 2025

Since the FindNetCDF.cmake file is no longer used, you may want to delete it. Other than that, I was able to build this branch on my personal Linux desktop and on Aspire2a (both use GNU and OpenMPI) without any problems.

It's possible that PIO would require netCDF, however. So perhaps the find_package invocation for NetCDF should depend on PIO being requested?

I will test that and respond accordingly. Thanks for the heads up about that possibility.

@mgduda netcdf is only required by src/core_atmosphere/physics/physics_noahmp/utility/ErrorHandleMod.F90, which contains one subroutine, ErrorHandle.
ErrorHandle is not called anywhere in the MPAS code base (in the develop branch).

I noticed that ErrorHandleMod.F90 is not built by make, and I removed it from the cmake build (see the change to src/core_atmosphere/CMakeLists.txt). That allowed me to remove the netcdf dependencies from cmake.

Perhaps ErrorHandleMod.F90 should be deleted from the repo?
That file was added as part of importing the Noah-MP land surface scheme, see commit 148dc18.

@jim-p-w
Copy link
Contributor Author

jim-p-w commented Feb 28, 2025

Given that the PIO version of mpas_atmosphere (what atmosphere_model is named when building with cmake) builds successfully without using FindNetCDF.cmake I think FindNetCDF.cmake should be deleted.

Other than that, are there other issues I need to address before we can consider merging this PR? (I have two downstream PR's in the mpas-bundle repository which depend on this PR.)

@islas
Copy link
Contributor

islas commented Feb 28, 2025

@jim-p-w I think it might be getting away with building if you are linking against a dynamic library of PIO. If it is static, it does require netCDF :

target_link_libraries(PIO::PIO_Fortran_STATIC INTERFACE NetCDF::NetCDF_C)

I would also think if it needs it, it should call find_package() inside of the call to resolve that dependency, but that is outside the scope of this PR. Mostly noting that this might be addressed in the future.

@jim-p-w
Copy link
Contributor Author

jim-p-w commented Feb 28, 2025

@jim-p-w I think it might be getting away with building if you are linking against a dynamic library of PIO. If it is static, it does require netCDF :

target_link_libraries(PIO::PIO_Fortran_STATIC INTERFACE NetCDF::NetCDF_C)

I would also think if it needs it, it should call find_package() inside of the call to resolve that dependency, but that is outside the scope of this PR. Mostly noting that this might be addressed in the future.

@islas @amstokely @michalakes In that case I will add back the call to find the netcdf package if building PIO. Thanks for the info Anthony!

The netcdf package is now required when building with PIO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants