-
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] Removing RRFS-related features. #893
[develop] Removing RRFS-related features. #893
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.
One required change and a few comments/questions. You will also need to add the following lines to the tests config.MET_ensemble_verification_only_vx.yaml
and config.MET_verification_only_vx.yaml
to ensure they pass:
nco:
NET_default: 'rrfs'
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.
@mkavulich Thanks for the review. I've addressed a bunch of your changes locally, and have made a few others @gspetro-NOAA found while updating the docs. I will test those changes and then push.
I've pushed my tested changes here, and am running a final round of the fundamental tests after I merged When I ran the comprehensive test suite on hera before I merged |
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.
Thanks for addressing my comments!
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.
@christinaholtNOAA - Thanks for addressing my concerns! All of the fundamental tests are successfully passing now:
----------------------------------------------------------------------------------------------------
Experiment name | Status | Core hours used
----------------------------------------------------------------------------------------------------
grid_RRFS_CONUScompact_25km_ics_HRRR_lbcs_RAP_suite_RRFS_v1beta COMPLETE 8.31
nco_grid_RRFS_CONUS_25km_ics_FV3GFS_lbcs_FV3GFS_timeoffset_suite_ COMPLETE 11.15
grid_RRFS_CONUS_25km_ics_FV3GFS_lbcs_FV3GFS_suite_GFS_v15p2 COMPLETE 6.43
grid_RRFS_CONUS_25km_ics_FV3GFS_lbcs_FV3GFS_suite_GFS_v17_p8_plot COMPLETE 12.51
grid_RRFS_CONUScompact_25km_ics_HRRR_lbcs_HRRR_suite_HRRR COMPLETE 25.76
grid_SUBCONUS_Ind_3km_ics_HRRR_lbcs_RAP_suite_WoFS_v0 COMPLETE 13.20
grid_RRFS_CONUS_25km_ics_NAM_lbcs_NAM_suite_GFS_v16 COMPLETE 18.68
----------------------------------------------------------------------------------------------------
Total COMPLETE 96.04
Approving now.
@MichaelLueken Thanks for the follow up. I am seeing the same on Hera when I checked back in on my latest run this morning. |
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.
@christinaholtNOAA - Unfortunately, the Jet coverage tests failed because the process_obs
WE2E test is still present in tests/WE2E/machine_suites/coverage.jet
. Please remove this test, then I can queue up the automated tests on Jet. Thanks!
@christinaholtNOAA - It looks like there are also GSI related changes that should be removed in @natalie-perlin - is it safe for @christinaholtNOAA to remove these lines as part of her removal of RRFS-related features in this PR? Thanks! |
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.
Thank you very much, @christinaholtNOAA! Reapproving now and resubmitting Gaea and Jet tests.
modulefiles/build_gaea_intel.lua
Outdated
@@ -20,7 +20,6 @@ local MKLROOT="/opt/intel/oneapi/mkl/2022.0.2/" | |||
prepend_path("LD_LIBRARY_PATH",pathJoin(MKLROOT,"lib/intel64")) | |||
pushenv("MKLROOT", MKLROOT) | |||
|
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.
@christinaholtNOAA ,
all these could be removed, too, as they were needed for GSI only:
prepend_path("LD_LIBRARY_PATH",pathJoin(MKLROOT,"lib/intel64"))
pushenv("MKLROOT", MKLROOT)
pushenv("CRAYPE_LINK_TYPE","dynamic")
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.
Alternatively, I could take care of these when updating the modulefiles #889 , as it may be scheduled to be merged after the present one (993)
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.
@natalie-perlin I can go ahead and remove those here.
@christinaholtNOAA - The rerun of the Jenkins tests on Gaea successfully passed. The Jet tests that require pulling data from HPSS failed to pull data from HPSS with the following errors:
Resubmitting the tests this morning to see if they will pass. Once successful, I can merge this work. |
Just as a head's up, according to the Jet helpdesk, the issue with accessing Jet via the Princeton bastion is also causing issues with Jet accessing HPSS. |
The Jenkins WE2E tests that previously failed on Jet, due to access issues to HPSS, have successfully passed. Will now move forward with merging this PR. |
DESCRIPTION OF CHANGES:
After recent high-level decisions to discontinue support of GSI, we have decided to discontinue support for any cycling with GSI in SRW. This PR aims to remove all the unnecessary components that have been added to the App consistent with those decisions.
Specifically, the following changes were made:
Type of change
TESTS CONDUCTED:
It would be helpful if someone could test this branch to ensure it doesn't cause problems for AQM. I have not tested that capability.
DEPENDENCIES:
None.
DOCUMENTATION:
@gspetro-NOAA Let me know what makes sense in terms of updating docs.
ISSUE:
None.
CHECKLIST