Replace ccpp-prebuild and ccpp-capgen v0 with ccpp-capgen v1 - #762
Replace ccpp-prebuild and ccpp-capgen v0 with ccpp-capgen v1#762climbfuji wants to merge 83 commits into
Conversation
… feature/capgen-ng
…gen-ng: entire constituent state now per instance
…ts (#764) This PR targets the feature/capgen-v1 branch with follow-up and cleanup changes from manual code reviews (capgen/metadata/parse-tools/*) plus a few selected, minor improvements. This branch is being used by the CAM-SIMA feature branches that replace the original capgen v0 with the new capgen v1. - Temporary: run CI tests also for pull requests against branch feature/capgen-v1. - Simplify capgen/metadata/parse_tools/parse_source.py. Some of the removed features were (ab)used by CAM-SIMA and are now in the updated capgen compatibility layer in the CAM-SIMA feature branch. - Simplify capgen/metadata/parse_tools/parse_log.py. Requires a tiny change in the CAM-SIMA compatibility layer (now in the (CAM-SIMA feature branch). - Add documentation, simplify, and add a comment about a latent bug that doesn't hurt is today w.r.t. escaping periods in decimals in capgen/metadata/parse_tools/fortran_conditional.py. - capgen/metadata/parse_tools/io_helpers.py: better documentation, preserve default umask. - Simplify capgen/metadata/parse_tools/xml_tools.py: remove unneeded arguments, trim down docstring tests (covered in unit tests and end-to-end tests). Requires small changes in the CAM-SIMA compatibility layer (now in the (CAM-SIMA feature branch). - Strip down capgen/metadata/parse_tools/parse_checkers.py, update capgen/metadata/metadata_table.py accordingly. - Not cleanup: Fix the trailing comments after the variable conversions in the Group cap ([feature/capgen-v1-cleanup-01] Add correct comment to supported variable conversions (#767) + bug fixes + unit test updates #768) by @dustinswales. - Not cleanup: Add additional report option --suite-variables to retrieve suite-owned variables from datatable. - Not cleanup: Log variable transformations during capgen call at default logging level (0d48ab0). --------- Co-authored-by: Courtney Peverley <courtneyp@ucar.edu> Co-authored-by: Dustin Swales <dustin.swales@noaa.gov>
Format update of `feature/capgen-v1` from `develop` to resolve conflicts after merging #765 into develop / #767 into `feature/capgen-v1`. There's a small leftover change in `end-to-end-tests/advection_auto_clone/cld_liq.F90` that was made in #765 but not in #767. The other test data change in `test/advection_test/test_host_data.F90` (see https://github.com/NCAR/ccpp-framework/pull/765/changes#diff-04bf1167afd493d2ea37bd03d093e200ba8b563383bfd1cca3fc9126aca9c4a9) is already in `feature/capgen-v1`. Co-authored-by: Courtney Peverley <courtneyp@ucar.edu>
… feature/capgen-v1
gold2718
left a comment
There was a problem hiding this comment.
I am having trouble giving this (moving target) a thorough review but have a question and a couple of concerns. The question is about the protected attribute (see below).
Concern 1: By removing and changing the old unit tests, it is harder to verify that the parser / generator works correctly. How do you verify test coverage relative to the earlier (admittedly not great) version? Are all the tests functionally replicated?
Concern 2: Validation of metadata against the Fortran source has been relegated to capgen/ccpp_validator.py. While running this tool can be built into the build process, the old version ran on the already parsed metadata. At best, this will slow down the validation process as the metadata has to be re-parsed. However, is the metadata in fact parsed the same? Is there a reason this could not be an optional part of capgen that works on the parsed metadata?
| source = 'suite' | ||
| elif host_entry is None and suite_var is None: | ||
| # Case 2 or 3. | ||
| if intent == 'out': |
There was a problem hiding this comment.
I may have missed it, but is the protected attribute checked anywhere? If not, can you introduce a check for host_entry.protected and intent in ('out', 'inout')?
There was a problem hiding this comment.
I'll get back to you on that one.
There was a problem hiding this comment.
Just checking in to see if this got resolved offline since we are nearly ready to process this PR.
There was a problem hiding this comment.
Yes, see PR #773. But this change has no significance for the UFS, since the protected attribute isn't used yet.
I hope that no more updates are needed for now. The most recent update to CAM-SIMA necessitated a number of changes and my limited bandwidth (technically on PTO) didn't allow me to create yet another set of branches and PRs. Future changes required due to CAM-SIMA updates will need to go in yet another staging branch to avoid updating the
This is indeed the case. The existing tests from capgen v0 and prebuild were copied over 1:1, then the validation trimmed down to just the Fortran part where applicable. The blocked-data test for prebuild was removed, because we never intended to support blocked data structures. New tests were added for new functionality (e.g. multi-instance). An additional advection test with the now-deprecated "auto-clone constituents" (= defined in metadata only) enabled via a legacy shim. Several passes were done with the LLM to ensure that the current tests cover everything the old tests did, plus more. But also to remove duplicates between unit tests and docstring tests. [Edit] In addition to an already better test coverage, we benefit greatly from the integration with the four host models. These exercise many different aspects of capgen, and the fact that the regression tests all pass and are bit-for-bit identical for CAM-SIMA, CCPP-SCM, NEPTUNE, and UFS, provides us with real evidence that the features that matter (i.e. that are actually being used) are correct.
This was a deliberate decision. A separation of concerns means that ccpp_capgen doesn't (need to) know anything about parsing Fortran code and can rely on metadata only. Separating these two steps also enables offline validation of metadata vs fortran without the need for a host model, and it's easier to see immediately if the validation step fails as part of the cmake build integration, or if it is the actual cap generation. Yes, the metadata is read twice in this approach, but the measured runtimes speak for themselves. For all 58 suites of CCPP-SCM, these are the runtimes:
It's cleaner, easier to understand, faster, and more aligned with modern software development practices (separation of concerns). |
|
Could we get approvals on this PR so that we can process ufs-community/ufs-weather-model#3288 ASAP? |
|
@gspetro-NOAA As part of 3288, we are switching to a new codebase and branch, feature/capgen-v1. So there are no reviews needed here for the overlaying UWM changes. |
|
Testing for ufs-community/ufs-weather-model#3288 completed successfully. This PR can be merged. |
|
@dustinswales Does this PR need to be merged if we are sticking w/the feature/capgen-v1 branch for the WM? Just trying to make sure I'm not waiting for a merge that won't happen... |
@gspetro-NOAA No merging here. |
|
Hey @dustinswales could you help confirm and/or clarify the direction for ccpp-framework as it relates to UFS? What I think I understand is that all development will be in |
I can answer this. Yes, the plan is to point to |
Thanks for the quick reply @climbfuji ! This seems fair to me. |
|
To all reviewers. Not being able to update this feature branch while waiting for reviews (how long?) is causing real problems. But since several models now point to this branch and reviewing a large set of changes is easier when the code doesn't change under the hood, should we branch off again and create If we were to do this, we could refer to that new branch when addressing change requests from the review of |
I think development should be able to continue as usual on this branch because @dustinswales had specifically been using the c4b7d9a commit in PR 3288, and this PR was not going to be merged. |
Correct - I am mainly thinking about the reviewers. |
|
I found a different way. For all reviewers. If you want to review a stable version without the latest updates, you can use this URL: This compares commit c4b7d9a from the pull request branch ( compares the missing commits from c4b7d9a to the head of the This way, you can do a first pass of your review based on the static commit c4b7d9a, and then look at the additional commits and differences added to |
…les, reorganize how Claude keeps track of future work (#773) For feature/capgen-v1: - Add missing check for protected variables (per metadata), mirroring the functionality in capgen v0 (capgen/generator/suite_resolver.py and capgen/metadata/metadata_table.py). - Reorganize bookkeeping of future/deferred work in Claude so that it travels between different computers/users. Ultimately, at the time we merge feature/capgen-v1 into develop, any open work items will be converted into GitHub issues if still required (all other files).
…74 (name collision in capgen v1) (#776) 1. Fix for issue #774 - host module import name clash with control variables passed via argument list, plus tests. Instead of implementing some black magic, we simply bail out and instruct the user to fix the name collision. This is easy - simply change the "local name" in the `type = control` metadata table, which is only used as as the l.h.s. in the explicit argument lists to the CCPP calls (`call ccpp_physics_run` etc.) and then passed down to the suite and group caps. 2. Also: tests for issue #772 in capgen v0 (not an issue in v1, but adding the same test to demonstrate). --------- Co-authored-by: Courtney Peverley <courtneyp@ucar.edu>
jimmielin
left a comment
There was a problem hiding this comment.
Thanks @climbfuji. Sharing a couple of findings from Claude for correctness bugs. I think all of them should be pretty easy to resolve.
| unit_backward = '' | ||
| if needs_unit and bwd_fn is not None and intent in ('out', 'inout'): | ||
| unit_backward_expr = '{}_l'.format(local) | ||
| unit_backward = _apply_transform_formula(bwd_fn, unit_backward_expr, host_kind) | ||
| elif needs_kind and intent in ('out', 'inout'): | ||
| unit_backward = _kind_cast_expr( | ||
| scheme_var.type, '{}_l'.format(local), host_kind, | ||
| local=local, std_name=std_name, scheme_name=scheme_name, | ||
| ) | ||
| elif needs_vert_flip and not needs_unit and intent in ('out', 'inout'): | ||
| unit_backward = '{}_l'.format(local) | ||
|
|
||
| needs_transform = needs_unit or needs_kind or needs_vert_flip | ||
|
|
||
| # ---- local variable names (transformation temp + pointer) ------------ | ||
| # ``used_local_names`` stores the LOWERCASED names so collision | ||
| # detection is Fortran-case-insensitive (see _local_name_conflict). | ||
| temp_name = '' | ||
| ptr_name = '' | ||
| if needs_transform: | ||
| candidate = '{}_l'.format(local) | ||
| temp_name = _local_name_conflict(candidate, used_local_names) |
There was a problem hiding this comment.
I think because unit_backward is built before _local_name_conflict checks here, there could be a bug if I declare a (rather evil) scheme local as p_hpa_l then at the same time I request a Pa -> hPa unit conversion for [p] standard_name = air_pressure this will end up reading the wrong name for unit_backward.
I would suggest moving this block to above unit_forward (which does not have this bug since it is after the local name conflict resolver).
| ) | ||
| pending_props[key] = val | ||
| # Try to build the MetadataTable as soon as we have name + type. | ||
| if 'name' in pending_props and 'type' in pending_props and current_table is None: |
There was a problem hiding this comment.
This check will cause a .meta table with a missing type (for example, a typo into typ = scheme) to be silently ignored.
I would suggest at the next header after the parse if pending_props is non-empty but no table was built to raise an error saying a key is required.
| if host_allocatable: | ||
| # Allocatable actual arguments must omit explicit dimension ranges: | ||
| # the callee declares the dummy as allocatable too and assumes the | ||
| # array shape from the actual. | ||
| subscript: str = '' | ||
| used_dim_std: Set[str] = set() |
There was a problem hiding this comment.
If host_allocatable is True and subscript = '' then _build_merged_subscript is never called, so flip_vertical=needs_vert_flip, if True, is ignored, but needs_transform is also True at this point so the temp pipeline is used.
I think if needs_vert_flip and host_allocatable is true it should error out (and maybe a xfail test be added to this case in TestVerticalFlipTransform?)
|
Thanks for the reports @jimmielin. I'll take a closer look next week; if you want to propose bug fixes for those, please go ahead. |


Description
This PR replaces the two code generators
ccpp-prebuildandccpp-capgen(version 0) with the new, unifiedccpp-capgen(version 1). It supports all host models (CAM-SIMA, CCPP-SCM, NEPTUNE, UFS) and combines the features fromccpp-capgen v0with the performance and operational robustness ofccpp-prebuild.This pull request will be updated continuously with links to updates of the CCPP technical documentation and additional information. See also #759 for background information and further materials.
User interface changes?: Yes (see documentation)
This feature branch
feature/capgen-v1will support the threeccpp-prebuild-based models CCPP-SCM, NEPTUNE, and UFS, which will transition toccpp-capgen v1before CAM-SIMA. Once CAM-SIMA transitions, this PR will be merged andfeature/capgen-v1will be locked.Todo
doc/need to be reviewed and used to update the existing CCPP technical documentation (https://github.com/ncar/ccpp-doc). Once that process is complete, we can remove or trim down the additional documentation indoc/.Issues
scripts/file_utils.py/move_modified_filesroutine #744To check/may not be implemented yet:
Testing
See #759