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

A RDAS branch re-organized after the current GDASapp using submodules and mpas related component added #9

Closed
wants to merge 12 commits into from

Conversation

TingLei-NOAA
Copy link
Contributor

Following the current GDASapp which had switched to Git submodules to manage components, re-organize RDAS repo including updating its' module setup according to the current GDASapp.
The mpas-jedi related components are also added while they are not included in the current GDASapp.
The building of this RDAS bundle had been successfully done on Hera .
More tests are to be done including a complete/clean "clone" and "build" process and on orion.
But this branch could be used as an starting point for current work on RDAS.

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.

It looks like a bunch of things were just copied from GDASApp to here without 1) thinking if they were relevant to the regional system or 2) renaming variables, scripts, etc.

I think this needs to be cleaned up before it gets merged or else these things will persist.

endif(CLONE_JCSDADATA)

endif(BUILD_RDASBUNDLE)
option(WORKFLOW_TESTS "Include global-workflow dependent tests" OFF)
Copy link
Contributor

Choose a reason for hiding this comment

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

global?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept them there , which would be a good place holder for future regional workflow if it is decided to be included in rdas.

DYCORE="FV3"

while getopts "p:t:c:m:hvdfa" opt; do
#cltorg CLONE_JCSDADATA="NO"
Copy link
Contributor

Choose a reason for hiding this comment

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

These commented out/changed make the build script options worthless

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I take them as a simplified version at the beginning. I am still not sure if components like gdas-utils should be included in rdas.

echo "Building RDASApp on $BUILD_TARGET"
hera | orion | hercules)
echo "Building GDASApp on $BUILD_TARGET"
export PS1="[\u@\h \W]\$ " #clt to avoid unbound PS1 error
Copy link
Contributor

Choose a reason for hiding this comment

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

We just need to move away from Conda. Spack-stack has the python modules we need now. PS1 should not be exported in a non-login shell

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line was added for building on herea, because, otherwise, the building process would abort complain unbound PSI.
Seems conda was invoked somewhere. I will see if a better fix could be found

build.sh Outdated
hera | orion)
echo "Building RDASApp on $BUILD_TARGET"
hera | orion | hercules)
echo "Building GDASApp on $BUILD_TARGET"
Copy link
Contributor

Choose a reason for hiding this comment

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

GDASApp?

build.sh Outdated
source $dir_root/ush/module-setup.sh
module use $dir_root/modulefiles
module load RDAS/$BUILD_TARGET
CMAKE_OPTS+=" -DMPIEXEC_EXECUTABLE=$MPIEXEC_EXEC -DMPIEXEC_NUMPROC_FLAG=$MPIEXEC_NPROC -DBUILD_GSIBEC=ON"
module load GDAS/$BUILD_TARGET.$COMPILER
Copy link
Contributor

Choose a reason for hiding this comment

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

GDAS?

endif()

# Gibbs seawater
#clt ecbuild_bundle( PROJECT gsw SOURCE "../sorc/gsw" )
Copy link
Contributor

Choose a reason for hiding this comment

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

don't need this, no soca

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, soca building failed in my tries and building of them were turned off or commented out.
But, I am wondering if they could be included as an optional components when some developments in it are interesting to regional jedi too. But this might be done without keeping them in rdas. I would follow the decision made during this reviewing process.


# FMS and FV3 dynamical core
ecbuild_bundle( PROJECT fms SOURCE "../sorc/fms" )
ecbuild_bundle( PROJECT fv3 SOURCE "../sorc/fv3" )
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for fv3 anymore ,right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For being now, I still hope to keep fv3/fv3-jedi still in rdas. At least, it includes rich examples for our jedi work.

ecbuild_bundle( PROJECT mpas-jedi-data SOURCE "../sorc/mpas-jedi-data" )
ecbuild_bundle( PROJECT mpas-jedi SOURCE "../sorc/mpas-jedi" )

# SOCA associated repositories
Copy link
Contributor

Choose a reason for hiding this comment

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

no soca in regional

@@ -0,0 +1,69 @@
#!/usr/bin/env python3
Copy link
Contributor

Choose a reason for hiding this comment

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

we shouldn't use this anymore, global is migrating to wxflow YAML tools. Ask @aerorahul for details

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@CoryMartin-NOAA Thanks a lot for your comments/suggestions in such a prompt way.
I would dig more on this and talk to @aerorahul on this .

@@ -0,0 +1,83 @@
#!/usr/bin/env python3
Copy link
Contributor

Choose a reason for hiding this comment

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

@danholdaway hopes to make this redundant with JCB, I suggest this script is not even needed anyways as of now

@TingLei-NOAA
Copy link
Contributor Author

It looks like a bunch of things were just copied from GDASApp to here without 1) thinking if they were relevant to the regional system or 2) renaming variables, scripts, etc.

I think this needs to be cleaned up before it gets merged or else these things will persist.

Yes, in this rdas re-organizing, I preferred keeping/copying components in gdasapp into rdas if they didn't cause much troubles for radas building. I (or other RDAS developers) would clean them through this PR process and future use/developments of radas.

@delippi delippi self-requested a review March 12, 2024 13:14
@CoryMartin-NOAA
Copy link
Contributor

@TingLei-NOAA I think we (@ShunLiu-NOAA @danholdaway ) should chat soon. GDASApp is containing the GDAS-specific implementation of JEDI, and will be where releases are tagged, and the source code is controlled with hashes/tags. I am nervous about blindly copying things for RDAS, as these two systems will be on a different upgrade cycle, and thus need different modulefiles, different hashes, and because of MPAS vs FV3 and other reasons, different utilities (like blending would not need to be in the global)

@ShunLiu-NOAA
Copy link

Ting, Thank you for working on this.
@CoryMartin-NOAA @danholdaway, I have the same concerns about how to keep RDASApp and GDASApp on the same page.

@TingLei-NOAA
Copy link
Contributor Author

TingLei-NOAA commented Mar 12, 2024

@TingLei-NOAA I think we (@ShunLiu-NOAA @danholdaway ) should chat soon. GDASApp is containing the GDAS-specific implementation of JEDI, and will be where releases are tagged, and the source code is controlled with hashes/tags. I am nervous about blindly copying things for RDAS, as these two systems will be on a different upgrade cycle, and thus need different modulefiles, different hashes, and because of MPAS vs FV3 and other reasons, different utilities (like blending would not need to be in the global)

Sure. a clear plan for how to use rdas is needed. The current PR is supposed to make rdas could at least work (be able to be built on hera/orion) by, at least, EMC developers for being now.
To use the current heads of submodules rather than using hashed commits in gdasapp (or rdas), a submodule updating script is added : checkout_submodules_branches.sh.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@TingLei-NOAA , This is bit-for-bit the same file as the GDAS/hera.intel.lua. Do we need the GDAS directory at all? Also, being bit-for-bit there are mentions to GDAS instead of RDAS. The same goes for the hercules and orion *lua files under the RDAS/ directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@delippi Yes. now it is just a mirror of GDAS modules. To have a RDAS dir, it is more readable for like the build.sh. We could make it a link pointing to the GDAS. However, at the same time, I am wondering if we need the flexibilities to use different setting up modules from GDAS and, hence, use the current practice.

@@ -24,7 +24,6 @@ usage() {
echo " -f force a clean build DEFAULT: NO"
echo " -d include JCSDA ctest data DEFAULT: NO"
echo " -a build everything in bundle DEFAULT: NO"
echo " -m select dycore DEFAULT: FV3"
Copy link
Collaborator

Choose a reason for hiding this comment

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

@TingLei-NOAA , why remove this option and not just change to default to MPAS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to re-sort out on this part of this part. Thanks.

@aerorahul
Copy link

@TingLei-NOAA I think we (@ShunLiu-NOAA @danholdaway ) should chat soon. GDASApp is containing the GDAS-specific implementation of JEDI, and will be where releases are tagged, and the source code is controlled with hashes/tags. I am nervous about blindly copying things for RDAS, as these two systems will be on a different upgrade cycle, and thus need different modulefiles, different hashes, and because of MPAS vs FV3 and other reasons, different utilities (like blending would not need to be in the global)

FWIW, I am with @CoryMartin-NOAA here. The RDASApp should use GDASApp as a guide and add features that are needed and build on them, rather than import everything and then spend time removing them later.

ShunLiu-NOAA
ShunLiu-NOAA previously approved these changes Mar 13, 2024
@ShunLiu-NOAA
Copy link

@TingLei-NOAA Can we close this PR and open an new PR to merge other changes that you want to do to the current "develop"

@TingLei-NOAA
Copy link
Contributor Author

This PR is closed for #11 has been merged into develop branch.
Thanks to all who had contributed to the discussions on this PR.

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