-
Notifications
You must be signed in to change notification settings - Fork 64
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
Remove optional arguments from CCPP (capgen metadata parser and ccpp_prebuild scripts) #408
Remove optional arguments from CCPP (capgen metadata parser and ccpp_prebuild scripts) #408
Conversation
…d ccpp_prebuild scripts
…move_optional_arguments_from_ccpp
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.
Looks okay, thanks!
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. |
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). |
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.
Looks straightforward, thanks.
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. |
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 <#408 (comment)>, 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>.
|
Okay then, proceed. I will add my changes to yours once they get to feature/capgen |
Description
Remove support for optional arguments from
capgen
's metadata parser. The code now raises an Exception when it finds anoptional
attribute. Note that thecapgen
metadata parser is used byccpp_prebuild
.Remove support for optional arguments from
ccpp_prebuild
and all its helper scripts.Fix two failing doctests in
metavar.py
andparse_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
#408
earth-system-radiation/rte-rrtmgp#143
NCAR/ccpp-physics#766
NOAA-EMC/fv3atm#416
ufs-community/ufs-weather-model#892
ESCOMP/CCPPStandardNames#23
For regression testing with the UFS, see ufs-community/ufs-weather-model#892.