ESCOMP / CISM-wrapper

Community Ice Sheet Model wrapper for CESM
http://www.cesm.ucar.edu/models/cesm2.0/land-ice/
Other
3 stars 16 forks source link

Auto-generate io files as part of the build, rather than relying on pre-generated default files #18

Closed billsacks closed 5 years ago

billsacks commented 6 years ago

It's a pain to have to maintain the pre-generated default io files. The CESM build should auto-generate the io files using the python script just like the standalone build does.

One piece to figure out here is how to support SourceMods - e.g., by checking SourceMods for a given vars.def file.

billsacks commented 6 years ago

Looking back at some notes on this, @whlipscomb said (about a year ago) that he didn't think it's important to support case-specific modifications of the vars.def files. This would make this task significantly easier: we don't need to mess with SourceMods for the vars.def files.

billsacks commented 5 years ago

Some old notes on this said that this auto-generation of io files took a long time: about 5 minutes on ORNL's old jaguar machine, according to notes from 2012. I haven't observed this recently (on yellowstone or cheyenne), and I'm not sure why something that takes order a second or so on cheyenne would take so long on other systems, but we should keep an eye on whether this is an issue anywhere.

billsacks commented 5 years ago

Note that resolving this issue will allow the removal of:

billsacks commented 5 years ago

We need to remember to update the documentation (see doc/source/controlling-output.rst)

billsacks commented 5 years ago

Some other notes:

Katetc commented 5 years ago

Branches have been created and changes made to the CMakeLists file in commit c683ce52fc7d , removal of unnecessary files from cism-wrapper in commit 4f714db3ae3 and removal of unnecessary code from cime in commit d4e87d4020 . Right now, the cism.template file is just an empty script, to support versions of cime that still look for the file. The code in my sandbox builds and runs for 5 months with these changes (writing cism history files monthly) without any problems. However, I tested the time required for a re-build and the time only reduced from 178 seconds (from clean) to 158 seconds. So it does seem to be triggering nearly an entirely new full build each time. We can look at using Make to try to avoid this?

billsacks commented 5 years ago

@Katetc thanks a lot for this!

First a side-note: I typically test things like this in a T1850G compset (something like SMS_D_Ly1.f09_g17_gl4.T1850G.cheyenne_intel), for which you should run for one or more years, but it should run in a couple of minutes or less.

As for the rebuild time: Do you want to first confirm that, with master, the rebuild time is much less than the initial build time (so we're not accidentally chasing down a pre-existing problem). Assuming the rebuild time is really from these rebuilt io files (which is likely), my initial thought is: can we do something similar to what's done for genf90 in the PIO and/or CESM build? I'm not sure how that's done, but it seems like that could be a good example of what we want to do here.

Katetc commented 5 years ago

So, testing on master shows a new build as 117.5 seconds long for cism, but only 4.6 seconds long on rebuild. It seems we have created a new problem with this change. :) I looked at the PIO CMake files, and I'm not sure they do avoid a rebuild after calling genf90. It may be that the files generated there are so low in the dependency chain that they don't cause much else to build and can be built every time. I'm not sure though, so I sent an email to Kevin Paul, who helped us build the PIO CMake build system in the first place years ago. I also found this pertinent StackOverflow post: https://stackoverflow.com/questions/4222326/cmake-compiling-generated-files I think I'll have to read through it a few times and try a few different things to see if any of it helps.

billsacks commented 5 years ago

Based on the last paragraph in https://cmake.org/cmake/help/v3.0/command/add_custom_command.html, I think the PIO example works the way we'll want: It sounds, if you don't specify DEPENDS, the command only runs if the output file doesn't yet exist; if you specify DEPENDS (as is done for PIO), then the command will run (a) if the output file doesn't yet exist, or (b) if any of the listed dependencies have changed.

Katetc commented 5 years ago

Just a couple notes about my commit yesterday afternoon. Currently, the classic bash scripts to autogenerate the _io files are still being called in the CMakeLists, but now they are dependent on changes to the input files. The bash scripts had to be tweaked to remove the "rm *" commands, as they are a bit too aggressive and were clobbering other files needed for the build. It would probably be possible to change the bash scripts such that no files are copied at all, and the files needed are just used in their spot in the source tree. Or, maybe, to remove the bash scripts all together and just call the python. But, that's still a "To Do."

I did the most important test this afternoon, and here are build times: Master: cism built in 177.516689 seconds (1st) 4.620249 seconds (2nd) Current: cism built in 171.102016 seconds (1st) cism built in 4.706947 seconds (2nd)

(I'm pretty sure the time for building Master in the comment previously should have read "177.5 seconds" as that is the value I saved from the test. Must have just mistyped it in the comment.)

Katetc commented 5 years ago

Just an update comment. We have revisited this project in the hopes of finishing it up and bringing it into master. I finished the last few details of removing the unneeded pre-generated files and updated the scripts to just use files directly from the source tree rather than copying them over and deleting them afterwards. They are committed to my branch now.

Then, we started testing with Python 3. From the release of CESM 2.1 and going forward, all python scripts must work with both python 2 and 3. These scripts were written with python 2. So I've been working all day to update them to support python 3. I made a temporary commit to my branch to save off this work at the end of the day.

We are having some trouble with the config parser, because in the newer version of python, in-line linking (referred to as interpolation) is allowed and uses % signs. We use %signs in our vars_def files. We can get around this in python 3 by using "raw=true" in the parsing, but not sure how to make that backwards compatible.

My most recent test is failing because of the error: configparser.DuplicateOptionError: While reading from '/home/katec/lndice/cism-wrapper-sandbox/source_cism/libglad/glad_vars.def' [line 24]: option 'standard_name' in section 'lat' already exists. So, another place were the actual vars_def files are conflicting with the configparser. Hmmmm.

billsacks commented 5 years ago

From a quick look, it looks like the python2 ConfigParser also supports raw=True (https://docs.python.org/2/library/configparser.html#configparser-objects) – and, alternatively, that python3 still supports RawConfigParser as a "legacy" option (https://docs.python.org/3/library/configparser.html#configparser.RawConfigParser) – though I didn't read through this closely, so may be missing something.

Regarding the most recent failure: that one looks like a typo: I see the following in glad_vars.def:

[lat]
dimensions:    time, y1, x1
units:         degreeN
long_name:     latitude
standard_name: latitude
data:          data%lat
load:          1
standard_name: latitude

So I would just remove one reference to standard_name for that variable.

billsacks commented 5 years ago

@Katetc - a couple of early-morning thoughts:

(1) Maybe you already realized this, but in terms of comparing the generated files against baselines, you can use the *.F90.default files that existed before you removed them. I think the only thing that should differ is the date/time stamp at the top of the file.

(2) If the only thing you end up needing six for is the ConfigParser, then it's probably easiest not to bother with six, instead just doing the rename manually. e.g., I found this in a python template that Ben Andre put together:

if sys.version_info[0] == 2:
    from ConfigParser import SafeConfigParser as config_parser
else:
    from configparser import ConfigParser as config_parser
billsacks commented 5 years ago

@Katetc I have done the necessary documentation update and pushed to master. (Hopefully nobody gets tripped up by the documentation that will be out of date until this branch is merged, but I don't get the sense that anybody has ever read that section of the documentation, so I'm not too worried :-)

Katetc commented 5 years ago

This issue was completed with the merge in 321e8d22089 and in cism-wrapper tag cism2_1_65. See PR #22