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.

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

Host metadata file not flagging mis-named variables #648

Open
mwaxmonsky opened this issue Mar 3, 2025 · 8 comments
Open

Host metadata file not flagging mis-named variables #648

mwaxmonsky opened this issue Mar 3, 2025 · 8 comments
Labels
bug For issues describing bugs, or PRs fixing bugs

Comments

@mwaxmonsky
Copy link
Collaborator

Description

The current ddthost_test has a test_host.meta has a ccpp variable declared in the test_host function block but the corresponding test_host.F90 has the variable named ccpp_info. This is inconsistent with non-host meta files as this is flagged as an error when run against the CCPP Framework.

Steps to Reproduce

Run the integration test in test/ddthost_test as documented from current main branch (current SHA 4102bdb)

Additional Context

N/A

Output

N/A

@mwaxmonsky mwaxmonsky added the bug For issues describing bugs, or PRs fixing bugs label Mar 3, 2025
@gold2718
Copy link
Collaborator

gold2718 commented Mar 3, 2025

I would suggest that this is not a bug at all. The purpose of the host metadata table type is to define the interface that the CCPP host cap presents. If you look at the generated test_host_ccpp_cap.F90 file, you will see that the dummy argument is called ccpp. Fortran does not care what the name of the actual argument is as long as the type matches.

Does this make sense or am I misunderstanding the problem?

@peverwhee
Copy link
Collaborator

Ah, I had never really looked into the specifics of how host metadata works as we do not currently use it in CAM-SIMA.

@gold2718 Would checking the local name woul? The idea of keeping it consistent with the other metadata parsers sounds ideal to me.

@dustinswales does anything on your end use host type metadata?

@gold2718
Copy link
Collaborator

gold2718 commented Mar 4, 2025

@peverwhee, How would this check work?

First, a bit more on why I think this is the proper behavior. Here is what metadata files do:

  • A type = scheme metadata file describes the interface of a scheme
  • A type = ddt metadata file describes the type and (optionally) names of component data of a DDT
  • A type = module metadata file describes variables in a host-model module preamble
  • A type = host metadata file describes the interface of the generated CCPP host model cap.

I feel these are consistent, the only difference is that the last type is generated, not 'hand written'.

The type = host metadata file does not describe the parts of the host model that call the host model cap. To force the host model to use the same variable names as the host model cap, we would first have to require a CCPP metadata comment field (like the two-line comments before a scheme routine or a DDT definition). This comment field would have to be in front of every use of the host model cap by the host model (e.g., _init, _run, _final, etc). How would we document which files to check and how many checks there are?

I feel like I'm missing something here but I still do not see anything that should or can be fixed.

@dustinswales
Copy link
Collaborator

Fortran does not care what the name of the actual argument is as long as the type matches.

^I second this.

@peverwhee In the CCPP SCM we are using the host header (e.g. scm_type_defs.F90), although we are violating what @gold2718 mentioned above for the module header. Thanks for the explanation on what distinguishes the different metadata files. I think I know what needs to happen now on my end to jive with this.

In the UFS physics we have schemes that call type-bound procedures defined in host DDTs, which are used to reset all of data stored within a type. For example, GFS_suite_interstitial_phys_reset.F90 zeros out all of these fields. This is not so great for many reasons and I'm strongly leaning towards removing the call to the internal procedure and just zeroing out the fields individually. This will remove the ugly scheme-to-host dependency we have in the UFS based physics and allow us the use the host and module headers properly.
@grantfirl Any objections to me making this cleanup in the UFS physics? @climbfuji Would NRL be okay with this change?

@gold2718
Copy link
Collaborator

gold2718 commented Mar 4, 2025

In the UFS physics we have schemes that call type-bound procedures defined in host DDT

As a side note, I've long wanted to figure out how to add metadata and capgen implementation for type-bound procedures.

Another side note is that I would also love to see more a more portable version of the GFS physics suite(s).

@peverwhee
Copy link
Collaborator

@gold2718 I don't think you're missing anything. I'm just discovering how little I know about host metadata. Thanks for the explanation. I think my original concern about checking is moot now.

And thanks @dustinswales seeing your use case has made things a little clearer. I'm still mildly confused about why the host type is necessary, especially for more "portable" code bases. Since you only ever need to pass in the variables used by the schemes, it seems odd to have static arguments.

@climbfuji
Copy link
Collaborator

Fortran does not care what the name of the actual argument is as long as the type matches.

^I second this.

@peverwhee In the CCPP SCM we are using the host header (e.g. scm_type_defs.F90), although we are violating what @gold2718 mentioned above for the module header. Thanks for the explanation on what distinguishes the different metadata files. I think I know what needs to happen now on my end to jive with this.

In the UFS physics we have schemes that call type-bound procedures defined in host DDTs, which are used to reset all of data stored within a type. For example, GFS_suite_interstitial_phys_reset.F90 zeros out all of these fields. This is not so great for many reasons and I'm strongly leaning towards removing the call to the internal procedure and just zeroing out the fields individually. This will remove the ugly scheme-to-host dependency we have in the UFS based physics and allow us the use the host and module headers properly. @grantfirl Any objections to me making this cleanup in the UFS physics? @climbfuji Would NRL be okay with this change?

Not a problem at all for NRL, thanks for asking

@gold2718
Copy link
Collaborator

gold2718 commented Mar 4, 2025

I'm still mildly confused about why the host type is necessary, especially for more "portable" code bases.

@peverwhee, the idea is that it gives the host model the ability to choose its interface to <hostname>_ccpp_physics_initialize, <hostname>_ccpp_physics_run, etc. For instance, say a host wanted to have horizontal_loop_begin and horizontal_loop_end be local variables to enable passing different 'chunks' of data to different threads. By adding those two variables to the host metadata table, those variables would be in the call list to <hostname>_ccpp_physics_run and can thus be thread safe. Anyway, this is the sort of use case I was pondering when I came up with the host metadata table idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug For issues describing bugs, or PRs fixing bugs
Projects
None yet
Development

No branches or pull requests

5 participants