Skip to content
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.

Sign up for GitHub

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

Capgen in SCM: Differing source and module name #637

Closed
wants to merge 5 commits into from
Closed

Capgen in SCM: Differing source and module name #637

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c135527
Work in progress
dustinswales Feb 3, 2025
03429da
Allow for module/source filename differences. Add test
dustinswales Feb 3, 2025
54f6f92
Merge branch 'develop' of https://github.com/NCAR/ccpp-framework into…
dustinswales Feb 3, 2025
690215f
Add logic to allow for module name to come from initialization. Doc t…
dustinswales Feb 3, 2025
c987241
Merge branch 'develop' of https://github.com/NCAR/ccpp-framework into…
dustinswales Feb 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion scripts/ccpp_suite.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -657,7 +657,8 @@ def __init__(self, sdfs, host_model, scheme_headers, run_env):
self.__ddt_lib = DDTLibrary('{}_api'.format(self.host_model.name),
run_env, ddts=all_ddts)
for header in [d for d in scheme_headers if d.header_type != 'ddt']:
if header.header_type != 'scheme':
# DJS2024: Schemes and modules with DDTs?
if header.header_type != 'scheme' and header.header_type != 'module':
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@climbfuji Here's another thing I needed to add. Capgen wasn't happy with modules that held DDT definitions (e.g. radlw_param.f).
I convinced myself that this is not intentional and probably due to a lack of testing Capgen with respect to DDTs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gold2718 @peverwhee Fortran modules containing DDTs should be allowed, but I want to be sure this change doesn't have any unintentional consequences elsewhere?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dustinswales I can't think of any reason why this wouldn't be ok.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree. The problem here is that you are using a host file (module_rad_ddt.meta) as a scheme file. The module table type is not (currently?) allowed in capgen. The error message could be improved (see below) but:

  • I do not see why the module header is needed or useful here
  • Defining a DDT used by both a scheme and the host makes the scheme inherently non-portable -- something the CCPP was created to avoid.

Copy link
Collaborator Author

@dustinswales dustinswales Mar 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gold2718

  • I do not see why the module header is needed or useful here

There are schemes with native DDTs which the host needs to know about (e.g. PUMAS, RRTMGP). How should this be handled with Capgen?

  • Defining a DDT used by both a scheme and the host makes the scheme inherently non-portable -- something the CCPP was created to avoid.

Fair point, but we need to accommodate schemes that come to the party with their own DDTs.

Also, what about the other way, that is interstitial schemes that use host-defined data containers? (e.g. GFS_phys_reset.F90)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gold2718 The issue with SCM is that the metadata changes introduced in #646 won't work with the physics in Prebuild, which is something I want to avoid in this transition. Allowing a module header here would avoid that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that the changes in 646 were optional ("An optional module_name keyword is added to the ccpp-table-properties (MetadataTable) section that allows the user to specify a Fortran module name that is independent from the name of the enclosing file."). Are you saying they are not sufficient or break existing prebuild functionality? Are you trying to run the same physics with both prebuild and capgen? Maybe we can talk about this at our next meeting.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The optional part is fine. It's not allowing a module header in the metadata that will break Prebuild. (There is no mechanism in Prebuild to handle the different module/file names w/o using the module header)
Yeah, up to this point I was able to use the same physics in both Prebuild/Capgen.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The optional part is fine. It's not allowing a module header in the metadata that will break Prebuild. (There is no mechanism in Prebuild to handle the different module/file names w/o using the module header) Yeah, up to this point I was able to use the same physics in both Prebuild/Capgen.

Is it necessary to open this (big IMHO) hole in capgen in order to allow the transition? Once the module header is allowed in capgen for schemes, anyone could use module metadata to completely circumvent at least one of the Kalnay rules (Rule 3: All communication with the package shall be through the argument list at the entry points). Since this is also a current CCPP rule for physics, I think it should warrant more discussion before adopting it just to work around an issue in prebuild.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gold2718 It's not necessary. I'm going to do some physics cleanup to follow the metadata section definitions you detailed in #648.

errmsg = "{} is an unknown CCPP API metadata header type, {}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
errmsg = "{} is an unknown CCPP API metadata header type, {}"
if header.header_type == 'module':
errmsg = f"{header.title} is a module metadata header type."
errmsg+=" This is not an allowed CCPP scheme header type."
else:
errmsg = f"{header.title} is an unknown CCPP API metadata header type, {header.header_type}"
# end if

raise CCPPError(errmsg.format(header.title, header.header_type))
# end if
Expand Down
56 changes: 53 additions & 3 deletions scripts/metadata_table.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,51 @@ def parse_metadata_file(filename, known_ddts, run_env, skip_ddt_check=False):

########################################################################

def find_module_name(filename):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am surprised that this isn't already done by the parser. Is that because everything is based on the assumption filename=modulename?

Copy link
Collaborator Author

@dustinswales dustinswales Feb 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. Metadata tables for new DDTs default to the source filename (e.g. here). Not the best.
This is a problem for DDTs in two instances, a) DDTs housed in differing module/file names and b) DDTs in files with multiple modules.
We don't have any of the latter in the SCM/UFS, so I haven't gone down that rabbit hole.

"""Find the module name from module header in <filename>"""
module_name = ''
if os.path.isfile(filename):
with open(filename, 'r') as infile:
fin_lines = infile.readlines()
# end with
num_lines = len(fin_lines)
context = ParseContext(linenum=1, filename=filename)
while context.line_num <= num_lines:
if MetadataTable.table_start(fin_lines[context.line_num - 1]):
found_start = False
while not found_start:
line = fin_lines[context.line_num].strip()
context.line_num += 1
if line and (line[0] == '['):
found_start = True
elif line:
props = _parse_config_line(line, context)
for prop in props:
# Look for name property
key = prop[0].strip().lower()
value = prop[1].strip()
if key == 'name' :
name = value
if key == 'type' :
if (value == 'module') or (value == 'scheme'):
module_name = name
break
# end if
# end for
# end if
if context.line_num > num_lines:
break
# end if
# end while
else:
context.line_num += 1
# end if
# end while
# end if
return module_name

########################################################################

def find_scheme_names(filename):
"""Find and return a list of all the physics scheme names in
<filename>. A scheme is identified by its ccpp-table-properties name.
Expand Down Expand Up @@ -813,9 +858,14 @@ def __init_from_file(self, table_name, table_type, known_ddts, run_env, skip_ddt
if self.header_type == "ddt":
known_ddts.append(self.title)
# end if
# We need a default module if none was listed
if self.module is None:
self.__module_name = self._default_module()
# We need a default module if none was listed.
# DJS2024: If module_name not provided through initialization, try
# to find module_name from the metadata. Otherwise, use file name
# as module_name (default).
if (self.__module_name == None):
self.__module_name = find_module_name(self.__pobj.filename)
if (self.__module_name == ''):
self.__module_name = self._default_module()
# end if
# Initialize our ParseSource parent
super().__init__(self.title, self.header_type, self.__pobj)
Expand Down
2 changes: 1 addition & 1 deletion test/var_compatibility_test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ get_filename_component(CCPP_ROOT "${TEST_ROOT}" DIRECTORY)
#
#------------------------------------------------------------------------------
LIST(APPEND SCHEME_FILES "var_compatibility_files.txt")
LIST(APPEND HOST_FILES "test_host_data" "test_host_mod")
LIST(APPEND HOST_FILES "module_rad_ddt" "test_host_data" "test_host_mod")
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gold2718 @peverwhee @climbfuji Design philosophy question below.

When a physics scheme has a native DDT that the host needs to be aware of, the file containing the DDT needs to be at the top of both the scheme_files and host_files list (e.g. host_files and scheme_files in CCPP SCM). In the SCM/UFS, we also do the inverse and use host defined DDTs within Interstitial physics schemes, so there is a need for a pool of DDTs available to both the host and the schemes.

Would it make sense to have a third class of files provided to Capgen, containing the DDT typedefs, that are processed before the scheme and host files?
I'm fine with having to define the host/scheme lists with the typedefs at the top, just curious to hear your thoughts on this? (I'm also not volunteering to do this!)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That third class would be a mix of scheme and host files. Something like this exists in prebuild, see for example https://github.com/NOAA-EMC/fv3atm/blob/2c902a670e89416ef49254c827e8ba45a68ce596/ccpp/config/ccpp_prebuild_config.py#L12

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for volunteering to do this, @dustinswales ! :)

I'm barely following what y'all are talking about. Would your proposed solution mean that no DDTs could be included within scheme or host files? They'd all have to be separate? Or would this additional class of files contain some duplicates vs the host and scheme files? Is that what @climbfuji is saying?

LIST(APPEND SUITE_FILES "var_compatibility_suite.xml")
# HOST is the name of the executable we will build.
# We assume there are files ${HOST}.meta and ${HOST}.F90 in CMAKE_SOURCE_DIR
Expand Down
28 changes: 28 additions & 0 deletions test/var_compatibility_test/module_rad_ddt.F90
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
module mod_rad_ddt
USE ccpp_kinds, ONLY: kind_phys
implicit none

public ty_rad_lw, ty_rad_sw

!> \section arg_table_mod_rad_ddt Argument Table
!! \htmlinclude arg_table_mod_rad_ddt.html
!!

!> \section arg_table_ty_rad_lw Argument Table
!! \htmlinclude arg_table_ty_rad_lw.html
!!
type ty_rad_lw
real(kind_phys) :: sfc_up_lw
real(kind_phys) :: sfc_down_lw
end type ty_rad_lw

!> \section arg_table_ty_rad_sw Argument Table
!! \htmlinclude arg_table_ty_rad_sw.html
!!
type ty_rad_sw
real(kind_phys) :: sfc_up_sw
real(kind_phys) :: sfc_down_sw
end type ty_rad_sw

end module mod_rad_ddt

45 changes: 45 additions & 0 deletions test/var_compatibility_test/module_rad_ddt.meta
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
[ccpp-table-properties]
name = mod_rad_ddt
type = module
[ccpp-arg-table]
name = mod_rad_ddt
type = module

[ccpp-table-properties]
name = ty_rad_lw
type = ddt
dependencies =
[ccpp-arg-table]
name = ty_rad_lw
type = ddt
[ sfc_up_lw ]
standard_name = surface_upwelling_longwave_radiation_flux
units = W m2
dimensions = ()
type = real
kind = kind_phys
[ sfc_down_lw ]
standard_name = surface_downwelling_longwave_radiation_flux
units = W m2
dimensions = ()
type = real
kind = kind_phys

[ccpp-table-properties]
name = ty_rad_sw
type = ddt
[ccpp-arg-table]
name = ty_rad_sw
type = ddt
[ sfc_up_sw ]
standard_name = surface_upwelling_shortwave_radiation_flux
units = W m2
dimensions = ()
type = real
kind = kind_phys
[ sfc_down_sw ]
standard_name = surface_downwelling_shortwave_radiation_flux
units = W m2
dimensions = ()
type = real
kind = kind_phys
35 changes: 35 additions & 0 deletions test/var_compatibility_test/rad_lw.F90
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
module rad_lw
use ccpp_kinds, only: kind_phys
use mod_rad_ddt, only: ty_rad_lw

implicit none
private

public :: rad_lw_run

contains

!> \section arg_table_rad_lw_run Argument Table
!! \htmlinclude arg_table_rad_lw_run.html
!!
subroutine rad_lw_run(ncol, fluxLW, errmsg, errflg)

integer, intent(in) :: ncol
type(ty_rad_lw), intent(inout) :: fluxLW(:)
character(len=512), intent(out) :: errmsg
integer, intent(out) :: errflg

! Locals
integer :: icol

errmsg = ''
errflg = 0

do icol=1,ncol
fluxLW(icol)%sfc_up_lw = 300._kind_phys
fluxLW(icol)%sfc_down_lw = 50._kind_phys
enddo

end subroutine rad_lw_run

end module rad_lw
35 changes: 35 additions & 0 deletions test/var_compatibility_test/rad_lw.meta
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
[ccpp-table-properties]
name = rad_lw
type = scheme
dependencies = module_rad_ddt.F90
[ccpp-arg-table]
name = rad_lw_run
type = scheme
[ ncol ]
standard_name = horizontal_loop_extent
type = integer
units = count
dimensions = ()
intent = in
[fluxLW]
standard_name = longwave_radiation_fluxes
long_name = longwave radiation fluxes
units = W m-2
dimensions = (horizontal_loop_extent)
type = ty_rad_lw
intent = inout
[ errmsg ]
standard_name = ccpp_error_message
long_name = Error message for error handling in CCPP
units = none
dimensions = ()
type = character
kind = len=512
intent = out
[ errflg ]
standard_name = ccpp_error_code
long_name = Error flag for error handling in CCPP
units = 1
dimensions = ()
type = integer
intent = out
35 changes: 35 additions & 0 deletions test/var_compatibility_test/rad_sw.F90
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
module rad_sw
use ccpp_kinds, only: kind_phys
use mod_rad_ddt, only: ty_rad_sw

implicit none
private

public :: rad_sw_run

contains

!> \section arg_table_rad_sw_run Argument Table
!! \htmlinclude arg_table_rad_sw_run.html
!!
subroutine rad_sw_run(ncol, fluxSW, errmsg, errflg)

integer, intent(in) :: ncol
type(ty_rad_sw), intent(inout) :: fluxSW(:)
character(len=512), intent(out) :: errmsg
integer, intent(out) :: errflg

! Locals
integer :: icol

errmsg = ''
errflg = 0

do icol=1,ncol
fluxSW(icol)%sfc_up_sw = 100._kind_phys
fluxSW(icol)%sfc_down_sw = 400._kind_phys
enddo

end subroutine rad_sw_run

end module rad_sw
35 changes: 35 additions & 0 deletions test/var_compatibility_test/rad_sw.meta
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
[ccpp-table-properties]
name = rad_sw
type = scheme
dependencies = module_rad_ddt.F90
[ccpp-arg-table]
name = rad_sw_run
type = scheme
[ ncol ]
standard_name = horizontal_loop_extent
type = integer
units = count
dimensions = ()
intent = in
[fluxSW]
standard_name = shortwave_radiation_fluxes
long_name = shortwave radiation fluxes
units = W m-2
dimensions = (horizontal_loop_extent)
type = ty_rad_sw
intent = inout
[ errmsg ]
standard_name = ccpp_error_message
long_name = Error message for error handling in CCPP
units = none
dimensions = ()
type = character
kind = len=512
intent = out
[ errflg ]
standard_name = ccpp_error_code
long_name = Error flag for error handling in CCPP
units = 1
dimensions = ()
type = integer
intent = out
14 changes: 10 additions & 4 deletions test/var_compatibility_test/run_test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,8 +127,8 @@ ccpp_files="${utility_files}"
ccpp_files="${ccpp_files},${build_dir}/ccpp/test_host_ccpp_cap.F90"
ccpp_files="${ccpp_files},${build_dir}/ccpp/ccpp_var_compatibility_suite_cap.F90"
#process_list=""
module_list="effr_calc,effr_diag,effr_post,effr_pre"
#dependencies=""
module_list="effr_calc,effr_diag,effr_post,effr_pre,rad_lw,rad_sw"
dependencies="module_rad_ddt"
suite_list="var_compatibility_suite"
required_vars_var_compatibility="ccpp_error_code,ccpp_error_message"
required_vars_var_compatibility="${required_vars_var_compatibility},cloud_graupel_number_concentration"
Expand All @@ -143,12 +143,14 @@ required_vars_var_compatibility="${required_vars_var_compatibility},flag_indicat
required_vars_var_compatibility="${required_vars_var_compatibility},horizontal_dimension"
required_vars_var_compatibility="${required_vars_var_compatibility},horizontal_loop_begin"
required_vars_var_compatibility="${required_vars_var_compatibility},horizontal_loop_end"
required_vars_var_compatibility="${required_vars_var_compatibility},longwave_radiation_fluxes"
required_vars_var_compatibility="${required_vars_var_compatibility},num_subcycles_for_effr"
required_vars_var_compatibility="${required_vars_var_compatibility},scalar_variable_for_testing"
required_vars_var_compatibility="${required_vars_var_compatibility},scalar_variable_for_testing_a"
required_vars_var_compatibility="${required_vars_var_compatibility},scalar_variable_for_testing_b"
required_vars_var_compatibility="${required_vars_var_compatibility},scalar_variable_for_testing_c"
required_vars_var_compatibility="${required_vars_var_compatibility},scheme_order_in_suite"
required_vars_var_compatibility="${required_vars_var_compatibility},shortwave_radiation_fluxes"
required_vars_var_compatibility="${required_vars_var_compatibility},vertical_layer_dimension"
input_vars_var_compatibility="cloud_graupel_number_concentration"
#input_vars_var_compatibility="${input_vars_var_compatibility},cloud_ice_number_concentration"
Expand All @@ -161,21 +163,25 @@ input_vars_var_compatibility="${input_vars_var_compatibility},flag_indicating_cl
input_vars_var_compatibility="${input_vars_var_compatibility},horizontal_dimension"
input_vars_var_compatibility="${input_vars_var_compatibility},horizontal_loop_begin"
input_vars_var_compatibility="${input_vars_var_compatibility},horizontal_loop_end"
input_vars_var_compatibility="${input_vars_var_compatibility},longwave_radiation_fluxes"
input_vars_var_compatibility="${input_vars_var_compatibility},num_subcycles_for_effr"
input_vars_var_compatibility="${input_vars_var_compatibility},scalar_variable_for_testing"
input_vars_var_compatibility="${input_vars_var_compatibility},scalar_variable_for_testing_a"
input_vars_var_compatibility="${input_vars_var_compatibility},scalar_variable_for_testing_b"
input_vars_var_compatibility="${input_vars_var_compatibility},scalar_variable_for_testing_c"
input_vars_var_compatibility="${input_vars_var_compatibility},scheme_order_in_suite"
input_vars_var_compatibility="${input_vars_var_compatibility},shortwave_radiation_fluxes"
input_vars_var_compatibility="${input_vars_var_compatibility},vertical_layer_dimension"
output_vars_var_compatibility="ccpp_error_code,ccpp_error_message"
output_vars_var_compatibility="${output_vars_var_compatibility},cloud_ice_number_concentration"
output_vars_var_compatibility="${output_vars_var_compatibility},effective_radius_of_stratiform_cloud_ice_particle"
output_vars_var_compatibility="${output_vars_var_compatibility},effective_radius_of_stratiform_cloud_liquid_water_particle"
output_vars_var_compatibility="${output_vars_var_compatibility},effective_radius_of_stratiform_cloud_rain_particle"
output_vars_var_compatibility="${output_vars_var_compatibility},effective_radius_of_stratiform_cloud_snow_particle"
output_vars_var_compatibility="${output_vars_var_compatibility},longwave_radiation_fluxes"
output_vars_var_compatibility="${output_vars_var_compatibility},scalar_variable_for_testing"
output_vars_var_compatibility="${output_vars_var_compatibility},scheme_order_in_suite"
output_vars_var_compatibility="${output_vars_var_compatibility},shortwave_radiation_fluxes"

##
## Run a database report and check the return string
Expand Down Expand Up @@ -241,8 +247,8 @@ echo -e "\nChecking lists from command line"
#check_datatable ${report_prog} ${datafile} "--process-list" ${process_list}
check_datatable ${report_prog} ${datafile} "--module-list" ${module_list}
#check_datatable ${report_prog} ${datafile} "--dependencies" ${dependencies}
check_datatable ${report_prog} ${datafile} "--suite-list" ${suite_list} \
--sep ";"
#check_datatable ${report_prog} ${datafile} "--suite-list" ${suite_list} \
# --sep ";"
echo -e "\nChecking variables for var_compatibility suite from command line"
check_datatable ${report_prog} ${datafile} "--required-variables" \
${required_vars_var_compatibility} "var_compatibility_suite"
Expand Down
Loading