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

[production/RRFS.v1] Fix 'check all' using 0-sized arrays #2158

Conversation

grantfirl
Copy link
Collaborator

@grantfirl grantfirl commented Feb 27, 2024

Commit Queue Requirements:

  • Fill out all sections of this template.
  • All sub component pull requests have been reviewed by their code managers.
  • Run the full Intel+GNU RT suite (compared to current baselines) on either Hera/Derecho/Hercules
  • [N/A] Commit 'test_changes.list' from previous step

Description:

This PR at least partially addresses #2023 (at least any runtime errors associated with the CCPP). The strategy is to simply make sure that every variable is allocated, although for variables that had been conditionally-allocated to save memory, they are now being allocated with zero size. This has a ripple effect in other repos where one needs to change from checking whether a variable is allocated to whether the size is > 0.

The change to Intel.cmake was used for testing only and I can remove this if we're not ready to permanently add it.

Commit Message:

* UFSWM - Add zero-size variable allocations to pass the `-check all` compiler option for CCPP-related code.
  * AQM - 
  * CDEPS - 
  * CICE - 
  * CMEPS - 
  * CMakeModules - 
  * FV3 - Add zero-size variable allocations to pass the `-check all` compiler option for CCPP-related code.
    * ccpp-physics - Add zero-size variable allocations to pass the `-check all` compiler option for CCPP-related code.
    * atmos_cubed_sphere - Add zero-size variable allocations to pass the `-check all` compiler option for CCPP-related code.
  * GOCART - 
  * HYCOM - 
  * MOM6 - 
  * NOAHMP - 
  * WW3 - 
  * stochastic_physics - 

Priority:

  • High: Needed for RRFS.v1 release

Git Tracking

UFSWM:

Sub component Pull Requests:

UFSWM Blocking Dependencies:

  • None

Changes

Regression Test Changes (Please commit test_changes.list):

  • No Baseline Changes.

Input data Changes:

  • None.

Library Changes/Upgrades:

  • No Updates

Testing Log:

  • RDHPCS
    • Hera
    • Orion
    • Hercules
    • Jet
    • Gaea
    • Derecho
  • WCOSS2
    • Dogwood/Cactus
    • Acorn
  • CI
  • opnReqTest (complete task if unnecessary)

@grantfirl
Copy link
Collaborator Author

Replaces #2102 and this is targeted at the RRFS release branch.

@grantfirl grantfirl marked this pull request as ready for review February 27, 2024 22:29
@grantfirl
Copy link
Collaborator Author

Retesting on Hera. Will post results when finished.

@grantfirl
Copy link
Collaborator Author

@jkbk2004 I kicked off the tests thinking that the baseline from where the RRFS branch was cut in the history would still be valid, but I see from #2147 that you're creating a new one. I'll retest using that baseline when it's ready.

@grantfirl
Copy link
Collaborator Author

@jkbk2004 My RTs were successful (no BL changes), but that was comparing against the saved baselines from the develop branch.

@grantfirl
Copy link
Collaborator Author

@jkbk2004 I'm re-running RTs pointing at the new RRFS baselines for 20240227. Just to confirm, those new baselines do not include the changes in #2147, right?

@grantfirl grantfirl changed the title Fix 'check all' using 0-sized arrays [production/RRFS.v1] Fix 'check all' using 0-sized arrays Feb 28, 2024
@grantfirl
Copy link
Collaborator Author

Throughput on Hera is slow for our account right now. It looks like it'll be a while.

@grantfirl grantfirl changed the base branch from production/RRFS.v1 to develop March 1, 2024 15:41
@grantfirl grantfirl changed the base branch from develop to production/RRFS.v1 March 1, 2024 15:41
@grantfirl
Copy link
Collaborator Author

@jkbk2004 Hi re-running RTs again because the last time that I did, there were unexpected failures, probably because the RRFS baselines already contained #2147.

@jkbk2004
Copy link
Collaborator

jkbk2004 commented Mar 2, 2024

@grantfirl I checked code base of production/rrfs.v1 branch. All looks good. On orion, baseline was reproduced ok. Somehow baseline files look screwed up on hera. I restored baselines and cleaned up to go only with rt.conf_rrfs. @MatthewPyle-NOAA hera/orion/hercules tests are done. This PR will be ready once you confirm on WCOSS2 test

@MatthewPyle-NOAA
Copy link
Collaborator

I had a lot of tests fail on WCOSS, which really didn't make sense to me. I'm recreating the baselines somewhat differently than I did the first time (using a -b rt.conf_rrfs option this time)

@BrianCurtis-NOAA
Copy link
Collaborator

I had a lot of tests fail on WCOSS, which really didn't make sense to me. I'm recreating the baselines somewhat differently than I did the first time (using a -b rt.conf_rrfs option this time)

You would want ./rt.sh -e -c -l rt.conf_rrfs

@jkbk2004
Copy link
Collaborator

jkbk2004 commented Mar 4, 2024

@MatthewPyle-NOAA I checked production/RRFS.v1 code base are ok during the merge process of #2147. So we are ok if you need to re-create baseline on wcoss. Sounds like similar things happened on hera.

@grantfirl
Copy link
Collaborator Author

@jkbk2004 @MatthewPyle-NOAA @BrianCurtis-NOAA I'm glad that tests are passing on Hera. This saves me from a "wild goose chase" of trying to figure out the failures that I was getting. My next course of action was to produce my own baseline from the top of production/RRFS.v1 and use that, but it sounds like everything is working OK now.

@MatthewPyle-NOAA
Copy link
Collaborator

I'm good with it now on WCOSS.

@MatthewPyle-NOAA
Copy link
Collaborator

@jkbk2004 Let me know if you need more from me at this stage. I do have a WCOSS log to add eventually.

@jkbk2004
Copy link
Collaborator

jkbk2004 commented Mar 4, 2024

@jkbk2004 Let me know if you need more from me at this stage. I do have a WCOSS log to add eventually.

@MatthewPyle-NOAA sure! go ahead to push wcoss log. then we can move on to merge/

@MatthewPyle-NOAA
Copy link
Collaborator

@jkbk2004 Let me know if you need more from me at this stage. I do have a WCOSS log to add eventually.

@MatthewPyle-NOAA sure! go ahead to push wcoss log. then we can move on to merge/

@jkbk2004 Where/how do I push it? I tested with Grant's feature branch, but don't have the permission to push to his space.

@jkbk2004
Copy link
Collaborator

jkbk2004 commented Mar 4, 2024

@jkbk2004 Let me know if you need more from me at this stage. I do have a WCOSS log to add eventually.

@MatthewPyle-NOAA sure! go ahead to push wcoss log. then we can move on to merge/

@jkbk2004 Where/how do I push it? I tested with Grant's feature branch, but don't have the permission to push to his space.

@grantfirl can you add Mathew for the permission to your repo?

@grantfirl
Copy link
Collaborator Author

@jkbk2004 Let me know if you need more from me at this stage. I do have a WCOSS log to add eventually.

@MatthewPyle-NOAA sure! go ahead to push wcoss log. then we can move on to merge/

@jkbk2004 Where/how do I push it? I tested with Grant's feature branch, but don't have the permission to push to his space.

@grantfirl can you add Mathew for the permission to your repo?

I've added @MatthewPyle-NOAA as a collaborator on my fork, so he should have permission to push now.

@MatthewPyle-NOAA
Copy link
Collaborator

Thanks @grantfirl WCOSS log file has been pushed now.

@grantfirl
Copy link
Collaborator Author

grantfirl commented Mar 4, 2024

@jkbk2004 @MatthewPyle-NOAA Are we ready to start merging? I can do ccpp-physics and ccpp-framework, but nobody has reviewed NOAA-EMC/GFDL_atmos_cubed_sphere#86. NOAA-GFDL/GFDL_atmos_cubed_sphere#315 was approved, but I opened a new PR when I changed the target and rebased.

They both contain the same simple change, so I imagine that the new PR should have implicit approval.

@grantfirl
Copy link
Collaborator Author

OK, NOAA-EMC/GFDL_atmos_cubed_sphere#86 was approved. I think that we're ready to merge.

@jkbk2004
Copy link
Collaborator

jkbk2004 commented Mar 5, 2024

@MatthewPyle-NOAA It will be good idea to clean up commit messages when squash/merge. Do you want me to merge this pr?

@MatthewPyle-NOAA
Copy link
Collaborator

I can merge it after cleaning up the merge message.

@MatthewPyle-NOAA MatthewPyle-NOAA merged commit d53f87a into ufs-community:production/RRFS.v1 Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants