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

Feature #2283 time looping consolidation #2311

Merged
merged 56 commits into from
Aug 30, 2023

Conversation

georgemccabe
Copy link
Collaborator

@georgemccabe georgemccabe commented Aug 16, 2023

These changes were made to make it easier to make the changes required for #2283. Without these changes, code changes are needed in many places to satisfy #2283.

Change Summary

  • Changed all wrappers to use time looping from RuntimeFreq so logic is not duplicated in each wrapper (remove run_at_time and run_all_times functions from individual wrappers)
  • Refactored ti_calculate from time_util to properly handle wildcard characters set from RuntimeFreq wrapper
  • Refactored MTD wrapper to properly gather files for all lead times for each init/valid runtime
  • For RUNTIME_FREQ=RUN_ONCE, if [INIT/VALID]_BEG == [INIT/VALID]_END, set init/valid to that time instead of wildcard (*)
  • Fixed bugs to clear out command line arguments properly before building commands (run once did not clear between CUSTOM_LOOP_LIST runs)

NOTE: Additional work could be done to make the input file logic consistent.
MTD wrapper provides a good start that can be enhanced to work for other wrappers,
including support for optional inputs, processing all fields (vars) in a single run, etc.
SeriesAnalysis, GridDiag, GridDiag, and UserScript wrappers could be modified to
not call subset_input_files function and instead gather only files needed for each run.

Pull Request Testing

  • Describe testing already performed for these changes:
  • Ensured all automated tests pass (with minimal diffs)
  • Added unit tests for time_util.ti_calculate
  • Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions:
  • Review Contributor's Guide changes, then review code changes

  • Ensure only failures in automated tests are due to differences in file names that are reasonable

  • Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? [Yes]

  • Do these changes include sufficient testing updates? [Yes]

  • Will this PR result in changes to the test suite? [Yes]

A few intermediate file names will change. develop-ref will need to be updated after this PR is merged.

  • Please complete this pull request review by 8/18/2023.

Pull Request Checklist

See the METplus Workflow for details.

  • Add any new Python packages to the METplus Components Python Requirements table.
  • Review the source issue metadata (required labels, projects, and milestone).
  • Complete the PR definition above.
  • Ensure the PR title matches the feature or bugfix branch name.
  • Define the PR metadata, as permissions allow.
    Select: Reviewer(s)
    Select: Organization level software support Project or Repository level development cycle Project
    Select: Milestone as the version that will include these changes
  • After submitting the PR, select the ⚙️ icon in the Development section of the right hand sidebar. Search for the issue that this PR will close and select it, if it is not already selected.
  • After the PR is approved, merge your changes. If permissions do not allow this, request that the reviewer do the merge.
  • Close the linked issue and delete your feature or bugfix branch from GitHub.

…d cluttering log output, make wrapper a LoopTimes wrapper to remove run_at_time function and reduce redundancy, log if example file path is found to make wrapper more useful
…edundant run_at_time functions in each wrapper, ci-run-all-diff
…TD) and ReformatGridded (PCPCombine, RegridDataPlane) wrappers to be LoopTimes wrappers so they use the same run_at_time method
…remove command check that is done inside build function
…, TCStat, and TCRMW to be RuntimeFreq wrappers -- testing if any issues arise because there may be an assumption that init==valid for these tools, but RuntimeFreq RUN_ONCE_PER_INIT_OR_VALID uses wildcard for forecast lead and does not compute the opposite of init/valid, ci-run-all-diff
…time_once to compute missing time values, ci-run-all-diff
…ess, update unit tests to match changes to calls to wrapper and naming of file list files
…ecks for lead not wildcard (relativedelta comparison operator to string '*' always reports False)
Copy link
Collaborator

@JohnHalleyGotway JohnHalleyGotway left a comment

Choose a reason for hiding this comment

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

I approve of these changes.

I see differences flagged in 2 of the tests in this GHA run.

  1. In marine_and_cryosphere, the file list is renamed from
    user_script_files_input0_init_ALL_valid_ALL_lead_ALL_truth.txt to user_script_files_input0_init_ALL_valid_20211028000000_lead_ALL_output.txt
    but the contents are the same.
  • For medium_range, the file 201908/alq2019083000.dorian_truth.tcst has moved up one directory toalq2019083000.dorian_output.tcst but the contents are the same.
  • All other GHA tests passed.

I see 60 modified files. Some comments:

  • In docs/Contributors_Guide/basic_components.rst would be cool to add a class hierarchy diagram to illustrate but its definitely not required.
  • I read through the updates to the documentation for clarity and typos, and it looks great.
  • I browsed the other code modifications and note the removal of run_at_time functions from many of the wrappers, which is great to streamline. And in particular a large overhaul to the MTD wrapper.

I can't really speak to the accuracy of the code changes but they do look consistent with the changes you described when we met the other day. There certainly are a lot of changes here and I'm leaning heavily on the automated GHA tests to validate that they are working as expected.

Copy link
Collaborator

@JohnHalleyGotway JohnHalleyGotway left a comment

Choose a reason for hiding this comment

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

I approve of these changes.

@georgemccabe georgemccabe merged commit 27b3b6d into develop Aug 30, 2023
@georgemccabe georgemccabe deleted the feature_2283_time_looping branch August 30, 2023 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: 🏁 Done
Development

Successfully merging this pull request may close these issues.

Enhancement: Improve time formatting logic to include certain times and use day of week to subset
2 participants