-
Notifications
You must be signed in to change notification settings - Fork 114
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
[develop] Update weather model hash and remove "_vrfy" from bash commands #1074
[develop] Update weather model hash and remove "_vrfy" from bash commands #1074
Conversation
…and remove _vrfy from bash cp, mv, rm, ln, cd, and mkdir commands
…nd removed ush/bash_utils/filesys_cmds_vrfy.sh reference from ush/source_util_funcs.sh
…_files.sh and ush/job_preamble.sh
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!
@MichaelLueken, thanks for this change:
Approving. |
@MichaelLueken It would probably be a good idea to keep what these were meant to do, which is, in bash scripts, check whether the cp, mv, etc functions were successful. These functions originally returned an error message and stopped execution, so that every time one needs to use say cp, one doesn't have to put in a line like this:
or something longer, and instead use
I saw in issue #861 that:
Obviously, this wasn't the original intent or behavior of these functions, but this erroneous behavior was introduced somewhere along the line due to insufficient testing. So instead of just removing these, what should be done is restore the original functionality of detecting errors and exiting. |
@gsketefian, as far as I know (if my memory serves me correctly), the bash commands like |
@***@***.*** I think I put in the “_vrfy” commands a long time ago
because I wanted to have clearer and standardized error messages for these
commands. The fact that they don’t actually exit is a bug that was
introduced later, probably during the partial conversion to python. It
would be great to restore that functionality to get more comprehensive
error messages as before. You can still add the -p to “cp_vrfy”.
…On Friday, April 19, 2024, Chan-Hoo.Jeon-NOAA ***@***.***> wrote:
@gsketefian <https://github.com/gsketefian>, as far as I know (if my
memory serves me correctly), the bash commands like cp exit with an error
message when they fail (while the current _vrfy function does not exit;
this was a big issue in my previous runs). For delivery to NCO, the -p
flag should be added to the copy command like (cp -p) though. So, I think cp
file1 file2 is good enough.
—
Reply to this email directly, view it on GitHub
<#1074 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHM3ZYTNU3RHYBTMGJOCWHTY6FLA3AVCNFSM6AAAAABGPFNOFSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANRXGA2DEOJQGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
Gerard Ketefian
Research Scientist
NOAA/OAR/ESRL/GSD/EMB, R/GSD1
325 Broadway
Boulder, CO 80305
phone: 970-703-3048
|
Thanks for expressing your concern on the removal of Would it be worthwhile to surround |
After thinking about this a bit more, it seems to me surrounding (wrapping)
those commands with set -e would be the most important thing to do. I’m
away from my computer and can’t check, but probably the reason an error
with the cp command causes scripts to exit (as Chan-Hoo mentioned) is that
those scripts have set -e declared at the top (I think default bash
behavior is not to exit a script if a command fails). So if you use the
wrapped versions of these commands that include set -e, then you’ll be sure
they will exit on error regardless of whether the calling script uses set
-e.
Just my two cents. Please proceed as you see fit Thanks.
…On Friday, April 19, 2024, Michael Lueken ***@***.***> wrote:
@gsketefian <https://github.com/gsketefian> -
Thanks for expressing your concern on the removal of _vrfy from the bash
commands. With this PR, I was simply following through with @mkavulich
<https://github.com/mkavulich>'s and @chan-hoo
<https://github.com/chan-hoo>'s recommendation of fully removing the _vrfy.
As @chan-hoo <https://github.com/chan-hoo> noted, The standard bash
commands should exit automatically on a failure.
Would it be worthwhile to surround cd, cp, ln, mkdir, mv, and rm commands
with set -x and set +x, so that they should error out if an issue is
encountered? I'm open to more opinions and suggestions, but I don't think
that we want to go back to using _vrfy once again.
—
Reply to this email directly, view it on GitHub
<#1074 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHM3ZYVBRLPZRL6IORANMO3Y6FS55AVCNFSM6AAAAABGPFNOFSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANRXGEZTIMZZG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
Gerard Ketefian
Research Scientist
NOAA/OAR/ESRL/GSD/EMB, R/GSD1
325 Broadway
Boulder, CO 80305
phone: 970-703-3048
|
@gsketefian Since it was my issue originally advocating for removal of these scripts I'll provide a defense: I don't see a reason to carry this burden and overhead of custom wrappers around simple shell builtins like this, and as I say in the original issue, they introduce additional problems! Unless I'm missing something, they provide no additional value over just using the For the record, here is an example of a failure using the shell builtin
And here is with the custom
That is nothing but a whole bunch of unhelpful extra text around the same error message. It seems as if it was meant to echo the script it was called from, but again, it does not do this! And that information would be mostly redundant anyway, since exscripts all print a message when they start. I did not realize until this PR that Is this a good time to mention that converting everything to python would make this problem moot? 😉 |
All scripts load the Addressing issue related to failures with the |
…h script to allow the test to run successfully while ran as part of the coverage and comprehensive test suite
After correcting the Gaea, Jet, and Orion are currently not available via Jenkins, so I will go ahead and manually run the WE2E coverage tests on those machines. |
The WE2E tests were manually run on Orion and all tests successfully passed:
Additionally, the metric test was also ran and successfully completed:
|
The Jet WE2E coverage tests were manually run and all passed successfully:
Awaiting results from Gaea and Hera GNU now. |
All Jenkins tests have successfully passed and the Gaea WE2E coverage tests were manually run and all successfully passed:
Merging this work now. |
DESCRIPTION OF CHANGES:
The weather model hash has been updated to 4f32a4b (April 15).
Additionally,
_vrfy
has been removed from thecd
,cp
,ln
,mkdir
,mv
, andrm
bash commands injobs
,scripts
,ush
, andush/bash_utils
. The modified commands don't function as intended (issue #861) and aren't accepted by NCO (issue #1021).Type of change
TESTS CONDUCTED:
The fundamental tests were run on all tier-1 platforms. Comprehensive tests were run on Hera Intel, Orion, and Hercules. The AQM WE2E test was run on Hera Intel and the sample warm start AQM configuration was run on Hercules.
DEPENDENCIES:
None
DOCUMENTATION:
None
ISSUE:
Fixes #861
Fixes #1021
CHECKLIST