-
Notifications
You must be signed in to change notification settings - Fork 204
Refactor Tar Archiving Logic to Python-Based Implementation #4336
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
base: develop
Are you sure you want to change the base?
Refactor Tar Archiving Logic to Python-Based Implementation #4336
Conversation
…do-NOAA/global-workflow into enabler/archive_tar
…do-NOAA/global-workflow into enabler/archive_tar
be6a91b to
c4139ac
Compare
aerorahul
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.
looks good. Just a couple of questions.
|
|
||
| # Archive timing booleans - increments (group a) | ||
| # Logic: (current_cycle - SDATE).days % ARCH_WARMICFREQ == 0 AND is_gdas AND ARCH_CYC == cycle_HH | ||
| if sdate: |
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.
When is this condition False? config_dict always contains SDATE, no? It is in the config.base
|
|
||
| # Archive timing booleans - ICs (group b) | ||
| # Logic: (ics_offset_cycle - SDATE).days % ARCH_WARMICFREQ == 0 AND is_gdas AND (ARCH_CYC - assim_freq) % 24 == cycle_HH | ||
| if sdate: |
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.
same here
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.
@aerorahul That's correct. sdate if statement is not needed here.
DavidHuber-NOAA
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.
Looks great! I just have a few minor suggestions/questions.
| {% for iaufhr in IAUFHRS %} | ||
| {% for itile in range(6) %} | ||
| - "{{ COMIN_ATMOS_ANALYSIS_MEM }}/{{ head }}recentered_jedi_increment.atm.i{{ '%03d' % iaufhr }}.tile{{ itile+1 }}.nc" |
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 it possible to format IAUFHRS in the Python code so you don't have to format iaufhr here? I'm not sure if that will have repercussions elsewhere...
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.
Done.
| # Archive timing and control | ||
| 'ARCH_CYC', 'ARCH_WARMICFREQ', 'ARCH_FCSTICFREQ', | ||
| # Other | ||
| 'NET', |
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.
NET is a basic configuration variable. It's the name of the system (GFS, GEFS, GCAFS, or SFS).
| # Basic configuration | ||
| 'ATARDIR', 'current_cycle', 'RUN', 'PDY', 'PSLOT', | ||
| # Archive control | ||
| 'DO_ARCHCOM', 'ARCHCOM_TO', 'ROTDIR', 'PARMgfs', 'ARCDIR', 'SDATE', 'MODE', |
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.
MODE, PARMgfs, ROTDIR, and SDATE are also basic configuration.
f1b4466
|
@aerorahul @DavidHuber-NOAA Addressed all the comments. Running all the tests on |
TravisElless-NOAA
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.
My individual testing shows things should be working correctly here.
054ce01
Description
This PR refactors the tar archiving (arch_tar) workflow to move all logic, cycle variable handling, and COM path resolution from YAML/Jinja templates into Python. YAML configuration is now limited to describing what files are archived, while Python scripts are responsible for how variables are generated and resolved.
This change aligns tar archiving with the local archiving (arch_vrfy) approach and establishes a unified, Python-driven archive framework.
Resolves [Enabler] Review the archiving process and determine best paths forward #3656
Refs Archive Enhancement — Migrate Template Variables from Jinja to Python #3910
Type of change
Change characteristics
How has this been tested?
Checklist