add cdeps prefix to the cdeps data component module names by junwang-noaa · Pull Request #31 · NOAA-EMC/CDEPS

junwang-noaa · 2021-06-03T16:55:51Z

Description of changes

The PR changes the cdeps data component module name from xxx_comp_nuopc to cdeps_dxxx_comp_nuopc, Those names are unique, so the issue with using same module name in the UFS or other model will be resolved.

Specific notes

CDEPS Issues Fixed (include github issue #): no c

Are there dependencies on other component PRs

CIME (list): the file names are not changed to avoid make file change, but the FRONT model name defined in NEMS driver will need to point to cdeps_dxxx_comp_nuopc, instead of xxx_comp_nuopc.
CDEPS (list): no other change in CDEPS

Are changes expected to change answers?

bit for bit
different at roundoff level
more substantial

Any User Interface Changes (namelist or namelist defaults changes)?

Yes
No

Testing performed:

(required) aux_cdeps
- machines and compilers:
- details (e.g. failed tests):
(optional) CESM prealpha test
- machines and compilers
- details (e.g. failed tests):

Hashes used for testing:

CIME
- repository to check out: https://github.com/ESCOMP/CESM.git
- branch:
- hash:
CMEPS
- repository to check out: https://github.com/ESCOMP/CESM.git
- branch:
- hash:
CESM
- repository to check out: https://github.com/ESCOMP/CESM.git
- branch:
- hash:

junwang-noaa · 2021-06-03T17:06:51Z

The UFS DATM test passes with this PR and the UFS #PR 611 ( line https://github.com/ufs-community/ufs-weather-model/pull/611/files#diff-a2e8c35f304291c60982542bf57cd54220347ddf28cacdb8b5c7bb90c9d2a7aaR30 needs to set DISABLE_FoX to public.

uturuncoglu · 2021-06-03T17:07:17Z

@junwang-noaa I think we need to test this with CESM also. Let me try to test and see what happens in CESM side. Did you run at least a couple application with this branch? CMEPS could also fail but I am not sure.

junwang-noaa · 2021-06-03T17:10:00Z

@uturuncoglu Would you please try the changes to see if it solves the issues in HAFS? No script changes required. I assume it will work with: FV3+HYCOM, DATM+MOM6+CICE6 and FV3+DOCN+MOM6+CICE6. Please let me know if you still have issues. Thanks

junwang-noaa · 2021-06-03T17:12:50Z

@uturuncoglu On CIME side, I think your need to update the FRONT models in make files, it needs to use: cdeps_dxxx_comp_nuopc, instead xxx_comp_nuopc. Thanks for testing in CIME.

uturuncoglu · 2021-06-03T17:14:56Z

@junwang-noaa Sure. My Orion account is expired last week and Arun is working on it. So, I am trying to move my development to Cheyenne but I don't have run directories for HAFS in there. So, it might take little bit time for me to test it.

DeniseWorthen · 2021-06-03T17:17:26Z

@uturuncoglu Let me know if you want me to transfer something from Orion to Cheyenne.

uturuncoglu · 2021-06-03T17:18:46Z

@DeniseWorthen That would be great, could you access to following directory /work/noaa/gmtb/tufuk/RUN/AtmOcnWav

uturuncoglu · 2021-06-03T17:19:16Z

@DeniseWorthen i am not sure you could access or not to gmtb group.

uturuncoglu · 2021-06-03T17:35:51Z

@uturuncoglu On CIME side, I think your need to update the FRONT models in make files, it needs to use: cdeps_dxxx_comp_nuopc, instead xxx_comp_nuopc. Thanks for testing in CIME.

I checked the CMEPS side and it seems that it is expecting certain names in drivers/cime/esm.F90. I need to discuss possible solution for it with Jim and Mariana before going forward and make the changes because this could effect CESM regression tests. I'll update you about it.

aerorahul

Code changes look good.
I have one suggestion that could make this less repetitive.

aerorahul · 2021-06-03T17:38:26Z

-    character(*)    ,parameter :: F00 = "('(atm_comp_nuopc) ',8a)"
-    character(*)    ,parameter :: F01 = "('(atm_comp_nuopc) ',a,2x,i8)"
-    character(*)    ,parameter :: F02 = "('(atm_comp_nuopc) ',a,l6)"
+    character(len=*),parameter :: subname='(cdeps_datm_comp_nuopc):(InitializeAdvertise) '


can this not just be:

character(len=*),parameter :: subname= trim(modName) // ':(InitializeAdvertise) '

and so on for the rest?

mvertens · 2021-06-03T17:45:56Z

@uturuncoglu - can you please run this by @jedwards4b as to what other changes need to be made for this to be compatible with cesm. It would be great to be using the same code in both systems moving forwards.

junwang-noaa · 2021-06-03T17:46:49Z

@uturuncoglu On CIME side, I think your need to update the FRONT models in make files, it needs to use: cdeps_dxxx_comp_nuopc, instead xxx_comp_nuopc. Thanks for testing in CIME.

I checked the CMEPS side and it seems that it is expecting certain names in drivers/cime/esm.F90. I need to discuss possible solution for it with Jim and Mariana before going forward and make the changes because this could effect CESM regression tests. I'll update you about it.

I see this may have conflict in CIME, but it looks to me the CIME config only allows one component for ATM (other components too), using different front model name allows us to use two models for ATM (e.g. turn on both hycom and docn as OCN model in HAFS to cover differrent regions).

uturuncoglu · 2021-06-03T17:48:20Z

@mvertens sure. I'll check with Jim.

jedwards4b · 2021-06-03T17:57:15Z

The name of these interface modules is intentionally the same for a given component and changing them will have serious consequences in cime. I agree with @junwang-noaa point that their may be an application that wants to use for example dlnd and clm or docn and pop at the same time thus requiring a change of this nature, however I think that we need to proceed with caution.

jedwards4b

This change will not work in cime without several other coordinated changes.

junwang-noaa · 2021-06-03T18:06:40Z

@uturuncoglu My understanding is that we do need both HYCOM and docn turned on in HAFS, please correct me if this is not the case.

DeniseWorthen · 2021-06-03T18:09:42Z

@DeniseWorthen That would be great, could you access to following directory /work/noaa/gmtb/tufuk/RUN/AtmOcnWav

@uturuncoglu I not sure everything transferred (there are a lot of sym-links in this directory) but you can check here:

/glade/scratch/worthen/AtmOcnWav

mvertens · 2021-06-03T18:34:05Z

I think having the capability to run a data ocn along with a prognostic ocean is something we will want in the future. So coming up with an implementation that permits this would be very useful.

…

On Thu, Jun 3, 2021 at 12:09 PM Denise Worthen ***@***.***> wrote: @DeniseWorthen <https://github.com/DeniseWorthen> That would be great, could you access to following directory /work/noaa/gmtb/tufuk/RUN/AtmOcnWav @uturuncoglu <https://github.com/uturuncoglu> I not sure everything transferred (there are a lot of sym-links in this directory) but you can check here: /glade/scratch/worthen/AtmOcnWav — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#31 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB4XCEYPMHD4MUNIPBMLFCLTQ7APNANCNFSM46BFC45A> .

-- Mariana Vertenstein CESM Software Engineering Group Head National Center for Atmospheric Research Boulder, Colorado Office 303-497-1349 Email: ***@***.***

uturuncoglu · 2021-06-03T18:59:22Z

@uturuncoglu My understanding is that we do need both HYCOM and docn turned on in HAFS, please correct me if this is not the case.

Yes, but this is just a case. The solution needs to be generic as much as possible to cover other data components

mvertens · 2021-06-03T19:03:15Z

Absolutely. I completely agree. But it deserves careful planning a design.

…

On Thu, Jun 3, 2021 at 12:59 PM Ufuk Turunçoğlu ***@***.***> wrote: @uturuncoglu <https://github.com/uturuncoglu> My understanding is that we do need both HYCOM and docn turned on in HAFS, please correct me if this is not the case. Yes, but this is just a case. The solution needs to be generic as much as possible to cover other data components — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#31 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB4XCE22K5RHASX56OKP34TTQ7GJTANCNFSM46BFC45A> .

-- Mariana Vertenstein CESM Software Engineering Group Head National Center for Atmospheric Research Boulder, Colorado Office 303-497-1349 Email: ***@***.***

uturuncoglu · 2021-06-03T19:42:11Z

@DeniseWorthen I think that links are not copied. If you don't mind, could you also copy AtmWav directory? Iy could contain the directories like log.* and model output. So, you could skip them.

binli2337 · 2021-06-03T19:51:09Z

@junwang-noaa I have made some test runs on Hera. The code changes are good.

uturuncoglu · 2021-06-03T20:30:02Z

@junwang-noaa it seems that the best way to make it work in the short term is to use CESMCOUPLED CPP flag (we are already using it for PIO initialization) rather than renaming the modules for all the applications. This approach allow us to use new names with UFS but old ones with CESM until all the CESM components are ready for the transition. Please let me know what do you think? As @aerorahul suggested, we could define module as a variable or maybe there could be a macro definition in the CPP region like #define MODULE_NAME cdeps_datm_comp_nuopc. I did not test this approach but it could help to use only one CESMCOUPLED CPP flag at the beginning and end of the file.

jedwards4b · 2021-06-03T20:35:02Z

Can you also consider reducing the redundancy in the module name before we get too far along?
How about cdeps_datm_comp (drop the _nuopc).

DeniseWorthen · 2021-06-03T21:29:42Z

@DeniseWorthen I think that links are not copied. If you don't mind, could you also copy AtmWav directory? Iy could contain the directories like log.* and model output. So, you could skip them.

@uturuncoglu I put the AtmWav directory in /glade/scratch/worthen/ also.

junwang-noaa · 2021-06-03T21:37:59Z

@uturuncoglu Sure, let's use "CESMCOUPLED" since it is already in the code. Otherwise we have to introduce a new CPP flag MODULE_NAM.
@jedwards4b I can reduce the module name to: cdeps_cxxx_comp without "_nuopc"

uturuncoglu · 2021-06-03T21:41:22Z

@DeniseWorthen Thanks. I hope this will bring the remaining files and I could run the model.

junwang-noaa · 2021-06-04T02:42:59Z

@uturuncoglu @jedwards4b @aerorahul The code is updated.

uturuncoglu

I think that there will be a follow-up PR for NEMS and CMEPS. Right? @junwang-noaa Let me test also with CESM first before merging it. I'll update you about it soon.

uturuncoglu · 2021-06-04T16:18:45Z

@jedwards4b i am trying to run tests with updated CDEPS using latest CESM tag and CDEPS is failing. I just wonder if it is related with the changes in this PR or just the version of the CDEPS because it seems that the version used in this PR is ahead of the version used under CESM. Here is the log file /glade/scratch/turuncu/ERS_Ld7_Vnuopc.f19_g17.B1850.cheyenne_intel.allactive-defaultio.20210604_100302_88f2pw/bld/CDEPS.bldlog.210604-100310

jedwards4b · 2021-06-04T16:35:06Z

Hi Ufuk, I believe that this is due to the changes in this PR - look at the error message: Error copying Fortran module "datm/cdeps_datm_comp.mod". Tried "datm/CDEPS_DATM_COMP.mod" and "datm/cdeps_datm_comp.mod". make[2]: *** [datm/CMakeFiles/datm.dir/cdeps_datm_comp.mod.stamp] Error 1

…

On Fri, Jun 4, 2021 at 10:19 AM Ufuk Turunçoğlu ***@***.***> wrote: @jedwards4b <https://github.com/jedwards4b> i am trying to run tests with updated CDEPS using latest CESM tag and CDEPS is failing. I just wonder if it is related with the changes in this PR or just the version of the CDEPS because it seems that the version used in this PR is ahead of the version used under CESM. Here is the log file /glade/scratch/turuncu/ERS_Ld7_Vnuopc.f19_g17.B1850.cheyenne_intel.allactive-defaultio.20210604_100302_88f2pw/bld/CDEPS.bldlog.210604-100310 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#31 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABOXUGALMOUEBN2CYFHSAUDTRD4HLANCNFSM46BFC45A> .

-- Jim Edwards CESM Software Engineer National Center for Atmospheric Research Boulder, CO

uturuncoglu · 2021-06-04T16:50:58Z

@jedwards4b I could see -DCESMCOUPLED in the build log. I am not sure why it is not working.

junwang-noaa · 2021-06-04T16:51:03Z

@uturuncoglu Do you have "CESMCOUPLED" defined in CIME? cdeps_datm_comp will show up only when the CESMCOUPLED is undefined.

junwang-noaa · 2021-06-04T16:56:00Z

@uturuncoglu Also the HAFS associated ufs-weather-model PR#611 and PR#615 are planned with a commit date 6/8 if all the tests pass. If you need more time for testing, please let us know, we have to push them to the week of 6/14 as we have code commit tutorial scheduled after 6/8.

uturuncoglu · 2021-06-04T17:03:08Z

@junwang-noaa we are looking into the issue. I'll let you know about it.

jedwards4b · 2021-06-04T17:29:41Z

cmake is creating a dependency file that includes cdeps_datm_comp.mod even when CESMCOUPLED is defined.

uturuncoglu · 2021-06-04T22:13:42Z

@junwang-noaa we are still looking for the solution of the issue in the CESM side. @jedwards4b will look at it more detailed next week. So, please hold this PR until we have more information or solve the issue.

junwang-noaa · 2021-06-05T21:55:11Z

OK. Please let us know when the issue is resolved.

jedwards4b · 2021-06-07T14:50:45Z

@junwang-noaa @uturuncoglu I have a fix and have opened a PR to Jun's fork.
junwang-noaa#1

uturuncoglu · 2021-06-07T15:27:28Z

@jedwards4b that is great. Do you want me to run CESM regression tests with the combined PR?

jedwards4b · 2021-06-07T15:31:15Z

sure

…

On Mon, Jun 7, 2021 at 9:27 AM Ufuk Turunçoğlu ***@***.***> wrote: @jedwards4b <https://github.com/jedwards4b> that is great. Do you want me to run CESM regression tests with the combined PR? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#31 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABOXUGCAN2IZGUHSHIQA2KLTRTQPPANCNFSM46BFC45A> .

-- Jim Edwards CESM Software Engineer National Center for Atmospheric Research Boulder, CO

uturuncoglu · 2021-06-08T15:29:29Z

@junwang-noaa the issue in CESM side is fixed after @jedwards4b PR. So, if you merge his PR (junwang-noaa#1) to this branch. This changes will be fine. BTW, do you have any time estimate for other changes in the build to reflect the changes in CDEPS. This is critical for me to continue to work on HAFS side.

add cdeps prefix to the cdeps data component module names

junwang-noaa · 2021-06-08T15:45:59Z

@uturuncoglu we have timing log for the compile jobs in the regression test. I can check the total time change when compiling datm_cdeps_control_cfsr test with this branch and when compiling the same test from develop branch. Please let me know if that is what you need.

uturuncoglu · 2021-06-08T15:50:33Z

@junwang-noaa Sorry for confusion. Actually, I am not asking the run time differences. I mean this change requires follow-up PRs in build, CMEPS and also NEMS. Right? So, do you have any estimate to finish those PRs. It is not too urgent but it would be nice to know to plan the work on HAFS side.

binli2337 · 2021-06-08T19:42:12Z

@junwang-noaa @uturuncoglu I will merge this PR to the develop branch if there are no additional updates. Thanks!

uturuncoglu · 2021-06-08T20:02:07Z

@binli2337 that is fine for me. Thanks.

junwang-noaa · 2021-06-08T23:41:17Z

Bin, you can commit it. We will need Rahul PR#611 when updating CDEPS in UFS. To Ufuk's question, since we missed the commit time with PR#600 (which is committed today). We have to reschedule the UFS PR#611, possibly the week after next week. Also I actually expect HAFS group to submit PRs for NEMS or CMEPS as the changes can only be tested in HAFS application when both docn and hycom are used. If we have those PR ready, we can schedule the commit.

…

On Tue, Jun 8, 2021 at 4:02 PM Ufuk Turunçoğlu ***@***.***> wrote: @binli2337 <https://github.com/binli2337> that is fine for me. Thanks. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#31 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AI7D6TLFFU4G7KEGSW2LZYLTRZZM7ANCNFSM46BFC45A> .

uturuncoglu · 2021-06-09T15:27:02Z

@junwang-noaa Sure, i'll prepare the required follow-up PR/s and let you know.

add cdeps prefix to the cdeps data component module names

1c2bdb6

junwang-noaa requested review from aerorahul, binli2337 and uturuncoglu June 3, 2021 17:02

aerorahul approved these changes Jun 3, 2021

View reviewed changes

jedwards4b suggested changes Jun 3, 2021

View reviewed changes

binli2337 approved these changes Jun 3, 2021

View reviewed changes

junwang-noaa added 2 commits June 4, 2021 02:35

use CESMCOUPLED CPP for CIME and drop _nuopc in module names

4e42741

use CESMCOUPLED CPP for CIME and drop _nuopc in lnd module

a686729

jedwards4b approved these changes Jun 4, 2021

View reviewed changes

uturuncoglu approved these changes Jun 4, 2021

View reviewed changes

fix for CESMCOUPLED build

db482bd

Merge pull request #1 from jedwards4b/modulenames

0ef8f81

add cdeps prefix to the cdeps data component module names

binli2337 merged commit 8d1f357 into NOAA-EMC:develop Jun 9, 2021

junwang-noaa mentioned this pull request Jun 10, 2021

Link full CDEPS library at compile time instead of data components. ufs-community/ufs-weather-model#611

Closed

12 tasks

Conversation

junwang-noaa commented Jun 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of changes

Specific notes

Uh oh!

junwang-noaa commented Jun 3, 2021

Uh oh!

uturuncoglu commented Jun 3, 2021

Uh oh!

junwang-noaa commented Jun 3, 2021

Uh oh!

junwang-noaa commented Jun 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

uturuncoglu commented Jun 3, 2021

Uh oh!

DeniseWorthen commented Jun 3, 2021

Uh oh!

uturuncoglu commented Jun 3, 2021

Uh oh!

uturuncoglu commented Jun 3, 2021

Uh oh!

uturuncoglu commented Jun 3, 2021

Uh oh!

aerorahul left a comment

Choose a reason for hiding this comment

Uh oh!

aerorahul Jun 3, 2021

Choose a reason for hiding this comment

Uh oh!

mvertens commented Jun 3, 2021

Uh oh!

junwang-noaa commented Jun 3, 2021

Uh oh!

uturuncoglu commented Jun 3, 2021

Uh oh!

jedwards4b commented Jun 3, 2021

Uh oh!

jedwards4b left a comment

Choose a reason for hiding this comment

Uh oh!

junwang-noaa commented Jun 3, 2021

Uh oh!

DeniseWorthen commented Jun 3, 2021

Uh oh!

mvertens commented Jun 3, 2021 via email

Uh oh!

uturuncoglu commented Jun 3, 2021

Uh oh!

mvertens commented Jun 3, 2021 via email

Uh oh!

uturuncoglu commented Jun 3, 2021

Uh oh!

binli2337 commented Jun 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

uturuncoglu commented Jun 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jedwards4b commented Jun 3, 2021

Uh oh!

DeniseWorthen commented Jun 3, 2021

Uh oh!

junwang-noaa commented Jun 3, 2021

Uh oh!

uturuncoglu commented Jun 3, 2021

Uh oh!

junwang-noaa commented Jun 4, 2021

Uh oh!

uturuncoglu left a comment

Choose a reason for hiding this comment

Uh oh!

uturuncoglu commented Jun 4, 2021

Uh oh!

jedwards4b commented Jun 4, 2021 via email

Uh oh!

uturuncoglu commented Jun 4, 2021

Uh oh!

junwang-noaa commented Jun 4, 2021

junwang-noaa commented Jun 3, 2021 •

edited

Loading

junwang-noaa commented Jun 3, 2021 •

edited

Loading

binli2337 commented Jun 3, 2021 •

edited

Loading

uturuncoglu commented Jun 3, 2021 •

edited

Loading