Skip to content

Move mrms_pull_topofhour.py from scripts/ to ush/ [develop]#571

Merged
jwolff-ncar merged 3 commits into
ufs-community:developfrom
jwolff-ncar:feature/mrms_topofhour
Aug 26, 2021
Merged

Move mrms_pull_topofhour.py from scripts/ to ush/ [develop]#571
jwolff-ncar merged 3 commits into
ufs-community:developfrom
jwolff-ncar:feature/mrms_topofhour

Conversation

@jwolff-ncar
Copy link
Copy Markdown
Contributor

DESCRIPTION OF CHANGES:

Moved the mrms_pull_topofhour.py from scripts/ into ush/.
Made a small change to the script to remove a 'tab' and make it 'spaces' instead.
Added a set +u in the load_modules_run_task.sh to ensure the correct environment for running the python script.

TESTS CONDUCTED:

Tested one case on hera including the verification step. The verification tasks are not yet supported on other platforms.

DEPENDENCIES:

None.

DOCUMENTATION:

No updates needed.

ISSUE (optional):

Fixes issue mentioned in https://github.com/NOAA-EMC/regional_workflow/issues/486

CONTRIBUTORS (optional):

@jwolff-ncar jwolff-ncar added enhancement New feature or request Tested on Hera Tested successfully on Hera machine labels Aug 17, 2021
Copy link
Copy Markdown
Collaborator

@JeffBeck-NOAA JeffBeck-NOAA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should ush/load_modules_run_task.sh have changed?

@jwolff-ncar
Copy link
Copy Markdown
Contributor Author

As I mentioned in the description of changes, I had to add the set +u to get the environment right for the python script. I had worked with @mkavulich on this so perhaps he wants to also take a look for approval.

@JeffBeck-NOAA
Copy link
Copy Markdown
Collaborator

As I mentioned in the description of changes, I had to add the set +u to get the environment right for the python script. I had worked with @mkavulich on this so perhaps he wants to also take a look for approval.

OK, if it's just to ensure the correct environment, it's likely only necessary for debugging, correct?

@@ -331,7 +332,6 @@ if [ -n "${SRW_ENV:-}" ] ; then
conda activate ${SRW_ENV}
fi

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to be entirely consistent with previous behavior, we should include a "set -u" line here. But I do not think it's necessary

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am pretty sure I tested with that and it didn't work so I would say let's not add that if it isn't deemed necessary.

@mkavulich
Copy link
Copy Markdown
Collaborator

@JeffBeck-NOAA The set +u is necessary prior to the call to conda activate on some machines due to the way the conda scripts work...they have some bash scripts with unset variables, which end up inheriting our -u flag which fails when it encounters unset variables. Ideally best practice would be to not have unset variables, but we do not have control over those scripts, so the next best thing is to use set +u to stop checking for unset variables prior to calling conda activate. This is the solution @christinaholtNOAA recommended.

@christinaholtNOAA
Copy link
Copy Markdown
Contributor

The way we approached it for the changes we introduced for Jet/Hera in RRFS_dev1 is here starting at line 332. I suggest wrapping only the single line that needs it (and is out of our control) in the set +u and set -u.

@JeffBeck-NOAA
Copy link
Copy Markdown
Collaborator

Thanks @mkavulich, @christinaholtNOAA. I'm still a bit confused, since I've not had any problems on Hera with the load_modules_run_task script before this PR.

@jwolff-ncar
Copy link
Copy Markdown
Contributor Author

@JeffBeck-NOAA you would have only run into this problem if you were pulling/processing MRMS data from the HPSS rather than using staged observations. It was not an issue when the capability was first added but noticed about 1.5 months ago so something changed and it is now required.

@jwolff-ncar
Copy link
Copy Markdown
Contributor Author

@mkavulich Were you going to make a change to include the the set +u and set -u updates for me to test?

@mkavulich
Copy link
Copy Markdown
Collaborator

@jwolff-ncar I made the change and tested on Hera, both for the GST and your sample MET case you sent me. Everything is functioning as expected with the updated set flags.

@jwolff-ncar jwolff-ncar merged commit c5bd1ff into ufs-community:develop Aug 26, 2021
christinaholtNOAA pushed a commit to christinaholtNOAA/regional_workflow that referenced this pull request Oct 6, 2021
…unity#571)

* Moved mrms_pull_topofhour.py to ush/ rather than scripts/

* Updates to get the mrms_pull_topofhour.py script to run with the proper environment loaded. Fixed a tab issue in the py script as well.

* Move "set +u" to the place it is absolutely needed, add a "set -u" at the end to restore the unset variable flag

Co-authored-by: Michael Kavulich, Jr <kavulich@ucar.edu>
@jwolff-ncar jwolff-ncar deleted the feature/mrms_topofhour branch December 22, 2021 21:26
shoyokota pushed a commit to shoyokota/regional_workflow that referenced this pull request Nov 27, 2023
* Save stdout for EnKF radardbz analysis
Remove Jet local directories from config.sh.RRFS_CONUS_3km_ens
Remove specification of "NET" from config.sh.RRFS_CONUS_3km and config.sh.RRFS_CONUS_3km_ens, and change the file dependencies of the ensctrl grib2 files for ensemble_maps and enspost

* Update exregional_run_prepstart.sh
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request Tested on Hera Tested successfully on Hera machine

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants