-
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
DRAFT TO DISCUSS - add CCPP register phase #582
base: develop
Are you sure you want to change the base?
Conversation
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.
This is great work! Unfortunately, I don't have any way to test this, therefore relying on CI.
scripts/ccpp_capgen.py
Outdated
@@ -594,21 +576,6 @@ def parse_scheme_files(scheme_filenames, run_env, skip_ddt_check=False): | |||
# end for | |||
# end for | |||
# Check for duplicate dynamic constituent routine names |
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.
this comment is no longer valid?
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.
indeed! removed.
scripts/host_cap.py
Outdated
@@ -555,6 +566,22 @@ def write_host_cap(host_model, api, module_name, output_dir, run_env): | |||
spart_args = spart.call_list.variable_list() | |||
for sp_var in spart_args: | |||
stdname = sp_var.get_prop_value('standard_name') | |||
# Skip any dynamic constituent objects in register phases |
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 this comment is confusing, because the next block 570-584 does something ONLY for the constituents and ONLY in the register phase (and then bails out, i.e. skips whatever comes later).
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.
good catch! that comment was outdated. Updated to:
# Special handling for run-time constituents in register phase
scripts/host_cap.py
Outdated
call_str = suite_part_call_list(host_model, const_dict, spart, False, | ||
dyn_const=True) | ||
stmt = "call {}_{}({})" | ||
cap.write(stmt.format(suite.name, stage, call_str), 3) |
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 no f-string in line 700 and then remove line 699?
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.
changed to f-string
scripts/suite_objects.py
Outdated
self.call_list.add_variable(newvar, self.run_env, | ||
exists_ok=exists_ok, | ||
gen_unique=gen_unique, | ||
adjust_intent=True) | ||
# We need to make sure that this variable's dimensions are available | ||
for vardim in newvar.get_dim_stdnames(include_constants=False): | ||
if vardim == '': |
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.
How can vardim
be empty? This should be an error, or?
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.
vardim can be empty if the variable is allocatable. I added a check only allow this when the variable is allocatable and to otherwise throw an error
for vardim in newvar.get_dim_stdnames(include_constants=False):
# Unnamed dimensions are ok for allocatable variables
if vardim == '' and newvar.get_prop_value('allocatable'):
continue
elif vardim == '':
emsg = f"{self.name}: Cannot have unnamed/empty string dimension"
raise ParseInternalError(emsg.format(self.name,
vardim, stdname))
# end if
...
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.
Thanks for addressing my comments! I'll leave the PR unapproved for now, since it is still in draft mode and I don't have any other tests than what is in CI. Once it is ready for review and others have tested it successfully, I'll approve.
… register-phase
Overview
This PR adds a new phase, register, that can be called by a host model and used by schemes to perform any set up that needs to happen BEFORE the grid is established.
NOTE: this PR also removes the old
dynamic_constituent_routine
metadata implementation for runtime constituents.Description
I have implemented it as an "optional" phase, by which I mean that it is not required that a host model call this phase (though I'm happy to be overruled!). As a result, the register phase does not change the CCPP "state" (but will produce an error if it is called after the
init
phase).More:
Dynamic/run-time constituent handling:
ccpp_constituent_properties_t
variable required):dynamic_constituents_for_<scheme>
<suite>_dynamic_constituents
, which are then used to pack and initialize the module level constituents object<host>_constituents_obj
.Generated host cap code examples
Misc notes
Since this phase is called before the grid is initialized, variables are not allocated at this time (that still happens in
init
) and no variables with horizontal and vertical dimensions can be passed in.UI Changes
User interface changes?: Yes, but they're optional
If a host model wishes to utilize schemes' register phases, they must add a call to
<host_model>_ccpp_physics_register(suite_name, errmsg, errflg)
Testing
test removed: removed unit tests for dyn_const_routines (old implementation of runtime constituent handling)
unit tests: Removed old dynamic constituents testing
system tests: Updated capgen and advection tests to include register phases (with and without dynamic constituents)
manual testing:
This update needs some additional testing and error handling (will implement during/after discussion), such as:
Fixes:
closes #572