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

Clean up run_cmake_test #699

Merged

Conversation

JessicaMeixner-NOAA
Copy link
Collaborator

@JessicaMeixner-NOAA JessicaMeixner-NOAA commented May 3, 2022

Pull Request Summary

Provides clean up and performance improvements for regression tests with cmake

Description

This PR:

  • cleans up the options in run_cmake_test to remove unused options
  • ensures that we are not building a MPI/SHRD version of the code unless we actually are using a mpi or omp option (essentially were double building before)
  • Optionally uses a "path_build_root" in run_cmake test which allows for multiple build directories if running multiple regression tests and allows for re-use of build directories.
  • Fixes the timing -T option in the run_cmake_test to properly count compile time which was broken

Please also include the following information:

  • Add any suggestions for a reviewer @ukmo-ccbunney @mickaelaccensi @thesser1 (Please tests your various matrix_cmake_* submit scripts)
  • Mention any labels that should be added: enhancement
  • Are answer changes expected from this PR? no

Issue(s) addressed

Commit Message

Enhancement and clean-up of run_cmake_test

Check list

Testing

  • How were these changes tested? matrix_cmake_test
  • Are the changes covered by regression tests? Yes
  • Have the matrix regression tests been run (if yes, please note HPC and compiler)? Yes, ncep intel
  • Please indicate the expected changes in the regression test output (Note the known list of non-identical tests). -- no expected changes (the list of differences have recently changed though, this gives me the same diffs as comparing two sets or runs from the develop branch):
**********************************************************************
********************* non-identical cases ****************************
**********************************************************************
mww3_test_03/./work_PR2_UQ_MPI_d2                     (15 files differ)
mww3_test_03/./work_PR3_UNO_MPI_d2_c                     (15 files differ)
mww3_test_03/./work_PR3_UNO_MPI_d2                     (15 files differ)
mww3_test_03/./work_PR2_UNO_MPI_d2                     (15 files differ)
mww3_test_03/./work_PR1_MPI_d2                     (17 files differ)
mww3_test_03/./work_PR3_UQ_MPI_d2_c                     (15 files differ)
mww3_test_03/./work_PR3_UQ_MPI_d2                     (15 files differ)
ww3_ta1/./work_UPD0F_U                     (0 files differ)
ww3_tp2.10/./work_MPI_OMPH                     (7 files differ)
ww3_tp2.16/./work_MPI_OMPH                     (4 files differ)
ww3_tp2.17/./work_c                     (1 files differ)
ww3_tp2.17/./work_b                     (1 files differ)
ww3_tp2.17/./work_a                     (1 files differ)
ww3_tp2.6/./work_ST0                     (1 files differ)
ww3_tp2.6/./work_ST4                     (1 files differ)
ww3_tp2.6/./work_pdlib                     (1 files differ)
ww3_ufs1.3/./work_a                     (1 files differ)

@JessicaMeixner-NOAA
Copy link
Collaborator Author

@ukmo-ccbunney I noticed in matrix_cmake_ukmo_cray you are still using w3_setenv, in matrix_cmake_ncep we require an input of the model path. w3_setenv will likely go away with the clean-up of the gnu build, do you want me to add the model dir input to your script here or leave it as is?

@aliabdolali aliabdolali added the enhancement New feature or request label May 5, 2022
Copy link
Collaborator

@mickaelaccensi mickaelaccensi left a comment

Choose a reason for hiding this comment

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

regtests passed

@ukmo-ccbunney
Copy link
Collaborator

@ukmo-ccbunney I noticed in matrix_cmake_ukmo_cray you are still using w3_setenv, in matrix_cmake_ncep we require an input of the model path. w3_setenv will likely go away with the clean-up of the gnu build, do you want me to add the model dir input to your script here or leave it as is?

Yes please. I have made the change locally and am testing your new changes.

@JessicaMeixner-NOAA
Copy link
Collaborator Author

@ukmo-ccbunney I added the update to grab the model path from the arguments. I also added the matrix divider at the end since I noticed that was missing... not sure if you want that added though?

@ukmo-ccbunney
Copy link
Collaborator

@ukmo-ccbunney I added the update to grab the model path from the arguments. I also added the matrix divider at the end since I noticed that was missing... not sure if you want that added though?

Thanks.
I usually run the matrix_divider manually, but I am happy for it to be included in the matrix generation script.
I should have results of regtests very soon.

@aliabdolali
Copy link
Contributor

@JessicaMeixner-NOAA thanks for doing all of this.
@ukmo-ccbunney I am going to test, and do my review, once I have your approval, we can proceed with this PR.

@ukmo-ccbunney
Copy link
Collaborator

@JessicaMeixner-NOAA I notice that in matrix_cmake_ncep you don't explicitly set any compiler anymore. How can you be sure that CMake is picking up the compiler you want (intel, I assume)?

Are you relying on what is in the FC environment variable?

@ukmo-ccbunney
Copy link
Collaborator

Apologies for the delay from my end - I am getting some differences in *_trck.nc and *_spec.nc files across multiple regtetst that I am trying to get to the bottom of (tiny differences in the freq arrays in the netCDF files).

Copy link
Collaborator

@ukmo-ccbunney ukmo-ccbunney left a comment

Choose a reason for hiding this comment

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

OK - after checking two fresh copies of develop and this branch, the mww3_test_02 regtests are now b4b. I have no idea what was happening previously to cause the differences...

Apologies for the delay - all good now!

@aliabdolali
Copy link
Contributor

All tests passed on NOAA hpc with pre-known non-identical cases

**********************************************************************
********************* non-identical cases ****************************
**********************************************************************
mww3_test_03/./work_PR2_UQ_MPI_d2                     (15 files differ)
mww3_test_03/./work_PR2_UNO_MPI_d2                     (15 files differ)
mww3_test_03/./work_PR3_UQ_MPI_d2_c                     (15 files differ)
mww3_test_03/./work_PR1_MPI_d2                     (17 files differ)
mww3_test_03/./work_PR3_UNO_MPI_d2_c                     (17 files differ)
mww3_test_03/./work_PR3_UNO_MPI_d2                     (15 files differ)
mww3_test_03/./work_PR3_UQ_MPI_d2                     (16 files differ)
ww3_ta1/./work_UPD0F_U                     (0 files differ)
ww3_tp2.10/./work_MPI_OMPH                     (6 files differ)
ww3_tp2.16/./work_MPI_OMPH                     (4 files differ)
ww3_ufs1.3/./work_a                     (1 files differ)

matrixCompFull.txt
matrixCompSummary.txt
matrixDiff.txt

@aliabdolali aliabdolali merged commit b00a2f8 into NOAA-EMC:develop May 11, 2022
@JessicaMeixner-NOAA
Copy link
Collaborator Author

@JessicaMeixner-NOAA I notice that in matrix_cmake_ncep you don't explicitly set any compiler anymore. How can you be sure that CMake is picking up the compiler you want (intel, I assume)?

Are you relying on what is in the FC environment variable?

@ukmo-ccbunney we're relying on the modules to pick the compiler.. I believe this then uses the environment variables but then how/where that happens that's a great question...

@JessicaMeixner-NOAA JessicaMeixner-NOAA deleted the feature/builddirforrt branch May 17, 2022 16:47
JessicaMeixner-NOAA added a commit that referenced this pull request May 27, 2022
Enhancement and clean-up of run_cmake_test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
4 participants