Skip to content
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

GSI related updates for v16.3.0 #840

Merged

Conversation

aerorahul
Copy link
Contributor

@aerorahul aerorahul commented Jun 2, 2022

Description

This PR:

  • updates the GSI branch to release/gfsda.v16.3.0 in checkout.sh
  • Uses updated build_gsi.sh script specifically for NCO (but also applicable for global-workflow developers)
  • Links the updated executable names gsi.x, enkf.x and ncdiag_cat_serial.x. Retains older executable link names for the time being until the GSI branch is updated.

Closes #743

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • Clone and Build tested on WCOSS2

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • My changes generate no new warnings
  • New and existing tests pass with my changes

Dependencies
Blocked by #839

Copy link

@MichaelLueken MichaelLueken left a comment

Choose a reason for hiding this comment

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

I have completed my review of these changes. The changes to build_gsi.sh and checkout.sh are fine. I have two issues with link_fv3gfs.sh:

  1. The paths to the DA Monitoring fix, jobs, parm, scripts, and ush files are different. There is no version numbers. Please remove the version numbers from lines 157 - 193.
  2. The release/gfsda.v16.3.0 branch was updated yesterday afternoon to use gsi.x, enkf.x, and ncdiag_cat_serial.x in the scripts at b19916b. Lines 235 - 241 are no longer necessary.

Following these changes, this PR should be ready to be merged.

@aerorahul aerorahul requested a review from MichaelLueken June 3, 2022 14:25
Comment on lines 235 to 241
# TODO - remove these lines when the scripts in the release/gfsda.v16.3.0 tag are updated
[[ -s global_gsi.x ]] && rm global_gsi.x
$LINK ../sorc/gsi.fd/exec/gsi.x global_gsi.x
[[ -s global_enkf.x ]] && rm global_enkf.x
$LINK ../sorc/gsi.fd/exec/enkf.x global_enkf.x
[[ -s ncdiag_cat.x ]] && rm ncdiag_cat.x
$LINK ../sorc/gsi.fd/exec/ncdiag_cat_serial.x ncdiag_cat.x
Copy link
Contributor

Choose a reason for hiding this comment

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

@MichaelLueken-NOAA had updated gfsda.v16.3.0
GSI PR 401

Copy link

@MichaelLueken MichaelLueken left a 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 concerns, @aerorahul! All changes look good.

Copy link
Member

@KateFriedman-NOAA KateFriedman-NOAA left a comment

Choose a reason for hiding this comment

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

Changes look good, thanks @aerorahul !

@@ -19,9 +19,7 @@ if [ ! -d "../exec" ]; then
mkdir ../exec
fi

cd gsi.fd/ush/
Copy link
Contributor

Choose a reason for hiding this comment

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

Advise against using build_4nco_global.sh. Use and design of this script should first be discussed with EIB. As currently configured build_4nco_global.sh does not build all DA executables. The following executables are missing: calc_analysis.x, interp_inc.x, ozmon*.x, radmon*.x

Recommend following @MichaelLueken-NOAA suggestion in #776, specifically his suggestions for modifying sorc/build_gsi.sh found here. This approach was tested in a working copy of ffeature/update_gsibuild_for_gfsda.v16.3.0. The above missing DA executables were created along with the other expected DA apps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RussTreadon-NOAA @MichaelLueken-NOAA
I would add -DBUILD_UTIL_MON=ON -DBUILD_UTIL_NCIO=ON to the build_4nco_global.sh to enable building the missing executables.

I will also add an option to PRUNE_4NCO block that either prunes or retains the "non-global" bits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for using the PRUNE_4NCO=YES option for NCO is to address the bugzilla ticket that NCO had opened about too many unused files in the gsi.fd directory.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @aerorahul. I can test your changes when they are ready.

Choose a reason for hiding this comment

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

@RussTreadon-NOAA I have added -DBUILD_UTIL_MON=ON -DBUILD_UTIL_NCIO=ON to ush/build_4nco_global.sh in release/gfsda.v16.3.0. This was done at 40f0efd.

Copy link
Contributor Author

@aerorahul aerorahul Jun 3, 2022

Choose a reason for hiding this comment

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

@RussTreadon-NOAA
I updated this branch and opened a PR in GSI NOAA-EMC/GSI#404.

Choose a reason for hiding this comment

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

@RussTreadon-NOAA I have merged @aerorahul modification to ush/build_4nco_global.sh to release/gfsda.v16.3.0 at e6535cc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RussTreadon-NOAA
The pruning PR in the GSI had a bug.
I opened a bugfix PR NOAA-EMC/GSI#405

Choose a reason for hiding this comment

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

@aerorahul Your ush/build_4nco_global.sh bug fix was merged to release/gfsda.v16.3.0 at 42cc837.

Copy link
Contributor

@RussTreadon-NOAA RussTreadon-NOAA left a comment

Choose a reason for hiding this comment

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

Changes to sorc/build_gsi.sh do not create all expected DA executables. Advise against using build_4nco_global.sh at present. Prefer @MichaelLueken-NOAA's suggestion.

@RussTreadon-NOAA
Copy link
Contributor

If we modify sorc/build_gsi.sh as

-./gsi.fd/ush/build_4nco_global.sh
+export GSI_MODE="GFS"
+export UTIL_OPTS="-DBUILD_UTIL_ENKF_GFS=ON -DBUILD_UTIL_MON=ON -DBUILD_UTIL_NCIO=ON"
+./gsi.fd/ush/build.sh

the following change is needed in sorc/link_fv3gfs.sh

-    $LINK ../sorc/gsi.fd/exec/$gsiexe .
+    $LINK ../sorc/gsi.fd/install/bin/$gsiexe .

@RussTreadon-NOAA
Copy link
Contributor

The following check was made:

  • Update working copy of feature/update_gsibuild_for_gfsda.v16.3.0 to fbf2d3d
  • Rerun sorc/checkout.sh to clone fresh copy of GSI release/gfsda.v16.3.0 (at e6535ccb)
  • Execute sorc/build_gsi.sh. Expected global DA apps built. Default behavior is to preserve files in sorc/gsi.fd.
  • Execute sorc/build_gsi.sh with PRUNE_4NCO=YES. Expected global DA apps built, but extraneous files were not removed as expected. I'll look more closely at this but this should not prevent this PR from moving forward

We should examine GSI ush/build_4nco_global.sh to confirm that it removes everything it should but this can be the subject of future GSI PR.

Copy link
Contributor

@RussTreadon-NOAA RussTreadon-NOAA left a comment

Choose a reason for hiding this comment

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

Expected DA apps built.

@RussTreadon-NOAA
Copy link
Contributor

The following check was made:

  • Update working copy of feature/update_gsibuild_for_gfsda.v16.3.0 to fbf2d3d
  • Rerun sorc/checkout.sh to clone fresh copy of GSI release/gfsda.v16.3.0 (at e6535ccb)
  • Execute sorc/build_gsi.sh. Expected global DA apps built. Default behavior is to preserve files in sorc/gsi.fd.
  • Execute sorc/build_gsi.sh with PRUNE_4NCO=YES. Expected global DA apps built, but extraneous files were not removed as expected. I'll look more closely at this but this should not prevent this PR from moving forward

We should examine GSI ush/build_4nco_global.sh to confirm that it removes everything it should but this can be the subject of future GSI PR.

Reran PRUNE_4NCO=YES test with GSI release/gfsda.v16.3.0 at 42cc837f. Global DA apps built and extraneous files removed.

@aerorahul aerorahul merged commit ab644cb into release/gfs.v16.3.0 Jun 3, 2022
@aerorahul aerorahul deleted the feature/update_gsibuild_for_gfsda.v16.3.0 branch June 3, 2022 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants