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

sync with head of NOAA-EMC UPP develop #845

Merged
merged 25 commits into from
Aug 23, 2024

Conversation

SamuelTrahanNOAA
Copy link
Contributor

@SamuelTrahanNOAA SamuelTrahanNOAA commented Jun 12, 2024

Description

This updates the upp submodule to the head of NOAA-EMC UPP's develop branch.

Also, the spack.yaml has explicit UPP dependencies (g2, g2tmpl, etc.) and all libraries have exact version numbers. These version numbers match the top of ufs-weather-model's develop branch for hercules.

Issue(s) addressed

Testing

How were these changes tested?

Tested in the HAFS-AR variant of the HAFS workflow in a large stationary regional domain over the north-east Pacific. Values looked reasonable. Also ran UPP and ufs-weather-model regression tests.

What compilers / HPCs was it tested with?

Hera intel and Hera gnu.

Are the changes covered by regression tests? (If not, why? Do new tests need to be added?)

The ufs-weather-model regression tests already test the inline post. However, I added three new number concentration fields to the ufs-weather-model gnv1_nested regression test.

Have the ufs-weather-model regression test been run? On what platform?

The ufs-weather-model regression testing is in progress now. Baseline generation on Hera succeeded.

UPP's own regression tests passed. That tests the offline post (running UPP outside FV3).

Will the code updates change regression test baseline? If yes, why? Please show the baseline directory below.

Results of many tests should change due to updating the UPP hash.

Please commit the regression test log files in your ufs-weather-model branch

Will do.

Dependencies

None.

@SamuelTrahanNOAA SamuelTrahanNOAA changed the title point to number-concentration branch of upp Add number concentration on pressure levels to inline post by updating UPP hash. Jun 12, 2024
@SamuelTrahanNOAA SamuelTrahanNOAA changed the title Add number concentration on pressure levels to inline post by updating UPP hash. update UPP hash with corresponding bug fix to inline post + add number concentration to UPP Jun 13, 2024
@WenMeng-NOAA
Copy link
Contributor

@SamuelTrahanNOAA When you submit ufs-weather-model PR with upp submodule updating to the latest commit from UPP repository, all UPP control files post-xconfig*.txt (under ufs-weather-model/test/parm) need to be recreated for UFS RTs.

@SamuelTrahanNOAA
Copy link
Contributor Author

When you submit ufs-weather-model PR with upp submodule updating to the latest commit from UPP repository, all UPP control files post-xconfig*.txt (under ufs-weather-model/test/parm) need to be recreated for UFS RTs.

I need your help with that. I'm uncertain of which txt file in tests/parm corresponds to which file in FV3/upp/parm. Also, I'm wondering if any need modification before copying to tests/parm.

@SamuelTrahanNOAA
Copy link
Contributor Author

SamuelTrahanNOAA commented Jun 13, 2024

These three files from tests/parm don't exist in FV3/upp/parm:

  • postxconfig-NT-fv3lam.txt
  • postxconfig-NT-gfs.txt
  • postxconfig-NT-gfs_FH00.txt

This does exist, but it's the wrong one:

  • postxconfig-NT-hafs.txt

The tests/parm version has no synthetic satellite brightness temperature fields, while the FV3/upp/parm one does. It isn't the postxconfig-NT-hafs_nosat.txt either, since that one differs in other ways.

@WenMeng-NOAA
Copy link
Contributor

These three files from tests/parm don't exist in FV3/upp/parm:

  • postxconfig-NT-fv3lam.txt
  • postxconfig-NT-gfs.txt
  • postxconfig-NT-gfs_FH00.txt

This does exist, but it's the wrong one:

  • postxconfig-NT-hafs.txt

The tests/parm version has no synthetic satellite brightness temperature fields, while the FV3/upp/parm one does. It isn't the postxconfig-NT-hafs_nosat.txt either, since that one differs in other ways.

These are simple versions of UPP control files for inline post testing for gfs, rffs and hafs. I will find corresponding xml files and provide you updated txt files later.

@SamuelTrahanNOAA
Copy link
Contributor Author

@WenMeng-NOAA - As mentioned in this comment, I need help debugging the inline post.

234:  get_g2_fixedsurfacetypes key:          255  not found in table 4.5

You can find the output here:

LOG: /scratch1/NCEPDEV/stmp2/Samuel.Trahan/FV3_RT/rt_3037467/gnv1_nested_intel.log
RUN: /scratch1/NCEPDEV/stmp2/Samuel.Trahan/FV3_RT/rt_3037467/gnv1_nested_intel

Many ranks give the same error many times, but it's always the same error.

Code is here:

ufs-community/ufs-weather-model#2326

It only works on Hera because only Hera has g2tmpl 1.12.0 in the same Spack Stack as ufs-weather-model prerequisites.

@SamuelTrahanNOAA
Copy link
Contributor Author

SamuelTrahanNOAA commented Jun 14, 2024

There are a multitude of warnings coming from a library when the latest UPP is used.

148: get_g2_fixedsurfacetypes key: 255 not found in table 4.5

@WenMeng-NOAA has tracked this down to the UPP pull request NOAA-EMC/UPP#929. The warnings vanish if you revert the changes to the postxconfig files (or rather, to the post_avblflds.xml used to generate them). The warnings don't seem to have an effect on the output.

This warning probably originates from a bug in one of the grib2 libraries (g2 or g2tmpl) or an incompatibility between them. Library updates can take a long time.

Question to everyone: Should we wait for the warnings to go away before updating UPP in fv3atm? Or should we live with the warnings for now?

EDIT: We have a bug fix from the UPP side that eliminates these error messages. It is in this PR.

@SamuelTrahanNOAA
Copy link
Contributor Author

Good news, everyone!

We have a fix for the "255 not found in table 4.5" messages!

That means this PR can continue review without thousands of error messages from g2tmpl.

@SamuelTrahanNOAA SamuelTrahanNOAA marked this pull request as ready for review June 17, 2024 21:05
@SamuelTrahanNOAA
Copy link
Contributor Author

I'd like @DusanJovic-NOAA @WenMeng-NOAA and @EricJames-NOAA to look at this PR.

Copy link

@EricJames-NOAA EricJames-NOAA left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for working on this.

@SamuelTrahanNOAA SamuelTrahanNOAA changed the title update UPP hash with corresponding bug fix to inline post + add number concentration to UPP sync with head of NOAA-EMC UPP develop Jun 20, 2024
@SamuelTrahanNOAA
Copy link
Contributor Author

@zhanglikate - Your message was garbled, so I don't know what you want.

Please do one of these:

  1. Use the github "suggested change" feature, or
  2. Open a PR to this branch with your changes.

@zhanglikate
Copy link

zhanglikate commented Jul 24, 2024 via email

@WenMeng-NOAA
Copy link
Contributor

@SamuelTrahanNOAA Could you update the upp harsh to the latest commit @81b38a8 from the UPP repository?

@SamuelTrahanNOAA
Copy link
Contributor Author

Yes. It matches that hash now.

@SamuelTrahanNOAA
Copy link
Contributor Author

Could I get some reviews by people "in the know?"
Maybe these individuals?

@WenMeng-NOAA
Copy link
Contributor

The upp submodule update and inline post interface "post_fv3.F90" look good to me.

@DusanJovic-NOAA
Copy link
Collaborator

Github workflow failed:

2024-08-14T18:45:15.8799819Z /home/runner/work/fv3atm/fv3atm/fv3atm/upp/sorc/ncep_post.fd/grib2_module.f:499:53:
2024-08-14T18:45:15.8800893Z 
2024-08-14T18:45:15.8801377Z   499 |                            g2sec4_temp0,g2sec4_temp8,g2sec4_temp9,g2sec4_temp44,         &
2024-08-14T18:45:15.8802380Z       |                                                     1
2024-08-14T18:45:15.8806022Z Error: Symbol ‘g2sec4_temp9’ referenced at (1) not found in module ‘grib2_all_tables_module’
2024-08-14T18:45:15.8807542Z /home/runner/work/fv3atm/fv3atm/fv3atm/upp/sorc/ncep_post.fd/grib2_module.f:499:80:
2024-08-14T18:45:15.8808321Z 
2024-08-14T18:45:15.8808820Z   499 |                            g2sec4_temp0,g2sec4_temp8,g2sec4_temp9,g2sec4_temp44,         &
2024-08-14T18:45:15.8809907Z       |                                                                                1
2024-08-14T18:45:15.8811126Z Error: Symbol ‘g2sec4_temp46’ referenced at (1) not found in module ‘grib2_all_tables_module’
2024-08-14T18:45:15.8812522Z /home/runner/work/fv3atm/fv3atm/fv3atm/upp/sorc/ncep_post.fd/grib2_module.f:501:79:
2024-08-14T18:45:15.8813317Z 
2024-08-14T18:45:15.8813853Z   501 |                            g2sec5_temp3,g2sec5_temp40,get_g2_sec5packingmethod,          &
2024-08-14T18:45:15.8814838Z       |                                                                               1
2024-08-14T18:45:15.8816070Z Error: Symbol ‘g2sec4_temp49’ referenced at (1) not found in module ‘grib2_all_tables_module’
2024-08-14T18:45:15.8986194Z make[2]: *** [upp/sorc/ncep_post.fd/CMakeFiles/upp.dir/build.make:868: upp/sorc/ncep_post.fd/CMakeFiles/upp.dir/grib2_module.f.o] Error 1
2024-08-14T18:45:15.8989127Z make[1]: *** [CMakeFiles/Makefile2:1278: upp/sorc/ncep_post.fd/CMakeFiles/upp.dir/all] Error 2
2024-08-14T18:45:15.8990581Z make[1]: *** Waiting for unfinished jobs....

@SamuelTrahanNOAA
Copy link
Contributor Author

That looks a lot like an out-of-date library. Maybe @RatkoVasic-NOAA can comment?

@SamuelTrahanNOAA
Copy link
Contributor Author

The github workflow gets its UPP dependencies for spack from this line in ci/spack.yaml:

- upp@develop

The upp@develop refers to whatever is in spack develop.

In other words, FV3 is getting its UPP dependencies from the spack repository instead of the UPP repository.

@WenMeng-NOAA
Copy link
Contributor

Github workflow failed:

2024-08-14T18:45:15.8799819Z /home/runner/work/fv3atm/fv3atm/fv3atm/upp/sorc/ncep_post.fd/grib2_module.f:499:53:
2024-08-14T18:45:15.8800893Z 
2024-08-14T18:45:15.8801377Z   499 |                            g2sec4_temp0,g2sec4_temp8,g2sec4_temp9,g2sec4_temp44,         &
2024-08-14T18:45:15.8802380Z       |                                                     1
2024-08-14T18:45:15.8806022Z Error: Symbol ‘g2sec4_temp9’ referenced at (1) not found in module ‘grib2_all_tables_module’
2024-08-14T18:45:15.8807542Z /home/runner/work/fv3atm/fv3atm/fv3atm/upp/sorc/ncep_post.fd/grib2_module.f:499:80:
2024-08-14T18:45:15.8808321Z 
2024-08-14T18:45:15.8808820Z   499 |                            g2sec4_temp0,g2sec4_temp8,g2sec4_temp9,g2sec4_temp44,         &
2024-08-14T18:45:15.8809907Z       |                                                                                1
2024-08-14T18:45:15.8811126Z Error: Symbol ‘g2sec4_temp46’ referenced at (1) not found in module ‘grib2_all_tables_module’
2024-08-14T18:45:15.8812522Z /home/runner/work/fv3atm/fv3atm/fv3atm/upp/sorc/ncep_post.fd/grib2_module.f:501:79:
2024-08-14T18:45:15.8813317Z 
2024-08-14T18:45:15.8813853Z   501 |                            g2sec5_temp3,g2sec5_temp40,get_g2_sec5packingmethod,          &
2024-08-14T18:45:15.8814838Z       |                                                                               1
2024-08-14T18:45:15.8816070Z Error: Symbol ‘g2sec4_temp49’ referenced at (1) not found in module ‘grib2_all_tables_module’
2024-08-14T18:45:15.8986194Z make[2]: *** [upp/sorc/ncep_post.fd/CMakeFiles/upp.dir/build.make:868: upp/sorc/ncep_post.fd/CMakeFiles/upp.dir/grib2_module.f.o] Error 1
2024-08-14T18:45:15.8989127Z make[1]: *** [CMakeFiles/Makefile2:1278: upp/sorc/ncep_post.fd/CMakeFiles/upp.dir/all] Error 2
2024-08-14T18:45:15.8990581Z make[1]: *** Waiting for unfinished jobs....

@DusanJovic-NOAA Please check if g2/3.51 and g2tmpl/1.13.0 are used in CI.

@SamuelTrahanNOAA
Copy link
Contributor Author

It's using a cached version of spack, not the latest build.

There should be an update to .github/workflows/GCC.yml to increment the cache version.

@DusanJovic-NOAA
Copy link
Collaborator

DusanJovic-NOAA commented Aug 16, 2024

It's using a cached version of spack, not the latest build.

There should be an update to .github/workflows/GCC.yml to increment the cache version.

Actually I think we should ci/spack.yaml, because the cache key is defined based on hash of spack.yaml. So in order to detect potential changes in any of the libraries we should explicitly list every single library and its version , and not use 'upp@develop' which basically 'hides' the changes in upp dependencies. upp@develop can mean different things at different time. Basically all dependencies must be versioned. And instead of just saying upp, I prefer to have a explicit list of libraries with explicit versions.

Try to change spack.yaml in your branch and see if it triggers spack cache rebuild.

@SamuelTrahanNOAA
Copy link
Contributor Author

I've updated spack.yaml, and it is rebuilding. There were library version conflicts, so I chose the latest version of the two.

What worries me is this one:

ip@develop precision=4,d,8

Using the @develop version means the version may change as the branch is retested. Wouldn't we be better off using a specific numbered version? If so, which one?

@DusanJovic-NOAA
Copy link
Collaborator

I've updated spack.yaml, and it is rebuilding. There were library version conflicts, so I chose the latest version of the two.

What worries me is this one:

ip@develop precision=4,d,8

Using the @develop version means the version may change as the branch is retested. Wouldn't we be better off using a specific numbered version? If so, which one?

Yes. All libraries should be versioned, develop is ambiguous. I would use versions from your ufs-weather-model branch ufs_common.lua from modulefiles directory, with updated versions of g2 and g2tmpl. Also, fv3atm does not need sigio, sfcio, nemsio and wrf-io. Those are required by the upp executable, which we are not building here.

@SamuelTrahanNOAA
Copy link
Contributor Author

Yes. All libraries should be versioned, develop is ambiguous. I would use versions from your ufs-weather-model branch ufs_common.lua from modulefiles directory, with updated versions of g2 and g2tmpl. Also, fv3atm does not need sigio, sfcio, nemsio and wrf-io. Those are required by the upp executable, which we are not building here.

@DusanJovic-NOAA - Could you check the spack.yaml? It should match your specifications now.

@DusanJovic-NOAA
Copy link
Collaborator

Yes. All libraries should be versioned, develop is ambiguous. I would use versions from your ufs-weather-model branch ufs_common.lua from modulefiles directory, with updated versions of g2 and g2tmpl. Also, fv3atm does not need sigio, sfcio, nemsio and wrf-io. Those are required by the upp executable, which we are not building here.

@DusanJovic-NOAA - Could you check the spack.yaml? It should match your specifications now.

The spack.yaml looks good. And I see workflow finished successfully. Thank you for fixing this.

@SamuelTrahanNOAA
Copy link
Contributor Author

I made an issue for the spack.yaml troubles:

It'll be automatically closed when this PR is merged.

@zach1221
Copy link
Collaborator

@jkbk2004 testing is finished on WM PR 2326. Feel free to merge this PR.

@jkbk2004 jkbk2004 merged commit ee3378b into NOAA-EMC:develop Aug 23, 2024
6 checks passed
HelinWei-NOAA pushed a commit to HelinWei-NOAA/fv3atm that referenced this pull request Sep 15, 2024
* send CCPP ebu_smoke to UPP ebb

* upp: remove GSD_NC synonyms and rename GSD_NC fields instead

* update upp hash: g2, g2tmpl, etc.

* upp: correct pressure levels for hafs-ar postxconfig files

* bugfix: fixed_sfc2_type defaults to fixed_sfc1_type

* Add aerosol fix from Li Pan.

---------

Co-authored-by: Wen Meng <Wen.Meng@noaa.gov>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants