NOAA-EMC / global-workflow

Global Superstructure/Workflow supporting the Global Forecast System (GFS)
https://global-workflow.readthedocs.io/en/latest
GNU Lesser General Public License v3.0
72 stars 163 forks source link

Clean out non-gfs top level variables #2332

Closed KateFriedman-NOAA closed 5 months ago

KateFriedman-NOAA commented 5 months ago

What new functionality do you need?

Removal of duplicate variables that define the top level folder locations (e.g. PARMwave is the same as PARMgfs).

This is related to cleanup that happened in issue #2184 for the FIX* variables.

Remove the non-gfs versions of PARM*, USH*, UTIL*, EXEC*, SCR*. Also remove *gfs variables outside of configs including HOME* variables and EXPDIRs that aren't needed.

What are the requirements for the new functionality?

Functionality unimpacted.

Acceptance Criteria

The only top level path variables are now *gfs versions.

Suggest a solution (optional)

No response

KateFriedman-NOAA commented 5 months ago

@CoryMartin-NOAA @aerorahul While looking for PARM* variables to remove and replace with their ${PARMgfs}/SOMETHING alternative I see the IODAPARM parm file variable. Here are the two instances of it that I see in global-workflow:

-bash-4.2$ grep IODAPARM * -r                                                                                                            
jobs/JGLOBAL_ATM_PREP_IODA_OBS:${EXSCRIPT} "${PDY}${cyc}" "${RUN}" "${DMPDIR}" "${IODAPARM}" "${COM_OBS}/"
parm/config/gfs/config.prepatmiodaobs:export IODAPARM="${HOMEgfs}/sorc/gdas.cd/parm/ioda/bufr2ioda"

While IODAPARM isn't a directory variable, it's a parm file being sourced from under the /sorc folder instead of via symlinks/copies within the top-level /parm folder. The ../sorc/gdas.cd/parm folder isn't currently included in the link step so it doesn't yet reside under the top level /parm folder after clone setup. It doesn't have to happen with this issue but perhaps the ../gdas.cd/parm/* folders should be added to the link step and references like IODAPARM updated? Asking now so this doesn't get forgotten until closer to ops handoff. Thoughts?

KateFriedman-NOAA commented 5 months ago

@JessicaMeixner-NOAA While working on this issue I noticed the wave_sys_var variable:

-bash-4.2$ grep wave_sys_ver * -r
parm/config/gefs/config.wave:export wave_sys_ver=v1.0.0
parm/config/gfs/config.wave:export wave_sys_ver=v1.0.0
ush/wave_grid_moddef.sh:  if [ -z "$grdID" ] || [ -z "$EXECwave" ] || [ -z "$wave_sys_ver" ]

The commit history shows it came with the creation of the config.wave back when Henrique and I committed changes to add waves 5yrs ago. I don't recall the history of the variable but it doesn't appear needed now. Do you know about the variable, if we still need it, or if we can remove it? Thanks!

JessicaMeixner-NOAA commented 5 months ago

I don't have any additional history. I agree with your assessment that it is not needed now and can be removed. Let me know if you have a PR you'll be adding that to or I can remove this in my open PR updating the model assuming testing hasn't started.

KateFriedman-NOAA commented 5 months ago

@JessicaMeixner-NOAA Great! If you can include it in your current open PR that works for me! Let me know if you aren't able to and I'll include it in the PR for this issue. Thanks!

JessicaMeixner-NOAA commented 5 months ago

@KateFriedman-NOAA - done - added to https://github.com/NOAA-EMC/global-workflow/pull/2338

KateFriedman-NOAA commented 5 months ago

Perfect, thanks @JessicaMeixner-NOAA !

RussTreadon-NOAA commented 5 months ago

@KateFriedman-NOAA , thank you for bringing IODAPARM to our attention. I made your suggested change in a working copy of global-workflow on Hera. The following changes were made:

  1. replace IODAPARM with PARMioda in jobs/JGLOBAL_ATM_PREP_IODA_OBS

    @@ -6,7 +6,7 @@ source "${HOMEgfs}/ush/jjob_header.sh" -e "prepatmiodaobs" -c "base prepatmiodao
    ##############################################
    # Set variables used in the script
    ##############################################
    -
    +PARMioda=${PARMioda:-${HOMEgfs}/parm/gdas/bufr2ioda}
    
    ##############################################
    # Begin JOB SPECIFIC work
    @@ -18,7 +18,7 @@ YMD=${PDY} HH=${cyc} generate_com -rx COM_OBS
    ###############################################################
    # Run relevant script
    EXSCRIPT=${BUFR2IODASH:-${HOMEgfs}/ush/run_bufr2ioda.py}
    -${EXSCRIPT} "${PDY}${cyc}" "${RUN}" "${DMPDIR}" "${IODAPARM}" "${COM_OBS}/"
    +${EXSCRIPT} "${PDY}${cyc}" "${RUN}" "${DMPDIR}" "${PARMioda}" "${COM_OBS}/"
    status=$?
  2. add a GDASApp parm section to sorc/link_workflow.sh

    
    @@ -199,6 +199,14 @@ if [[ -d "${HOMEgfs}/sorc/gdas.cd" ]]; then
    done
    fi

+#------------------------------ +#--add GDASApp parm directory +#------------------------------ +if [[ -d "${HOMEgfs}/sorc/gdas.cd" ]]; then

  1. Remove IODAPARM from parm/config/gfs/config.prepatmiodaobs. Also removed BUFR2IODASH from the same config file since jobs/JGLOBAL_ATM_PREP_IODA_OBS defaults to the correct path.
    
    @@ -8,7 +8,4 @@ echo "BEGIN: config.prepatmiodaobs"
    # Get task specific resources
    . "${EXPDIR}/config.resources" prepatmiodaobs

-export BUFR2IODASH="${HOMEgfs}/ush/run_bufr2ioda.py" -export IODAPARM="${HOMEgfs}/sorc/gdas.cd/parm/ioda/bufr2ioda"

echo "END: config.prepatmiodaobs"



Job `gdasprepatmiodaobs` was run with the above changes.  The job successfully ran to completion.
KateFriedman-NOAA commented 5 months ago

@RussTreadon-NOAA Thanks for looking into this and providing tested changes to address it.

Changes 2 and 3 I can fold into my fork branch for this issue. I'll do that.

Change 1 goes in the opposite direction that this issue is going = remove non-gfs PARM* variables (e.g. remove PARMioda). For your suggested change 1 I would counter with replacing your suggested PARMioda with ${PARMgfs}/gdas/bufr2ioda and delete the line that sets PARMioda. Does that work for this job?

RussTreadon-NOAA commented 5 months ago

@KateFriedman-NOAA , do you mean

@@ -18,7 +18,7 @@ YMD=${PDY} HH=${cyc} generate_com -rx COM_OBS
 ###############################################################
 # Run relevant script
 EXSCRIPT=${BUFR2IODASH:-${HOMEgfs}/ush/run_bufr2ioda.py}
-${EXSCRIPT} "${PDY}${cyc}" "${RUN}" "${DMPDIR}" "${IODAPARM}" "${COM_OBS}/"
+${EXSCRIPT} "${PDY}${cyc}" "${RUN}" "${DMPDIR}" "${PARMgfs}/gdas/bufr2ioda" "${COM_OBS}/"
 status=$?
 [[ ${status} -ne 0 ]] && (echo "FATAL ERROR: Error executing ${EXSCRIPT}, ABORT!"; exit "${status}")

in jobs/JGLOBAL_ATM_PREP_IODA_OBS?

I made this change and gdasprepatmiodaobs successfully ran to completion.

KateFriedman-NOAA commented 5 months ago

I made this change and gdasprepatmiodaobs successfully ran to completion.

@RussTreadon-NOAA Yep that's what I meant. Awesome, appreciate you testing that and confirming it works. I will fold that change into my branch as well. Thanks for your feedback and help!

RussTreadon-NOAA commented 5 months ago

I made this change and gdasprepatmiodaobs successfully ran to completion.

@RussTreadon-NOAA Yep that's what I meant. Awesome, appreciate you testing that and confirming it works. I will fold that change into my branch as well. Thanks for your feedback and help!

FYI, my working copy is on Hera. Look in /scratch1/NCEPDEV/da/Russ.Treadon/git/global-workflow/work

        modified:   jobs/JGLOBAL_ATM_PREP_IODA_OBS
        modified:   parm/config/gfs/config.prepatmiodaobs
        modified:   sorc/gdas.cd (modified content, untracked content)
        modified:   sorc/link_workflow.sh
KateFriedman-NOAA commented 5 months ago

FYI, my working copy is on Hera. Look in /scratch1/NCEPDEV/da/Russ.Treadon/git/global-workflow/work

Thanks @RussTreadon-NOAA ! I have committed the discussed changes to my fork branch @ 798bb3c.

RussTreadon-NOAA commented 5 months ago

Good catch on adding parm/gdas/bufr2ioda to .gitignore. In addition to removing parm/gldas from.gitingore, shall we also remove fix/gldas?

KateFriedman-NOAA commented 5 months ago

@EdwardSafford-NOAA I am replacing the various non-gfs USH* variables (e.g. USHradmon) with their USHgfs alternatives. I think the USHradmon in exgdas_atmos_verfrad.sh:

   "${USHradmon}/clean_tankdir.sh" glb 60

...should be changed to USHgfs but I don't see the clean_tankdir.sh script anywhere nor do I see that block being involked. This block: https://github.com/NOAA-EMC/global-workflow/blob/develop/scripts/exgdas_atmos_verfrad.sh#L134:L141

-bash-4.2$ pwd
/scratch1/NCEPDEV/global/Kate.Friedman/git/feature-issue2332
-bash-4.2$ ll ush/clean_tankdir.sh                                                                                                                     
ls: cannot access ush/clean_tankdir.sh: No such file or directory
-bash-4.2$ ll sorc/gsi_monitor.fd/ush/clean_tankdir.sh
ls: cannot access sorc/gsi_monitor.fd/ush/clean_tankdir.sh: No such file or directory

Do you have a recommendation for replacing that USHradmon and/or if we still want this if-block? Thanks!

KateFriedman-NOAA commented 5 months ago

Good catch on adding parm/gdas/bufr2ioda to .gitignore. In addition to removing parm/gldas from.gitingore, shall we also remove fix/gldas?

Yes, good catch on fix/gldas. I've cleaned that out too @ 399fa34f. Thanks @RussTreadon-NOAA !

EdwardSafford-NOAA commented 5 months ago

@KateFriedman-NOAA that if block in exgdas_atmos_verfrad.sh can be removed. As you noted the script it calls is not included in workflow so it cannot be called as is. The original purpose was to keep the size of the extracted RadMon data to a reasonable size but when run within the global-workflow that issue is probably best managed at a workflow level.

KateFriedman-NOAA commented 5 months ago

@KateFriedman-NOAA that if block in exgdas_atmos_verfrad.sh can be removed. As you noted the script it calls is not included in workflow so it cannot be called as is. The original purpose was to keep the size of the extracted RadMon data to a reasonable size but when run within the global-workflow that issue is probably best managed at a workflow level.

@EdwardSafford-NOAA Gotcha, I'll remove that if-block, thanks for the feedback and info!

So with the updates we made to GSI monitor running within global-workflow late last year via PR #1983, the monitor outputs land in com/ROTDIR instead of an online archive, are archived to HPSS alongside the other outputs, and then are scrubbed from com/ROTDIR as the tests move forward. This is in line with what happens in ops now. Given that, I'm not seeing a need to retain any other mechanisms/scripts to clean out those outputs. So I think we're good to remove that if-block for good. But, let me know if we should ever revive copies of the monitor output in an online archive, in addition to what we're now doing. Thanks!

KateFriedman-NOAA commented 5 months ago

@EdwardSafford-NOAA I have now removed the if-block in question @ fd02bef9. Thanks again!