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.

Sign up for GitHub

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

Jump to bottom

Fully coupled test case (cpld_bmarkfrac_wave_v16) not thread-safe #367

Closed
JessicaMeixner-NOAA opened this issue Jan 13, 2021 · 28 comments
Closed

Fully coupled test case (cpld_bmarkfrac_wave_v16) not thread-safe #367

JessicaMeixner-NOAA opened this issue Jan 13, 2021 · 28 comments
Labels
bug Something isn't working

Comments

@JessicaMeixner-NOAA
Copy link
Collaborator

Description

It has been reported by @SMoorthi-emc and @shansun6 that when running cpld_bmarkfrac_wave_v16 test with and without threads do not reproduce each other.

To Reproduce:

Run cpld_bmarkfrac_wave_v16 and then run cpld_bmarkfrac_wave_v16 with multiple threads

Additional context

Going to try to run with different aspects of wave coupling turned on or off (ie, no wave feedback to ocean) to narrow down area to look for what part of the code is not thread safe. Will update when have more information.

@SMoorthi-emc to turn off OMP parts of WW3, remove OMPG OMPH from WW3/model/esmf/switch
@JessicaMeixner-NOAA JessicaMeixner-NOAA added the bug Something isn't working label Jan 13, 2021
@JessicaMeixner-NOAA
Copy link
Collaborator Author

I have so far narrowed this down to wav->atm coupling as the fully coupled test with the feedback from the wave to atmosphere turned off reproduced itself with 1 and 2 threads. A simpler test, with just wave-atm coupling: fv3_ccpp_gfdlmprad_atmwav is being used for debugging now. I'll post more updates when I have them.

@arunchawla-NOAA
Copy link

@JamesAbeles-NOAA do you have some time to help Jessica narrow this down? The wave model is not thread safe and you helped the waves team track this down for GFS v16 when it was one way coupled. This has re surfaced for two way coupled and our hunch is this is more sections of the code that have some bugs in openmp similar to what you found earlier

@JamesAbeles-NOAA
Copy link
Collaborator

I can help if @JessicaMeixner-NOAA points me to a tarfile that has the code and run directory. Hopefully, I can do the testing on Hera as I don't get much through WCOSS queues

@JessicaMeixner-NOAA
Copy link
Collaborator Author

Is a pointer for what to git clone acceptable instead of a tar file?

@JamesAbeles-NOAA
Copy link
Collaborator

@JessicaMeixner-NOAA Sure, instructions are fine

@JessicaMeixner-NOAA
Copy link
Collaborator Author

Clone the repo and branch:

git clone https://github.com/JessicaMeixner-NOAA/ufs-weather-model 
cd ufs-weather-model 
git checkout bug/threadatmwav 
git submodule update --init --recursive

This is the develop branch as of today (6a4c307) but has the updates to just run the test needed and added a identical test but with 2 threads.

To run the test:

cd tests 
./rt.sh -ke

This runs two tests for wav-atm 2-way coupling, 1 with 2 threads another with 1 thread. You'll get two output directories in: /scratch1/NCEPDEV/stmp2/$user/FV3_RT/rt_*/ and comparing the netcdf output of the atm model they are different. The original test used nemsio output but I thought netcdf would be easier to debug with netcdf output so I switched it.

For example I have output in:
/scratch1/NCEPDEV/stmp2/Jessica.Meixner/FV3_RT/rt_173317/fv3_ccpp_gfdlmprad_atmwav_prod
/scratch1/NCEPDEV/stmp2/Jessica.Meixner/FV3_RT/rt_173317/fv3_ccpp_gfdlmprad_atmwav2_prod

and you will see that for example the phyf002.tile6.nc tiles have differences.

@JamesAbeles-NOAA Let me know if you'd prefer the test set up differently.

I suspect the issue is in WW3/model/ftn/wmesmfmd.ftn in the call from SetExport to CalcRoughl subroutine because the passing of z0 roughness length from wave model to the atm model is what is different from the one-way coupled wav->atm case which is thread safe.

@JessicaMeixner-NOAA
Copy link
Collaborator Author

So removing the FLD2 switch, we regain thread-safe for the 2-way coupling test, which likely means it's something going on in https://github.com/NOAA-EMC/WW3/blob/develop/model/ftn/w3fld2md.ftn (likewise a similar issue exists when using FLD1 instead of FLD2, so there's likely the same bug in w3fld1md.ftn).

@arunchawla-NOAA
Copy link

arunchawla-NOAA commented Feb 1, 2021 via email

@JamesAbeles-NOAA
Copy link
Collaborator

I think we should comment out the openMP for the loop that has "CALL W3SRCE" to verify it is this loop that is causing the problem. Once verified we can decide how to best find the problem. Subroutines called in a threaded region often are the hardest to debug.

@JamesAbeles-NOAA
Copy link
Collaborator

OK, I did what I suggested (removed openMP on the loop) and for the two rt tests Jessica had me run, now the phy002.tile6.nc files are identical
nccmp -dm fv3_ccpp_gfdlmprad_atmwav_prod/phyf002.tile6.nc fv3_ccpp_gfdlmprad_atmwav2_prod/phyf002.tile6.nc

@JessicaMeixner-NOAA
Copy link
Collaborator Author

Thanks @JamesAbeles-NOAA that's good information to have. In terms of debugging having threadding that high up is obviously makes it difficult. In terms of performance of threading, is it best to have the threading as high up in loops/subroutines as possible, or would it be better to have them in the inner-most loops, subroutines, etc?

I have not been able to get a WW3 only small scale test to consistently give differences for a smaller test case to isolate this issue, but I will keep trying. I am also trying to turn on debug flags one-by-one to turn on the debugging options and looking for uninitialized variables in w3fld(1/2)md.ftn but haven't found anything yet.

@JamesAbeles-NOAA
Copy link
Collaborator

The higher up the better since threading has to start/stop regions, so one large region is good. SO on the wW3 only small scale test, the answers replicate when the loop is threaded and the FLD2 switch is on?

@JamesAbeles-NOAA
Copy link
Collaborator

I wanted to see what the OpenMP standard said about saved variables in threaded regions. I found this, so I need to see if this will affect us
Local variables with the SAVE attribute declared in procedures called from a parallel region are implicitly SHARED. The specification will be clarified.

@JessicaMeixner-NOAA
Copy link
Collaborator Author

The test I was trying would replicate itself with threads every other time you would run, so it was not consistently failing.

We have saved variables in the fld1/2... that could be the issue?

@JamesAbeles-NOAA
Copy link
Collaborator

Well, there are some in w3srce, so I wonder about them. I am surprised this is not an issue with the code already, so maybe this is a red herring. I see these definitions in w3srce
REAL, SAVE :: TAUNUX, TAUNUY
LOGICAL, SAVE :: FLTEST = .FALSE., FLAGNN = .TRUE.

I'm grasping at at straws here, but the variable declarations in the OMP loop look correct

@JamesAbeles-NOAA
Copy link
Collaborator

I added this to w3srcemd.ftn to see if it helps
!/OMPG/!$omp threadprivate( TAUNUX, TAUNUY)
If that works, then we'll have to see how to do this properly

@JamesAbeles-NOAA
Copy link
Collaborator

It worked!

@JessicaMeixner-NOAA
Copy link
Collaborator Author

Yay!!! Thank you @JamesAbeles-NOAA !!!

@JessicaMeixner-NOAA
Copy link
Collaborator Author

I confirmed that @JamesAbeles-NOAA fix fixed the issue in the wave-atm coupling case. I'm now running this on orion to make sure it's working on multiple machines and going to run the fully-coupled test which is what originally was not working. I'll update the issue here when those tests are done and start the process for pushing the updates back to WW3 and to ufs-weather-model after that.

@JessicaMeixner-NOAA
Copy link
Collaborator Author

The fully coupled test case is now thread-safe as well. If someone needs the WW3 update before it makes it way back into develop here's the commit: JessicaMeixner-NOAA/WW3@2c0d3f6

Also, since this is thread safe, assuming it's okay I'll add waves to the cpld threading test to make sure we keep it that way when I push the update for WW3 back.

@JessicaMeixner-NOAA
Copy link
Collaborator Author

Oh the wave-atm case also worked with the fix on orion, so it's been tested on at least 2 platforms.

@SMoorthi-emc
Copy link
Contributor

SMoorthi-emc commented Feb 4, 2021 via email

@JamesAbeles-NOAA
Copy link
Collaborator

@SMoorthi-emc I can't answer the first part to the question, but any variables declared with the save attribute that are in routines inside a threaded region are shared by default.

@SMoorthi-emc
Copy link
Contributor

SMoorthi-emc commented Feb 4, 2021 via email

@JamesAbeles-NOAA
Copy link
Collaborator

If they don't need to be saved, then they would be private and no need for the omp directive

@JessicaMeixner-NOAA JessicaMeixner-NOAA mentioned this issue Feb 4, 2021
Closed
@JessicaMeixner-NOAA
Copy link
Collaborator Author

@SMoorthi-emc the variables are supposed to be saved to be a first guess of an iterative solve, however in looking at the code I agree this might not be needed. I made an issue on WW3 with your point: NOAA-EMC/WW3#308 and regardless of if this needs to be saved or not, we will also look in all of WW3 for other variables that should be saved as thread private (NOAA-EMC/WW3#307) to make sure other parts of the code are okay.

@SMoorthi-emc
Copy link
Contributor

SMoorthi-emc commented Feb 4, 2021 via email

@JessicaMeixner-NOAA JessicaMeixner-NOAA mentioned this issue Feb 23, 2021
Closed
@DeniseWorthen DeniseWorthen mentioned this issue Mar 15, 2021
Merged
9 tasks
@junwang-noaa
Copy link
Collaborator

The code updates were committed, close this issue.

epic-cicd-jenkins pushed a commit that referenced this issue Apr 17, 2023
Add double quotes to sfc_climo_gen namelist variable: (#367)
88060a6 
orog_files_mdl="${CRES}${DOT_OR_USCORE}oro_data.tile${TILE_RGNL}.halo${NH4}.nc"
to run workflow with the UFS_UTILS GNU build.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Update WW3 for porting and threading issues JessicaMeixner-NOAA/ufs-weather-model
5 participants
@JessicaMeixner-NOAA @junwang-noaa @SMoorthi-emc @JamesAbeles-NOAA @arunchawla-NOAA