-
Notifications
You must be signed in to change notification settings - Fork 49
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
Use ip@5: if available instead of sp #524
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All tests pass.
Looks good to me.
ping |
@scrasmussen I saw #551. I'm going to run some tests on Hera to verify that this works with spack-stack 1.8.0 before I approve. If it works, we can merge it, although I don't think that we want the SCM to get ahead of where the UFS is in terms of which version spack-stack is being used by default: https://github.com/ufs-community/ufs-weather-model/blob/develop/modulefiles/ufs_hera.intel.lua |
@grantfirl If we don't want the SCM to get ahead I can just answer gabersek with instructions to checkout my updated fature/ip5 branch and use that as a workaround until the default spack-stack gets updated and we merge this? (Hopefully I'm understanding the situation correctly) |
I think that it should be OK to merge before UFS updates their spack-stack version because Dom changed the CMakeLists to look for IP first and then SP if not found. This should work fine with spack-stack 1.6 until it is updated to coincide with UFS. gabersek was already doing something "nonstandard" by trying the SCM with spack-stack 1.8, but it wouldn't work because CMakeLists.txt didn't even know to look for IP yet. It's weird, though, because sp supposedly still exists in 1.8, so I'm not 100% sure why the issue cropped up yet. |
Note that the RTs run via CI do not use IP. They are falling back to SP. The model is running, but apparently all baselines are changed? That seems odd to me. On another note, I have an older version of spack-stack installed on my Mac that has ip that tries to get used. When it does, I'm getting compilation errors in RRTMGP. ccpp-scm-new/ccpp/physics/physics/Radiation/RRTMGP/rte-rrtmgp/extensions/cloud_optics/mo_cloud_optics.F90:530:23: 530 | !$omp map(from:optical_props%tau) 545 | !$omp map(from:optical_props%tau, optical_props%ssa, optical_props%g) 545 | !$omp map(from:optical_props%tau, optical_props%ssa, optical_props%g) I'm going to try to use spack-stack 1.8.0 on Hera and test with IP to see if I get the same compilation error. |
@dustinswales When you get a chance, try to compile this PR on Hera with spack-stack 1.8.0 and see if the compilation error seems easily fixable to you. This doesn't make a whole lot of sense to me because I doubt RRTMGP has anything to do with the IP library, but it fails nevertheless. On my mac version of spack-stack, I'm getting failures in mo_cloud_optics.F90 related to openmp. On Hera, I'm getting compilation errors in mo_fluxes_broadband_kernels.F90 also related to openmp statements. |
I can compile everything successfully on Hera with the same exact spack-stack 1.8.0 stack substituting sp/2.5.0 back in for ip/5.0.0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to understand why ip/5.0.0 is causing RRTMGP to fail compilation before approving this. Granted, the code as-is will find sp/2.5.0 using the default spack-stack 1.6.0, but when updating to 1.8.0 and ip/5.0.0, this problem will need to be figured out anyway.
The sp library is deprecated and replaced by ip@5 and newer (drop-in replacement). While sp@2.5.0 is still in spack-stack-1.8.0, ip-5.0.0 is also available. We expect sp to be removed in spack-stack-1.9.0.
This PR goes with NCAR/ccpp-physics#1090