-
Notifications
You must be signed in to change notification settings - Fork 204
Replace Links with cpfs/cpreq in Workflow Scripts
#4392
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?
Replace Links with cpfs/cpreq in Workflow Scripts
#4392
Conversation
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 good. There's just one member loop that should stay as links and I spotted a COMIN that should be a COMOUT.
dev/scripts/exgdas_enkf_post.sh
Outdated
| ${NLN} "${COMIN_ATMOS_HISTORY}/${PREFIX}sfc.f${fhrchar}.nc" "sfcf${fhrchar}_${memchar}" | ||
| ${NLN} "${COMIN_ATMOS_HISTORY}/${PREFIX}atm.f${fhrchar}.nc" "atmf${fhrchar}_${memchar}" | ||
| cpreq "${COMIN_ATMOS_HISTORY}/${PREFIX}sfc.f${fhrchar}.nc" "sfcf${fhrchar}_${memchar}" | ||
| cpreq "${COMIN_ATMOS_HISTORY}/${PREFIX}atm.f${fhrchar}.nc" "atmf${fhrchar}_${memchar}" |
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.
Since these are member forecasts, they can stay as links for the post job.
| for cycle in $(seq -f "%02g" -s ' ' 0 "${INTERVAL_GFS}" "${cyc}"); do | ||
| YMD=${PDY} HH=${cycle} GRID="1p00" declare_from_tmpl gempak_dir:COM_ATMOS_GEMPAK_TMPL | ||
| for file_in in "${gempak_dir}/gfs_1p00_${PDY}${cycle}f"*; do | ||
| file_out="${COMIN}/$(basename "${file_in}")" |
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.
This should be updated to COMOUT.
gempak/ush/gdas_meta_loop.sh
Outdated
| if [[ ! -L "${COMIN}" ]]; then | ||
| ${NLN} "${COMIN_ATMOS_GEMPAK_1p00}" "${COMIN}" | ||
| fi | ||
| cpreq -R "${COMIN_ATMOS_GEMPAK_1p00}" "${COMIN}" |
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.
Alright, sorry for the whiplash. This will take some more thought. Please revert these cpreq calls to NLN and add a TODO. I will create a follow-up issue.
gempak/ush/gdas_ecmwf_meta_ver.sh
Outdated
| if [[ ! -L ${COMIN} ]]; then | ||
| ${NLN} "${COMIN_ATMOS_GEMPAK_1p00}" "${COMIN}" | ||
| fi | ||
| cpreq -R "${COMIN_ATMOS_GEMPAK_1p00}" "${COMIN}" |
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.
I don't think this is the intended use of cpreq with -R to copy a directory.
A required directory may exist, but may not contain all the required files. cpreq operates on a file and if it does not exist or is corrupted, reports an error.
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 I'm not sure if this is the intended purpose, but the cpreq script does take $* as the argument to cp (cpreq#L3):
cp $*Thus, the -R does get passed as an argument to cp and if anything fails in the copy, it will call err_exit. That said, the standards only have one example with a single file operation:
cpreq $COMIN/inputfile inputfile # copy essential input files into working directoryAll of that said, I think these linking/copying operations need to be reworked anyway and I'll open a follow-up issue for that.
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.
Oh, I misread your comment. You're correct about the potentially-missing files. Still, I'll open another issue.
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.
Yes. cpreq is essentially this: cp $*; if [[ $? -ne 0 ]]; then echo "we have a problem"; exit 1; fi
I think that's quite silly. Instead, it should work on file-by-file basis, because a directory may exist with none of the required files and yet cpreq will succeed.
Instead, we should propose to NCO that cpreq should be more thorough.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
| if [[ ! -L ${COMIN} ]]; then | ||
| ${NLN} "${COMIN_ATMOS_GEMPAK_1p00}" "${COMIN}" | ||
| fi | ||
| cpreq -R "${COMIN_ATMOS_GEMPAK_1p00}" "${COMIN}" |
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.
This should be reverted to linking.
Description
Some jobs in the current global-workflow create links from their working directories to files in the COM directory, allowing output to be written or accessed directly in COM.
Use
cpreqwhen files are copied from COM directory.Use
cpfswhen files are copied to COM directoryRefs [NCO Bug] Replace links with copy/move/rsync in workflow scripts #712
Type of change
Change characteristics
How has this been tested?
Checklist