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

Clean up IFI support and add library prereqs to modulefiles #1056

Merged

Conversation

SamuelTrahanNOAA
Copy link
Contributor

@SamuelTrahanNOAA SamuelTrahanNOAA commented Sep 30, 2024

This is part of an ongoing project to beautify and document the libIFI and associated programs so other people can develop the code. Changes are:

  1. The IFI.F's NetCDF diagnostic file writer takes less memory and runs faster.
  2. compile_upp.sh -i loads a modulefile for libIFI on Hera through modulefiles/hera_external_ifi.lua
  3. Running compile_upp.sh -I -B will compile internal IFI and its internal test programs. This also loads the hera_ifi_test_prereqs.lua modulefile. That modulefile adds the NetCDF C++ wrapper, needed by libIFI's "fip2-lookalike.x" program.
  4. Build system knows how to build the IFI executables, such as its test programs and the fip2-lookalike.x.
  5. Updated the libIFI hash. (This is necessary for items 3 and 4 above.)
  6. Improved logging and general code clean-up.

ALSO, a bug fix I accidentally left in these changes corrects bug #1064, an incorrect array length in INITPOST_NETCDF.f

Dependency on closed-source library PR. (Most users cannot access this link.)

Issues:

@WenMeng-NOAA
Copy link
Collaborator

@SamuelTrahanNOAA The pre-installed libIFI is on Hera only?

@SamuelTrahanNOAA
Copy link
Contributor Author

I'm installing the latest library on Jet now. Once Hera comes back, I'll update it there too.

My test data isn't on Jet anymore, so I have no way to test it at the moment. I'd have to copy the data and scripts back from Hera, which will take a few hours.

@SamuelTrahanNOAA
Copy link
Contributor Author

I don't own the libIFI installation on Acorn. The NCEP library group will need to update it there.

@SamuelTrahanNOAA
Copy link
Contributor Author

The libIFI is on Jet, and I can confirm UPP on Jet can compile:

  1. UPP with external IFI (compile_upp.sh -i)
  2. UPP with internal IFI (compile_upp.sh -I)
  3. The libIFI's own test programs (compile_upp.sh -I -B)

@WenMeng-NOAA WenMeng-NOAA added the enhancement New feature or request label Oct 3, 2024
@WenMeng-NOAA WenMeng-NOAA added the Ready for Review This PR is ready for code review. label Oct 4, 2024
@WenMeng-NOAA
Copy link
Collaborator

@SamuelTrahanNOAA The build tests on Hera look good to me. Can you provide me the instructions of testing the executable ' fip2-lookalike.x' on Hera?

@SamuelTrahanNOAA
Copy link
Contributor Author

@SamuelTrahanNOAA The build tests on Hera look good to me. Can you provide me the instructions of testing the executable ' fip2-lookalike.x' on Hera?

There are two scripts.

  • Revised script to run UPP and generate diagnostic files for input to fip2-lookalike.x
    /scratch2/BMC/ifi/Samuel.Trahan/fip/regtest/UPP/submit_run_post_rrfs_ifi_internal_hera.sh
  • New script to test fip2-lookalike.x in the same directory where the post ran. It uses the diagnostic files as input.
    /scratch2/BMC/ifi/Samuel.Trahan/fip/regtest/UPP/submit_run_ifi_standalone_hera.sh

To confirm fip2-lookalike.x is working, you must compare:

  • UPP output: cat_vars_0.nc
  • fip2-lookalike output: icing-category-output.nc

The z0 variable is missing from one file, but the data within should be otherwise identical.

I'll update the code to output the missing z0 variable in a later PR.

@WenMeng-NOAA
Copy link
Collaborator

@SamuelTrahanNOAA I don't have access permission to /scratch2/BMC/ifi/Samuel.Trahan/fip.

@SamuelTrahanNOAA
Copy link
Contributor Author

I copied them here. Can you see these?

  • UPP internal libIFI test: /scratch2/BMC/ifi/Samuel.Trahan/for-wen/submit_run_post_rrfs_ifi_internal_hera.sh
  • fip2-lookalike.x test: /scratch2/BMC/ifi/Samuel.Trahan/for-wen/submit_run_ifi_standalone_hera.sh

@SamuelTrahanNOAA
Copy link
Contributor Author

@gspetro-NOAA - I invited you as a collaborator to my repository. If you accept, you should be able to push logs.

@WenMeng-NOAA - The libIFI pull request has no reviews. It needs to be merged first so I can point to a hash of the main branch instead of my feature branch.

Copy link
Collaborator

@gspetro-NOAA gspetro-NOAA left a comment

Choose a reason for hiding this comment

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

@WenMeng-NOAA All RTs pass on Hera, Orion, and Hercules with no baseline changes. Sounds like merging should wait until @SamuelTrahanNOAA 's dependent PR gets merged, so I'll hold off on hitting the "Approve" button, but once his PR gets merged, we should be good to go for this PR.

@SamuelTrahanNOAA
Copy link
Contributor Author

I've made an issue for the bug fix in INITPOST_NETCDF.f where I changed len=20 to len=*

I left the bug fix in this branch by accident, but we may as well fix it here, rather than opening yet another PR that needs to be tested.

@SamuelTrahanNOAA
Copy link
Contributor Author

@WenMeng-NOAA @gspetro-NOAA - The github GCC Linux Build test is failing with this error:

-- The Fortran compiler identification is unknown
/home/runner/work/UPP/UPP/spack/var/spack/environments/upp-env/.spack-env/view/bin/mpif90: line 386: /usr/bin/gfortran-10: No such file or directory
-- The C compiler identification is unknown
CMake Error at /usr/local/share/cmake-3.30/Modules/CMakeDetermineCXXCompiler.cmake:48 (message):
  Could not find compiler set in environment variable CXX:

  g++-10.

Call Stack (most recent call first):
  CMakeLists.txt:7 (project)


CMake Error: CMAKE_CXX_COMPILER not set, after EnableLanguage
-- Configuring incomplete, errors occurred!

@WenMeng-NOAA
Copy link
Collaborator

@WenMeng-NOAA @gspetro-NOAA - The github GCC Linux Build test is failing with this error:

-- The Fortran compiler identification is unknown
/home/runner/work/UPP/UPP/spack/var/spack/environments/upp-env/.spack-env/view/bin/mpif90: line 386: /usr/bin/gfortran-10: No such file or directory
-- The C compiler identification is unknown
CMake Error at /usr/local/share/cmake-3.30/Modules/CMakeDetermineCXXCompiler.cmake:48 (message):
  Could not find compiler set in environment variable CXX:

  g++-10.

Call Stack (most recent call first):
  CMakeLists.txt:7 (project)


CMake Error: CMAKE_CXX_COMPILER not set, after EnableLanguage
-- Configuring incomplete, errors occurred!

@AlexanderRichert-NOAA Do you have any suggestions to solve this GCC CI failure? Thanks!

@AlexanderRichert-NOAA
Copy link
Contributor

AlexanderRichert-NOAA commented Oct 15, 2024

Yes-- the ubuntu-latest runner now only has gcc 12, 13, and 14 available on accounting of now using ubuntu 24.04, so you can either update gcc to one of those versions, or revert to an earlier ubuntu version for the runner (I would suggest the former unless you specifically need gcc 10).

@SamuelTrahanNOAA
Copy link
Contributor Author

You should choose a specific ubuntu version so our tests are consistent.

@SamuelTrahanNOAA
Copy link
Contributor Author

This PR's dependency, in the UPP_IFI repository, has been merged.

@SamuelTrahanNOAA
Copy link
Contributor Author

I'd prefer that a different PR update the github tests, not my PR.

Instead, I've changed the gnu.yml to explicitly specify the ubuntu-22.04 image. That way, it tests exactly what it did before ubuntu-latest was changed to ubuntu-24.04.

In another PR, someone should update to a specific image version (ie. ubuntu-24.04) not a "latest" alias (ie. ubuntu-latest). Another linux distribution is fine. Also, I suggest you use a GCC version that matches what is used on one of the supported machines (hercules, hera, etc.)

@WenMeng-NOAA
Copy link
Collaborator

This PR's dependency, in the UPP_IFI repository, has been merged.

@SamuelTrahanNOAA Have you updated the hash of libIFI.fd in this PR?

@SamuelTrahanNOAA
Copy link
Contributor Author

Have you updated the hash of libIFI.fd in this PR?

Now I have.

@WenMeng-NOAA
Copy link
Collaborator

I'd prefer that a different PR update the github tests, not my PR.

Instead, I've changed the gnu.yml to explicitly specify the ubuntu-22.04 image. That way, it tests exactly what it did before ubuntu-latest was changed to ubuntu-24.04.

In another PR, someone should update to a specific image version (ie. ubuntu-24.04) not a "latest" alias (ie. ubuntu-latest). Another linux distribution is fine. Also, I suggest you use a GCC version that matches what is used on one of the supported machines (hercules, hera, etc.)

@SamuelTrahanNOAA Thanks for testing the CI fix. The updates to the GCC CI can be submitted in a separate PR. All IFI relevant updates look good to me. Would you like to keep the current CI fix in your PR for merging, or incorporate it into the CI fix PR?

@SamuelTrahanNOAA
Copy link
Contributor Author

I do not see my changes as a "CI fix." All I did was restore it to the behavior it had before GitHub changed ubuntu-latest to ubuntu 24.04. Any version change of GCC (if you decide to do one) should be in another PR.

@WenMeng-NOAA
Copy link
Collaborator

@SamuelTrahanNOAA Thanks for clarifying. We will proceed with your PR for the merging process.

@WenMeng-NOAA WenMeng-NOAA added the No Baseline Change No baseline of the UPP regression tests are made. label Oct 16, 2024
@WenMeng-NOAA
Copy link
Collaborator

@gspetro-NOAA Please go ahead to approve this PR.

@WenMeng-NOAA
Copy link
Collaborator

This PR is ready for merging.

@WenMeng-NOAA WenMeng-NOAA merged commit ef204d7 into NOAA-EMC:develop Oct 17, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request No Baseline Change No baseline of the UPP regression tests are made. Ready for commit queue Ready for Review This PR is ready for code review.
Projects
None yet
4 participants