-
Notifications
You must be signed in to change notification settings - Fork 65
Add capability to add new kind definitions to capgen #713
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
base: develop
Are you sure you want to change the base?
Conversation
climbfuji
left a comment
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 reasonable to me
| result[key.strip()] = value.split(',') | ||
| return result | ||
|
|
||
| parser.add_argument("--kind-types", type=parse_kind_dict, |
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.
I think that a more modern way of specifying multiple kind types on the command line would be to allow multiple occurrences of --kind-type. But there may be other argument for capgen that use semicolons to pass multiple values to an argument.
--kind-type kind_r8=iso_fortran_env,real64 --kind-type kind_phys=iso_fortran_env,real32 ...
gold2718
left a comment
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.
@nusbaume, thanks for addressing this need!
I have several small concerns which I document inline. I also have a few larger concerns:
- Although not explicitly documented, the
--kind-typeargument to ccpp_capgen was developed as a way to make it easy for a host model to redefine basic CCPP kinds. The case documented in #712 seems to me to be the opposite of that in that this type was created explicitly to avoid overriding by the host model (or at least to insulate PUMAS internal data from changes to ther8kind). - This PR forces host model builders to know and document any kinds defined in the suites they happen to be using for any experiment. I suppose they could include kinds that are not in use but that could be confusing and error prone.
- The CCPP style is to document data and interfaces in the metadata. I feel that these new types should be somewhere in the suite's metadata -- ideally in the metadata associated with the module that defines the new kind. If the group sentiment agrees with this philosophy, I have a branch I could submit as a PR to this PR branch that adds a metadata table to the kinds module.
- I like the old
--kind-typeuser interface and would like to return to it. It did not need any module information since it was using well-known kinds. Still, if we want the ability to use kinds that are not standard (why?), I can see modifying the RHS while still retaining the multiple use syntax (as also suggested by @climbfuji).
| msg = f'Writing {KINDS_FILENAME} to {output_dir}' | ||
| run_env.logger.info(msg) |
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.
Is lazy-style formatting no longer preferred for logging messages (to avoid unnecessary formatting)?
| if kind_type in self.__kind_dict: | ||
| # The kind module should always be | ||
| # the first element in the list: | ||
| kind_mod = self.__kind_dict[kind_type][0] |
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.
Do we need a check here to make sure we have an iterable with at least one item or some other sanity check? I suppose a failure here would be an internal error but we might get a cleaner one here by checking.
| kind_spec = self.__kind_dict[kind_type] | ||
| # The kind specification should always be | ||
| # the second element in the list: | ||
| kind_spec = self.__kind_dict[kind_type][1] |
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.
Do we need a check here as well?
|
|
||
| parser.add_argument("--kind-types", type=parse_kind_dict, | ||
| metavar="kind_types", default=dict(), | ||
| help="""Data size for real(<kind_types>) data. |
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.
Why just real types? For instance, doesn't PUMAS define a kind for integers?
| dict_entries = arg_str.split(':') | ||
| for entry in dict_entries: | ||
| key, value = entry.split('=') | ||
| result[key.strip()] = value.split(',') |
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.
Should value be checked to make sure it has exactly one comma?
| <kind_val> SHOULD be a valid ISO_FORTRAN_ENV type""") | ||
| # Define parser for kind dictionary input | ||
| def parse_kind_dict(arg_str): | ||
| """Parse a 'key1=value1:key2=value2' string |
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.
Would doctests be useful here? If I am reading the code correctly, this function is used by the argparse library which makes it responsible for input-data sanitation for this keyword. Am I thinking about it the wrong way?
|
@gold2718 Ahh, my mistake! I just assumed that the |
I think mistake is too strong a word here -- it's not like there is some documentation you forgot to read :)
I can add this to my PR as I already have the mods necessary to add the kind to the metadata (with some tests). The biggest issue I have with my mods is that they allow a module metadata header into the suite world. Once it is visible, we need to think about whether or not there might be unintended side effects. |
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.
@nusbaume Thanks for adding this feature.
I don't have a strong opinion on kind-types vs. kind-type. We use kind-type in the CCPP SCM, but switching to kind-types would be trivial.
|
@gold2718 i'm not sure what you mean by:
But I'm happy to comment once you open the PR (at which point hopefully I understand the context!) |
Adds the ability for the host model to provide additional kind definitions to the auto-generated
ccpp_kinds.F90file.This PR provides the ability for a host model, during the creation of the cap in capgen, to provide a dictionary of kind types (the kind type name, module, and specification/name within the module), which then add the kind to
ccpp_kinds.F90, and thus make it accessible throughout the CCPP cap. An example of this dictionary being used in practice can be found in a branch of CAM-SIMA here:https://github.com/nusbaume/CAM-SIMA/blob/pumas_round3/cime_config/cam_autogen.py#L497-L504
Additional info about what this PR is specifically trying to fix can also be found in issue #712
User interface changes?: Yes
The
kind_typesargument forCCPPFrameworkEnvhas been changed from a string to a dictionary. If one is using command-line arguments then it thekind_typesargument has changed fromkind_type=kind_spectokind_type=kind_module,kind_spec, with a colon-delimiter for multiple definitions. Examples should be findable in the comments/testing code provided in this PR.Fixes:
Fixes #712 - Need ability to expose scheme-defined kind to cap in capgen
Testing:
test removed: N/A
unit tests: Updated python tests to properly use the new
kind_typesdictionary structure.system tests: Added new kind to
capgen_testto ensure this fucntionality works as expected.manual testing: Tested this functionality in CAM-SIMA with the PUMAS cloud microphysics scheme (which has its own kind definition).