search

NCAR / ccpp-framework

Common Community Physics Package (CCPP)
http://www.dtcenter.org/community-code/common-community-physics-package-ccpp/
Other
26 stars 64 forks source link

Fix for ccpp_track_variables.py: Need "recursive=True" for glob on .meta files #581

Closed mkavulich closed 3 weeks ago

mkavulich commented 1 month ago

Because of the change in directory structure for CCPP physics (https://github.com/ufs-community/ccpp-physics/pull/99), there are now .meta files at different levels in the directory tree. The ccpp_track_variables.py script needs the location of these .meta files as an input argument to the script, but the call to glob.glob in the script does not use the recursive=True argument, so even if the user passes in the argument -m './physics/physics/**/' (which should include all subdirectories), the call to glob.glob only searches one level. Our simple test case only has .meta files at a single directory level, so we never caught this issue.

Simply adding the recursive=True argument fixes this issue.

User interface changes?: No

Fixes: #580 ccpp_track_variables.py is broken since CCPP physics directory structure was reorganized

Testing: test removed: none unit tests: Passing system tests: none manual testing: See Issue #580

peverwhee commented 1 month ago

@dustinswales @mkavulich I vote for targeting develop as is, but, if this needs to get into main soon, we should make this the "event" that triggers a PR from develop to main

mkavulich commented 3 weeks ago

I'm a bit conflicted on the merge destination here: While I would love to merge directly to main here to skip the requirements of needing to run all tests for all dycores -- and in this case it should not affect anything since it only changes a stand-alone script and not anything else in the framework -- I think it would set a bad precedent to change the repo's workflow for this PR.

My preferred solution is that SCM can point directly to this hash on the develop branch once it is merged; ideally host model releases should point to the main branch, but since main is so young I think it would be fine in this case. @grantfirl @dustinswales I'm curious if you have different thoughts.