-
Notifications
You must be signed in to change notification settings - Fork 119
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] bug fix for processing radar reflectivity task #849
[develop] bug fix for processing radar reflectivity task #849
Conversation
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.
@EdwardSnyder-NOAA These changes look good to me!
I also ran the fundamental WE2E tests on Jet and ran the process_obs
test on Jet as well to ensure that the changes successfully passed on the machine that the test is actively ran on. All tests successfully passed.
Approving this work now.
@EdwardSnyder-NOAA quick question about this line in your PR message:
Does this mean the da_cycling test should be updated to include these variables? |
@mkavulich - These variables ( |
@mkavulich & @MichaelLueken - For this PR, I've now added the cycling variables ( |
Thank you very much for adding the cycling variables to all of the machine files, adding the When PR #853 was merged last Monday, it caused a conflict 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.
These changes look good, thanks for answering my questions about those variables. Sorry I have forgotten the conclusion of the conversation at the last meeting, is the plan still to merge this PR despite the RRFS removal?
@mkavulich and @EdwardSnyder-NOAA - It looks like the modifications to With the removal of all DA and RRFS-related work in @christinaholtNOAA's PR #893, which would include removing the rest of the changes that would be introduced as part of this PR, would @EdwardSnyder-NOAA be willing to remove the rest of the changes (since they are going to be removed shortly in another PR) and just keep the fixes in |
@MichaelLueken I removed the rrfs related work and left changes in the prep.yaml and orion.yaml |
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.
Re-approving, still looks good 👍
The WE2E coverage tests were manually ran on Jet (the old jet-epic pipeline is no longer valid since all tier-1 platforms are now using the EPIC allocations and locations). All tests successfully passed:
Additionally, a rerun of the Hera Intel tests were run and all successfully passed:
The Jenkins tests on Gaea, Hera GNU, and Orion all successfully passed. Moving forward with merging this work now. |
DESCRIPTION OF CHANGES:
While testing the new
process_radarref_prod
task on AWS and Cheyenne, I noticed that the script would return a successful message even though there is an error present. See code snippet below.There are two issues:
process_radarref_prod
task returns a successful message when it should error out.After some investigation, the 1st issue is resolved by modifying the print command in
scripts/exregional_process_radarref.sh
. And the 2nd issue is resolved by adding theenvars
variable to the processing radar task inparm/wflow/da_data_preproc.yaml
file. Addingenvars
allows thennodes
variable to be updated when the task is called, which results in the right number of processes (nprocs). Presumably we didn't catch this error before because this task was tested on Jet and Hera which use srun, while AWS and Cheyenne use mpiexec/mpirun which uses the nprocs argument.This test was run by copying the
config.da_cycling.yaml
and adding some da cycling variables to the machine file likeRAP_OBS_BUFR
andNSSLMOSAIC
. This case data is present on Hera, Jet, Gaea, Cheyenne, and AWS with it partially staged on Orion.Type of change
TESTS CONDUCTED:
DEPENDENCIES:
DOCUMENTATION:
ISSUE:
CHECKLIST
LABELS (optional):
A Code Manager needs to add the following labels to this PR:
CONTRIBUTORS (optional):