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

Compile with DEBUG=Y must add "--debug" to ccpp_prebuild.py #135

Closed
climbfuji opened this issue May 28, 2020 · 17 comments · Fixed by #297
Closed

Compile with DEBUG=Y must add "--debug" to ccpp_prebuild.py #135

climbfuji opened this issue May 28, 2020 · 17 comments · Fixed by #297

Comments

@climbfuji
Copy link
Collaborator

This used to be the case, however, it doesn't work any longer.

./compile_cmake.sh $PWD/.. hera.intel 'CCPP=Y DEBUG=Y'

does not add --debug to the ccpp_prebuild.py command.

@junwang-noaa
Copy link
Collaborator

Dom, is this still an issue?

@climbfuji
Copy link
Collaborator Author

climbfuji commented Oct 22, 2020

Yes, it is. Now in fv3atm, since the call to ccpp_prebuild.py has been moved into this repository unfortunately (major hiccup for many users who are accustomed calling it from the top-level directory).

  message("Calling CCPP code generator (ccpp_prebuild.py) ... ${_ccpp_suites_arg}")
  execute_process(COMMAND ${Python_EXECUTABLE}
                          "ccpp/framework/scripts/ccpp_prebuild.py"
                          "--config=ccpp/config/ccpp_prebuild_config.py"
                          "--builddir=${CMAKE_CURRENT_BINARY_DIR}" ${_ccpp_suites_arg}
                  WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
                  OUTPUT_FILE ${CMAKE_CURRENT_BINARY_DIR}/ccpp_prebuild.out
                  ERROR_FILE  ${CMAKE_CURRENT_BINARY_DIR}/ccpp_prebuild.err
                  RESULT_VARIABLE RC)

There is no logic to add --debug if the code is compiled in DEBUG mode. Further, in issue NOAA-EMC/fv3atm#187 I mentioned that the message line needs to be reformatted.

In my opinion, it would be best if we could call ccpp_prebuild.py from the model's top-level directory rather than inside FV3 to avoid user churn and updating a whole lot of documentation. Essentially every tutorial we have been given in the past two years tell people to run it from ufs-weather-model, which is no longer possible.

@climbfuji
Copy link
Collaborator Author

Yes, it is. Now in fv3atm, since the call to ccpp_prebuild.py has been moved into this repository unfortunately (major hiccup for many users who are accustomed calling it from the top-level directory).

  message("Calling CCPP code generator (ccpp_prebuild.py) ... ${_ccpp_suites_arg}")
  execute_process(COMMAND ${Python_EXECUTABLE}
                          "ccpp/framework/scripts/ccpp_prebuild.py"
                          "--config=ccpp/config/ccpp_prebuild_config.py"
                          "--builddir=${CMAKE_CURRENT_BINARY_DIR}" ${_ccpp_suites_arg}
                  WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
                  OUTPUT_FILE ${CMAKE_CURRENT_BINARY_DIR}/ccpp_prebuild.out
                  ERROR_FILE  ${CMAKE_CURRENT_BINARY_DIR}/ccpp_prebuild.err
                  RESULT_VARIABLE RC)

There is no logic to add --debug if the code is compiled in DEBUG mode. Further, in issue NOAA-EMC/fv3atm#187 I mentioned that the message line needs to be reformatted.

In my opinion, it would be best if we could call ccpp_prebuild.py from the model's top-level directory rather than inside FV3 to avoid user churn and updating a whole lot of documentation. Essentially every tutorial we have been given in the past two years tell people to run it from ufs-weather-model, which is no longer possible.

@junwang-noaa @aerorahul @DusanJovic-NOAA I just discussed with the other CCPP developers. We feel that ccpp_prebuild.py should still be called (as a command) from the top-level directory, not from inside FV3. This is to ensure consistency with existing documentation, tutorials etc.

The logic to trigger the call can still sit in the fv3atm CMakeLists.txt, not a problem. The changes would look like this (FV3/CMakeLists.txt), which also address this issue here and #187:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 320a158..db51e2c 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -1,14 +1,19 @@
 if(CCPP)

+  if(DEBUG)
+    set(_ccpp_debug_arg "--debug")
+  endif()
   if(DEFINED CCPP_SUITES)
     set(_ccpp_suites_arg "--suites=${CCPP_SUITES}")
+    message("Calling CCPP code generator (ccpp_prebuild.py) for suites ${_ccpp_suites_arg} ...")
+  else()
+    message("Calling CCPP code generator (ccpp_prebuild.py) for all available suites ...")
   endif()
-  message("Calling CCPP code generator (ccpp_prebuild.py) ... ${_ccpp_suites_arg}")
   execute_process(COMMAND ${Python_EXECUTABLE}
-                          "ccpp/framework/scripts/ccpp_prebuild.py"
-                          "--config=ccpp/config/ccpp_prebuild_config.py"
-                          "--builddir=${CMAKE_CURRENT_BINARY_DIR}" ${_ccpp_suites_arg}
-                  WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
+                          "FV3/ccpp/framework/scripts/ccpp_prebuild.py"
+                          "--config=FV3/ccpp/config/ccpp_prebuild_config.py"
+                          "--builddir=${CMAKE_CURRENT_BINARY_DIR}" ${_ccpp_suites_arg} ${_ccpp_debug_arg}
+                  WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/..
                   OUTPUT_FILE ${CMAKE_CURRENT_BINARY_DIR}/ccpp_prebuild.out
                   ERROR_FILE  ${CMAKE_CURRENT_BINARY_DIR}/ccpp_prebuild.err
                   RESULT_VARIABLE RC)

Of course we'll have to add back in the prefix FV3/ in ccpp_prebuild_config.py. This file is, by the way, currently broken for in-source gnumake builds when the argument --builddir is not given as far as I can see because the default build dir still has FV3:

# Default build dir, relative to current working directory,
# if not specified as command-line argument
DEFAULT_BUILD_DIR = 'FV3'

@aerorahul
Copy link
Contributor

Why can't we update the documentation and keep the code where it actually belongs.

The --debug not being added is not a because of moving the call one level down. That issue existed before this move.

@climbfuji
Copy link
Collaborator Author

Why can't we update the documentation and keep the code where it actually belongs.

The --debug not being added is not a because of moving the call one level down. That issue existed before this move.

None of the "code" is moved elsewhere, it's just called from one directory up. I guess you underestimate how many presentations and documents are out there that tell developers that for debugging they need to do

./FV3/ccpp/framework/scripts/ccpp_prebuild.py --config=FV3/ccpp/config/ccpp_prebuild_config.py ...

@climbfuji
Copy link
Collaborator Author

Why can't we update the documentation and keep the code where it actually belongs.
The --debug not being added is not a because of moving the call one level down. That issue existed before this move.

None of the "code" is moved elsewhere, it's just called from one directory up. I guess you underestimate how many presentations and documents are out there that tell developers that for debugging they need to do

./FV3/ccpp/framework/scripts/ccpp_prebuild.py --config=FV3/ccpp/config/ccpp_prebuild_config.py ...

I am wondering how/when we can make this change. Not before the completion of the ufs-s2s and ufs-weather merge, certainly. I suggest the first PR afterwards for the combined repository, combined with Moorthi's change in ccpp-physics to establish the GFSv16 comparison. That will push out the HWRF PR by one, but I think that is ok, this will be a quick one.

@junwang-noaa
Copy link
Collaborator

junwang-noaa commented Oct 23, 2020 via email

@climbfuji
Copy link
Collaborator Author

Dom, from the code structure level, I don't think it's proper to put CCPP under ufs-weather-model along with atmosphere, ocean, ice, ww3 model rather than under FV3 atmosphere directory. I do understand you have history documentation. But the current model needs an updated documentation to go along with it, in other words, CCPP documentation needs to have a version associated with it so that people who use previous version can use the version of documentation working with the corresponding repo, and the latest code can have newer version of documentation associated with the code. In long term would this be a better solution than using the component documentation to decide the application code structure?

We have one comprehensive documentation called the CCPP Technical Documentation. This needs to refer to host models as well as internals of the CCPP. It's in the ccpp-doc github repo and visible here: https://ccpp-techdoc.readthedocs.io/en/latest/ It is not practical to split this documentation up and maintain fragments in various repositories. As for this specific change, I don't understand what the problem is. CCPP sits under fv3atm, ccpp_prebuild.py is triggered by fv3atm's CMakeLists.txt, the only thing I am doing is add a /.. to the working directory for the ccpp_prebuild.py call and prepend the path to the script and config with FV3. This has absolutely nothing to do with where the code is hosted or how the logic of the build is controlled.

@junwang-noaa
Copy link
Collaborator

junwang-noaa commented Oct 23, 2020 via email

@climbfuji
Copy link
Collaborator Author

climbfuji commented Oct 23, 2020 via email

@aerorahul
Copy link
Contributor

Is the upcoming UFS tutorial based on develop or a release tag?

@climbfuji
Copy link
Collaborator Author

climbfuji commented Oct 23, 2020 via email

@climbfuji
Copy link
Collaborator Author

Just to be clear: if I had paid better attention (and would have had more time) I would not have let this change to call ccpp_prebuild.py from within FV3 get past the code review for all of the reasons above. I am simply fixing the mistake of having overlooked the detail in the large PR that included the change.

@aerorahul
Copy link
Contributor

I have to say this is a first: revert code to keep up with existing documentation.

@climbfuji
Copy link
Collaborator Author

I have to say this is a first: revert code to keep up with existing documentation.

There is always a first by definition ;-)

@aerorahul
Copy link
Contributor

I have to say this is a first: revert code to keep up with existing documentation.

There is always a first by definition ;-)

That was not a complement.

@DomHeinzeller
Copy link
Contributor

This issue has been sitting here for some time, and in the meanwhile we had the UFS MRW tutorial during which we had to teach the participants how to run ccpp_prebuild.py manually based on what is in develop now. We are therefore going to change the CCPP technical documentation (see NCAR/ccpp-doc#31) and back off from reverting the directory from which ccpp_prebuild.py is called. The remainder of this issue, however, is still valid and will be addressed in one of my coming PRs:

(1) add --debug to the ccpp_prebuild.py call if the model is compiled in DEBUG mode
(2) pretty-print the output of how ccpp_prebuild.py is called in the build log

This should make @aerorahul happy ;-)

junwang-noaa pushed a commit to NOAA-EMC/fv3atm that referenced this issue Nov 25, 2020
…, updates to other GSL physics (#202)

This PR updates the GSL physics and does a few other things:

*add --debug to ccpp_prebuild.py call when model is compiled in DEBUG mode, and pretty print the output in the build log (fixes #187 and ufs-community/ufs-weather-model#135)
*add unified gravity wave drag (called unified ugwp) that combines the UGWP v1 with the GSL drag suite
*add suite definition file for GFS v16beta for coupled model
*move static array zs (depth of soil layer), dimensioned as 1:lsoil_lsm from Sfcprop to Model (since this is a model configuration and only needs to be stored once, not for each block)

Co-authored-by: DomHeinzeller <58610420+DomHeinzeller@users.noreply.github.com>
Co-authored-by: samuel.trahan <Samuel.Trahan@noaa.gov>
Co-authored-by: hannah barnes <hannah.barnes@noaa.gov>
Co-authored-by: tanyasmirnova <tanya.smirnova@noaa.gov>
Co-authored-by: Joseph Olson <Joseph.B.Olson@noaa.gov>
Co-authored-by: Michael Toy <michael.toy@noaa.gov>
epic-cicd-jenkins pushed a commit that referenced this issue Apr 17, 2023
- Update Externals.cfg to point to hash of UFS_UTILS with bug fix
- Update Quickstart documentation accordingly

Out-of-the-box case builds with GNU and workflow runs to completion on Cheyenne.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants