-
Notifications
You must be signed in to change notification settings - Fork 65
DRAFT: Skip variable parse checks during fortran parsing #673
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
Conversation
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.
Is there a test that demonstrates (reproduces) the problem this is supposed to solve?
| # checks are already done during metadata parsing | ||
| super().__init__(prop_dict, source, run_env, context=context, | ||
| clone_source=clone_source) | ||
| clone_source=clone_source, skip_checks=True) |
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 feels like the wrong place to add this check as the problem (which I do not really follow without a regression test to read) is not within this type but its use in parse_fortran.py.
I suggest adding skip_checks as an input to the __init__ method for this class and calling it from parse_fortran.py
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've added some new test files to the ddthost test that will cause the test to fail without the changes in this PR. Hope that helps - and thanks for the suggestion to add a test.
I'm not sure I'm following your suggestion here. The checks are done in super().__init__ so I don't know how to avoid passing the skip_checks into there. I figured since the FortranVar class is only used during fortran parsing, it would be unnecessary to add the flag to the FortranVar.__init__. But if I'm missing something please let me know!
|
@gold2718 - good call. I'll add a wrapped DDT to the DDT test. |
|
@peverwhee develop is no longer frozen, time to update this PR? |
|
@DomHeinzeller thanks Dom! I'm waiting on @gold2718 to perhaps come up with a better approach to this problem, so I've switched it to a draft PR |
|
closing - new approach here: #684 |
Summary
Since the variables are checked during the metadata parsing stage and the metadata is validated against the Fortran, there's no need to also check the variables during the Fortran parsing stage.
Context
For some external packages, we need to wrap DDTs to provide metadata, which means the underlying DDT is unknown to the framework and that should be OK (the metadata is only provided for the top-level DDT). Because the Fortran parsing parses all variables even if there's no metadata provided, these wrapped DDTs result in "unknown DDT" errors.
Example from RRTMGP:
Corresponding metadata:
These changes add a flag to the Var class
__init__to conditionally not perform the parse checks on the parsed variables (this flag is then triggered for Fortran-parsed variables). As a result, there are no errors about "unknown DDT ty_fluxes_broadband" in the above example.User interface changes?: No