[develop] bugfix: set PDYext and cycext in get_extrn_ics/lbcs for time-offset

[chan-hoo]

DESCRIPTION OF CHANGES:

The PDY in the paths for the real-time external data in ush/machine/wcoss2.yaml' is replaced with yyyymmdd` to fix the time-offset issue on WCOSS2.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

TESTS CONDUCTED:

• WE2E tests:
  grid_RRFS_CONUS_25km_ics_FV3GFS_lbcs_FV3GFS_suite_GFS_2017_gfdlmp_regional
  grid_RRFS_CONUS_25km_ics_FV3GFS_lbcs_RAP_suite_HRRR
  grid_RRFS_CONUScompact_25km_ics_HRRR_lbcs_HRRR_suite_RRFS_v1beta
  nco_grid_RRFS_CONUS_25km_ics_FV3GFS_lbcs_FV3GFS_timeoffset_suite_GFS_v16

• hera.intel

• orion.intel

• cheyenne.intel

• cheyenne.gnu

• gaea.intel

• jet.intel

• wcoss2.intel

• NOAA Cloud (indicate which platform)

• Jenkins

• fundamental test suite

• comprehensive tests (specify which if a subset was used)

ISSUE:

Fixes issue mentioned in #474

CHECKLIST

• My code follows the style guidelines in the Contributor's Guide
• I have performed a self-review of my own code using the Code Reviewer's Guide
• I have commented my code, particularly in hard-to-understand areas
• My changes need updates to the documentation. I have made corresponding changes to the documentation
• My changes do not require updates to the documentation (explain).
• My changes generate no new warnings
• New and existing tests pass with my changes
• Any dependent changes have been merged and published

[MatthewPyle-NOAA]

Shifting away from the production date variable (PDY) seems suboptimal to me as a fix. Could it be fixed with logic changes elsewhere?

[MichaelLueken]

@chan-hoo Is there a reason that you added envir: prod to the nco_grid_RRFS_CONUS_25km_ics_FV3GFS_lbcs_FV3GFS_timeoffset_suite_GFS_v16 configuration file? Is this something that should also be done to the rest of the nco configuration files, or just a fix for the noted configuration?

[danielabdi-noaa]

@MatthewPyle-NOAA I was about to mentione the same thing. It will only work in ex-scripts that define yyyymmdd so i say keep PDY. But we did ask @chan-hoo in his other PR to change $cyc to $hh in the past, so not his fault. Maybe use both $cyc and $PDY in the compath.py definiton but override the IC/LBC date values in the ex-scripts i.e. PDY=$yyyymmdd and cyc=$hh. This will not change the parent shell's $PDY and $cyc so should be fine elsewhere, but need to be careful in the same script from the point they are assigned. Lesser of two evils I suppose.

[chan-hoo]

@MatthewPyle-NOAA @danielabdi-noaa , I'll set them back to PDY and cyc. @MichaelLueken, I used it to reproduce the error message. It doesn't affect the we2e test at all. I'll remove this change too.

[MichaelLueken]

These changes look good to me!

[BenjaminBlake-NOAA]

@chan-hoo Thanks for making the changes, it looks good to me. I agree with @MatthewPyle-NOAA and @danielabdi-noaa about keeping $PDY and $cyc since we'll want to have those variables defined in operations.

[MichaelLueken]

@chan-hoo All of the Jenkins tests have completed running. There was a disconnect on Hera overnight, which is why the Jenkins test is reporting that it was aborted. Looking at the output in /scratch1/NCEPDEV/stmp2/role.epic/jenkins/workspace/fs-srweather-app_pipeline_PR-475/expt_dirs, all tests successfully completed.

@MatthewPyle-NOAA Before merging this work, I would like to make sure that you are also happy with the modifications that have been made. Please indicate whether you approve of this PR in a comment. Thank you very much!

[MatthewPyle-NOAA]

@MatthewPyle-NOAA Before merging this work, I would like to make sure that you are also happy with the modifications that have been made. Please indicate whether you approve of this PR in a comment. Thank you very much!

I'm good with it now that the PDY variable has been restored - no objections assuming all checks pass.

[christinaholtNOAA]

As we'd discussed in #457, I don't think it makes sense here to use PDY and cyc when talking about the external model. Those are set as global variables for the SRW (RRFS) cycle we're running. This is referring to the cycle of the external model.

It doesn't seem like an appropriate standard to set these variables, or reset them, in the scripts layer, and it doesn't seem like it should be done when they are meant to be constants associated with the host system.

@MatthewPyle-NOAA thoughts?

Edit: It seems like this is more akin to PDYm# in the EE2 standards doc. Since it's configurable how offset this is, it doesn't make sense to bake an offset into the name of the variable used to define how much offset, since it could be 0 and PDY is used, or any other number where it is PDYmX.

[mark-a-potts]

FWIW, I spent some time yesterday trying to get the wrapper scripts to run on Cheyenne and both PDY and cyc currently need to be set explicitly by the user before the run_get_*cs.sh scripts will work. If we can find a way to solve that problem at the same time, it would be great.

[MatthewPyle-NOAA]

As we'd discussed in #457, I don't think it makes sense here to use PDY and cyc when talking about the external model. Those are set as global variables for the SRW (RRFS) cycle we're running. This is referring to the cycle of the external model.

It doesn't seem like an appropriate standard to set these variables, or reset them, in the scripts layer, and it doesn't seem like it should be done when they are meant to be constants associated with the host system.

@MatthewPyle-NOAA thoughts?

Edit: It seems like this is more akin to PDYm# in the EE2 standards doc. Since it's configurable how offset this is, it doesn't make sense to bake an offset into the name of the variable used to define how much offset, since it could be 0 and PDY is used, or any other number where it is PDYmX.

This aspect will likely be handled somewhat differently in operations (there won't be get_extrn_* type tasks), so I'm not going to be religious about the use of PDY and cyc here. But another option would be to define PDYext and cycext kinds of variable to be used in specifying cycle of the external model data. Would remove ambiguity by not using the SRW cycle variables, but also would add some complexity. Just a thought...

[BenjaminBlake-NOAA]

I think @MatthewPyle-NOAA 's suggestion of PDYext and cycext makes sense. Though as he noted it would add some complexity by adding more variables that the user would need to define. @christinaholtNOAA what do you think? This could also solve @mark-a-potts 's issue where PDY (PDYext) and cyc (cycext) need to be set explicitly for the run_get* jobs to work on Cheyenne. I don't know if I'm a huge fan of the names PDYext and cycext, but something similar would work too.

[chan-hoo]

@mark-a-potts, I am sorry but I can't understand why PDY and cyc should be set explicitly on Cheyenne. PDY and cyc are set in the XML file. Does it mean Cheyenne does not use the XML file? Please explain a little bit more about this. @MatthewPyle-NOAA, @christinaholtNOAA, @BenjaminBlake-NOAA, the problem is that we should use the same variable names between the machine file and ex-script. How can we use either 'PDYext' or 'PDYm1' without changing FV3GFS: compath.py ${envir}/gfs/${gfs_ver}/gfs.${PDY}/${cyc}/atmos. Please correct me if I am wrong.

[BenjaminBlake-NOAA]

@chan-hoo In operations it looks like the compath.py definitions for pointing to external model data stop after the version info (e.g. ${gfs_ver}), and then the part of the path that needs $PDY is defined at the ex-script level. So I think modifying the compath.py definition to use PDYext would be ok since we are defining a path to external model data:

compath.py ${envir}/gfs/${gfs_ver}/gfs.${PDYext}/${cycext}/atmos

Or would doing something like this cause additional problems elsewhere that I'm not aware of?

[chan-hoo]

@BenjaminBlake-NOAA, now I get it. You mean that we can modify PDY and cyc to PDYext and cycext in the 'compath.py' line. Is this correct? Do you think it is better to introduce new variables PDYext and cycext rather than to use yyyymmdd and hh? yyyymmdd and hh are defined in all the ex-scripts. If so, we'll need to hear from @christinaholtNOAA because she is requesting change :)

[mark-a-potts]

@chan-hoo The issue I was referencing has to do with using the wrapper scripts to work manually through the steps of the workflow. The wrappers don't use any XML scripts, so a user must set the PDY and cyc environment variables by hand before running them. Although I tested on Cheyenne, it would be a problem anywhere for a user trying to use the wrapper scripts.

[danielabdi-noaa]

@mark-a-potts I totally agree and that is why I proposed the wrapper scripts should be removed entirely. Please take a look at the discussion in the issue here: #473 . There is probably nothing that @chan-hoo's PR or any other change on the XML side can do to solve the issue. I think if those kind of scripts (wrappers) are needed for development purposes, it is much better to just add the task entry in the rocoto xml file, and use rocotoboot to start the job, which will save you from worrying about dependencies until one is ready to include them.

[chan-hoo]

@mark-a-potts, I got it. Thank you for the clarification! I hope @danielabdi-noaa or @christinaholtNOAA will resolve this issue in another PR :)

[chan-hoo]

Oops. I was a little bit late. Thank you for the explanation, @danielabdi-noaa ! :)

[BenjaminBlake-NOAA]

@chan-hoo Yes that is what me (and @MatthewPyle-NOAA) are suggesting. But if there is a compelling argument/reason to use yyyymmdd and hh for these tasks then I don't think that would be a major concern.

[christinaholtNOAA]

My major concern was mis-using the NCO-required variables in ways that aren't quite aligned with the host system.

I personally think that YYYYMMDDHH naming is more descriptive as a variable in a template like this, but will defer to others on exactly which variable name it is. I just really don't think it should be PDY or cyc.

[MichaelLueken]

@BenjaminBlake-NOAA and @MatthewPyle-NOAA Before merging, I want to make sure that both of you are okay with the change from PDY and cyc to PDYext and cycext. Thanks!

[BenjaminBlake-NOAA]

@MichaelLueken Yes this change is fine with me, thanks!