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

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

Merged
merged 1 commit into from
Aug 15, 2024

Conversation

mkavulich
Copy link
Collaborator

Because of the change in directory structure for CCPP physics (ufs-community/ccpp-physics#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

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.

def review_pr581(approved=False):
    if not approved:
        approved = review_pr581(approved=not approved)

    return approved

Copy link
Collaborator

@dustinswales dustinswales left a comment

Choose a reason for hiding this comment

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

@mkavulich This change looks good to me.
Should this PR be targeting the main branch?

@peverwhee
Copy link
Collaborator

@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
Copy link
Collaborator Author

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.

@mkavulich mkavulich merged commit af90b94 into NCAR:develop Aug 15, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants