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

Restore capability to export metadata sections (previously headers) to html #445

Merged
merged 2 commits into from
Apr 12, 2022

Conversation

climbfuji
Copy link
Collaborator

Restore capability to export metadata sections (previously headers) to html.

The capability to write metadata sections to html was removed with the refactoring of the feature/capgen branch, subsequently slipped the reviews and got merged into main. As a result, the metadata2html.py script, required to generate scientific documenation (especially at times of releases!) doesn't work anymore. This PR restores the capability by putting the original code blocks back into the metadata sections (previously these were called headers).

I ran this in the two supported ways within the UFS. Once merged, the code will work like this. Maybe there are some slight path changes compared to what is in the CCPP tech doc - @mkavulich FYI.

Batch mode using ccpp_prebuild_config.py:

git clone -b develop --recursive https://github.com/ufs-community/ufs-weather-model
cd ufs-weather-model/FV3/ccpp
PYTHONPATH="$PWD/framework/scripts:$PWD/framework/scripts/parse_tools:$PYTHONPATH"  \
    ./framework/scripts/metadata2html.py -c config/ccpp_prebuild_config.py

Single-file mode:

mkdir test
PYTHONPATH="$PWD/framework/scripts:$PWD/framework/scripts/parse_tools:$PYTHONPATH" ./framework/scripts/metadata2html.py -m physics/physics/mp_thompson.meta -o test

User interface changes?: Yes (I assume some of the paths have changed compared to the documentation)

Closes #444

Testing: see above. The code changes will also be tested within the ufs-weather-model regression testing environment. I recommend that we pull this PR into #443, since #443 and associated PRs do not change the baselines. Note that #444 is required for CCPP release v6.

Other testing:

  • for ccpp_prebuild.py:
# relative to above directory
cd framework/tests
PYTHONPATH="$PWD/../scripts:$PWD/../scripts/parse_tools:$PYTHONPATH" python3 test_metadata_parser.py
PYTHONPATH="$PWD/../scripts:$PWD/../scripts/parse_tools:$PYTHONPATH" python3 test_mkstatic.py 
  • for capgen.py:
# relative to previous `tests` directory
cd ../test
./run_tests.sh
# All tests PASSed!
./run_doctests.sh
# No output, which means no error - all tests PASSed

test removed: none
unit tests: none
system tests: none
manual testing: see above

Copy link
Collaborator

@gold2718 gold2718 left a comment

Choose a reason for hiding this comment

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

A couple of suggestions but basically okay.

@@ -1266,6 +1279,36 @@ def is_scalar_reference(test_val):
"""Return True iff <test_val> refers to a Fortran scalar."""
return check_fortran_ref(test_val, None, False) is not None

def to_html(self, outdir, props):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would move this up to where the other active routines are, say right after write_to_file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

# Write table header
header = "<tr>"
for prop in props:
header += "<th>{}</th>".format(prop)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have been converting to f strings, this seems like a good place.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have adopted f strings in this PR, thanks for the suggestion.

Copy link
Collaborator

@mzhangw mzhangw left a comment

Choose a reason for hiding this comment

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

Tested. It works well.

@climbfuji
Copy link
Collaborator Author

@mkavulich @grantfirl I addressed @gold2718's review comments, now it is your turn ...

Copy link
Collaborator

@grantfirl grantfirl left a comment

Choose a reason for hiding this comment

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

Looks OK to me. I did not test myself, but others have reported success, so approved.

@ligiabernardet ligiabernardet added the CCPP v6 Needed for CCPP v6 public release label Apr 7, 2022
@grantfirl
Copy link
Collaborator

@climbfuji Could we combine this with #443 for the purposes of merging with UFS?

@climbfuji
Copy link
Collaborator Author

Yes, I was planning to combine #443 #445 and #447 (if approved by then).

climbfuji added a commit that referenced this pull request Apr 12, 2022
Remove legacy src/ccpp_api.F90 - contains #445 (fix metadata2html.py) and #447 (remove NEMSfv3gfs test scripts)
@climbfuji climbfuji merged commit 4393986 into NCAR:main Apr 12, 2022
@climbfuji climbfuji deleted the bugfix/restore_metadata_header_to_html branch June 27, 2022 02:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CCPP v6 Needed for CCPP v6 public release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error in scripts/metadata2html.py
5 participants