NCAR / ccpp-framework

Common Community Physics Package (CCPP)
http://www.dtcenter.org/community-code/common-community-physics-package-ccpp/
Other
26 stars 64 forks source link

Remove optional arguments from CCPP (capgen metadata parser and ccpp_prebuild scripts) #408

Closed climbfuji closed 2 years ago

climbfuji commented 2 years ago

Description

  1. Remove support for optional arguments from capgen's metadata parser. The code now raises an Exception when it finds an optional attribute. Note that the capgen metadata parser is used by ccpp_prebuild.

  2. Remove support for optional arguments from ccpp_prebuild and all its helper scripts.

  3. Fix two failing doctests in metavar.py and parse_checkers.py.

After this PR, the feature/capgen branch must be updated from main.

User interface changes?

Yes. Need to remove all optional attributes from the metadata.

Note also that the OPTIONAL_ARGUMENTS section in the CCPP prebuild config is ignored.

Issues

Fixes #407

Associated PRs

https://github.com/NCAR/ccpp-framework/pull/408 https://github.com/earth-system-radiation/rte-rrtmgp/pull/143 https://github.com/NCAR/ccpp-physics/pull/766 https://github.com/NOAA-EMC/fv3atm/pull/416 https://github.com/ufs-community/ufs-weather-model/pull/892 https://github.com/ESCOMP/CCPPStandardNames/pull/23

For regression testing with the UFS, see https://github.com/ufs-community/ufs-weather-model/pull/892.

gold2718 commented 2 years ago

BTW, metavar.py is one of the files I have refactored so I will have to wait for this PR to be merged and then brought into feature/capgen before I can open my PR.

climbfuji commented 2 years ago

BTW, metavar.py is one of the files I have refactored so I will have to wait for this PR to be merged and then brought into feature/capgen before I can open my PR.

We could merge it right away into feature/capgen and then later - exactly the same commit history - into main, then there will be no issues later on. Or you create your PR and I resolve the merge conflict later. I prefer option 1.

gold2718 commented 2 years ago

We could merge it right away into feature/capgen and then later

If you want to do that, you will need to close this PR and open a new one. It is okay with me to go that way. We still need another review.

climbfuji commented 2 years ago

We could merge it right away into feature/capgen and then later

If you want to do that, you will need to close this PR and open a new one. It is okay with me to go that way. We still need another review.

We can keep this one open and create a second one to feature/capgen. As long as we don't change the commits in one of them, we'll be fine. Closing this PR is too much work (PR numbers cross-referenced in all the other PRs etc).

gold2718 commented 2 years ago

As long as we don't change the commits in one of them, we'll be fine.

This is a bit of a risk. Do you need the metavar.py change for prebuild? If not, I can add that change to my PR.

climbfuji commented 2 years ago

Yes, I do to prevent "optional" attributes being reintroduced. There is nothing risky about this process, we do this all the time. But anyway, let's proceed with your update of feature/capgen and I will resolve the merge conflict afterwards.

On Oct 26, 2021, at 12:43 PM, goldy @.***> wrote:

As long as we don't change the commits in one of them, we'll be fine.

This is a bit of a risk. Do you need the metavar.py change for prebuild? If not, I can add that change to my PR.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/NCAR/ccpp-framework/pull/408#issuecomment-952212074, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB5C2RJOZ2H7YSWYP6V75YLUI3ZG5ANCNFSM5GVJHGRA. Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

gold2718 commented 2 years ago

There is nothing risky about this process, we do this all the time.

Okay then, proceed. I will add my changes to yours once they get to feature/capgen