Merge "Add new debugging capability to detect array size mismatches in ccpp_prebuild" and "Cleanup CCPP cmake build" (main branch), and "Remove optional arguments from CCPP" (#408) into feature/capgen

[climbfuji]

Description

This PR merges the contents of main, which consists of the changes in PR https://github.com/NCAR/ccpp-framework/pull/404 (Rename --debug flag to --verbose, add new debugging capability to detect array size mismatches, pretty-print auto-generated caps) and https://github.com/NCAR/ccpp-framework/pull/382 (Cleanup CCPP cmake build), and the changes in PR https://github.com/NCAR/ccpp-framework/pull/408 (Remove optional arguments from CCPP (capgen metadata parser and ccpp_prebuild scripts)) into feature/capgen.

Doing this instead of waiting for https://github.com/NCAR/ccpp-framework/pull/408 and then updating feature/capgen from main will prevent conflicts with ongoing refactoring work that will be merged into feature/capgen shortly.

This PR changes ccpp_prebuild related files only except for the following two files:

scripts/metavar.py
scripts/parse_tools/parse_checkers.py

The changes are related to fixing two doc tests and removing the optional argument from CCPP metadata.

Testing

Aside from the successful, full regression tests with the UFS, I ran the following doctests without errors:

python metadata_table.py
python metavar.py
python parse_log.py
python parse_checkers.py
python parse_object.py
python parse_source.py
python preprocess.py

[gold2718]

I am not sure where to document this but I think we need to clarify what tests should be run in feature/capgen (and will be needed in main for testing capgen when that becomes the default). Here is my current list (all starting from the repo root):

• ./test/capgen_test/run_test
• ./test/advection_test/run_test
• ./test/run_doctest.sh
• (for test inls test/unit_tests/test*.py; do python3 ${test}; done)

Let me know when you have run them. Also let me know if we should add a script or two to help automate this.

[climbfuji]

I am not sure where to document this but I think we need to clarify what tests should be run in feature/capgen (and will be needed in main for testing capgen when that becomes the default). Here is my current list (all starting from the repo root):

• ./test/capgen_test/run_test
• ./test/advection_test/run_test
• ./test/run_doctest.sh
• (for test inls test/unit_tests/test*.py; do python3 ${test}; done)

Let me know when you have run them. Also let me know if we should add a script or two to help automate this.

This is useful to know. Either the README.md in the code, or the wiki seem to be good places for keeping such information.

The first test already fails (./test/capgen_test/run_test):

-- Running: /Users/dom.heinzeller/work/ccpp-framework/ccpp-framework-feature-capgen-merge-remove-optional-arguments-from-ccpp/scripts/ccpp_capgen.py --host-files test_host_data.meta,test_host_mod.meta,test_host.meta --scheme-files temp_scheme_files.txt,ddt_suite_files.txt --suites ddt_suite.xml,temp_suite.xml --host-name test_host --output-root /Users/dom.heinzeller/work/ccpp-framework/ccpp-framework-feature-capgen-merge-remove-optional-arguments-from-ccpp/ct_build/ccpp
-- test_suites: 'suite_info' is not a valid derived Fortran type, at /Users/dom.heinzeller/work/ccpp-framework/ccpp-framework-feature-capgen-merge-remove-optional-arguments-from-ccpp/test/capgen_test/test_host.F90:195
Invalid metadata variable property, 'optional', at /Users/dom.heinzeller/work/ccpp-framework/ccpp-framework-feature-capgen-merge-remove-optional-arguments-from-ccpp/test/capgen_test/temp_adjust.F90:31

CMake Error at CMakeLists.txt:160 (MESSAGE):
  CCPP cap generation FAILED: result = 1

Apparently, capgen wants that a variable carrying a Fortran optional attribute to also have it in the CCPP metadata. We should discuss if this is what we want or not (on Thursday). For example, there are many physics parameterizations that have optional Fortran arguments, and those would work just fine with CCPP not caring about this Fortran attribute. The (hard-coded) presence or absence of certain variables in the metadata would determine how a scheme is run in CCPP, allowing developers to use the optional Fortran syntax in other models outside of CCPP. This is, for example, how RRTMG worked all the time. Certain Fortran optional variables were never listed in the metadata and therefore never passed to the schemes, others were listed (w/o the optional argument) and were always passed to the schemes. But I can also see disadvantages to this approach.

There are also failures for ./test/run_doctest.sh.

I suggest we table this merge for now and discuss how exactly what we want w.r.t. the optional Fortran attribute on Thursday. Please continue with your update of feature/capgen and let's resolve any merge conflicts later when we bring updates from main (plus yet to be determined modifications) to feature/capgen.

[gold2718]

Apparently, capgen wants that a variable carrying a Fortran optional attribute to also have it in the CCPP metadata.

No, I think this was put in the test to explicitly test that optional Fortran arguments did not have to be in the metadata. This currently works in capgen so I see two paths:

1. Remove that requirement (no idea how much code could be impacted but I'd have to check on our side).
2. Fix the PR so that the tests pass

Thoughts?

[gold2718]

Note, as I mentioned last week, I cannot make this week's Framework meeting. I should have mentioned that I am in favor of keeping the current ability to have optional Fortran arguments that are omitted from the metadata.

[climbfuji]

Note, as I mentioned last week, I cannot make this week's Framework meeting.

Thanks for reminding me, of course I forgot already.

I should have mentioned that I am in favor of keeping the current ability to have optional Fortran arguments that are omitted from the metadata.

Great, because this is also what NOAA prefers.

[climbfuji]

Apparently, capgen wants that a variable carrying a Fortran optional attribute to also have it in the CCPP metadata.

No, I think this was put in the test to explicitly test that optional Fortran arguments did not have to be in the metadata. This currently works in capgen so I see two paths:

1. Remove that requirement (no idea how much code could be impacted but I'd have to check on our side).
2. Fix the PR so that the tests pass

Thoughts?

I think the reason for the failure is that the Fortran parser converts the Fortran variable properties into the same class as the metadata is converted to, and that class doesn't know the optional attribute anymore.

[gold2718]

I think the reason for the failure is that the Fortran parser converts the Fortran variable properties into the same class as the metadata is converted to, and that class doesn't know the optional attribute anymore.

Right. I have a good idea of a clean way to fix this. I suggest:

1. I add this to my code and submit this to feature/capgen.
2. We merge to main and add in the rest of the changes to remove optional
3. We merge main back to feature/capgen

Sound okay?

[climbfuji]

I think the reason for the failure is that the Fortran parser converts the Fortran variable properties into the same class as the metadata is converted to, and that class doesn't know the optional attribute anymore.

Right. I have a good idea of a clean way to fix this. I suggest:

1. I add this to my code and submit this to feature/capgen.
2. We merge to main and add in the rest of the changes to remove optional
3. We merge main back to feature/capgen

Sound okay?

Except that we can't merge to main without delaying my PRs for the UFS. Can we just apply the fix to my PR/branch?

[gold2718]

Except that we can't merge to main without delaying my PRs for the UFS. Can we just apply the fix to my PR/branch?

Sigh, it's always something ;)

The merge is going to be wacko because metavar.py has been through a lot of changes. As long as metavar.py ends up the same in main as in feature/capgen, we should be okay so I think you should merge #408 and close this PR. So now, is the plan:

1. You merge #408
2. I open a new feature/capgen PR which includes removing the optional keyword from metadata files
3. When that PR is merged, we merge it to main
4. We then merge main back to feature/capgen

or is the plan:

1. You merge #408
2. We merge main to feature/capgen
3. I open a new feature/capgen PR which includes removing the optional keyword from metadata files
4. When that is merged, merge feature/capgen to main

They both seem okay to me.

[climbfuji]

Let's go with option 2 then. Thanks!

On Oct 26, 2021, at 4:46 PM, goldy @.***> wrote:

Except that we can't merge to main without delaying my PRs for the UFS. Can we just apply the fix to my PR/branch?

Sigh, it's always something ;)

The merge is going to be wacko because metavar.py has been through a lot of changes. As long as metavar.py ends up the same in main as in feature/capgen, we should be okay so I think you should merge #408 https://github.com/NCAR/ccpp-framework/pull/408 and close this PR. So now, is the plan:

You merge #408 https://github.com/NCAR/ccpp-framework/pull/408 I open a new feature/capgen PR which includes removing the optional keyword from metadata files When that PR is merged, we merge it to main We then merge main back to feature/capgen or is the plan:

You merge #408 https://github.com/NCAR/ccpp-framework/pull/408 We merge main to feature/capgen I open a new feature/capgen PR which includes removing the optional keyword from metadata files When that is merged, merge feature/capgen to main They both seem okay to me.

— 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/409#issuecomment-952384988, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB5C2RM64JMTX3OQE6D3OJDUI4VSVANCNFSM5GYTWQ6Q. 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.