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

Creating mega rdas.x to replace individual executables #95

Merged
merged 12 commits into from
Jul 19, 2024

Conversation

SamuelDegelia-NOAA
Copy link
Contributor

@SamuelDegelia-NOAA SamuelDegelia-NOAA commented Jun 28, 2024

This PR creates two "mega" executables called rdas_fv3jedi.x and rdas_mpasjedi.x. These larger executables combine many of the individual executables that previously only worked for a single application (i.e., variational, letkf, bump). This is needed for NCO compliance and generally follows GDASApp PR #1075.

The choice to create separate executables for fv3jedi and mpasjedi will make it easier to simply remove one whenever we start real-time runs.

There are some differences for rdas_${dycore}.x from gdas.x including the new mpas-jedi version, removal of soca options, addition of bump option, and a slightly different build route via RDASApp/bundle/CMakeLists.txt.

The mega executables have been tested on Hera for both fv3-jedi and mpas-jedi including the newly added bump option. I also updated the ctests and the template scripts run_fv3jedi_template.sh and run_mpasjedi_template.sh for this new executable structure. One thing I noticed is that some of the mpasjedi ctests initially failed due to a memory error. I am not sure what change could have caused this, but I added a --ntasks-per-node=18 option to the ctests to allow them to use 2 nodes instead of 1. After that, everything seems to be working as expected.

Usage examples:

./rdas_fv3jedi.x variational ${yaml}
./rdas_fv3jedi.x localensembleda ${yaml}
./rdas_fv3jedi.x bump ${yaml}
./rdas_mpasjedi.x variational ${yaml}
./rdas_mpasjedi.x localensembleda ${yaml}
./rdas_mpasjedi.x bump ${yaml}

CTest results:

[Samuel.Degelia@hfe08 rrfs-test]$ cd RDASApp/build/rrfs-test
[Samuel.Degelia@hfe08 rrfs-test]$ ctest
Test project /scratch1/NCEPDEV/da/Samuel.Degelia/RDASApp_dev/RDASApp/build/rrfs-test
    Start 1: rrfs_fv3jedi_hyb_2022052619
1/5 Test #1: rrfs_fv3jedi_hyb_2022052619 ............   Passed  126.21 sec
    Start 2: rrfs_fv3jedi_letkf_2022052619
2/5 Test #2: rrfs_fv3jedi_letkf_2022052619 ..........   Passed   21.84 sec
    Start 3: rrfs_mpasjedi_2022052619_Ens3Dvar
3/5 Test #3: rrfs_mpasjedi_2022052619_Ens3Dvar ......   Passed   53.59 sec
    Start 4: rrfs_mpasjedi_2022052619_atms_npp_qc
4/5 Test #4: rrfs_mpasjedi_2022052619_atms_npp_qc ...   Passed   78.00 sec
    Start 5: rrfs_mpasjedi_2022052619_letkf
5/5 Test #5: rrfs_mpasjedi_2022052619_letkf .........   Passed   42.73 sec

100% tests passed, 0 tests failed out of 5

Label Time Summary:
mpi            = 322.37 sec*proc (5 tests)
rdas-bundle    = 322.37 sec*proc (5 tests)
script         = 322.37 sec*proc (5 tests)

Total Test time (real) = 323.51 sec

@guoqing-noaa
Copy link
Collaborator

@SamuelDegelia-NOAA Thanks for working on this. Look forward to it.

It is a good idea for everyone to create an issue for what he is working on or planning to work on to avoid duplicate efforts and facilitate discussions.

@SamuelDegelia-NOAA
Copy link
Contributor Author

SamuelDegelia-NOAA commented Jun 28, 2024

Thank you for the suggestion, @guoqing-noaa. I mentioned that I would be working on this during some internal EMC meetings but it would be better to open an issue here on RDASApp so that all collaborators are aware. I will do that now.

@SamuelDegelia-NOAA SamuelDegelia-NOAA linked an issue Jun 28, 2024 that may be closed by this pull request
@SamuelDegelia-NOAA SamuelDegelia-NOAA changed the title Creating mega rdas.x Creating mega rdas.x to replace individual executables Jul 4, 2024
@SamuelDegelia-NOAA SamuelDegelia-NOAA marked this pull request as ready for review July 5, 2024 12:43
@SamuelDegelia-NOAA
Copy link
Contributor Author

I think this PR is now good to go for review. I updated the main text with some new information.

CoryMartin-NOAA
CoryMartin-NOAA previously approved these changes Jul 5, 2024
Copy link
Contributor

@CoryMartin-NOAA CoryMartin-NOAA left a comment

Choose a reason for hiding this comment

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

One suggestion but otherwise this looks great!

@TingLei-NOAA
Copy link
Contributor

@SamuelDegelia-NOAA I have two general comments. First, It is good to follow gdas to have a super executable to be ready when it is the solution to meet the nco requirement. But I don't think it is good to use now in rrfs scripts/ctests ( because, use of such a super executable only makes the readability/organization less clear). I would suggest this PR only include the changes for codes. Second, I believe, there would be similar situations in the future when rdas could borrow stuff from gdas and do rdas's own changes/modifications to them. If possible, I would suggest to explore "git subtree" to transfer those components from gdas to rdas. Doing so will not only keep a complete history of the codes but, also make rdas developer could use similar git "pull" command to incorporate new changes to the incorporated codes in GDAS.

@SamuelDegelia-NOAA
Copy link
Contributor Author

@TingLei-NOAA I feel like it is good to get developers used to using the rdas.x version so that it isn't a big adjustment down the road. That is why I updated the ctests and run scripts. Plus, I do not think using ./rdas_fv3jedi.x variational instead of ./fv3jedi_variational is significantly less readable or clear.

Regarding the git subtree, this sounds like a good idea going forward. I wasn't aware of this option but it would be great to track changes from the original gdas version.

@TingLei-NOAA
Copy link
Contributor

@SamuelDegelia-NOAA The key is that use of gdas.x or rdas.x is for compliance with current nco requirement (and not the only work-around). It is definitely not contributing to readability/maintainability ( of course, I would not argue how "significantly" it does on the other direction). If needed, changes in the scripts from the original jedi executables to the use of the super executable with corresponding extra parameters is only a text "finding/replacing" process and I don't think developers needs a time to be familiar with that. But, starting from such use of that super executable for developers to dig/understand jedi is to add an extra (and unnecessary) wrapping interface. Of course, that is my opinion. The decision would be made by rdasapp managing team. I just want to emphasize that the use of such a super executable is a decision/choice to be made by rdasapp community and not a logical following up after current gdas' example.

@SamuelDegelia-NOAA
Copy link
Contributor Author

SamuelDegelia-NOAA commented Jul 5, 2024

@TingLei-NOAA I guess that adding this mega executable can make it harder to track down an issue with a specific application. For example, knowing that you should search for fv3jedi_variational.x if you have an issue with the variational component of rdas.x. If others think that the ctests and template scripts should use the older exes then I am fine reverting that part of the PR.

hu5970
hu5970 previously approved these changes Jul 15, 2024
Copy link
Contributor

@hu5970 hu5970 left a comment

Choose a reason for hiding this comment

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

looks good to me

@SamuelDegelia-NOAA
Copy link
Contributor Author

After some discussion today with @TingLei-NOAA and @ShunLiu-NOAA, I reverted the ctests and experiment scripts to use the original executables. Everything else with the PR is the same, but we only plan to use the mega executable once we move to the workflow and working on WCOSS.

@ShunLiu-NOAA
Copy link

@SamuelDegelia-NOAA Thank you for the update.

@ShunLiu-NOAA
Copy link

@hu5970 @guoqing-noaa please review Sam's modification.

@ShunLiu-NOAA ShunLiu-NOAA merged commit 586bdcb into develop Jul 19, 2024
3 checks passed
@ShunLiu-NOAA ShunLiu-NOAA deleted the feature/mega_exe branch July 19, 2024 16: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.

Create mega rdas.x executable
6 participants